Make actions/unpinned-tag lockfile- and $/-aware - #22155
Open
nodeselector wants to merge 23 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
nodeselector
force-pushed
the
nodeselector-actions-lockfile-aware-pinning
branch
from
August 31, 2026 22:25
059814e to
f1e28c2
Compare
A $/ reference (e.g. "uses: $/path/to/action") is a same-repo self-reference that resolves to the commit the workflow is running at. It is inherently pinned, exactly like a "./" local reference, so it must never be reported by actions/unpinned-tag. Adds an isSelfReference(nwo) guard to the query plus a test fixture covering the bare "$/actions/foo" form and the "$/actions/foo@v1" form (the latter is rejected by the $/ rule but writable by a user; the guard suppresses it either way). Part of github/actions-dispatch#755.
Adds the seam for making actions/unpinned-tag aware of a repository's Actions lockfile (.github/workflows/actions.lock), so that a tag ref bound to a verified commit in the lockfile is not reported as unpinned (Option A from the github#755 spike). Introduces the extensible predicate pinnedByLockfileDataModel(workflow_path, nwo, ref) in ConfigExtensions.qll, re-exported through Config.qll, with a data-extension stub in ext/config/pinned_by_lockfile.yml documenting the intended row shape. The query gains a "not pinnedByLockfile(...)" clause keyed on the workflow file's relative path. The predicate is meant to be populated by the CodeQL Actions extractor, which must parse actions.lock at database-creation time using the canonical parser github.com/github/actions-lockfile/go. That extractor work is a separate change and is not implemented here; until it ships the predicate is empty and the new clause is a no-op. A test-scoped data extension exercises the clause end to end. Part of github/actions-dispatch#755.
Add a Go tool that parses a repository's Actions lockfile (.github/workflows/actions.lock) with the canonical parser at github.com/github/actions-lockfile/go and emits a CodeQL data extension populating pinnedByLockfileDataModel, the predicate the actions/unpinned-tag query already consumes to suppress lockfile-pinned refs. The generator is transport-agnostic: it produces the same [workflow_path, nwo, ref] rows whether they ship as a model pack applied via --model-packs (as today, mirroring codeql/immutable-actions-list) or later feed an extractor-native relation, so the parsing core is reusable without touching the query. Lockfiles record the resolved ref (e.g. v4.3.1) while workflows usually write a shorter mutable tag (v4). Since the query matches the ref as written, the generator expands every full-semver resolved ref into its major.minor and major-only forms, so uses: owner/action@v4 is recognized as pinned by a v4.3.1 lockfile entry. Verified end to end against a synthetic repo: the lockfile-pinned short-tag ref is suppressed while unlocked refs still report. actions-lockfile is not yet public, so go.mod carries a local replace directive for building and testing; remove it once the module is published.
Wire the lockfile-extension-generator into the Actions extractor autobuild so that codeql database create automatically emits the pinnedByLockfileDataModel data extension from a repository's .github/workflows/actions.lock. The extension is written into the database as a self-contained model pack under <db>/lockfile-extension (codeql/actions-lockfile-pins). A new generate-lockfile-extension.sh runs after JS extraction: it locates the lockfile relative to the captured source root, resolves the generator (prebuilt binary if shipped, else builds from source when a Go toolchain is present), and writes the pack. It is a clean no-op when the repository has no lockfile, so it is safe to run against every database. CodeQL does not auto-apply extensions carried inside a database, so analysis still adds the pack explicitly via --model-packs codeql/actions-lockfile-pins (--additional-packs <db>/lockfile-extension). Wiring that into the analysis harness is the remaining step and lives outside this repo. Verified end to end locally by overlaying the modified extractor into the CLI bundle: database create emits the extension, and analyze suppresses a lockfile-pinned short-tag ref (uses: owner/action@v4 resolved to v4.3.1) while still reporting refs not covered by the lockfile.
…imits The committed go.mod for the lockfile-extension generator carried an absolute-path replace directive pointing at a local clone of the private actions-lockfile repo, which would leak a developer path and break builds for anyone else. Keep the how-to-test comment but drop the replace line; local testing uses `go mod edit -replace`. Also update the two change notes to state that the extractor now generates the pinnedByLockfileDataModel data into a database-local model pack (applied via --model-packs), and document the composite-action completeness gap in the generator README.
The previous commit re-staged a dirty working-tree go.mod, so the machine-specific replace directive pointing at a local actions-lockfile clone leaked back into the tree. Drop it for real and move the local replace into a gitignored go.work so committed module metadata stays portable while local builds still resolve the not-yet-public dependency.
Canonical terminology flip: `$/` resolves to the same REPOSITORY at the
running SHA ("self repository"), while `./` is "self workspace". Rename the
isSelfReference predicate to isSelfRepository, reword the code comment, and
update the change note (renamed to ...-self-repository.md) and test fixture
comments to match. No change to query results, the finding message, or any
.expected output.
Stage the generated model pack in a temp dir inside the WIP database and publish it with a single rename only after it is fully written, with an EXIT trap that cleans up on any failure. Previously a failed 'go build' (expected until the private actions-lockfile dependency is public) left a half-written pack dir behind (an ext/ with no qlpack.yml) that could break analyses run with --additional-packs. Verified across three cases: repo with a lockfile (atomic publish), repo without one (clean no-op), and no Go toolchain available (graceful skip, no partial pack).
The generator pulled in github.com/github/actions-lockfile/go purely to parse a small, stable YAML file, which meant it could not build without a local clone of that (currently private) module -- forcing a gitignored go.work with a machine-specific replace and breaking any CI/bazel build. Parse the minimal core of the lockfile format directly instead (new lockfile.go: YAML unmarshal, pin-key parsing, and the semver major/minor/full logic), faithfully mirroring the canonical parser's semantics. The golden fixture (testdata/expected.yml) is unchanged, byte for byte, which proves the reimplementation matches. Added unit tests for parsePin, parseSemVer/isFull, and parseLockfile. The generator now depends only on gopkg.in/yaml.v3 and builds anywhere the Go toolchain is available, with no replace directive and no go.work. Verified end-to-end through the real extractor: on-demand 'go build' during database create succeeds with a stock toolchain, and the lockfile-pinned ref is still suppressed while an unlocked ref still fires.
A lockfile that pins no repo-level actions (e.g. only sub-path actions like github/codeql-action/init@v3, which parsePin skips) produced a bare `data:` (YAML null) extension, which CodeQL's `resolve extensions-by-pack` rejects and aborts the analysis. Emit `data: []` for the zero-row case, matching the repo convention, and note the narrow transitive-per-path over-suppression edge in the change note.
…/repo The lockfile generator lower-cases owner/repo (GitHub treats them case-insensitively) while preserving the ref, so a mixed-case ref such as `uses: Azure/login@v1` never matched the lowercase pinnedByLockfileDataModel row and was still reported as unpinned. Lower-case nwo only for the data-model lookup, keeping source casing in the alert message. Adds a mixed-case suppression test plus a mixed-case control that must still fire.
behaviour -> behavior, recognised -> recognized, serialises -> serializes across the lockfile-aware pinning comments and change notes. Comments/docs only; no logic change.
Removes the Go lockfile-extension-generator tool and its extractor autobuild hook, keeping this PR to its intended discussion scope: the $/ self-repository suppression plus the QL seam (pinnedByLockfileDataModel extensible predicate and the not pinnedByLockfile(...) clause). Parsing actions.lock and populating the predicate is extractor-team work on the CodeQL CLI cycle, and the tool here reimplemented the canonical parser at github.com/github/actions-lockfile rather than depending on it, so it would drift. It also wrote a model pack into the database that CodeQL does not auto-apply, so it never took effect without extra analysis flags. Better to scaffold the seam and let the extractor own population. Reverts the additions to actions/extractor/tools/autobuild.sh and deletes actions/extractor/tools/generate-lockfile-extension.sh and actions/extractor/tools/lockfile-extension-generator/. Query behavior and test expected output are unchanged; the CWE-829 query tests still pass.
nodeselector
force-pushed
the
nodeselector-actions-lockfile-aware-pinning
branch
from
September 2, 2026 14:34
f1e28c2 to
75cbb24
Compare
hvitved
reviewed
Sep 2, 2026
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Lock matching mishandles valid mixed-case pins and accepts entries missing required repository identities.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
actions/ql/lib/codeql/actions/Lock.qll — The dependency is accepted without the required owner_id and repo_id fields, so a lock entry… |
 |
