fix: retry trailing-slash redirect when URL has a query string#31843
Open
helio-cf wants to merge 2 commits into
Open
fix: retry trailing-slash redirect when URL has a query string#31843helio-cf wants to merge 2 commits into
helio-cf wants to merge 2 commits into
Conversation
The worker's trailing-slash retry ran `request.url.replace(/([^/])$/, "$1/")` against the full URL string. For requests with a query string, that regex appended the slash at the end of the query (e.g. `?test=whatever/`) rather than to the pathname, so a rule like `/foo/* /bar/:splat 301` was never matched and the request 404'd. The retry now: - Skips entirely when pathname already ends in `/` (avoids a redundant evaluator call on every non-redirected request). - Appends `/` to the pathname only, so the retry actually forces a trailing slash on paths that carry a query string. - Reuses the URL parsed at the top of `fetch` instead of allocating another one. Adds two regression tests: one exercising a query string on a source path that already has a trailing slash (baseline for the library), and one on a source path that requires the trailing-slash retry to fire — the case the fix restores.
Contributor
Review✅ No issues found in commit Code ReviewThis code review is in beta and may not always be helpful — use your judgment. No code review issues found. ConventionsChecks PR title, description, and redirect checklist. No convention issues found. Style Guide ReviewNo style-guide issues found. RedirectsNo missing redirect entries found. CommandsOnly codeowners can run commands. Post a comment with the command to trigger it.
|
helio-cf
commented
Jul 1, 2026
| return isMarkdownRequest | ||
| ? rewriteRedirectForMarkdown(redirect, url) | ||
| : redirect; | ||
| if (!pathname.endsWith("/")) { |
Author
There was a problem hiding this comment.
pathname is not new, declared at around line 80 of this function
| if (!pathname.endsWith("/")) { | ||
| try { | ||
| const redirect = await redirectsEvaluator( | ||
| new Request(`${url.origin}${pathname}/${url.search}`, request), |
Author
There was a problem hiding this comment.
joining these should be safe, pathname will always have a leading slash
For requests like /old/index.md the pathname doesn't end in '/', so the retry would evaluate /old/index.md/. The first pass already covers these by stripping /index.md and evaluating the base path, and if the retry had happened to match anything, rewriteRedirectForMarkdown would then have re-appended index.md, producing paths like /new/index.md/index.md if the destination itself ended with /index.md. Gate the retry on !isMarkdownRequest as well, and drop the now-dead isMarkdownRequest branch inside it. Adds a regression test asserting that /docs/index.md resolves through the base path in the first pass.
helio-cf
commented
Jul 1, 2026
Comment on lines
-168
to
-180
| try { | ||
| const forceTrailingSlashURL = new URL( | ||
| request.url.replace(/([^/])$/, "$1/"), | ||
| request.url, | ||
| ); | ||
| const redirect = await redirectsEvaluator( | ||
| new Request(forceTrailingSlashURL, request), | ||
| this.env.ASSETS, | ||
| ); | ||
| if (redirect) { | ||
| return isMarkdownRequest | ||
| ? rewriteRedirectForMarkdown(redirect, url) | ||
| : redirect; |
Author
There was a problem hiding this comment.
the new code does away with the regex but keeps the rest up until the ternary, i dont believe we need rewriteRedirectForMarkdown(redirect, url) at this point since it would have already been covered by the first redirect check above... or am i reading this wrong?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The worker's trailing-slash retry ran
request.url.replace(/([^/])$/, "$1/")against the full URL string. For requests with a query string, that regex appended the slash at the end of the query (e.g.?test=whatever/) rather than to the pathname, so a rule like/foo/* /bar/:splat 301was never matched and the request 404'd.TLDR: https://developers.cloudflare.com/cloudflare-one/policies/data-loss-prevention?x=y 404s but should redirect the same way as https://developers.cloudflare.com/cloudflare-one/policies/data-loss-prevention/?x=y
The retry now:
/(avoids a redundant evaluator call on every non-redirected request)./to the pathname only, so the retry actually forces a trailing slash on paths that carry a query string.fetchinstead of allocating another one.Adds two regression tests: one exercising a query string on a source path that already has a trailing slash (baseline for the library), and one on a source path that requires the trailing-slash retry to fire — the case the fix restores.