diff --git a/.claude/skills/analyze-action-pr/SKILL.md b/.claude/skills/analyze-action-pr/SKILL.md index 29afa07b..401d2126 100644 --- a/.claude/skills/analyze-action-pr/SKILL.md +++ b/.claude/skills/analyze-action-pr/SKILL.md @@ -70,6 +70,18 @@ GITHUB_TOKEN=$(gh auth token) uv run python -m verify_action_build --from-pr Exit 0 = all actions pass. Exit 1 = at least one failed. +For a PR whose `verify` check has already run in CI, read that job's log +instead — it is the same tool with the same output, costs no Docker +rebuild, and is the only option when the local environment can't run one: + +``` +gh pr checks --json name,link --jq '.[] | select(.name=="verify") | .link' +gh run view --log --job | sed -e 's/\x1b\[[0-9;]*[a-zA-Z]//g' +``` + +Strip the ANSI escapes (as above) or the summary table is unreadable. Re-run +locally only when you need to test a change to `verify-action-build` itself. + If the count of `Extracted action reference` lines is lower than the number of new hashes in the diff, the extractor is buggy → see case **E** under "Classify". @@ -83,7 +95,7 @@ under "Classify". | **C** | nested-action issue | Top-level action passes but a `uses:` dependency (e.g. `install/foo`) hits A or B | | **D** | metadata-only | `No LICENSE`, input interpolation in `run:` blocks, `GITHUB_PATH` writes — soft warnings, mention but don't block | | **E** | verify-script gap | The script gets the wrong answer for a reason unrelated to the action's actual security: false positive (regex hole, missing pattern), missing capability (new action type / build flow / verification mechanism it doesn't yet recognize), bad attribution (extractor drops an action that's clearly in the diff), or a check that misreads a legitimate input shape | -| **F** | unverified in-tree binaries | The action ships pre-compiled native binaries directly in the repo (Go cross-compile, `.exe` / `.dll` / `.so` / `.dylib`, `.jar`, `.wasm`, etc.) and exec's them from a small launcher. `verify-action-build`'s `In-tree binary check` tries to reconcile each binary with verifiable upstream provenance — first via `gh attestation verify --owner ` (SLSA attestation transparency log), then by comparing SHA256 against the release's `SHA256SUMS` asset. Binaries that pass either check are ✓; those that pass neither are case **F** and should be rejected until upstream adds provenance (`actions/attest-build-provenance` or a signed `SHA256SUMS`). Successful verification is a hard pass: the binary's bytes are tied back to the workflow run that produced them or to a release-time checksum | +| **F** | unverified in-tree binaries | The action ships pre-compiled native binaries directly in the repo (Go cross-compile, `.exe` / `.dll` / `.so` / `.dylib`, `.jar`, `.wasm`, etc.) and exec's them from a small launcher. `verify-action-build`'s `In-tree binary check` tries to reconcile each binary with verifiable upstream provenance — first via `gh attestation verify --owner ` (SLSA attestation transparency log), then by comparing SHA256 against the release's `SHA256SUMS` asset. Binaries that pass either check are ✓; those that pass neither are case **F** and should be rejected until upstream adds provenance (`actions/attest-build-provenance` or a signed `SHA256SUMS`). Successful verification is a hard pass: the binary's bytes are tied back to the workflow run that produced them or to a release-time checksum. **Not every unverified binary is case F**: one a bundler copied into the output directory from a lockfile-pinned dependency (a `.wasm`, `.node`, native lib) has npm's provenance chain, not the action's, so demanding a GitHub release attestation for it is case **E** — check whether the clean rebuild reproduces it before treating it as F | ### 4. Look up upstream verification material (for A/B/C) @@ -210,6 +222,10 @@ as templates when proposing your own extension): | Extraction shape | Hash added under an existing `actions.yml` key | [#804](https://github.com/apache/infrastructure-actions/pull/804) | | Rebuild emits intermediate output the action doesn't ship | Two-stage `tsc` → `esbuild` build where OUT_DIR resolves to the sub-project, so the gitignored stage-one `lib/` appears only in the rebuild | see the #1123 row under "Recent precedents" | | Per-ecosystem exemption | Lock-file rule too strict for library-first projects | [#770](https://github.com/apache/infrastructure-actions/pull/770) | +| New lock-file format | `aube-lock.yaml` (pnpm lockfileVersion 9) unrecognised | [#1175](https://github.com/apache/infrastructure-actions/pull/1175) | +| Registry tarball root assumed | DefinitelyTyped roots `@types/*` at the bare package name, not `package/` | [#1174](https://github.com/apache/infrastructure-actions/pull/1174) | +| Executable source not diffed or scanned | `.sh` / `Dockerfile*` outside the source-diff extension set, and script analysis limited to composite/docker actions | see the #1196 row under "Recent precedents" | +| Binary verifiable by rebuild, not by release | Bundler-copied dependency asset in `dist/` has npm provenance, not GitHub-release provenance | see the #1195 row under "Recent precedents" | When you hit a case that's clearly one of these — or a new kind not in the table — propose the fix. The cost of leaving a false positive @@ -296,3 +312,7 @@ future runs can cite a precedent instead of re-deriving the analysis. | #1123 | `JetBrains/qodana-action@v2026.2.0` — `JS build verification ✗` whose entire content was 4 files **only in the rebuild** (`lib/{annotations,main,output,utils}.js`). Two-stage build: `tsc --build` (`outDir: ./lib`) → `esbuild lib/main.js --bundle --outfile=dist/index.js`. `main: scan/dist/index.js` makes the Dockerfile's `cut -d'/' -f1` resolve OUT_DIR to the whole `scan/` sub-project, so stage-one output — gitignored upstream via `**/lib`, never committed — lands in the compared tree. **#960 hit this too and was merged over it; only the gradle-jar half got fixed in #951.** Also burns a pointless approved-lock-file retry rebuild | E | Fixed by treating only-in-rebuilt files as informational (not published → never runs on a consumer's runner → not a supply-chain vector), keeping only-in-original a hard failure and guarding the no-published-JS case. Action itself clean: dist bundle maps 1:1 to the TS source diff, `common/cli.json` checksums bumped in lockstep | | #1013 | `jdx/mise-action@5228313` (v3.6.3, node20) — clean all-pass: dist matches rebuild, LICENSE + lockfile present, and its `curl \| tar` fetch of the mise binary is **not** flagged (verifier reports "no binary downloads detected" — it's the tool pulling its own tarball from its official pinned release). Checksum verification is opt-in only (`sha256` input, from mise-action#185), off by default | clean (soft D note) | Approved. Gentle hardening issue mise-action#547: verify by default via a **signature against a pinned key** (mise's `minisign.pub`, key `64113EDF160FDEC2`, stable since Dec 2024), not a same-source `SHASUMS256.txt` — a checksum fetched from the same release as the binary is integrity, not authenticity, and can't cover the `mise.jdx.dev` CDN path. **Outcome: upstream accepted and fixed it same-day in mise-action#548 (signed-checksum verification); #547 closed completed** | | #1133 / #1134 | `pypa/gh-action-pypi-publish` v1.13.0→v1.14.2 (composite) and `hadolint/hadolint-action` v3.3.0→v3.4.0 (docker) — both clean all-pass, each with the single warning `Dockerfile FROM is tag-pinned, not digest-pinned`. In both cases the **already-approved prior version carries the identical shape**, so the warning is a standing property of the action, not something the bump introduced. pypi-publish's `oidc-exchange.py` looks alarming at +83/-15 but is ~90% type annotations; the substantive edits are hardening (`os.getenv()`→`os.environ[]`, explicit `IdentityError` when `detect_credential()` returns `None`, `format_map(locals())`→explicit kwargs), with no new endpoints and no change to the token-exchange flow | clean (soft D note) | Approved and merged. **Rule of thumb: for a tag-pinned-base-image warning, diff against the approved version before treating it as a finding — carried-over warnings are not regressions.** Worth tracking as a general hardening theme (digest-pinning base images) rather than blocking individual bumps | +| #1167 | `jdx/mise-action@7e36c90` (v4.2.4) — single red check `Lock file presence ✗`, everything else green including `JS build verification ✓`. The repo *does* have a lock file: v4.2.4 moved the build to [aube](https://github.com/jdx/aube), which writes `aube-lock.yaml` (pnpm lockfileVersion 9, 460 deps pinned with sha512 integrity). The presence check only knew the five established filenames | E | Fix in PR #1175. **Confirm a "missing lock file" verdict by listing the repo root before believing it** — `gh api 'repos///contents/?ref='` | +| #1171 | `JamesIves/github-pages-deploy-action@fa24774` (v4.9.0) — `Vendored npm registry check ✗ 13 extra`, but 0 modified, 0 errors, 116 packages integrity-verified, `JS build verification ✓`, and the vendored `node_modules` comparison matching all 3008 files across 75 packages. The 13 "extra" files were *every* file of exactly three packages (`@types/esrecurse`, `@types/estree`, `@types/json-schema`) — DefinitelyTyped roots its tarballs at the bare package name, so nothing lined up with `node_modules/@types//`. Switched on for the first time by this version's yarn → npm migration adding `node_modules/.package-lock.json` | E | Fix in PR #1174. **Heuristic: when *all* files of a package are reported extra, that is tarball resolution failing, not tampering. Injected code shows up as a few extra files among many accounted-for ones** | +| #1196 | `uraimo/run-on-arch-action@460cb8e` (v3.2.0) — two findings. (a) `JS build verification ✗` is entirely vendored-`node_modules` drift (87 files / 3 packages committed vs 192 / 6 rebuilt); the upstream compare touches no `node_modules` file, so the already-approved v3.1.0 carries the identical shape — carried over, not a regression, and the stricter registry check never engages because there is no `node_modules/.package-lock.json`. (b) The tool's "source changes" section listed only `action.yml`, hiding `src/run-on-arch.sh` **+12/-1** and three new `Dockerfiles/Dockerfile.*` — and `src/run-on-arch.js:127` `exec()`s that very script, so the shell payload *is* the action. `.sh` was outside `diff_source.py`'s extension set, and `analyze_scripts` ran only for composite/docker actions and only for scripts named in `action.yml` or a Dockerfile | E (+ carried-over) | Fix in branch `verify-diff-shell-sources`. The hidden `.sh` change was benign (bind-mounts `GITHUB_OUTPUT`/`ENV`/`PATH`/`STEP_SUMMARY`/`STATE` into the container to replace deprecated `set-output`). **For a node action, always check the upstream `compare` API for changed non-JS files — the tool's source diff is not the whole diff** | +| #1195 | `1Password/load-secrets-action@70062d7` (v5.0.1) — two findings, both carried over from the approved v5.0.0. (a) `Binary download verification ✗ 3 unverified` — `tc.downloadTool()` pulls the `op` CLI from the `cache.agilebits.com` CDN (not GitHub releases) with no checksum, then `core.addPath()`s it; the three installer `.ts` files are **untouched** by the bump. (b) `In-tree binary check ✗ dist/core_bg.wasm` — but the build is `ncc build ./src/index.ts`, and ncc copies dependency assets verbatim, so that 14 MB wasm comes from `@1password/sdk-core@0.5.0`, pinned by sha512 integrity in `package-lock.json`. Its provenance chain is npm's; asking for a GitHub-release attestation was the wrong question — case E, not F | E (+ carried-over B) | Fix in branch `verify-intree-rebuild-credit`: delete bundler-copied binaries before the rebuild as the minified JS already is, and credit any that come back byte-identical. **Both findings were already filed upstream by @potiuk — 1Password/load-secrets-action#168 (CLI download) and #186 (wasm provenance). Search the upstream repo before drafting anything** |