Java/C#: Update telemetry queries to report callables with sink/source neutrals as being supported.

[michaelnebel]

The telemetry queries (*/telemetry/supported-external-api and */telemetry/unsupported-external-api for java and C#) used for reporting the usage of third party APIs (or missing modeling of third party APIs) are based on the isSupported() predicate below (from ExternalApi.qll).

  /** Holds if this API is a known neutral. */
  pragma[nomagic]
  predicate isNeutral() { this instanceof FlowSummaryImpl::Public::NeutralCallable }

  /**
   * Holds if this API is supported by existing CodeQL libraries, that is, it is either a
   * recognized source, sink or neutral or it has a flow summary.
   */
  predicate isSupported() {
    this.hasSummary() or this.isSource() or this.isSink() or this.isNeutral()
  }

That is, we think of an API as being supported if there exist any model for the API. However, we didn't take sink and source neutrals into account. This is fixed in this PR.

We re-factor NeutralCallable to include all neutrals regardless of kind and introduce a new class of neutrals (NeutralSummaryCallable) to contain exactly the callables that have neutral of kind summary (i.e. this is equivalent to the previous definition of NeutralCallable).

Furthermore, we also rename SummarizedCallableBase to ModelingCandidateCallable as the name SummarizedCallableBase is misleading. Please feel free to bikeshed the name ModelingCandidateCallable.

Thank you to @starcke for bringing this to our attention!

[michaelnebel]

DCA looks good.

[jcogs33]

Looks reasonable to me. I added a couple questions.

[jcogs33]

Would it make sense for the NeutralCallable class to be in a different file than FlowSummaryImpl.qll since it is no longer specific to summaries?

Also, would it potentially make sense to move the ModelingCandidateCallable class above to a different file than FlowSummary.qll? Or would this be too much code redesign?

I might have use cases for creating SourceCallable, SinkCallable, and NeutralSinkCallable classes 😄, which could extend ModelingCandidateCallable and NeutralCallable, so I'm wondering if it would make sense to place all these callables somewhere different from the FlowSummary* files.

[michaelnebel]

IMO it is the naming of the file FlowSummaryImpl.qll (and friends) that seem incorrect as the file also includes predicates (and probably has done so for a long time) related to sources, sinks (and now also neutrals). As such, I don't think we need to split the file because (as an example) the External module in the file contains predicates which uses several of modelling predicates in conjunction with each other (as relevantSpec).
If you have uses cases for SourceCallable and friends, this is probably still the right file, but we can consider renaming the files and get rid of the FlowSummary part as it doesn't properly describe the content.

[jcogs33]

For my own understanding: Since the ModelingCandidateCallable class is not specific to flow summaries, could I have used it when creating the ModelApi class? I hadn't considered it at the time since the name and QLDoc implied that it was specific to summaries. 🤔

[michaelnebel]

By inspecting this again, I think it needs to re-written. The claims I have made are too strong (especially because there is currently no connection between sources and sinks and then this class - however I think it might be doable to make, but it is out of scope for this PR). Furthermore, since java has synthetic callables, we probably need to separate base classes for summary and neutral candidates.
I will need to do some more work on this.

I don't think you should base the ModelApi on any of the classes already related to modelling from the FlowSummary like files as we using it to test these classes.

[michaelnebel]

Looks reasonable to me. I added a couple questions.

Thank you for the review! These are excellent questions!

[michaelnebel]

DCA looks good.

[geoffw0]

Changes to Swift look harmless to me. 👍

[jcogs33]

Looks good to me. I just added a couple suggestions regarding the updates in TopJdkApis.qll.

[jcogs33]

Java looks good to me.