Brian Wolff has uploaded a new change for review. ( 
https://gerrit.wikimedia.org/r/395754 )

Change subject: Support checking getQueryInfo() return; Process $options & 
$join_conds
......................................................................

Support checking getQueryInfo() return; Process $options & $join_conds

This now supports looking at the return of getQueryInfo()
(e.g. QueryPages, Pager subclasses) as if it was a call to
IDatabase::select.

This adds special casing for the $options and $join_cond argument
to IDatabase::select, as the format is rather complex.

Change-Id: I13f0d72419c2f28a7a954e15243b84119e0ac8a4
---
M src/MWVisitor.php
M src/MediaWikiSecurityCheckPlugin.php
M tests/general-phan-config.php
M tests/integration/arraynumkey/db.php
M tests/integration/arraynumkey/expectedResults.txt
M tests/integration/arraynumkey/test.php
A tests/integration/getqueryinfo/db.php
A tests/integration/getqueryinfo/expectedResults.txt
A tests/integration/getqueryinfo/test.php
9 files changed, 441 insertions(+), 25 deletions(-)


  git pull 
ssh://gerrit.wikimedia.org:29418/mediawiki/tools/phan/SecurityCheckPlugin 
refs/changes/54/395754/1

diff --git a/src/MWVisitor.php b/src/MWVisitor.php
index 925d62f..7353bed 100644
--- a/src/MWVisitor.php
+++ b/src/MWVisitor.php
@@ -5,6 +5,7 @@
 use Phan\Language\FQSEN\FullyQualifiedFunctionName;
 use Phan\Language\FQSEN\FullyQualifiedFunctionLikeName;
 use Phan\Language\FQSEN;
+use Phan\Language\Element\Method;
 use Phan\Language\Type\CallableType;
 use Phan\Plugin;
 use Phan\CodeBase;
@@ -82,6 +83,61 @@
                        }
                } catch ( Exception $e ) {
                        // ignore
+               }
+
+               $this->doSelectWrapperSpecialHandling( $node, $method );
+       }
+
+       /**
+        * Special casing for complex format of IDatabase::select
+        *
+        * This handled the $options, and $join_cond. Other args are
+        * handled through normal means
+        *
+        * @param Node $node Either an AST_METHOD_CALL or AST_STATIC_CALL
+        * @param Method $method
+        */
+       private function doSelectWrapperSpecialHandling( Node $node, Method 
$method ) {
+               $clazz = $method->getClass( $this->code_base );
+
+               $implementIDB = false;
+               do {
+                       $interfaceList = $clazz->getInterfaceFQSENList();
+                       foreach ( $interfaceList as $interface ) {
+                               if ( (string)$interface === 
'\\Wikimedia\\Rdbms\\IDatabase' ) {
+                                       $implementIDB = true;
+                                       break 2;
+                               }
+                       }
+               // @codingStandardsIgnoreStart 
MediaWiki.ControlStructures.AssignmentInControlStructures.AssignmentInControlStructures
+               } while (
+                       $clazz->hasParentType() &&
+                       ( $clazz = $clazz->getParentClass( $this->code_base ) )
+               );
+               // @codingStandardsIgnoreEnd
+
+               if ( !$implementIDB ) {
+                       return;
+               }
+
+               $relevantMethods = [
+                       'select',
+                       'selectField',
+                       'selectFieldValues',
+                       'selectSQLText',
+                       'selectRowCount'
+               ];
+
+               if ( !in_array( $method->getName(), $relevantMethods ) ) {
+                       return;
+               }
+
+               $args = $node->children['args']->children;
+               if ( isset( $args[4] ) ) {
+                       $this->checkSQLOptions( $args[4] );
+               }
+               if ( isset( $args[5] ) ) {
+                       $this->checkJoinCond( $args[5] );
                }
        }
 
@@ -250,6 +306,11 @@
                        return;
                }
                $funcFQSEN = $this->context->getFunctionLikeFQSEN();
