Skip to content

Commit 2acd616

Browse files
asger-semmleasger-semmle
committed
JS: Review comments
1 parent bbb6dad commit 2acd616

File tree

8 files changed

+441
-428
lines changed

8 files changed

+441
-428
lines changed

‎javascript/ql/src/semmle/javascript/Regexp.qll‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -990,7 +990,7 @@ module RegExp {
990990
}
991991

992992
/**
993-
* Holds if `term` is matches any character except for explicitly listed exceptions.
993+
* Holds if `term` matches any character except for explicitly listed exceptions.
994994
*
995995
* For example, holds for `.`, `[^<>]`, or `\W`, but not for `[a-z]`, `\w`, or `[^\W\S]`.
996996
*/
@@ -999,11 +999,13 @@ module RegExp {
999999
or
10001000
term.(RegExpCharacterClassEscape).getValue().isUppercase()
10011001
or
1002+
// [^a-z]
10021003
exists(RegExpCharacterClass cls | term = cls |
10031004
cls.isInverted() and
10041005
not cls.getAChild().(RegExpCharacterClassEscape).getValue().isUppercase()
10051006
)
10061007
or
1008+
// [\W]
10071009
exists(RegExpCharacterClass cls | term = cls |
10081010
not cls.isInverted() and
10091011
cls.getAChild().(RegExpCharacterClassEscape).getValue().isUppercase()
@@ -1024,6 +1026,7 @@ module RegExp {
10241026
isFullyAnchoredTerm(term) and
10251027
not isWildcardLike(term.getAChild*())
10261028
or
1029+
// Character set restrictions like `/[^a-z]/.test(x)` sanitize in the false case
10271030
outcome = false and
10281031
exists(RegExpTerm root |
10291032
root = term
@@ -1043,7 +1046,7 @@ module RegExp {
10431046
RegExpTerm getRegExpObjectFromNode(DataFlow::Node node) {
10441047
exists(DataFlow::RegExpCreationNode regexp |
10451048
regexp.getAReference().flowsTo(node) and
1046-
result = regexp.getRegExpTerm()
1049+
result = regexp.getRoot()
10471050
)
10481051
}
10491052

‎javascript/ql/src/semmle/javascript/dataflow/Nodes.qll‎

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -546,6 +546,11 @@ class RegExpLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode {
546546

547547
/** Gets the root term of this regular expression. */
548548
RegExpTerm getRoot() { result = astNode.getRoot() }
549+
550+
/** Gets the flags of this regular expression literal. */
551+
string getFlags() {
552+
result = astNode.getFlags()
553+
}
549554
}
550555

551556
/**
@@ -1331,9 +1336,10 @@ class RegExpConstructorInvokeNode extends DataFlow::InvokeNode {
13311336
}
13321337

13331338
/**
1334-
* Gets the AST of the regular expression created here, if it is constant.
1339+
* Gets the AST of the regular expression created here, provided that the
1340+
* first argument is a string literal.
13351341
*/
1336-
RegExpTerm getRegExpTerm() {
1342+
RegExpTerm getRoot() {
13371343
result = getArgument(0).asExpr().(StringLiteral).asRegExp()
13381344
}
13391345

@@ -1362,28 +1368,6 @@ class RegExpConstructorInvokeNode extends DataFlow::InvokeNode {
13621368
}
13631369
}
13641370

1365-
/**
1366-
* A data flow node corresponding to a regular expression literal.
1367-
*
1368-
* Example:
1369-
* ```js
1370-
* /[a-z]+/i
1371-
* ```
1372-
*/
1373-
class RegExpLiteralNode extends DataFlow::SourceNode, DataFlow::ValueNode {
1374-
override RegExpLiteral astNode;
1375-
1376-
/** Gets the root term of this regular expression literal. */
1377-
RegExpTerm getRegExpTerm() {
1378-
result = astNode.getRoot()
1379-
}
1380-
1381-
/** Gets the flags of this regular expression literal. */
1382-
string getFlags() {
1383-
result = astNode.getFlags()
1384-
}
1385-
}
1386-
13871371
/**
13881372
* A data flow node corresponding to a regular expression literal or
13891373
* an invocation of the `RegExp` constructor.
@@ -1406,9 +1390,9 @@ class RegExpCreationNode extends DataFlow::SourceNode {
14061390
*
14071391
* Has no result for calls to `RegExp` with a non-constant argument.
14081392
*/
1409-
RegExpTerm getRegExpTerm() {
1410-
result = this.(RegExpConstructorInvokeNode).getRegExpTerm() or
1411-
result = this.(RegExpLiteralNode).getRegExpTerm()
1393+
RegExpTerm getRoot() {
1394+
result = this.(RegExpConstructorInvokeNode).getRoot() or
1395+
result = this.(RegExpLiteralNode).getRoot()
14121396
}
14131397

14141398
/**

‎javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss.expected‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -59,16 +59,16 @@ nodes
5959
| exception-xss.js:129:10:129:10 | e |
6060
| exception-xss.js:130:18:130:18 | e |
6161
| exception-xss.js:130:18:130:18 | e |
62-
| tst.js:298:9:298:16 | location |
63-
| tst.js:298:9:298:16 | location |
64-
| tst.js:299:10:299:10 | e |
65-
| tst.js:300:20:300:20 | e |
66-
| tst.js:300:20:300:20 | e |
67-
| tst.js:305:10:305:17 | location |
68-
| tst.js:305:10:305:17 | location |
69-
| tst.js:307:10:307:10 | e |
70-
| tst.js:308:20:308:20 | e |
71-
| tst.js:308:20:308:20 | e |
62+
| tst.js:304:9:304:16 | location |
63+
| tst.js:304:9:304:16 | location |
64+
| tst.js:305:10:305:10 | e |
65+
| tst.js:306:20:306:20 | e |
66+
| tst.js:306:20:306:20 | e |
67+
| tst.js:311:10:311:17 | location |
68+
| tst.js:311:10:311:17 | location |
69+
| tst.js:313:10:313:10 | e |
70+
| tst.js:314:20:314:20 | e |
71+
| tst.js:314:20:314:20 | e |
7272
edges
7373
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:9:11:9:13 | foo |
7474
| exception-xss.js:2:9:2:31 | foo | exception-xss.js:15:9:15:11 | foo |
@@ -127,14 +127,14 @@ edges
127127
| exception-xss.js:128:11:128:52 | session ... ssion') | exception-xss.js:129:10:129:10 | e |
128128
| exception-xss.js:129:10:129:10 | e | exception-xss.js:130:18:130:18 | e |
129129
| exception-xss.js:129:10:129:10 | e | exception-xss.js:130:18:130:18 | e |
130-
| tst.js:298:9:298:16 | location | tst.js:299:10:299:10 | e |
131-
| tst.js:298:9:298:16 | location | tst.js:299:10:299:10 | e |
132-
| tst.js:299:10:299:10 | e | tst.js:300:20:300:20 | e |
133-
| tst.js:299:10:299:10 | e | tst.js:300:20:300:20 | e |
134-
| tst.js:305:10:305:17 | location | tst.js:307:10:307:10 | e |
135-
| tst.js:305:10:305:17 | location | tst.js:307:10:307:10 | e |
136-
| tst.js:307:10:307:10 | e | tst.js:308:20:308:20 | e |
137-
| tst.js:307:10:307:10 | e | tst.js:308:20:308:20 | e |
130+
| tst.js:304:9:304:16 | location | tst.js:305:10:305:10 | e |
131+
| tst.js:304:9:304:16 | location | tst.js:305:10:305:10 | e |
132+
| tst.js:305:10:305:10 | e | tst.js:306:20:306:20 | e |
133+
| tst.js:305:10:305:10 | e | tst.js:306:20:306:20 | e |
134+
| tst.js:311:10:311:17 | location | tst.js:313:10:313:10 | e |
135+
| tst.js:311:10:311:17 | location | tst.js:313:10:313:10 | e |
136+
| tst.js:313:10:313:10 | e | tst.js:314:20:314:20 | e |
137+
| tst.js:313:10:313:10 | e | tst.js:314:20:314:20 | e |
138138
#select
139139
| exception-xss.js:11:18:11:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:11:18:11:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
140140
| exception-xss.js:17:18:17:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:17:18:17:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
@@ -147,5 +147,5 @@ edges
147147
| exception-xss.js:107:18:107:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:107:18:107:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value |
148148
| exception-xss.js:119:14:119:30 | "Exception: " + e | exception-xss.js:117:13:117:25 | req.params.id | exception-xss.js:119:14:119:30 | "Exception: " + e | Cross-site scripting vulnerability due to $@. | exception-xss.js:117:13:117:25 | req.params.id | user-provided value |
149149
| exception-xss.js:130:18:130:18 | e | exception-xss.js:125:48:125:64 | document.location | exception-xss.js:130:18:130:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:125:48:125:64 | document.location | user-provided value |
150-
| tst.js:300:20:300:20 | e | tst.js:298:9:298:16 | location | tst.js:300:20:300:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:298:9:298:16 | location | user-provided value |
151-
| tst.js:308:20:308:20 | e | tst.js:305:10:305:17 | location | tst.js:308:20:308:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:305:10:305:17 | location | user-provided value |
150+
| tst.js:306:20:306:20 | e | tst.js:304:9:304:16 | location | tst.js:306:20:306:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:304:9:304:16 | location | user-provided value |
151+
| tst.js:314:20:314:20 | e | tst.js:311:10:311:17 | location | tst.js:314:20:314:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:311:10:311:17 | location | user-provided value |

0 commit comments

Comments
 (0)