fix: path traversal guard + SPA method check#85
Conversation
- Validate resolved file paths stay within publicDir (prevents traversal) - Only return SPA fallback HTML for GET requests (POST/PUT → 404) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review Please evaluate:
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds path traversal protection to the asset-serving handler by validating that resolved filesystem paths remain within Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
chittyfinance | 5393e1f | Apr 08 2026, 06:46 AM |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5393e1fe53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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) { |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens static asset serving in the Node production entrypoint by adding a path traversal guard and tightening SPA fallback behavior so that only GET requests can receive index.html.
Changes:
- Add a path traversal check to ensure resolved asset paths remain within
publicDir. - Restrict SPA fallback to GET requests so non-GET unknown routes return 404.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const filePath = resolve(publicDir, url.pathname.replace(/^\//, '')); | ||
| // Prevent path traversal — resolved path must stay inside publicDir | ||
| if (!filePath.startsWith(publicDir + '/') && filePath !== publicDir) { | ||
| return new Response('Forbidden', { status: 403 }); | ||
| } |
There was a problem hiding this comment.
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.
| // 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' } }); | ||
| } |
There was a problem hiding this comment.
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).
Summary
publicDirindex.htmlfor GET requests; POST/PUT/DELETE to unknown routes return 404Test plan
GET /→ 200 HTMLGET /assets/*.css→ 200 correct MIMEGET /dashboard→ 200 SPA fallbackPOST /nonexistent→ 404 (not HTML)/../../server/dev.ts) → blocked by guardGET /health→ 200 JSON🤖 Generated with Claude Code
Summary by CodeRabbit