JS: Add some support for indirect route handlers#4275
JS: Add some support for indirect route handlers#4275codeql-ci merged 18 commits intogithub:mainfrom
Conversation
erik-krogh
force-pushed
the
CVE760-indirect
branch
from
5edc518 to
0b16f81
Compare
esbena
left a comment
esbena
left a comment
There was a problem hiding this comment.
So in summary: we can now identify RouteHandlerCandidates as proper route handlers if they are invoked trivially through wrapper-functions. The idea SGTM.
My main concern is the location of the predicates.
| */ | ||
| private predicate forwardingCall(DataFlow::SourceNode callee, HTTP::RouteHandlerCandidate f) { | ||
| exists(DataFlow::CallNode call | call = callee.getACall() | | ||
| f.getNumParameter() >= 2 and |
There was a problem hiding this comment.
Why do we exclude f.getNumParameter() = 1?
There was a problem hiding this comment.
That was a "this should look like a route-handler" constraint.
But now that the type of f is HTTP::RouteHandlerCandidate, that constraint is unnecessary.
| } | ||
|
|
||
| override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) { | ||
| override RouteHandlerCandidate getRouteHandler(DataFlow::SourceNode access) { |
There was a problem hiding this comment.
Do we really need this restriction, isn't the inline cast sufficient anymore? I get that the extends HTTP::RouteHandlerCandidateContainer::Range naming heavily implies this type, but then the return type ought to be restricted in that super class instead IMO. I think the generalized return type in the super class is quite deliberate.
There was a problem hiding this comment.
The inline cast is perfectly sufficient.
I just did this change as a drive-by refactorization to help my own understanding of the predicate while I was editing the predicate.
But yeah, having a different return-type compared to the super class can be confusing, so I'll change it back.
There was a problem hiding this comment.
it looks like this wasn't changed back after all.
There was a problem hiding this comment.
Right. I only changed it only for ContainerObject, and forgot ContainerCollection.
| } | ||
|
|
||
| override DataFlow::SourceNode getRouteHandler(DataFlow::SourceNode access) { | ||
| override RouteHandlerCandidate getRouteHandler(DataFlow::SourceNode access) { |
There was a problem hiding this comment.
it looks like this wasn't changed back after all.
esbena
left a comment
esbena
left a comment
There was a problem hiding this comment.
Last nit: Lets add the improved framework models to the change-notes.
4 participants
Gets a TP for CVE-2020-15135 (after #4247 is merged).
Only the first commit is needed to get the TP.
The other commits tries to generalize a bit, in order to support these two examples of indirect route handlers: one, two.
An evaluation shows a decent amount of new route-handlers found.And performance is "not great not terrible" - I'll look into it.An evaluation shows a decent amount of new route-handlers found.
And performance looks good.