Skip to content

Commit 6c33013

Browse files
asgerfasgerf
committed
JS: Enable association with headers without needing a route handler
Previously it was not possible to associate a ResponseSendArgument with its header definitions if they did not have the same route handler. But for calls like `new Response(body, { headers })` the headers are fairly obvious whereas the route handler is unnecessarily hard to find. So we use the direct and obvious association between 'body' and 'headers' in the call.
1 parent db2720e commit 6c33013

File tree

6 files changed

+31
-29
lines changed

6 files changed

+31
-29
lines changed

‎javascript/ql/lib/semmle/javascript/frameworks/HTTP.qll‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,12 @@ module Http {
108108
* Gets the route handler that sends this expression.
109109
*/
110110
abstract RouteHandler getRouteHandler();
111+
112+
/**
113+
* Gets a header definition associated with this response body, if it they are provided
114+
* by the same call.
115+
*/
116+
HeaderDefinition getAnAssociatedHeaderDefinition() { none() }
111117
}
112118

113119
/**

‎javascript/ql/lib/semmle/javascript/frameworks/WebResponse.qll‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,4 +93,8 @@ private class ResponseSink extends Http::ResponseSendArgument {
9393
ResponseSink() { this = response.getArgument(0) }
9494

9595
override Http::RouteHandler getRouteHandler() { none() }
96+
97+
override ResponseArgumentHeaders getAnAssociatedHeaderDefinition() {
98+
result.getResponse() = response
99+
}
96100
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/ReflectedXssCustomizations.qll‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,11 @@ module ReflectedXss {
3232
* Gets a HeaderDefinition that defines a XSS safe content-type for `send`.
3333
*/
3434
Http::HeaderDefinition getAXssSafeHeaderDefinition(Http::ResponseSendArgument send) {
35-
exists(Http::RouteHandler h |
36-
send.getRouteHandler() = h and
37-
result = xssSafeContentTypeHeader(h)
38-
|
39-
// The HeaderDefinition affects a response sent at `send`.
35+
isSafeContentTypeHeader(result) and
36+
(
37+
result = send.getAnAssociatedHeaderDefinition()
38+
or
39+
result = send.getRouteHandler().getAResponseHeader("content-type") and
4040
headerAffects(result, send)
4141
)
4242
}
@@ -54,14 +54,20 @@ module ReflectedXss {
5454
]
5555
}
5656

