Make WordPress Core

Opened 13 years ago

Closed 10 years ago

#17052 closed defect (bug) (fixed)

wp_sanitize_redirect() removes square brackets from URL

Reported by: bluntelk's profile bluntelk Owned by: johnbillion's profile johnbillion
Milestone: 4.1 Priority: normal
Severity: minor Version: 3.1
Component: Formatting Keywords: has-patch
Focuses: Cc:

Description

The function wp_sanitize_redirect() removes square brackets from URLs.

PHP's functionality with arrays in the URL require square braces, stripping them from the URL means that pages (and plugins) that rely on them fail.

To Reproduce:

<?php
$url = 'http://example.com/my_url_array[1]=hello+world';
print wp_sanitize_redirect($url);
?>

Current Output:
http://example.com/my_url_array1=hello+world

Expected Output:
http://example.com/my_url_array[1]=hello+world

Whilst developers should be able to work around this as the function is pluggable I believe this should just work out of the box.

Attachments (4)

pluggable.17052.patch (499 bytes) - added by bluntelk 13 years ago.
adds square braces to the regular expression for allowed characters in a safe URL
basic_auth_for_wp_redirect.diff (504 bytes) - added by david.binda 10 years ago.
17052-02.patch (1.6 KB) - added by gcorne 10 years ago.
17052.diff (1.4 KB) - added by voldemortensen 10 years ago.

Download all attachments as: .zip

Change History (20)

#1 @dd32
13 years ago

  • Keywords has-patch removed
  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

whilst referring to blogroll, the root cause is the same

In short, the brackets shouldn't be in the url, they *should* be encoded, but often arn't.

#16859

@bluntelk
13 years ago

adds square braces to the regular expression for allowed characters in a safe URL

#2 @dd32
13 years ago

  • Milestone set to Awaiting Review
  • Resolution duplicate deleted
  • Status changed from closed to reopened

Probably shouldnt've closed this so fast as a duplicate.

It's the same cause, but probably a lot better explained and in a different usecase.

#3 @bluntelk
13 years ago

  • Keywords has-patch needs-testing added

Hi dd32,

Thanks for re-opening this ticket.

I was working under the assumption that the characters we simply missing from the regular expression due to an oversight.

It is a powerful feature to support.

If it is determined under review that they should be stripped from the URL, we can always automatically encode the square brackets into their entities inside the function.

Thanks for your time.

Jason

#4 @bluntelk
12 years ago

  • Version changed from 3.1 to 3.4

#5 @SergeyBiryukov
12 years ago

  • Version changed from 3.4 to 3.1

Version number indicates when the bug was initially introduced/reported.

#6 @c3mdigital
11 years ago

  • Keywords needs-refresh added; needs-testing removed

#7 @SergeyBiryukov
11 years ago

#26037 was marked as a duplicate.

#8 @nacin
10 years ago

  • Component changed from General to Formatting
  • Milestone changed from Awaiting Review to Future Release

#9 @david.binda
10 years ago

"@" is being removed from URL as well. As "@" might be a part of basic-auth URL, it should be preserved in the URL.

As I noticed that https://core.trac.wordpress.org/ticket/26037 was closed as a dupe (despite it contains "@" as well), I'm posting a patch containing both ("@" and "[]") to his ticket.

@gcorne
10 years ago

#10 @gcorne
10 years ago

Allowing square brackets is also important when dealing with IPv6 literals (see RFC2732). The patch 17052-02.patch adds a couple unit tests in addition to adding [ and ] to the whitelist of allowed characters.

I am not sure that we want to "@" as part of this same patch, but "@" is a reserved character so I think it is appropriate for it to be allowed.

#11 @gcorne
10 years ago

  • Keywords commit added; needs-refresh removed

#12 @jkohlbach
10 years ago

Hi all, throwing my hat in the ring here..

I've raised a couple of these types of tickets in the past relating to characters being stripped out incorrectly (most recent is here: #30308 just the other day).

If we can avoid one more ticket and do the fix for @ while we're here, that would be preferable so it gets fixed sooner.

Last edited 10 years ago by SergeyBiryukov (previous) (diff)

#13 @johnbillion
10 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Milestone changed from Future Release to 4.1

#14 @voldemortensen
10 years ago

  • Keywords has-patch added; needs-patch removed

Refresh. Allows []'s in the url as an IPv6 and as a param. Adds unit tests.

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


10 years ago

#16 @johnbillion
10 years ago

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

In 30683:

Allow square brackets in a URL when it's sanitised for a redirect. Square brackets are valid in query parameters and IPv6 addresses.

Fixes #17052
Props voldemortensen

Note: See TracTickets for help on using tickets.