withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
46.81k stars 2.49k forks source link

Port changes in the latest node integration release are causing production failure #12211

Closed kamranahmedse closed 1 month ago

kamranahmedse commented 1 month ago

Astro Info

Astro                    v4.16.1
Node                     v20.5.1
System                   macOS (arm64)
Package Manager          npm
Output                   hybrid
Adapter                  @astrojs/node
Integrations             @astrojs/tailwind
                         @astrojs/sitemap
                         @astrojs/react

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

This recent change which is always adding req.socket.remotePort to the URL has caused our production app to have timeouts whenever a non-existent URL is accessed (i.e. 404 page is never rendered).

We have the following setup in production

Cloudfront -> Nginx -> Astro SSR

Since the request is being proxied through CloudFront and Nginx, the req.socket.remotePort value would correspond to the port used by Nginx (which is forwarding the request to the Node.js server) rather than the client's original port. Typically, this port is dynamically assigned by Nginx for each outgoing connection when proxying requests, so the value would depend on the specific connection instance Nginx used to communicate with the Node.js server.

Step 1:
======
Let's say user accesses  https://roadmap.sh

Step 2:
======
Here is how the request flow will look like:
— Port 443 —> [Cloudfront] — Port 443 —> [Nginx] — Port 45494 —> [Astro SSR]
                                                         ^ randomly assigned port

Step 3:
=====
Astro tries to fetch any of the pages with req.socket.remotePort added to the URL i.e. below URL

https://roadmap.sh:45494

It will timeout because 45494 is not available when accessing the website through Cloudfront

I changed my Nginx configuration to add X-Forwarded-Port which has fixed it for us but I still think enforcing req.socket.remotePort is not the right thing to do in the framework since X-Forwarded-Port is not always present by default.

What's the expected result?

Proxied setup should work fine.

Link to Minimal Reproducible Example

Can't add because it only happens in the proxy setup I detailed in the issue.

Participation

bluwy commented 1 month ago

I think this is the same as https://github.com/withastro/astro/issues/12192. I'll close this in favour of that for now, but this is indeed an urgent issue that we'll look into as soon as possible on Monday. Sorry for the regression!