Skip to content

Commit 3dd5d4d

Browse files
asgerfasgerf
committed
JS: Instantiate for Express and add tests
1 parent aae4260 commit 3dd5d4d

File tree

8 files changed

+563
-4
lines changed

8 files changed

+563
-4
lines changed

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

Lines changed: 64 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,63 @@ module Express {
7575
result = "del"
7676
}
7777

78+
private class RouterRange extends Routing::Router::Range {
79+
RouterDefinition def;
80+
81+
RouterRange() { this = def.flow() }
82+
83+
override DataFlow::SourceNode getAReference() { result = def.ref() }
84+
}
85+
86+
private class RoutingTreeSetup extends Routing::RouteSetup::MethodCall {
87+
RoutingTreeSetup() { asExpr() instanceof RouteSetup }
88+
89+
override string getRelativePath() {
90+
not getMethodName() = "param" and // do not treat parameter name as a path
91+
result = getArgument(0).getStringValue()
92+
}
93+
94+
override HTTP::RequestMethodName getHttpMethod() { result.toLowerCase() = getMethodName() }
95+
}
96+
97+
/**
98+
* A route setup performed via `express-limiter`.
99+
*
100+
* `express-limiter` is unusual in that it can install the middleware on its own,
101+
* rather than expecting the caller to install it with `app.use()` or similar.
102+
*
103+
* For example:
104+
* ```js
105+
* let app = express();
106+
* require('express-limiter')(app, client)({ method: 'get', path: '/foo' });
107+
* ```
108+
*/
109+
private class RateLimiterRouteSetup extends Routing::RouteSetup::Range {
110+
DataFlow::CallNode limitCall;
111+
112+
RateLimiterRouteSetup() {
113+
limitCall = DataFlow::moduleImport("express-limiter").getACall() and
114+
exists(this.(DataFlow::CallNode).getOptionArgument(0, ["path", "method"])) and
115+
this = limitCall.getACall()
116+
}
117+
118+
override predicate isInstalledAt(Routing::Router::Range router, ControlFlowNode cfgNode) {
119+
router.getAReference().getALocalUse() = limitCall.getArgument(0) and
120+
cfgNode = asExpr()
121+
}
122+
}
123+
124+
private class AppTree extends Routing::Node {
125+
AppTree() { this = Routing::getNode(appCreation()) }
126+
127+
override DataFlow::Node getValueAtAccessPath(int n, string path) {
128+
// req.app and res.app refer to the app object
129+
n = [0, 1] and
130+
path = "app" and
131+
this = Routing::getNode(result)
132+
}
133+
}
134+
78135
/**
79136
* A call to an Express router method that sets up a route.
80137
*/
@@ -788,10 +845,13 @@ module Express {
788845
exists(DataFlow::TypeTracker t2 | result = this.ref(t2).track(t2, t))
789846
}
790847

848+
/** Gets a data flow node referring to this router. */
849+
DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
850+
791851
/**
792852
* Holds if `sink` may refer to this router.
793853
*/
794-
predicate flowsTo(Expr sink) { this.ref(DataFlow::TypeTracker::end()).flowsToExpr(sink) }
854+
predicate flowsTo(Expr sink) { this.ref().flowsToExpr(sink) }
795855

