WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#15774 closed defect (bug) (fixed)

Don't Double URL Query Args in Canonical Redirect

Reported by: filosofo Owned by: MarkJaquith
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.1
Component: Canonical Keywords: has-patch
Focuses: Cc:

Description

Currently, when using "default" permalinks and requesting something that triggers canonical redirect, the query portion of the request URL can double up the same request arguments, which leads to redirect_canonical()'s returning falsely different URLs.

In other words, suppose you have a custom post type of custom_post_thing with an object that has a slug a-custom-post-thingy-title and an ID of 123.

If you request /?p=123 redirect_canonical() figures out that its correct, canonical URL is actually

/?custom_post_thing=a-custom-post-thingy-title

However, when redirect_canonical() is called again at line 362, it appends the same query value to the fetched URL, like so:

/?custom_post_thing=a-custom-post-thingy-title&custom_post_thing=a-custom-post-thingy-title

The result is that

/?custom_post_thing=a-custom-post-thingy-title&custom_post_thing=a-custom-post-thingy-title

and

/?custom_post_thing=a-custom-post-thingy-title

differ as strings even though they're the same in actual meaning, and the redirect fails and a 404 results.

(I have seen this happen in other redirect situations--it's not limited to custom post types.)

The patch parses the redirect URL's query value and uses add_query_arg() to add it, rather than the current naive string concatenation. The result is that arguments do not get doubled, and the redirect succeeds.

Attachments (1)

do-not-double-redirect-query.15774.diff (679 bytes) - added by filosofo 3 years ago.

Download all attachments as: .zip

Change History (5)

comment:1 filosofo3 years ago

It's also worth pointing out that the current behavior results in differing behavior between default permalinks and pretty permalinks. With pretty permalinks canonical will correctly redirect a request like /?p=123 above; with the default ones, it fails to redirect and produces a 404.

comment:2 filosofo3 years ago

Note: #15775 uses the patch here. Committing #15775 will fix this ticket as well.

comment:3 nacin3 years ago

  • Owner set to MarkJaquith
  • Status changed from new to assigned

comment:4 markjaquith3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [17030]) Do not add URL query args twice in certain Canonical Redirect situations. Use add_query_arg(). props filosofo. fixes #15774

Note: See TracTickets for help on using tickets.