Page MenuHomePhabricator
Files F13957513

D14024.diff
No OneTemporary
Actions

D14024.diff
View Options

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<string, string>',
- '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<string, string>',
- '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 @@
-<?php
-
-csprintf('x');
-csprintf($x);
-
-qsprintf();
-qsprintf('x');
-qsprintf('x', 'y');
-qsprintf('x', $y);
-
-~~~~~~~~~~
-warning:4:1
-warning:9:1
diff --git a/src/lint/linter/xhpast/rules/ArcanistUnsafeDynamicStringLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistUnsafeDynamicStringLinterRule.php
new file mode 100644
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/ArcanistUnsafeDynamicStringLinterRule.php
@@ -0,0 +1,128 @@
+<?php
+
+final class ArcanistUnsafeDynamicStringLinterRule
+ extends ArcanistXHPASTLinterRule {
+
+ const ID = 31;
+
+ private $dynamicStringFunctions = array();
+ private $dynamicStringClasses = array();
+
+ public function getLintName() {
+ return pht('Unsafe Dynamic String');
+ }
+
+ public function getLintSeverity() {
+ return ArcanistLintSeverity::SEVERITY_WARNING;
+ }
+
+ public function getLinterConfigurationOptions() {
+ $options = array(
+ 'xhpast.dynamic-string.functions' => array(
+ 'type' => 'optional map<string, int>',
+ '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<string, int>',
+ '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 @@
+<?php
+
+final class ArcanistUnsafeDynamicStringLinterRuleTestCase
+ extends ArcanistXHPASTLinterRuleTestCase {
+
+ public function testLinter() {
+ $this->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 @@
+<?php
+DB::queryMaster('select * from some_table');
+DB::queryMaster("select * from some_table where %s order by %s", array($c, $o));
+DB::queryMaster($query);
+DB::queryMaster('select ' . " * " . ' from ' . " some_table");
+DB::queryMaster('select * from ' . $table_name);
+DB::queryMaster("select * from $table_name");
+DB::queryMaster('select * from $table_name');
+~~~~~~~~~~
+warning:4:1
+warning:6:1
+warning:7:1
+~~~~~~~~~~
+~~~~~~~~~~
+{
+ "config": {
+ "xhpast.dynamic-string.functions": {
+ "db::querymaster": 0
+ }
+ }
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/unsafe-dynamic-string/xsprintf.lint-test b/src/lint/linter/xhpast/rules/__tests__/unsafe-dynamic-string/xsprintf.lint-test
new file mode 100644
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/unsafe-dynamic-string/xsprintf.lint-test
@@ -0,0 +1,23 @@
+<?php
+
+csprintf('x');
+csprintf($x);
+
+qsprintf();
+qsprintf('x');
+qsprintf('x', 'y');
+qsprintf('x'.$y);
+
+~~~~~~~~~~
+warning:4:1
+warning:9:1
+~~~~~~~~~~
+~~~~~~~~~~
+{
+ "config": {
+ "xhpast.dynamic-string.functions": {
+ "qsprintf": 0,
+ "csprintf": 0
+ }
+ }
+}

File Metadata

Mime Type
text/plain
Expires
Tue, Oct 15, 9:41 AM (3 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6712414
Default Alt Text
D14024.diff (14 KB)

Event Timeline

Phabricator · Built by Phacility