Merge pull request #222 from xiemaisi/js/identity-replacement

JavaScript: Add new query flagging identity replacements.

javascript/config/suites/javascript/correctness-core

@@ -32,6 +32,7 @@
 + semmlecode-javascript-queries/RegExp/BackrefIntoNegativeLookahead.ql: /Correctness/Regular Expressions
 + semmlecode-javascript-queries/RegExp/DuplicateCharacterInCharacterClass.ql: /Correctness/Regular Expressions
 + semmlecode-javascript-queries/RegExp/EmptyCharacterClass.ql: /Correctness/Regular Expressions
++ semmlecode-javascript-queries/RegExp/IdentityReplacement.ql: /Correctness/Regular Expressions
 + semmlecode-javascript-queries/RegExp/UnboundBackref.ql: /Correctness/Regular Expressions
 + semmlecode-javascript-queries/RegExp/UnmatchableCaret.ql: /Correctness/Regular Expressions
 + semmlecode-javascript-queries/RegExp/UnmatchableDollar.ql: /Correctness/Regular Expressions

javascript/ql/src/RegExp/IdentityReplacement.qhelp

@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Replacing a substring with itself has no effect and usually indicates a mistake, such as
+misspelling a backslash escape.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Examine the string replacement to find and correct any typos.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following code snippet attempts to backslash-escape all double quotes in <code>raw</code>
+by replacing all instances of <code>"</code> with <code>\"</code>:
+</p>
+<sample src="examples/IdentityReplacement.js" />
+<p>
+However, the replacement string <code>'\"'</code> is actually the same as <code>'"'</code>,
+with <code>\"</code> interpreted as an identity escape, so the replacement does nothing.
+Instead, the replacement string should be <code>'\\"'</code>:
+</p>
+<sample src="examples/IdentityReplacementGood.js" />
+</example>
+
+<references>
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String#Escape_notation">String escape notation</a>.</li>
+</references>
+</qhelp>

javascript/ql/src/RegExp/IdentityReplacement.ql

@@ -0,0 +1,68 @@
+/**
+ * @name Replacement of a substring with itself
+ * @description Replacing a substring with itself has no effect and may indicate a mistake.
+ * @kind problem
+ * @problem.severity warning
+ * @id js/identity-replacement
+ * @precision very-high
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-116
+ */
+
+import javascript
+
+/**
+ * Holds if `e`, when used as the first argument of `String.prototype.replace`, matches
+ * `s` and nothing else.
+ */
+predicate matchesString(Expr e, string s) {
+  exists (RegExpLiteral rl |
+    rl = e and
+    not rl.isIgnoreCase() and
+    regExpMatchesString(rl.getRoot(), s)
+  )
+  or
+  s = e.getStringValue()
+}
+
+/**
+ * Holds if `t` matches `s` and nothing else.
+ */
+language[monotonicAggregates]
+predicate regExpMatchesString(RegExpTerm t, string s) {
+  // constants match themselves
+  s = t.(RegExpConstant).getValue()
+  or
+  // assertions match the empty string
+  (t instanceof RegExpCaret or
+   t instanceof RegExpDollar or
+   t instanceof RegExpWordBoundary or
+   t instanceof RegExpNonWordBoundary or
+   t instanceof RegExpLookahead or
+   t instanceof RegExpLookbehind) and
+  s = ""
+  or
+  // groups match their content
+  regExpMatchesString(t.(RegExpGroup).getAChild(), s)
+  or
+  // single-character classes match that character
+  exists (RegExpCharacterClass recc | recc = t and not recc.isInverted() |
+    recc.getNumChild() = 1 and
+    regExpMatchesString(recc.getChild(0), s)
+  )
+  or
+  // sequences match the concatenation of their elements
+  exists (RegExpSequence seq | seq = t |
+    s = concat(int i, RegExpTerm child | child = seq.getChild(i) |
+      any(string subs | regExpMatchesString(child, subs)) order by i
+    )
+  )
+}
+
+from MethodCallExpr repl, string s, string friendly
+where repl.getMethodName() = "replace" and
+      matchesString(repl.getArgument(0), s) and
+      repl.getArgument(1).getStringValue() = s and
+      (if s = "" then friendly = "the empty string" else friendly = "'" + s + "'")
+select repl.getArgument(0), "This replaces " + friendly + " with itself."

javascript/ql/src/RegExp/examples/IdentityReplacement.js

@@ -0,0 +1 @@
+var escaped = raw.replace(/"/g, '\"');

javascript/ql/src/RegExp/examples/IdentityReplacementGood.js

@@ -0,0 +1 @@
+var escaped = raw.replace(/"/g, '\\"');

javascript/ql/test/query-tests/RegExp/IdentityReplacement/IdentityReplacement.expected

@@ -0,0 +1,12 @@
+| IdentityReplacement.js:1:27:1:30 | /"/g | This replaces '"' with itself. |
+| tst.js:1:13:1:16 | "\\\\" | This replaces '\\' with itself. |
+| tst.js:2:13:2:18 | /(\\\\)/ | This replaces '\\' with itself. |
+| tst.js:3:13:3:17 | /["]/ | This replaces '"' with itself. |
+| tst.js:6:13:6:18 | /foo/g | This replaces 'foo' with itself. |
+| tst.js:9:13:9:17 | /^\\\\/ | This replaces '\\' with itself. |
+| tst.js:10:13:10:17 | /\\\\$/ | This replaces '\\' with itself. |
+| tst.js:11:13:11:18 | /\\b\\\\/ | This replaces '\\' with itself. |
+| tst.js:12:13:12:18 | /\\B\\\\/ | This replaces '\\' with itself. |
+| tst.js:13:13:13:22 | /\\\\(?!\\\\)/ | This replaces '\\' with itself. |
+| tst.js:14:13:14:23 | /(?<!\\\\)\\\\/ | This replaces '\\' with itself. |
+| tst.js:16:13:16:15 | /^/ | This replaces the empty string with itself. |

javascript/ql/test/query-tests/RegExp/IdentityReplacement/IdentityReplacement.js

@@ -0,0 +1 @@
+var escaped = raw.replace(/"/g, '\"');

javascript/ql/test/query-tests/RegExp/IdentityReplacement/IdentityReplacement.qlref

@@ -0,0 +1 @@
+RegExp/IdentityReplacement.ql

javascript/ql/test/query-tests/RegExp/IdentityReplacement/IdentityReplacementGood.js

@@ -0,0 +1 @@
+var escaped = raw.replace(/"/g, '\\"');

javascript/ql/test/query-tests/RegExp/IdentityReplacement/tst.js

@@ -0,0 +1,16 @@
+raw.replace("\\", "\\"); // NOT OK
+raw.replace(/(\\)/, "\\"); // NOT OK
+raw.replace(/["]/, "\""); // NOT OK
+raw.replace("\\", "\\\\"); // OK
+
+raw.replace(/foo/g, 'foo'); // NOT OK
+raw.replace(/foo/gi, 'foo'); // OK
+
+raw.replace(/^\\/, "\\"); // NOT OK
+raw.replace(/\\$/, "\\"); // NOT OK
+raw.replace(/\b\\/, "\\"); // NOT OK
+raw.replace(/\B\\/, "\\"); // NOT OK
+raw.replace(/\\(?!\\)/, "\\"); // NOT OK
+raw.replace(/(?<!\\)\\/, "\\"); // NOT OK
+
+raw.replace(/^/, ""); // NOT OK