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 |
github-actions
bot
added
the
pkg: astro
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.
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?
github-actions
bot
added
the
pkg: integration
|
The code still fails with a 500 for curl localhost:4321 -H X-Forwarded-Host: hostname:1111 -H X-Forwarded-Port: 9999Maybe that's fine? It's obviously malformed. |
|
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.
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:8080andX-Forwarded-Port: 8080are 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.