57+
private predicate isSafeContentTypeHeader(Http::HeaderDefinition header) {
58+
header.getAHeaderName() = "content-type" and
59+
not exists(string tp | header.defines("content-type", tp) |
60+
tp.toLowerCase().matches(xssUnsafeContentType() + "%")
61+
)
62+
}
63+
5764
/**
65+
* DEPRECATED. Use `getAXssSafeHeaderDefinition` instead.
5866
* Holds if `h` may send a response with a content type that is safe for XSS.
5967
*/
60-
Http::HeaderDefinition xssSafeContentTypeHeader(Http::RouteHandler h) {
68+
deprecated Http::HeaderDefinition xssSafeContentTypeHeader(Http::RouteHandler h) {
6169
result = h.getAResponseHeader("content-type") and
62-
not exists(string tp | result.defines("content-type", tp) |
63-
tp.toLowerCase().matches(xssUnsafeContentType() + "%")
64-
)
70+
isSafeContentTypeHeader(result)
6571
}
6672

6773
/**
@@ -80,6 +86,8 @@ module ReflectedXss {
8086
dominatingHeader.getBasicBlock().(ReachableBasicBlock).dominates(sender.getBasicBlock())
8187
)
8288
)
89+
or
90+
header = sender.getAnAssociatedHeaderDefinition()
8391
}
8492

8593
bindingset[headerBlock]

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

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,14 +43,10 @@
4343
| response-object.js:9:18:9:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:9:18:9:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
4444
| response-object.js:10:18:10:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:10:18:10:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
4545
| response-object.js:11:18:11:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:11:18:11:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
46-
| response-object.js:13:18:13:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:13:18:13:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
4746
| response-object.js:14:18:14:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:14:18:14:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
48-
| response-object.js:16:18:16:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:16:18:16:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
4947
| response-object.js:17:18:17:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:17:18:17:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
50-
| response-object.js:20:18:20:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:20:18:20:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
5148
| response-object.js:23:18:23:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:23:18:23:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
5249
| response-object.js:26:18:26:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:26:18:26:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
53-
| response-object.js:30:18:30:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:30:18:30:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
5450
| response-object.js:34:18:34:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:34:18:34:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
5551
| response-object.js:38:18:38:21 | data | response-object.js:7:18:7:25 | req.body | response-object.js:38:18:38:21 | data | Cross-site scripting vulnerability due to a $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
5652
| tst2.js:7:12:7:12 | p | tst2.js:6:9:6:9 | p | tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to a $@. | tst2.js:6:9:6:9 | p | user-provided value |
@@ -165,14 +161,10 @@ edges
165161
| response-object.js:7:11:7:25 | data | response-object.js:9:18:9:21 | data | provenance | |
166162
| response-object.js:7:11:7:25 | data | response-object.js:10:18:10:21 | data | provenance | |
167163
| response-object.js:7:11:7:25 | data | response-object.js:11:18:11:21 | data | provenance | |
168-
| response-object.js:7:11:7:25 | data | response-object.js:13:18:13:21 | data | provenance | |
169164
| response-object.js:7:11:7:25 | data | response-object.js:14:18:14:21 | data | provenance | |
170-
| response-object.js:7:11:7:25 | data | response-object.js:16:18:16:21 | data | provenance | |
171165
| response-object.js:7:11:7:25 | data | response-object.js:17:18:17:21 | data | provenance | |
172-
| response-object.js:7:11:7:25 | data | response-object.js:20:18:20:21 | data | provenance | |
173166
| response-object.js:7:11:7:25 | data | response-object.js:23:18:23:21 | data | provenance | |
174167
| response-object.js:7:11:7:25 | data | response-object.js:26:18:26:21 | data | provenance | |
175-
| response-object.js:7:11:7:25 | data | response-object.js:30:18:30:21 | data | provenance | |
176168
| response-object.js:7:11:7:25 | data | response-object.js:34:18:34:21 | data | provenance | |
177169
| response-object.js:7:11:7:25 | data | response-object.js:38:18:38:21 | data | provenance | |
178170
| response-object.js:7:18:7:25 | req.body | response-object.js:7:11:7:25 | data | provenance | |
@@ -364,14 +356,10 @@ nodes
364356
| response-object.js:9:18:9:21 | data | semmle.label | data |
365357
| response-object.js:10:18:10:21 | data | semmle.label | data |
366358
| response-object.js:11:18:11:21 | data | semmle.label | data |
367-
| response-object.js:13:18:13:21 | data | semmle.label | data |
368359
| response-object.js:14:18:14:21 | data | semmle.label | data |
369-
| response-object.js:16:18:16:21 | data | semmle.label | data |
370360
| response-object.js:17:18:17:21 | data | semmle.label | data |
371-
| response-object.js:20:18:20:21 | data | semmle.label | data |
372361
| response-object.js:23:18:23:21 | data | semmle.label | data |
373362
| response-object.js:26:18:26:21 | data | semmle.label | data |
374-
| response-object.js:30:18:30:21 | data | semmle.label | data |
375363
| response-object.js:34:18:34:21 | data | semmle.label | data |
376364
| response-object.js:38:18:38:21 | data | semmle.label | data |
377365
| tst2.js:6:7:6:30 | p | semmle.label | p |

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,10 @@
4141
| response-object.js:9:18:9:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
4242
| response-object.js:10:18:10:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
4343
| response-object.js:11:18:11:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
44-
| response-object.js:13:18:13:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
4544
| response-object.js:14:18:14:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
46-
| response-object.js:16:18:16:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
4745
| response-object.js:17:18:17:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
48-
| response-object.js:20:18:20:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
4946
| response-object.js:23:18:23:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
5047
| response-object.js:26:18:26:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
51-
| response-object.js:30:18:30:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
5248
| response-object.js:34:18:34:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
5349
| response-object.js:38:18:38:21 | data | Cross-site scripting vulnerability due to $@. | response-object.js:7:18:7:25 | req.body | user-provided value |
5450
| tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/response-object.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,14 @@ express().get('/foo', (req) => {
1010
new Response(data, {}); // $ Alert
1111
new Response(data, { headers: null }); // $ Alert
1212

13-
new Response(data, { headers: { 'content-type': 'text/plain'}}); // $ SPURIOUS: Alert
13+
new Response(data, { headers: { 'content-type': 'text/plain'}});
1414
new Response(data, { headers: { 'content-type': 'text/html'}}); // $ Alert
1515

16-
new Response(data, { headers: { 'Content-Type': 'text/plain'}}); // $ SPURIOUS: Alert
16+
new Response(data, { headers: { 'Content-Type': 'text/plain'}});
1717
new Response(data, { headers: { 'Content-Type': 'text/html'}}); // $ Alert
1818

1919
const headers1 = new Headers({ 'content-type': 'text/plain'});
20-
new Response(data, { headers: headers1 }); // $ SPURIOUS: Alert
20+
new Response(data, { headers: headers1 });
2121

2222
const headers2 = new Headers({ 'content-type': 'text/html'});
2323
new Response(data, { headers: headers2 }); // $ Alert
@@ -27,7 +27,7 @@ express().get('/foo', (req) => {
2727

2828
const headers4 = new Headers();
2929
headers4.set('content-type', 'text/plain');
30-
new Response(data, { headers: headers4 }); // $ SPURIOUS: Alert
30+
new Response(data, { headers: headers4 });
3131

3232
const headers5 = new Headers();
3333
headers5.set('content-type', 'text/html');

0 commit comments

Comments
 (0)