Merge pull request #127 from xiemaisi/js/incomplete-sanitisation-doc-improvement

Approved by esben-semmle

Author: semmle-qlci

change-notes/1.18/analysis-javascript.md

@@ -103,7 +103,7 @@
 | Comparison between inconvertible types | Lower severity | The severity of this rule has been revised to "warning". |
 | CORS misconfiguration for credentials transfer | More true-positive results | This rule now treats header names case-insensitively. |
 | Hard-coded credentials | More true-positive results | This rule now recognizes secret cryptographic keys. |
-| Incomplete sanitization | More true-positive results | This rule now recognizes incomplete URL encoding and decoding. |
+| Incomplete string escaping or encoding | Better name, more true-positive results | This rule has been renamed to more clearly reflect its purpose. Also, it now recognizes incomplete URL encoding and decoding. |
 | Insecure randomness | More true-positive results | This rule now recognizes secret cryptographic keys. |
 | Missing rate limiting | More true-positive results, fewer false-positive results | This rule now recognizes additional rate limiters and expensive route handlers. | 
 | Missing X-Frame-Options HTTP header | Fewer false-positive results | This rule now treats header names case-insensitively. |

javascript/ql/src/Security/CWE-116/IncompleteSanitization.qhelp

@@ -20,6 +20,10 @@ sanitization. In the latter case, preceding a meta-character with a backslash le
 backslash being escaped, but the meta-character appearing un-escaped, which again makes the
 sanitization ineffective.
 </p>
+<p>
+Even if the escaped string is not used in a security-critical context, incomplete escaping may
+still have undesirable effects, such as badly rendered or confusing output.
+</p>
 </overview>
 
 <recommendation>

javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql

@@ -1,7 +1,7 @@
 /**
- * @name Incomplete sanitization
- * @description A sanitizer that does not replace or escape all occurrences of a
- *              problematic substring may be ineffective.
+ * @name Incomplete string escaping or encoding
+ * @description A string transformer that does not replace or escape all occurrences of a
+ *              meta-character may be ineffective.
  * @kind problem
  * @problem.severity warning
  * @precision high
@@ -16,9 +16,6 @@ import javascript
 
 /**
  * Gets a character that is commonly used as a meta-character.
- *
- * We heuristically assume that string replacements involving one of these
- * characters are meant to be sanitizers.
  */
 string metachar() {
   result = "'\"\\&<>\n\r\t*|{}[]%$".charAt(_)
@@ -75,7 +72,7 @@ predicate isBackslashEscape(MethodCallExpr mce, RegExpLiteral re) {
 }
 
 /**
- * Holds if data flowing into `nd` has no unescaped backslashes.
+ * Holds if data flowing into `nd` has no un-escaped backslashes.
  */
 predicate allBackslashesEscaped(DataFlow::Node nd) {
   // `JSON.stringify` escapes backslashes