Skip to content

fix: path traversal guard + SPA method check #85

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chitcommit merged 1 commit into from
Apr 8, 2026
Merged

fix: path traversal guard + SPA method check #85

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions server/dev.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,21 @@ const stubAssets = {
? async (req: Request) => {
const url = new URL(req.url);
const filePath = resolve(publicDir, url.pathname.replace(/^\//, ''));
// Prevent path traversal — resolved path must stay inside publicDir
if (!filePath.startsWith(publicDir + '/') && filePath !== publicDir) {
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use platform-safe path prefix check

The traversal guard compares filePath against publicDir + '/', which assumes POSIX separators. In server/dev.ts, both resolve() outputs use backslashes on Windows (for example, C:\app\public\...), so this condition rejects valid in-tree files and returns 403 for most asset and SPA routes whenever NODE_ENV=production is used on Windows. This is a functional regression introduced by the new guard; use a separator-agnostic check (e.g., path.relative-based) instead of hardcoding '/'.

Useful? React with 👍 / 👎.

return new Response('Forbidden', { status: 403 });
}
Comment on lines 57 to +61
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path-traversal guard is implemented via a string prefix check (filePath.startsWith(publicDir + '/')). This is brittle (e.g., depends on a hard-coded '/' separator and can break on non-POSIX paths) and is easier to get wrong than a path-based containment check. Consider using a path.relative(publicDir, filePath)-based check (and/or canonicalizing with realpath) to reliably ensure the target is inside publicDir before calling statSync/readFileSync.

Copilot uses AI. Check for mistakes.
try {
if (statSync(filePath).isFile()) {
const body = readFileSync(filePath);
const mime = getMimeType(filePath) || 'application/octet-stream';
return new Response(body, { headers: { 'content-type': mime } });
}
} catch {}
// SPA fallback: return index.html for non-file routes
if (indexHtml) return new Response(indexHtml, { headers: { 'content-type': 'text/html; charset=utf-8' } });
// SPA fallback: return index.html only for GET requests
if (req.method === 'GET' && indexHtml) {
return new Response(indexHtml, { headers: { 'content-type': 'text/html; charset=utf-8' } });
}
Comment on lines +69 to +72
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SPA fallback will return index.html for any GET that isn't a file on disk. That means missing static assets like /assets/missing.css will incorrectly return HTML with a 200 instead of a 404, which can break caching and make asset errors harder to detect. Consider restricting the SPA fallback to "route" paths only (e.g., exclude /assets/ and/or only fallback when the path has no file extension or when the request Accept prefers text/html).

Copilot uses AI. Check for mistakes.
return new Response('Not found', { status: 404 });
}
: async () => new Response('Not found', { status: 404 }),
Expand Down
Loading