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.

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

After updating to v3.11.1 rewrite/redirects append a port to the URL #987

Closed
3 tasks done
pepijn-vanvlaanderen opened this issue Apr 8, 2024 · 19 comments
Closed
3 tasks done
Labels
bug Something isn't working

Comments

@pepijn-vanvlaanderen
Copy link

Description

The last change for internal redirecting appends a port to the URL. However in our setup at Railway.app this does a faulty redirect. When the middleware should rewrite / redirect, it takes a long time and eventually redirects to the correct URL but with the port number and that crashes.

Reverting to v3.11.0 works.

Verifications

  • I've verified that the problem I'm experiencing isn't covered in the docs.
  • I've searched for similar, existing issues on GitHub and Stack Overflow.
  • I've compared my app to a working example to look for differences.

Mandatory reproduction URL

https://yeahgummy.webbers.dev/cart

Reproduction description

Steps to reproduce:

  1. Open reproduction
  2. Wait about a minute or so
  3. See error: the page does not work and is redirected to https://yeahgummy.webbers.dev:3000/winkelwagen

Expected behaviour

A redirect to https://yeahgummy.webbers.dev/winkelwagen showing a page.

@pepijn-vanvlaanderen pepijn-vanvlaanderen added bug Something isn't working unconfirmed Needs triage. labels Apr 8, 2024
@awkaiser-tr
Copy link
Contributor

awkaiser-tr commented Apr 8, 2024

@pepijn-vanvlaanderen your suggestion of config to control what port is used may be the way to go, but it's also possible that the current logic is suboptimal. To troubleshoot further, what request headers are available to Next.js? Do you have values for any of the x-forwarded-* headers, e.g.

... or a forwarded header?

@amannn
Copy link
Owner

amannn commented Apr 8, 2024

@pepijn-vanvlaanderen Sorry for the inconvenience caused by this change!

But yes, as @awkaiser-tr has pointed out, it would be important to get details about the specific request headers your app receives. Based on that, we can add a failing test like @awkaiser-tr did in #979 and look into an appropriate fix.

I'd really like to avoid config as much as possible if we can, therefore the best option from my perspective would be to just do the right thing based on the information we have available.

Can you also share your domains config?

@awkaiser-tr Can you too share your domains config? Maybe we could also utilize a port that is potentially defined there for the detection.

@awkaiser-tr
Copy link
Contributor

My domains config looks like:

{
  defaultLocale: 'de-DE',
  domain: getDomain('de-DE').host,
  locales: ['de-DE'],
},
{
  defaultLocale: 'en-CA',
  domain: getDomain('en-CA').host,
  locales: ['en-CA', 'fr-CA'],
},
{
  defaultLocale: 'en-GB',
  domain: getDomain('en-GB').host,
  locales: ['en-GB'],
},
// ... and so on ...

... where host is the locale domain for current Next.js environment, e.g.

  • development @ de-yourcompany-dev.localhost
  • production @ www.yourcompany.de

@awkaiser-tr
Copy link
Contributor

awkaiser-tr commented Apr 8, 2024

@amannn are you suggesting an optional port property on the per-domain config?

Since most deployments will be reached via standard HTTP (80) / HTTPS (443) port, treating non-standard ports as outliers that require config makes sense and would probably result in fewer issues filed on this project.1

However, if DomainConfig were to begin caring about port, it would also need to consider protocol:

{ // For sake of discussion ...
  defaultLocale: 'en-CA',
  domain: getDomain('en-CA').host, // `'www.yourcompany.ca'` || `'ca-yourcompany-dev.localhost'`
  locales: ['en-CA', 'fr-CA'],
  protocol: getDomain('en-CA').protocol, // `'https://'` || `'http://'`
  port: getDomain('en-CA').port, // `undefined` || `3000`
},

... where by default next-intl redirects use Next.js' request protocol and omit port, overridden by DomainConfig values first and proxy headers in absence of that config.

Footnotes

  1. Do not many people test domain-based l10n during development on alternative ports like Next.js' default dev port 3000? 🤔

@amannn
Copy link
Owner

amannn commented Apr 9, 2024

Yep, that's correct. We currently use the protocol and port of an incoming request to determine wether to add it to a domain that we're redirecting to.

I was wondering if the port could even be part of domain (e.g. domain: 'www.yourcompany.de:3000') to avoid introducing new properties, but I guess that would be somewhat of a stretch for the term "domain". A separate property like you're suggesting might be the most explicit option. EDIT: Seems like we've silently already supported this, so maybe this is the way to go and we should document it.

