diff --git a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql index 7d0dc71a2a84..754917de1b14 100644 --- a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql +++ b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql @@ -100,6 +100,18 @@ predicate removesFirstOccurrence(StringReplaceCall repl, string str) { not exists(repl.getRegExp()) and repl.replaces(str, "") } +/** + * Holds if `repl` removes a single character `str` (either via a string argument or a regex). + */ +predicate removesCharacter(StringReplaceCall repl, string str) { + removesFirstOccurrence(repl, str) + or + exists(DataFlow::Node re | re = repl.getRegExp() | + getAMatchedString(re) = str and + repl.replaces(str, "") + ) +} + /** * Holds if `leftUnwrap` and `rightUnwrap` unwraps a string from a pair of surrounding delimiters. */ @@ -117,8 +129,8 @@ predicate isDelimiterUnwrapper( or left = "'" and right = "'" | - removesFirstOccurrence(leftUnwrap, left) and - removesFirstOccurrence(rightUnwrap, right) and + removesCharacter(leftUnwrap, left) and + removesCharacter(rightUnwrap, right) and leftUnwrap.getAMethodCall() = rightUnwrap ) } diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js index d457af27f143..f4ab00a8ea4a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js @@ -137,10 +137,17 @@ function goodOrBad12(s) { s = s.replace('[', ''); s = s.replace(']', ''); - s.replace(/{/, '').replace(/}/, ''); // $ Alert[js/incomplete-sanitization] - should have used a string literal if a single replacement was intended + s.replace(/{/, '').replace(/}/, ''); // bracket pair removal via regex - no alert expected s.replace(']', '').replace('[', ''); // $ Alert[js/incomplete-sanitization] - probably OK, but still flagged } +function goodBracketPairRegex(s) { + // Bracket pair removal via regex should not be flagged + s.replace(/\[/, '').replace(/\]/, ''); + s.replace(/\{/, '').replace(/\}/, ''); + s.replace(/\(/, '').replace(/\)/, ''); +} + function newlines(s) { // motivation for whitelist require("child_process").execSync("which emacs").toString().replace("\n", "");