Skip to content

Commit 6041d52

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #3424 from asger-semmle/js/express-param-handler
Approved by esbena
2 parents 135eae9 + 0171c9e commit 6041d52

File tree

8 files changed

+277
-89
lines changed

8 files changed

+277
-89
lines changed

‎change-notes/1.25/analysis-javascript.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
## General improvements
44

55
* Support for the following frameworks and libraries has been improved:
6+
- [express](https://www.npmjs.com/package/express)
67
- [fstream](https://www.npmjs.com/package/fstream)
78
- [jGrowl](https://github.com/stanlemon/jGrowl)
89
- [jQuery](https://jquery.com/)

‎javascript/ql/src/semmle/javascript/frameworks/ConnectExpressShared.qll‎

Lines changed: 103 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -7,24 +7,104 @@
77
import javascript
88

99
module ConnectExpressShared {
10+
/**
11+
* String representing the signature of a route handler, that is,
12+
* the list of parameters taken by the route handler.
13+
*
14+
* Concretely this is a comma-separated list of parameter kinds, which can be either
15+
* `request`, `response`, `next`, `error`, or `parameter`, but this is considered an
16+
* implementation detail.
17+
*/
18+
private class RouteHandlerSignature extends string {
19+
RouteHandlerSignature() {
20+
this = "request,response" or
21+
this = "request,response,next" or
22+
this = "request,response,next,parameter" or
23+
this = "error,request,response,next"
24+
}
25+
26+
/** Gets the index of the parameter corresonding to the given `kind`, if any. */
27+
pragma[noinline]
28+
int getParameterIndex(string kind) { this.splitAt(",", result) = kind }
29+
30+
/** Gets the number of parameters taken by this signature. */
31+
pragma[noinline]
32+
int getArity() { result = count(getParameterIndex(_)) }
33+
34+
/** Holds if this signature takes a parameter of the given kind. */
35+
predicate has(string kind) { exists(getParameterIndex(kind)) }
36+
}
37+
38+
private module RouteHandlerSignature {
39+
/** Gets the signature corresonding to `(req, res, next, param) => {...}`. */
40+
RouteHandlerSignature requestResponseNextParameter() {
41+
result = "request,response,next,parameter"
42+
}
43+
44+
/** Gets the signature corresonding to `(req, res, next) => {...}`. */
45+
RouteHandlerSignature requestResponseNext() { result = "request,response,next" }
46+
47+
/** Gets the signature corresonding to `(err, req, res, next) => {...}`. */
48+
RouteHandlerSignature errorRequestResponseNext() { result = "error,request,response,next" }
49+
}
50+
51+
/**
52+
* Holds if `fun` appears to match the given signature based on parameter naming.
53+
*/
54+
private predicate matchesSignature(Function function, RouteHandlerSignature sig) {
55+
function.getNumParameter() = sig.getArity() and
56+
function.getParameter(sig.getParameterIndex("request")).getName() = ["req", "request"] and
57+
function.getParameter(sig.getParameterIndex("response")).getName() = ["res", "response"] and
58+
(
59+
sig.has("next")
60+
implies
61+
function.getParameter(sig.getParameterIndex("next")).getName() = ["next", "cb"]
62+
)
63+
}
64+
65+
/**
66+
* Gets the parameter corresonding to the given `kind`, where `routeHandler` is interpreted as a
67+
* route handler with the signature `sig`.
68+
*
69+
* This does not check if the function is actually a route handler or matches the signature in any way,
70+
* so the caller should restrict the function accordingly.
71+
*/
72+
pragma[inline]
73+
private Parameter getRouteHandlerParameter(
74+
Function routeHandler, RouteHandlerSignature sig, string kind
75+
) {
76+
result = routeHandler.getParameter(sig.getParameterIndex(kind))
77+
}
78+
79+
/**
80+
* Gets the parameter of kind `kind` of a Connect/Express route parameter handler function.
81+
*
82+
* `kind` is one of: "error", "request", "response", "next".
83+
*/
84+
pragma[inline]
85+
Parameter getRouteParameterHandlerParameter(Function routeHandler, string kind) {
86+
result =
87+
getRouteHandlerParameter(routeHandler, RouteHandlerSignature::requestResponseNextParameter(),
88+
kind)
89+
}
90+
1091
/**
1192
* Gets the parameter of kind `kind` of a Connect/Express route handler function.
1293
*
1394
* `kind` is one of: "error", "request", "response", "next".
1495
*/
15-
SimpleParameter getRouteHandlerParameter(Function routeHandler, string kind) {
16-
exists(int index, int offset |
17-
result = routeHandler.getParameter(index + offset) and
18-
(if routeHandler.getNumParameter() = 4 then offset = 0 else offset = -1)
19-
|
20-
kind = "error" and index = 0
21-
or
22-
kind = "request" and index = 1
23-
or
24-
kind = "response" and index = 2
25-
or
26-
kind = "next" and index = 3
27-
)
96+
pragma[inline]
97+
Parameter getRouteHandlerParameter(Function routeHandler, string kind) {
98+
if routeHandler.getNumParameter() = 4
99+
then
100+
// For arity 4 there is ambiguity between (err, req, res, next) and (req, res, next, param)
101+
// This predicate favors the 'err' signature whereas getRouteParameterHandlerParameter favors the other.
102+
result =
103+
getRouteHandlerParameter(routeHandler, RouteHandlerSignature::errorRequestResponseNext(),
104+
kind)
105+
else
106+
result =
107+
getRouteHandlerParameter(routeHandler, RouteHandlerSignature::requestResponseNext(), kind)
28108
}
29109

30110
/**
@@ -34,39 +114,16 @@ module ConnectExpressShared {
34114
*/
35115
class RouteHandlerCandidate extends HTTP::RouteHandlerCandidate {
36116
RouteHandlerCandidate() {
37-
exists(string request, string response, string next, string error |
38-
(request = "request" or request = "req") and
39-
(response = "response" or response = "res") and
40-
next = "next" and
41-
(error = "error" or error = "err")
42-
|
43-
// heuristic: parameter names match the documentation
44-
astNode.getNumParameter() >= 2 and
45-
getRouteHandlerParameter(astNode, "request").getName() = request and
46-
getRouteHandlerParameter(astNode, "response").getName() = response and
47-
(
48-
astNode.getNumParameter() >= 3
49-
implies
50-
getRouteHandlerParameter(astNode, "next").getName() = next
51-
) and
52-
(
53-
astNode.getNumParameter() = 4
54-
implies
55-
getRouteHandlerParameter(astNode, "error").getName() = error
56-
) and
57-
not (
58-
// heuristic: max four parameters (the server will only supply four arguments)
59-
astNode.getNumParameter() > 4
60-
or
61-
// heuristic: not a class method (the server invokes this with a function call)
62-
astNode = any(MethodDefinition def).getBody()
63-
or
64-
// heuristic: does not return anything (the server will not use the return value)
65-
exists(astNode.getAReturnStmt().getExpr())
66-
or
67-
// heuristic: is not invoked (the server invokes this at a call site we cannot reason precisely about)
68-
exists(DataFlow::InvokeNode cs | cs.getACallee() = astNode)
69-
)
117+
matchesSignature(astNode, _) and
118+
not (
119+
// heuristic: not a class method (the server invokes this with a function call)
120+
astNode = any(MethodDefinition def).getBody()
121+
or
122+
// heuristic: does not return anything (the server will not use the return value)
123+
exists(astNode.getAReturnStmt().getExpr())
124+
or
125+
// heuristic: is not invoked (the server invokes this at a call site we cannot reason precisely about)
126+
exists(DataFlow::InvokeNode cs | cs.getACallee() = astNode)
70127
)
71128
}
72129
}

‎javascript/ql/src/semmle/javascript/frameworks/Express.qll‎

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -127,14 +127,14 @@ module Express {
127127
/**
128128
* Gets the HTTP request type this is registered for, if any.
129129
*
130-
* Has no result for `use` and `all` calls.
130+
* Has no result for `use`, `all`, or `param` calls.
131131
*/
132132
HTTP::RequestMethodName getRequestMethod() { result.toLowerCase() = getMethodName() }
133133

134134
/**
135135
* Holds if this registers a route for all request methods.
136136
*/
137-
predicate handlesAllRequestMethods() { getMethodName() = "use" or getMethodName() = "all" }
137+
predicate handlesAllRequestMethods() { getMethodName() = ["use", "all", "param"] }
138138

139139
/**
140140
* Holds if this route setup sets up a route for the same
@@ -146,6 +146,11 @@ module Express {
146146
that.handlesAllRequestMethods() or
147147
this.getRequestMethod() = that.getRequestMethod()
148148
}
149+
150+
/**
151+
* Holds if this route setup is a parameter handler, such as `app.param("foo", ...)`.
152+
*/
153+
predicate isParameterHandler() { getMethodName() = "param" }
149154
}
150155

151156
/**
@@ -314,7 +319,7 @@ module Express {
314319
/**
315320
* Gets the parameter of kind `kind` of this route handler.
316321
*
317-
* `kind` is one of: "error", "request", "response", "next".
322+
* `kind` is one of: "error", "request", "response", "next", or "parameter".
318323
*/
319324
abstract SimpleParameter getRouteHandlerParameter(string kind);
320325

@@ -340,11 +345,14 @@ module Express {
340345
class StandardRouteHandler extends RouteHandler, HTTP::Servers::StandardRouteHandler,
341346
DataFlow::ValueNode {
342347
override Function astNode;
348+
RouteSetup routeSetup;
343349

344-
StandardRouteHandler() { this = any(RouteSetup setup).getARouteHandler() }
350+
StandardRouteHandler() { this = routeSetup.getARouteHandler() }
345351

346352
override SimpleParameter getRouteHandlerParameter(string kind) {
347-
result = getRouteHandlerParameter(astNode, kind)
353+
if routeSetup.isParameterHandler()
354+
then result = getRouteParameterHandlerParameter(astNode, kind)
355+
else result = getRouteHandlerParameter(astNode, kind)
348356
}
349357
}
350358

@@ -453,32 +461,31 @@ module Express {
453461
string kind;
454462

455463
RequestInputAccess() {
456-
exists(DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
464+
exists(DataFlow::SourceNode request | request = rh.getARequestSource().ref() |
457465
kind = "parameter" and
458466
(
459-
this.(DataFlow::MethodCallNode).calls(request, "param")
467+
this = request.getAMethodCall("param")
460468
or
461-
exists(DataFlow::PropRead base, string propName |
462-
// `req.params.name` or `req.query.name`
463-
base.accesses(request, propName) and
464-
this = base.getAPropertyReference(_)
465-
|
466-
propName = "params" or
467-
propName = "query"
468-
)
469+
this = request.getAPropertyRead(["params", "query"]).getAPropertyRead()
469470
)
470471
or
471472
// `req.originalUrl`
472473
kind = "url" and
473-
this.(DataFlow::PropRef).accesses(request, "originalUrl")
474+
this = request.getAPropertyRead("originalUrl")
474475
or
475476
// `req.cookies`
476477
kind = "cookie" and
477-
this.(DataFlow::PropRef).accesses(request, "cookies")
478+
this = request.getAPropertyRead("cookies")
478479
)
479480
or
480481
kind = "body" and
481482
this.asExpr() = rh.getARequestBodyAccess()
483+
or
484+
// `value` in `router.param('foo', (req, res, next, value) => { ... })`
485+
kind = "parameter" and
486+
exists(RouteSetup setup | rh = setup.getARouteHandler() |
487+
this = DataFlow::parameterNode(rh.getRouteHandlerParameter("parameter"))
488+
)
482489
}
483490

484491
override RouteHandler getRouteHandler() { result = rh }
@@ -848,10 +855,14 @@ module Express {
848855
*/
849856
private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
850857
HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
851-
TrackedRouteHandlerCandidateWithSetup() { this = any(RouteSetup s).getARouteHandler() }
858+
RouteSetup routeSetup;
859+
860+
TrackedRouteHandlerCandidateWithSetup() { this = routeSetup.getARouteHandler() }
852861

853862
override SimpleParameter getRouteHandlerParameter(string kind) {
854-
result = getRouteHandlerParameter(astNode, kind)
863+
if routeSetup.isParameterHandler()
864+
then result = getRouteParameterHandlerParameter(astNode, kind)
865+
else result = getRouteHandlerParameter(astNode, kind)
855866
}
856867
}
857868

‎javascript/ql/src/semmle/javascript/frameworks/Firebase.qll‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,12 +219,14 @@ module Firebase {
219219
*/
220220
private class RouteHandler extends Express::RouteHandler, HTTP::Servers::StandardRouteHandler,
221221
DataFlow::ValueNode {
222+
override Function astNode;
223+
222224
RouteHandler() { this = any(RouteSetup setup).getARouteHandler() }
223225

224226
override SimpleParameter getRouteHandlerParameter(string kind) {
225-
kind = "request" and result = this.(DataFlow::FunctionNode).getParameter(0).getParameter()
227+
kind = "request" and result = astNode.getParameter(0)
226228
or
227-
kind = "response" and result = this.(DataFlow::FunctionNode).getParameter(1).getParameter()
229+
kind = "response" and result = astNode.getParameter(1)
228230
}
229231
}
230232
}

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

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,20 @@ module HTTP {
204204
*/
205205
abstract HeaderDefinition getAResponseHeader(string name);
206206

207+
/**
208+
* Gets a request object originating from this route handler.
209+
*
210+
* Use `RequestSource.ref()` to get reference to this request object.
211+
*/
212+
final Servers::RequestSource getARequestSource() { result.getRouteHandler() = this }
213+
214+
/**
215+
* Gets a response object originating from this route handler.
216+
*
217+
* Use `ResponseSource.ref()` to get reference to this response object.
218+
*/
219+
final Servers::ResponseSource getAResponseSource() { result.getRouteHandler() = this }
220+
207221
/**
208222
* Gets an expression that contains a request object handled
209223
* by this handler.
@@ -296,14 +310,18 @@ module HTTP {
296310
*/
297311
abstract RouteHandler getRouteHandler();
298312

299-
predicate flowsTo(DataFlow::Node nd) { ref(DataFlow::TypeTracker::end()).flowsTo(nd) }
313+
/** DEPRECATED. Use `ref().flowsTo()` instead. */
314+
deprecated predicate flowsTo(DataFlow::Node nd) { ref().flowsTo(nd) }
300315

301316
private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
302317
t.start() and
303318
result = this
304319
or
305320
exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
306321
}
322+
323+
/** Gets a `SourceNode` that refers to this request object. */
324+
DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
307325
}
308326

309327
/**
@@ -317,14 +335,18 @@ module HTTP {
317335
*/
318336
abstract RouteHandler getRouteHandler();
319337

320-
predicate flowsTo(DataFlow::Node nd) { ref(DataFlow::TypeTracker::end()).flowsTo(nd) }
338+
/** DEPRECATED. Use `ref().flowsTo()` instead. */
339+
deprecated predicate flowsTo(DataFlow::Node nd) { ref().flowsTo(nd) }
321340

322341
private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
323342
t.start() and
324343
result = this
325344
or
326345
exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
327346
}
347+
348+
/** Gets a `SourceNode` that refers to this response object. */
349+
DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
328350
}
329351

330352
/**
@@ -333,7 +355,7 @@ module HTTP {
333355
class StandardRequestExpr extends RequestExpr {
334356
RequestSource src;
335357

336-
StandardRequestExpr() { src.flowsTo(DataFlow::valueNode(this)) }
358+
StandardRequestExpr() { src.ref().flowsTo(DataFlow::valueNode(this)) }
337359

338360
override RouteHandler getRouteHandler() { result = src.getRouteHandler() }
339361
}
@@ -344,7 +366,7 @@ module HTTP {
344366
class StandardResponseExpr extends ResponseExpr {
345367
ResponseSource src;
346368

347-
StandardResponseExpr() { src.flowsTo(DataFlow::valueNode(this)) }
369+
StandardResponseExpr() { src.ref().flowsTo(DataFlow::valueNode(this)) }
348370

349371
override RouteHandler getRouteHandler() { result = src.getRouteHandler() }
350372
}
@@ -370,6 +392,7 @@ module HTTP {
370392
/**
371393
* Gets a route handler that is defined by this setup.
372394
*/
395+
pragma[nomagic]
373396
abstract DataFlow::SourceNode getARouteHandler();
374397

375398
/**

0 commit comments

Comments
 (0)