actions/ql/lib/codeql/actions/Lock.qll — This equality does not normalize pinnedNwo; it requires the lockfile spelling to already be… |
What changed in this PR
Makes actions/unpinned-tag aware of lockfile-backed pins and self-repository references.
Changes:
- Adds workflow-scoped
actions.lockpin detection. - Excludes
$/references from unpinned-tag findings. - Adds focused query fixtures and change notes.
| File | Description |
|---|---|
actions/ql/lib/codeql/actions/Lock.qll |
Implements lockfile pin matching. |
actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql |
Suppresses locked and self-repository references. |
actions/ql/test/utils/ActionsInlineExpectationsTestQuery.ql |
Adds YAML inline-expectation processing. |
actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected |
Updates expected data-flow edges. |
actions/ql/test/query-tests/Security/CWE-829/.github/workflows/self_ref_dollar.yml |
Tests $/ references. |
actions/ql/test/query-tests/Security/CWE-829-Lockfile/UnpinnedActionsTag.qlref |
Configures the lockfile query test. |
actions/ql/test/query-tests/Security/CWE-829-Lockfile/UnpinnedActionsTag.expected |
Records expected findings. |
actions/ql/test/query-tests/Security/CWE-829-Lockfile/options |
Enables lockfile YAML extraction. |
actions/ql/test/query-tests/Security/CWE-829-Lockfile/.github/workflows/rust-ci.yml |
Provides workflow test cases. |
actions/ql/test/query-tests/Security/CWE-829-Lockfile/.github/workflows/actions.lock |
Provides lockfile test data. |
actions/ql/src/change-notes/2026-07-09-unpinned-tag-self-repository.md |
Documents self-reference handling. |
actions/ql/src/change-notes/2026-07-09-unpinned-tag-lockfile-aware.md |
Documents lockfile awareness. |
actions/ql/lib/change-notes/2026-09-01-actions-lock-yaml.md |
Updates the ActionsLock API note. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
hvitved
requested changes
Sep 3, 2026
hvitved
left a comment
hvitved
left a comment
Contributor
There was a problem hiding this comment.
I have created nodeselector#1 against your branch with code review suggestions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
actions/unpinned-tagreports symbolic action refs even when Actions resolves them to the commit recorded for that workflow in.github/workflows/actions.lock. It also reports$/self-repository references, which resolve at the running commit.This removes both false positives. See #22464.
What changed
ActionsLock.owner/repo@refpin and the matching dependency has a full SHA-1 or SHA-256 commit.actions/cache/saveas pins onactions/cache.$/references from this query because they resolve at the running repository commit.CodeQL trusts the checked-in lockfile syntax here. It does not call GitHub, verify provenance or validate the transitive dependency graph. Reusable workflow findings are unchanged.
Validation
Risk and rollback
Low. The new suppression requires a workflow-scoped pin, matching dependency ref and full commit digest. Reverting this PR restores the previous findings.