An explicit port in the domain config should also give users more flexibility, maybe one domain runs on a different port than another domain. I think this might be a reasonable path forward without having to rely on detecting a reverse proxy, which seemingly people have different configurations for.

What do you think?

@pepijn-vanvlaanderen
Copy link
Author

Hi @amannn , hereby my domains config:

    localePrefix: 'as-needed',
    localeDetection: false,
    locales: ['nl'],
    defaultLocale: 'nl',
    pathnames: {
        ...,
        '/cart': {
            en: '/cart',
            nl: '/winkelwagen',
        },
        ...
    },
    domains: [
        {
            domain: `${process.env.DOMAIN_NL ?? 'localhost:3000'}`,
            defaultLocale: 'nl',
        },
    ],

As you can see, we currently only support 1 language as we are still waiting on translated content, but internally we use next-intl already to translate short texts.

@awkaiser-tr how does your getDomain look like? I was as well wondering for an easy ussage on local developing here.

@amannn amannn removed the unconfirmed Needs triage. label Apr 9, 2024
@awkaiser-tr
Copy link
Contributor

Local development w/ locale-based domains

@pepijn-vanvlaanderen the getDomain function returns { host, origin, pathname, port, protocol } by locale, with values determined by environment. You just need to associate some locale-specific domains with your loopback address, e.g. /etc/hosts:

127.0.0.1 ca-yourcompany-dev.localhost
127.0.0.1 de-yourcompany-dev.localhost
127.0.0.1 es-yourcompany-dev.localhost
127.0.0.1 fr-yourcompany-dev.localhost
127.0.0.1 it-yourcompany-dev.localhost
127.0.0.1 pt-yourcompany-dev.localhost
127.0.0.1 uk-yourcompany-dev.localhost
127.0.0.1 us-yourcompany-dev.localhost
127.0.0.1 yourcompany-dev.localhost

@awkaiser-tr
Copy link
Contributor

@amannn: I was wondering if the port could even be part of domain (e.g. domain: 'www.yourcompany.de:3000') to avoid introducing new properties, but I guess that would be somewhat of a stretch for the term "domain".

Agreed, that would be overloading domain in a surprising way. 🙅🏻

A separate property like you're suggesting might be the most explicit option. EDIT: Seems like we've silently already supported this, so maybe this is the way to go and we should document it.

An explicit port in the domain config should also give users more flexibility, maybe one domain runs on a different port than another domain. I think this might be a reasonable path forward without having to rely on detecting a reverse proxy, which seemingly people have different configurations for.

What do you think?

