WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#21841 closed defect (bug) (fixed)

redirect_canonical produces invalid redirects when post ID is specified

Reported by: solarissmoke Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4
Component: Canonical Keywords: has-patch commit
Focuses: Cc:

Description

With pretty permalinks disabled, visit the following url:

http://example.com/?p=1&feed=rss2

Assuming a post with that ID exists, this is valid and shouldn't be redirected anywhere. But it is redirected to:

http://localhost/wp/?feed=rss2&p=2

Note the encoded ampersand in the URL.

Same happens with http://example.com/?page_id=2&feed=rss2. But it doesn't happen if you don't supply a post ID (e.g., with http://example.com/?name=hello-world&feed=rss2 or http://example.com/?year=2012&feed=rss2).

This was introduced in WP 3.4 from what I can tell (it doesn't happen in 3.3).

Attachments (2)

21841.patch (587 bytes) - added by SergeyBiryukov 3 years ago.
21841.2.patch (2.3 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 @solarissmoke3 years ago

Sorry, those links should have feed=comments-rss2 instead of feed=rss2. The same incorrect redirects happen with both, however.

@SergeyBiryukov3 years ago

comment:2 @SergeyBiryukov3 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.5

Introduced in [20396], so it's indeed a regression in 3.4.

http://example.com/?p=1&feed=rss2
Assuming a post with that ID exists, this is valid and shouldn't be redirected anywhere.

Actually, the canonical URL for post comments feed without pretty permalinks would be http://example.com/?feed=rss2&p=1 (also broken in 3.4). The bug is the encoded ampersand.

  1. get_post_comments_feed_link() returns & as an entity:
    http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/link-template.php#L489
    (get_edit_post_link() has a context parameter for that, but other functions don't.)
    http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/link-template.php#L895
  1. Because of that, $redirect_url and $requested_url do not match:
    $redirect_url: http://trunk.wordpress/?feed=rss2&p=1
    $requested_url: http://trunk.wordpress/?feed=rss2&p=1
    
    And a pointless redirect is performed.

Not sure what's the best way to fix this, but 21841.patch:

  1. Doesn't break the canonical URL (http://trunk.wordpress/?feed=rss2&p=1).
  2. Properly redirects http://example.com/?p=1&feed=rss2 to the canonical URL.
  3. Passes our current unit tests (might need new).

There's a similar replacement in wp_nonce_url():
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/functions.php#L1154

comment:3 @dd323 years ago

Added a unit test in [UT1104]

comment:4 follow-up: @markjaquith3 years ago

We could just make the function return raw ampersands, and make sure that any time we're using it in a strict XML context, we're passing it through esc_url()

@SergeyBiryukov3 years ago

comment:5 in reply to: ↑ 4 @SergeyBiryukov3 years ago

Replying to markjaquith:

We could just make the function return raw ampersands, and make sure that any time we're using it in a strict XML context, we're passing it through esc_url()

21841.2.patch is an attempt to do that.

feed-rss2.php already has esc_url() there:
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/feed-rss2.php#L53

comment:6 @nacin2 years ago

  • Keywords commit added

comment:7 @nacin2 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 22431:

Don't return encoded ampersands from get_post_comments_feed_link() to avoid canonical redirect issues. Apply esc_url() when appropriate.

props markjaquith, SergeyBiryukov. fixes #21841.

Note: See TracTickets for help on using tickets.