fix(security): prevent path traversal and restrict file size + types in file content endpoint#1606
Open
atul-upadhyay-7 wants to merge 3 commits into
Open
fix(security): prevent path traversal and restrict file size + types in file content endpoint#1606atul-upadhyay-7 wants to merge 3 commits into
atul-upadhyay-7 wants to merge 3 commits into
Conversation
…in file content endpoint (nisshchayarathi#1589)
|
@anshika1179 is attempting to deploy a commit to the Nisshchaya's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughAdds validateFilePath and encodePathSegments to the file content GET handler, rejects unsafe paths/extensions, URL-encodes path segments for raw.githubusercontent.com requests, enforces a 1MB size limit, and expands Jest tests to cover traversal, encoding, size, allowed/blocked extensions, and upstream errors. ChangesPath Traversal Mitigation
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as GET Handler
participant Validator as validateFilePath
participant Encoder as encodePathSegments
participant GitHub as raw.githubusercontent.com
participant HandlerInternal as Handler(size checks)
Client->>Handler: GET /api/repositories/[id]/files/content?path=...
Handler->>Validator: validateFilePath(path)
alt invalid
Validator-->>Handler: error
Handler->>Client: 400 Bad Request
else valid
Handler->>Encoder: encodePathSegments(path)
Encoder-->>Handler: encodedPath
Handler->>GitHub: GET encoded raw URL
GitHub-->>Handler: content + headers
Handler->>HandlerInternal: check content-length and text size ≤ 1MB
alt size too large
Handler->>Client: 400 Payload Too Large
else ok
Handler->>Client: 200 OK with JSON { content, path }
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🎉 Thanks for your contribution, @atul-upadhyay-7!Your PR has passed our automated GSSoC quality checks. Here's a quick summary:
A maintainer will review your PR soon. Please be patient and available for feedback. 💪 GSSoC'26 automation · Maintainer: @nisshchayarathi |
🎉 Thanks for your contribution, @atul-upadhyay-7!Your PR has passed our automated GSSoC quality checks. Here's a quick summary:
A maintainer will review your PR soon. Please be patient and available for feedback. 💪 GSSoC'26 automation · Maintainer: @nisshchayarathi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/repositories/`[id]/files/content/__tests__/route.test.ts:
- Around line 73-111: Add two new test cases in the same test suite that
exercise the GET handler: (1) duplicate the ".env" sensitive-file test but use
paths ".env.local" and "cert.PEM" (call NextRequest with ?path=.env.local and
?path=cert.PEM, invoke GET(request, { params: { id: "1" } }) and assert status
400 and error contains "Access to sensitive files is restricted"); (2) add a
test for an oversized streamed body when Content-Length is missing by mocking
global.fetch to return ok: true, headers.get -> null for "content-length", and
text() -> a string larger than 1MB (e.g. 'a'.repeat(1024*1024+1)), then call GET
with ?path=src/index.js and assert status 400 and error contains "File size
exceeds 1MB limit"; reference the GET handler, NextRequest constructor, and the
existing tests in route.test.ts to place and mirror assertion style.
In `@app/api/repositories/`[id]/files/content/route.ts:
- Around line 101-112: The current code calls response.text() and uses
content.length after optionally trusting response.headers.get("content-length"),
which is unsafe; change to stream and enforce a byte-accurate cap while reading
the response body so you never buffer the entire payload: read from
response.body (ReadableStream / getReader()) increment a byte counter (use the
Uint8Array chunk.byteLength) and abort/throw once the counter exceeds the 1MB
limit, and use that streamed data (or reject with a 413/400) instead of
response.text() or string.length; update any logic that referenced content,
contentLengthHeader, or response to use the streamed result or early-rejection
path.
- Around line 13-16: The current filename checks (using filename === ".env" and
endsWith(".env")/".pem"/".key") are bypassable by paths like
"secrets/.env.local" and by case variations like "CERT.PEM"; update the
validation to operate on the basename and normalized case: obtain the file's
basename (e.g., via path.basename(filename)) and compare against a
case-normalized value (e.g., lowercased) using a pattern that matches ".env" and
any ".env.*" (for example check basename === ".env" or basename
startsWith(".env.")) and use case-insensitive checks for ".pem" and ".key"
(e.g., lowercased basename.endsWith(".pem")/".key") so that ".env.local",
"CERT.PEM", and similar variants are correctly rejected; adjust the logic around
the filename variable in route.ts accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27c564f4-5493-458c-88a5-3f85437273a6
📒 Files selected for processing (2)
app/api/repositories/[id]/files/content/__tests__/route.test.tsapp/api/repositories/[id]/files/content/route.ts
🎉 Thanks for your contribution, @atul-upadhyay-7!Your PR has passed our automated GSSoC quality checks. Here's a quick summary:
A maintainer will review your PR soon. Please be patient and available for feedback. 💪 GSSoC'26 automation · Maintainer: @nisshchayarathi |
1 similar comment
🎉 Thanks for your contribution, @atul-upadhyay-7!Your PR has passed our automated GSSoC quality checks. Here's a quick summary:
A maintainer will review your PR soon. Please be patient and available for feedback. 💪 GSSoC'26 automation · Maintainer: @nisshchayarathi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/api/repositories/[id]/files/content/__tests__/route.test.ts (1)
511-530: 💤 Low value
Scenario 9.3name and assertion disagree.The title says "blocks long path names exceeding bounds safely" but the route enforces no length bound, so the test correctly asserts
200. Rename to reflect that long paths are allowed to avoid implying a non-existent limit for future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/repositories/`[id]/files/content/__tests__/route.test.ts around lines 511 - 530, The test title for Scenario 9.3 is misleading (it says "blocks long path names exceeding bounds safely" while the assertion expects 200); update the test's description string in the describe/it block (look for the "Scenario 9.3" it in route.test.ts) to indicate that long paths are allowed (e.g. "permits long path names" or similar) so the test name matches the assertion and intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/api/repositories/`[id]/files/content/__tests__/route.test.ts:
- Around line 511-530: The test title for Scenario 9.3 is misleading (it says
"blocks long path names exceeding bounds safely" while the assertion expects
200); update the test's description string in the describe/it block (look for
the "Scenario 9.3" it in route.test.ts) to indicate that long paths are allowed
(e.g. "permits long path names" or similar) so the test name matches the
assertion and intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93035df5-ef47-4b3b-9271-511fce86f329
📒 Files selected for processing (1)
app/api/repositories/[id]/files/content/__tests__/route.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a critical security vulnerability in the repository file content endpoint. Previously, the endpoint did not perform any validation or encoding on the path query parameter, allowing attackers to access arbitrary internal configuration files or sensitive files (like .env) using path traversal sequences.
Changes:
..), starting with/, containing null bytes (\0), or ending with.env,.pem,.key(sensitive files).encodeURIComponentto prevent bypasses while preserving directory structure.Closes #1589
Summary by CodeRabbit
Bug Fixes
Tests