Adding port to DomainConfig would be fine, so long as protocol (e.g. https://, http://) is also available. A secure or insecure web server can listen on any available port, and the protocol of an internal Next.js server could mismatch its public-facing reverse proxy, so we can't make assumptions about protocol with no way to override. Presumably this also means consideration of proxy-related headers would be removed? Leaving it up to the user, with sensible defaults in absence of DomainConfig values, does feel like the best choice. If they want to set config values based on incoming headers, they'd be free to do that.

@amannn
Copy link
Owner

amannn commented Apr 12, 2024

@pepijn-vanvlaanderen Can you share which request headers your app receives in your Railway setup? Maybe you can add some logging in the middleware?

I did some research and it seems like Railway uses Envoy, which in turn should support x-forwarded-host and x-forwarded-port.

Supporting different ports for different domains as discussed above could be interesting, but for now, the easiest fix might be to apply ports correctly when running behind a reverse proxy. It would be helpful to learn if Railway is accidentally stripping important headers here of if the middleware logic of next-intl has a bug.

With the fix from @awkaiser-tr in #979 we now have this logic:

  1. If a x-forwarded-host is present, we assume that the app runs behind a reverse proxy
  2. If the app runs behind a reverse proxy, we apply x-forwarded-proto and x-forwarded-port to a domain we're potentially redirecting to. If these headers are absent, we default to request.nextUrl.protocol and no port.

It would be interesting to learn about the exact values of these request headers in your app to diagnose where the issue is. Can you share them? Thanks!

@pepijn-vanvlaanderen
Copy link
Author

Hi @amannn, after some testing I see now that the issue occurs on the pathname rewrite. So i.e. I have in pathnames this:

pathnames: {
    '/cart': : {
        de: '/cart',
        en: '/cart',
        fr: '/cart',
        it: '/cart', 
        nl: '/winkelwagen',
    },
}

For all (custom) domains it will work, except for the nl (Dutch) variant. It will take some long time loading and end up at
https://yeahgummy-de.webbers.dev/cart -> https://yeahgummy-de.webbers.dev/cart
https://yeahgummy-fr.webbers.dev/cart -> https://yeahgummy-fr.webbers.dev/cart
https://yeahgummy-it.webbers.dev/cart -> https://yeahgummy-it.webbers.dev/cart
https://yeahgummy-nl.webbers.dev/cart -> https://yeahgummy-nl.webbers.dev:3000/winkelmand

When logging the x-forwarded-host and x-forwarded-port on Railway, I get these results:

xForwardedHost: yeahgummy-fr.webbers.dev
xForwardedPort: 3000

@awkaiser-tr
Copy link
Contributor

@amannn funny enough, I just experienced the same bug RE: localized pathname redirects 💣

@pepijn-vanvlaanderen quick sanity check: I see you're reporting x-forwarded-port as 3000, but was that just for the https://yeahgummy-nl.webbers.dev:3000/winkelmand request? What is the value of the x-forwarded-* headers for the /cart requests?

@pepijn-vanvlaanderen
Copy link
Author

@awkaiser-tr for all the requests the port is set as 3000 and the domain I have set per locale as the host.

@TomasLukes
Copy link

TomasLukes commented Apr 30, 2024

@awkaiser-tr for all the requests the port is set as 3000 and the domain I have set per locale as the host.

Same problem for us, when updating to 3.11.1, for all locales:

After update (port added to URL):
https://something.cz/complaints => https://something.cz:3000/reklamace
https://something.pl/complaints => https://something.pl:3000/formularz-reklamacyjny

Before update (correct pathname translation for locale):
https://something.cz/complaints => https://something.cz/reklamace
https://something.pl/complaints => https://something.pl/formularz-reklamacyjny

@amannn
Copy link
Owner

amannn commented Apr 30, 2024

@pepijn-vanvlaanderen Are you saying that for a request to https://yeahgummy-nl.webbers.dev/cart, you're seeing x-forwarded-port: 3000? If so, that sounds like a bug of the reverse proxy to me, x-forwarded-port should contain the value of the port of the original request (443; standard https port).

See also:

Load Balancing with Port Preservation: In a load-balanced environment (NGINX) where the backend servers are listening on different ports, the X-Forwarded-Port header helps the load balancer preserve and convey the original client port information to the server. This information can be used by the server for various purposes, such as generating absolute URLs.

(source)

Can you verify this with Railway?

@awkaiser-tr Does this match your understanding too?

@TomasLukes Which hosting provider are you using? Can you also double check what the value of x-forwarded-port is when https://something.pl/complaints hits your middleware? The correct value should be 443.

@pepijn-vanvlaanderen
Copy link
Author

Hi @amannn, thanks for your message. Actually I think the issue then is probably how I had setup the Dockerfile. There is a reverse proxy in front of the container and inside the container it then uses the mentioned port. Maybe Railway should superseed that from the proxy, but in the meantime I was just able to expose port 443 in the dockerfile instead of 3000 and it works now, thanks!

@awkaiser-tr
Copy link
Contributor

@amannn: Does this match your understanding too?

Yup.


@pepijn-vanvlaanderen when you say "it works now" do you mean that, in addition to your other routes, the https://yeahgummy-nl.webbers.dev/cart localized pathname redirect also now leads to https://yeahgummy-nl.webbers.dev/winkelmand as expected?

This GitHub Issue has been confused since details have changed over time:

  1. Undesired port always added to redirects
  2. Specifically, localized pathname redirects include undesired port

@TomasLukes
Copy link

@TomasLukes Which hosting provider are you using? Can you also double check what the value of x-forwarded-port is when https://something.pl/complaints hits your middleware? The correct value should be 443.

Thanks for quick help!👌 "x-forwarded-port" value in middleware is 3000 now. Should I do something like this?
const modifiedReq = req.headers.set("x-forwarded-port", "443");

It looks like its behaving correctly after that 🤔

@awkaiser-tr
Copy link
Contributor

@TomasLukes just as Pepijn did, I'd recommend digging deeper and determining why the x-forwarded-port header is being set to 3000 and correcting that before the request reaches Next.js

@pepijn-vanvlaanderen
Copy link
Author

@awkaiser-tr yes the redirect also leads to localized pathnames correctly. The undesired port issue originated from a bad config.

I exposed port number 3000 in the production environment before, and thats why it redirected to the domain with port addition after reading x-forwarded-port . So in the end I set the port in Railway to 443, which resulted in a proper config and working redirects. Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants