JS: More data flow through classes

[asger-semmle]

Some improvements inspired by ODASA-7605, improving our data-flow library's handling of classes.

The first commit adds a return step from this in a constructor back to the caller, assuming the caller is a new call.

The two other commits are a bit more controversial:

One adds a call step from this in a constructor to this in each of the methods in that class. The idea is that you can't call the method without first calling the constructor on that class, so anything you pass to the constructor can most likely flow into the methods. For example:

class C {
  constructor(x) { this.x = x }
  unsafe() { sink(x) }
}
new C(source());

There is potential for FPs here as the specific call site passing in a tainted value might not be using the unsafe method. However, it could be useful for analyzing frameworks where the methods are being invoked from client code.

The last change introduces DataFlow::ClassNode for dealing with various forms of class definitions. I opted to stick with a chainable API for handling common tasks well, rather than mirroring the entire class hierarchy from Classes.qll.

I can separate out the commits into individual PRs if anyone prefers.

[asger-semmle]

Evaluation shows no result changes but a little bit of performance to investigate.

[ghost]

I like the ideas in this PR, but I would prefer a separate merge of the DataFlow::ClassNode feature.
I think the React libraries and queries could be a good test of the new API, js/unbound-event-handler-receiver is for example hard-wired to only reason about ES6 classes at the moment.

[ghost]

A call to MemberDeclaration::isStatic is required here to avoid flow between static and non-static members.

[asger-semmle]

Should be handled by ClassNode.

[ghost]

This disjunct seems useful in a separate predicate or module for prototype reasoning (later).

[asger-semmle]

Again, I believe ClassNode resolved this comment

[ghost]

Maybe document that this flowstep does not traverse the prototype chain.

[ghost]

Could we make this class extensible? I think it would be useful to be able to support ES5 libraries for creating classes, like https://www.npmjs.com/package/create-react-class.

[ghost]

This duplicates most of ClassDefinition::getInstanceMethod.

[asger-semmle]

Some duplication here can't be avoided due to methods with computed property names.

[ghost]

Do we really want the isAmbient check in the library? Most other uses of isAmbient are in the queries.
Dittos below.

[asger-semmle]

Since this is the dataflow library, I figured we should ignore things that don't have any runtime behaviour.

[ghost]

I think the possibility of multiple prototype definitions and multiple method definitions motivates a name change to getAnInstanceMethod for this method.

[xiemaisi]

I agree with Esben about landing the third commit separately. While you're at it, you could also make the first commit its own PR. I think those two PRs would be largely uncontroversial (but I like Esben's idea of making ClassNode separate, perhaps using the ::Range pattern).

The second commit is more interesting; I'm a little bit concerned about the long-term implications of adding this flow step, but I haven't looked at the implementation in detail yet.

[asger-semmle]

Closing to open separate PRs