diff --git a/.arclint b/.arclint --- a/.arclint +++ b/.arclint @@ -52,6 +52,29 @@ "xhpast.blacklisted.function": { "eval": "The eval() function should be avoided. It is potentially unsafe and makes debugging more difficult." }, + "xhpast.dynamic-string.functions": { + "pht": 0, + + "hsprintf": 0, + "jsprintf": 0, + + "hgsprintf": 0, + + "csprintf": 0, + "vcsprintf": 0, + "execx": 0, + "exec_manual": 0, + "phutil_passthru": 0, + + "qsprintf": 1, + "vqsprintf": 1, + "queryfx": 1, + "queryfx_all": 1, + "queryfx_one": 1 + }, + "xhpast.dynamic-string.classes": { + "execfuture": 0 + }, "xhpast.php-version": "5.2.3", "xhpast.php-version.windows": "5.3.0" } diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -331,6 +331,8 @@ 'ArcanistUnnecessaryFinalModifierXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistUnnecessaryFinalModifierXHPASTLinterRuleTestCase.php', 'ArcanistUnnecessarySemicolonXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnnecessarySemicolonXHPASTLinterRule.php', 'ArcanistUnnecessarySemicolonXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistUnnecessarySemicolonXHPASTLinterRuleTestCase.php', + 'ArcanistUnsafeDynamicStringLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnsafeDynamicStringLinterRule.php', + 'ArcanistUnsafeDynamicStringLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistUnsafeDynamicStringLinterRuleTestCase.php', 'ArcanistUpgradeWorkflow' => 'workflow/ArcanistUpgradeWorkflow.php', 'ArcanistUploadWorkflow' => 'workflow/ArcanistUploadWorkflow.php', 'ArcanistUsageException' => 'exception/ArcanistUsageException.php', @@ -691,6 +693,8 @@ 'ArcanistUnnecessaryFinalModifierXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistUnnecessarySemicolonXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistUnnecessarySemicolonXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', + 'ArcanistUnsafeDynamicStringLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistUnsafeDynamicStringLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistUpgradeWorkflow' => 'ArcanistWorkflow', 'ArcanistUploadWorkflow' => 'ArcanistWorkflow', 'ArcanistUsageException' => 'Exception', diff --git a/src/lint/linter/ArcanistPhutilXHPASTLinter.php b/src/lint/linter/ArcanistPhutilXHPASTLinter.php --- a/src/lint/linter/ArcanistPhutilXHPASTLinter.php +++ b/src/lint/linter/ArcanistPhutilXHPASTLinter.php @@ -4,13 +4,10 @@ const LINT_ARRAY_COMBINE = 2; const LINT_DEPRECATED_FUNCTION = 3; - const LINT_UNSAFE_DYNAMIC_STRING = 4; const LINT_RAGGED_CLASSTREE_EDGE = 5; const LINT_EXTENDS_PHOBJECT = 6; private $deprecatedFunctions = array(); - private $dynamicStringFunctions = array(); - private $dynamicStringClasses = array(); public function getInfoName() { return 'XHPAST/libphutil Lint'; @@ -28,8 +25,6 @@ pht('%s Unreliable', 'array_combine()'), self::LINT_DEPRECATED_FUNCTION => pht('Use of Deprecated Function'), - self::LINT_UNSAFE_DYNAMIC_STRING => - pht('Unsafe Usage of Dynamic String'), self::LINT_RAGGED_CLASSTREE_EDGE => pht('Class Not %s Or %s', 'abstract', 'final'), self::LINT_EXTENDS_PHOBJECT => @@ -52,7 +47,6 @@ return array( self::LINT_ARRAY_COMBINE => $warning, self::LINT_DEPRECATED_FUNCTION => $warning, - self::LINT_UNSAFE_DYNAMIC_STRING => $warning, self::LINT_RAGGED_CLASSTREE_EDGE => $warning, self::LINT_EXTENDS_PHOBJECT => $advice, ); @@ -65,18 +59,6 @@ 'help' => pht( 'Functions which should should be considered deprecated.'), ), - 'phutil-xhpast.dynamic-string.functions' => array( - 'type' => 'optional map', - 'help' => pht( - 'Functions which should should not be used because they represent '. - 'the unsafe usage of dynamic strings.'), - ), - 'phutil-xhpast.dynamic-string.classes' => array( - 'type' => 'optional map', - 'help' => pht( - 'Classes which should should not be used because they represent the '. - 'unsafe usage of dynamic strings.'), - ), ); return $options + parent::getLinterConfigurationOptions(); @@ -87,12 +69,6 @@ case 'phutil-xhpast.deprecated.functions': $this->setDeprecatedFunctions($value); return; - case 'phutil-xhpast.dynamic-string.functions': - $this->setDynamicStringFunctions($value); - return; - case 'phutil-xhpast.dynamic-string.classes': - $this->setDynamicStringClasses($value); - return; default: parent::setLinterConfigurationValue($key, $value); return; @@ -114,7 +90,6 @@ $method_codes = array( 'lintArrayCombine' => self::LINT_ARRAY_COMBINE, - 'lintUnsafeDynamicString' => self::LINT_UNSAFE_DYNAMIC_STRING, 'lintDeprecatedFunctions' => self::LINT_DEPRECATED_FUNCTION, 'lintRaggedClasstreeEdges' => self::LINT_RAGGED_CLASSTREE_EDGE, 'lintClassExtendsPhobject' => self::LINT_EXTENDS_PHOBJECT, @@ -138,87 +113,9 @@ return $this; } - public function setDynamicStringClasses(array $map) { - $this->dynamicStringClasses = $map; - return $this; - } - - public function setDynamicStringFunctions(array $map) { - $this->dynamicStringFunctions = $map; - return $this; - } - /* -( Linter Rules )------------------------------------------------------- */ - private function lintUnsafeDynamicString(XHPASTNode $root) { - $safe = $this->dynamicStringFunctions + array( - 'pht' => 0, - - 'hsprintf' => 0, - 'jsprintf' => 0, - - 'hgsprintf' => 0, - - 'csprintf' => 0, - 'vcsprintf' => 0, - 'execx' => 0, - 'exec_manual' => 0, - 'phutil_passthru' => 0, - - 'qsprintf' => 1, - 'vqsprintf' => 1, - 'queryfx' => 1, - 'queryfx_all' => 1, - 'queryfx_one' => 1, - ); - - $calls = $root->selectDescendantsOfType('n_FUNCTION_CALL'); - $this->lintUnsafeDynamicStringCall($calls, $safe); - - $safe = $this->dynamicStringClasses + array( - 'ExecFuture' => 0, - ); - - $news = $root->selectDescendantsOfType('n_NEW'); - $this->lintUnsafeDynamicStringCall($news, $safe); - } - - private function lintUnsafeDynamicStringCall( - AASTNodeList $calls, - array $safe) { - - $safe = array_combine( - array_map('strtolower', array_keys($safe)), - $safe); - - foreach ($calls as $call) { - $name = $call->getChildByIndex(0)->getConcreteString(); - $param = idx($safe, strtolower($name)); - - if ($param === null) { - continue; - } - - $parameters = $call->getChildByIndex(1); - if (count($parameters->getChildren()) <= $param) { - continue; - } - - $identifier = $parameters->getChildByIndex($param); - if (!$identifier->isConstantString()) { - $this->raiseLintAtNode( - $call, - self::LINT_UNSAFE_DYNAMIC_STRING, - pht( - "Parameter %d of %s should be a scalar string, ". - "otherwise it's not safe.", - $param + 1, - $name.'()')); - } - } - } - private function lintArrayCombine(XHPASTNode $root) { $function_calls = $this->getFunctionCalls($root, array('array_combine')); diff --git a/src/lint/linter/__tests__/phlxhp/xsprintf.lint-test b/src/lint/linter/__tests__/phlxhp/xsprintf.lint-test deleted file mode 100644 --- a/src/lint/linter/__tests__/phlxhp/xsprintf.lint-test +++ /dev/null @@ -1,13 +0,0 @@ - array( + 'type' => 'optional map', + 'help' => pht( + 'Functions which should should not be used because they represent '. + 'the unsafe usage of dynamic strings.'), + ), + 'xhpast.dynamic-string.classes' => array( + 'type' => 'optional map', + 'help' => pht( + 'Classes which should should not be used because they represent the '. + 'unsafe usage of dynamic strings.'), + ), + ); + + return $options + parent::getLinterConfigurationOptions(); + } + + public function setDynamicStringFunctions(array $map) { + $this->dynamicStringFunctions = $map; + return $this; + } + + public function setDynamicStringClasses(array $map) { + $this->dynamicStringClasses = $map; + return $this; + } + + public function setLinterConfigurationValue($key, $value) { + switch ($key) { + case 'xhpast.dynamic-string.functions': + $this->setDynamicStringFunctions($value); + return; + case 'xhpast.dynamic-string.classes': + $this->setDynamicStringClasses($value); + return; + default: + parent::setLinterConfigurationValue($key, $value); + return; + } + } + + public function process(XHPASTNode $root) { + $safe = $this->dynamicStringFunctions; + + $calls = $root->selectDescendantsOfType('n_FUNCTION_CALL'); + $this->lintCall($calls, $safe); + + $safe = $this->dynamicStringClasses; + + $news = $root->selectDescendantsOfType('n_NEW'); + $this->lintCall($news, $safe); + } + + public function lintCall( + AASTNodeList $calls, + array $safe) { + + $safe = array_combine( + array_map('strtolower', array_keys($safe)), + $safe); + + foreach ($calls as $call) { + + $first_child = $call->getChildByIndex(0); + + if ($first_child->getTypeName() == 'n_CLASS_STATIC_ACCESS') { + + if ($first_child->getChildByIndex(0)->getTypeName() != 'n_CLASS_NAME') { + continue; + } + + $called_method = $first_child->getChildByIndex(1); + if ($called_method->getTypeName() != 'n_STRING') { + continue; + } + + $name = $first_child->getConcreteString(); + + } else { + $name = $call->getChildByIndex(0)->getConcreteString(); + } + + $param = idx($safe, strtolower($name)); + + if ($param === null) { + continue; + } + + $parameters = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST'); + if (count($parameters->getChildren()) <= $param) { + continue; + } + + $identifier = $parameters->getChildByIndex($param); + + if ($identifier->isConstantString() != 1) { + $this->raiseLintAtNode( + $call, + self::ID, + pht( + "Parameter %d of %s should be a scalar string, ". + "otherwise it's not safe.", + $param + 1, + $name.'()')); + } + } + } +} diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistUnsafeDynamicStringLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistUnsafeDynamicStringLinterRuleTestCase.php new file mode 100644 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistUnsafeDynamicStringLinterRuleTestCase.php @@ -0,0 +1,11 @@ +executeTestsInDirectory( + dirname(__FILE__).'/unsafe-dynamic-string/'); + } + +} diff --git a/src/lint/linter/__tests__/phlxhp/pht.lint-test b/src/lint/linter/xhpast/rules/__tests__/unsafe-dynamic-string/pht.lint-test rename from src/lint/linter/__tests__/phlxhp/pht.lint-test rename to src/lint/linter/xhpast/rules/__tests__/unsafe-dynamic-string/pht.lint-test --- a/src/lint/linter/__tests__/phlxhp/pht.lint-test +++ b/src/lint/linter/xhpast/rules/__tests__/unsafe-dynamic-string/pht.lint-test @@ -25,3 +25,12 @@ warning:8:1 warning:10:1 warning:18:1 +~~~~~~~~~~ +~~~~~~~~~~ +{ + "config": { + "xhpast.dynamic-string.functions": { + "pht": 0 + } + } +} diff --git a/src/lint/linter/xhpast/rules/__tests__/unsafe-dynamic-string/unsafe-dynamic-string.lint-test b/src/lint/linter/xhpast/rules/__tests__/unsafe-dynamic-string/unsafe-dynamic-string.lint-test new file mode 100644 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/unsafe-dynamic-string/unsafe-dynamic-string.lint-test @@ -0,0 +1,21 @@ +