Skip to content

Actions: mass enable diff-informed data flow #19659

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

d10c
Copy link
Contributor

@d10c d10c commented Jun 3, 2025

An auto-generated patch that enables diff-informed data flow in the obvious cases.

Builds on #18346 and https://github.com/github/codeql-patch/pull/88

@github-actions github-actions bot added the Actions Analysis of GitHub Actions label Jun 3, 2025
@d10c d10c marked this pull request as ready for review June 4, 2025 11:32
@Copilot Copilot AI review requested due to automatic review settings June 4, 2025 11:32
@d10c d10c requested a review from a team as a code owner June 4, 2025 11:32
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables diff-informed data flow by adding the observeDiffInformedIncrementalMode predicate in the DataFlow::ConfigSig modules across various Action and security query files.

  • Introduces predicate observeDiffInformedIncrementalMode() { any() } in reusable and composite workflow config modules.
  • Adds the same predicate to security query config modules for secret exfiltration, request forgery, and output clobbering.
  • Builds on prior work to support incremental/diff-based analysis in obvious cases.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
actions/ql/src/Models/ReusableWorkflowsSummaries.ql Added observeDiffInformedIncrementalMode predicate to enable diff-informed flow.
actions/ql/src/Models/ReusableWorkflowsSources.ql Added observeDiffInformedIncrementalMode predicate to enable diff-informed flow.
actions/ql/src/Models/ReusableWorkflowsSinks.ql Added observeDiffInformedIncrementalMode predicate to enable diff-informed flow.
actions/ql/src/Models/CompositeActionsSummaries.ql Added observeDiffInformedIncrementalMode predicate to enable diff-informed flow.
actions/ql/src/Models/CompositeActionsSources.ql Added observeDiffInformedIncrementalMode predicate to enable diff-informed flow.
actions/ql/src/Models/CompositeActionsSinks.ql Added observeDiffInformedIncrementalMode predicate to enable diff-informed flow.
actions/ql/lib/codeql/actions/security/SecretExfiltrationQuery.qll Added observeDiffInformedIncrementalMode predicate to enable diff-informed flow.
actions/ql/lib/codeql/actions/security/RequestForgeryQuery.qll Added observeDiffInformedIncrementalMode predicate to enable diff-informed flow.
actions/ql/lib/codeql/actions/security/OutputClobberingQuery.qll Added observeDiffInformedIncrementalMode predicate to enable diff-informed flow.
Comments suppressed due to low confidence (2)

actions/ql/src/Models/CompositeActionsSummaries.ql:29

  • Add a brief doc comment above this predicate to explain its role in enabling diff-informed incremental analysis for readers unfamiliar with the feature.
predicate observeDiffInformedIncrementalMode() { any() }

actions/ql/lib/codeql/actions/security/SecretExfiltrationQuery.qll:19

  • No tests were added to validate diff-informed incremental mode. Consider adding unit or integration tests to ensure this predicate actually enables the intended incremental behavior.
predicate observeDiffInformedIncrementalMode() { any() }

@d10c
Copy link
Contributor Author

d10c commented Jun 5, 2025

It turns out that some of the generated changes in the PRs were not correct, e.g. because they should have also generated a getASelected{Source,Sink}Location() override but didn't (see Chuan-kai's comment here). So for now I'm putting them back in Draft until I make sure (via the patch script) that we are correctly handling all 3 documented query patterns, starting with the simplest one (both source and sink are used as location sources). If you have already started reviewing the PRs, thank you (also for your patience) and stay tuned for an update as to what has changed in the meantime!

@d10c d10c marked this pull request as draft June 5, 2025 15:58
@d10c
Copy link
Contributor Author

d10c commented Jun 10, 2025

Update: no changes since last time I opened the PR. It turns out that it's sound (but not optimally performant) to leave getASelected{Source,Sink}Location() un-overridden, specifically in case of a select clause containing only one of source or sink but not both. The patch script currently does not differentiate between that case and the one in which both source and sink are present in the select clause. So I will re-open these PRs as they are, and generate an appropriate getASelected{Source,Sink}Location() override in a follow-up round of PRs.

@d10c d10c marked this pull request as ready for review June 10, 2025 15:05
@d10c d10c added the no-change-note-required This PR does not need a change note label Jun 11, 2025
An auto-generated patch that enables diff-informed data flow in the obvious cases.

Builds on github#18346 and github/codeql-patch#88
@d10c d10c force-pushed the d10c/actions/diff-informed branch from 01f331d to f2bd454 Compare June 11, 2025 17:12
@d10c
Copy link
Contributor Author

d10c commented Jun 13, 2025

Note, according to the follow-up PR, all 9 of these queries have a missing source/sink location in their select clause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Actions Analysis of GitHub Actions no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant