JavaScript: Add new query flagging identity replacements.

Author: Max Schaefer

change-notes/1.19/analysis-javascript.md

@@ -14,6 +14,7 @@
 |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094  | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. |
 | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
+| Replacement of a substring with itself (`js/identity-replacement`) | correctness | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. |
 
 ## Changes to existing queries
 

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,44 @@
+/**
+ * @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
+ * @tags correctness
+ * @precision very-high
+ */
+
+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 (RegExpConstant c |
+    matchesConstant(e.(RegExpLiteral).getRoot(), c) and
+    s = c.getValue()
+  )
+  or
+  s = e.getStringValue()
+}
+
+/**
+ * Holds if `t` matches `c` and nothing else.
+ */
+predicate matchesConstant(RegExpTerm t, RegExpConstant c) {
+  c = t
+  or
+  matchesConstant(t.(RegExpGroup).getAChild(), c)
+  or
+  exists (RegExpCharacterClass recc | recc = t and not recc.isInverted() |
+    recc.getNumChild() = 1 and
+    matchesConstant(recc.getChild(0), c)
+  )
+}
+
+from MethodCallExpr repl, string s
+where repl.getMethodName() = "replace" and
+      matchesString(repl.getArgument(0), s) and
+      repl.getArgument(1).getStringValue() = s
+select repl.getArgument(0), "This replaces '" + s + "' 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,4 @@
+| 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. |

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,4 @@
+raw.replace("\\", "\\"); // NOT OK
+raw.replace(/(\\)/, "\\"); // NOT OK
+raw.replace(/["]/, "\""); // NOT OK
+raw.replace("\\", "\\\\"); // OK