Skip to content

Commit ecccb8a

Browse files
committed
only flag React elements in ClientSideUrlRedirect if it's a HTML element, or known link class
1 parent 36049f0 commit ecccb8a

File tree

4 files changed

+29
-2
lines changed

4 files changed

+29
-2
lines changed

javascript/ql/src/semmle/javascript/JSX.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,12 @@ class JSXElement extends JSXNode {
6565
}
6666

6767
override string getAPrimaryQlClass() { result = "JSXElement" }
68+
69+
/**
70+
* Holds if this JSX element is a HTML element.
71+
* That is, the name starts with a lowercase letter.
72+
*/
73+
predicate isHTMLElement() { getName().regexpMatch("[a-z].*") }
6874
}
6975

7076
/**

javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,10 @@ module ClientSideUrlRedirect {
173173
class ReactAttributeWriteUrlSink extends ScriptUrlSink {
174174
ReactAttributeWriteUrlSink() {
175175
exists(JSXAttribute attr |
176-
attr.getName() = DOM::getAPropertyNameInterpretedAsJavaScriptUrl()
176+
attr.getName() = DOM::getAPropertyNameInterpretedAsJavaScriptUrl() and
177+
attr.getElement().isHTMLElement()
178+
or
179+
DataFlow::moduleImport("next/link").flowsToExpr(attr.getElement().getNameExpr())
177180
|
178181
this = attr.getValue().flow()
179182
)

javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@ nodes
2121
| react.js:34:43:34:64 | documen ... on.hash |
2222
| react.js:34:43:34:74 | documen ... bstr(1) |
2323
| react.js:34:43:34:74 | documen ... bstr(1) |
24+
| react.js:40:19:40:35 | document.location |
25+
| react.js:40:19:40:35 | document.location |
26+
| react.js:40:19:40:40 | documen ... on.hash |
27+
| react.js:40:19:40:50 | documen ... bstr(1) |
28+
| react.js:40:19:40:50 | documen ... bstr(1) |
2429
| sanitizer.js:2:9:2:25 | url |
2530
| sanitizer.js:2:15:2:25 | window.name |
2631
| sanitizer.js:2:15:2:25 | window.name |
@@ -223,6 +228,10 @@ edges
223228
| react.js:34:43:34:59 | document.location | react.js:34:43:34:64 | documen ... on.hash |
224229
| react.js:34:43:34:64 | documen ... on.hash | react.js:34:43:34:74 | documen ... bstr(1) |
225230
| react.js:34:43:34:64 | documen ... on.hash | react.js:34:43:34:74 | documen ... bstr(1) |
231+
| react.js:40:19:40:35 | document.location | react.js:40:19:40:40 | documen ... on.hash |
232+
| react.js:40:19:40:35 | document.location | react.js:40:19:40:40 | documen ... on.hash |
233+
| react.js:40:19:40:40 | documen ... on.hash | react.js:40:19:40:50 | documen ... bstr(1) |
234+
| react.js:40:19:40:40 | documen ... on.hash | react.js:40:19:40:50 | documen ... bstr(1) |
226235
| sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url |
227236
| sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url |
228237
| sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url |
@@ -396,6 +405,7 @@ edges
396405
| react.js:21:24:21:45 | documen ... on.hash | react.js:21:24:21:40 | document.location | react.js:21:24:21:45 | documen ... on.hash | Untrusted URL redirection due to $@. | react.js:21:24:21:40 | document.location | user-provided value |
397406
| react.js:28:43:28:74 | documen ... bstr(1) | react.js:28:43:28:59 | document.location | react.js:28:43:28:74 | documen ... bstr(1) | Untrusted URL redirection due to $@. | react.js:28:43:28:59 | document.location | user-provided value |
398407
| react.js:34:43:34:74 | documen ... bstr(1) | react.js:34:43:34:59 | document.location | react.js:34:43:34:74 | documen ... bstr(1) | Untrusted URL redirection due to $@. | react.js:34:43:34:59 | document.location | user-provided value |
408+
| react.js:40:19:40:50 | documen ... bstr(1) | react.js:40:19:40:35 | document.location | react.js:40:19:40:50 | documen ... bstr(1) | Untrusted URL redirection due to $@. | react.js:40:19:40:35 | document.location | user-provided value |
399409
| sanitizer.js:4:27:4:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:4:27:4:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
400410
| sanitizer.js:16:27:16:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:16:27:16:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
401411
| sanitizer.js:19:27:19:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:19:27:19:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |

javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/react.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,12 @@ function Page({ router }) {
3434
return <span onClick={() => router.push(document.location.hash.substr(1))}>Click to XSS 2</span>
3535
}
3636

37-
export const pageWithRouter = withRouter(Page);
37+
export const pageWithRouter = withRouter(Page);
38+
39+
export function plainLink() {
40+
return <a href={document.location.hash.substr(1)}>my plain link!</a>;
41+
}
42+
43+
export function someUnknown() {
44+
return <FOO data={document.location.hash.substr(1)}>is safe.</FOO>;
45+
}

0 commit comments

Comments
 (0)