Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#23284 closed defect (bug) (fixed)

Wrong parameter order for stripos in wp-includes/functions.php

Reported by: kalindor's profile Kalindor Owned by: nacin's profile nacin
Milestone: 3.5.2 Priority: normal
Severity: normal Version: 3.5
Component: General Keywords: needs-unit-tests
Focuses: Cc:

Description

In wp-includes/functions.php lines 658 and 661 stripos is used two times with parameters in the wrong order.

This would be correct:

if ( 0 === stripos( $uri, 'http://' ) ) {
} elseif ( 0 === stripos( $uri, 'http://' ) ) {

Attachments (2)

23284.diff (1.0 KB) - added by nacin 12 years ago.
23284.2.diff (551 bytes) - added by nacin 12 years ago.
For 3.5

Download all attachments as: .zip

Change History (19)

#1 @nacin
12 years ago

  • Milestone changed from Awaiting Review to 3.5.1

Broken in [21865]. Not sure how this didn't break unit tests.

#2 @nacin
12 years ago

In 1192/tests:

Test add_query_arg() when used with only a query string. see #4903. see #23284.

@nacin
12 years ago

#3 follow-up: @nacin
12 years ago

Best I can tell, we are dealing with a bit of dead code here.

Next to the $protocol check, #4903 added a check for strpos( $uri, '=' ) === false. There is never a situation where the former is true, while the latter is false. By this point, a URL will not have = as long as it has a protocol, because that means it also must have a ?, which is the earlier if branch. A URL like 'http://example.com/=' isn't valid, but is the only thing that would fail here.

23284.diff is a patch for 3.6. For 3.5, we can switch back the variables.

#4 @kurtpayne
12 years ago

  • Cc kurtpayne added
  • Keywords has-patch added

I can replicate these results.

Simply put: we do not need to know a URL's protocol to add a query argument. We just care about question marks and equals signs.

Both patches look sane and pass the unit tests.

@nacin
12 years ago

For 3.5

#5 @nacin
12 years ago

  • Milestone changed from 3.5.1 to 3.6

#6 in reply to: ↑ 3 @SergeyBiryukov
12 years ago

Replying to nacin:

By this point, a URL will not have = as long as it has a protocol, because that means it also must have a ?, which is the earlier if branch. A URL like 'http://example.com/=' isn't valid, but is the only thing that would fail here.

#23402 has ​http://www.nanowerk.com/news2/newsid=28843.php as an example.

#7 @lumpysimon
12 years ago

  • Cc piemanek@… added

#8 @billerickson
12 years ago

  • Cc bill.erickson@… added

#9 @rzen
12 years ago

  • Cc brian@… added

AH! Yes! This has been driving me mildly batty on local with wp_debug on for a couple weeks, but I've been too heads-down to research and post.

In my experience, this generates a number of debug warnings, but only when working with un-saved CPTs, e.g. http://d.pr/i/Qkf8+

It also affects the JS for creating new taxonomies if the post hasn't been saved. The taxonomy does get created, but there is no immediate feedback in the editor (nor is it automatically selected for the post).

Last edited 12 years ago by rzen (previous) (diff)

#10 @nacin
12 years ago

  • Milestone changed from 3.6 to 3.5.2

#11 @andykeith
12 years ago

  • Cc andykeith added

This also causes problems when using Minify as its URL rewriting allows URLs of the form http://example.org/g=css or http://example.org/min/f=style.css.

As it stands in 3.5.1, dots are replaced with underscores (also reported in #23402), e.g.

add_query_arg('foo', 'bar', 'http://example.org/g=css') = 'http://example_org/g=css?foo=bar'
Version 0, edited 12 years ago by andykeith (next)

#12 @onetarek
12 years ago

For this bug, we are getting warning messages after publishing any post.

Warning: stripos() [function.stripos]: needle is not a string or an integer in /home/....../wp-includes/functions.php on line 658

Warning: stripos() [function.stripos]: needle is not a string or an integer in /home/....../wp-includes/functions.php on line 661

Warning: Cannot modify header information - headers already sent by (output started at /home/....../wp-includes/functions.php:658) in /home/....../wp-includes/pluggable.php on line 876

#13 @nacin
11 years ago

First, some history. [3857] added the initial protocol support. A much younger MarkJaquith left a comment on #2793 that explained it:

You can see in http://core.trac.wordpress.org/browser/trunk/wp-includes/functions.php?annotate=blame&rev=3857#L839 that we used to look for /, which when a full protocol was supplied, that's what got tripped up.

Thanks to a few of you for pointing out that a full URL with = but not ? is functional, if weird, and is thus why this code is required. As Sergey pointed out in #23402, simply fixing the arguments would be best. Let's make sure this is fully unit tested soon, to make sure we don't go on wild chases in the future.

#14 @nacin
11 years ago

In 24444:

Use correct variable order in add_query_arg(). This had mostly just filled error logs; it also broke some obscure URL situations. see #23284.

#15 @nacin
11 years ago

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

In 24445:

Use correct variable order in add_query_arg(). This had mostly just filled error logs; it also broke some obscure URL situations.

Merges [24444] to the 3.5 branch.

fixes #23284.

#16 @nacin
11 years ago

#23402 was marked as a duplicate.

#17 @nacin
11 years ago

  • Keywords needs-unit-tests added; has-patch removed
Note: See TracTickets for help on using tickets.