JS: reintroduce js/server-crash

[esbena]

This closes https://github.com/github/codeql-javascript-team/issues/188 by rewriting the reverted js/server-crash - see original PR and its revert.

The core idea remains the same, but there are two major implementation changes:

• the callgraph is now traversed with type-tracking like predicates instead of explicit transitive closures
• the query is a @kind path-problem query that uses the path to explain how the exception connects to the async call back and the route handler
  • please consider the usability of this thoroughly since this is the first path explanation that is not about data flow

Sample run: https://lgtm.com/query/4927397113480631507/. To view a path explanation in vscode, try the eclipse/orion.client database.

The qhelp is still missing. Lets first agree that we believe in the current formulation of the query.

For the record, there is plenty of future work for this query. See https://github.com/github/codeql-javascript-team/issues/105.

[erik-krogh]

The approach sounds good to me, and I also like the use of path-problem to show how the exception throw relates to the route handler.

I think we could consider hiding the mid-nodes that are function-nodes.
E.g. looking at eclipse/orion.client, the function writeResponse takes a lot of space, without really contributing.

Just a single suggestion (in two parts) explicitly stating that invokedByRouteHandler can pass through try-catch statements.

[esbena]

Lets try again - I had to do another rewrite 🙄
The qhelp is also available now.

Presentation

(see this 236-run)

The approach sounds good to me, and I also like the use of path-problem to show how the exception throw relates to the route handler.

We only present the exception path now, the path from the thrower to the callback is a data flow path, and the path from the asynchronous call to the route handler is a control flow path, and it can become quite confusing for the reader to traverse a path with two kinds of flow. Besides, I found that there is a context sensitivity requirement that makes the edges relation more complex to compute if the asynchronous call is a mandatory mid-node.

I think we could consider hiding the mid-nodes that are function-nodes.

I went back and forth on this, ultimately I found it more useful (albeit verbose) to have the function nodes in the path.

Performance

After a LGTM-run-on-all and a few optimizations, I have not observed any of the slowest project to require more than 100 seconds of analysis locally.

The speed is backed by a unstable dist-compare for js/xss vs js/xss + js/server-crash.

I had to do another rewrite to handle performance and presentation.

[erik-krogh]

A few nits, but otherwise it looks fine to me.