Canonical: Don't redirect when the query string differs only in encoding by f0rthelulz · Pull Request #13693 · WordPress/wordpress-develop

f0rthelulz · 2026-09-23T19:20:19Z

Trac ticket: https://core.trac.wordpress.org/ticket/41712

When redirect_canonical() redirects and carries over extra query vars, it rebuilds them with rawurlencode_deep(). A value containing a literal + (e.g. ?utm_source=Some+Campaign, which email tools commonly generate) comes back as %20. The resulting URL differs from the request, so a 301 is issued even though the query is semantically identical.

On its own that's one harmless extra hop. But a page cache that normalizes or decodes the query when building its cache key (Batcache is one example) treats + and %20 as the same object, so it can cache that 301 and serve it for the target URL too, which shows up as an infinite redirect loop. A cache keyed on the raw URL wouldn't loop.

This change adds a late check in redirect_canonical(), just before the hex-octet comparison: if the redirect URL and the requested URL are identical apart from the query string and fragment, and both query strings parse to the same variables (parse_str() + ksort()), no redirect is issued. This catches encoding-only differences wherever they arise (+ vs %20, a= vs a, a stray &, array params), including targets that already carry a query string, such as plain-permalink feeds (/?feed=rss2&p=1&x=a+b) or a filtered home_url(). Redirects that change the path, host or scheme still happen, and their extra query vars are still encoded as on trunk. Note that parse_str() turns . and spaces in keys into _, so ?a.b=c and ?a_b=c compare equal. That's intended, since WordPress sees the same query vars either way.

The approach and tests are from @rbcorrales's review. An earlier version of this PR re-encoded the query only in the extra-query-vars branch, building on #865 by @roborourke. That version has been reverted.

Testing

New Tests_Canonical::test_no_redirect_when_query_only_differs_in_encoding covers 7 cases on a static front page: plus for space, empty value, unencoded pipe, leading &, trailing &, period in key, and array append.
New data_canonical rows check that path-changing redirects keep their extra query vars encoded (/2008/09/03/non-paged-post-test/3/?x=a|b → ?x=a%7Cb, ?page=3&x=a+b → ?x=a%20b). Another row checks that a stray trailing & on a non-front-page URL (/2008/09/03/non-paged-post-test/?x=1&) doesn't redirect.
Mutation check: with the new return null removed, 8 tests fail (all 7 provider cases and the stray-& row). With it restored, they all pass.
canonical group: OK (1067 tests, 1144 assertions).
rewrite group: OK (1423 tests, 2856 assertions).
query group: OK (1906 tests, 4446 assertions).
link group: OK (195 tests, 321 assertions).
composer lint and composer compat are clean on both changed files.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

…ing. A literal '+' in a query value (e.g. utm_source=foo+bar) was re-encoded to '%20', producing a redirect that caches keying on the raw URL can replay as a loop. See #41712.

github-actions · 2026-09-23T19:20:30Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props angelulz, rcorrales, sanchothefat.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2026-09-23T19:24:04Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

roborourke · 2026-09-24T08:47:56Z

@f0rthelulz thanks for taking this and improving on it

rbcorrales

I reproduced the loop on trunk with a static front page and a page cache that treats + and %20 as the same key. The PR fixes that case, but targets that already have a query string still loop. With plain permalinks, /?feed=rss2&p=1&x=a+b redirects to /?feed=rss2&p=1&x=a%20b. Same for a home_url() filtered to include a query arg.

I'd revert the canonical.php change (which also avoids the issues on L601), keep the test, and instead skip the redirect when the target only differs from the request in how the query is written. That also covers the feed case and stray & (/?x=1&), and keeps trunk's encoding on real redirects.

For tests, I'd add path-changing cases that must stay encoded. This one fails with the current PR:

array( '/2008/09/03/non-paged-post-test/3/?x=a|b', '/2008/09/03/non-paged-post-test/?x=a%7Cb', 41712 ),

rbcorrales · 2026-10-02T21:45:25Z

+		/*
+		 * Only re-encode the query string if it differs from the original request.
+		 * Re-encoding a semantically identical query (e.g. '+' vs '%20') produces a
+		 * redirect that caches keying on the raw URL can replay as a loop.
+		 * See https://core.trac.wordpress.org/ticket/41712
+		 */


The loop needs a cache that normalizes or decodes the query when building its key (Batcache is one example). A cache keyed on the raw URL treats + and %20 as different keys, so it wouldn't loop.

Agreed, that comment is gone with the revert, and the PR description now says the loop needs a cache that normalizes the query when building its key (e.g. Batcache), not one keyed on the raw URL.

rbcorrales · 2026-10-02T21:49:03Z

