fix(review): resolve CodeQL path-injection + CSRF introduced by the upload change

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) <noreply@anthropic.com>
This commit is contained in:
Gergo Magyar 2026-06-09 09:13:38 +00:00
parent 9c46cc0387
commit 9b19b18ec2
5 changed files with 37 additions and 77 deletions

View file

@ -168,7 +168,7 @@ export const RepoAnalyzer = ({ variant, onComplete, onCancel }: RepoAnalyzerProp
const { t } = useTranslation(['common', 'errors', 'onboarding']);
const inputId = useId();
const [mode, setMode] = useState<InputMode>('github');
const [uploadPercent, setUploadPercent] = useState<number | null>(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
<FolderOpen className="h-3.5 w-3.5" />
{t('onboarding:repoAnalyzer.upload.button')}
</button>
{uploadPercent !== null && (
{uploading && (
<div role="status" data-testid="upload-progress" className="space-y-1">
<div className="h-1.5 w-full overflow-hidden rounded-full bg-elevated">
<div
className="h-full bg-accent transition-all duration-150"
style={{ width: `${uploadPercent}%` }}
/>
<div className="h-full w-1/3 animate-pulse rounded-full bg-accent" />
</div>
<p className="text-xs text-text-muted">
{t('onboarding:repoAnalyzer.upload.uploading', { percent: uploadPercent })}
{t('onboarding:repoAnalyzer.upload.uploading')}
</p>
</div>
)}
{uploadSummary && uploadPercent === null && phase !== 'error' && (
{uploadSummary && !uploading && phase !== 'error' && (
<p className="text-xs text-text-muted" data-testid="upload-summary">
{t('onboarding:repoAnalyzer.upload.selected', {
count: uploadSummary.count,

View file

@ -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."
}

View file

@ -64,7 +64,7 @@
"hideBackground": "隐藏(分析继续在后台进行)",
"upload": {
"button": "上传文件夹",
"uploading": "上传中… {{percent}}%",
"uploading": "上传中…",
"selected": "已准备 {{count}} 个文件(已跳过 {{dropped}} 个:.git、node_modules、构建产物)",
"empty": "该文件夹中未找到可分析的文件。"
}

View file

@ -761,50 +761,26 @@ export const fetchClusterDetail = async (repo: string, name: string): Promise<un
* Upload a folder (selected via `<input webkitdirectory>`) 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 ────────────────────────────────────────────────────────────

View file

@ -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 });