fix(security): prevent path traversal in /assets/ endpoint#40
fix(security): prevent path traversal in /assets/ endpoint#40JulesB40 wants to merge 1 commit intoT3-Content:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughReplaces string-based asset path construction with canonical path resolution against a PUBLIC_DIR, validates resolved paths to prevent directory traversal (returns 403 if outside), and returns 404 for empty asset requests; existing caching headers are preserved. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server\n(request handler)
participant FS as FileSystem\n(PUBLIC_DIR)
Client->>Server: GET /assets/<path>
Server->>Server: resolve PUBLIC_DIR + requested path
Server->>FS: check resolved path location
alt path outside PUBLIC_DIR
Server-->>Client: 403 Forbidden
else empty path
Server-->>Client: 404 Not Found
else path inside PUBLIC_DIR
Server->>FS: read file
FS-->>Server: file bytes
Server-->>Client: 200 OK + file + caching headers
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Block path traversal and return 404/403 in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server.ts (1)
427-427: Hoistresolve("./public")out of the request handler.
resolve("./public")recomputes the same absolute path (viaprocess.cwd()) on every/assets/request. Move it to module scope.♻️ Proposed refactor
+const PUBLIC_DIR = resolve("./public"); + const server = Bun.serve<WsData>({ ... async fetch(req, server) { ... if (url.pathname.startsWith("/assets/")) { - const publicDir = resolve("./public"); - const assetPath = url.pathname.slice(8); - const resolved = resolve(publicDir, assetPath); - if (!resolved.startsWith(publicDir)) { + const assetPath = url.pathname.slice(8); + const resolved = resolve(PUBLIC_DIR, assetPath); + if (!resolved.startsWith(PUBLIC_DIR + "/") && resolved !== PUBLIC_DIR) { return new Response("Forbidden", { status: 403 }); } - const file = Bun.file(resolved); + const file = Bun.file(resolved);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.ts` at line 427, The code currently calls resolve("./public") inside the request handler for /assets/ on every request (assigned to publicDir); hoist that call to module scope so the absolute path is computed once at load time. Move the declaration/initialization of publicDir = resolve("./public") out of the handler function and into the top-level of the module, then update the handler to reference the module-scoped publicDir variable (look for the request handler that builds asset paths for "/assets/"). Ensure no other logic relies on a per-request recomputation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server.ts`:
- Around line 428-429: The code uses assetPath = url.pathname.slice(8) and
resolve(publicDir, assetPath) which makes "/assets/" produce an empty assetPath
and resolve to publicDir (a directory) leading to Bun.file(publicDir) returning
an invalid response; fix by rejecting empty assetPath before resolving (e.g., if
(!assetPath) return a 404) or equivalently check url.pathname === '/assets' ||
url.pathname === '/assets/' and return 404; additionally keep the existing
startsWith/publicDir containment guard to prevent directory traversal.
- Around line 430-432: The guard using resolved.startsWith(publicDir) is unsafe;
replace it with a path.relative-based containment check: compute
relative(publicDir, resolved) (using the already-imported relative) and return
403 if that result starts with '..' or is exactly '..' (i.e., not contained),
otherwise allow; update the check around the resolved/publicDir logic where
resolved and publicDir are used so sibling paths like "/app/public-extra" no
longer pass.
---
Nitpick comments:
In `@server.ts`:
- Line 427: The code currently calls resolve("./public") inside the request
handler for /assets/ on every request (assigned to publicDir); hoist that call
to module scope so the absolute path is computed once at load time. Move the
declaration/initialization of publicDir = resolve("./public") out of the handler
function and into the top-level of the module, then update the handler to
reference the module-scoped publicDir variable (look for the request handler
that builds asset paths for "/assets/"). Ensure no other logic relies on a
per-request recomputation.
The /assets/ endpoint was vulnerable to path traversal attacks that could allow reading arbitrary files on the server (e.g., /assets/../.env, /assets/../server.ts). This fix resolves the path and validates that it remains within the public directory before serving the file. Returns 403 Forbidden if path escapes the intended directory.
JulesB40
force-pushed
the
fix-path-traversal
branch
from
8e295eb to
307f325
Compare
|
Duplicate of #6 (or, rather, #6 covers this and more). Also, while the concatenation isn't ideal, it's not actually vulnerable to path traversal. The line above that wraps the incoming URL in a new Line 493 in ccaa86b For context, see the WHATWG URL specification. Any |
2 participants
Summary
Fixes a path traversal vulnerability in the
/assets/endpoint that could allow reading arbitrary files on the server.Vulnerability Details
The original code concatenated the URL pathname directly to
./publicwithout validation:This allowed attackers to request paths like
/assets/../.envor/assets/../server.tsto read sensitive files outside the public directory.Fix
node:path/resolveTesting
Tested the following scenarios:
/assets/logo.svg→ ✅ Returns file/assets/../server.ts→ ✅ Returns 403 Forbidden/assets/..%2Fserver.ts→ ✅ Returns 403 ForbiddenNotes
While the live site currently has CDN-level path normalization that mitigates this, the vulnerability exists in the code itself and would be exploitable if:
Summary by CodeRabbit