+		ksort( $_parsed_original_query );
+
+		if ( $_sorted_query === $_parsed_original_query && ! str_contains( $redirect_url, '?' ) ) {
+			$redirect_url .= '?' . $original['query'];


On path/host-changing redirects, wp_sanitize_redirect() strips what rawurlencode_deep() used to encode, for example:

GET /sample-page/2/?q=a|b$c{d}^e On trunk: Location: /sample-page/?q=a%7Cb%24c%7Bd%7D%5Ee PR branch: Location: /sample-page/?q=abcde

It also appends after any #fragment in $redirect_url, so the query gets dropped (e.g. a page_link filter adding #top).

Reverted. Real redirects go through trunk's rawurlencode_deep() + add_query_arg() again, and there's a data_canonical row asserting /3/?x=a|b -> ?x=a%7Cb.

rbcorrales · 2026-10-02T21:49:36Z

+	/**
+	 * @ticket 41712
+	 */


Missing @covers.

Test removed, since the new provider's "plus for space" case covers it, and the provider test has @covers ::redirect_canonical.

rbcorrales · 2026-10-02T21:53:12Z

+		update_option( 'show_on_front', 'page' );
+		update_option( 'page_on_front', $p );
+
+		$this->go_to( get_permalink( $p ) );


Since $p is the front page, get_permalink( $p ) returns /, so this simulates a request to / but checks /?utm_source=foo+bar. Works here, but $url = home_url( '/?utm_source=foo+bar' ); $this->go_to( $url ); and passing the same $url would keep the two in sync.

Test removed. The provider test uses one $url for both go_to() and redirect_canonical().

rbcorrales · 2026-10-02T22:22:56Z

+		// A literal '+' in a query value must not redirect to the '%20'-encoded form.
+		$redirect = redirect_canonical( home_url( '/?utm_source=foo+bar' ), false );
+
+		delete_option( 'page_on_front' );


Not needed, DB changes roll back after each test.

Test removed, and the provider test doesn't clean up options.

rbcorrales · 2026-10-02T23:57:11Z

Also, this isn't limited to static front pages. On trunk with default settings, a leading or trailing & in the query (/sample-page/?x=1&, /?&x=1) gets a 301 to the same URL without it, so a cache that keys on the decoded query loops the same way. The approach I suggested in my review should cover that too.

Reverts the re-encoding change in the extra query vars branch and instead adds a late check in `redirect_canonical()`: if the redirect URL and the requested URL differ only in how the query string is written (e.g. '+' vs '%20', 'a=' vs 'a', a stray '&'), no redirect is issued. A page cache that normalizes the query when building its key could otherwise serve the redirect for its own target URL, causing a redirect loop. See #41712. Props rcorrales, sanchothefat.

f0rthelulz · 2026-10-06T21:49:10Z

Thanks @rbcorrales, both for reproducing the loop and for the patch. I've applied your approach as-is: the extra-query-vars change is reverted to trunk's rawurlencode_deep() + add_query_arg(), and there's now a single late check that skips the redirect when the target only differs from the request in how the query is written. I also added your data_canonical rows for path-changing redirects.

On top of that:

I removed my original test_plus_encoded_query_value_does_not_trigger_redirect, because the provider's "plus for space" case covers the same thing with one consistent $url. If you'd rather keep it as a standalone regression test, I'm happy to restore it with your three fixes applied.
I added @covers ::redirect_canonical to the new provider test.
I added a non-front-page stray-& row: /2008/09/03/non-paged-post-test/?x=1& doesn't redirect.

Results: canonical 1067 tests OK, rewrite 1423 OK, query 1906 OK, link 195 OK, and lint/compat are clean. With the new return null block removed, 8 tests fail (all 7 provider cases and the stray-& row). CI is green on the new head. I've also updated the PR description, including the cache wording.

Canonical: Don't redirect when the query string differs only in encod…

cbc4ac2

…ing. A literal '+' in a query value (e.g. utm_source=foo+bar) was re-encoded to '%20', producing a redirect that caches keying on the raw URL can replay as a loop. See #41712.

rbcorrales suggested changes Oct 2, 2026

View reviewed changes

f0rthelulz requested a review from rbcorrales October 6, 2026 21:49

Conversation

f0rthelulz commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

github-actions Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 23, 2026

Test using WordPress Playground

Some things to be aware of

Uh oh!

roborourke commented Sep 24, 2026

Uh oh!

rbcorrales left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbcorrales commented Oct 2, 2026

Uh oh!

f0rthelulz commented Oct 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

f0rthelulz commented Sep 23, 2026 •

edited

Loading

github-actions Bot commented Sep 23, 2026 •

edited

Loading