-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: don't include port twice from x-forwarded-host and x-forwarded-port headers #10917
Conversation
🦋 Changeset detectedLatest commit: a6ebe3c The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/astro/src/core/app/node.ts
Outdated
@@ -66,7 +66,11 @@ export class NodeApp extends App { | |||
const hostname = | |||
req.headers['x-forwarded-host'] ?? req.headers.host ?? req.headers[':authority']; | |||
const port = req.headers['x-forwarded-port']; | |||
const url = `${protocol}://${hostname}${port ? `:${port}` : ''}${req.url}`; | |||
|
|||
const portInHostname = typeof hostname === "string" && typeof port === "string" && hostname.endsWith(port); |
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.
The typeof checks here are a bit ugly but necessary because the types here for hostname
and port
are string | string[] | undefined
.
I'm assuming string[] happens when passing a header multiple times? The code as written before doesn't support that and instead templates an URl like https://hostname1,hostname2:port1,port2
. I don't know what behavior would be correct here, respecting only the last sent header?
The code still fails with a 500 for curl localhost:4321 -H X-Forwarded-Host: hostname:1111 -H X-Forwarded-Port: 9999 Maybe that's fine? It's obviously malformed. |
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.
Great, thanks!
Looks like this does break some tests 🤔 |
packages/astro/src/core/app/node.ts
Outdated
const url = `${protocol}://${hostname}${port ? `:${port}` : ''}${req.url}`; | ||
|
||
const portInHostname = typeof hostname === "string" && typeof port === "string" && hostname.endsWith(port); | ||
const hostnamePort = portInHostname ? hostname : `${hostname}:${port}`; |
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.
This should be const hostnamePort = portInHostname ? hostname : `${hostname}${port ? `:${port}` : ''}`
because port
can still be undefined.
Tests are fixed now |
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
fixes #10916
Changes
X-Forwarded-Host: hostname:8080
andX-Forwarded-Port: 8080
are setTesting
add another test next to the existing URL tests for
X-Forwarded-*
Docs
The docs shouldn't need to be changed since this is just a bugfix and now works as expected.