Fixed parsing of x-forwarded-* headers #12130
Conversation
🦋 Changeset detectedLatest commit: af15d0c 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
|
@bluwy @ematipico Hey, can I ask you please for review? Is there anything else needed/missing in this MR to get this fix released? Thanks |
| const portInHostname = typeof hostname === 'string' && /:\d+$/.test(hostname); | ||
| const hostnamePort = portInHostname ? hostname : `${hostname}:${port}` |
There was a problem hiding this comment.
This isn't part of the PR description; why was this changed? What are we fixing?
There was a problem hiding this comment.
The old code was adding port to URL only when x-forwarded-port header was present and part of host header. This change handles edge-case when the port isn't part of the "host" header, the server listens on a non-standard port and there's no x-forward-port header in req. In that case it should fallback to req.socket.remotePort, default port for protocol in the following order and add it to URL (default ports such as 80 and 443 are hidden in the URL).
Althought, it's unlikely this would happen in a real-world scenario without broken proxy config / missing request headers.
There was a problem hiding this comment.
Thank you for the thorough explanation!
Co-authored-by: Emanuele Stoppa <[email protected]>
Co-authored-by: Emanuele Stoppa <[email protected]>
Hah, as much as I would like your praise I don’t think you meant to tag me :( |
|
After updating to the latest version of Astro, it lists Astro.url.href, Astro.url.origin and Astro.url.host port in the address. So instead of https://www.example.com/ it prints the address with the port https://www.example.com:[RANDOM_PORT]/ Is this the correct behavior? I don't think the production should list the port after the URL, but maybe I'm wrong. |
|
It's probably a regression. @Fakerko, can you open a bug report with a reproduction? @thehansys, would you be up to fixing the bug if it's valid? |
|
@ematipico done, bug report opened on #12192 |
Changes
This MR fixes parsing of
x-forwarded-*headers such asx-forwarded-for,x-forwarded-host,x-forwarded-protoandx-forwarded-port. The current Astro version with node adapter cannot parse multiple values in these headers, for examplex-forwarded-proto: https,http. This issue occurs when you run your Astro server behind two proxies, for example Cloudflare and then Nginx. All requests to SSR pages result in a 500 error in this case. Astro cannot be hosted with certain hosting providers because of this bug.Testing
x-forwarded-proto: https,http" to any SSR page of Astro server with node adapter.Docs