$_REQUEST's slashes may differ from $_GET/$_POST

[dd32]

$_REQUEST is re-created from $_GET/$_POST on line 51 of wp-settings.php

However, $_GET/$_POST/$_COOKIE are slashed ~ line 580, The end result is that $_POST['test'] may not equal $_REQUEST['test'] depending on the server setup, and content of such globals.

My suggestion would be to move both code blocks from where they currently are, and place them in about line 260. (Or alternativly, Just add $_REQUEST to the deslash/addslash code block..)

[Denis-de-Bernardy]

this seems rather blocking imo

[Denis-de-Bernardy]

Curious to know why $_REQUEST isn't slashed, alongside $_GET, $_POST and $_COOKIE...

[dd32]

Probably because when that code was written, It was taboo to touch $_REQUEST (Or it was simply forgotten..)

That being said, I hate that plugins have come to expect slashed content..

[Denis-de-Bernardy]

yea. the real question will be what will happen if/when we drop the behavior, though... Imagine all of the security holes that we'll be introducing.

[dd32]

most of the WP Private API has moved/started to move to expecting non-slashed data, Well, aside from those which retain the back-compat since they're called directly.

It is a shame that it'd introduce holes, but it'll happen one day..

[hakre]

Dropping "Request Abstraction" again. API Style. Should save us a lot of headaches w/o doing much bloat.

+1 for "Legacy code can stay legacy and fails sometime"

[dd32]

Except its hardly Legacy, And is used more often in WP every version. Generally only or comparison of action args. Its rarely used for data which may include slashes, infact, pretty much unheard of.

there was a recent changeset which forces $_REQUEST to be $_GET + $_POST, so it removes most of the usual PHP-arguements against using $_REQUEST.

[hakre]

The argument against using $_REQUEST is taht, well using it disables you to determine of which the source comes from. So merging $_GET + $_POST into $_REQUEST still keep this argument.

[vladimir_kolesnikov]

In brief:

If magic_quotes_gpc is on: $_GET, $_POST, $_COOKIE, $_SERVER and $_REQUEST will be slashed;
If magic_quotes_gpc is off: $_GET, $_POST, $_COOKIE and $_SERVER will be slashed, $_REQUEST won't.

And, $_REQUEST = array_merge($_GET, $_POST) does not take into account php.ini's variables_order variable and (for PHP 5.3.0), request_order variable.

BTW, throwing $_COOKIE out of $_REQUEST breaks phpBB.

[Denis-de-Bernardy]

@vladimir: There was some discussion related to what should end up in $_REQUEST in the related ticket.

FWIW, the whole point of the ticket was to remove $_COOKIES from $_REQUEST for security reasons, without introducing a wp_gpc() function that would end up adding needless overhead. If phpBB uses $_REQUEST where $_COOKIES should be used, they took a very questionable option.

It was decided to ignore the gpc order and to stick to $_GET and $_POST, because there's no straightforward way to know whether $_SERVER and $_ENV are included and in which order. (There are many php ini settings that serve the same purpose.)

[vladimir_kolesnikov]

Thanks for the explanation.

Unless I am missing something, according to ​http://us.php.net/manual/en/reserved.variables.request.php $_REQUEST is always $_GET + $_POST + $_COOKIE and the order depends upon variables_order setting. $_SERVER and $_ENV are not merged into $_REQUEST.

[Denis-de-Bernardy]

yeah, but see also the changelog on the same page (it used to have $_FILES), and:

• ​http://us.php.net/manual/en/ini.core.php#ini.request-order

• ​http://us.php.net/manual/en/ini.core.php#ini.variables-order and its note on S meaning ES

• the discussions on #8814

[vladimir_kolesnikov]

Replying to Denis-de-Bernardy:

• ​http://us.php.net/manual/en/ini.core.php#ini.variables-order and its note on S meaning ES

This only affects $_SERVER - that is, $_SERVER will always have $_ENV array merged in.

See ​http://blog.sjinks.org.ua/p.php (screenshot: ​https://url.odesk.com/bw6ig) - the server has variables_order set to EGPCS but $_REQUEST still contains only $_GET + $_POST + $_COOKIE.

variables_order affects which superglobals are created, the order how G/P/C are put into _REQUEST and the order in which E/G/P/C/S get imported into the global namespace if register_global is on.

​http://us.php.net/manual/en/ini.core.php#ini.request-order

"This directive describes the order in which PHP registers GET, POST and Cookie variables into the _REQUEST array" - the manual does not mention any other superglobals.

Also ​http://us.php.net/manual/en/reserved.variables.request.php: "When running on the command line, this will not include the argv and argc entries; these are present in the $_SERVER array." - this explicitly states that $_SERVER is not a part of $_REQUEST.

Finally, if you take a look at main/php_variables.c in the PHP source (you need the function named php_auto_globals_create_request), you'll see it never uses anything else but G/P/C (screenshot: ​https://url.odesk.com/1eo0y).

