Brian Wolff has uploaded a new change for review. (
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(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/tools/phan/SecurityCheckPlugin
refs/changes/58/395758/1
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: newchange
Gerrit-Change-Id: I5e0117cc41e6504537d0fa7d4926d354728746eb
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
