From 0c0676c34eeced65847f20b896049070a766a532 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Tue, 3 Sep 2019 10:25:19 +1000 Subject: [PATCH] Stop mangling $_GET and provide WebRequest::getQueryValuesOnly() I doubt there was ever a good reason for mangling $_GET to add the title, this was just b/c for the sake of b/c. It was formerly used in core but that was so long ago that I doubt there was any usage in extensions at the time. Now there is one usage of $_GET['title'] in an unmaintained extension, but it was only added in 2017. Also I added WebRequest::getQueryValuesOnly() which is an interface to the unmodified $_GET. The motivation is allowing OAuth to work with the REST API, since OAuth needs an unmangled view of $_GET for signature generation. The Action API gets around the problem with a special hack in interpolateTitle(), disabling it for the Action API only. A review of callers of getQueryValues() suggests that many would benefit from using getQueryValuesOnly() instead. But I only changed it for callers in api.php and thumb.php since the effect of the change there is certainly beneficial, whereas callers under index.php may possibly be using the path parameters to construct self-links. Rest\RequestFromGlobals uses $_GET directly, which means that this change causes it to not return PathRouter matches as GET parameters anymore. Change-Id: Ic469577fae17c0b1ac69466df7bc9f03e61c74e3 --- RELEASE-NOTES-1.34 | 2 ++ api.php | 2 +- includes/WebRequest.php | 44 +++++++++++++++++++++++++++++++++++----- includes/api/ApiBase.php | 2 +- thumb.php | 2 +- 5 files changed, 44 insertions(+), 8 deletions(-) diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 275f4c2fb5..c9c96c29ed 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -360,6 +360,8 @@ because of Phabricator reports. initialized after calling SearchResult::initFromTitle(). * The UserIsBlockedFrom hook is only called if a block is found first, and should only be used to unblock a blocked user. +* Parameters for index.php from PATH_INFO, such as the title, are no longer + written to $_GET. * … === Deprecations in 1.34 === diff --git a/api.php b/api.php index 0fb674b9eb..fe13263921 100644 --- a/api.php +++ b/api.php @@ -44,7 +44,7 @@ if ( !$wgRequest->checkUrlExtension() ) { // PATH_INFO can be used for stupid things. We don't support it for api.php at // all, so error out if it's present. if ( isset( $_SERVER['PATH_INFO'] ) && $_SERVER['PATH_INFO'] != '' ) { - $correctUrl = wfAppendQuery( wfScript( 'api' ), $wgRequest->getQueryValues() ); + $correctUrl = wfAppendQuery( wfScript( 'api' ), $wgRequest->getQueryValuesOnly() ); $correctUrl = wfExpandUrl( $correctUrl, PROTO_CANONICAL ); header( "Location: $correctUrl", true, 301 ); echo 'This endpoint does not support "path info", i.e. extra text between "api.php"' diff --git a/includes/WebRequest.php b/includes/WebRequest.php index bbaa10fea3..4d7195c532 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -39,9 +39,28 @@ use MediaWiki\Session\SessionManager; * @ingroup HTTP */ class WebRequest { - /** @var array */ + /** + * The parameters from $_GET, $_POST and the path router + * @var array + */ protected $data; - /** @var array */ + + /** + * The parameters from $_GET. The parameters from the path router are + * added by interpolateTitle() during Setup.php. + * @var array + */ + protected $queryAndPathParams; + + /** + * The parameters from $_GET only. + */ + protected $queryParams; + + /** + * Lazy-initialized request headers indexed by upper-case header name + * @var array + */ protected $headers = []; /** @@ -99,6 +118,8 @@ class WebRequest { // POST overrides GET data // We don't use $_REQUEST here to avoid interference from cookies... $this->data = $_POST + $_GET; + + $this->queryAndPathParams = $this->queryParams = $_GET; } /** @@ -332,7 +353,7 @@ class WebRequest { $matches = self::getPathInfo( 'title' ); foreach ( $matches as $key => $val ) { - $this->data[$key] = $_GET[$key] = $_REQUEST[$key] = $val; + $this->data[$key] = $this->queryAndPathParams[$key] = $val; } } @@ -664,14 +685,27 @@ class WebRequest { } /** - * Get the values passed in the query string. + * Get the values passed in the query string and the path router parameters. * No transformation is performed on the values. * * @codeCoverageIgnore * @return array */ public function getQueryValues() { - return $_GET; + return $this->queryAndPathParams; + } + + /** + * Get the values passed in the query string only, not including the path + * router parameters. This is less suitable for self-links to index.php but + * useful for other entry points. No transformation is performed on the + * values. + * + * @since 1.34 + * @return array + */ + public function getQueryValuesOnly() { + return $this->queryParams; } /** diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index d8134bb6b7..d10b915076 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -992,7 +992,7 @@ abstract class ApiBase extends ContextSource { return; } - $queryValues = $this->getRequest()->getQueryValues(); + $queryValues = $this->getRequest()->getQueryValuesOnly(); $badParams = []; foreach ( $params as $param ) { if ( $prefix !== 'noprefix' ) { diff --git a/thumb.php b/thumb.php index 13dbc0e767..f425d87e59 100644 --- a/thumb.php +++ b/thumb.php @@ -35,7 +35,7 @@ if ( defined( 'THUMB_HANDLER' ) ) { wfThumbHandle404(); } else { // Called directly, use $_GET params - wfStreamThumb( $wgRequest->getQueryValues() ); + wfStreamThumb( $wgRequest->getQueryValuesOnly() ); } $mediawiki = new MediaWiki(); -- 2.20.1