In cases where we copy actual business logic into the location selection predicates, we should consider to re-factor the code to enable sharing and discoverability. If someone changes the logic of the query, then it will be easy to forget to check whether location selection predicates need to be changed as well.

Maybe consider introducing a helper predicate (above the config module)

Event getRelevantEvent(DataFlow::Node node) {
  inPrivilegedContext(node.asExpr(), result) and
  not exists(ControlCheck check | check.protects(node.asExpr(), result, "argument-injection"))
}

Then it is possible to write the where clause of the query as

ArgumentInjectionFlow::flowPath(source, sink) and
event = getRelevantEvent(sink.getNode())

and then one can declare

Location getASelectedSinkLocation(DataFlow::Node sink) {
  result = sink.getLocation()
  or
  result = getRelevantEvent(sink).getLocation()
}

Same comment as above.

I don't fully understand this. The part of the where clauses this appears to mirror is

  inPrivilegedContext(sink.getNode().asExpr(), event) and
  source.getNode().(RemoteFlowSource).getEventName() = event.getName() and
  not exists(ControlCheck check | check.protects(sink.getNode().asExpr(), event, "code-injection")) and
  // exclude cases where the sink is a JS script and the expression uses toJson
  not exists(UsesStep script |
    script.getCallee() = "actions/github-script" and
    script.getArgumentExpr("script") = sink.getNode().asExpr() and
    exists(getAToJsonReferenceExpression(sink.getNode().asExpr().(Expression).getExpression(), _))
  )

and it looks like maybe we should perceive the location of an event as being related to the source node location (instead of the sink)?

Location getASelectedSourceLocation(DataFlow::Node source) {
  exists(Event event | result = event.getLocation() |
    source.(RemoteFlowSource).getEventName() = event.getName()
  )
}

This yields an over-approximation of the mapping of locations from source nodes.

The only change I made from the original where-clause is to replace source.getNode().(RemoteFlowSource) with a RemoteFlowSource source where isSource(source). In this case event is related to both source and sink, so whichever one we use to determine event it will be an over-approximation. There are more ties to sink, so I chose to put it in getASelectedSinkLocation. I could also do it the way you suggest, but I don't know enough about the query to know which one will yield a tighter approximation.

Ah, yes, then I understand. In that case, I am a bit worried that the current implementation might yield an under-approximation (also note that I am not familiar with the actions queries or libraries). In the where clause in the query, only the sources where there is flow to the sink are used under the negation. If we consider all remote flow sources (for each sink - no matter if there is flow) we might risk filtering out more than we want.
The two options I see are

• Use the getASelectedSourceLocation as above.
• Use the inPrivilegedContext predicate in getASelectedSinkLocation to get event(s) and use their locations as possible selected sink locations (and remove all the negation stuff).

I don't know which of the two that will yield the tightest approximation.

Since in the ArgumentInjection and ArtifactPoisoning case above we are fine with having sink under negation, that should be fine in this case too, right? Or should I avoid the negation in those cases too?

In those cases, the negation should be fine as the negation is on the sink node itself.
The reason why I am worried about the negation on sources is because it basically says something like
\forall sources. \not p(source, ...)
where the sources are not limited to a specific set of sources that satisfies flow(source, sink)

Right, so if we ignore all the sources, and only limit it to the clauses regarding sink, then I guess it should be fine.

Yes, that is the best (over)approximation I can come up with. Alternatively, the query should not be diff-informed.

This doesn't look equivalent to the where clause in the query as "any" source is used (and not only the one from the flowpath), which could produce a smaller set of locations as the source is used under a negation.

I see. I suppose getting rid of the not exists(…) would be fine then? In which case, we would get an over-approximation.

Yes, that seems plausible!

Maybe re-factor to avoid code duplication.

Same comment wrt. using "any" source and not only those from flow - we might get an under-approximation?

Same comment wrt. under-approximation.