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

[giantrobot]

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.

[knutsp]

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.

[johnbillion]

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?

[boonebgorges]

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

[Akeif]

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

[kouratoras]

Unit test

[kouratoras]

I added a unit test for this change.

[flefebvre2]

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.

[kouratoras]

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.

[boonebgorges]

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

2. 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?

[kouratoras]

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

[Akeif]

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

[kouratoras]

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.

[boonebgorges]

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

[Akeif]

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.

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

[boonebgorges]

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.

[kouratoras]

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.

[Akeif]

Reworked version of 33372_2.diff

[Akeif]

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

[Akeif]

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

[Akeif]

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

[kouratoras]

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.

[Akeif]

@kouratoras I think we nailed it :p

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

[boonebgorges]

Thanks - I think this looks pretty good.

[boonebgorges]

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.

[swissspidy]

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
+

[kouratoras]

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.

[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?

[kouratoras]

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.

[boonebgorges]

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

[kouratoras]

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]

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?

[boonebgorges]

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

[kouratoras]

@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)

[swissspidy]

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.