diff --git a/javascript/ql/lib/semmle/javascript/StandardLibrary.qll b/javascript/ql/lib/semmle/javascript/StandardLibrary.qll index b40f10d93691..f33efd59f779 100644 --- a/javascript/ql/lib/semmle/javascript/StandardLibrary.qll +++ b/javascript/ql/lib/semmle/javascript/StandardLibrary.qll @@ -115,7 +115,9 @@ class StringReplaceCall extends DataFlow::MethodCallNode { * Holds if this is a global replacement, that is, the first argument is a regular expression * with the `g` flag, or this is a call to `.replaceAll()`. */ - predicate isGlobal() { this.getRegExp().isGlobal() or this.getMethodName() = "replaceAll" } + predicate isGlobal() { + RegExp::isGlobal(this.getRegExp().getFlags()) or this.getMethodName() = "replaceAll" + } /** * Holds if this call to `replace` replaces `old` with `new`. diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll index d88dab4d4317..59ed567e392c 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll @@ -1682,9 +1682,6 @@ class RegExpCreationNode extends DataFlow::SourceNode { result = this.(RegExpLiteralNode).getFlags() } - /** Holds if the constructed predicate has the `g` flag. */ - predicate isGlobal() { RegExp::isGlobal(this.getFlags()) } - /** Gets a data flow node referring to this regular expression. */ private DataFlow::SourceNode getAReference(DataFlow::TypeTracker t) { t.start() and diff --git a/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql b/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql index d16f72d4172a..997ee8971a5a 100644 --- a/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql +++ b/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql @@ -65,8 +65,8 @@ predicate isCaseSensitiveMiddleware( arg = call.getArgument(0) and regexp.getAReference().flowsTo(arg) and exists(string flags | - flags = regexp.getFlags() and - not RegExp::isIgnoreCase(flags) + flags = regexp.tryGetFlags() and + not RegExp::maybeIgnoreCase(flags) ) ) } diff --git a/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected b/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected index 2195bbf00297..2be1780188a0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected +++ b/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected @@ -2,6 +2,7 @@ | tst.js:14:5:14:28 | new Reg ... (.*)?') | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/FOO/1' will bypass the middleware. | tst.js:14:5:14:28 | new Reg ... (.*)?') | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | case-insensitive path | | tst.js:41:9:41:25 | /\\/foo\\/([0-9]+)/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/FOO/1' will bypass the middleware. | tst.js:41:9:41:25 | /\\/foo\\/([0-9]+)/ | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | case-insensitive path | | tst.js:64:5:64:28 | new Reg ... (.*)?') | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAR/1' will bypass the middleware. | tst.js:64:5:64:28 | new Reg ... (.*)?') | pattern | tst.js:73:1:74:2 | app.get ... ware\\n}) | case-insensitive path | +| tst.js:64:5:64:28 | new Reg ... (.*)?') | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAR/1' will bypass the middleware. | tst.js:64:5:64:28 | new Reg ... (.*)?') | pattern | tst.js:107:1:108:2 | app.get ... ware\\n}) | case-insensitive path | | tst.js:76:9:76:20 | /\\/baz\\/bla/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAZ/BLA' will bypass the middleware. | tst.js:76:9:76:20 | /\\/baz\\/bla/ | pattern | tst.js:77:1:79:2 | app.get ... });\\n}) | case-insensitive path | | tst.js:86:9:86:30 | /\\/[Bb] ... 3\\/[a]/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAZ3/A' will bypass the middleware. | tst.js:86:9:86:30 | /\\/[Bb] ... 3\\/[a]/ | pattern | tst.js:87:1:89:2 | app.get ... });\\n}) | case-insensitive path | | tst.js:91:9:91:40 | /\\/summ ... ntGame/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/CURRENTGAME' will bypass the middleware. | tst.js:91:9:91:40 | /\\/summ ... ntGame/ | pattern | tst.js:93:1:95:2 | app.get ... O");\\n}) | case-insensitive path | diff --git a/javascript/ql/test/query-tests/Security/CWE-178/tst.js b/javascript/ql/test/query-tests/Security/CWE-178/tst.js index 5fcc3cc94a0f..81bbe993291a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-178/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-178/tst.js @@ -93,3 +93,16 @@ app.use(/\/summonerByName|\/currentGame/,apiLimit1, apiLimit2); app.get('/currentGame', function (req, res) { res.send("FOO"); }); + +app.get( + new RegExp('^/bar(.*)?', unknownFlag()), // OK - Might be OK if the unknown flag evaluates to case insensitive one + unknown(), + function(req, res, next) { + if (req.params.blah) { + next(); + } + } +); + +app.get('/bar/*', (req, res) => { // OK - not a middleware +});