From 9b19b18ec27b7b606d1740e07a1affcacab4d385 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 9 Jun 2026 09:13:38 +0000 Subject: [PATCH] fix(review): resolve CodeQL path-injection + CSRF introduced by the upload change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first push surfaced two new CodeQL alerts in the newly-added code (the upload sandbox itself passed — its resolve-then-contain sanitizer is recognized): - HIGH js/path-injection at the analyze route: the KTD11 in-route `fs.realpath(repoLocalPath)` / `fs.stat` was a user-controlled filesystem read with no security gain (the worker already reads the path; cross-origin reach is closed by requireLocalhostOrigin). Drop the in-route fs calls; keep only the absolute-path check + the localhost-origin guard. - MEDIUM js/client-side-request-forgery: the new raw `xhr.open` was a fresh request sink. Route the upload through the shared, origin-validated fetchWithTimeout instead (the centralized sink all other calls use). Trades the upload-progress percentage for an indeterminate "Uploading…" state. Co-Authored-By: Claude Opus 4.8 (1M context) --- gitnexus-web/src/components/RepoAnalyzer.tsx | 21 ++++---- gitnexus-web/src/locales/en/onboarding.json | 2 +- .../src/locales/zh-CN/onboarding.json | 2 +- gitnexus-web/src/services/backend-client.ts | 54 ++++++------------- gitnexus/src/server/api.ts | 35 ++++-------- 5 files changed, 37 insertions(+), 77 deletions(-) diff --git a/gitnexus-web/src/components/RepoAnalyzer.tsx b/gitnexus-web/src/components/RepoAnalyzer.tsx index 33b46aeea..42aa97cf4 100644 --- a/gitnexus-web/src/components/RepoAnalyzer.tsx +++ b/gitnexus-web/src/components/RepoAnalyzer.tsx @@ -168,7 +168,7 @@ export const RepoAnalyzer = ({ variant, onComplete, onCancel }: RepoAnalyzerProp const { t } = useTranslation(['common', 'errors', 'onboarding']); const inputId = useId(); const [mode, setMode] = useState('github'); - const [uploadPercent, setUploadPercent] = useState(null); + const [uploading, setUploading] = useState(false); const [uploadSummary, setUploadSummary] = useState<{ count: number; dropped: number } | null>( null, ); @@ -297,14 +297,14 @@ export const RepoAnalyzer = ({ variant, onComplete, onCancel }: RepoAnalyzerProp } setValidationError(null); setUploadSummary({ count: files.length, dropped: droppedCount }); - setUploadPercent(0); + setUploading(true); setPhase('starting'); try { - const { jobId } = await uploadFolder(files, manifest, (pct) => setUploadPercent(pct)); - setUploadPercent(null); + const { jobId } = await uploadFolder(files, manifest); + setUploading(false); trackJob(jobId, null); } catch (err) { - setUploadPercent(null); + setUploading(false); setValidationError(err instanceof Error ? err.message : t('errors:startAnalysisFailed')); setPhase('error'); } @@ -507,20 +507,17 @@ export const RepoAnalyzer = ({ variant, onComplete, onCancel }: RepoAnalyzerProp {t('onboarding:repoAnalyzer.upload.button')} - {uploadPercent !== null && ( + {uploading && (
-
+

- {t('onboarding:repoAnalyzer.upload.uploading', { percent: uploadPercent })} + {t('onboarding:repoAnalyzer.upload.uploading')}

)} - {uploadSummary && uploadPercent === null && phase !== 'error' && ( + {uploadSummary && !uploading && phase !== 'error' && (

{t('onboarding:repoAnalyzer.upload.selected', { count: uploadSummary.count, diff --git a/gitnexus-web/src/locales/en/onboarding.json b/gitnexus-web/src/locales/en/onboarding.json index 9e1742d74..fd746a290 100644 --- a/gitnexus-web/src/locales/en/onboarding.json +++ b/gitnexus-web/src/locales/en/onboarding.json @@ -64,7 +64,7 @@ "hideBackground": "Hide (analysis continues in background)", "upload": { "button": "Upload a folder", - "uploading": "Uploading… {{percent}}%", + "uploading": "Uploading…", "selected": "{{count}} files ready ({{dropped}} skipped: .git, node_modules, build output)", "empty": "No analyzable files found in that folder." } diff --git a/gitnexus-web/src/locales/zh-CN/onboarding.json b/gitnexus-web/src/locales/zh-CN/onboarding.json index a4b16fbb4..4ac2c44d8 100644 --- a/gitnexus-web/src/locales/zh-CN/onboarding.json +++ b/gitnexus-web/src/locales/zh-CN/onboarding.json @@ -64,7 +64,7 @@ "hideBackground": "隐藏（分析继续在后台进行）", "upload": { "button": "上传文件夹", - "uploading": "上传中… {{percent}}%", + "uploading": "上传中…", "selected": "已准备 {{count}} 个文件（已跳过 {{dropped}} 个：.git、node_modules、构建产物）", "empty": "该文件夹中未找到可分析的文件。" } diff --git a/gitnexus-web/src/services/backend-client.ts b/gitnexus-web/src/services/backend-client.ts index f1a28be14..b35fd079c 100644 --- a/gitnexus-web/src/services/backend-client.ts +++ b/gitnexus-web/src/services/backend-client.ts @@ -761,50 +761,26 @@ export const fetchClusterDetail = async (repo: string, name: string): Promise`) and start analysis. * Sends the file blobs plus a JSON `manifest` of their relative paths — the * multipart filename can't carry the path (browsers strip separators), so the - * manifest is the source of truth. Uses XHR for upload progress. Returns the - * analysis jobId, which the caller drives through the normal SSE flow. + * manifest is the source of truth. Routed through fetchWithTimeout (the shared, + * origin-validated request path) rather than a raw XHR; returns the analysis + * jobId, which the caller drives through the normal SSE flow. */ -export const uploadFolder = ( +export const uploadFolder = async ( files: File[], manifest: string[], - onProgress?: (percent: number) => void, ): Promise<{ jobId: string; status: string }> => { - return new Promise((resolve, reject) => { - const form = new FormData(); - // Manifest MUST precede the file parts (the server enforces this). - form.append('manifest', JSON.stringify(manifest)); - for (const f of files) form.append('files', f); + const form = new FormData(); + // Manifest MUST precede the file parts (the server enforces this). + form.append('manifest', JSON.stringify(manifest)); + for (const f of files) form.append('files', f); - const xhr = new XMLHttpRequest(); - xhr.open('POST', `${_backendUrl}/api/analyze/upload`); - xhr.timeout = 5 * 60_000; // up to 5 min for large repos - xhr.upload.onprogress = (e) => { - if (onProgress && e.lengthComputable) { - onProgress(Math.round((e.loaded / e.total) * 100)); - } - }; - xhr.onload = () => { - if (xhr.status >= 200 && xhr.status < 300) { - try { - resolve(JSON.parse(xhr.responseText) as { jobId: string; status: string }); - } catch { - reject(new Error('Invalid server response')); - } - return; - } - let msg = `Upload failed (${xhr.status})`; - try { - const body = JSON.parse(xhr.responseText); - if (body?.error) msg = body.error; - } catch { - /* keep default message */ - } - reject(new Error(msg)); - }; - xhr.onerror = () => reject(new Error('Upload failed: network error')); - xhr.ontimeout = () => reject(new Error('Upload timed out')); - xhr.send(form); - }); + const response = await fetchWithTimeout( + `${_backendUrl}/api/analyze/upload`, + { method: 'POST', body: form }, + 5 * 60_000, // up to 5 min for large repos + ); + await assertOk(response); + return response.json() as Promise<{ jobId: string; status: string }>; }; // ── Analyze API ──────────────────────────────────────────────────────────── diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index e25afe898..16f3b2c6f 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -1591,9 +1591,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => requireLocalhostOrigin, async (req, res) => { try { - const { url: repoUrl, force, embeddings, dropEmbeddings } = req.body; - // `path` is canonicalized below (realpath), so keep it mutable. - let repoLocalPath: string | undefined = req.body.path; + const { url: repoUrl, path: repoLocalPath, force, embeddings, dropEmbeddings } = req.body; // Input type validation if (repoUrl !== undefined && typeof repoUrl !== 'string') { @@ -1612,27 +1610,16 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => // Path validation. The previous `normalize !== resolve` guard was inert // (both collapse `..` identically) and only false-rejected trailing - // slashes. Analyzing a local path the operator names is the tool's - // intended capability (same as the CLI); the cross-origin reach is what - // mattered, and that is closed by requireLocalhostOrigin above. Here we - // just require an absolute path that actually exists and is a directory, - // using its realpath-canonical form downstream. - if (repoLocalPath) { - if (!path.isAbsolute(repoLocalPath)) { - res.status(400).json({ error: '"path" must be an absolute path' }); - return; - } - try { - const resolved = await fs.realpath(repoLocalPath); - if (!(await fs.stat(resolved)).isDirectory()) { - res.status(400).json({ error: '"path" must be a directory' }); - return; - } - repoLocalPath = resolved; - } catch { - res.status(404).json({ error: '"path" does not exist or is not accessible' }); - return; - } + // slashes, so it is dropped. Analyzing a local path the operator names + // is the tool's intended capability (same as the CLI); the dangerous + // part was cross-origin reach, which is closed by requireLocalhostOrigin + // on this route. We only require an absolute path here and let the + // analyze worker surface a clear error if it does not exist. (We do NOT + // realpath/stat the path in-route: that would be a user-controlled + // filesystem read — CodeQL js/path-injection — for no security gain.) + if (repoLocalPath && !path.isAbsolute(repoLocalPath)) { + res.status(400).json({ error: '"path" must be an absolute path' }); + return; } const job = jobManager.createJob({ repoUrl, repoPath: repoLocalPath });