Inconsistent handling of `Accept-Language` header #2524

rudolfbyker · 2023-10-25T11:58:43Z

Update 2024/01/02

See updated reproduction and steps to reproduce at #2524 (comment)

Environment

Working directory: /home/projects/aoniwxvlm.github 13:55:30
Nuxt project info: 13:55:30

Operating System: Linux
Node Version: v18.18.0
Nuxt Version: 3.8.0
CLI Version: 3.9.1
Nitro Version: 2.7.0
Package Manager: [email protected]
Builder: -
User Config: devtools, modules, i18n
Runtime Modules: @nuxtjs/[email protected]
Build Modules: -

Reproduction

https://stackblitz.com/edit/github-fyaxh8

Describe the bug

The root redirect takes the Accept-Language header into account:

curl -H "Accept-Language: fr" -I http://localhost:3000

HTTP/1.1 302 Found
access-control-allow-origin: *
location: /fr
content-type: text/html
date: Wed, 25 Oct 2023 11:49:52 GMT
connection: close

But everything else does not:

curl -H "Accept-Language: fr" -I http://localhost:3000/foo

HTTP/1.1 302 Found
access-control-allow-origin: *
location: /en/foo
content-type: text/html
date: Wed, 25 Oct 2023 11:49:41 GMT
connection: close

This also relates to #2131 because I'm using the prefix strategy, which means I should actually get a 404 on the /foo request. Therefore, I think fixing #2131 will also fix this issue, but not vice versa.

Additional context

No response

Logs

No response

The text was updated successfully, but these errors were encountered:

BobbieGoede · 2023-11-08T12:55:35Z

To redirect using cookies or the accept-language header on routes other than the root route, you need to set detectBrowserLanguage.redirectOn to 'all'. In which case curl -H "Accept-Language: fr" -I http://localhost:3000/foo will redirect to http://localhost:3000/fr/foo. Does this fit your use case?

Disabling browser language detection has some other behavior that seems a bit unintuitive with the prefix strategy, it still redirects to the default language (so technically no detection), I suppose it makes it possible to still render a catchall/404 page instead of returning a plain 404 response.

rudolfbyker · 2023-11-08T20:55:10Z

Thanks for the feedback, and for the work on #2538 !

http://localhost:3000/foo will redirect to http://localhost:3000/fr/foo. Does this fit your use case?

In my use case, the first path segment MUST be the language, and since foo is not a language, it must give 404. Only /en/foo and /fr/foo, etc. must be available.

I'll do some testing with #2538 and give you my feedback, as soon as I can.

rudolfbyker · 2023-11-22T14:21:05Z

I tried #2538 and it seems to make no difference at all.

rudolfbyker · 2023-11-23T10:12:06Z

I updated my repro at https://stackblitz.com/edit/github-fyaxh8 :

