Opened 4 months ago

Last modified 3 months ago

#23284 new defect (bug)

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

Reported by: Kalindor Owned by:
Priority: normal Milestone: 3.5.2
Component: General Version: 3.5
Severity: normal Keywords: has-patch
Cc: kurtpayne, piemanek@…, bill.erickson@…, brian@…, andykeith

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 4 months ago.
23284.2.diff (551 bytes) - added by nacin 4 months ago.
For 3.5

Download all attachments as: .zip

Change History (13)

  • Milestone changed from Awaiting Review to 3.5.1

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

In 1192/tests:

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

nacin4 months ago

comment:3 follow-up: ↓ 6   nacin4 months 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.

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

nacin4 months ago

For 3.5

  • Milestone changed from 3.5.1 to 3.6

comment:6 in reply to: ↑ 3   SergeyBiryukov4 months 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.

  • Cc piemanek@… added
  • Cc bill.erickson@… added
  • 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 4 months ago by rzen (previous) (diff)
  • Milestone changed from 3.6 to 3.5.2
  • 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 3 months ago by andykeith (next)
Note: See TracTickets for help on using tickets.