Make WordPress Core

Opened 13 years ago

Closed 9 years ago

#18818 closed defect (bug) (fixed)

wp_sanitize_redirect() kills "@" in URL's

Reported by: theandystratton's profile theandystratton Owned by: theandystratton's profile theandystratton
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.2.1
Component: Formatting Keywords: has-patch
Focuses: Cc:

Description

We had a URL like:

http://site.com/path/to/page?email=theandystratton@gmail.com

WP 301's to

http://site.com/path/to/page/?email=theandystratton@gmail.com

But wp_redirect()'s call to wp_sanitize_redirect() kills the "@" symbol. The reason for this being that a theme/plugin could be using query string arguments for something (i.e. form that accepts pre-populated input via query string, like an email address or arbitrary text).

This could have been an oversight OR it could be on purpose, if so, would like to know (I'd assume a security reason).

Attachments (2)

18818.diff (536 bytes) - added by theandystratton 13 years ago.
Allows @ symbol in wp_sanitize_redirect
18818.2.diff (1.5 KB) - added by markjaquith 10 years ago.
refreshed and added a unit test

Download all attachments as: .zip

Change History (9)

@theandystratton
13 years ago

Allows @ symbol in wp_sanitize_redirect

#1 @nacin
13 years ago

The @ symbol is a reserved character in URLs. It's for separating the username from the host. You need to encode it.

As stated in #18814, it may be possible to encode it in query strings only, but I don't think we should.

#2 follow-up: @theandystratton
13 years ago

Gotcha, makes sense. For some reason I was getting a similar behavior with encoding (%40) but not now. I figured there was probably something I was overlooking in terms of URL structure as reason for why it wasn't there. Thankyasir.

At least I'm 1 step closer to being able to submit a true patch that gets accepted ;]

#3 in reply to: ↑ 2 @nacin
13 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Replying to theandystratton:

At least I'm 1 step closer to being able to submit a true patch that gets accepted ;]

Indeed! :)

#4 @markjaquith
10 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

The @ symbol is a reserved character in URLs. It's for separating the username from the host. You need to encode it.

This is not true, according to RFC 3986. "@" is a perfectly valid character in a URL path or query string. It is only within the "user information" portion of a URL that it is reserved as a delimiter. https://medium.com/@nacin is a valid URL, and "@" shouldn't be stripped out of the path.

#5 @SergeyBiryukov
10 years ago

  • Milestone set to Awaiting Review

@markjaquith
10 years ago

refreshed and added a unit test

#6 @DrewAPicture
9 years ago

  • Component changed from General to Formatting
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.4

18818.2.diff still applies and unit tests pass. Moving for consideration.

#7 @wonderboymusic
9 years ago

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

In 33707:

In wp_sanitize_redirect(), don't eat @ characters. According to RFC 3986, "@" is a perfectly valid character in a URL path or query string.

Adds unit test.

Props markjaquith.
Fixes #18818.

Note: See TracTickets for help on using tickets.