Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: don't include port twice from x-forwarded-host and x-forwarded-port headers #10917

Merged
merged 5 commits into from
May 3, 2024
Merged

fix: don't include port twice from x-forwarded-host and x-forwarded-port headers #10917

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
03cd454
fix: don't include port twice from x-forwarded-host and x-forwarded-p…
jakobhellermann Apr 30, 2024
56ca10c
add changeset
jakobhellermann Apr 30, 2024
892936d
add test for port both in forwarded host and forwarded port
jakobhellermann Apr 30, 2024
b999f7e
don't include port if undefined
jakobhellermann Apr 30, 2024
a6ebe3c
Update .changeset/forty-wolves-turn.md
jakobhellermann May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/forty-wolves-turn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

Fix internal server error when bot `X-Forwarded-Host: hostname:8080` and `X-Forwarded-Port: 8080` are set
jakobhellermann marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 5 additions & 1 deletion packages/astro/src/core/app/node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

@jakobhellermann jakobhellermann Apr 30, 2024 •
edited
Loading

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?

const hostnamePort = portInHostname ? hostname : `${hostname}:${port}`;
Copy link

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.


const url = `${protocol}://${hostnamePort}${req.url}`;
const options: RequestInit = {
method: req.method || 'GET',
headers: makeRequestHeaders(req),
Expand Down
Loading