-
Notifications
You must be signed in to change notification settings - Fork 1.7k
JS: Add routing trees library #7049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a3374f2
to
57cc611
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 🎉
This is just a preliminary commit-by-commit review with some small comments.
I'll take another look at the entire thing later.
// The callback parameter is either a MongoClient or Db depending on the mongodb package version, | ||
// but we just model it as both. | ||
result = getAMongoDbCallback().getParameter(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a motivating example (or test) for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The db
parameter on this line is actually a client/connection, so the previous filter led to a FN there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually digging into this a bit further, it looks like mongodb@2.x
had a .db
method on a database, and had no separate concept for a connection/client. I'll update the model to reflect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and merged the two concepts entirely
@@ -18,12 +18,13 @@ import javascript | |||
import semmle.javascript.security.dataflow.MissingRateLimiting | |||
import semmle.javascript.RestrictedLocations | |||
|
|||
// TODO: Previously we used (FirstLineOf) here, but that doesn't work anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just remove this TODO.
c7ead7a
to
5559681
Compare
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very, very nice work 👍
It takes a bit to understand how the routing-trees are recursively created, but it's nice.
I only have some minor comments.
Should be good to merge afterwards.
javascript/ql/lib/semmle/javascript/security/dataflow/MissingRateLimiting.qll
Show resolved
Hide resolved
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
0b1db21
to
615b2ec
Compare
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
Thanks for the thorough review @erik-krogh! All comments addressed, and I've started another round of evaluations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Updated evaluations:
|
This introduces the
Routing
library, modeling the composition of middleware functions and route handlers, and helps reason about data flow between these.Previously we were missing data flow in cases like this:
Data-flow configurations as well as API graphs can now track flow between the two functions above.
What's in this PR
Routing
library with a bunch of abstract classes for framework models to extend.req.body
is now seen as a deeply tainted object in more cases, as the logic for detecting the body parser is now based on routing trees. We get some new results because of this.Routing tree examples
The structure of the router hierarchy is seen as a tree, with each node being a function that can dispatch the incoming request to its children, or pass it on to its next sibling. We a graph-structure used to represent the possible trees that can be constructed at runtime. (Branching control-flow can cause the graph to be non-tree shaped)
For example:
would generate a tree of form:
app
app.use()
foo()
app.get()
bar()
app.get('/)
a
b
c
Now suppose
foo()
is defined to return another router:The inner router would appear as a child in the overall routing tree:
app
app.use()
foo()
router
router.use(...)
In general, if there is flow from A to B, then A becomes a child of B. For example,
The hierarchy would be as follows, with
A
occurring in two different places, but their corresponding use-sites (B1
,B2
) still explicit in the tree.app
app.use()
B1
A
app.use()
B2
A
(In this example, the explicit
let
bindings were just there to make it easier to refer to the use-sites, but separate use-site nodes would exist even if thelet
bindings were inlined)If a router is returned, the data-flow rule wires things up nicely:
app
app.use()
makeRouter()
router
router.use()
A
An interesting case is when a mutable router object escapes into another function. In this case, we need interprocedural reasoning about side effects, since ordering of middlewares reflects the order in which certain calls are made. We simplify this by creating a separate node for each function that touches the router object, and at the point where the router escapes, we add a child relationship between caller and callee. For example:
app
app.use(cookie())
addMiddlewares(app)
(point of escape)router
(callee's view of router)router.use(A)
router.use(B)
app.get('/', ...)
With the above tree, we can see that
A,B
come aftercookie()
but before theget('/')
handler.Evaluations (internal links)