WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#33372 closed defect (bug) (fixed)

WP_Query may return incorrect results when using negative values with parameter p.

Reported by: giantrobot Owned by: boonebgorges
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

In parse_query(), parameter "p" is passed through absint(), thus converting negative post IDs to positive ones.

For example, querying for a post with id -1 will return a post with id 1 (if it exists), which is incorrect.

That behaviour is not expected or documented.

Attachments (9)

WP_Query_returns_404_when_negative_p_parameter.33372.diff (509 bytes) - added by Akeif 3 years ago.
33372_unit_test.diff (560 bytes) - added by kouratoras 3 years ago.
Unit test
33372_unit_test_2.diff (591 bytes) - added by kouratoras 3 years ago.
33372.diff (1.6 KB) - added by kouratoras 3 years ago.
33372_2.diff (1.6 KB) - added by kouratoras 3 years ago.
33372_3.diff (1.6 KB) - added by Akeif 3 years ago.
Reworked version of 33372_2.diff
33372_4.diff (1.6 KB) - added by Akeif 3 years ago.
Reworked version of 33372_2.diff (svn compatible patch)
33372_5.diff (1.5 KB) - added by kouratoras 3 years ago.
33372.2.diff (885 bytes) - added by kouratoras 3 years ago.

Download all attachments as: .zip

Change History (46)

#1 @knutsp
4 years ago

  • Version trunk deleted

I don't like the many uses of absint() in core. If the goal is to sanitize a value that should be a positive integer then all non-postive integer values, after casting to integer, should either return an error or false (bail), or nothing (not found). This may be done by sanitizing negative integers 0 and continue.

1234 is not a sane value for -1234. -1234 is just illegal, can not and should not, be sanitized into something legal.

#2 @johnbillion
4 years ago

  • Keywords reporter-feedback dev-feedback added

Bearing in mind the wp_posts.ID field is unsigned, what's the expected behaviour when querying for p => -1? Should it cause an empty result set to be returned?

Is the expected behaviour the same for all similar numeric fields?

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


3 years ago

#4 @boonebgorges
3 years ago

  • Keywords needs-patch needs-unit-tests good-first-bug added; reporter-feedback dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

@giantrobot Thanks for the report, and welcome to WordPress Trac!

After some discussion during a recent bug scrub, we came to the following conclusions:

  • The current behavior is, indeed, bad.
  • The most sensible behavior is to 404.
  • This can probably be done by swapping absint() with intval().

The larger problem of absint() (ab)use through core should be handled on a case-by-case basis.

#5 @Akeif
3 years ago

@boonebgorges We're working on a patch at work and it look like using intval() isn't reliable since passing objects will return 1 and thus will be a valid parameter.

http://php.net/manual/en/function.intval.php

Passing array('foo') to intval() return 1, thus return post 1.

That beeing said, passing an object to absint() also return 1 so it's still an improvment.

#6 @Akeif
3 years ago

  • Keywords has-patch added; needs-patch removed

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


3 years ago

@kouratoras
3 years ago

Unit test

#8 follow-up: @kouratoras
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

I added a unit test for this change.

#9 in reply to: ↑ 8 ; follow-up: @flefebvre2
3 years ago

Replying to kouratoras:

I added a unit test for this change.

Good job. I would only suggest adding the ticket id in the phpdoc as what was done with test_parse_query_s_array function.

#10 in reply to: ↑ 9 @kouratoras
3 years ago

Replying to flefebvre2:

Good job. I would only suggest adding the ticket id in the phpdoc as what was done with test_parse_query_s_array function.

Good point, ticket number added.

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


3 years ago

#12 follow-up: @boonebgorges
3 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests removed

@kouratoras @Akeif Thanks for the patch and the test. A few notes/questions:

  1. @Akeif correctly points out that passing non-scalar values to 'p' will result in odd behavior. We should have unit tests that demonstrate our intended behavior.
  1. WP_Query_returns_404_when_negative_p_parameter.33372.diff doesn't address the non-scalar weirdness, since intval() will cast arrays and objects to 1. Should we take this opportunity to fix that problem too? Something like: if ( ! is_scalar( $p ) || $p < 0 ) { 404 }, with the intval() typecasting happening after the check?

#13 in reply to: ↑ 12 @kouratoras
3 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

@boonebgorges I am adding a patch that addresses both issues.
I am also setting p=1 in case of object, as intval() should not be used in this case directly.

@kouratoras
3 years ago

#14 follow-up: @Akeif
3 years ago

@kouratoras Why test is_object() and !is_scalar()? is_scalar() should return false on an object right?

#15 in reply to: ↑ 14 @kouratoras
3 years ago

Replying to Akeif:

@kouratoras Why test is_object() and !is_scalar()? is_scalar() should return false on an object right?

Correct, is_scalar() returns false on an object.

But, I am also checking is_object() and setting p=1 before that, in order not to apply intval() directly to an object.
PHP manual says:

intval() should not be used on objects, as doing so will emit an E_NOTICE level error and return 1.

#16 follow-up: @boonebgorges
3 years ago

Why would we want 'p' set to 1 in the case of an object? Why set p=1 if 404 is true?

#17 @Akeif
3 years ago

The logic doesn't make sense to me but, I'm not sure what is the correct way of completing this fix.

@boonebgorges Do we need to give a value to $qv['p'] when we set $qv['error'] = 404? If $qv['p'] need to be set should we set it to 0 in the case of a 404?

What I understand is, if the value is not a positive scalar value, we should throw a 404. In which case, if validation passes, the intval() is useless since we know it's a positive scalar.

