Skip to content

JS: Add more metric queries #3779

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

Merged
merged 3 commits into from
Jun 24, 2020
Merged

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jun 24, 2020

Adds some of the metrics described in https://github.com/github/codeql-javascript-team/issues/191 and cherry-picks the TypedExprs metric from #3731.

Example run, unfortunately with a buggy version of TaintedSink, and a shorter run with that bug fixed.

@asgerf asgerf added the JS label Jun 24, 2020
@asgerf asgerf requested a review from a team as a code owner June 24, 2020 09:22
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Two nits.

It is a shame that the sinks need to be mentioned explicitly, thereby demanding an update when we add new path queries. But I think we can live with that.


/** A file we ignore because it is a test file or compiled/generated/bundled code. */
class IgnoredFile extends File {
IgnoredFile() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using ClassifyFiles::classify/2 here?
(or in a different PR, since this PR just moves the class)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worry was that changes to classification would make metrics incomparable over time so I'd rather have something that excludes the most obviously uninteresting code but otherwise is fixed.

This was something that bit me when I tried to classify external calls in order measure the relative call graph completeness more accurately. It meant the goal posts kept moving and the results were much harder to interpret.

But now is definitely a good time to review this class so please suggest improvements if you think of any.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Lets keep it simple then.

Perhaps we should measure the size of classify/2 as a metric as well. If we see that it is stable in practice, then there's little harm in focusing our results with it.

(Hmm. We will even have problems without classify/2 moving goal posts. js/meta/tainted-nodes will decrease when js/meta/sanitizers-reachable-from-source increases...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If js/meta/tainted-nodes decreases due to the addition of more sanitizers, isn't that just the case of some of the old ones being FPs? All the metrics can decrease when removing FPs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Never mind that latter comment.


predicate relevantStep(DataFlow::Node pred, DataFlow::Node succ) {
any(BasicTaintConfiguration cfg).isAdditionalFlowStep(pred, succ) and
not pred.getFile() instanceof IgnoredFile and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should count it if we step into a file that we do not ignore.
Ideally, we can track taint flow through a minimised library into the ordinary client code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, one of the issues I wanted to avoid is having to triage results involving minified files. That's also something that can be solved at the UI level, like the dist-compare reports currently do for ordinary alerts, and if we do that maybe we won't need IgnoredFile at all. Even as I write this it seems obvious that that's what we should do, but maybe we can keep it like this until that's ready?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@semmle-qlci semmle-qlci merged commit daeb13d into github:master Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants