fix: stop stripping credential headers from rewrite proxy requests#277
Conversation
|
/bigbonk review |
commit: |
|
cc @threepointone - i believe this was related to the security advisory changes |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The behavioral change here is correct — Next.js does not strip credential headers from rewrite proxy requests, and vinext should match that behavior. However, this PR has two problems that need to be addressed before merging.
1. CI is failing: existing tests assert the old behavior
Vitest is failing on both tests/shims.test.ts (line 3707) and tests/app-router.test.ts (line 2748). Both tests explicitly assert that cookie, authorization, x-api-key, and proxy-authorization are stripped from proxy requests — which is exactly the behavior this PR removes.
These tests need to be updated to assert the new behavior: that credential headers are forwarded through the proxy. Specifically:
tests/shims.test.ts:3707— "strips credentials and x-middleware-* headers from proxied requests" should be renamed and updated to verify thatcookie,authorization,x-api-keyare preserved, whilex-middleware-*headers are still stripped.tests/app-router.test.ts:2748— "strips credential headers from proxied requests to external rewrite targets" — same treatment.
2. Missing parity check: Pages Router prod server
Per AGENTS.md, bug fixes must check dev/prod server parity. The prod server (server/prod-server.ts) and the Cloudflare worker entry (generated by deploy.ts) both delegate to proxyExternalRequest from config-matchers.ts, so they get the fix automatically. The Pages Router dev server (server/dev-server.ts) has no proxy logic, so it's not affected. This is fine — just confirming the parity check was done.
Aside: proxy-authorization as a hop-by-hop header
Note that proxy-authorization is listed in HOP_BY_HOP_HEADERS (config-matchers.ts:15), which is used to strip hop-by-hop headers from upstream responses. This is correct per RFC 2616 — proxy-authorization is a hop-by-hop header that should not be forwarded in responses. But this is orthogonal to the request-side forwarding that this PR addresses. Just flagging for clarity: the request-side proxy-authorization will now be forwarded (matching Next.js), while the response-side is still correctly stripped.
| @@ -606,12 +606,6 @@ export async function proxyExternalRequest( | |||
| headers.set("host", targetUrl.host); | |||
| // Remove headers that should not be forwarded to external services | |||
| headers.delete("connection"); | |||
There was a problem hiding this comment.
The credential-stripping removal here is correct — Next.js forwards all request headers (including cookie, authorization, etc.) to rewrite destinations via http-proxy with no filtering.
However, the existing test at tests/shims.test.ts:3707 ("strips credentials and x-middleware-* headers from proxied requests") asserts the old behavior and is now failing in CI. Please update it to verify that credential headers are preserved while x-middleware-* headers remain stripped.
| @@ -1185,12 +1185,6 @@ async function __proxyExternalRequest(request, externalUrl) { | |||
| const headers = new Headers(request.headers); | |||
| headers.set("host", targetUrl.host); | |||
| headers.delete("connection"); | |||
There was a problem hiding this comment.
Same as above — correct change, but the integration test at tests/app-router.test.ts:2748 ("strips credential headers from proxied requests to external rewrite targets") is now failing. It needs to be updated to match the new forwarding behavior.
|
Review submitted. Here's a summary of what I found: The behavioral change is correct — I verified that Next.js does not strip credential headers from rewrite proxy requests. It uses However, I requested changes because:
|
Rewrite destinations are typically the user's own backend services, not untrusted third-party origins. Stripping cookie, authorization, x-api-key, and proxy-authorization headers breaks authentication for proxied API requests. Next.js does not strip these headers for rewrites either — it forwards them as-is to the destination.
Update tests to match new behavior where credential headers (cookie, authorization, x-api-key, proxy-authorization) are forwarded through the rewrite proxy, matching Next.js behavior. x-middleware-* headers are still stripped.
southpolesteve
force-pushed
the
fix/preserve-credentials-proxy
branch
from
760c0f4 to
ae52c07
Compare
southpolesteve
dismissed
ask-bonk[bot]’s stale review
All feedback addressed: tests updated to assert forwarding behavior, parity confirmed, conflicts resolved after rebase onto main.
3 participants
Fixes #264
Problem
proxyExternalRequestand__proxyExternalRequestunconditionally stripcookie,authorization,x-api-key, andproxy-authorizationheaders from all proxied rewrite requests. This breaks authentication for the most common use case — proxying to the user's own backend.Context
Next.js does not strip these headers for rewrites. It forwards all request headers as-is to the rewrite destination. vinext's blanket stripping is a deviation from Next.js behavior that breaks real-world usage.
Fix
Remove the credential-stripping logic from both:
config-matchers.ts—proxyExternalRequest(used by prod server)app-dev-server.ts—__proxyExternalRequest(inlined in dev server virtual module)Internal
x-middleware-*headers are still stripped as before.