Last modified: 2008-02-12 22:11:26 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T13559, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 11559 - WebRequest input handling cleanup: use $_GET and $_POST, not $_REQUEST
WebRequest input handling cleanup: use $_GET and $_POST, not $_REQUEST
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
General/Unknown (Other open bugs)
unspecified
All All
: Normal enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: 7681
  Show dependency treegraph
 
Reported: 2007-10-04 14:46 UTC by Brion Vibber
Modified: 2008-02-12 22:11 UTC (History)
1 user (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Proposed patch (446 bytes, patch)
2007-10-04 15:21 UTC, Victor Vasiliev
Details
Another patch (987 bytes, patch)
2007-10-04 17:01 UTC, Victor Vasiliev
Details

Description Brion Vibber 2007-10-04 14:46:12 UTC
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.
Comment 1 Victor Vasiliev 2007-10-04 15:21:31 UTC
Created attachment 4209 [details]
Proposed patch

Ok, here's a patch that makes #wgRequest to use only $_GET and $_POST, not $_COOKIES and $_FILES
Comment 2 Victor Vasiliev 2007-10-04 17:01:02 UTC
Created attachment 4210 [details]
Another patch

Ok, another more reviewed patch
Comment 3 Brion Vibber 2007-10-04 18:17:58 UTC
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.
Comment 4 Rob Church 2007-10-04 18:23:37 UTC
(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.
Comment 5 Brion Vibber 2007-10-04 19:12:50 UTC
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
Comment 6 Brion Vibber 2008-02-12 22:11:26 UTC
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. :)

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links