#18 in reply to: ↑ 16 ; follow-up: @kouratoras
3 years ago

Replying to boonebgorges:

Why would we want 'p' set to 1 in the case of an object? Why set p=1 if 404 is true?

The only reason doing that is not to pass an object to intval(). Otherwise, we do not need that value.

#19 in reply to: ↑ 18 ; follow-up: @boonebgorges
3 years ago

Replying to kouratoras:

Replying to boonebgorges:

Why would we want 'p' set to 1 in the case of an object? Why set p=1 if 404 is true?

The only reason doing that is not to pass an object to intval(). Otherwise, we do not need that value.

It doesn't seem semantically correct to me. If I hook to 'parse_query' in a plugin, I'll see p=1 (as well as is_single=true), which isn't accurate. Maybe use 0 instead.

#20 in reply to: ↑ 19 @kouratoras
3 years ago

Replying to boonebgorges:

It doesn't seem semantically correct to me. If I hook to 'parse_query' in a plugin, I'll see p=1 (as well as is_single=true), which isn't accurate. Maybe use 0 instead.

Good point, submitting revised patch.

@kouratoras
3 years ago

@Akeif
3 years ago

Reworked version of 33372_2.diff

#21 @Akeif
3 years ago

@kouratoras If you don't mind I'd like to rework this a bit. Please take a look at 33372_3.diff and let me know what you think.

Edit: Hum. I think I did something wrong with SVN the patch is broken. Let me fix this quick.

Last edited 3 years ago by Akeif (previous) (diff)

@Akeif
3 years ago

Reworked version of 33372_2.diff (svn compatible patch)

#22 follow-up: @Akeif
3 years ago

@kouratoras I uploaded the new patch 33372_4.diff. Let me know what you think :)

Last edited 3 years ago by Akeif (previous) (diff)

#23 in reply to: ↑ 22 @kouratoras
3 years ago

Replying to Akeif:

@kouratoras I upload the new patch 33372_4.diff. Let me know what you think :)

I have 2 comments:

  • is_object() is not needed, as it's covered by ! is_scalar()
  • We should remove $this->assertSame( -3, $q->query_vars['p'] ); from unit test, as in this case it is 0.

@kouratoras
3 years ago

#24 @Akeif
3 years ago

@kouratoras I think we nailed it :p

@boonebgorges Can you take a look and give your blessings to patch 33372_5.diff?

#25 @boonebgorges
3 years ago

  • Keywords good-first-bug removed
  • Milestone changed from Future Release to 4.7
  • Owner set to boonebgorges
  • Status changed from new to assigned

Thanks - I think this looks pretty good.

#26 @boonebgorges
3 years ago

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

In 38288:

Query: Non-scalar and negative values for 'p' should always result in a 404.

Previously, the 'p' query var was being run through absint(), which
caused unexpected results.

Props Akeif, kouratoras.
Fixes #33372.

#27 follow-up: @swissspidy
3 years ago

Worth noting that the tests fail on HHVM:

1) Tests_Query_ParseQuery::test_parse_query_p_array
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-404
+

#28 in reply to: ↑ 27 @kouratoras
3 years ago

Replying to swissspidy:

Worth noting that the tests fail on HHVM:

@swissspidy can you give more info on this? I am running the tests for this file on HHVM locally and all succeed.

#29 follow-up: @boonebgorges
3 years ago

I played with this a bit recently, and I wonder whether it's tied to a certain version of HHVM. @kouratoras what version are you running?

#30 in reply to: ↑ 29 @kouratoras
3 years ago

Replying to boonebgorges:

I played with this a bit recently, and I wonder whether it's tied to a certain version of HHVM. @kouratoras what version are you running?

@boonebgorges I am running 3.14.5.

#31 @boonebgorges
3 years ago

@kouratoras Oh interesting - that's the version being run by Travis. It could be something with Travis's configuration. The plot thickens.

#32 @kouratoras
3 years ago

It seems it was a glitch, I managed to reproduce:

There was 1 failure:

1) Tests_Query_ParseQuery::test_parse_query_p_array
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-404
+

Investigating more...

@kouratoras
3 years ago

#33 @kouratoras
3 years ago

For some reason, is_scalar returned TRUE in case of array input on HHVM. So, I added a check is_array.

I also had an error in another test in the same file:

There was 1 error:

1) Tests_Query_ParseQuery::test_parse_query_s_array
strlen() expects parameter 1 to be string, array given

so I added an is_array check for s as well.

Tests seem to succeed now in HHVM too.
@boonebgorges can you take a look at the new patch?

#34 @boonebgorges
3 years ago

@kouratoras Thanks for the patch. But I'm wary of just dropping it in for what seems, at a glance, to be incorrect behavior on HHVM's part. Are you able to do more debugging into why is_scalar() is returning true in these instances? Can you examine exactly what's being passed into is_scalar() at this point? There are a lot of references being thrown around in WP_Query, and I wonder if HHVM might be choking on one of them.

#35 @kouratoras
3 years ago

@boonebgorges at that point, an array array(0) {} is being passed to is_scalar().

I am checking var_dump( is_scalar( $qv['p'] ) );:

  • In case of HHVM, it returns bool(true)
  • In case of PHP 5.6, it returns bool(false)

#36 @swissspidy
3 years ago

Neither HHVM 3.9.1, 3.10.0, 3.11.0 nor 3.12.0 (all tested on 3v4l.org) return true when passing an empty array to is_scalar(). Testing on 3.15.0-dev on a site, it works correctly there as well.

This ticket was mentioned in Slack in #core by mark-k. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.