fix(ci): audit the whole call tree in the no-caching gate - #861
Conversation
`scripts/lint-no-workflow-caching.mjs` keeps the GitHub Actions cache out of the two credential-bearing workflows, because a poisoned cache entry would execute in a job that can publish to npm. It had three fail-open holes, each of which printed `OK`. **1. It stopped at a local composite action.** The gate read a step's own `uses:` and went no further, so a workflow could reach `actions/cache` through one indirection — `uses: ./.github/actions/x` — and stay green. Reproduced against a copy of release.yml with a cache-restoring composite spliced in: exit 0, no output, while the composite it never opened restored two caches. It now flattens local composites and checks every step inside them, naming the whole trail so the report points at the file the cache is actually in. **2. It skipped a job that delegates to a reusable workflow.** Such a job has no `steps:` at all — it is `jobs.<id>.uses` — so the walker was handed an empty list and skipped the job entire. Confirmed before the fix against a caller whose only job was `uses: ./.github/workflows/reusable.yml` with `secrets: inherit`, the called workflow holding `actions/cache@v4`: exit 0, `OK`, nothing scanned. The verdict deliberately ignores `secrets:` — `permissions:` is inherited independently and is what mints the OIDC token npm trusted publishing signs with, so a call passing no secrets can still publish. **3. A third-party cache action was invisible.** The rules only recognised an action literally named `actions/cache*` and one taking a `cache:` input, so `useblacksmith/cache@v5` and `Swatinem/rust-cache@v2` both passed. The repair is an inversion rather than a longer denylist: every REMOTE `uses:` reachable from a targeted workflow must now appear in an `AUDITED_ACTIONS` allowlist, so an action this gate has never seen is a finding by default whatever it is called. A denylist fails open on the action nobody has met yet — silently correct until the day it is silently wrong, and wrong in the direction that prints `OK`. It also cannot cover the class most likely to be added by accident: a `setup-<tool>` action that caches BY DEFAULT, with no `cache:` input to inspect and no "cache" in its name. The allowlist cannot go stale silently, which is why it was chosen: its staleness is a build failure naming the exact action and the file it was added to, so the person adding it is the person told to audit it, in the same PR. Cost was measured rather than assumed — the two targeted workflows reach four actions between them. Also adds the missing premise assertion to `integration-workflow-paths`: its requirement set is DERIVED from `@/`-aliased imports, so "no suite uses that alias" and "every import is covered" were the same green. Mutation-tested — rewriting the suites onto the public entry empties the set, and the check then passed with `packages/stack/src/dynamodb/**` deleted from the filter, which is verbatim the #815 gap the file exists to prevent. Second of four stacked PRs splitting the protect-ffi absorption. Independent of the vendoring: these are pre-existing holes in a control that already shipped.
🦋 Changeset detectedLatest commit: 05f84bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Verdict: Approve. Verified locally: pnpm run test:scripts 295/295 pass, pnpm run lint:workflow-cache exits 0 on the real release.yml / tests-supply-chain.yml, and biome check on the three changed source files is clean. The core traversal is correct across every shape the fixtures exercise, the fail-closed allowlist is the right design, and the changeset + stash-supply-chain-security skill are both updated as AGENTS.md requires. No blocking issues.
Non-blocking
1. with.cache false-positive on a local-composite invocation. checkStep runs the with.cache: <truthy> rule on every step, including a step whose uses: is a local composite. For a composite, with: is arbitrary inputs — an input named cache is unrelated to the Actions cache. Reproduced against a composite with a cache input invoked as with: { cache: true }:
job "release" step "Call composite with cache input": `with.cache: true` restores the GitHub Actions cache
This is the same false-positive class you deliberately avoid one level up for reusable workflows — job-level with: is kept out of checkStep, and reusable-input-named-cache.yml pins that. It's currently inert (no target workflow reaches a composite) and errs in the fail-closed direction, so not blocking — but consider skipping the with.cache rule when the step is a local-composite uses:, or a one-line comment noting the asymmetry is intentional.
2. Exit-2 suppresses cache offenders on a mixed run. unresolved is printed and process.exit(2) fires before the offenders (exit 1) block. A run that collects both an un-auditable reference and a real cache offender prints only the former. Both fail CI, so it's cosmetic, but the actionable cache finding stays hidden until the path is fixed. Consider printing both lists before exiting with the higher code.
3. Minor scope. The pnpm/action-setup@v6.0.8 → v6.0.9 bump in .github/actions/integration-setup/action.yml is unrelated to the gate. Harmless, noted only for commit atomicity.
Highlights
- Fail-closed allowlist with a thoroughly documented rationale (why a denylist was rejected; the
setup-<tool>-caches-by-default class with nocache:input and no telling name). - The module-load assertion that no
AUDITED_ACTIONSentry is cache-shaped makes the one careless re-opening edit impossible. - Fixtures are exhaustive and adversarial: composite and reusable cycles (asserted by offender count, not just exit),
action.yamlvs.yml, leading-whitespaceuses:, remote step vs remote reusable workflow,secrets:-agnostic verdict, invalidsteps:+uses:job, and multiple third-party cache vendors. - The
required.size > 0premise assertion added tointegration-workflow-paths.test.mjscloses a genuine "green because it checked nothing" gap, mirroring the #815 fix.
2 participants
Stack 2 of 4 — splitting #858. Base: #860.
packages/protect-ffi(subtree, upstream history preserved)What
scripts/lint-no-workflow-caching.mjskeeps the GitHub Actions cache out of the two credential-bearing workflows, because a poisoned cache entry would execute in a job that can publish to npm. It had three fail-open holes, each of which printedOK:release.ymlwith a cache-restoring composite spliced in: exit 0, no output, while the composite it never opened restored two caches.steps:at all, so the walker got an empty list and skipped the job entire. Confirmed before the fix: exit 0,OK, nothing scanned. The verdict ignoressecrets:deliberately —permissions:is inherited independently and is what mints the OIDC token npm trusted publishing signs with.useblacksmith/cache@v5andSwatinem/rust-cache@v2both passed. Fixed by inverting to an allowlist rather than lengthening a denylist: a denylist fails open on the action nobody has met yet, and cannot cover the class most likely to be added by accident — asetup-<tool>action that caches by default, with nocache:input and no telling name.Also adds the missing premise assertion to
integration-workflow-paths: its requirement set is derived from@/-aliased imports, so "no suite uses that alias" and "every import is covered" were the same green. Mutation-tested — rewriting the suites onto the public entry empties the set, and the check then passed withpackages/stack/src/dynamodb/**deleted from the filter, verbatim the #815 gap.Why it is separable
These are pre-existing holes in a control that already shipped. Nothing here depends on the vendoring.
Verification
Scripts suite 211 passing (51 in the two touched files); all three lint gates OK; biome 0 errors.