796856
/**
797857
* Gets a `RouteSetup` that was used for setting up a route on this router.
@@ -970,9 +1030,9 @@ module Express {
9701030
*/
9711031
private class RenderCallAsTemplateInstantiation extends Templating::TemplateInstantiation::Range,
9721032
DataFlow::CallNode {
973-
RenderCallAsTemplateInstantiation() {
974-
this = any(ResponseSource res).ref().getAMethodCall("render")
975-
}
1033+
ResponseSource res;
1034+
1035+
RenderCallAsTemplateInstantiation() { this = res.ref().getAMethodCall("render") }
9761036

9771037
override DataFlow::Node getTemplateFileNode() { result = this.getArgument(0) }
9781038

‎javascript/ql/test/library-tests/Routing/route-setups.js

Lines changed: 293 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,293 @@
1+
const express = require('express');
2+
const TestObj = require('@example/test');
3+
4+
function basicTaint() {
5+
const app = express();
6+
app.use((req, res, next) => {
7+
req.tainted = source();
8+
req.safe = 'safe';
9+
next();
10+
});
11+
app.get('/', (req, res) => {
12+
sink(req.tainted); // NOT OK
13+
sink(req.safe); // OK
14+
});
15+
}
16+
17+
function basicApiGraph() {
18+
const app = express();
19+
app.use((req, res, next) => {
20+
req.obj = new TestObj();
21+
next();
22+
});
23+
app.get('/', (req, res) => {
24+
sink(req.obj.getSource()); // NOT OK
25+
req.obj.getSink(source()); // NOT OK
26+
});
27+
}
28+
29+
function noTaint() {
30+
const app = express();
31+
function middleware(req, res, next) {
32+
req.tainted = source();
33+
req.safe = 'safe';
34+
next();
35+
}
36+
app.get('/unsafe', middleware, (req, res) => {
37+
sink(req.tainted); // NOT OK
38+
sink(req.safe); // OK
39+
});
40+
app.get('/safe', (req, res) => {
41+
sink(req.tainted); // OK - not preceded by middleware
42+
sink(req.safe); // OK
43+
});
44+
}
45+
46+
function looseApiGraphStep() {
47+
const app = express();
48+
function middleware(req, res, next) {
49+
req.obj = new TestObj();
50+
next();
51+
}
52+
app.get('/unsafe', middleware, (req, res) => {
53+
sink(req.obj.getSource()); // NOT OK
54+
});
55+
app.get('/safe', (req, res) => {
56+
sink(req.obj.getSource()); // NOT OK - we allow API graph steps within the same app
57+
});
58+
}
59+
60+
function chainMiddlewares() {
61+
const app = express();
62+
function step1(req, res, next) {
63+
req.taint = source();
64+
next();
65+
}
66+
function step2(req, res, next) {
67+
req.locals.alsoTaint = req.taint;
68+
next();
69+
}
70+
app.get('/', step1, step2, (req, res) => {
71+
sink(req.taint); // NOT OK
72+
sink(req.locals.alsoTaint); // NOT OK
73+
});
74+
}
75+
76+
function routerEscapesIntoParameter() {
77+
const app = express();
78+
function setupMiddlewares(router) {
79+
router.use((req, res, next) => {
80+
req.taint = source();
81+
next();
82+
});
83+
}
84+
app.get('/before', (req, res) => {
85+
sink(req.taint); // OK
86+
});
87+
setupMiddlewares(app);
88+
app.get('/after', (req, res) => {
89+
sink(req.taint); // NOT OK
90+
});
91+
}
92+
93+
function routerReturned() {
94+
const app = express();
95+
function makeMiddlewares() {
96+
let router = new express.Router();
97+
router.use((req, res, next) => {
98+
req.taint = source();
99+
next();
100+
});
101+
return router;
102+
}
103+
app.get('/before', (req, res) => {
104+
sink(req.taint); // OK
105+
});
106+
app.use(makeMiddlewares());
107+
app.get('/after', (req, res) => {
108+
sink(req.taint); // NOT OK
109+
});
110+
}
111+
112+
function routerCaptured() {
113+
const app = express();
114+
function addMiddlewares() {
115+
app.use((req, res, next) => {
116+
req.taint = source();
117+
next();
118+
});
119+
}
120+
app.get('/before', (req, res) => {
121+
sink(req.taint); // OK
122+
});
123+
addMiddlewares();
124+
app.get('/after', (req, res) => {
125+
sink(req.taint); // NOT OK
126+
});
127+
}
128+
129+
function withPath() {
130+
const app = express();
131+
app.use('/foo', (req, res, next) => {
132+
req.taint = source();
133+
next();
134+
});
135+
app.get('/foo', (req, res) => {
136+
sink(req.taint); // NOT OK
137+
});
138+
app.get('/bar', (req, res) => {
139+
sink(req.taint); // OK
140+
});
141+
}
142+
143+
144+
function withNestedPath() {
145+
const app = express();
146+
app.use('/foo/bar', (req, res, next) => {
147+
req.taint = source();
148+
next();
149+
});
150+
function makeRouter() {
151+
const router = express.Router();
152+
router.get('/bar', (req, res) => {
153+
sink(req.taint); // NOT OK
154+
})
155+
router.get('/baz', (req, res) => {
156+
sink(req.taint); // OK
157+
})
158+
return router;
159+
}
160+
app.get('/foo', makeRouter());
161+
app.get('/bar', (req, res) => {
162+
sink(req.taint); // OK
163+
});
164+
}
165+
166+
function withHttpMethods() {
167+
const app = express();
168+
app.get('/foo', (req, res, next) => {
169+
req.taintGet = source();
170+
next();
171+
});
172+
app.post('/foo', (req, res, next) => {
173+
req.taintPost = source();
174+
next();
175+
});
176+
app.all('/foo', (req, res, next) => {
177+
req.taintAll = source();
178+
next();
179+
});
180+
app.get('/foo/a', (req, res) => {
181+
sink(req.taintGet); // NOT OK
182+
sink(req.taintPost); // OK - not tainted for GET requests
183+
sink(req.taintAll); // NOT OK
184+
});
185+
app.post('/foo/b', (req, res) => {
186+
sink(req.taintGet); // OK - not tainted for POST requests
187+
sink(req.taintPost); // NOT OK
188+
sink(req.taintAll); // NOT OK
189+
});
190+
app.all('/foo/c', (req, res) => {
191+
sink(req.taintGet); // NOT OK
192+
sink(req.taintPost); // NOT OK
193+
sink(req.taintAll); // NOT OK
194+
});
195+
}
196+
197+
function withChaining() {
198+
const app = express();
199+
app.use('/', (req, res) => {
200+
sink(req.taint); // OK
201+
});
202+
function middleware() {
203+
return express.Router()
204+
.use((req, res, next) => {
205+
req.taint = source();
206+
next();
207+
})
208+
.use(blah());
209+
}
210+
app.use(middleware());
211+
app.use('/', (req, res) => {
212+
sink(req.taint); // NOT OK
213+
});
214+
}
215+
216+
function withClass() {
217+
class App {
218+
constructor(router) {
219+
this.router = router;
220+
this.earlyRoutes();
221+
this.addMiddleware();
222+
this.lateRoutes();
223+
}
224+
earlyRoutes() {
225+
this.router.get('/', (req, res) => {
226+
sink(req.taint); // OK
227+
})
228+
}
229+
addMiddleware() {
230+
this.router.use((req, res, next) => {
231+
req.taint = source();
232+
next();
233+
})
234+
}
235+
lateRoutes() {
236+
this.router.get('/', (req, res) => {
237+
sink(req.taint); // NOT OK
238+
})
239+
}
240+
}
241+
let app = new App(express());
242+
}
243+
244+
function withArray() {
245+
const app = express();
246+
app.get('/before', (req, res) => {
247+
sink(req.taint); // OK
248+
});
249+
app.use([
250+
(req, res, next) => {
251+
sink(req.taint); // OK
252+
next();
253+
},
254+
(req, res, next) => {
255+
req.taint = source();
256+
next();
257+
},
258+
(req, res, next) => {
259+
sink(req.taint); // NOT OK
260+
next();
261+
}
262+
]);
263+
app.get('/after', (req, res) => {
264+
sink(req.taint); // NOT OK
265+
});
266+
}
267+
268+
function withForLoop() {
269+
const app = express();
270+
let middlewares = [
271+
(req, res, next) => {
272+
sink(req.taint); // OK
273+
next();
274+
},
275+
(req, res, next) => {
276+
req.taint = source();
277+
next();
278+
},
279+
(req, res, next) => {
280+
sink(req.taint); // NOT OK
281+
next();
282+
}
283+
];
284+
app.get('/before', (req, res) => {
285+
sink(req.taint); // OK
286+
});
287+
for (let route of middlewares) {
288+
app.use(route);
289+
}
290+
app.get('/after', (req, res) => {
291+
sink(req.taint); // NOT OK
292+
});
293+
}

‎javascript/ql/test/library-tests/Routing/test.expected

Whitespace-only changes.

0 commit comments

Comments
 (0)