Ruby: rack - model more responses and app types #13483
Conversation
alexrford
force-pushed
the
rb/rack-extend-app-and-resp
branch
from
d7dbc40 to
eb5b6a1
Compare
alexrford
force-pushed
the
rb/rack-extend-app-and-resp
branch
from
eb5b6a1 to
f8140bc
Compare
| /** | ||
| * Gets the singleton method named `name` available in this module, including methods inherited from ancestors. | ||
| * | ||
| * Overridden methods are not included. | ||
| */ | ||
| MethodNode getSingletonMethod(string name) { | ||
| result.asCallableAstNode() = super.getAnOwnSingletonMethod() and | ||
| result.getMethodName() = name | ||
| } |
There was a problem hiding this comment.
The comment and method name suggests that this includes inherited methods, but the implementation only includes the own singleton methods.
There was a problem hiding this comment.
🤦 thanks, yes the body doesn't match. We don't need this predicate if we drop the distinction between class apps and anonymous apps, so I'll just remove this predicate.
| * A callable node that takes a single argument and, if it has a method name, | ||
| * is called "call". | ||
| */ | ||
| private class PotentialCallNode extends DataFlow::CallableNode { |
There was a problem hiding this comment.
Since this is a callable, the name should not imply that it is a call, which can lead to a lot of confusion. (Same applies to CallNode below)
Could we rename it to something like a "request handler" instead of just "call method" or "callable"?
There was a problem hiding this comment.
Renamed this as PotentialRequestHandler and the version where we track the response type as RequestHandler.
|
|
||
| private newtype TApp = | ||
| TClassApp(DataFlow::ClassNode cn, CallNode call) or | ||
| TAnonymousApp(CallNode call) |
There was a problem hiding this comment.
This newtype doesn't seem necessary to me.
The corresponding class can always be determined by checking if call is a method of some class.
But also, do we actually care about what the class is? Does Rack distinguish between handlers that are Procs versus handlers that are some of type of object? If not I'd just model the request handler as a DataFlow::CallableNode.
There was a problem hiding this comment.
The main purpose here was to maintain similar behaviour as before where we considered the class containing the call method as the app if such a class existed. In practice, I don't think it matters? As you mention, we can figure this out from the RequestHandler if it ends up being relevant.
On that note I've moved the RequestHandler itself to replace the RackApplication class - this is the DataFlow::CallableNode corresponding to the method/proc/lambda that takes the env and returns a rack compliant response.
|
@asgerf this is updated - as we briefly discussed, recognition of request handlers can likely be improved using your API graphs overhaul. The overall diff for this PR is simpler than before. I'm interested in if the publicly exposed API for |
|
I've started another evaluation with meta-queries as I'd like a closer look at what sinks are found by this heuristic in practice. LGTM if the results look reasonable. |
asgerf
left a comment
asgerf
left a comment
There was a problem hiding this comment.
LGTM - the evaluation was quiet
2 participants
This is a follow-up to #13289.
Rack responses can be constructed using calling
finishon aRack::Responseinstance. This may be more convenient in some cases (e.g. default values are set when callingRack::Response.newif not specified). This PR adds modelling to consider these calls tofinishas creating a rack response.This PR also extends recognition of rack apps to include:
call(env)method is a class method rather than an instance methodIn the case of anonymous applications we limit candidates to those which flow to arg0 of a
runcall, as considering all lambdas/procs can be prohibitively expensive.