Skip to content

Harden the action-pin audit against uses-syntax, docker and case evasions - #10264

Merged
Evangelink merged 13 commits into
mainfrom
dev/amauryleve/dev-amauryleve-check-action-pins
Jul 28, 2026
Merged

Harden the action-pin audit against uses-syntax, docker and case evasions#10264
Evangelink merged 13 commits into
mainfrom
dev/amauryleve/dev-amauryleve-check-action-pins

Conversation

@Evangelink

@Evangelink Evangelink commented Jul 27, 2026

Copy link
Copy Markdown
Member

Note: #10266 landed an equivalent audit while this PR was in review. This PR has been rebased onto it and now hardens the shipped gate rather than introducing a new one. Its scope changed after approval, so it needs a fresh look.

What this adds

main's audit (from #10266) is kept wholesale — rule structure (R1–R3), generated-file classification, .github/actions/ scanning, --list, and fail-closed actions-lock.json handling are all unchanged. Layered on top are four evasions that let an unpinned or drifted action pass the shipped gate.

Each was verified against a clean checkout of main, inside a generated .lock.yml where R1 applies, using a plain unpinned reference as the control:

Evasion main This PR
"uses": quoted key ❌ passes ✅ caught
uses : whitespace before colon ❌ passes ✅ caught
uses: >- block scalar / {uses: …} flow mapping / alias ❌ passes ✅ caught
docker://alpine:latest ❌ passes ✅ caught
Actions/Checkout@<any SHA> ❌ passes ✅ caught
plain unpinned (control) ✅ caught ✅ caught

The control matters: it confirms these are genuine evasions of R1, not the deliberate allowance of mutable tags in hand-authored files.

