JS: add support for custom CSRF protecting middlewares in js/missing-token-validation#4478
Conversation
890763c to
ff054b9
Compare
| */ | ||
| private DataFlow::SourceNode nodeLeadingToCsrfWrite(DataFlow::TypeBackTracker t) { | ||
| t.start() and | ||
| exists(DataFlow::PropRef value | |
There was a problem hiding this comment.
Wouldn't this be simpler if we inline value? (Or if not, change its type to PropWrite)
| setCsrfToken: setCsrfToken | ||
| }; | ||
|
|
||
| app.use(express.csrf({ value: csrf.getCsrfToken })); |
There was a problem hiding this comment.
What is express.csrf? Does that offer CSRF protection? Because it looks like the test has two CSRF middlewares now.
There was a problem hiding this comment.
It does half of the CSRF protection.
express.csrf is responsible for checking the CSRF token.
And csrf.setCsrfToken is responsible for setting the CSRF cookie.
express.csrf has been deprecated in favor of csurf, so you will only see this pattern in older codebases.
(Altough it is easy to find uses on github).
There was a problem hiding this comment.
Shouldn't we add express.csrf to csrfMiddlewareCreation then? If it's the one that performs the check, then it's the half that matters for security.
Co-authored-by: Asger F <asgerf@github.com>
af92ead to
017c73d
Compare
Gets a TN for CVE-2020-15135
Adds detection for custom made CSFR protecting route-handlers in
js/missing-token-validation.A CSFR protecting route-handler is detected by it having some write to a CSFR related cookie or session variable.
Here are some example CSRF protecting route-handlers found with GitHub grep: https://lgtm.com/query/4468160787766813050/
An evaluation looks ok.