fix(dev): prevent X-Forwarded-For spoofing on Unix socket VFS access#4114
Open
nyxsky404 wants to merge 2 commits intonitrojs:mainfrom
Open
fix(dev): prevent X-Forwarded-For spoofing on Unix socket VFS access#4114nyxsky404 wants to merge 2 commits intonitrojs:mainfrom
nyxsky404 wants to merge 2 commits intonitrojs:mainfrom
Conversation
Deny VFS access when running on Unix socket since IP cannot be reliably determined. Never trust X-Forwarded-For header for this security-sensitive endpoint. Fixes nitrojs#4103
|
@nyxsky404 is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDisable X-Forwarded-For usage for VFS IP detection and add explicit 403 rejection for requests arriving via Unix sockets; add unit tests covering Unix-socket and network-socket VFS access denial scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 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 unit tests (beta)
📝 Coding Plan
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 Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/dev/vfs.ts (1)
17-20: Remove explanatory inline comments in this block to match repo style.These two comments describe the code action/rationale inline; this file pattern prefers self-explanatory code without such line comments.
As per coding guidelines: `src/**/*.{ts,js}`: "Do not add comments explaining what the line does unless prompted".💡 Minimal style-only diff
- // Never trust X-Forwarded-For for VFS access - it's attacker-controlled const ip = getRequestIP(event, { xForwardedFor: false }); - // Deny access on Unix socket since IP cannot be reliably determined if (isUnixSocket) { throw new HTTPError({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dev/vfs.ts` around lines 17 - 20, Remove the two explanatory inline comments above the getRequestIP call so the code follows the repo style; specifically delete the comment "Never trust X-Forwarded-For for VFS access - it's attacker-controlled" and the comment "Deny access on Unix socket since IP cannot be reliably determined" and leave the call to getRequestIP(event, { xForwardedFor: false }) and the ip variable as-is (referencing getRequestIP, event, xForwardedFor, and ip to locate the lines).test/unit/vfs-security.test.ts (1)
56-61: Deduplicate repeated Nitro fixture setup across tests.
mockNitrois rebuilt four times with identical shape; extracting one helper will make the suite easier to evolve.♻️ Suggested cleanup
+function createMockNitro() { + return { + options: { rootDir: "/test/root" }, + vfs: new Map([["/test/root/test.js", { render: () => "test content" }]]), + } as unknown as Nitro; +} + describe("VFS Security - X-Forwarded-For spoofing on Unix socket", () => { it("should reject requests with spoofed X-Forwarded-For header on Unix socket", async () => { - const mockNitro = { - options: { - rootDir: "/test/root", - }, - vfs: new Map([["/test/root/test.js", { render: () => "test content" }]]), - } as unknown as Nitro; + const mockNitro = createMockNitro(); @@ it("should reject requests without X-Forwarded-For on Unix socket", async () => { - const mockNitro = { - options: { - rootDir: "/test/root", - }, - vfs: new Map([["/test/root/test.js", { render: () => "test content" }]]), - } as unknown as Nitro; + const mockNitro = createMockNitro(); @@ it("should reject requests from non-local IP on regular network socket", async () => { - const mockNitro = { - options: { - rootDir: "/test/root", - }, - vfs: new Map([["/test/root/test.js", { render: () => "test content" }]]), - } as unknown as Nitro; + const mockNitro = createMockNitro(); @@ it("should NOT trust X-Forwarded-For header on regular network socket", async () => { - const mockNitro = { - options: { - rootDir: "/test/root", - }, - vfs: new Map([["/test/root/test.js", { render: () => "test content" }]]), - } as unknown as Nitro; + const mockNitro = createMockNitro();Also applies to: 76-81, 94-99, 112-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/vfs-security.test.ts` around lines 56 - 61, Extract the repeated mockNitro construction into a single test helper (e.g., makeMockNitro or getMockNitro) that returns the typed Nitro fixture used in tests; replace each inline const mockNitro = { options: { rootDir: "/test/root" }, vfs: new Map([["/test/root/test.js", { render: () => "test content" }]]) } as unknown as Nitro with calls to that helper in all test cases (the occurrences around lines 56, 76, 94, 112), ensuring the helper exports or is declared in the test file scope and preserves the same shape and typing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/dev/vfs.ts`:
- Around line 17-20: Remove the two explanatory inline comments above the
getRequestIP call so the code follows the repo style; specifically delete the
comment "Never trust X-Forwarded-For for VFS access - it's attacker-controlled"
and the comment "Deny access on Unix socket since IP cannot be reliably
determined" and leave the call to getRequestIP(event, { xForwardedFor: false })
and the ip variable as-is (referencing getRequestIP, event, xForwardedFor, and
ip to locate the lines).
In `@test/unit/vfs-security.test.ts`:
- Around line 56-61: Extract the repeated mockNitro construction into a single
test helper (e.g., makeMockNitro or getMockNitro) that returns the typed Nitro
fixture used in tests; replace each inline const mockNitro = { options: {
rootDir: "/test/root" }, vfs: new Map([["/test/root/test.js", { render: () =>
"test content" }]]) } as unknown as Nitro with calls to that helper in all test
cases (the occurrences around lines 56, 76, 94, 112), ensuring the helper
exports or is declared in the test file scope and preserves the same shape and
typing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d476b7b-a0b9-45e7-8950-d618c567a15d
📒 Files selected for processing (2)
src/dev/vfs.tstest/unit/vfs-security.test.ts
- Remove explanatory inline comments in vfs.ts per repo style - Extract createMockNitro helper to deduplicate test fixtures
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
1 participant
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.
🔗 Linked issue
#4103
❓ Type of change
📚 Description
This PR fixes a security vulnerability where the VFS handler trusted the
X-Forwarded-Forheader when running on a Unix socket behind a reverse proxy.The vulnerability:
When
NITRO_UNIX_SOCKETis used, the code passed{ xForwardedFor: isUnixSocket }togetRequestIP(). An attacker behind a reverse proxy could sendX-Forwarded-For: 127.0.0.1to satisfy the localhost check and access VFS content (application source code).The fix:
X-Forwarded-Forfor VFS access - it's attacker-controlled📝 Checklist
pnpm formatandpnpm typecheck