fix: handle multiple `x-forwarded-proto` headers #58824

Jonas-PFX · 2023-11-23T10:54:28Z

What?

This PR changes how protocol is determined.
A change was recently made (in PR 57815) that did not take into account cases where there are multiple x-forwarded-proto headers. In such cases, the protocol becomes e.g "https, https".

Why?

An error will occur in parseUrl on line 1616, since its not a valid url (e.g. https, https://localhost:3000).

How?

Reverted part of the changes in PR 57815.

Fixes #58764 and fixes #59031

Closes NEXT-2437

…ed-proto header.

Bahlaouane-Hamza · 2023-11-28T19:25:00Z

Hey @Jonas-PFX can you add a fix for this one too please ? #59031
Same story, It's this line https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/router-utils/resolve-routes.ts#L140

Jonas-PFX · 2023-11-29T08:06:10Z

Hey @Jonas-PFX can you add a fix for this one too please ? #59031 Same story, It's this line https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/router-utils/resolve-routes.ts#L140

done 🙂

barikhan1986 · 2023-12-10T10:05:10Z

Still the same issue in latest stable version 14.0.4

barikhan1986 · 2023-12-22T07:33:02Z

what is update on this? when you can merge it? its really a big issue

Jonas-PFX · 2023-12-22T09:17:28Z

what is update on this? when you can merge it? its really a big issue

Yeah its a big problem for me too. I was hoping it would be faster, but the PR is still awaiting review.
Perhaps @huozhi has time to take a look ?

aboda1986 · 2023-12-28T09:19:00Z

I have the same issue, i'm stuck in 14.0.1 , cannot upgrade it to latest version, hopefully they will fix it in next stable version :(

barikhan1986 · 2023-12-30T20:17:34Z

temp solution: create express js app

`
const express = require('express');
const { createProxyMiddleware } = require('http-proxy-middleware');

const app = express();
const target = 'http://localhost:3005'; // Replace with your Next.js app's URL

app.use('/', createProxyMiddleware({
target,
changeOrigin: true,
onProxyReq(proxyReq, req, res) {
proxyReq.removeHeader('X-Forwarded-Proto');
},
}));

app.listen(8080, () => {
console.log('Proxy server listening on port 8080');
});
`

ritesh-cyble · 2024-01-03T09:04:35Z

This issue is breaking the production and we are unable to migrate from v13.5.6 to v14.0.4.

@Jonas-PFX I have a small suggestion regarding the changes.
If you check the changes done in PR 57815, the code was moved to set these headers up to base-server.ts so they are present in Middleware too.

So, we can add the changes done by you directly in base-server.ts, then we will not need to do the changes in both the places.

I am also attaching the screenshots for debugging the issue in the code for NextJS 14.0.4 -

ijjk · 2024-02-12T11:47:54Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
buildDuration	19.9s	19.7s	N/A
buildDurationCached	8.6s	6.9s	N/A
nodeModulesSize	196 MB	196 MB	⚠️ +2.2 kB
nextStartRea..uration (ms)	425ms	442ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
1068-HASH.js gzip	30 kB	30 kB	N/A
3f784ff6-HASH.js gzip	53.5 kB	53.5 kB	N/A
4944-HASH.js gzip	4.96 kB	4.96 kB	N/A
8423.HASH.js gzip	181 B	181 B	✓
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	241 B	242 B	N/A
main-HASH.js gzip	32 kB	32 kB	N/A
webpack-HASH.js gzip	1.7 kB	1.7 kB	✓
Overall change	47.1 kB	47.1 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
_app-HASH.js gzip	196 B	196 B	✓
_error-HASH.js gzip	184 B	183 B	N/A
amp-HASH.js gzip	503 B	504 B	N/A
css-HASH.js gzip	323 B	324 B	N/A
dynamic-HASH.js gzip	2.5 kB	2.51 kB	N/A
edge-ssr-HASH.js gzip	258 B	259 B	N/A
head-HASH.js gzip	353 B	351 B	N/A
hooks-HASH.js gzip	370 B	370 B	✓
image-HASH.js gzip	4.21 kB	4.2 kB	N/A
index-HASH.js gzip	259 B	259 B	✓
link-HASH.js gzip	2.68 kB	2.67 kB	N/A
routerDirect..HASH.js gzip	313 B	314 B	N/A
script-HASH.js gzip	386 B	385 B	N/A
withRouter-HASH.js gzip	309 B	311 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	931 B	931 B	✓

Client Build Manifests

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
_buildManifest.js gzip	485 B	484 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
index.html gzip	529 B	527 B	N/A
link.html gzip	541 B	540 B	N/A
withRouter.html gzip	525 B	522 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
edge-ssr.js gzip	94.4 kB	94.4 kB	N/A
page.js gzip	151 kB	151 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
middleware-b..fest.js gzip	625 B	626 B	N/A
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	44.4 kB	44.4 kB	✓
edge-runtime..pack.js gzip	1.94 kB	1.94 kB	✓
Overall change	46.5 kB	46.5 kB	✓

Next Runtimes

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
app-page-exp...dev.js gzip	166 kB	166 kB	✓
app-page-exp..prod.js gzip	95.5 kB	95.5 kB	✓
app-page-tur..prod.js gzip	97.2 kB	97.2 kB	✓
app-page-tur..prod.js gzip	91.7 kB	91.7 kB	✓
app-page.run...dev.js gzip	136 kB	136 kB	✓
app-page.run..prod.js gzip	90.2 kB	90.2 kB	✓
app-route-ex...dev.js gzip	22 kB	22 kB	✓
app-route-ex..prod.js gzip	14.9 kB	14.9 kB	✓
app-route-tu..prod.js gzip	14.9 kB	14.9 kB	✓
app-route-tu..prod.js gzip	14.6 kB	14.6 kB	✓
app-route.ru...dev.js gzip	21.7 kB	21.7 kB	✓
app-route.ru..prod.js gzip	14.6 kB	14.6 kB	✓
pages-api-tu..prod.js gzip	9.43 kB	9.43 kB	✓
pages-api.ru...dev.js gzip	9.7 kB	9.7 kB	✓
pages-api.ru..prod.js gzip	9.43 kB	9.43 kB	✓
pages-turbo...prod.js gzip	22 kB	22 kB	✓
pages.runtim...dev.js gzip	22.7 kB	22.7 kB	✓
pages.runtim..prod.js gzip	22 kB	22 kB	✓
server.runti..prod.js gzip	50 kB	50 kB	N/A
Overall change	875 kB	875 kB	✓

build cache

	vercel/next.js canary	Jonas-PFX/next.js protocol-bug-with-multiple-headers	Change
0.pack gzip	1.55 MB	1.55 MB	N/A
index.pack gzip	104 kB	104 kB	N/A
Overall change	0 B	0 B	✓

Diff details

Diff for server.runtime.prod.js

Diff too large to display

Commit: 3fcd0e6

ritesh-cyble · 2024-02-12T13:51:43Z

test/e2e/app-dir/x-forwarded-headers/x-forwarded-headers.test.ts

+      expect(res.status).toBe(200)
+
+      const headers = await res.json()
+      expect(headers['x-forwarded-proto']).toBe('https, https')


@balazsorban44 Just confirming if this value should be just 'https' or 'https, https'

I assume we don't want to modify the original header value, we just want to set the correct protocol for the server.

The behavior is not new in this PR, it just now supports passing a concatenated value as x-forwarded-proto, without causing a 400 response

Thanks for the explanation 👍

ijjk · 2024-02-12T13:52:11Z

Tests Passed

ritesh-cyble · 2024-02-14T15:04:35Z

Thanks a lot @balazsorban44 🥇

excplicitly using https or http instead of the value inside x-forward…

069aaa0

…ed-proto header.

ijjk added the type: next label Nov 23, 2023

added nullcheck

0567a6a

Jonas-PFX marked this pull request as ready for review November 23, 2023 10:58

Jonas-PFX requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, ztanner and wyattjoh as code owners November 23, 2023 10:58

This was referenced Nov 27, 2023

TypeError: Invalid URL when request: NextRequest is called in app router api route in production #58914

Open

x-forwarded-host header does not match origin #58295

Closed

fixed protocol resolution in resolve-routes.ts

1841546

This comment was marked as off-topic.

Sign in to view

Merge branch 'canary' into protocol-bug-with-multiple-headers

3dd958d

balazsorban44 added 2 commits February 12, 2024 14:43

fix linting

a1cd8ea

add test

e663377

ijjk added the area: tests label Feb 12, 2024

ritesh-cyble reviewed Feb 12, 2024

View reviewed changes

drop .only

a8b35cf

balazsorban44 changed the title ~~Fix: duplicate protocol in initUrl when multiple x-forwarded-proto headers~~ fix: handle multiple x-forwarded-proto headers Feb 12, 2024

Merge branch 'canary' into protocol-bug-with-multiple-headers

e7a1b3a

balazsorban44 enabled auto-merge (squash) February 13, 2024 21:46

Merge branch 'canary' into protocol-bug-with-multiple-headers

3fcd0e6

balazsorban44 approved these changes Feb 14, 2024

View reviewed changes

balazsorban44 merged commit 7744cc9 into vercel:canary Feb 14, 2024
69 checks passed

github-actions bot added the locked label Feb 29, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle multiple `x-forwarded-proto` headers #58824

fix: handle multiple `x-forwarded-proto` headers #58824

Jonas-PFX commented Nov 23, 2023 •

edited by balazsorban44

Bahlaouane-Hamza commented Nov 28, 2023 •

edited

Jonas-PFX commented Nov 29, 2023

barikhan1986 commented Dec 10, 2023

barikhan1986 commented Dec 22, 2023

Jonas-PFX commented Dec 22, 2023

aboda1986 commented Dec 28, 2023

barikhan1986 commented Dec 30, 2023 •

edited

ritesh-cyble commented Jan 3, 2024

This comment was marked as off-topic.

ijjk commented Feb 12, 2024 •

edited

ritesh-cyble Feb 12, 2024

balazsorban44 Feb 12, 2024 •

edited

ritesh-cyble Feb 12, 2024

ijjk commented Feb 12, 2024 •

edited

ritesh-cyble commented Feb 14, 2024

fix: handle multiple x-forwarded-proto headers #58824

fix: handle multiple x-forwarded-proto headers #58824

Conversation

Jonas-PFX commented Nov 23, 2023 • edited by balazsorban44

What?

Why?

How?

Bahlaouane-Hamza commented Nov 28, 2023 • edited

Jonas-PFX commented Nov 29, 2023

barikhan1986 commented Dec 10, 2023

barikhan1986 commented Dec 22, 2023

Jonas-PFX commented Dec 22, 2023

aboda1986 commented Dec 28, 2023

barikhan1986 commented Dec 30, 2023 • edited

ritesh-cyble commented Jan 3, 2024

This comment was marked as off-topic.

ijjk commented Feb 12, 2024 • edited

Stats from current PR

ritesh-cyble Feb 12, 2024

Choose a reason for hiding this comment

balazsorban44 Feb 12, 2024 • edited

Choose a reason for hiding this comment

ritesh-cyble Feb 12, 2024

Choose a reason for hiding this comment

ijjk commented Feb 12, 2024 • edited

Tests Passed

ritesh-cyble commented Feb 14, 2024

fix: handle multiple `x-forwarded-proto` headers #58824

fix: handle multiple `x-forwarded-proto` headers #58824

Jonas-PFX commented Nov 23, 2023 •

edited by balazsorban44

Bahlaouane-Hamza commented Nov 28, 2023 •

edited

barikhan1986 commented Dec 30, 2023 •

edited

ijjk commented Feb 12, 2024 •

edited

balazsorban44 Feb 12, 2024 •

edited

ijjk commented Feb 12, 2024 •

edited