-
Notifications
You must be signed in to change notification settings - Fork 26.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: correctly set x-forwarded-*
in Middleware
#57815
Conversation
Tests Passed |
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | vercel/next.js fix/x-forwarded-headers-middleware | Change | |
---|---|---|---|
buildDuration | 10.5s | 10.6s | |
buildDurationCached | 6s | 6.8s | |
nodeModulesSize | 175 MB | 175 MB | N/A |
nextStartRea..uration (ms) | 401ms | 400ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | vercel/next.js fix/x-forwarded-headers-middleware | Change | |
---|---|---|---|
199-HASH.js gzip | 30 kB | 30 kB | N/A |
3f784ff6-HASH.js gzip | 53.2 kB | 53.2 kB | ✓ |
494.HASH.js gzip | 182 B | 182 B | ✓ |
framework-HASH.js gzip | 45.5 kB | 45.5 kB | ✓ |
main-app-HASH.js gzip | 253 B | 252 B | N/A |
main-HASH.js gzip | 33.1 kB | 33.1 kB | N/A |
webpack-HASH.js gzip | 1.75 kB | 1.75 kB | N/A |
Overall change | 98.9 kB | 98.9 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | vercel/next.js fix/x-forwarded-headers-middleware | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | vercel/next.js fix/x-forwarded-headers-middleware | Change | |
---|---|---|---|
_app-HASH.js gzip | 205 B | 205 B | ✓ |
_error-HASH.js gzip | 182 B | 181 B | N/A |
amp-HASH.js gzip | 505 B | 507 B | N/A |
css-HASH.js gzip | 322 B | 323 B | N/A |
dynamic-HASH.js gzip | 2.59 kB | 2.59 kB | N/A |
edge-ssr-HASH.js gzip | 258 B | 259 B | N/A |
head-HASH.js gzip | 348 B | 347 B | N/A |
hooks-HASH.js gzip | 369 B | 368 B | N/A |
image-HASH.js gzip | 4.38 kB | 4.38 kB | N/A |
index-HASH.js gzip | 256 B | 256 B | ✓ |
link-HASH.js gzip | 2.67 kB | 2.67 kB | N/A |
routerDirect..HASH.js gzip | 318 B | 318 B | ✓ |
script-HASH.js gzip | 384 B | 383 B | N/A |
withRouter-HASH.js gzip | 319 B | 320 B | N/A |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 885 B | 885 B | ✓ |
Client Build Manifests
vercel/next.js canary | vercel/next.js fix/x-forwarded-headers-middleware | Change | |
---|---|---|---|
_buildManifest.js gzip | 484 B | 484 B | ✓ |
Overall change | 484 B | 484 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | vercel/next.js fix/x-forwarded-headers-middleware | Change | |
---|---|---|---|
index.html gzip | 528 B | 527 B | N/A |
link.html gzip | 541 B | 542 B | N/A |
withRouter.html gzip | 524 B | 521 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Edge SSR bundle Size Overall increase ⚠️
vercel/next.js canary | vercel/next.js fix/x-forwarded-headers-middleware | Change | |
---|---|---|---|
edge-ssr.js gzip | 96.1 kB | 96.2 kB | |
page.js gzip | 140 kB | 140 kB | |
Overall change | 236 kB | 236 kB |
Middleware size Overall increase ⚠️
vercel/next.js canary | vercel/next.js fix/x-forwarded-headers-middleware | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 625 B | 627 B | N/A |
middleware-r..fest.js gzip | 148 B | 151 B | N/A |
middleware.js gzip | 23 kB | 24.8 kB | |
edge-runtime..pack.js gzip | 1.92 kB | 1.92 kB | ✓ |
Overall change | 25 kB | 26.8 kB |
Diff details
Diff for page.js
Diff too large to display
Diff for middleware.js
Diff too large to display
Diff for edge-ssr.js
Diff too large to display
Diff for main-HASH.js
Diff too large to display
…om/vercel/next.js into fix/x-forwarded-headers-middleware
@@ -24,7 +24,6 @@ export async function proxyRequest( | |||
target, | |||
changeOrigin: true, | |||
ignorePath: true, | |||
xfwd: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With xfwd: true
, http-proxy
attempts to set x-forwarded-*
headers. Since we are setting these headers higher up in base-server.ts
, this should not be necessary to set here anymore, to avoid double setting them, as reported in #52266
ws: true, | ||
// we limit proxy requests to 30s by default, in development | ||
// we don't time out WebSocket requests to allow proxying | ||
proxyTimeout: proxyTimeout === null ? undefined : proxyTimeout || 30_000, | ||
headers: { | ||
'x-forwarded-host': req.headers.host || '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still set this header to respect #17557, but without concating without adding a comma ,
. See https://github.com/vercel/next.js/pull/57815/files#r1378313718
I believe this is missing an edge case that breaks my application’s behavior—in the old implementation (prior to #52492) the |
Co-authored-by: @BRKalow <bryce@clerk.dev> ### What? A number of our customers have been experiencing issues stemming from an `x-forwarded-host` header that doesn't match the `host` header. ### Why? [This PR](#57815) removes functionality which sets `x-forwarded-host` to `req.headers['host']` and relies solely on the server's hostname and port. This can be seen locally when visiting the app via a localhost subdomain. The `x-forwarded-host` header will remain as `localhost:${port}` while the actual requested host will contain the subdomain. ### Related - #57815 (comment) --------- Co-authored-by: BRKalow <bryce@clerk.dev> Co-authored-by: Zack Tanner <zacktanner@gmail.com>
What?
Follow-up of #56797
While working on this, I noticed that some logic around stripping internal headers was duplicated, so I did some cleanup too.
Why?
In #56797 we set these headers, but it only affected Route Handlers, Middleware is still missing them, which is a regression introduced in #52492
(Related: vercel/next-learn#252)
How?
Move to set these headers up to
base-server.ts
so they are present in Middleware too.Closes NEXT-
Fixes #52266