Skip to content

Go: mass enable diff-informed data flow - #19660

Merged
d10c merged 1 commit into
github:mainfrom
d10c:d10c/go/diff-informed
Jun 17, 2025
Merged

Go: mass enable diff-informed data flow#19660
d10c merged 1 commit into
github:mainfrom
d10c:d10c/go/diff-informed

Conversation

@d10c

@d10cd10c commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

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

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

@d10c
d10c marked this pull request as ready for review June 4, 2025 11:32
CopilotAI review requested due to automatic review settings June 4, 2025 11:32
@d10c
d10c requested a review from a team as a code ownerJune 4, 2025 11:32

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

Enables diff-informed data flow tracking in Go QL security queries by adding the required predicate to each DataFlow configuration.

  • Adds observeDiffInformedIncrementalMode() returning any() to all DataFlow::ConfigSig modules
  • Ensures incremental, diff-based analysis is enabled for each relevant security check

Reviewed Changes

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

Show a summary per file
FileDescription
go/ql/src/Security/CWE-352/ConstantOauth2State.qlAdd observeDiffInformedIncrementalMode predicate
go/ql/src/Security/CWE-326/InsufficientKeySize.qlAdd observeDiffInformedIncrementalMode predicate
go/ql/src/Security/CWE-209/StackTraceExposure.qlAdd observeDiffInformedIncrementalMode predicate
go/ql/src/Security/CWE-020/SuspiciousCharacterInRegexp.qlAdd observeDiffInformedIncrementalMode predicate
go/ql/src/Security/CWE-020/MissingRegexpAnchor.qlAdd observeDiffInformedIncrementalMode predicate
go/ql/src/Security/CWE-020/IncompleteHostnameRegexp.qlAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/ZipSlip.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/XPathInjection.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/UnsafeUnzipSymlink.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/UncontrolledAllocationSize.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/TaintedPath.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/StringBreak.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/StoredXss.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/StoredCommand.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/SqlInjection.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/OpenUrlRedirect.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/MissingJwtSignatureCheck.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/LogInjection.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/ExternalAPIs.qllAdd observeDiffInformedIncrementalMode predicate
go/ql/lib/semmle/go/security/CleartextLogging.qllAdd observeDiffInformedIncrementalMode predicate
Comments suppressed due to low confidence (1)

go/ql/src/Security/CWE-352/ConstantOauth2State.ql:44

  • No tests were added to verify the new observeDiffInformedIncrementalMode predicate. Include tests to confirm that diff-informed incremental data flow is actually activated.
predicate observeDiffInformedIncrementalMode() { any() }

Comment threadgo/ql/src/Security/CWE-352/ConstantOauth2State.ql
@d10c

d10c commented Jun 5, 2025

Copy link
Copy Markdown
ContributorAuthor

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:59
@d10c

d10c commented Jun 10, 2025

Copy link
Copy Markdown
ContributorAuthor

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 11, 2025 09:35
@d10cd10c 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#18345 and github/codeql-patch#88
@d10c

d10c commented Jun 13, 2025

Copy link
Copy Markdown
ContributorAuthor

Note, according to the follow-up PR, 5 of these queries (MissingRegexpAnchor.ql, HtmlTemplateEscapingBypassXss.ql, InsufficientKeySize.ql, PamAuthBypass.ql, DivideByZero.ql) have a missing source/sink in their select clauses; the other ones should have both.

@michaelnebelmichaelnebel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@d10c
d10c merged commit a8dc300 into github:mainJun 17, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Gono-change-note-requiredThis PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@d10c@michaelnebel