Page MenuHomePhabricator

WebRequest input handling cleanup: use $_GET and $_POST, not $_REQUEST
Closed, ResolvedPublic

Description

WebRequest is currently a little wonky in how it handles some things. It would be nice to do a cleanup, changing a few properties.

Currently WebRequest uses $_REQUEST as its data source, which contains both $_GET and $_POST data for convenience.

But it also contains $_COOKIE data, which may lead to unexpected behavior. If another tool on the site sets a cookie whose name conflicts with one of MediaWiki's expected input parameters, Weird Things can happen as "phantom" input keeps showing up.

Polling $_GET and $_POST in order instead of $_REQUEST would avoid this.


Version: unspecified
Severity: enhancement

Details

Reference
bz11559

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:00 PM
bzimport set Reference to bz11559.
bzimport added a subscriber: Unknown Object (MLST).

Created attachment 4209
Proposed patch

Ok, here's a patch that makes #wgRequest to use only $_GET and $_POST, not $_COOKIES and $_FILES

attachment request-filter.patch ignored as obsolete

Created attachment 4210
Another patch

Ok, another more reviewed patch

Attached:

My one concern with this is that it'll be re-generating the merged array consisting of *all* input on every access for *any* input.

When dealing with large form submissions that doesn't sound very efficient, and could bloat our already-obscene memory usage. (Might be worth checking if that's actually so, though.)

Might be cleaner to do this as a fallback: have a list of data sources and check each source in turn.

robchur wrote:

(In reply to comment #3)

My one concern with this is that it'll be re-generating the merged array
consisting of *all* input on every access for *any* input.

You could potentially do some lazy-initialisation to counter this.

On some really unscientific testing, it looks like the array_merge() won't significantly boost memory usage (the strings are apparently refcounted and not duplicated as long as they're not changed, yay?).

The preemptive stripslashes()ing (bug 11558), though, *does* increase memory usage throughout the lifetime of the script -- it looks like both the original strings and the stripped strings sit in memory until the script dies, whereas an on-demand stripped string can be freed once it's out of scope.

Fun. :D

Went ahead and swapped out the $_REQUESTs for a merged array of $_GET and $_POST in r30882.

The merged array is being initialized in the constructor, right after magic quotes removal. Per my notes in above comment, this shouldn't alter memory usage much at present.

Over the previous patch, this fixes some other cases by ensuring all operations against $_REQUEST use the same data set.

Bug 7681 was an example of the cookie processing in $_REQUEST causing unexpected results. Now happily fixed. :)