docs/runtime: warn on external rewrites, sanitize Content-Disposition#418
docs/runtime: warn on external rewrites, sanitize Content-Disposition#418Divkix wants to merge 7 commits intocloudflare:mainfrom
Conversation
Add a startup warning when next.config contains rewrites targeting external origins (e.g. https://...), alerting that credential headers (cookie, authorization, x-api-key, proxy-authorization) are forwarded. Sanitize contentDispositionType in image optimization and prod server to only allow "attachment" — any other value defaults to "inline", preventing header injection via arbitrary config values. Closes cloudflare#414
commit: |
There was a problem hiding this comment.
Pull request overview
Adds runtime safeguards and visibility around two security-sensitive config areas in vinext: external rewrites (reverse-proxy behavior) and Content-Disposition handling for image responses.
Changes:
- Emit a startup/config-resolution
console.warnwhennext.configrewrites include external destinations, listing affected rewrites and noting credential header forwarding. - Sanitize
images.contentDispositionTypeat runtime to only allow"attachment"(otherwise defaulting to"inline") in both image optimization and the Node prod server. - Add unit tests for the external rewrite warning behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/next-config.test.ts | Adds new tests asserting warnings are emitted (or not) based on presence of external rewrite destinations. |
| packages/vinext/src/config/next-config.ts | Imports isExternalUrl and warns when resolved rewrites include external destinations. |
| packages/vinext/src/server/image-optimization.ts | Forces Content-Disposition to "attachment" only when explicitly configured; otherwise "inline". |
| packages/vinext/src/server/prod-server.ts | Applies the same Content-Disposition sanitization for static image serving in both app/pages router servers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Should have warned about the external rewrite | ||
| const externalWarning = warn.mock.calls.find( | ||
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||
| ); | ||
| expect(externalWarning).toBeDefined(); | ||
| expect(externalWarning![0]).toContain("1 external rewrite"); | ||
| expect(externalWarning![0]).toContain("https://api.example.com/:path*"); | ||
| expect(externalWarning![0]).toContain("Credential headers"); | ||
|
|
||
| // The internal rewrite should not appear in the warning | ||
| expect(externalWarning![0]).not.toContain("/other"); |
There was a problem hiding this comment.
The test only asserts that the warning contains the destination URL, but the intended behavior (and issue acceptance criteria) is to list each external rewrite as source → destination. Consider also asserting that the warning string includes the rewrite source and the arrow/formatting so regressions in the listing don’t slip through.
| console.warn( | ||
| `[vinext] Found ${externalRewrites.length} external rewrite(s) that proxy requests to third-party origins:\n` + |
There was a problem hiding this comment.
The warning message uses the "external rewrite(s)" pluralization pattern. Consider switching to proper singular/plural wording (e.g., "1 external rewrite" vs "2 external rewrites") to improve log readability and grep-ability.
| console.warn( | |
| `[vinext] Found ${externalRewrites.length} external rewrite(s) that proxy requests to third-party origins:\n` + | |
| const count = externalRewrites.length; | |
| const noun = count === 1 ? "external rewrite" : "external rewrites"; | |
| console.warn( | |
| `[vinext] Found ${count} ${noun} that proxy requests to third-party origins:\n` + |
| // Warn about external rewrites that act as reverse proxies | ||
| { | ||
| const allRewrites = [...rewrites.beforeFiles, ...rewrites.afterFiles, ...rewrites.fallback]; | ||
| const externalRewrites = allRewrites.filter((r) => isExternalUrl(r.destination)); | ||
| if (externalRewrites.length > 0) { | ||
| const listing = externalRewrites.map((r) => ` ${r.source} → ${r.destination}`).join("\n"); | ||
| console.warn( | ||
| `[vinext] Found ${externalRewrites.length} external rewrite(s) that proxy requests to third-party origins:\n` + | ||
| `${listing}\n` + | ||
| `Credential headers (cookie, authorization, x-api-key, proxy-authorization) are forwarded to the external origin. ` + | ||
| `If this is unintentional, consider proxying at the CDN level instead.`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue #414’s proposed solution includes adding/adjusting documentation to note the security implications of external rewrites, but this PR only adds a runtime warning and header sanitization (no docs changes). If docs updates are still required for closing the issue, consider adding them in this PR or tracking them explicitly in a follow-up.
| headers.set( | ||
| "Content-Disposition", | ||
| config?.contentDispositionType === "attachment" ? "attachment" : "inline", | ||
| ); |
There was a problem hiding this comment.
This change intentionally sanitizes contentDispositionType to only allow "attachment" (otherwise defaulting to "inline"), but there isn’t a unit test covering the invalid/unsafe input case. Consider adding a test that passes a non-"attachment" value (e.g., via as any) and asserts the response header falls back to "inline" so the sanitization guarantee is enforced.
…itization test Use proper singular/plural in the external rewrite warning message instead of "rewrite(s)". Add test assertions verifying the warning includes the source pattern and arrow separator. Add a test confirming that invalid contentDispositionType values default to "inline".
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
This PR adds two security-related improvements: a startup warning for external rewrites that forward credentials, and sanitization of contentDispositionType for image responses. Both are reasonable changes. I have a couple of suggestions and one substantive concern.
Content-Disposition sanitization — The allowlist approach (=== "attachment" instead of passthrough) is the right call. All three codepaths (image-optimization, app router prod server, pages router prod server) are consistently updated, and the worker entry delegates to handleImageOptimization so it's covered too. Good.
External rewrite warning — Clean implementation, good test coverage. The warning message accurately reflects the current behavior of proxyExternalRequest (which does forward cookie, authorization, etc.).
One concern: the warning lists specific credential headers (cookie, authorization, x-api-key, proxy-authorization) but proxyExternalRequest actually forwards all request headers except connection and x-middleware-*. This is slightly misleading — it implies only those four are forwarded when the reality is worse. See inline comment.
| `[vinext] Found ${externalRewrites.length} ${noun} that proxy requests to third-party origins:\n` + | ||
| `${listing}\n` + | ||
| `Credential headers (cookie, authorization, x-api-key, proxy-authorization) are forwarded to the external origin. ` + | ||
| `If this is unintentional, consider proxying at the CDN level instead.`, |
There was a problem hiding this comment.
The warning says "Credential headers (cookie, authorization, x-api-key, proxy-authorization) are forwarded" — but looking at proxyExternalRequest in config-matchers.ts, it actually forwards all request headers (only stripping connection and x-middleware-*). This is a broader exposure than the warning suggests.
Consider either:
- Rewording to say "All request headers (including cookies, authorization tokens, etc.) are forwarded to the external origin" — which is more accurate, or
- Actually stripping credential headers in
proxyExternalRequestand then the warning can say they're stripped (which would be a more meaningful security improvement but is a larger change)
Option 1 is a minimal fix for accuracy:
| `If this is unintentional, consider proxying at the CDN level instead.`, | |
| `All request headers (including cookies, authorization tokens, and other credentials) are forwarded to the external origin. ` + |
| `${listing}\n` + | ||
| `Credential headers (cookie, authorization, x-api-key, proxy-authorization) are forwarded to the external origin. ` + | ||
| `If this is unintentional, consider proxying at the CDN level instead.`, | ||
| ); |
There was a problem hiding this comment.
Minor: this suggests "proxying at the CDN level" as an alternative, which is reasonable advice for Cloudflare users. But for non-Cloudflare deployments, this might not be as actionable. Consider mentioning the more general alternative of using server-side fetch in an API route/route handler, which works everywhere.
tests/next-config.test.ts
Outdated
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||
| ); | ||
| expect(externalWarning).toBeDefined(); | ||
| expect(externalWarning![0]).toContain("3 external rewrite"); |
There was a problem hiding this comment.
Nit: this assertion checks for "3 external rewrite" which matches both "3 external rewrites" (correct) and "3 external rewrite " (hypothetical singular bug). Consider asserting the exact plural form:
| expect(externalWarning![0]).toContain("3 external rewrite"); | |
| expect(externalWarning![0]).toContain("3 external rewrites"); |
| }), | ||
| }; | ||
| const response = await handleImageOptimization(request, handlers, undefined, { | ||
| contentDispositionType: "bogus" as "inline", |
There was a problem hiding this comment.
Good addition — testing the sanitization boundary with an invalid value cast via as. This directly exercises the security fix.
|
Review posted. Here's a summary of the feedback I left: Key concern: The warning message in Other comments:
The Content-Disposition changes look solid — consistent allowlist approach across all three codepaths (image-optimization, app router prod server, pages router prod server), and the worker entry is covered via |
…ding proxyExternalRequest copies all request headers, not just credential headers. Updated warning text to accurately state this and suggest server-side fetch as an alternative. Tightened test assertion for exact plural form.
|
/bigbonk re-review |
There was a problem hiding this comment.
Review
Good PR — the Content-Disposition sanitization is correct and consistent across all codepaths, and the external rewrite warning is a useful security signal. A few observations, one of which is substantive.
Content-Disposition
The allowlist approach (=== "attachment" ? "attachment" : "inline") is the right pattern. All three codepaths (image-optimization, app router prod server, pages router prod server) are updated consistently. The worker entry delegates to handleImageOptimization so it's covered transitively. The new test exercises the invalid-value boundary. This part is solid.
External rewrite warning
The implementation is clean and the test coverage is good. One accuracy issue and one minor nit.
| console.warn( | ||
| `[vinext] Found ${externalRewrites.length} ${noun} that proxy requests to third-party origins:\n` + | ||
| `${listing}\n` + | ||
| `All request headers (including cookies, authorization tokens, and other credentials) are forwarded to the external origin. ` + |
There was a problem hiding this comment.
The warning text is accurate about what happens (all request headers are forwarded), which is good — the earlier revision that listed specific headers was misleading.
However, the warning describes the symptom but doesn't address the cause. proxyExternalRequest (in config-matchers.ts:940-953) copies all request headers and only strips connection and x-middleware-*. The HOP_BY_HOP_HEADERS set (which includes proxy-authorization) is only applied to the response from the upstream, not to the outbound request.
This means proxy-authorization from the client is forwarded to the external origin on the request side — it's only stripped from the upstream's response. This is the actual security gap that issue #414 is about.
The warning is good as a short-term measure, but the real fix should be stripping credential headers from the outbound request in proxyExternalRequest. That's a separate change and probably out of scope for this PR, but worth noting — the warning alone doesn't mitigate the credential leakage, it just makes it visible.
Consider filing a follow-up issue (or noting in the PR description) that proxyExternalRequest should strip authorization, cookie, proxy-authorization, etc. from outbound requests by default (with an opt-in to forward them). That would be the actual security fix.
| `[vinext] Found ${externalRewrites.length} ${noun} that proxy requests to third-party origins:\n` + | ||
| `${listing}\n` + | ||
| `All request headers (including cookies, authorization tokens, and other credentials) are forwarded to the external origin. ` + | ||
| `If this is unintentional, consider using a server-side fetch in an API route or proxying at the CDN level instead.`, |
There was a problem hiding this comment.
The suggestion to use "a server-side fetch in an API route" is good and works for all deployment targets. The "proxying at the CDN level" suggestion is Cloudflare-specific. Since vinext's primary target is Cloudflare Workers this is reasonable, but it could be slightly clearer:
| `If this is unintentional, consider using a server-side fetch in an API route or proxying at the CDN level instead.`, | |
| `If this is unintentional, consider using a server-side fetch in an API route (which gives you control over which headers are forwarded) or proxying at the CDN/edge level instead.`, |
tests/next-config.test.ts
Outdated
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||
| ); | ||
| expect(externalWarning).toBeDefined(); | ||
| expect(externalWarning![0]).toContain("1 external rewrite"); |
There was a problem hiding this comment.
Nit: "1 external rewrite" also matches "1 external rewrites" (hypothetical pluralization bug). Since the code explicitly handles singular/plural, the test should verify it:
| expect(externalWarning![0]).toContain("1 external rewrite"); | |
| expect(externalWarning![0]).toContain("1 external rewrite "); |
The trailing space after rewrite ensures it doesn't match "1 external rewrites". Alternatively, use a regex match for the exact phrase.
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||
| ); | ||
| expect(externalWarning).toBeDefined(); | ||
| expect(externalWarning![0]).toContain("3 external rewrites"); |
There was a problem hiding this comment.
Same nit as above — this correctly tests the plural form, but the assertion "3 external rewrites" would also match a (very unlikely) string like "3 external rewrites-foo". This is fine in practice, just noting for consistency with the singular test.
| }), | ||
| }; | ||
| const response = await handleImageOptimization(request, handlers, undefined, { | ||
| contentDispositionType: "bogus" as "inline", |
There was a problem hiding this comment.
Good test — this directly exercises the sanitization boundary. The as "inline" cast is the right way to test the runtime guard while keeping TypeScript happy.
|
Review submitted. Here's a summary of the feedback: Content-Disposition sanitization — Looks good. The allowlist approach ( External rewrite warning — Clean implementation with good test coverage. Key feedback:
|
Clarify why API routes are preferable (control over forwarded headers), tighten test assertion to prevent false singular/plural match. Refs: cloudflare#430
|
/bigbonk re-review |
There was a problem hiding this comment.
Review
Overall this is a solid, well-scoped PR. The Content-Disposition sanitization is correct and consistently applied across all codepaths. The external rewrite warning is well-implemented with good test coverage. A few comments below — one substantive, the rest minor.
The prior bonk reviews already flagged the key architectural point: the warning is a visibility measure, but proxyExternalRequest still forwards all request headers (including authorization, cookie, proxy-authorization) to external origins. The HOP_BY_HOP_HEADERS set is only applied to the response side. A follow-up to actually strip credential headers on the outbound request would be the real security fix. I'd suggest noting this explicitly in the PR description or filing a tracking issue, so it doesn't get lost.
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||
| ); | ||
| expect(externalWarning).toBeDefined(); | ||
| expect(externalWarning![0]).toContain("1 external rewrite that"); |
There was a problem hiding this comment.
Nit: The assertion "1 external rewrite that" is tight enough to distinguish singular from plural (since the plural would produce "2 external rewrites that"). Good — this is more precise than what the earlier reviews suggested, since the trailing " that" prevents false matches against "1 external rewrites".
| `[vinext] Found ${externalRewrites.length} ${noun} that proxy requests to third-party origins:\n` + | ||
| `${listing}\n` + | ||
| `All request headers (including cookies, authorization tokens, and other credentials) are forwarded to the external origin. ` + | ||
| `If this is unintentional, consider using a server-side fetch in an API route (which gives you control over which headers are forwarded) or proxying at the CDN/edge level instead.`, |
There was a problem hiding this comment.
The warning text is accurate and the mitigation suggestions are reasonable. One small improvement: the "API route" suggestion could be slightly more explicit about why it's better, since less experienced developers may not immediately see the connection.
| `If this is unintentional, consider using a server-side fetch in an API route (which gives you control over which headers are forwarded) or proxying at the CDN/edge level instead.`, | |
| `If this is unintentional, consider using a server-side fetch in an API route or route handler (where you control exactly which headers are forwarded) or proxying at the CDN/edge level instead.`, |
|
|
||
| // Warn about external rewrites that act as reverse proxies | ||
| { | ||
| const allRewrites = [...rewrites.beforeFiles, ...rewrites.afterFiles, ...rewrites.fallback]; |
There was a problem hiding this comment.
Substantive: This warning fires at config-resolution time (during resolveNextConfig), which means it runs once on dev server startup and once during build. For production servers that load a pre-resolved config, this code path isn't hit — so the warning won't appear in production logs.
Is that intentional? If someone deploys with external rewrites, they'll see the warning during vite dev and vite build but not when the production server actually starts handling traffic. That seems like the right time to surface it. Consider whether the prod server startup paths (prod-server.ts or worker-entry.ts) should also warn.
This isn't a blocker — the build-time warning is already useful — but worth noting for a follow-up.
| "X-Content-Type-Options": "nosniff", | ||
| "Content-Disposition": imageConfig?.contentDispositionType ?? "inline", | ||
| "Content-Disposition": | ||
| imageConfig?.contentDispositionType === "attachment" ? "attachment" : "inline", |
There was a problem hiding this comment.
The sanitization pattern === "attachment" ? "attachment" : "inline" is correct and consistent across all three locations (image-optimization.ts:176, prod-server.ts:603, prod-server.ts:777). Good that this is an allowlist rather than a blocklist — any unexpected value falls back to the safe default.
| }), | ||
| }; | ||
| const response = await handleImageOptimization(request, handlers, undefined, { | ||
| contentDispositionType: "bogus" as "inline", |
There was a problem hiding this comment.
Good test. The as "inline" cast is the right approach to test the runtime guard while satisfying the type system. This ensures the sanitization works even if the config is loaded from an untrusted source that bypasses TypeScript checks.
| (call) => typeof call[0] === "string" && call[0].includes("external rewrite"), | ||
| ); | ||
| expect(externalWarning).toBeDefined(); | ||
| expect(externalWarning![0]).toContain("3 external rewrites"); |
There was a problem hiding this comment.
Minor: consider also asserting that all three sources appear in the listing, to verify that beforeFiles, afterFiles, and fallback are all being checked:
| expect(externalWarning![0]).toContain("3 external rewrites"); | |
| expect(externalWarning![0]).toContain("3 external rewrites"); | |
| // Verify all three rewrite phases are included | |
| expect(externalWarning![0]).toContain("/proxy1"); | |
| expect(externalWarning![0]).toContain("/proxy2"); | |
| expect(externalWarning![0]).toContain("/proxy3"); |
The destination URLs are already asserted, but asserting the source paths too ensures the source → destination listing format is correct for all phases.
|
Review posted successfully. Here's a summary of my feedback: Overall: Solid, well-scoped PR. Both changes are correct and consistently applied. Key points raised:
|
…rtions Mention route handlers alongside API routes in the warning suggestion, and assert that source paths (/proxy1, /proxy2, /proxy3) appear in the multi-rewrite warning message.
External rewrites via proxyExternalRequest now strip authorization, cookie, proxy-authorization, and x-api-key headers before forwarding to third-party origins. This prevents leaking user credentials to external services. Updated the warning message to reflect that credentials are stripped (not forwarded) and updated tests to assert the new behavior. Closes cloudflare#430
The integration test in app-router.test.ts was missed when c4a128d added credential header stripping to proxyExternalRequest. Assertions now correctly expect credentials to be undefined (stripped), matching the unit test in shims.test.ts and the actual implementation.
|
/bigbonk re-review |
There was a problem hiding this comment.
Review
This PR is well-scoped and addresses two concrete security concerns: credential header stripping on external rewrite proxying, and Content-Disposition sanitization for image responses. Both changes are correct, consistent across codepaths, and well-tested.
Credential header stripping
The key improvement here is that proxyExternalRequest now actually strips authorization, cookie, proxy-authorization, and x-api-key from outbound requests — and the warning message in resolveNextConfig accurately describes this behavior. Earlier revisions of the PR (flagged by prior reviews) had a mismatch where the warning listed headers but the code didn't strip them. That's been resolved.
The CREDENTIAL_HEADERS list in config-matchers.ts:957 and the warning text in next-config.ts:452 are consistent.
Content-Disposition sanitization
The allowlist pattern (=== "attachment" ? "attachment" : "inline") is the right approach — any unexpected or attacker-controlled value falls back to the safe default. Applied consistently across all three codepaths (image-optimization, app router prod server, pages router prod server), and the worker entry delegates to handleImageOptimization so it's covered transitively.
Tests
Good coverage:
- The
shims.test.tschanges correctly flip from "forwards" to "strips" for credential headers - The
app-router.test.tsintegration test verifies the full request flow - The new
next-config.test.tstests cover singular/plural, multi-phase rewrites, internal-only, and no-rewrites cases - The
"bogus" as "inline"Content-Disposition test exercises the runtime guard
Two minor comments below, neither blocking.
|
|
||
| // Strip credential headers to prevent leaking secrets to third-party origins. | ||
| // Users who need to forward credentials should use API routes or route handlers. | ||
| const CREDENTIAL_HEADERS = ["authorization", "cookie", "proxy-authorization", "x-api-key"]; |
There was a problem hiding this comment.
Nit: CREDENTIAL_HEADERS is re-allocated on every call to proxyExternalRequest. Since this is a constant, consider hoisting it to module scope (similar to HOP_BY_HOP_HEADERS at line 161) so it's allocated once:
| const CREDENTIAL_HEADERS = ["authorization", "cookie", "proxy-authorization", "x-api-key"]; | |
| const CREDENTIAL_HEADERS = ["authorization", "cookie", "proxy-authorization", "x-api-key"]; |
(moved to module level, and the inline const on line 957 removed)
Not a correctness issue — just a minor consistency/efficiency thing.
| `${listing}\n` + | ||
| `Credential headers (cookie, authorization, proxy-authorization, x-api-key) are stripped from outbound requests. ` + | ||
| `All other request headers are still forwarded. ` + | ||
| `If you need to forward credentials to the external origin, use an API route or route handler where you control exactly which headers are sent.`, |
There was a problem hiding this comment.
The guidance here is good and actionable. One thought: the message says "use an API route or route handler where you control exactly which headers are sent" — it might be worth adding that credentials can be injected server-side in the API route (e.g., from environment variables or a secrets store) rather than forwarded from the client request. That makes the security benefit more concrete for developers who aren't sure why an API route is better.
Not blocking — the current wording is clear enough.
|
Review submitted with approval. Here's a summary of what I found: Approved -- this is a clean, well-scoped security PR. Key findings:
Two minor (non-blocking) comments left:
|
Summary
cookie,authorization,proxy-authorization,x-api-key) from outbound proxy requests to external originsnext.configcontains rewrites to external origins, noting that credential headers are strippedcontentDispositionTypein image optimization and prod server to only allow"attachment"(default"inline")Closes #414
Closes #430
Test plan