Skip to content

Commit 3171f38

Browse files
committed
JS: fixed bad alert messages when it came to incomplete sanitization for new RegExp objects
1 parent 13afd63 commit 3171f38

File tree

2 files changed

+20
-6
lines changed

2 files changed

+20
-6
lines changed

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,12 +143,26 @@ predicate whitelistedRemoval(StringReplaceCall repl) {
143143
)
144144
}
145145

146-
from StringReplaceCall repl, DataFlow::Node old, string msg
146+
/**
147+
* Gets a nice string representation of the pattern or value of the node.
148+
*/
149+
predicate getPatternOrValueString(DataFlow::Node node, string patternOrValue) {
150+
if node instanceof DataFlow::RegExpConstructorInvokeNode
151+
then
152+
exists(DataFlow::RegExpConstructorInvokeNode regExp |
153+
node = regExp and
154+
patternOrValue = "/" + regExp.getRoot() + "/"
155+
)
156+
else patternOrValue = node.toString()
157+
}
158+
159+
from StringReplaceCall repl, DataFlow::Node old, string patternOrValue, string msg
147160
where
148161
(old = repl.getArgument(0) or old = repl.getRegExp()) and
162+
getPatternOrValueString(old, patternOrValue) and
149163
(
150164
not repl.maybeGlobal() and
151-
msg = "This replaces only the first occurrence of " + old + "." and
165+
msg = "This replaces only the first occurrence of " + patternOrValue + "." and
152166
// only flag if this is likely to be a sanitizer or URL encoder or decoder
153167
exists(string m | m = getAMatchedString(old) |
154168
// sanitizer

javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,9 @@
2929
| tst.js:149:2:149:24 | x.repla ... replace | This replaces only the first occurrence of "\\n". |
3030
| tst.js:193:9:193:17 | s.replace | This replaces only the first occurrence of /'/. |
3131
| tst.js:202:10:202:18 | p.replace | This replaces only the first occurrence of "/../". |
32-
| tst.js:341:9:341:17 | p.replace | This replaces only the first occurrence of new Reg ... .\\\\./"). |
32+
| tst.js:341:9:341:17 | p.replace | This replaces only the first occurrence of /\\.\\.//. |
3333
| tst.js:345:9:345:17 | s.replace | This does not escape backslash characters in the input. |
34-
| tst.js:349:9:349:17 | s.replace | This replaces only the first occurrence of new RegExp("\\'"). |
34+
| tst.js:349:9:349:17 | s.replace | This replaces only the first occurrence of /'/. |
3535
| tst.js:353:9:353:17 | s.replace | This does not escape backslash characters in the input. |
36-
| tst.js:362:2:362:10 | x.replace | This replaces only the first occurrence of new RegExp("\\n"). |
37-
| tst.js:363:2:363:24 | x.repla ... replace | This replaces only the first occurrence of new RegExp("\\n"). |
36+
| tst.js:362:2:362:10 | x.replace | This replaces only the first occurrence of /\n/. |
37+
| tst.js:363:2:363:24 | x.repla ... replace | This replaces only the first occurrence of /\n/. |

0 commit comments

Comments
 (0)