It shows all routes on the home page for easier debugging.
I manualy edited node_modules/@nuxtjs/i18n/dist/module.mjs to have includeUprefixedFallback = false; (mimicking the fix from fix: exclude unprefixed routes for strategy prefix #2538).

Results:

The unprefixed routes are gone (this is an improvement, thanks!)
The handling of the Accept-Language header is still inconsistent (relates to this issue)
I still have access to /foo, which should be 404 instead of a redirect (relates to issue With strategy prefix, unprefixed routes are accessible #2131)

/ redirects to /fr as expected:

~/projects/github-fyaxh8
❯ curl -H "Accept-Language: fr" http://localhost:3000
<!DOCTYPE html><html><head><meta http-equiv="refresh" content="0; url=/fr"></head></html>
~/projects/github-fyaxh8

/asdf is 404 as expected:

❯ curl -H "Accept-Language: fr" http://localhost:3000/asdf
{"url":"/asdf","statusCode":404,"statusMessage":"Page not found: /asdf","message":"Page not found: /asdf","stack":""}
~/projects/github-fyaxh8

/foo redirects to /en/foo. There are two problems with this: Firstly, we expect 404 instead of a redirect, since foo is not a language. Secondly, the "Accept-Language" header was ignored, and we got en instead of fr.

❯ curl -H "Accept-Language: fr" http://localhost:3000/foo
<!DOCTYPE html><html><head><meta http-equiv="refresh" content="0; url=/en/foo"></head></html>

Note that there are no catch-all routes in this example!

rudolfbyker · 2023-11-23T10:19:27Z

By the way, includeUprefixedFallback is a pervasive typo in this module and in vue-i18n-routing. I guess it should be includeUnprefixedFallback, but that would be a breaking change in vue-i18n-routing.

rudolfbyker · 2023-11-23T10:54:50Z

Further debugging revealed the following underlying causes:

The locale-changing route middleware runs on all routes, not just on /, even when using detectBrowserLanguage: { redirectOn: "root" }.
detectLocale returns en when visiting /foo but fr when visiting / (both with the Accept-Language: fr header).

rudolfbyker · 2023-11-23T11:00:46Z

I think the underlying problem to all of this is that a lot of checks that are happening in the detectBrowserLanguage function should actually be happening in the route middleware. detectBrowserLanguage should do just one thing: Detect the language. The decision about whether it should be used or not belongs in the route middleware. But currently, detectBrowserLanguage looks at redirectOn, and returns "" for the locale if redirectOn === "root" (ignoring the header completely). So it basically lies to the detectLocale function, which then falls back on the default locale, which is en in this case.

In fact, just commenting out this part of detectBrowserLanguage solves both problems for me:

  // if (strategy !== "no_prefix") {
  //   if (redirectOn === "root") {
  //     if (path !== "/") {
  //       console.log("detectBrowserLanguage: not root");
  //       return { locale: "", stat: false, reason: "not_redirect_on_root" };
  //     }
  //   } else if (redirectOn === "no prefix") {
  //     console.log("detectBrowserLanguage: no prefix (path) -", path);
  //     if (!alwaysRedirect && path.match(getLocalesRegex(localeCodes))) {
  //       return { locale: "", stat: false, reason: "not_redirect_on_no_prefix" };
  //     }
  //   }
  // }

This should probably be replaced by something like this at the top of the route middleware:

const redirectOn = nuxtI18nOptions.detectBrowserLanguage?.redirectOn;
if (redirectOn == "root" && to.path !== "/") {
  console.log("Skipping locale-changing middleware because were not on the root path.")
  return;
}
if (redirectOn == "no prefix" && ... etcetera ... ) {
  return;
}

BobbieGoede · 2023-11-23T11:32:07Z

In fact, just commenting out this part of detectBrowserLanguage solves both problems for me

I think you will get the same results when setting detectBrowserLanguage.redirectOn: "all", this change would still redirect /foo to /fr/foo right?

rudolfbyker · 2023-11-23T12:17:22Z

I think you will get the same results when setting detectBrowserLanguage.redirectOn: "all", this change would still redirect /foo to /fr/foo right?

As I have said many times before, I do NOT want /foo to redirect to /fr/foo. The redirect should ONLY happen on /.

BobbieGoede · 2023-11-23T12:20:16Z

I'm asking you whether the changes you made would still result in the same behavior

rudolfbyker · 2023-11-23T13:23:45Z

Sorry for misunderstanding your question, and for my impatience!

To be honest, I'm still not sure what you are asking.

You wrote:

I think you will get the same results when setting detectBrowserLanguage.redirectOn: "all", this change would still redirect /foo to /fr/foo right?

Does "this change" refer to

setting detectBrowserLanguage.redirectOn to "all"?
commenting out the part of detectBrowserLanguage as mentioned here ?
your changes from fix: exclude unprefixed routes for strategy prefix #2538 ?
something else?

BobbieGoede · 2023-11-23T14:21:32Z

You said that commenting out part of detectBrowserLanguage solved both of your problems, my question is referring to that. Because from what I can tell this does something similar to, if not the same, as setting detectBrowserLanguage.redirectOn to "all", this is not the behavior you want, hence the question.

The confusion for me is, this issue is titled "Inconsistent handling of Accept-Language header", but you're experiencing 2 separate (though related) issues.

1. Inconsistent handling of `Accept-Language` header

This is possibly not an issue, which is why I refer to detectBrowserLanguage.redirectOn to "all", as this would change when Accept-Language is used to redirect a request (among other things).

2. Non-existent routes do not respond with 404 but redirect instead

This is a separate issue, related to routing and redirection. A new issue should be opened if it doesn't exist already. This sounds similar to #2131 but is not the same, that issue describes how unprefixed routes are still registered within the router, and that they are able to access these routes without being redirected, #2538 aims to fix the described bug in #2131.

rudolfbyker · 2023-11-25T08:56:03Z

Sorry, I looked at this again, and I was mistaken!

Commenting out this part in detectBrowserLanguage

// if (strategy !== "no_prefix") {
//   ...
// }

caused some other bugs, so that was a bad idea. This is because detectBrowserLanguage is not only called from the middleware that I was trying to fix. I was hyper-focused on that.

However, adding something like this at the top of the middleware does solve my problem:

const redirectOn = nuxtI18nOptions.detectBrowserLanguage?.redirectOn;
if (redirectOn == "root" && to.path !== "/") {
  console.log("Skipping locale-changing middleware because were not on the root path.")
  return;
}
if (redirectOn == "no prefix" && ... etcetera ... ) {
  // Not implemented yet because I'm not using redirectOn == "no prefix"
  console.log("Skipping locale-changing middleware because ...");
  return;
}

It solves the problem by preventing the middleware from running when it should not. This, together with the changes of #2538 solves both #2131 and this issue.

kazupon · 2023-12-04T14:57:14Z

close with #2538

rudolfbyker · 2023-12-04T15:36:48Z

@kazupon Thanks, but #2538 only solves half of this issue. But if you prefer, I'll make a new issue with a more specific reproduction after the next release.

rudolfbyker · 2024-01-02T10:05:45Z

Updated reproduction

https://stackblitz.com/edit/github-fyaxh8

Steps to reproduce

In the terminal:
1. npm install
2. npm run build
3. npm run preview
In another terminal:
1. curl --include --header "Accept-Language: fr" http://localhost:3000
2. curl --include --header "Accept-Language: fr" http://localhost:3000/foo

Results

Correct

GET http://localhost:3000 replies with:

HTTP/1.1 302 Found
connection: close
content-length: 89
content-type: text/html
date: Tue, 02 Jan 2024 10:01:03 GMT
location: /fr

<!DOCTYPE html><html><head><meta http-equiv="refresh" content="0; url=/fr"></head></html>

Incorrect

GET http://localhost:3000/foo replies with:

HTTP/1.1 302 Found
connection: close
content-length: 93
content-type: text/html
date: Tue, 02 Jan 2024 10:02:17 GMT
location: /en/foo

<!DOCTYPE html><html><head><meta http-equiv="refresh" content="0; url=/en/foo"></head></html>

This is wrong, because there is no /foo route, so it should give 404, not 302.

Proposed solution

Add something like this at the top of the route middleware in /dist/runtime/plugins/i18n.mjs:

const redirectOn = nuxtI18nOptions.detectBrowserLanguage?.redirectOn;
if (redirectOn == "root" && to.path !== "/") {
  console.log("Skipping locale-changing middleware because were not on the root path.")
  return;
}
if (redirectOn == "no prefix" && ... etcetera ... ) {
  // Not implemented yet because I'm not using redirectOn == "no prefix"
  console.log("Skipping locale-changing middleware because ...");
  return;
}

It solves the problem by preventing the middleware from running when it should not.

Here is the patch I'm currently using with yarn patch:

diff --git a/dist/runtime/plugins/i18n.mjs b/dist/runtime/plugins/i18n.mjs
index f013c8181a0a72be6a54e2777519949f4540b6f7..fce4e49e27f5ebc97bcd1cc9feef0ba8bff4e8fb 100644
--- a/dist/runtime/plugins/i18n.mjs
+++ b/dist/runtime/plugins/i18n.mjs
@@ -376,6 +376,12 @@ export default defineNuxtPlugin({
       "locale-changing",
       // eslint-disable-next-line @typescript-eslint/no-unused-vars
       defineNuxtRouteMiddleware(async (to, from) => {
+        const redirectOn = nuxtI18nOptions.detectBrowserLanguage?.redirectOn;
+        if (redirectOn === "root" && to.path !== "/") {
+          __DEBUG__ && console.log("Skipping locale-changing middleware because we are not on the root path.")
+          return;
+        }
+
         __DEBUG__ && console.log("locale-changing middleware", to, from);
         const locale = detectLocale(
           to,

rudolfbyker added the pending triage label Oct 25, 2023

kazupon added v8 browser language detection labels Oct 27, 2023 — with Volta.net

BobbieGoede mentioned this issue Nov 6, 2023

fix: exclude unprefixed routes for strategy prefix #2538

Merged

7 tasks

kazupon removed the pending triage label Nov 11, 2023

kazupon closed this as completed Dec 4, 2023

kazupon reopened this Dec 5, 2023

BobbieGoede removed the v8 label Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inconsistent handling of `Accept-Language` header #2524

Inconsistent handling of `Accept-Language` header #2524

rudolfbyker commented Oct 25, 2023 •

edited

Loading

BobbieGoede commented Nov 8, 2023

rudolfbyker commented Nov 8, 2023

rudolfbyker commented Nov 22, 2023

rudolfbyker commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023 •

edited

Loading

BobbieGoede commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023

BobbieGoede commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023 •

edited

Loading

BobbieGoede commented Nov 23, 2023

rudolfbyker commented Nov 25, 2023

kazupon commented Dec 4, 2023

rudolfbyker commented Dec 4, 2023

rudolfbyker commented Jan 2, 2024

Inconsistent handling of Accept-Language header #2524

Inconsistent handling of Accept-Language header #2524

Comments

rudolfbyker commented Oct 25, 2023 • edited Loading

Update 2024/01/02

Environment

Reproduction

Describe the bug

Additional context

Logs

BobbieGoede commented Nov 8, 2023

rudolfbyker commented Nov 8, 2023

rudolfbyker commented Nov 22, 2023

rudolfbyker commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023 • edited Loading

BobbieGoede commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023

BobbieGoede commented Nov 23, 2023

rudolfbyker commented Nov 23, 2023 • edited Loading

BobbieGoede commented Nov 23, 2023

1. Inconsistent handling of Accept-Language header

2. Non-existent routes do not respond with 404 but redirect instead

rudolfbyker commented Nov 25, 2023

kazupon commented Dec 4, 2023

rudolfbyker commented Dec 4, 2023

rudolfbyker commented Jan 2, 2024

Updated reproduction

Steps to reproduce

Results

Correct

Incorrect

Proposed solution

Inconsistent handling of `Accept-Language` header #2524

Inconsistent handling of `Accept-Language` header #2524

rudolfbyker commented Oct 25, 2023 •

edited

Loading

rudolfbyker commented Nov 23, 2023 •

edited

Loading

rudolfbyker commented Nov 23, 2023 •

edited

Loading

1. Inconsistent handling of `Accept-Language` header