fix(docs): prevent path traversal in /docs static handler#80
fix(docs): prevent path traversal in /docs static handler#80
Conversation
📝 WalkthroughWalkthrough该更改引入了安全路径解析功能，通过新增 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential security vulnerability by preventing path traversal in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a security fix to prevent path traversal in the /docs and /__tsr static file handlers. The approach of creating a resolvePathWithinBase utility is good. My review includes a suggestion to make this utility more robust by allowing valid relative paths while still preventing traversal.
Critically, a similar path traversal vulnerability appears to have been missed in the spaPlugin which handles the /* route. The static file serving logic in spaPlugin (around line 181 in backend/src/index.ts) uses path.join without sanitization and should also be updated to use resolvePathWithinBase to prevent arbitrary file reads.
I have also added a suggestion for an additional test case to cover the improved path resolution logic.
| import { resolve } from "node:path"; | ||
|
|
||
| export function resolvePathWithinBase( | ||
| baseDir: string, | ||
| requestPath: string, | ||
| ): string | null { | ||
| const normalizedRequestPath = requestPath.replaceAll("\\", "/"); | ||
| const pathSegments = normalizedRequestPath | ||
| .replace(/^\/+/, "") | ||
| .split("/") | ||
| .filter(Boolean); | ||
|
|
||
| if (pathSegments.includes("..")) { | ||
| return null; | ||
| } | ||
|
|
||
| return resolve(baseDir, ...pathSegments); | ||
| } |
There was a problem hiding this comment.
The current implementation is a bit too restrictive as it rejects any path containing .., even if it resolves to a path still within the base directory (e.g., /foo/../bar). A more robust approach is to resolve the path first and then verify that it is still within the intended base directory. This allows for valid relative paths while still preventing directory traversal attacks. I've also added a check for null byte injection as an extra precaution.
import { isAbsolute, relative, resolve } from "node:path";
export function resolvePathWithinBase(
baseDir: string,
requestPath: string,
): string | null {
// Prevent null byte injection.
if (requestPath.includes("\u0000")) {
return null;
}
// Resolve the path. This will process '..', '.', etc.
// We must treat requestPath as relative to baseDir, so we remove leading slashes.
const resolvedPath = resolve(baseDir, requestPath.replace(/^\/+/, ""));
// Check if the resolved path is a sub-path of baseDir.
const relation = relative(baseDir, resolvedPath);
// If `relation` is empty, it's the base directory itself.
// If it starts with '..', it has escaped the base directory.
// An absolute path as `relation` is also an escape.
if (relation.startsWith("..") || isAbsolute(relation)) {
return null;
}
return resolvedPath;
}| expect(resolvePathWithinBase(baseDir, "//etc/passwd")).toBe( | ||
| resolve(baseDir, "etc/passwd"), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
To complement the suggested change in safePath.ts, it would be beneficial to add a test case that verifies that valid paths containing .. which resolve within the base directory are handled correctly.
);
});
test("resolves valid paths with '..' segments that stay within the base directory", () => {
expect(
resolvePathWithinBase(baseDir, "/assets/css/../js/app.js"),
).toBe(resolve(baseDir, "assets/js/app.js"));
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/index.ts (1)
180-184:⚠️ Potential issue | 🔴 Critical严重安全漏洞：
spaPlugin仍存在路径遍历漏洞。
docsPlugin已修复，但spaPlugin的第 181 行仍直接使用join(dir, path)构建文件路径，未应用任何遍历检查。攻击者可通过类似GET /../../etc/passwd的请求读取任意文件。🐛 修复 spaPlugin 中的路径遍历漏洞
.get("/*", async ({ path, status }) => { // Skip /docs and /__tsr routes - they're handled by docsPlugin if (path.startsWith("/docs") || path.startsWith("/__tsr")) { return status(404); } // Skip API routes and metrics (include trailing slash to prevent SPA fallback) if ( path.startsWith("/api") || path.startsWith("/v1") || path === "/metrics" || path === "/metrics/" ) { return status(404); } // Try to serve as static file first - const staticResponse = await serveStaticFile(join(dir, path)); + const safePath = resolvePathWithinBase(dir, path); + const staticResponse = safePath ? await serveStaticFile(safePath) : null; if (staticResponse) { return staticResponse; } // Fall back to index.html for SPA routing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/index.ts` around lines 180 - 184, spaPlugin still constructs filesystem paths directly using join(dir, path) before calling serveStaticFile, leaving a path traversal vulnerability; update spaPlugin to validate/sanitize the requested path (normalize and ensure it stays within the base dir) before joining or serving: resolve and normalize the incoming path, reject or canonicalize any paths that escape the base directory (e.g., containing ../ after normalization), then call serveStaticFile with the safe path (references: spaPlugin, serveStaticFile, join(dir, path)); if the normalized path is outside dir return a 403/404 instead of serving the file.
🧹 Nitpick comments (2)
backend/src/utils/safePath.test.ts (1)
18-26: 建议增加 URL 编码和其他边界情况的测试用例。当前测试覆盖了基本场景，但缺少一些安全关键的边界情况测试，建议补充：
🧪 建议添加的测试用例
test("rejects traversal outside the base directory", () => { expect(resolvePathWithinBase(baseDir, "/../../etc/passwd")).toBeNull(); }); + test("handles URL-encoded path segments as literals", () => { + // %2e%2e should be treated as literal directory name, not traversal + expect(resolvePathWithinBase(baseDir, "/%2e%2e/etc/passwd")).toBe( + resolve(baseDir, "%2e%2e/etc/passwd"), + ); + }); + + test("rejects mixed traversal attempts", () => { + expect(resolvePathWithinBase(baseDir, "/foo/../../../etc/passwd")).toBeNull(); + }); + + test("handles empty path", () => { + expect(resolvePathWithinBase(baseDir, "")).toBe(baseDir); + }); + test("contains repeated leading slashes within the base directory", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/utils/safePath.test.ts` around lines 18 - 26, Add tests in safePath.test.ts for resolvePathWithinBase to cover URL-encoded and other edge cases: verify encoded traversal sequences (e.g., "%2e%2e" and "%2f"), mixed separators and redundant segments (e.g., "subdir/../etc", "subdir\\..\\etc"), inputs with trailing dots or dot-segments ("./file", "../file"), empty or multiple consecutive slashes, and percent-encoded slashes; for each case assert that traversal outside base returns null and valid in-base paths resolve to resolve(baseDir, "...") using the existing resolvePathWithinBase helper to ensure the function properly decodes/normalizes before validation.backend/src/utils/safePath.ts (1)
13-17: 建议增加纵深防御：验证解析后的路径确实位于基目录内。当前实现通过检查字面量
..来防止目录遍历。虽然 URL 编码的遍历序列（如%2e%2e）不会构成真正的威胁——因为浏览器在发送请求前已解码路径，而 Elysia 不会再次解码——但从安全纵深防御角度，建议在解析后额外验证最终路径确实位于基目录内。🛡️ 建议的纵深防御修复
import { resolve } from "node:path"; export function resolvePathWithinBase( baseDir: string, requestPath: string, ): string | null { const normalizedRequestPath = requestPath.replaceAll("\\", "/"); const pathSegments = normalizedRequestPath .replace(/^\/+/, "") .split("/") .filter(Boolean); if (pathSegments.includes("..")) { return null; } - return resolve(baseDir, ...pathSegments); + const resolvedBase = resolve(baseDir); + const resolvedPath = resolve(baseDir, ...pathSegments); + + // Defense-in-depth: ensure resolved path is within base directory + if (!resolvedPath.startsWith(resolvedBase + "/") && resolvedPath !== resolvedBase) { + return null; + } + + return resolvedPath; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/utils/safePath.ts` around lines 13 - 17, The current check only rejects literal ".." segments; after computing resolvedPath with resolve(baseDir, ...pathSegments) (using resolve and baseDir/pathSegments), additionally verify the resolvedPath is inside baseDir: compute the absolute resolved path and the absolute baseDir (using resolve) and reject (return null) if the resolved path is outside the base (e.g., path.relative(baseDirAbs, resolvedPath) starts with ".." or resolvedPath does not have baseDirAbs as its prefix). Apply this check in safePath.ts right after calling resolve and before returning the path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/src/index.ts`:
- Around line 180-184: spaPlugin still constructs filesystem paths directly
using join(dir, path) before calling serveStaticFile, leaving a path traversal
vulnerability; update spaPlugin to validate/sanitize the requested path
(normalize and ensure it stays within the base dir) before joining or serving:
resolve and normalize the incoming path, reject or canonicalize any paths that
escape the base directory (e.g., containing ../ after normalization), then call
serveStaticFile with the safe path (references: spaPlugin, serveStaticFile,
join(dir, path)); if the normalized path is outside dir return a 403/404 instead
of serving the file.
---
Nitpick comments:
In `@backend/src/utils/safePath.test.ts`:
- Around line 18-26: Add tests in safePath.test.ts for resolvePathWithinBase to
cover URL-encoded and other edge cases: verify encoded traversal sequences
(e.g., "%2e%2e" and "%2f"), mixed separators and redundant segments (e.g.,
"subdir/../etc", "subdir\\..\\etc"), inputs with trailing dots or dot-segments
("./file", "../file"), empty or multiple consecutive slashes, and
percent-encoded slashes; for each case assert that traversal outside base
returns null and valid in-base paths resolve to resolve(baseDir, "...") using
the existing resolvePathWithinBase helper to ensure the function properly
decodes/normalizes before validation.
In `@backend/src/utils/safePath.ts`:
- Around line 13-17: The current check only rejects literal ".." segments; after
computing resolvedPath with resolve(baseDir, ...pathSegments) (using resolve and
baseDir/pathSegments), additionally verify the resolvedPath is inside baseDir:
compute the absolute resolved path and the absolute baseDir (using resolve) and
reject (return null) if the resolved path is outside the base (e.g.,
path.relative(baseDirAbs, resolvedPath) starts with ".." or resolvedPath does
not have baseDirAbs as its prefix). Apply this check in safePath.ts right after
calling resolve and before returning the path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 52af95d6-32c2-44a8-963b-f5cf2c66bb24
📒 Files selected for processing (3)
backend/src/index.tsbackend/src/utils/safePath.test.tsbackend/src/utils/safePath.ts
1 participant
Motivation
/docs/*handler constructed filesystem paths from request paths without normalization or traversal checks, allowing unauthenticated arbitrary file reads outside the docs directory.__tsrasset route used the same unsafe pattern and remained exposed.Description
resolvePathWithinBaseinbackend/src/utils/safePath.tsthat normalizes request paths, rejects..segments, and resolves into the configured base directory.resolvePathWithinBaseinbackend/src/index.tsfor the/docs/*handler and the/__tsr/*asset route to return404when a path would escape the docs directory instead of reading arbitrary files.backend/src/utils/safePath.test.tscovering in-base resolution, root requests, traversal rejection, and repeated leading slashes.Testing
bun test ./src/utils/safePath.test.ts, which passed all tests.git diff --checkto validate the diff, which reported no whitespace errors.bun run checkfor broader type checks, which failed due to missing repository dependencies/type definitions in the environment (pre-existing module resolution/type errors unrelated to this patch).Codex Task
Summary by CodeRabbit
发布说明
新功能
测试