Migrate to Vite + other updates #345
Conversation
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Something went wrong with PR preview build please check |
|
Something went wrong with PR preview build please check |
|
Something went wrong with PR preview build please check |
|
Preview is available here: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Preview is available here: |
|
Preview is available here: |
|
Preview is available here: |
| app.use( | ||
| express.static(staticDir, { | ||
| // Security options | ||
| dotfiles: "deny", // Don't serve hidden files (.env, .git, etc.) | ||
| index: "index.html", | ||
| maxAge: "1d", // Cache static assets for 1 day | ||
| // Restrict to specific file extensions for security | ||
| extensions: [ | ||
| "html", | ||
| "js", | ||
| "css", | ||
| "png", | ||
| "jpg", | ||
| "jpeg", | ||
| "gif", | ||
| "ico", | ||
| "svg", | ||
| "woff", | ||
| "woff2", | ||
| "ttf", | ||
| "eot", | ||
| ], | ||
| setHeaders: (res, filePath) => { | ||
| // Add security headers | ||
| res.setHeader("X-Content-Type-Options", "nosniff"); | ||
| res.setHeader("X-Frame-Options", "DENY"); | ||
| res.setHeader("X-XSS-Protection", "1; mode=block"); | ||
| res.setHeader("Referrer-Policy", "strict-origin-when-cross-origin"); | ||
|
|
||
| // Set appropriate cache headers based on file type | ||
| if (filePath.endsWith(".html")) { | ||
| res.setHeader("Cache-Control", "no-cache, no-store, must-revalidate"); | ||
| res.setHeader("Pragma", "no-cache"); | ||
| res.setHeader("Expires", "0"); | ||
| } else if ( | ||
| filePath.match( | ||
| /\.(js|css|png|jpg|jpeg|gif|ico|svg|woff|woff2|ttf|eot)$/, | ||
| ) | ||
| ) { | ||
| res.setHeader("Cache-Control", "public, max-age=86400, immutable"); // 1 day with immutable | ||
| } | ||
| }, | ||
| }), | ||
| ); |
Check warning
Code scanning / CodeQL
Exposure of private files Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 17 days ago
In general, the problem is that express.static is configured with "." (the process working directory) in some environments, which can unintentionally expose files outside the intended build output. To fix this, we should never serve "." or any directory derived from process.cwd(); instead, we should always use a known, explicit path to the built static assets, and ensure that this path cannot “expand” to include private files.
The best minimal fix without changing existing behavior is:
- Remove the special-case branch that sets
staticDir = "."in Docker. - Always compute
staticDirfrom__dirnameto the knowndistdirectory (which the rest of the code already expects viaindex.htmlindist). - Optionally allow overriding via an environment variable (e.g.,
STATIC_DIR) if needed, but still resolve it from__dirname(notprocess.cwd()). - Keep all the current security options on
express.static(dotfiles deny, limited extensions, security headers).
Concretely, in backend/routes.ts:
- Replace the conditional block at lines 92–98 with a deterministic resolution of
staticDirto the intended distribution folder, e.g.path.join(__dirname, "..", "..", "dist"). If you want a Docker-specific path, it should still be explicit (e.g."/usr/share/app/dist"), but not".". - Leave the rest of the static-serving configuration (logging, checks for
index.html, headers) unchanged.
No new imports are required; path is already imported, and fs is required locally in the existing code.
| @@ -89,13 +89,8 @@ | ||
| // Determine the correct static directory based on environment | ||
| let staticDir: string; | ||
|
|
||
| if (process.cwd().endsWith("/dist")) { | ||
| // Docker environment: already in dist directory | ||
| staticDir = "."; | ||
| } else { | ||
| // Heroku/other environments: serve from dist directory | ||
| staticDir = path.join(__dirname, "..", "..", "dist"); | ||
| } | ||
| // Always serve static files from the built dist directory, regardless of CWD | ||
| staticDir = path.join(__dirname, "..", "..", "dist"); | ||
|
|
||
| console.log(`Serving static files from: ${path.resolve(staticDir)}`); | ||
|
|
|
Preview is available here: |
|
Something went wrong with PR preview build please check |
|
Preview is available here: |
|
I gave a glance at the preview link before diving into the code. I am noticing that
|
Thanks for checking, @jeesunikim ! We should be comparing to the current version PR preview [PR] and not the production. It doesn't look like 30-day data aggregation works on PR previews. |
| this.state = { | ||
| loading: true, | ||
| chartWidth: 400, | ||
| chartHeigth: this.props.chartHeigth || 120, |
|
Can we update |
|
The latest |
|
Preview is available here: |
@jeesunikim, I think it's OK to use an older Vite version for now, since we have some old dependencies we can't update. This migration is more like a band-aid to unblock Ops. We'll see what the long-term plan for this is. |
|
Thanks for the migration work. The followings are the suggestions that came up as I reviewed this with Claude. However, since this was created to unblock devops team, we can create a separate issue while we figure out what the long term plan for this is:
|
Thanks, @jeesunikim! All of these are good suggestions, but might be out of scope for this migration. I don't think it's meant to be a full refactor, but the quickest/simplest way to upgrade the tech stack. @sagpatil , should we spend more time fixing these things? They shouldn't be regressions; they're existing issues in the current codebase. |
| import BarChart from "react-d3-components/lib/BarChart"; | ||
| import * as d3 from "d3"; | ||
| import D3BarChartNoXLabels from "./D3BarChartNoXLabels.jsx"; | ||
| import clone from "lodash/clone"; |
| this.getLedgers(); | ||
| // Update chart width | ||
| this.updateSize(); | ||
| setInterval(() => this.updateSize(), 5000); |
There was a problem hiding this comment.
let's clean the interval with componentWillUnmount?
| export async function inflationLumens() { | ||
| const [totalLumensValue, originalSupply] = await Promise.all([ | ||
| totalLumens(horizonLiveURL), | ||
| ORIGINAL_SUPPLY_AMOUNT, |
There was a problem hiding this comment.
dear cluade, this is hardcoded, no reason to use Promise.all
| lumensV2V3.v2CirculatingSupplyHandler, | ||
| ); | ||
|
|
||
| app.get("/api/v3/lumens", lumensV2V3.v3Handler); |
There was a problem hiding this comment.
/lumens doesn't need a rate limiter?
| @@ -1,5 +1,5 @@ | |||
| import React from "react"; | |||
| import AmountWidget from "./AmountWidget"; | |||
| import AmountWidget from "./AmountWidget.jsx"; | |||
There was a problem hiding this comment.
It should be just ./AmountWidget. There are five different files that use /AmountWidget.jsx. It should be corrected to just /AmountWidget
| import TotalCoins from "./TotalCoins"; | ||
| import TransactionsChart from "./TransactionsChart"; | ||
| import FailedTransactionsChart from "./FailedTransactionsChart"; | ||
| import AppBar from "./AppBar.jsx"; |
There was a problem hiding this comment.
Those import files don't need .jsx. But we can leave them for now since they aren't critical. I am just flagging.
| import { EventEmitter } from "fbemitter"; | ||
| import axios from "axios"; | ||
| import moment from "moment"; | ||
| import { Server } from "stellar-sdk"; |
There was a problem hiding this comment.
we could do Horizon for better tree shaking
|
@quietbits I approved it! I meant to create a separate issue on this github with the bullets I mentioned once this merged. |
|
since this is a maintenance release, I am hesitant to add new features which might break things and also take a lot of time. At this point as long as the minimum requirements are met, @quietbits we should merge this in. @jeesunikim can you create an issue with your findings to track your suggestions for later. |
|
SGTM. Issue created: #347 |
muicssframework for the UI, and it hasn't been updated in 5 years. Because the whole UI depends on it, we can't update most of the other dependencies.react-d3-componentslibrary doesn't work with newer Node versions; it's no longer maintained.