ATM: Simplify query configurations

[tiferet]

This PR simplifies the query configurations. It moves common functionality from the derived classes up to AtmConfig. It also combines the two configs each query previously had into a single config, simplifying the code and making it easier to understand. This will prevent code duplication and bugs, improve maintainability, and make it easier to add new queries.

Once again commit-by-commit review is recommended 😄

Main changes

• AtmConfig inherits from TaintTracking::Configuration. That way the specific query configs (e.g. DomBasedXssAtmConfig), which inherit from AtmConfig, also inherit from TaintTracking::Configuration. This removes the need for two separate config classes for each query (e.g. DomBasedXssAtmConfig and XssATM::Configuration). Note: We could also keep AtmConfig as is and have the query configs inherit both from it and from TaintTracking::Configuration directly, but I think having AtmConfig inherit from TaintTracking::Configuration makes more sense: it's a query configuration for a boosted taint tracking query, which is a type of taint tracking query that adds a few extra predicates (e.g. isKnownSink).
• Move the definition of isSource to the base class: A long as we're not boosting sources, isSource is identical to isKnownSource.
• Move the definition of isSink to the base class: Holds if sink is a known taint sink or an "effective" sink (a candidate to be classified by an ML model).
• Remove code duplication in query .ql files: Define the query for finding ATM alerts in the base class AtmConfig, and call it from each query's .ql file.

Consistency check

This DCA experiment uses the model on main with the final commit of each PR from the CodeQL revamp. The idea is to double-check that none of the changes I've made has affected the endpoints that get scored at inference time, since this is not directly tested for in the PR checks. We see identical results from all variants (e.g. this), confirming that none of the revamp work has changed the endpoints being scored.

Timing checks

• ✅ KPI timing experiment: https://github.com/github/codeql-dca-main/issues/8684
• ☑️ The local runtime of endpoint_large_scale/ExtractEndpointDataTraining remains like it was after the last PR that affected timing: About 5s.

Addresses most of https://github.com/github/ml-ql-adaptive-threat-modeling/issues/2134.

[tiferet]

@kaeluka when you review this PR, do you mind looking at the possible issue Henry raised here. I think it's OK, but I have limited understanding of the issue he's pointing out. Henry said From a very very quick scan, it looks OK, but I’d recommend taking a closer look. I'd appreciate your eyes on it as well to make sure there isn't a problem.

[kaeluka]

Thank you for point this out! I was going to, but I definitely want to talk to Henry about it as well!

[kaeluka]

As part of digging into the questions brought up by Henry, I added the following snippet to each file modified in this PR and quick-evaluated it:

predicate test(DataFlow::Configuration a) {
  any()
}

This showed me that there's actually a few files where there's several instances of DataFlow::Configuration in scope. I believe that, in each case, the situation is not preventing the PR from being merged.

These files are:

• ✅ EndpointFeatures.ql - benign because the query only computes features, rather than evaluating flow.
• ✅ FilteredTruePositives.ql - benign because the query only runs queries involving characteristics, rather than evaluating flow (and it's just a unit test).
• ✅ ExtractEndpointMapping.ql - benign because the query does not evaluate flow
• ✅ DebugResultInclusion.ql - this might actually be slowed down because it uses hasFlow but a) it's only a debug query, and b) the situation is like that already on main (the PR only changes some names in the file).

@henrymercer, is my reasoning sound?

[henrymercer]

@henrymercer, is my reasoning sound?

Your reasoning around the files you listed looks good. Most of those files are test or debug files. I'm not sure how you handled the case of libraries being changed, as we'd need to check all the files that use this library and not just the ones being changed.

I did try evaluating the predicate top-down on each of the queries and they each reported a single dataflow configuration, so I think we're good to go.

[kaeluka]

Perfect! I'll continue with the conventional part of the review :) This should be done ~tomorrow lunchtime, @tiferet. Only minor comments so far, but will take some time to think about the big picture tomorrow.

[tiferet]

@kaeluka Since you're already in the middle of reviewing this PR, I don't want to rebase it on top of the latest version of tiferet/complexity-reduction and interfere with your ongoing work. Please LMK what your preference is. Should I:

• Rebase on top of tiferet/complexity-reduction now?
• Wait until tiferet/complexity-reduction is merged to main and then rebase on top of main?
• Wait until tiferet/complexity-reduction is merged to main and then merge main into this branch?
• Something else?

[kaeluka]

The big question, whether this might introduce a pathological dependency graph, here has been tackled in the PR discussion already.

👍: The rest of the review is that I:

• Like the change and am not asking you to make any big changes.
• I'd slightly prefer to follow convention in the naming of the Config classes but it's not a hill I'll die on.
• I was a bit surprised to see the PR, as it seems like it's a bit of a side track from the grand design we're following. Happy to see it either way, though!

[tiferet]

I was a bit surprised to see the PR, as it seems like it's a bit of a side track from the grand design we're following. Happy to see it either way, though!

Yes, I think we're already done implementing the design. All the issues left here are improvements we can make either to the code or to the data now that the refactoring is complete 🎉

[adityasharad]

Minor/bikeshed: I'd prefer a name that doesn't suggest a return/result value, and mentions flow. Also makes it easier to see the parallels between unboosted and boosted queries. How about hasLikelyFlowPath?

[tiferet]

I don't know about hasLikelyFlowPath, because one of the conditions it must meet is hasFlowPath. Logically hasFlowPath should imply hasLikelyFlowPath.

Maybe something like hasBoostedFlowPath? We've been trying to get rid of the "boosted" terminology, but I can't think of something better right now. I think hasAlert would be clearer (ultimately that's what this predicate does -- it surfaces all the alerts for this config), but that doesn't mention flow.

[tiferet]

Note: I tracked this renaming together with the other renamings here, so that we can merge this PR and XssThroughDOM and open a separate PR for all the renamings