[Denis-de-Bernardy]

But that applies to recent php versions, no? I couldn't care less if SERVER and ENV get in there, personally, as long as cookies aren't included. ;-)

[vladimir_kolesnikov]

Replying to Denis-de-Bernardy:

But that applies to recent php versions, no?

4.3.0 behaves the same way but the code is located in main/main.c

[Denis-de-Bernardy]

oh, ok. so then, merging $_GET and $_POST seems like the perfectly valid shortcut in retrospect. Remains to slash it properly.

[hakre]

Using $_REQUEST should just considered bad practice. Whatever is merged in there. Infact you can't properly determine without checking the php configuration and with wordpress even the core code. For any developer I would advise to not use it. Anyway that is just a sidenote.

Since parts of the wordpress code (still) depend on $_REQUEST it should contain at least the same values like $_POST and $_GET (like Denis points out). If $_POST and $_GET is slashed in another way then $_REQUEST is (as this ticket suggests), there is need to have it the same way.

Merging should be placed after salshing $_POST and $_GET.

Additionally an object could be instatiated that abstracts and contains the request data as submitted. This object can then be used to give access to untainted values as well to give access to post, get, files, cookies, (a configured) request and whatever is needed or usefull.

So is there any piece of documentation available wether or not $_REQUEST should contained untainted or slashed data? Even though it does differ from $_POST / $_GET it might be the intended behaviour. For the case it should be the same I can provide a patch.

[dd32]

attachment 10360.diff added

• Moves $_REQUEST merge of GET/POST to after slashing has been taken care of.

[Denis-de-Bernardy]

alternative approach: slash $_REQUEST

[Denis-de-Bernardy]

added a patch that slashes $_REQUEST instead, in case we want to keep $_REQUEST with the same data all along.

[hakre]

Just reviewed the code. According to inline documentation

// Force REQUEST to be GET + POST.  If SERVER, COOKIE, or ENV are needed, use those superglobals directly.

, $_REQUEST is to be expected forced GET + POST. Both are mostly untainted that time so therefore I strongly speak to wontfix or invalid. There is no need to expect $_REQUEST to be slashed.

I do not see any motivation to write a patch then.

[hakre]

Just to remind: If it's the point to not change the way to slash request variables, then this is true for $_REQUEST as well. This Ticket is invalid then.

[dd32]

added a patch that slashes $_REQUEST instead

I didnt see the point in slashing the data twice, and before that block is hit, you cant guarantee what condition the data is in those variables. AFAIK nothing(mission critical) uses any of the request-related globals before then anyway

Just to remind: If it's the point to not change the way to slash request variables, then this is true for $_REQUEST as well. This Ticket is invalid then.

Not sure i can follow that english..

[Denis-de-Bernardy]

hehe, as I wrote, it's "in case we want to keep $_REQUEST with the same data all along" (which I don't think we do either).

@hakre: ticket is not invalid, and not slashing $_REQUEST can introduce potential security issues if a plugin uses it expecting it's always slashed.

[dd32]

Sorry missed your first comment

, $_REQUEST is to be expected forced GET + POST. Both are mostly untainted that time so therefore I strongly speak to wontfix or invalid. There is no need to expect $_REQUEST to be slashed.

Except like what i originally said: (now paraphrased)

WordPress will force $_GET/$_POST to be slashed REGARDLESS OF SERVER SETUP, $_REQUEST doesnt gain the same treatment.

the comment in the code is just to force $_REQUEST to $_GET/$_POST (ie. To exclude $_SERVER and $_COOKIE)

The end result on a Server where Magic quotes are disabled is as such:

POST Data = "O'Niel";
$_POST = "O\'Niel";
$_REQUEST = "O'Niel";

[vladimir_kolesnikov]

Replying to Denis-de-Bernardy:

@hakre: ticket is not invalid, and not slashing $_REQUEST can introduce potential security issues if a plugin uses it expecting it's always slashed.

On the other hand, this might break the plugins that relied upon the old behavior (for example, if I know that $_REQUEST is unslashed, I can pass its variables (without applying stripslashes()) to $wpdb->insert()/$wpdb->update()).

I agree that relying upon $_REQUEST is a bad practice that should be avoided unless you know what you do; what I dislike is that hacking with $_REQUEST breaks 3rd party software.

[hakre]

Replying to dd32:

Just to remind: If it's the point to not change the way to slash request variables, then this is true for $_REQUEST as well. This Ticket is invalid then.

Not sure i can follow that english..

In diverse discussion it has been stated many, many times that there will be no change in the way WordPress slashes the data. This is because of the status quo. So this ticket's report must be bogus as well. Because changing the well accepted behavior of $_REQUEST will change the WordPress behavior of how request variables are slashed.

As it has been clearly said that the way WordPress slahes request related data has not to be touched this ticket is plain invalid.

I hope I can make the point more clear now.

[hakre]

Replying to Denis-de-Bernardy:

@hakre: ticket is not invalid, and not slashing $_REQUEST can introduce potential security issues if a plugin uses it expecting it's always slashed.

The way WordPress handles the request data per se introduces a lot of potential security isseus. So this is ticket is not the right place to pick that topic. Especially having $_REQUEST untainted is a kind of better practice as the pseudo-security with $_POST and $_GET.

[dd32]

We all agree that relying upon slashed data in superglobals is bad. Theres no question about it.

This is about CONSISTENCY.

$_POST['something'] should be able to be replaced by $_REQUEST['something'] and act EXACTLY THE SAME. This is not currently happening due to WordPress's Slashing of data in $_GET/$_POST but NOT in $_REQUEST (Which may be slashed if the server has it enabled, or not slashed otherwise..)

The slashing of data is NOT for this ticket, and another ticket has recently been closed around it.

[hakre]

+1 for removing slashes from _POST and _GET sothat - as dd32 makes bold - _POST and _GET can be replaced anytime with _REQUEST. Plus the point that "we all agree that relying upon slashed data in superglobals is bad." (is the wordpress maintainer part of that "we" or not?)

The currrent patch does not reflect that, it just merges (slashes) _POST & _GET into _REQUEST and not the other way round (dd32 wrote about replacing _POST resp. _GET with _REQUEST and not the other way round).

So I see no has-patch nor commit readyness.

How can we get a valid statement from the maintainer on this issue?

[hakre]

Replying to dd32:

The slashing of data is NOT for this ticket, and another ticket has recently been closed around it.

Please reference that ticket. Thanks.

[dd32]

+1 for removing slashes from _POST and _GET sothat - as dd32 makes bold

My comment was never intended to be taken that way.

I intended that the use of $_POST inside wordpress should be able to be replaced with $_REQUEST at any stage and operate the same. This infers that the slashes in REQUEST should be the same as POST. I am not referring to some non-WP application. Simple fact is, WordPress uses/expects POST/GET data to be slashed, Even though internally data is used unslashed in recent versions. $_REQUEST is NOT slashed, and therefor, cannot be used interchangably

Please reference that ticket. Thanks.

Search for it. I'm not your personal search engine.

[hakre]

("'Quote' on Quote") Simple fact is, WordPress uses/expects the $_REQUEST data not to be slashed. If you say A for _POST and _GET, then you must say B for_REQUEST. If you argue to keep up with the status quo, then this is an argument against changing current $_REQUEST data.

If you decide to change the current slashing of the superglobals containing request data, then I suggest to stop slashing them to improve the dataflow instead of repeating wrong decisions of the past.

Additionally I have only asked that you link to the other ticket you used as argument. I know that there are a lot of tickets that are about slashing or not slashing data so I can not exactly follow your point until you link that certain ticket you meant. That's all. I know how to deal with the trac search, so please do not feel offended by me asking. I thought you have it in your browsers history or similar.

[dd32]

see #10452

If you decide to change the current slashing of the superglobals containing request data, then I suggest to stop slashing them to improve the dataflow instead of repeating wrong decisions of the past.

I would agree, Except that since it was decided in #10452 to not removing slashing (Whch is where the 'remove slashing' discussion belongs) then the request-related vars should follow a similar curve to each other.

I'm going to bring this up in this weeks Developer discussion on IRC.

[hakre]

+1 that really is a good idea. I would love to see a maintainers statement in/after that discussion. I know that this is not easy to decide but I think it should be given a clear statement that leads into the right direction for the future.

Next to what I (personally) would prefer it was for me in WP devrlopment to stick with the current status quo (wether or not it's a personal preference). I'm personally very open minded to have a more well defined data flow (that means to not slash the data as this is consent in the PHP community _mostly_ but not overall (WordPress is one exception, another one might be phplist [not that shure] but then my knowledge ends).

[markjaquith]

I regret that "always slashed" was chosen for $_GET and $_POST, but that ship has sailed. If we want to change that, it's going to be a long journey, because there is a very real possibility that such a change will introduce security issues in plugins. But that is a separate topic.

I agree with dd32 that this is about consistency. Honestly, it was news to me that $_REQUEST is not slashed (not that I'm in the habit of trusting user data!) I regard $_REQUEST as $_GET + $_POST, and both $_GET and $_POST are always slashed—so why wouldn't $_REQUEST always be slashed? There is the possibility that changing this will break a few plugins for some setups, but those plugins were already broken because on other server setups $_REQUEST will be slashed.

Simply put, as it is now, $_REQUEST is unpredictable, and any use of it that would be affected by "slashable" data is currently unstable. dd32's patch fixes this, by making it consistent (with itself) and consistend with $_GET and $_POST, as well as fixes it in a way that won't introduce SQL injection vulnerabilities.

[markjaquith]

(In [11760]) Be consistent about slashing _REQUEST superglobal. props dd32. fixes #10360

[hakre]

Then so shall it be.