+
+               if ( strpos( (string)$funcFQSEN, '::getQueryInfo' ) !== false ) 
{
+                       $this->handleGetQueryInfoReturn( 
$node->children['expr'] );
+               }
+
                $hookType = $this->plugin->isSpecialHookSubscriber( $funcFQSEN 
);
                switch ( $hookType ) {
                case '!ParserFunctionHook':
@@ -269,6 +330,167 @@
        }
 
        /**
+        * Methods named getQueryInfo() in MediaWiki usually
+        * return an array that is later fed to select
+        *
+        * @note This will only work where the return
+        *  statement is an array literal.
+        * @param Node|Mixed $node Node from ast tree
+        * @suppress PhanTypeMismatchForeach
+        */
+       private function handleGetQueryInfoReturn( $node ) {
+               if (
+                       !( $node instanceof Node ) ||
+                       $node->kind !== \ast\AST_ARRAY
+               ) {
+                       return;
+               }
+               // The argument order is
+               // $table, $vars, $conds = '', $fname = __METHOD__,
+               // $options = [], $join_conds = []
+               $keysToArg = [
+                       'tables' => 0,
+                       'fields' => 1,
+                       'conds' => 2,
+                       'options' => 4,
+                       'join_conds' => 5,
+               ];
+               $args = [ '', '', '', '' ];
+               foreach ( $node->children as $child ) {
+                       assert( $child->kind === \ast\AST_ARRAY_ELEM );
+                       if ( !isset( $keysToArg[$child->children['key']] ) ) {
+                               continue;
+                       }
+                       $args[$keysToArg[$child->children['key']]] = 
$child->children['value'];
+               }
+               $selectFQSEN = 
FullyQualifiedMethodName::fromFullyQualifiedString(
+                       '\Wikimedia\Rdbms\IDatabase::select'
+               );
+               $select = $this->code_base->getMethodByFQSEN( $selectFQSEN );
+               $taint = $this->getTaintOfFunction( $select );
+               $this->handleMethodCall( $select, $selectFQSEN, $taint, $args );
+               if ( isset( $args[4] ) ) {
+                       $this->checkSQLOptions( $args[4] );
+               }
+               if ( isset( $args[5] ) ) {
+                       $this->checkJoinCond( $args[5] );
+               }
+       }
+
+       /**
+        * Check the options parameter to IDatabase::select
+        *
+        * This only works if its specified as an array literal.
+        *
+        * Relavent options:
+        *  GROUP BY is put directly in the query (array get's imploded)
+        *  HAVING is treated like a WHERE clause
+        *  ORDER BY is put directly in the query (array get's imploded)
+        *  USE INDEX is directly put in string (both array and string version)
+        *  IGNORE INDEX ditto
+        * @param Node|mixed $node The node from the AST tree
+        */
+       private function checkSQLOptions( $node ) {
+               if ( !( $node instanceof Node ) || $node->kind !== 
\ast\AST_ARRAY ) {
+                       return;
+               }
+               $relevant = [
+                       'GROUP BY',
+                       'ORDER BY',
+                       'HAVING',
+                       'USE INDEX',
+                       'IGNORE INDEX',
+               ];
+               foreach ( $node->children as $arrayElm ) {
+                       assert( $arrayElm->kind === \ast\AST_ARRAY_ELEM );
+                       $val = $arrayElm->children['value'];
+                       $key = $arrayElm->children['key'];
+                       $taintType = ( $key === 'HAVING' && $this->nodeIsArray( 
$val ) ) ?
+                               SecurityCheckPlugin::SQL_NUMKEY_EXEC_TAINT :
+                               SecurityCheckPlugin::SQL_EXEC_TAINT;
+
+                       if ( in_array( $key, $relevant ) ) {
+                               $ctx = clone $this->context;
+                               $this->overrideContext = 
$ctx->withLineNumberStart(
+                                       $val->lineno ?? 
$ctx->getLineNumberStart()
+                               );
+                               $this->maybeEmitIssue(
+                                       $taintType,
+                                       $this->getTaintedness( $val ),
+                                       "$key clause is user controlled"
+                                               . $this->getOriginalTaintLine( 
$val )
+                               );
+                               $this->overrideContext = null;
+                       }
+               }
+       }
+
+       /**
+        * Check a join_cond structure.
+        *
+        * Syntax is like
+        *
+        *  [ 'aliasOfTable' => [ 'JOIN TYPE', $onConditions ], ... ]
+        *  join type is usually something safe like INNER JOIN, but it is not
+        *  validated or escaped. $onConditions is the same form as a WHERE 
clause.
+        *
+        * @param Node|mixed $node
+        */
+       private function checkJoinCond( $node ) {
+               if ( !( $node instanceof Node ) || $node->kind !== 
\ast\AST_ARRAY ) {
+                       return;
+               }
+               foreach ( $node->children as $table ) {
+                       assert( $table->kind === \ast\AST_ARRAY_ELEM );
+
+                       $tableName = is_string( $table->children['key'] ) ?
+                               $table->children['key'] :
+                               '[UNKNOWN TABLE]';
+                       $joinInfo = $table->children['value'];
+                       if (
+                               $joinInfo instanceof Node
+                               && $joinInfo->kind === \ast\AST_ARRAY
+                       ) {
+                               if (
+                                       count( $joinInfo->children ) === 0 ||
+                                       $joinInfo->children[0]->children['key'] 
!== null
+                               ) {
+                                       $this->debug( __METHOD__, "join info 
has named key??" );
+                                       continue;
+                               }
+                               $joinType = 
$joinInfo->children[0]->children['value'];
+                               // join type does not get escaped.
+                               $this->maybeEmitIssue(
+                                       SecurityCheckPlugin::SQL_EXEC_TAINT,
+                                       $this->getTaintedness( $joinType ),
+                                       "join type for $tableName is user 
controlled"
+                                               . $this->getOriginalTaintLine( 
$joinType )
+                               );
+                               // On to the join ON conditions.
+                               if (
+                                       count( $joinInfo->children ) === 1 ||
+                                       $joinInfo->children[1]->children['key'] 
!== null
+                               ) {
+                                       $this->debug( __METHOD__, "join info 
has named key??" );
+                                       continue;
+                               }
+                               $onCond = 
$joinInfo->children[1]->children['value'];
+                               $ctx = clone $this->context;
+                               $this->overrideContext = 
$ctx->withLineNumberStart(
+                                       $onCond->lineno ?? 
$ctx->getLineNumberStart()
+                               );
+                               $this->maybeEmitIssue(
+                                       
SecurityCheckPlugin::SQL_NUMKEY_EXEC_TAINT,
+                                       $this->getTaintedness( $onCond ),
+                                       "The ON conditions are not properly 
escaped for the join to `$tableName`"
+                                               . $this->getOriginalTaintLine( 
$onCond )
+                               );
+                               $this->overrideContext = null;
+                       }
+               }
+       }
+
+       /**
         * Check to see if isHTML => true and is tainted.
         *
         * @param Node $node The expr child of the return. NOT the return itself
diff --git a/src/MediaWikiSecurityCheckPlugin.php 
b/src/MediaWikiSecurityCheckPlugin.php
index 2c25a07..06fb4a9 100644
--- a/src/MediaWikiSecurityCheckPlugin.php
+++ b/src/MediaWikiSecurityCheckPlugin.php
@@ -58,6 +58,23 @@
         * @inheritDoc
         */
        protected function getCustomFuncTaints() : array {
+               $selectWrapper = [
+                               self::SQL_EXEC_TAINT,
+                               // List of fields. MW does not escape things 
like COUNT(*)
+                               self::SQL_EXEC_TAINT,
+                               // Where conditions
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               // the function name doesn't seem to be escaped
+                               self::SQL_EXEC_TAINT,
+                               // OPTIONS. Its complicated. HAVING is like 
WHERE
+                               // This is treated as special case
+                               self::NO_TAINT,
+                               // Join conditions. This is treated as special 
case
+                               self::NO_TAINT,
+                               // What should DB results be considered?
+                               'overall' => self::YES_TAINT
+               ];
+
                return [
                        // Note, at the moment, this checks where the function
                        // is implemented, so you can't use IDatabase.
@@ -76,42 +93,92 @@
                                // What should DB results be considered?
                                'overall' => self::YES_TAINT
                        ],
-                       '\Wikimedia\Rdbms\IDatabase::select' => [
-                               self::SQL_EXEC_TAINT,
+                       '\Wikimedia\Rdbms\IDatabase::select' => $selectWrapper,
+                       '\Wikimedia\Rdbms\Database::select' => $selectWrapper,
+                       '\Wikimedia\Rdbms\DBConnRef::select' => $selectWrapper,
+                       '\Wikimedia\Rdbms\IDatabase::selectField' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\Database::selectField' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\DBConnRef::selectField' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\IDatabase::selectFieldValues' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\DBConnRef::selectFieldValues' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\Database::selectFieldValues' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\IDatabase::selectSQLText' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\DBConnRef::selectSQLText' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\Database::selectSQLText' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\IDatabase::selectRowCount' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\Database::selectRowCount' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\DBConnRef::selectRowCount' => 
$selectWrapper,
+                       '\Wikimedia\Rdbms\IDatabase::delete' => [
                                self::SQL_EXEC_TAINT,
                                self::SQL_NUMKEY_EXEC_TAINT,
-                               // the function name doesn't seem to be escaped
                                self::SQL_EXEC_TAINT,
-                               // I'm not even sure for options
-                               self::SQL_EXEC_TAINT,
-                               self::SQL_NUMKEY_EXEC_TAINT,
-                               // What should DB results be considered?
-                               'overall' => self::YES_TAINT
+                               'overall' => self::NO_TAINT
                        ],
-                       '\Wikimedia\Rdbms\Database::select' => [
-                               self::SQL_EXEC_TAINT,
+                       '\Wikimedia\Rdbms\Database::delete' => [
                                self::SQL_EXEC_TAINT,
                                self::SQL_NUMKEY_EXEC_TAINT,
-                               // the function name doesn't seem to be escaped
                                self::SQL_EXEC_TAINT,
-                               // I'm not even sure for options
-                               self::SQL_EXEC_TAINT,
-                               self::SQL_NUMKEY_EXEC_TAINT,
-                               // What should DB results be considered?
-                               'overall' => self::YES_TAINT
+                               'overall' => self::NO_TAINT
                        ],
-                       '\Wikimedia\Rdbms\DBConnRef::select' => [
-                               self::SQL_EXEC_TAINT,
+                       '\Wikimedia\Rdbms\DBConnRef::delete' => [
                                self::SQL_EXEC_TAINT,
                                self::SQL_NUMKEY_EXEC_TAINT,
-                               // the function name doesn't seem to be escaped
                                self::SQL_EXEC_TAINT,
-                               // I'm not even sure for options
-                               self::SQL_EXEC_TAINT,
-                               self::SQL_NUMKEY_EXEC_TAINT,
-                               // What should DB results be considered?
-                               'overall' => self::YES_TAINT
+                               'overall' => self::NO_TAINT
                        ],
+                       '\Wikimedia\Rdbms\IDatabase::insert' => [
+                               self::SQL_EXEC_TAINT, // table name
+                               // Insert values. The keys names are unsafe.
+                               // Unclear how well this works for the multi 
case.
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               self::SQL_EXEC_TAINT, // method name
+                               self::SQL_EXEC_TAINT, // options. They are not 
escaped
+                               'overall' => self::NO_TAINT
+                       ],
+                       '\Wikimedia\Rdbms\Database::insert' => [
+                               self::SQL_EXEC_TAINT, // table name
+                               // Insert values. The keys names are unsafe.
+                               // Unclear how well this works for the multi 
case.
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               self::SQL_EXEC_TAINT, // method name
+                               self::SQL_EXEC_TAINT, // options. They are not 
escaped
+                               'overall' => self::NO_TAINT
+                       ],
+                       '\Wikimedia\Rdbms\DBConnRef::insert' => [
+                               self::SQL_EXEC_TAINT, // table name
+                               // Insert values. The keys names are unsafe.
+                               // Unclear how well this works for the multi 
case.
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               self::SQL_EXEC_TAINT, // method name
+                               self::SQL_EXEC_TAINT, // options. They are not 
escaped
+                               'overall' => self::NO_TAINT
+                       ],
+                       '\Wikimedia\Rdbms\IDatabase::update' => [
+                               self::SQL_EXEC_TAINT, // table name
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               self::SQL_EXEC_TAINT, // method name
+                               self::NO_TAINT, // options. They are validated
+                               'overall' => self::NO_TAINT
+                       ],
+                       '\Wikimedia\Rdbms\Database::update' => [
+                               self::SQL_EXEC_TAINT, // table name
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               self::SQL_EXEC_TAINT, // method name
+                               self::NO_TAINT, // options. They are validated
+                               'overall' => self::NO_TAINT
+                       ],
+                       '\Wikimedia\Rdbms\DBConnRef::update' => [
+                               self::SQL_EXEC_TAINT, // table name
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               self::SQL_NUMKEY_EXEC_TAINT,
+                               self::SQL_EXEC_TAINT, // method name
+                               self::NO_TAINT, // options. They are validated
+                               'overall' => self::NO_TAINT
+                       ],
+                       // This is subpar, as addIdentifierQuotes isn't always
+                       // the right type of escaping.
                        '\Wikimedia\Rdbms\Database::addIdentifierQuotes' => [
                                self::YES_TAINT & ~self::SQL_TAINT,
                                'overall' => self::NO_TAINT,
diff --git a/tests/general-phan-config.php b/tests/general-phan-config.php
index 4f8c262..7907ccf 100644
--- a/tests/general-phan-config.php
+++ b/tests/general-phan-config.php
@@ -262,7 +262,9 @@
         * to this black-list to inhibit them from being reported.
         */
        'suppress_issue_types' => [
-               // 'PhanDeprecatedProperty',
+               // Phan seems to have a bug when it comes
+               // to doing foreach's of children array of \ast\Node's
+               'PhanTypeMismatchForeach'
        ],
        /*'suppress_issue_types' => [
                // approximate error count: 8
diff --git a/tests/integration/arraynumkey/db.php 
b/tests/integration/arraynumkey/db.php
index c607198..7186cfe 100644
--- a/tests/integration/arraynumkey/db.php
+++ b/tests/integration/arraynumkey/db.php
@@ -3,6 +3,10 @@
 
 interface IDatabase {
        public function query( $sql, $method );
+       public function select(
+               $table, $vars, $conds = '', $fname = __METHOD__,
+               $options = [], $join_conds = []
+       );
 }
 
 class Database implements IDatabase {
diff --git a/tests/integration/arraynumkey/expectedResults.txt 
b/tests/integration/arraynumkey/expectedResults.txt
index ec727f1..949e140 100644
--- a/tests/integration/arraynumkey/expectedResults.txt
+++ b/tests/integration/arraynumkey/expectedResults.txt
@@ -5,3 +5,5 @@
 integration/arraynumkey/test.php:45 SecurityCheck-SQLInjection Calling method 
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using 
tainted argument $where3. (Caused by: integration/arraynumkey/test.php +44)
 integration/arraynumkey/test.php:50 SecurityCheck-SQLInjection Calling method 
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using 
tainted argument $where4.
 integration/arraynumkey/test.php:54 SecurityCheck-SQLInjection Calling method 
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using 
tainted argument $where5. (Caused by: integration/arraynumkey/test.php +53)
+integration/arraynumkey/test.php:59 SecurityCheck-SQLInjection The ON 
conditions are not properly escaped for the join to `t` (Caused by: 
integration/arraynumkey/test.php +53) (Originally at: 
integration/arraynumkey/test.php:57)
+integration/arraynumkey/test.php:80 SecurityCheck-SQLInjection HAVING clause 
is user controlled (Originally at: integration/arraynumkey/test.php:78)
diff --git a/tests/integration/arraynumkey/test.php 
b/tests/integration/arraynumkey/test.php
index ca63c55..7b16917 100644
--- a/tests/integration/arraynumkey/test.php
+++ b/tests/integration/arraynumkey/test.php
@@ -52,3 +52,31 @@
 // unsafe
 $where5 = [ 1 => $_GET['d'] ];
 $db->select( 't', 'f', $where5 );
+
+// unsafe
+$db->select( 't', 'f', '', __METHOD__, [],
+       [
+               't' => [ 'INNER JOIN', $where5 ]
+       ]
+);
+
+// unsafe
+$db->select( 't', 'f', '', __METHOD__, [],
+       [
+               't' => [ 'INNER JOIN', $_GET['string'] ]
+       ]
+);
+
+// unsafe
+$db->select( 't', 'f', '', __METHOD__, [],
+       [
+               'HAVING' => $where5
+       ]
+);
+
+// unsafe
+$db->select( 't', 'f', '', __METHOD__,
+       [
+               'HAVING' => $_GET['string']
+       ]
+);
diff --git a/tests/integration/getqueryinfo/db.php 
b/tests/integration/getqueryinfo/db.php
new file mode 100644
index 0000000..7186cfe
--- /dev/null
+++ b/tests/integration/getqueryinfo/db.php
@@ -0,0 +1,30 @@
+<?php
+namespace Wikimedia\Rdbms;
+
+interface IDatabase {
+       public function query( $sql, $method );
+       public function select(
+               $table, $vars, $conds = '', $fname = __METHOD__,
+               $options = [], $join_conds = []
+       );
+}
+
+class Database implements IDatabase {
+       public function query( $sql, $method ) {
+               // do some stuff
+               return (object)[ 'some_field' => 'some value' ];
+       }
+
+       public function select(
+               $table, $vars, $conds = '', $fname = __METHOD__,
+               $options = [], $join_conds = []
+       ) {
+               return (object)[ 'some_field' => 'some value' ];
+       }
+}
+
+class MysqlDatabase extends Database {
+       public function getType() {
+               return 'mysql';
+       }
+}
diff --git a/tests/integration/getqueryinfo/expectedResults.txt 
b/tests/integration/getqueryinfo/expectedResults.txt
new file mode 100644
index 0000000..4334a35
--- /dev/null
+++ b/tests/integration/getqueryinfo/expectedResults.txt
@@ -0,0 +1,5 @@
+integration/getqueryinfo/test.php:14 SecurityCheck-SQLInjection Calling method 
\Wikimedia\Rdbms\IDatabase::select() in \MySpecialPage::getQueryInfo that 
outputs using tainted argument $[arg #2]. (Caused by: 
integration/getqueryinfo/test.php +12)
+integration/getqueryinfo/test.php:14 SecurityCheck-SQLInjection Calling method 
\Wikimedia\Rdbms\IDatabase::select() in \MySpecialPage::getQueryInfo that 
outputs using tainted argument $[arg #3].
+integration/getqueryinfo/test.php:22 SecurityCheck-SQLInjection HAVING clause 
is user controlled (Originally at: integration/getqueryinfo/test.php:14)
+integration/getqueryinfo/test.php:24 SecurityCheck-SQLInjection GROUP BY 
clause is user controlled (Originally at: integration/getqueryinfo/test.php:14)
+integration/getqueryinfo/test.php:32 SecurityCheck-SQLInjection The ON 
conditions are not properly escaped for the join to `page` (Originally at: 
integration/getqueryinfo/test.php:14)
diff --git a/tests/integration/getqueryinfo/test.php 
b/tests/integration/getqueryinfo/test.php
new file mode 100644
index 0000000..58db9aa
--- /dev/null
+++ b/tests/integration/getqueryinfo/test.php
@@ -0,0 +1,56 @@
+<?php
+class SpecialPage {
+}
+class QueryPage extends SpecialPage {
+       public function getQueryInfo() {
+               return null;
+       }
+}
+
+class MySpecialPage extends QueryPage {
+       public function getQueryInfo() {
+               $prefix = $_GET['prefix'];
+               return [
+                       'tables' => [ 'pagelinks' ],
+                       'fields' => [
+                               'namespace' => 'pl_namespace',
+                               'title' => "CONCAT( '$prefix', pl_title)",
+                               'value' => 'COUNT(*)',
+                               'page_namespace'
+                       ],
+                       'options' => [
+                               'HAVING' => $_GET['evil'],
+                               'GROUP BY' => [
+                                       'pl_namespace', 'pl_title',
+                                       'page_namespace', $_GET['evil']
+                               ]
+                       ],
+                       'join_conds' => [
+                               'page' => [
+                                       'LEFT JOIN',
+                                       [
+                                               'page_namespace = pl_namespace',
+                                               'page_title = pl_title',
+                                               "page_size = " . $_GET['size']
+                                       ]
+                               ]
+                       ],
+                       'conds' => [
+                               $_GET['somethingEvil']
+                       ]
+               ];
+       }
+}
+
+class MySpecialPage2 extends QueryPage {
+       public function getQueryInfo() {
+               // This should be safe.
+               return [
+                       'tables' => 'someTable',
+                       'fields' => [ 'value' => 'COUNT(*)' ],
+                       'options' => [
+                               'HAVING' => [ 'value' => $_GET['count'] ]
+                       ]
+               ];
+       }
+}

-- 
To view, visit https://gerrit.wikimedia.org/r/395754
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I13f0d72419c2f28a7a954e15243b84119e0ac8a4
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/tools/phan/SecurityCheckPlugin
Gerrit-Branch: master
Gerrit-Owner: Brian Wolff <bawolff...@gmail.com>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to