Skip to content

Commit b79f7f3

Browse files
Alvaro Muñozerik-krogh
Alvaro Muñoz
and
erik-krogh
committed
Address code review comments
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
1 parent 6ab62da commit b79f7f3

File tree

8 files changed

+164
-106
lines changed

8 files changed

+164
-106
lines changed

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

Lines changed: 13 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ module Restify {
138138
/**
139139
* An access to a header on a Restify request.
140140
*/
141-
private class RequestHeaderAccess extends Http::RequestHeaderAccess {
141+
private class RequestHeaderAccess extends Http::RequestHeaderAccess instanceof DataFlow::MethodCallNode {
142142
RouteHandler rh;
143143

144144
RequestHeaderAccess() {
@@ -150,7 +150,7 @@ module Restify {
150150
}
151151

152152
override string getAHeaderName() {
153-
result = this.(DataFlow::MethodCallNode).getArgument(0).getStringValue().toLowerCase()
153+
super.getArgument(0).mayHaveStringValue(any(string s | s.toLowerCase() = result))
154154
}
155155

156156
override RouteHandler getRouteHandler() { result = rh }
@@ -185,9 +185,8 @@ module Restify {
185185
/**
186186
* Gets a reference to the multiple headers object that is to be set.
187187
*/
188-
private DataFlow::SourceNode getAHeaderSource() {
189-
this.getArgument(0).getALocalSource() instanceof DataFlow::ObjectLiteralNode and
190-
result.flowsTo(this.getArgument(0))
188+
private DataFlow::ObjectLiteralNode getAHeaderSource() {
189+
result = this.getArgument(0).getALocalSource()
191190
}
192191

193192
override predicate definesHeaderValue(string headerName, DataFlow::Node headerValue) {
@@ -198,9 +197,7 @@ module Restify {
198197
}
199198

200199
override DataFlow::Node getNameNode() {
201-
exists(DataFlow::PropWrite write | this.getAHeaderSource().getAPropertyWrite() = write |
202-
result = write.getPropertyNameExpr().flow()
203-
)
200+
result = this.getAHeaderSource().getAPropertyWrite().getPropertyNameExpr().flow()
204201
}
205202

206203
override RouteHandler getRouteHandler() { this.getReceiver() = result.getAResponseNode() }
@@ -265,34 +262,29 @@ module Restify {
265262
.hasPropertyWrite("formatters", formatters)
266263
}
267264

268-
DataFlow::SourceNode getAFormatterHandler() { formatters.hasPropertyWrite(_, result) }
265+
DataFlow::FunctionNode getAFormatterHandler() { formatters.hasPropertyWrite(_, result) }
269266
}
270267

271268
/**
272269
* A Restify route handler.
273270
*/
274271
class FormatterHandler extends Http::Servers::StandardRouteHandler, DataFlow::FunctionNode {
275-
Function function;
276-
277-
FormatterHandler() {
278-
function = astNode and
279-
any(FormatterSetup setup).getAFormatterHandler() = this
280-
}
272+
FormatterHandler() { any(FormatterSetup setup).getAFormatterHandler() = this }
281273

282274
/**
283275
* Gets the parameter of the formatter handler that contains the request object.
284276
*/
285-
Parameter getRequestParameter() { result = function.getParameter(0) }
277+
DataFlow::ParameterNode getRequestParameter() { result = this.getParameter(0) }
286278

287279
/**
288280
* Gets the parameter of the formatter handler that contains the response object.
289281
*/
290-
Parameter getResponseParameter() { result = function.getParameter(1) }
282+
DataFlow::ParameterNode getResponseParameter() { result = this.getParameter(1) }
291283

292284
/**
293285
* Gets the parameter of the formatter handler that contains the body object.
294286
*/
295-
Parameter getBodyParameter() { result = function.getParameter(2) }
287+
DataFlow::ParameterNode getBodyParameter() { result = this.getParameter(2) }
296288
}
297289

298290
/**
@@ -302,7 +294,7 @@ module Restify {
302294
private class FormatterRequestSource extends Http::Servers::RequestSource {
303295
FormatterHandler fh;
304296

305-
FormatterRequestSource() { this = DataFlow::parameterNode(fh.getRequestParameter()) }
297+
FormatterRequestSource() { this = fh.getRequestParameter() }
306298

307299
/**
308300
* Gets the formatter handler that handles this request.
@@ -317,7 +309,7 @@ module Restify {
317309
private class FormatterResponseSource extends Http::Servers::ResponseSource {
318310
FormatterHandler fh;
319311

320-
FormatterResponseSource() { this = DataFlow::parameterNode(fh.getResponseParameter()) }
312+
FormatterResponseSource() { this = fh.getResponseParameter() }
321313

322314
/**
323315
* Gets the route handler that provides this response.
@@ -362,10 +354,7 @@ module Restify {
362354
result = this.getArgument(1)
363355
or
364356
this.getNumArgument() = 2 and
365-
this.getArgument(0)
366-
.getALocalSource()
367-
.(DataFlow::ObjectLiteralNode)
368-
.hasPropertyWrite("hostname", result)
357+
this.getArgument(0).getALocalSource().hasPropertyWrite("hostname", result)
369358
or
370359
this.getNumArgument() = 2 and
371360
result = this.getArgument(0)

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

Lines changed: 43 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,10 @@ module Spife {
2525
TaggedTemplateExpr template;
2626

2727
RouteSetup() {
28-
exists(CallExpr templateCall |
29-
this.getCalleeNode().asExpr() = template and
30-
API::moduleImport(["@npm/spife/routing", "spife/routing"])
31-
.asSource()
32-
.flowsToExpr(template.getTag()) and
33-
templateCall.getAChild() = template
34-
)
28+
this.getCalleeNode().asExpr() = template and
29+
API::moduleImport(["@npm/spife/routing", "spife/routing"])
30+
.asSource()
31+
.flowsToExpr(template.getTag())
3532
}
3633

3734
private string getRoutePattern() {
@@ -184,13 +181,13 @@ module Spife {
184181
/**
185182
* An access to a user-controlled Spife context input.
186183
*/
187-
private class ContextInputAccess extends Http::RequestInputAccess {
184+
private class ContextInputAccess extends Http::RequestInputAccess instanceof DataFlow::MethodCallNode {
188185
ContextSource request;
189186
string kind;
190187

191188
ContextInputAccess() {
192-
request.ref().flowsTo(this.(DataFlow::MethodCallNode).getReceiver()) and
193-
this.(DataFlow::MethodCallNode).getMethodName() = "get" and
189+
request.ref().flowsTo(super.getReceiver()) and
190+
super.getMethodName() = "get" and
194191
kind = "path"
195192
}
196193

@@ -202,7 +199,7 @@ module Spife {
202199
/**
203200
* An access to a header on a Spife request.
204201
*/
205-
private class RequestHeaderAccess extends Http::RequestHeaderAccess {
202+
private class RequestHeaderAccess extends Http::RequestHeaderAccess instanceof DataFlow::PropRead {
206203
RouteHandler rh;
207204

208205
RequestHeaderAccess() {
@@ -211,7 +208,7 @@ module Spife {
211208
}
212209

213210
override string getAHeaderName() {
214-
result = this.(DataFlow::PropRead).getPropertyName().toLowerCase()
211+
result = super.getPropertyName().toLowerCase()
215212
}
216213

217214
override RouteHandler getRouteHandler() { result = rh }
@@ -223,7 +220,7 @@ module Spife {
223220
* A Spife response source, that is, the response variable used by a
224221
* route handler.
225222
*/
226-
private class ReplySource extends Http::Servers::ResponseSource {
223+
private class ReplySource extends Http::Servers::ResponseSource instanceof DataFlow::CallNode {
227224
ReplySource() {
228225
// const reply = require("@npm/spife/reply")
229226
// reply(resp)
@@ -232,14 +229,12 @@ module Spife {
232229
this = API::moduleImport(["@npm/spife/reply", "spife/reply"]).getAMember().getACall()
233230
}
234231

235-
private DataFlow::SourceNode reachesHandlerReturn(
236-
DataFlow::CallNode headerCall, DataFlow::TypeTracker t
237-
) {
238-
result = headerCall and
232+
private DataFlow::SourceNode reachesHandlerReturn(DataFlow::TypeTracker t) {
233+
result = this and
239234
t.start()
240235
or
241236
exists(DataFlow::TypeTracker t2 |
242-
result = this.reachesHandlerReturn(headerCall, t2).track(t2, t)
237+
result = this.reachesHandlerReturn(t2).track(t2, t)
243238
)
244239
}
245240

@@ -249,7 +244,7 @@ module Spife {
249244
override RouteHandler getRouteHandler() {
250245
exists(RouteHandler handler |
251246
handler.(DataFlow::FunctionNode).getAReturn().getALocalSource() =
252-
this.reachesHandlerReturn(this, DataFlow::TypeTracker::end()) and
247+
this.reachesHandlerReturn(DataFlow::TypeTracker::end()) and
253248
result = handler
254249
)
255250
}
@@ -258,14 +253,15 @@ module Spife {
258253
/**
259254
* An HTTP header defined in a Spife response.
260255
*/
261-
private class HeaderDefinition extends Http::ExplicitHeaderDefinition, DataFlow::MethodCallNode {
262-
ReplySource reply;
256+
private class HeaderDefinition extends Http::ExplicitHeaderDefinition, DataFlow::MethodCallNode instanceof ReplySource {
263257

264258
HeaderDefinition() {
265259
// reply.header(RESPONSE, 'Cache-Control', 'no-cache')
266-
reply.ref().(DataFlow::MethodCallNode).getMethodName() = "header" and
267-
reply.ref().(DataFlow::MethodCallNode).getNumArgument() = 3 and
268-
this = reply
260+
exists(DataFlow::MethodCallNode call |
261+
this.ref() = call and
262+
call.getMethodName() = "header" and
263+
call.getNumArgument() = 3
264+
)
269265
}
270266

271267
override predicate definesHeaderValue(string headerName, DataFlow::Node headerValue) {
@@ -276,7 +272,7 @@ module Spife {
276272

277273
override DataFlow::Node getNameNode() { result = this.getArgument(1) }
278274

279-
override RouteHandler getRouteHandler() { result = reply.getRouteHandler() }
275+
override RouteHandler getRouteHandler() { result = this.getRouteHandler() }
280276
}
281277

282278
/**
@@ -297,11 +293,8 @@ module Spife {
297293
/**
298294
* Gets a reference to the multiple headers object that is to be set.
299295
*/
300-
private DataFlow::SourceNode getAHeaderSource() {
301-
exists(int i |
302-
this.getArgument(i).getALocalSource() instanceof DataFlow::ObjectLiteralNode and
303-
result.flowsTo(this.getArgument(i))
304-
)
296+
private DataFlow::ObjectLiteralNode getAHeaderSource() {
297+
result = this.getAnArgument().getALocalSource()
305298
}
306299

307300
override predicate definesHeaderValue(string headerName, DataFlow::Node headerValue) {
@@ -312,9 +305,7 @@ module Spife {
312305
}
313306

314307
override DataFlow::Node getNameNode() {
315-
exists(DataFlow::PropWrite write | this.getAHeaderSource().getAPropertyWrite() = write |
316-
result = write.getPropertyNameExpr().flow()
317-
)
308+
result = this.getAHeaderSource().getAPropertyWrite().getPropertyNameExpr().flow()
318309
}
319310

320311
override RouteHandler getRouteHandler() { result = reply.getRouteHandler() }
@@ -324,8 +315,7 @@ module Spife {
324315
* A header produced by a route handler with no explicit declaration of a Content-Type.
325316
*/
326317
private class ContentTypeRouteHandlerHeader extends Http::ImplicitHeaderDefinition,
327-
DataFlow::FunctionNode {
328-
ContentTypeRouteHandlerHeader() { this instanceof RouteHandler }
318+
DataFlow::FunctionNode instanceof RouteHandler {
329319

330320
override predicate defines(string headerName, string headerValue) {
331321
headerName = "content-type" and headerValue = "application/json"
@@ -337,20 +327,18 @@ module Spife {
337327
/**
338328
* An HTTP cookie defined in a Spife HTTP response.
339329
*/
340-
private class CookieDefinition extends Http::CookieDefinition, DataFlow::MethodCallNode {
341-
ReplySource reply;
330+
private class CookieDefinition extends Http::CookieDefinition, DataFlow::MethodCallNode instanceof ReplySource {
342331

343332
CookieDefinition() {
344333
// reply.cookie(RESPONSE, 'TEST', 'FOO', {"maxAge": 1000, "httpOnly": true, "secure": true})
345-
this = reply.ref().(DataFlow::MethodCallNode) and
346-
this.getMethodName() = "cookie"
334+
this.ref().(DataFlow::MethodCallNode).getMethodName() = "cookie"
347335
}
348336

349337
override DataFlow::Node getNameArgument() { result = this.getArgument(1) }
350338

351339
override DataFlow::Node getValueArgument() { result = this.getArgument(2) }
352340

353-
override RouteHandler getRouteHandler() { result = reply.getRouteHandler() }
341+
override RouteHandler getRouteHandler() { result = this.getRouteHandler() }
354342
}
355343

356344
/**
@@ -360,14 +348,15 @@ module Spife {
360348
RouteHandler rh;
361349

362350
ReplyArgument() {
363-
exists(ReplySource reply |
364-
reply.ref().(DataFlow::CallNode).getCalleeName() =
351+
exists(ReplySource reply, DataFlow::CallNode call |
352+
reply.ref() = call and
353+
call.getCalleeName() =
365354
["reply", "cookie", "link", "header", "headers", "raw", "status", "toStream", "vary"] and
366-
this = reply.ref().(DataFlow::CallNode).getArgument(0) and
355+
this = call.getArgument(0) and
367356
rh = reply.getRouteHandler()
368357
)
369358
or
370-
this = rh.(DataFlow::FunctionNode).getAReturn()
359+
this = rh.getAReturn()
371360
}
372361

373362
override RouteHandler getRouteHandler() { result = rh }
@@ -394,8 +383,11 @@ module Spife {
394383
ReplySource reply;
395384

396385
TemplateObjectInput() {
397-
reply.ref().(DataFlow::MethodCallNode).getMethodName() = "template" and
398-
this = reply.ref().(DataFlow::MethodCallNode).getArgument(1)
386+
exists(DataFlow::MethodCallNode call |
387+
reply.ref() = call and
388+
call.getMethodName() = "template" and
389+
this = call.getArgument(1)
390+
)
399391
}
400392

401393
/**
@@ -407,28 +399,23 @@ module Spife {
407399
/**
408400
* An invocation of the `redirect` method of an HTTP response object.
409401
*/
410-
private class RedirectInvocation extends Http::RedirectInvocation, DataFlow::MethodCallNode {
411-
ReplySource reply;
402+
private class RedirectInvocation extends Http::RedirectInvocation, DataFlow::MethodCallNode instanceof ReplySource {
412403

413404
RedirectInvocation() {
414-
this = reply.ref().(DataFlow::MethodCallNode) and
415-
this.getMethodName() = "redirect"
405+
this.ref().(DataFlow::MethodCallNode).getMethodName() = "redirect"
416406
}
417407

418408
override DataFlow::Node getUrlArgument() { result = this.getAnArgument() }
419409

420-
override RouteHandler getRouteHandler() { result = reply.getRouteHandler() }
410+
override RouteHandler getRouteHandler() { result = this.getRouteHandler() }
421411
}
422412

423413
/**
424414
* A call to `reply.template('template', { ... })`, seen as a template instantiation.
425415
*/
426-
private class TemplateCall extends Templating::TemplateInstantiation::Range, DataFlow::CallNode {
416+
private class TemplateCall extends Templating::TemplateInstantiation::Range, DataFlow::MethodCallNode instanceof ReplySource {
427417
TemplateCall() {
428-
exists(ReplySource reply |
429-
reply.ref().(DataFlow::MethodCallNode).getMethodName() = "template" and
430-
this = reply.ref()
431-
)
418+
this.getMethodName() = "template"
432419
}
433420

434421
override DataFlow::SourceNode getOutput() { result = this }

‎javascript/ql/test/library-tests/frameworks/Restify2/src/index.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ server.get('/foo', // test: setup
101101
return next();
102102
},
103103
function(req, res, next) { // test: handler
104-
res.header("Content-Type", "text/html");
104+
res.header("Content-Type", "text/html"); // test: headerDefinition
105105
res.send(req.someData); // test: stackTraceExposureSink, xssSink, xss
106106
return next();
107107
}
@@ -119,7 +119,7 @@ server.get('/foo2', // test: setup
119119
);
120120

121121
function xss(req, res, next) { // test: handler
122-
res.header("Content-Type", "text/html");
122+
res.header("Content-Type", "text/html"); // test: headerDefinition
123123
res.send('hello ' + req.query.name); // test: source, stackTraceExposureSink, xssSink, xss
124124
next();
125125
}
@@ -140,7 +140,7 @@ function xss2(req, res, next) { // test: handler
140140
});
141141

142142
function xss3(req, res, next) { // test: handler
143-
res.header("Content-Type", "text/html");
143+
res.header("Content-Type", "text/html"); // test: headerDefinition
144144
res.send('hello ' + req.header("foo")); // test: source, stackTraceExposureSink, xssSink, !xss
145145
next();
146146
}

‎javascript/ql/test/library-tests/frameworks/Restify2/tests.expected‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ passingPositiveTests
2626
| PASSED | handler | src/index.js:192:39:192:61 | // test ... handler |
2727
| PASSED | handler | src/index.js:194:49:194:71 | // test ... handler |
2828
| PASSED | handler | src/index.js:198:65:198:87 | // test ... handler |
29+
| PASSED | headerDefinition | src/index.js:104:46:104:70 | // test ... inition |
30+
| PASSED | headerDefinition | src/index.js:122:44:122:68 | // test ... inition |
31+
| PASSED | headerDefinition | src/index.js:143:44:143:68 | // test ... inition |
2932
| PASSED | redirectSink | src/index.js:78:32:78:60 | // test ... ectSink |
3033
| PASSED | redirectSink | src/index.js:87:45:87:73 | // test ... ectSink |
3134
| PASSED | redirectSink | src/index.js:88:40:88:68 | // test ... ectSink |

0 commit comments

Comments
 (0)