Brian Wolff has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/395758 )
Change subject: Minor fixes discovered during testing ...................................................................... Minor fixes discovered during testing Change-Id: I5e0117cc41e6504537d0fa7d4926d354728746eb --- M src/MWVisitor.php M src/MediaWikiSecurityCheckPlugin.php M src/PreTaintednessVisitor.php M src/TaintednessBaseVisitor.php M src/TaintednessVisitor.php A tests/integration/factory/expectedResults.txt A tests/integration/factory/get.php A tests/integration/factory/test.php 8 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/MWVisitor.php b/src/MWVisitor.php index 7353bed..24c1328 100644 --- a/src/MWVisitor.php +++ b/src/MWVisitor.php @@ -80,12 +80,12 @@ case '\Hooks::run': $this->triggerHook( $node ); break; + default: + $this->doSelectWrapperSpecialHandling( $node, $method ); } } catch ( Exception $e ) { // ignore } - - $this->doSelectWrapperSpecialHandling( $node, $method ); } /** @@ -125,7 +125,8 @@ 'selectField', 'selectFieldValues', 'selectSQLText', - 'selectRowCount' + 'selectRowCount', + 'selectRow' ]; if ( !in_array( $method->getName(), $relevantMethods ) ) { diff --git a/src/MediaWikiSecurityCheckPlugin.php b/src/MediaWikiSecurityCheckPlugin.php index 06fb4a9..e67f6d9 100644 --- a/src/MediaWikiSecurityCheckPlugin.php +++ b/src/MediaWikiSecurityCheckPlugin.php @@ -108,6 +108,9 @@ '\Wikimedia\Rdbms\IDatabase::selectRowCount' => $selectWrapper, '\Wikimedia\Rdbms\Database::selectRowCount' => $selectWrapper, '\Wikimedia\Rdbms\DBConnRef::selectRowCount' => $selectWrapper, + '\Wikimedia\Rdbms\IDatabase::selectRow' => $selectWrapper, + '\Wikimedia\Rdbms\Database::selectRow' => $selectWrapper, + '\Wikimedia\Rdbms\DBConnRef::selectRow' => $selectWrapper, '\Wikimedia\Rdbms\IDatabase::delete' => [ self::SQL_EXEC_TAINT, self::SQL_NUMKEY_EXEC_TAINT, diff --git a/src/PreTaintednessVisitor.php b/src/PreTaintednessVisitor.php index bc146f0..d45e5c8 100644 --- a/src/PreTaintednessVisitor.php +++ b/src/PreTaintednessVisitor.php @@ -48,7 +48,7 @@ if ( $value->kind !== \ast\AST_VAR ) { $this->debug( __METHOD__, "FIXME foreach complex case not handled" ); - Debug::printNode( $node ); + // Debug::printNode( $node ); return; } diff --git a/src/TaintednessBaseVisitor.php b/src/TaintednessBaseVisitor.php index a1957d2..40c644a 100644 --- a/src/TaintednessBaseVisitor.php +++ b/src/TaintednessBaseVisitor.php @@ -193,7 +193,7 @@ if ( strlen( $elem->taintedOriginalErrorByArg[$arg] ) > 254 ) { $this->debug( __METHOD__, "Too long original error! $elem" ); $elem->taintedOriginalErrorByArg[$arg] = substr( - $elem->taintedOriginalError[$arg], 0, 250 + $elem->taintedOriginalErrorByArg[$arg], 0, 250 ) . '... '; } } diff --git a/src/TaintednessVisitor.php b/src/TaintednessVisitor.php index 09039ab..9325058 100644 --- a/src/TaintednessVisitor.php +++ b/src/TaintednessVisitor.php @@ -594,12 +594,15 @@ try { if ( $node->kind === \ast\AST_NEW ) { $clazzName = $node->children['class']->children['name']; + if ( $clazzName === 'self' && $this->context->isInClassScope() ) { + $clazzName = (string)$this->context->getClassFQSEN(); + } $fqsen = FullyQualifiedMethodName::fromStringInContext( $clazzName . '::__construct', $this->context ); if ( !$this->code_base->hasMethodWithFQSEN( $fqsen ) ) { - $this->debug( __METHOD__, "FIXME no constructor or parent class" ); + $this->debug( __METHOD__, "FIXME no constructor for $fqsen" ); throw new exception( "Cannot find __construct" ); } $func = $this->code_base->getMethodByFQSEN( $fqsen ); @@ -658,7 +661,7 @@ } if ( $varName === '' ) { $this->debug( __METHOD__, "FIXME: Complex variable case not handled." ); - Debug::printNode( $node ); + // Debug::printNode( $node ); return SecurityCheckPlugin::UNKNOWN_TAINT; } if ( !$this->context->getScope()->hasVariableWithName( $varName ) ) { @@ -895,8 +898,9 @@ $node->children['prop'] ); } else { - $this->debug( __METHOD__, "Unexpected number of phan objs " . count( $props ) . "" ); - Debug::printNode( $node ); + // FIXME, we should handle $this->foo->bar + $this->debug( __METHOD__, "Nested property reference " . count( $props ) . "" ); + # Debug::printNode( $node ); } if ( count( $props ) === 0 ) { // Should this be NO_TAINT? diff --git a/tests/integration/factory/expectedResults.txt b/tests/integration/factory/expectedResults.txt new file mode 100644 index 0000000..ce1ff3e --- /dev/null +++ b/tests/integration/factory/expectedResults.txt @@ -0,0 +1 @@ +integration/factory/test.php:7 SecurityCheck-XSS Echoing expression that was not html escaped (Caused by: integration/factory/test.php +5) diff --git a/tests/integration/factory/get.php b/tests/integration/factory/get.php new file mode 100644 index 0000000..713fb5c --- /dev/null +++ b/tests/integration/factory/get.php @@ -0,0 +1,18 @@ +<?php +class GetFetcher { + + /** @var String */ + private $arg; + + private function __construct( $arg ) { + $this->arg = $arg; + } + + public static function make( $arg ) { + return new self( $arg ); + } + + public function get() { + return $_GET[$this->arg]; + } +} diff --git a/tests/integration/factory/test.php b/tests/integration/factory/test.php new file mode 100644 index 0000000..6d0bd59 --- /dev/null +++ b/tests/integration/factory/test.php @@ -0,0 +1,7 @@ +<?php + +require( "get.php" ); + +$a = GetFetcher::make( 'foo' )->get(); + +echo $a; -- To view, visit https://gerrit.wikimedia.org/r/395758 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5e0117cc41e6504537d0fa7d4926d354728746eb Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/tools/phan/SecurityCheckPlugin Gerrit-Branch: master Gerrit-Owner: Brian Wolff <bawolff...@gmail.com> Gerrit-Reviewer: Brian Wolff <bawolff...@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits