redirect_canonical produces invalid redirects when post ID is specified

[solarissmoke]

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).

[solarissmoke]

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

[SergeyBiryukov]

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
   

2. 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

[dd32]

Added a unit test in ​[UT1104]

[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()

[SergeyBiryukov]

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

[nacin]

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.