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

diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34
index 275f4c2..c9c96c2 100644 (file)
--- 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 0fb674b..fe13263 100644 (file)
--- 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 bbaa10f..4d7195c 100644 (file)
--- 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 d8134bb..d10b915 100644 (file)
--- 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 13dbc0e..f425d87 100644 (file)
--- 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();