fix(i18n): ensure the redirect pathname is non-empty#13730
fix(i18n): ensure the redirect pathname is non-empty#13730florian-lefebvre merged 6 commits intowithastro:mainfrom
Conversation
🦋 Changeset detectedLatest commit: f32d957 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 |
CodSpeed Performance ReportMerging #13730 will not alter performanceComparing Summary
|
ematipico
left a comment
There was a problem hiding this comment.
This PR isn't complete. We need:
- a test
- a changeset
For the test please create a new fixture inside the test/fixtures folder that represents the use case you're trying to fix. The update this file and add a test test case at the end. Ideally we want to test dev and build
|
Tests and changeset added, I hope I'm doing it correctly. |
| // Ensure the pathname are non-empty. Redirects like "/fr" => "" may create infinite loops, | ||
| // as the "Location" response header is empty. |
There was a problem hiding this comment.
The comment is still unclear. For example, you don't explain why a redirect from /fr to / could create an infinite loop.
By the way, I believe this isn't the right fix, or to be more precise, the bug is at line 387 where we use the includes. Maybe we should check if base == "/", and if so, at line 388 we shouldn't remove the slash from the replace. What do you think?
There was a problem hiding this comment.
The root cause is that the Location response header is empty in the case that the /{any-lang} will be replaced as empty pathname.
And the empty Location should be considered as invalid URL, it will cause infinite loops (redirecting to current page again and again) on some browsers (e.g. Zen based on FireFox) or stop redirecting (just blank page) on some browsers like Chrome.
Maybe we should check if base == "/", and if so, at line 388 we shouldn't remove the slash from the replace.
I'm not sure if this approach will solve it, for exmaple, /fr/about will be replaced as //about if we do not replace the slash, then browser will redirect to http(s)://about, so in this approach, we will need to replace // to / as well. So in my opinion the if equal and then assign should have better performance than string replacing.
I'm not good at writing English, you can edit that file and adopt the tests 🤝 .
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Closes #13729
Changes
pnpm exec changesetTesting
Docs