How

  • R4 — container actions. docker:// was skipped entirely, so a mutable image tag passed while an equally mutable action tag failed. Now requires docker://<image>@sha256:<64-hex>, the only immutable identifier for an image.
  • R5 — structural reconciliation. Block scalars, flow mappings and aliases execute but are invisible to any line scan, so widening the regex again would only move the goalposts. Each workflow is now parsed with PyYAML and every reachable uses reconciled per occurrence against the line scan; anything the scan cannot see fails. The generated # Custom actions used: header is excluded from the scanned side, so a comment cannot vouch for a hidden step.
  • Canonical comparison. Owner/repo were compared verbatim, so Actions/Checkout looked untracked and escaped R2 at any SHA. Names are now canonicalized (subpaths keep their case — they're repository paths, and Linux runners are case-sensitive); SHAs and digests are hex, so those are compared case-insensitively too.
  • Line pattern. The uses key now matches optional quotes and whitespace before the colon, as loosely as YAML parses it.

PyYAML is a new dependency, hash-pinned in check-action-pins-requirements.txt and installed identically in CI. A missing import exits non-zero rather than degrading to a line-only scan — an unpinned install would be a supply-chain hole in the job that validates our pins.

Drift still present in main, fixed here

  • peter-evans/create-pull-request labelled # v7 on the v8.1.1 SHA in copilot-curate-update.yml and update-bundled-playwright.yml (and backport-base.yml). SHAs unchanged; only the misleading comments corrected, verified against the upstream tag API.
  • Three mutable tags in dedup-analysis.yml (actions/setup-node@v7, actions/upload-artifact@v7 ×2), pinned to the SHAs already used repo-wide.

Known limitation (documented, not fixed)

Collection is position-independent: a uses: nested under with:, or inside a run: block, is data but is still collected. It fails closed and affects no file in this repo. Restricting to jobs.*.uses would swap a conservative over-collection for an inclusion list that fails open. A complete fix drives the audit from the parsed document with line marks (yaml.compose) and excludes known data mappings; that's a rewrite of the core and belongs in its own PR. Recorded in the script docstring with the rationale — see the open thread on this PR.

Validation

  • Audit passes on the merged tree: 2076 references across 45 files, no false positives.
  • Nine-case battery (six evasions + three must-still-pass) — zero unexpected results.
  • Merge is scoped to .github/ only; no .xlf, eng/, or src/ changes ride along.

Local `gh aw compile` silently rewrites two security-relevant pins in the
generated `.lock.yml` files: it downgrades `actions/checkout` from the
repo-aligned v7.0.1 to v7.0.0 and un-pins `github/gh-aw-actions/setup` to the
mutable `@v0.83.1` tag. Both land in generated files reviewers rarely read
line-by-line, and the `compiler_version` header does not reveal it because it
is an identity claim rather than a statement about the emitted bytes.

Add `check_action_pins.py` plus a `check-action-pins.yml` PR gate that verifies
every `uses:` reference under `.github/workflows` is pinned to a 40-character
SHA, carries a version comment, agrees with `.github/aw/actions-lock.json`,
resolves to a single SHA repo-wide, and matches the `gh-aw-manifest` header.

The new gate surfaced pre-existing drift, fixed here:

- `dedup-analysis.yml` used mutable `actions/setup-node@v7` and
  `actions/upload-artifact@v7` tags.
- `backport-base.yml` labelled the `actions/github-script` v9.0.0 SHA as `# v7`
  and the `peter-evans/create-pull-request` v8.1.1 SHA as `# v7`.

Fixes #10258

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 16:15

Copilot AI 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

Adds CI validation to prevent GitHub Actions pin drift caused by misaligned gh aw tooling.

Changes:

  • Adds a Python pin-validation script and CI workflow.
  • Corrects existing mutable or mislabeled action references.
  • Documents pinning and regeneration requirements.
Show a summary per file
File Description
.github/scripts/check_action_pins.py Validates workflow references and manifests against the pin ledger.
.github/workflows/check-action-pins.yml Runs pin validation in CI.
.github/workflows/dedup-analysis.yml Replaces mutable action tags with SHAs.
.github/workflows/backport-base.yml Corrects action version comments.
.github/workflows/README.md Documents action-pinning policy and workflow.
.github/copilot-instructions.md Requires pin validation after lock regeneration.

Review details

  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread .github/scripts/check_action_pins.py Outdated
@Evangelink
Evangelink enabled auto-merge (squash) July 27, 2026 17:29
The SHA-vs-ledger comparison was only a warning, so a tracked action could be
repointed at any 40-character SHA — consistently across every call site, keeping
the ledger's version comment — and the gate still exited 0. The cross-workflow
consistency check does not establish that a SHA is approved, and hand-authored
workflows have no gh-aw-manifest to back it up.

Record the one legitimate divergence explicitly instead: `gh aw` stores an
annotated tag-object SHA in the ledger for github/codeql-action while emitting
the dereferenced commit SHA in `uses:` lines, so DEREFERENCED_TAG_SHAS maps that
tag object to its commit. Every other SHA mismatch is now an error.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 17:32

Copilot AI 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.

Review details

  • Files reviewed: 6/6 changed files
  • Comments generated: 3
  • Review effort level: Medium

Comment thread .github/scripts/check_action_pins.py Outdated
Comment thread .github/workflows/backport-base.yml
Comment thread .github/scripts/check_action_pins.py Outdated
Review found three ways a bad pin could pass the gate:

- The `uses:` pattern only matched an unquoted key with no space before the
  colon. YAML also accepts `"uses":`, `'uses':` and `uses :`, all of which still
  execute, so an unpinned action could slip through by being written differently.
- Check B accepted any non-empty comment token as a version, so `# TODO` was
  treated as a valid `# <version>` comment. It now has to look like a version.
- Correcting the mislabelled `peter-evans/create-pull-request` pin in
  backport-base.yml left the same SHA labelled `# v7` in copilot-curate-update.yml
  and update-bundled-playwright.yml, and nothing detected the disagreement.

Fix the two stale labels and add check G, which requires a given SHA to carry the
same version label everywhere. For actions the gh-aw ledger does not track, that
is the only thing keeping their labels honest, and it prevents this exact class
of half-finished correction from recurring.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 17:40

Copilot AI 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.

Review details

  • Files reviewed: 8/8 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread .github/scripts/check_action_pins.py Outdated
Comment thread .github/workflows/README.md Outdated
The line-based scanner could still be evaded. `uses: >-` with the value on a
continuation line, `{uses: ...}` flow mappings, and anchors/aliases all parse to
a real `uses` string that GitHub Actions executes, but the scanner saw only the
first line and skipped it, so an unpinned action passed CI. Broadening the regex
again would just move the goalposts.

Parse each workflow with PyYAML and walk the tree for every reachable `uses`
value, then reconcile that against the text pass. The text pass still owns
version comments and the `# Custom actions used:` inventory, since a structural
parse discards comments. Anything reachable in the parsed document but not
written as a plain scannable line is now a hard failure telling the author to
rewrite it, so the gate is closed by construction rather than by pattern count.

PyYAML is hash-pinned in a dedicated requirements file, and a missing import
exits non-zero instead of degrading to a text-only scan.

Also correct the README, which claimed every `uses:` in the directory must be
SHA-pinned. The agentic `*.md` sources legitimately reference versions and are
not scanned, so the rule is now scoped to executable workflow YAML.

Verified: block scalars (`>-`, `|`), flow mappings, anchor/alias pairs, quoted
keys, whitespace before the colon, and a folded-but-pinned reference are all
rejected, while the real tree still passes at 2076 references across 45 files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 17:52
@Evangelink Evangelink added the state/needs-review Awaiting review from the team. label Jul 27, 2026

Copilot AI 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.

Review details

Comments suppressed due to low confidence (1)

.github/scripts/check_action_pins.py:368

  • Using sets here loses occurrence counts and lets unsupported syntax pass whenever the same reference also appears on one plain line in the file. For example, a normal uses: owner/action@<sha> # v1 plus { uses: owner/action@<sha> } produces equal sets, so the flow-mapping occurrence is not rejected despite lacking its own version comment. Reconcile each parsed occurrence one-to-one against USES_RE matches (excluding inventory-only LISTED_RE matches) so every executable occurrence is independently validated.
        scanned = {f"{reference.repo}@{reference.ref}" for reference in file_references}
        for value in sorted(structural - scanned):
  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread .github/scripts/check_action_pins.py Outdated
`docker://` was excluded from the gate as if it were a container image rather
than executable code, so `uses: docker://alpine:latest` passed while an equally
mutable `actions/checkout@v7` failed. A container action runs arbitrary code
exactly like a repository action, so the exclusion contradicted the documented
invariant that every executable `uses:` reference is immutable.

Validate them instead of skipping them: repository actions still require a
40-character commit SHA, and container actions now require an image digest,
which is the only immutable identifier for an image.

Reference now carries the raw `uses` value, so the structural reconciliation
compares exact strings rather than reassembling `repo@ref`, which would not have
round-tripped a tagged image that has no `@`.

Verified: mutable tag, bare image, registry-qualified tag, a truncated digest,
and a container action hidden in a block scalar are all rejected, while a
correctly digest-pinned image passes. The nine earlier evasion cases still fail
and the real tree still passes at 2076 references across 45 files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 18:03

Copilot AI 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.

Review details

  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread .github/scripts/check_action_pins.py Outdated
GitHub resolves action owner and repository names case-insensitively, but every
lookup here used the spelling verbatim. `Actions/Checkout@<arbitrary SHA> #
v7.0.1` therefore looked like an untracked action, skipped the ledger version and
SHA checks entirely, and passed the gate while executing the same repository as
`actions/checkout`. Confirmed before fixing.

Canonicalize the owner and repository segments when loading the ledger, grouping
references, and checking manifests. Action subpaths keep their original case
because they are repository paths and Linux runners are case-sensitive. SHAs and
digests are hex, so those are lowercased too; without that an uppercase but
perfectly valid pin was rejected with a misleading "not pinned" message.

Verified: a case-variant with an arbitrary SHA and a case-variant with a stale
version label are both rejected, while a case-variant with the correct pin and an
uppercase-but-correct SHA pass. The nine earlier evasion cases and the docker
digest cases still behave as before, and the real tree passes at 2076 references
across 45 files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 18:13

Copilot AI 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.

Review details

  • Files reviewed: 9/9 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread .github/scripts/check_action_pins.py Outdated
Comment thread .github/scripts/check_action_pins.py Outdated
Two review findings, both confirmed before fixing.

Check D grouped by the full action locator, so `owner/repo/action-a@<sha1>` and
`owner/repo/action-b@<sha2>` formed separate groups and passed even though they
come from one repository checkout and must agree. That check is the only
cross-reference available for actions the ledger does not track, so the gap
mattered most exactly where the fallback is relied on. Consistency (checks D and
G) now groups by the canonical `owner/repo` root, while ledger lookups keep the
full locator, which is how the ledger keys entries such as
`actions/cache/restore`.

The missing-PyYAML error also suggested an unpinned `pip install pyyaml`, which
undercut the hash-pinned requirements file added to protect this gate. It now
points at the same --require-hashes command CI runs.

Verified: an untracked repository with two subpaths on divergent SHAs is now
rejected, the same repository with both subpaths on one SHA still passes, and
ledger lookups still resolve subpath entries. The twelve earlier evasion and
false-positive cases are unchanged, and the real tree passes at 2076 references
across 45 files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 18:25

Copilot AI 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.

Review details

Comments suppressed due to low confidence (1)

.github/scripts/check_action_pins.py:462

  • The structural/text reconciliation loses occurrence information by converting both sides to sets, and scanned also includes the generated # Custom actions used: inventory. In a lock file, a flow-mapped or aliased uses that resolves to an action already present in that inventory (or on any other plain line) is therefore removed by structural - scanned and passes, despite the documented guarantee that these forms fail. Keep executable USES_RE matches separate from LISTED_RE inventory entries and reconcile occurrence counts/locations rather than distinct values.
        scanned = {reference.raw for reference in file_references}
        for value in sorted(structural - scanned):
  • Files reviewed: 9/9 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

The structural/text reconciliation compared sets, so an occurrence written in an
unscannable form was excused whenever the same value also appeared on a plain
line, or merely in the generated `# Custom actions used:` inventory. A valid
`uses: actions/checkout@<sha> # v7.0.1` next to `{uses: actions/checkout@<sha>}`
produced equal sets and passed, contradicting the documented guarantee that these
forms fail, and letting the hidden occurrence skip its own version comment.

There is no pin-integrity breach in that case, since the hidden occurrence
duplicates an already-validated immutable pin, but it defeats the property the
check exists to provide: that nothing executable escapes the text pass.

Count occurrences instead, and exclude the inventory from the scanned side since
a comment must not vouch for an executable step. Raised as a suppressed
low-confidence review note; verified as a real gap before fixing.

Verified: a duplicate value hidden in a flow mapping or block scalar is now
rejected, and the count is reported, while the same action legitimately used
twice on plain lines still passes. All fourteen evasion and false-positive cases
behave as expected and the real tree passes at 2076 references across 45 files,
which also confirms excluding the inventory introduced no false positives in the
generated lock files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 18:37

Copilot AI 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.

Review details

Comments suppressed due to low confidence (1)

.github/workflows/README.md:254

  • This restatement excludes the docker:// digest form that the new policy explicitly permits above, so it incorrectly says every executable uses: value needs a commit SHA and version comment. Keep the convention consistent with the Action pinning section.
- **Pinned actions only.** Every `uses:` reference in the executable workflow YAML is pinned to an immutable SHA with a `# <version>` comment matching [`.github/aw/actions-lock.json`](../aw/actions-lock.json). Strict mode pins generated references, and [`check-action-pins.yml`](./check-action-pins.yml) verifies the result — see [Action pinning](#action-pinning).
  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread .github/scripts/check_action_pins.py Outdated
Review correctly observed that any mapping key named `uses` is treated as
executable, so an action input or environment variable literally named `uses`
whose value looks like an action reference is collected and reported as unpinned.
Verified: a `uses:` nested under `with:` is rejected today.

Not fixing the behaviour here, and recording why rather than leaving it as an
undocumented sharp edge. It fails closed, and no such key exists in this
repository. The suggested remedy, restricting collection to `jobs.*.uses` and
`jobs.*.steps[*].uses`, swaps a conservative over-collection for an inclusion
list that fails open if the workflow schema ever grows another executable `uses`
position; five earlier rounds of this review were all under-collection holes, so
trading a fail-closed property for a fail-open one is the wrong direction to take
unsupervised. A correct fix drives validation from the parsed document with line
marks and excludes known data mappings, keeping unknown structures fail-closed,
but that rewrites the core of the gate and belongs in its own change.

The review thread is left open for a human to decide.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 18:46

Copilot AI 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.

Review details

Comments suppressed due to low confidence (1)

.github/scripts/check_action_pins.py:497

  • scanned includes any regex-matching line, even lines inside a block scalar, while structural includes only parsed uses keys. Because reconciliation only checks structural_count - scanned_count, a non-executable line in a run: | heredoc can offset an executable flow-mapping/block-scalar uses with the same value. The hidden occurrence then passes without being written on the required plain, version-commented line. Reconcile source locations from the parsed YAML (as suggested by the existing known limitation), or at minimum reject unmatched scanner-only occurrences so they cannot vouch for executable ones.
        scanned = Counter(
            reference.raw for reference in file_references if not reference.from_inventory
        )
  • Files reviewed: 9/9 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

…tion

Two suppressed low-confidence review notes, both verified.

The Conventions bullet still said every `uses:` needs a commit SHA and version
comment, which contradicts the Action pinning section directly above it now that
`docker://` container actions are pinned by image digest instead. Reworded to
match.

A literal `uses:` line inside a `run:` block is also collected by the text pass,
so it is reported as unpinned, and because reconciliation compares counts per
value it can offset an executable occurrence of the same value hidden in a flow
mapping. Confirmed both. This is the same root cause as the open thread on
position-independent collection: the text pass is context-free and cannot tell an
executable step from data. Neither breaches pin integrity, since every value
involved has itself been validated as an immutable pin.

Not fixing it here for the reason already recorded on that thread, and the
reviewer's fallback of rejecting scanner-only occurrences would reject legitimate
`run:` blocks, trading one false positive for another. The known-limitation note
now covers this facet too, so the deferred work is described in one place.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 27, 2026 18:54

Copilot AI 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.

Review details

  • Files reviewed: 9/9 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

PR #10266 landed an equivalent audit while this branch was in review, so the two
implementations collided on `.github/scripts/check_action_pins.py`. Main's version
is authoritative and is kept wholesale: its rule structure (R1-R3), generated-file
classification, `.github/actions/` scanning, `--list` table, and fail-closed
handling of `actions-lock.json` are all preserved unchanged.

Layered onto it are the evasions this branch had already found and fixed, each
re-verified against a clean checkout of main inside a generated `.lock.yml` where
R1 applies, using a plain unpinned reference as the control:

  - A quoted `"uses":` key and `uses :` with whitespace before the colon both
    parse as `uses` and execute, but did not match the line pattern.
  - `uses: >-` block scalars, `{uses: ...}` flow mappings and anchor/alias pairs
    execute but are invisible to any line scan. R5 now parses each workflow and
    reconciles per occurrence against the scan.
  - `docker://` was skipped entirely, so `docker://alpine:latest` passed while an
    equally mutable action tag failed. R4 now requires an image digest.
  - Owner and repository names were compared verbatim, so `Actions/Checkout`
    looked untracked and escaped R2 at any SHA. Comparisons are now canonical.

PyYAML is a new dependency, hash-pinned in its own requirements file and
installed the same way in CI; a missing import exits non-zero rather than
silently degrading to a line-only scan.

Also carried over: three `peter-evans/create-pull-request` references labelled
`# v7` on the v8.1.1 SHA, and three mutable tags in dedup-analysis.yml.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 28, 2026 07:51
@Evangelink Evangelink changed the title Gate workflow action pins against the gh-aw ledger Harden the action-pin audit against uses-syntax, docker and case evasions Jul 28, 2026

Copilot AI 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.

Review details

Comments suppressed due to low confidence (2)

.github/scripts/check_action_pins.py:363

  • R5 filters the parsed values through the same narrow recognizer used by the line scan. A valid remote action whose subpath contains a Git path character outside [A-Za-z0-9._-] (for example, owner/repo/action+name@v1) makes split_ref() return None here; the text collector also skips it, so even a mutable reference in a generated workflow is absent from every rule. Count every parsed non-local/non-expression uses value, then either recognize it for auditing or fail closed as unsupported syntax.
        if split_ref(value) is not None or split_docker_ref(value) is not None

.github/scripts/check_action_pins.py:431

  • R3 still groups by the full action locator because canonical_repo() preserves subpaths. Consequently, two actions from one untracked repository—such as owner/repo/action-a@<sha1> and owner/repo/action-b@<sha2>—land in separate groups and the advertised single-SHA-per-repository check passes. Group this consistency check by the canonical owner/repo root while retaining the full locator for the R2 lock lookup.
        if reference.is_sha_pinned and not reference.is_docker:
            by_repo[reference.repo][reference.ref].append(reference)
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

Two suppressed low-confidence review notes, both verified.

R3 grouped by the full action locator, so `owner/repo/action-a@<sha1>` and
`owner/repo/action-b@<sha2>` formed separate groups and passed even though they
come from one repository checkout. This was fixed on this branch before the merge
and lost when main's implementation was taken wholesale, so it is a regression I
introduced rather than a gap in main. R3 now groups by the canonical `owner/repo`
root while R2 keeps the full locator, which is how the lock keys subpath actions
such as `actions/cache/restore`.

R5 also filtered parsed values through the same narrow recognizer used by the line
scan, so a reference the recognizer could not parse -- for example a subpath
containing a character outside `[A-Za-z0-9._-]` -- was skipped by the scan *and*
filtered out of the structural side, leaving it exempt from every rule even in a
generated workflow. That is the one direction this audit must never fail. R5 now
counts every non-local, non-expression `uses` and reports an unparsable form as
unsupported, with a message distinct from the hidden-syntax one.

Verified: a mutable `owner/repo/action+name@v1` in a generated file and an
untracked repository with two subpaths on divergent SHAs are both rejected, while
two subpaths sharing one SHA still pass. The full thirteen-case battery is clean
and the tree still audits at 2076 references across 45 files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 28, 2026 08:04

Copilot AI 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.

Review details

  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

Collection was position-independent: any mapping key named `uses` counted as
executable, and the line scan matched any `uses:`-shaped line. An action input
named `uses` under `with:`, or a literal `uses:` line inside a `run:` block, was
therefore audited as if it were a step, and because R5 reconciled the two passes
by counting values, a non-executable line could offset a hidden executable one.

Collection now walks the `yaml.compose` node tree and uses each node's line marks
to read the trailing version label from the source. Strings that merely contain
`uses:` are never mapping keys, so the `run:` case disappears by construction,
and R5 validates each occurrence directly instead of reconciling counts.

Deliberately an exclusion list, not an inclusion list. Restricting collection to
`jobs.*.uses` and `jobs.*.steps[*].uses` as suggested would fail open if the
enumeration were wrong or the Actions schema grew another executable position.
Every `uses` key is audited unless nested below a known data container (`with`,
`env`, `secrets`, `inputs`, `outputs`, `defaults`), so unknown structures stay in
scope. Verified that a job-level reusable-workflow `uses` is still audited, which
is exactly the position an inclusion list would have had to enumerate.

Generated-header references stay text-scanned, since YAML cannot see comments.
The `from_header` flag is dropped: it only existed to exclude those references
from the count-based reconciliation that no longer exists, and leaving a field
that implies behaviour the code no longer has is a maintenance hazard.

Verified: the thirteen-case battery is unchanged, `uses` under `with:`, `env:`,
`secrets:` and inside `run: |` are no longer flagged, and a real unpinned step
alongside such data is still caught. Clean tree audits at 2076 references across
45 files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c96caa1-8d43-4661-8baa-68a5b8699a81
Copilot AI review requested due to automatic review settings July 28, 2026 08:25
@Evangelink
Evangelink merged commit 27b51f6 into main Jul 28, 2026
22 checks passed
@Evangelink
Evangelink deleted the dev/amauryleve/dev-amauryleve-check-action-pins branch July 28, 2026 08:25

Copilot AI 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.

Review details

Comments suppressed due to low confidence (1)

.github/scripts/check_action_pins.py:535

  • The PR description says R1–R3 are unchanged, but this changes R3 from grouping by the full action locator to grouping by the repository root. It also still labels the yaml.compose exclusion-list rewrite as a known limitation that was not fixed, although the current diff implements it. Please update the description so the requested fresh review accurately reflects the final security behavior and scope.
    # R3: every action must resolve to a single SHA repo-wide. Grouping is by the
    # `owner/repo` root rather than the full locator: actions sharing a repository
    # share a checkout, so `owner/repo/action-a` and `owner/repo/action-b` must agree.
    # This is also the only cross-check available for actions absent from the lock.
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state/needs-review Awaiting review from the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants