From 1c9a7e0def22f9361f7ffae0c87d9502aaa65dae Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Fri, 23 Oct 2026 20:45:02 +0800 Subject: [PATCH 1/2] Block only the vulnerabilities a merge adds cargo-audit ran inside the portable lane against the whole lockfile, so an advisory published against an existing crate failed every open pull request until main was fixed, and nothing tracked the repair. A separate audit job now scans the tested merge and fails only on findings its base did not have; an unchanged lockfile gets an informational scan. Main still needs a clean full scan before build and release publish anything. A daily workflow keeps one bot-authored tracking issue: it reopens the same issue when findings return, comments only when they change, and closes it after a clean scan, so subscribers hear about each change once. --- .claude/skills/codetrial-triage/pulls.md | 11 +- .claude/skills/codetrial-verify/SKILL.md | 36 +- .claude/skills/codetrial-verify/ci-failure.sh | 4 +- .github/actions/install-audit/action.yml | 14 + .github/workflows/audit.yml | 80 ++++ .github/workflows/check.yml | 70 +-- README.md | 2 +- docs/development.md | 75 ++- scripts/audit.py | 196 ++++++++ scripts/test.sh | 12 +- tests/test_audit.py | 444 ++++++++++++++++++ tests/test_skill_scripts.py | 24 + tests/test_test_gate.py | 112 ++++- 13 files changed, 998 insertions(+), 82 deletions(-) create mode 100644 .github/actions/install-audit/action.yml create mode 100644 .github/workflows/audit.yml create mode 100644 scripts/audit.py create mode 100644 tests/test_audit.py diff --git a/.claude/skills/codetrial-triage/pulls.md b/.claude/skills/codetrial-triage/pulls.md index 41a78aa26..26b768d02 100644 --- a/.claude/skills/codetrial-triage/pulls.md +++ b/.claude/skills/codetrial-triage/pulls.md @@ -335,15 +335,20 @@ says why it reads the whole log rather than `--log-failed`, and which markers count. A check outside Actions, cubic among them, comes back with its link only; read it from the PR's checks tab. -A `cargo-audit` advisory that `main` also has is the maintainer's, and the PR -must not carry a lockfile bump to clear it: #304 was told it "needs nothing from -you", and #305 to drop its unrelated `Cargo.lock` change. Once `main` lands the +For historical runs with `cargo-audit` in the portable lane, an advisory that +`main` also has is the maintainer's, and the PR must not carry a lockfile bump +to clear it: #304 was told it "needs nothing from you", and #305 to drop its +unrelated `Cargo.lock` change. Once `main` lands the fix (#331 moved to zip 9), a PR still failing on that advisory has a stale base, and the sweep puts it in `rebase` under the same limits as a conflicting one, so a request in any wording on its head, such as #326's own, keeps it out. A PR that touches a Cargo file is never a stale base: read whether its lockfile change brought the advisory before asking for anything. +The independent `audit` job compares the actual merge against its base tip; +inherited findings do not fail it. Read its summary for newly introduced +vulnerabilities or an incomplete scan instead of classifying it as stale. + A clean merge does not establish readiness. For a dependent draft, check whether its prerequisite merged and whether the author still needs to rebase, remove prerequisite commits, or mark it ready. Compare overlapping PRs by behavior as diff --git a/.claude/skills/codetrial-verify/SKILL.md b/.claude/skills/codetrial-verify/SKILL.md index fdbfda36a..278466eb2 100644 --- a/.claude/skills/codetrial-verify/SKILL.md +++ b/.claude/skills/codetrial-verify/SKILL.md @@ -15,12 +15,16 @@ make check # every gate, plus a live Gemini credential check CI runs one `gates ()` job per lane in parallel: `gates (rust)`, `gates (portable)` and `gates (browser)`. The `check` job only collects their -results and the mutation jobs', so its log does not say what failed: open the -failing lane, or, when every lane is green, the failing `plan-mutants` job or +results, the independent `audit` job and the mutation jobs', so its log does +not say what failed: open the failing lane, `audit`, `plan-mutants` job or `mutants` shard. On a pull request the portable lane checks the commit messages first, and a bad message skips the rest of that lane only. A gate names its lane where it is declared, one naming none is portable, and a test -proves the three lanes together cover the full run. +proves the three lanes plus the independent audit cover the full run. +`cargo-audit` runs only in the default local gate; CI uses `scripts/audit.py` +for merge-relative PR checks and full main checks. See +[dependency security checks](../../../docs/development.md#dependency-security-checks). +Its regression suite is `tests/test_audit.py`. Reproduce a failure with the same lane locally. `scripts/test.sh` is a list of named gates that all run before it reports, so @@ -149,16 +153,22 @@ What this adds is one call that reads all of it at the PR's head: Read its `failing` entries in this order. `current` false is history, not the current failure: say so and stop there. `current` or `on_main` null means its lookup failed, and `gates`, `errors` and `excerpt` null mean the job log could -not be read; check either with the raw calls below. `on_main` true means the latest -finished run on `main` fails the same job, so the failure is the maintainer's -and the author is not asked to fix it. Otherwise `gates` names what -`scripts/test.sh` failed and `errors` the lines that say why; rerun that lane -locally with the same head. A `cargo-audit` failure on a PR whose base predates -the advisory fix is a stale base, answered by a rebase (#334), not a code -change; `stale_base` (defined in the script's header) says exactly that, and -the PR sweep lists such a PR in `rebase` on its own. The aggregate `check` job -only collects results, so its entry points at the failing job it collected, -as in the lanes section above. A check outside Actions has only its link, and +not be read; check either with the raw calls below. `on_main` true means the +latest finished run on `main` fails the same job. Read whether the cause is +shared before asking the author for a fix: the PR `audit` compares added +findings, while main runs a full scan, so both can fail for different reasons. +Otherwise `gates` names what `scripts/test.sh` failed and `errors` the lines +that say why; rerun that lane locally with the same head. For historical runs +that still put `cargo-audit` in the portable lane, a failure on a PR whose base +predates the advisory fix is a stale base, answered by a rebase (#334), not a +code change; `stale_base` (defined in the script's header) says exactly that, +and the PR sweep lists such a historical PR in `rebase` on its own. +The independent `audit` job already compares the proposed merge with the base +tip: inherited findings are informational, while added findings and incomplete +dependency-change scans require investigation rather than an automatic rebase. +The aggregate `check` job only collects results, so its entry points at the +failing job it collected, as in the lanes section above. A check outside +Actions has only its link, and `held` checks have no log until a maintainer approves the run. The raw calls behind it, when one answer needs checking: diff --git a/.claude/skills/codetrial-verify/ci-failure.sh b/.claude/skills/codetrial-verify/ci-failure.sh index 882b7bb52..f192f589d 100755 --- a/.claude/skills/codetrial-verify/ci-failure.sh +++ b/.claude/skills/codetrial-verify/ci-failure.sh @@ -20,7 +20,9 @@ # Actions, its link only. current and on_main are null when the # lookup behind them failed, and gates, errors and excerpt when # the job log could not be read. -# stale_base true when every failing lane failed only gates that read +# stale_base legacy portable-audit runs only; the independent audit job +# reports inherited findings without failing. True when every +# failing lane failed only gates that read # outside data (the table in the last jq below: cargo-audit # reads the advisory database), on the current head, main's # latest run started after those runs and succeeded in each of diff --git a/.github/actions/install-audit/action.yml b/.github/actions/install-audit/action.yml new file mode 100644 index 000000000..762ababfa --- /dev/null +++ b/.github/actions/install-audit/action.yml @@ -0,0 +1,14 @@ +name: Install cargo-audit +description: Install the pinned, digest-checked RustSec release binary. +runs: + using: composite + steps: + - shell: sh + env: + GH_TOKEN: ${{ github.token }} + CARGO_AUDIT_VERSION: 0.22.2 + run: | + name="cargo-audit-x86_64-unknown-linux-musl-v$CARGO_AUDIT_VERSION" + ./scripts/install-release-binary.sh rustsec/rustsec \ + "cargo-audit/v$CARGO_AUDIT_VERSION" "$name.tgz" "$name/cargo-audit" + cargo audit --version | grep -qxF "cargo-audit-audit $CARGO_AUDIT_VERSION" diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml new file mode 100644 index 000000000..893ffc62d --- /dev/null +++ b/.github/workflows/audit.yml @@ -0,0 +1,80 @@ +name: security audit + +on: + schedule: + - cron: '43 4 * * *' + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: security-audit + +jobs: + audit: + # Only trusted main code may update the repository's tracking issue. + if: github.ref == 'refs/heads/main' + runs-on: ubuntu-24.04 + timeout-minutes: 10 + permissions: + contents: read + issues: write + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + - name: Install cargo-audit + uses: ./.github/actions/install-audit + - name: Audit main + id: audit + run: python3 scripts/audit.py --report "$RUNNER_TEMP/audit.md" + - name: Track remediation + if: ${{ !cancelled() && steps.audit.outputs.findings != '' }} + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + FINDINGS: ${{ steps.audit.outputs.findings }} + run: | + title='Dependency security audit' + fingerprint() { sed -n '/^Findings fingerprint:/p'; } + # gh refuses --slurp with --jq, so filter the slurped pages with jq. + # Closed issues count too: reopening keeps existing subscribers. + found=$(gh api --paginate --slurp --method GET "repos/$GH_REPO/issues" \ + -f state=all -f 'creator=github-actions[bot]' -f per_page=100 \ + | jq -r --arg title "$title" '[.[][] | select(.pull_request == null and .user.login == "github-actions[bot]" and .title == $title)][0] // empty | "\(.number) \(.state)"') + read -r issue state <<< "$found" + if [ "$FINDINGS" != true ]; then + # The clean body replaces the old fingerprint, so findings that + # return later count as a change and notify subscribers again. + if [ "$state" = open ]; then + gh issue edit "$issue" --body-file "$RUNNER_TEMP/audit.md" + gh issue close "$issue" --reason completed + fi + exit 0 + fi + if [ -z "$issue" ]; then + gh issue create --title "$title" --body-file "$RUNNER_TEMP/audit.md" + exit 0 + fi + current=$(fingerprint < "$RUNNER_TEMP/audit.md") + previous=$(gh issue view "$issue" --json body --jq .body | fingerprint) + # Keep the old fingerprint until notification succeeds so failures + # retry, but skip the comment if a failed edit left it posted. On a + # closed issue the last comment predates the clean scan, so recurring + # findings always notify. + if [ "$previous" != "$current" ]; then + notified= + if [ "$state" = open ]; then + notified=$(gh api --paginate --slurp "repos/$GH_REPO/issues/$issue/comments" \ + | jq -r '[.[][] | select(.user.login == "github-actions[bot]")] | last | .body // ""' \ + | fingerprint) + fi + if [ "$notified" != "$current" ]; then + gh issue comment "$issue" --body-file "$RUNNER_TEMP/audit.md" + fi + fi + gh issue edit "$issue" --body-file "$RUNNER_TEMP/audit.md" + if [ "$state" = closed ]; then + gh issue reopen "$issue" + fi diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 1253044ec..90ed89613 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -54,23 +54,51 @@ env: # install, and a guard skips the install whenever a cached binary exists: # bumping only the install line would leave the old key matching, the old # binary restored, and CI running a version nobody asked for. - CARGO_AUDIT_VERSION: 0.22.2 CARGO_MUTANTS_VERSION: 27.1.0 - # Pinned for the same reason the two above are: a linter that moves under you + # Pinned for the same reason as the one above: a linter that moves under you # turns an unrelated push red, and the fix is then a version bump nobody chose # to make. Kept in step with whatever contributors run locally, which is # whatever `ruff` is on their PATH; the gate skips rather than fails when they # have none. RUFF_VERSION: 0.15.2 - # The workflow's own linter, pinned like the three above. Installed rather + # The workflow's own linter, pinned like the tools above. Installed rather # than assumed: `scripts/test.sh` skips this gate when the binary is absent, # and absent is what it was here, so the file that describes every other # check was the one file nothing checked. ACTIONLINT_VERSION: 1.7.12 jobs: + audit: + if: github.event_name != 'schedule' && github.event_name != 'workflow_dispatch' + runs-on: ubuntu-24.04 + timeout-minutes: 10 + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 2 + persist-credentials: false + - name: Install cargo-audit + uses: ./.github/actions/install-audit + - name: Audit the tested merge or main + env: + EVENT: ${{ github.event_name }} + GROUP_BASE: ${{ github.event.merge_group.base_sha }} + run: | + case "$EVENT" in + pull_request) + base=$(git rev-parse HEAD^1) + python3 scripts/audit.py --base "$base" + ;; + merge_group) + git cat-file -e "$GROUP_BASE^{commit}" || git fetch --no-tags --depth=1 origin "$GROUP_BASE" + python3 scripts/audit.py --base "$GROUP_BASE" + ;; + push) python3 scripts/audit.py ;; + *) exit 2 ;; + esac + gates: name: gates (${{ matrix.lane }}) if: github.event_name != 'schedule' && github.event_name != 'workflow_dispatch' @@ -225,25 +253,6 @@ jobs: id: chromium uses: ./.github/actions/chromium - # The gate skips this audit when the binary is missing, which is right for - # a contributor's laptop and wrong for CI: a silent skip here means the - # dependency surface stops being checked and nothing says so. Installed - # ahead of the gate so the skip branch is never the one CI takes. - # - # The upstream release binary, checked against the digest its release - # records, rather than `cargo install`: that compiled the crate graph for - # minutes on every cache miss. musl, so the binary does not depend on the - # runner's glibc. The version check proves the pin is what installed. - - name: Install cargo-audit - if: matrix.lane == 'portable' - env: - GH_TOKEN: ${{ github.token }} - run: | - name="cargo-audit-x86_64-unknown-linux-musl-v$CARGO_AUDIT_VERSION" - ./scripts/install-release-binary.sh rustsec/rustsec \ - "cargo-audit/v$CARGO_AUDIT_VERSION" "$name.tgz" "$name/cargo-audit" - cargo audit --version | grep -qxF "cargo-audit-audit $CARGO_AUDIT_VERSION" - # The Python here generates the problem bank and drives two integration # harnesses, and `ruff check` is its only linter. The gate skips when the # binary is missing, which is right on a laptop and wrong here. pipx is on @@ -366,7 +375,7 @@ jobs: target key: ${{ steps.cargo-cache.outputs.key }} - # The one status: every gate lane passed, and on a pull request the mutation + # The one status: every gate lane and audit passed, and on a PR the mutation # plan and every shard did too. Planning only runs on pull requests, and the # shards only after a plan that found mutants, so a skipped shard job after a # successful plan means the plan counted none. A push or a merge group plans @@ -377,7 +386,7 @@ jobs: # reaches this job and fails it. Not on a schedule or a dispatch, which run # no gates. check: - needs: [gates, plan-mutants, mutants] + needs: [gates, audit, plan-mutants, mutants] if: ${{ !cancelled() && github.event_name != 'schedule' && github.event_name != 'workflow_dispatch' }} runs-on: ubuntu-24.04 timeout-minutes: 5 @@ -385,6 +394,7 @@ jobs: - name: Require every gate lane and mutation shard env: GATES: ${{ needs.gates.result }} + AUDIT: ${{ needs.audit.result }} PLAN: ${{ needs.plan-mutants.result }} MUTANTS: ${{ needs.mutants.result }} run: | @@ -392,6 +402,10 @@ jobs: echo "::error::gate lanes: $GATES. This job only collects them; the failed gates lane has the cause." exit 1 fi + if [ "$AUDIT" != success ]; then + echo "::error::security audit: $AUDIT. See the audit job for new vulnerabilities or an incomplete scan." + exit 1 + fi case "$MUTANTS/$PLAN" in success/success | skipped/success | skipped/skipped) ;; *) echo "::error::mutants: $MUTANTS (plan: $PLAN)"; exit 1 ;; @@ -653,11 +667,11 @@ jobs: # compiles `web/` (including its WASM) into it. Native runners avoid a # cross-platform linker or packaging toolchain. # - # After `gates` rather than `check`: on a push `check` adds nothing, and the - # mutation jobs it waits on are skipped, which the default `success()` reads + # After `gates` and `audit` rather than `check`: on a push `check` adds + # nothing, and its mutation jobs are skipped, which the default `success()` reads # as a reason to skip everything downstream. build: - needs: gates + needs: [gates, audit] if: github.event_name == 'push' && github.ref == 'refs/heads/main' name: ${{ matrix.name }} timeout-minutes: 45 @@ -970,7 +984,7 @@ jobs: # One rolling release for the current main build. GitHub releases require a # tag; `latest` avoids exposing an implementation SHA as a release name. release: - needs: [gates, build] + needs: [gates, audit, build] if: github.event_name == 'push' && github.ref == 'refs/heads/main' runs-on: ubuntu-24.04 timeout-minutes: 15 diff --git a/README.md b/README.md index b47054515..0465e604e 100644 --- a/README.md +++ b/README.md @@ -57,7 +57,7 @@ rather than guessing. | Phase | Required | Notes | |---|---|---| | Build time | Rust stable, `curl` or `wget`, and `sha256sum` or `shasum`; Linux also needs Clang 21+ and GLib development headers | Cargo resolves application crates from `Cargo.lock`. `make build` fetches the checksum-pinned browser assets on the first build. | -| Test time | Build-time tools, Node.js 18+, and Python 3.9+ | Node runs browser and fixture checks; Python verifies generated problem-bank files. `npm ci` adds pinned Prettier, ESLint and Playwright for the full local browser gate. `ruff`, `shellcheck`, `shfmt`, `commentflow`, `actionlint`, and `cargo-audit` are optional: each gate reports that it skipped rather than failing without them. CI installs all of them, so those lanes are enforced on a pull request whatever a checkout can run; locally the `actionlint` lane also takes its container image. | +| Test time | Build-time tools, Node.js 18+, and Python 3.9+ | Node runs browser and fixture checks; Python verifies generated problem-bank files. `npm ci` adds pinned Prettier, ESLint and Playwright for the full local browser gate. `ruff`, `shellcheck`, `shfmt`, `commentflow`, `actionlint`, and `cargo-audit` are optional: each gate reports that it skipped rather than failing without them; locally the `actionlint` lane also takes its container image. CI installs these tools and enforces its test lanes whatever a checkout can run. The independent [dependency security check](docs/development.md#dependency-security-checks) blocks newly introduced vulnerabilities on PRs and runs full audits on main. | | Runtime | The compiled `codetrial` binary and a config file | No Node.js, Python, or `node_modules` is required. Rust dependencies are compiled into the binary; browser dependencies are vendored, checksum-pinned, and embedded, so a `web/` directory is optional and only overrides what is already inside. | On Linux x86_64, `make` downloads the checksum-pinned WebRTC compiler into diff --git a/docs/development.md b/docs/development.md index 711825453..219ecf973 100644 --- a/docs/development.md +++ b/docs/development.md @@ -5,6 +5,55 @@ time actually goes. `make` targets and the one-line summary of each are in [README.md](../README.md#development); everything here is the detail behind them. +## Dependency security checks + +A new RustSec advisory must reach maintainers without making an unrelated +feature look broken. The independent `audit` job uses `scripts/audit.py`: + +- Pull requests compare the tested merge commit against its first parent, the + event's base tip. Merge groups compare against `merge_group.base_sha`. + Never use the old branch's merge-base: that can hide a reintroduced weakness. +- Both lockfiles use one freshly fetched advisory database snapshot. Findings + are compared by advisory ID, crate and version, so replacing one vulnerable + dependency with another cannot pass by keeping the same count. +- An unchanged lockfile gets one informational scan. Inherited vulnerabilities + and scan errors appear in the job summary without failing the PR. With a + changed lockfile, new findings or an incomplete scan fail the audit. Removing + a finding passes; upgrading to another affected version remains blocking. +- Pushes to main run a full audit. Builds and the rolling release wait for it, + so a known vulnerability or scan error blocks publication. This can delay + unrelated fixes while the previous release remains vulnerable; prioritize + the dependency repair rather than bypassing the audit. + +Checkout and tool-installation failures still fail the job. The informational +exception covers scanner and database errors after the tool is installed. + +The separate `security audit` workflow runs daily and on manual dispatch from +main. It maintains one bot-authored issue titled `Dependency security audit`, +updating its findings or closing it after a clean scan, and reopening the same +issue when findings return so its subscribers keep following it. Changes to the +findings also leave a comment to notify subscribers, once even when a retry +follows a failed update; a database refresh alone does not. +An incomplete scan fails without closing the issue. Only this trusted-main job +has `issues: write`; fork PRs use read permissions and job summaries, without +comments or secrets. Maintainers should subscribe to the tracking issue and +workflow failures, assign an owner and a repair deadline, and link the repair +PR in the issue. A recurring scan is not a repair deadline. + +Enable Dependabot alerts and security updates in repository settings; the +existing `.github/dependabot.yml` groups coupled security updates. RustSec +findings may precede GitHub's advisory import, so the daily audit remains +necessary. Review security PRs promptly and link them to the audit issue. +Do not add blanket advisory ignores. Any proposed exception needs a maintainer, +reason, expiry and tracking issue; no exception is implemented by this change. + +`./scripts/test.sh` retains the full local `cargo audit` when installed. Its +three CI lanes omit that gate because the separate audit job owns CI policy. +To reproduce CI locally, run `python3 scripts/audit.py` for a full scan or +`python3 scripts/audit.py --base ` on the tested merge tree. +The scanner tests run with +`python3 scripts/run-python-tests.py tests/test_audit.py`. + ## What the gate checks, and what it will not `./scripts/test.sh` finishes by printing what it did not check, because several @@ -22,7 +71,7 @@ cargo install cargo-audit # the dependency advisory scan CI runs `scripts/test.sh rust`, `scripts/test.sh portable` and `scripts/test.sh browser` in parallel, with fail-fast disabled so a failure in one lane does not hide the others. Clippy and Rust tests share the Rust lane's -build cache; formatting, audits, Python and shell checks run in the portable +build cache; formatting, Python and shell checks run in the portable lane; Node browser tests have their own lane, except the face-detector suite, which needs the Rust binary and runs after its build. All lanes fetch and verify vendor assets; each restores the vendored-asset cache, and only the @@ -32,18 +81,18 @@ clone only `HEAD`; the portable lane keeps full history for the commit-message check. The Rust lane deletes the runner's unused toolchains only when less than 40 GB is free. Running `./scripts/test.sh` locally still runs every gate. -`check` is the only status a branch rule for main should require. It needs -every lane to succeed, and on a pull request the mutation plan and every shard -too; shards may be skipped only when the plan found no mutants. A push to main -or a merge queue entry plans no mutants, so `check` reports as soon as the -lanes finish. The workflow also runs on `merge_group`, so once main has a merge -queue, it tests each pull request on top of the main it will land on; two pull -requests that pass against their own bases can still break main together. A -merge group saves no caches, since its queue ref is gone once it lands. -Mutation planning checks the Rust diff before installing its tools, so a PR -with no Rust changes starts no mutation shards and pays no mutation-tool setup -cost. Rust diffs still use the complete diff-selected mutant set and the -existing exclusions. +`check` is the only status a branch rule for main should require. It needs every +lane and the independent security audit to succeed, and on a pull request the +mutation plan and every shard too; shards may be skipped only when the plan +found no mutants. A push to main or a merge queue entry plans no mutants, so +`check` reports as soon as the lanes finish. The workflow also runs on +`merge_group`, so once main has a merge queue, it tests each pull request on top +of the main it will land on; two pull requests that pass against their own bases +can still break main together. A merge group saves no caches, since its queue +ref is gone once it lands. Mutation planning checks the Rust diff before +installing its tools, so a PR with no Rust changes starts no mutation shards and +pays no mutation-tool setup cost. Rust diffs still use the complete +diff-selected mutant set and the existing exclusions. Every skip the gate sees joins its `not checked by this run:` summary: a gate's own skip, a Node browser test's skip or todo, a missing formatter in diff --git a/scripts/audit.py b/scripts/audit.py new file mode 100644 index 000000000..39cdd7ba7 --- /dev/null +++ b/scripts/audit.py @@ -0,0 +1,196 @@ +#!/usr/bin/env python3 +"""Audit the full lockfile, or only vulnerabilities a proposed merge adds.""" + +import argparse +import hashlib +import json +import os +import subprocess +import sys +import tempfile +from pathlib import Path + + +def git(root, *args): + return subprocess.check_output(["git", "-C", str(root), *args]) + + +def audit(lockfile, database, fetch): + command = [ + "cargo", + "audit", + "--json", + "--db", + str(database), + "--file", + str(lockfile), + ] + if not fetch: + command.append("--no-fetch") + # A single configuration must govern both trees; do not load PR ignore rules. + result = subprocess.run( + command, cwd=lockfile.parent, capture_output=True, text=True + ) + if result.stderr: + print(result.stderr, file=sys.stderr, end="") + try: + report = json.loads(result.stdout) + vulnerabilities = report["vulnerabilities"] + warnings = report.get("warnings", {}) + if ( + not isinstance(vulnerabilities["list"], list) + or type(vulnerabilities["found"]) is not bool + or type(vulnerabilities["count"]) is not int + or not isinstance(warnings, dict) + or not all(isinstance(items, list) for items in warnings.values()) + ): + raise ValueError("invalid audit report schema") + findings = { + ( + item["advisory"]["id"], + item["package"]["name"], + item["package"]["version"], + ) + for item in vulnerabilities["list"] + } + valid = ( + vulnerabilities["found"] == bool(findings) + and vulnerabilities["count"] == len(vulnerabilities["list"]) + and result.returncode == int(bool(findings)) + and all( + isinstance(value, str) and value for key in findings for value in key + ) + ) + if not valid: + raise ValueError("inconsistent audit report or exit status") + return findings, warnings + except (KeyError, TypeError, ValueError) as error: + raise ValueError(f"audit did not complete: {error}") from error + + +def scan(head, previous, informational): + """Return head findings, those the base already had, warnings and the DB revision.""" + with tempfile.TemporaryDirectory(prefix="codetrial-audit-") as directory: + scratch = Path(directory) + database = scratch / "advisory-db" + lockfile = scratch / "Cargo.lock" + lockfile.write_bytes(head) + findings, warnings = audit(lockfile, database, True) + revision = git(database, "rev-parse", "HEAD").decode().strip() + if informational: + inherited = findings + elif previous is not None: + lockfile.write_bytes(previous) + inherited, _ = audit(lockfile, database, False) + else: + inherited = set() + return findings, inherited, warnings, revision + + +def render(lines, findings, added, warnings, revision): + lines.append(f"RustSec database: `{revision}`") + lines.append(f"\nVulnerabilities: {len(findings)}; blocking: {len(added)}") + fingerprint = hashlib.sha256(json.dumps(sorted(findings)).encode()).hexdigest() + lines.append(f"Findings fingerprint: `{fingerprint}`") + for finding in sorted(findings): + advisory, crate, version = finding + blocking = finding in added + lines.append( + f"- [{advisory}](https://rustsec.org/advisories/{advisory}.html): " + f"`{crate} {version}` ({'blocking' if blocking else 'inherited'})" + ) + if blocking: + message = ( + f"{advisory} {crate} {version}".replace("%", "%25") + .replace("\r", "%0D") + .replace("\n", "%0A") + ) + print(f"::error::{message}", file=sys.stderr) + if warnings: + lines.append( + "\nNon-blocking cargo-audit warnings (details in job log): " + + ", ".join(f"{kind}: {len(items)}" for kind, items in warnings.items()) + ) + print(json.dumps(warnings, indent=2), file=sys.stderr) + if findings: + lines.append( + "\nMaintainers track remediation in [the security audit issue]" + "(https://github.com/sysprog21/codetrial/issues?q=" + "is%3Aissue+is%3Aopen+author%3Aapp%2Fgithub-actions+" + "in%3Atitle+%22Dependency+security+audit%22) " + "and [Dependabot alerts](https://github.com/sysprog21/codetrial/security/dependabot)." + ) + + +def run(root, base, lines): + head = (root / "Cargo.lock").read_bytes() + # Failing to read the base is fatal: it decides whether errors may pass. + previous = git(root, "show", f"{base}:Cargo.lock") if base else None + informational = previous == head + if informational: + mode = "informational (unchanged lockfile)" + else: + mode = "new vulnerabilities in proposed merge" if base else "full" + lines.append("Mode: " + mode) + try: + findings, inherited, warnings, revision = scan(head, previous, informational) + except (OSError, ValueError, subprocess.CalledProcessError) as error: + if not informational: + raise + lines.append( + f"\nScan incomplete: {error}. No dependency change; informational only." + ) + print( + "::warning::Security scan incomplete; unchanged lockfile, see audit summary." + ) + return 0, None + added = findings - inherited + render(lines, findings, added, warnings, revision) + return int(bool(added)), bool(findings) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--base", help="base tip SHA of the tested merge, not its merge-base" + ) + parser.add_argument( + "--report", type=Path, help="write a Markdown report for security tracking" + ) + args = parser.parse_args() + if args.base == "": + parser.error("--base must name the tested merge's base commit") + root = Path(__file__).resolve().parents[1] + lines = ["# Dependency security audit", ""] + findings = None + try: + status, findings = run(root, args.base, lines) + except (OSError, ValueError, subprocess.CalledProcessError) as error: + lines.append(f"\nScan incomplete: {error}") + print( + "::error::Security scan did not complete; see audit summary.", + file=sys.stderr, + ) + status = 2 + report = "\n".join(lines) + "\n" + print(report) + if args.report: + # GitHub issue bodies accept at most 65,536 characters. + args.report.write_text( + report + if len(report) <= 60000 + else report[:60000] + + "\n\nReport truncated; see the security audit workflow log for all findings.\n" + ) + if summary_path := os.environ.get("GITHUB_STEP_SUMMARY"): + with open(summary_path, "a") as summary: + summary.write(report) + output_path = os.environ.get("GITHUB_OUTPUT") + if findings is not None and output_path: + with open(output_path, "a") as output: + output.write(f"findings={str(findings).lower()}\n") + return status + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/test.sh b/scripts/test.sh index 3f38c70e9..d8d9dbe15 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -307,11 +307,10 @@ ruff_gate() # gate. Repo-wide false positives are answered once in `.shellcheckrc`; the few # local ones carry a directive naming why, beside the code it is about. # -# Optional the way cargo-audit is: CI installs it, and a contributor without it -# gets a note rather than a failure they cannot act on. That note counts the -# scripts off the same glob the loop walks, because the number was typed into -# the sentence once and adding a script left it describing a tree that no longer -# existed. +# Optional: CI installs it, and a contributor without it gets a note rather than +# a failure they cannot act on. That note counts the scripts off the same glob +# the loop walks, because the number was typed into the sentence once and adding +# a script left it describing a tree that no longer existed. shell_syntax() { status=0 @@ -464,6 +463,7 @@ gate study-plan-guards "$PYTHON" "$ROOT/scripts/check-study-plan-guards.py" gate calibration-fixtures "$PYTHON" "$ROOT/scripts/gen-calibration-fixtures.py" --check gate calibration-tests unittest_gate "$ROOT/tests/test_calibrate_framework.py" gate gemini-usage-tests unittest_gate "$ROOT/tests/test_analyze_gemini_usage.py" +gate audit-tests unittest_gate "$ROOT/tests/test_audit.py" gate test-gate-tests unittest_gate "$ROOT/tests/test_test_gate.py" gate mutants-plan-tests unittest_gate "$ROOT/tests/test_mutants_plan.py" gate mutants-install-tests unittest_gate "$ROOT/tests/test_mutants_install.py" @@ -477,7 +477,7 @@ gate skill-refs "$PYTHON" "$ROOT/scripts/check-skill-refs.py" gate skill-scripts-tests unittest_gate "$ROOT/tests/test_skill_scripts.py" gate --lane browser browser-tests browser_tests static gate eslint eslint_gate -gate cargo-audit cargo_audit_gate +gate --lane all cargo-audit cargo_audit_gate gate vendor-patches vendor_patches_gate gate actionlint actionlint_gate diff --git a/tests/test_audit.py b/tests/test_audit.py new file mode 100644 index 000000000..bc4ed0662 --- /dev/null +++ b/tests/test_audit.py @@ -0,0 +1,444 @@ +"""Exercise audits with real base commits and a deterministic cargo boundary.""" + +import importlib.util +import json +import os +import re +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +SPEC = importlib.util.spec_from_file_location( + "support", Path(__file__).with_name("support.py") +) +support = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(support) + +CARGO = """#!/usr/bin/env python3 +import json, os, subprocess, sys +from pathlib import Path +args = sys.argv +db = Path(args[args.index("--db") + 1]) +with open(os.environ["AUDIT_CALLS"], "a") as log: + log.write(json.dumps(args[1:]) + "\\n") +if "--no-fetch" not in args: + db.mkdir() + subprocess.run(["git", "init", "--quiet", str(db)], check=True) + subprocess.run(["git", "-C", str(db), "-c", "user.name=Test", + "-c", "user.email=test@example.org", "-c", "core.hooksPath=/dev/null", + "commit", "--quiet", "--allow-empty", "-m", "snapshot"], check=True) +data = json.loads(Path(args[args.index("--file") + 1]).read_text()) +if "raw" in data: + print(data["raw"]) + sys.exit(data.get("exit", 1)) +items = [{"advisory": {"id": a}, "package": {"name": n, "version": v}} + for a, n, v in data["findings"]] +print(json.dumps({"vulnerabilities": {"found": bool(items), "count": len(items), + "list": items}, "warnings": data.get("warnings", {})})) +sys.exit(data.get("exit", int(bool(items)))) +""" + +GH = """#!/usr/bin/env python3 +import json, os, subprocess, sys +args = sys.argv[1:] +action = "api" if args[0] == "api" else args[1] +with open(os.environ["GH_CALLS"], "a") as log: + log.write(json.dumps(args) + "\\n") +if action == os.environ["FAIL_ACTION"]: + sys.exit(1) +if "--slurp" in args and ("--jq" in args or "--template" in args): + sys.exit("the `--slurp` option is not supported with `--jq` or `--template`") +if action == "api": + comments = any(arg.endswith("/comments") for arg in args) + print(os.environ["COMMENT_PAGES" if comments else "ISSUE_PAGES"]) +if action == "view": + print(os.environ["OLD_BODY"]) +""" + +A = ["RUSTSEC-2026-0385", "ctutils", "0.4.3"] +B = ["RUSTSEC-2026-9999", "other", "1.0.0"] +ISSUE = { + "number": 42, + "title": "Dependency security audit", + "state": "open", + "user": {"login": "github-actions[bot]"}, +} + + +class AuditTests(unittest.TestCase): + def test_empty_base_cannot_fall_back_to_a_full_scan(self): + result = subprocess.run( + [sys.executable, str(support.ROOT / "scripts/audit.py"), "--base", ""], + capture_output=True, + text=True, + timeout=60, + ) + self.assertEqual(result.returncode, 2) + self.assertIn("--base must name", result.stderr) + + def run_tracking( + self, pages, *, findings="true", old="", current="", fail="", comments=() + ): + workflow = (support.ROOT / ".github/workflows/audit.yml").read_text() + tracking = support.step(support.job("audit", workflow), "Track remediation") + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + (root / "audit.md").write_text(current) + support.write_executable(root / "gh", GH) + result = subprocess.run( + # Actions runs bash steps with errexit and pipefail. + ["bash", "-euo", "pipefail", "-c", support.run_block(tracking)], + env={ + **os.environ, + "PATH": str(root) + os.pathsep + os.environ["PATH"], + "GH_CALLS": str(root / "calls"), + "RUNNER_TEMP": str(root), + "ISSUE_PAGES": json.dumps(pages), + "COMMENT_PAGES": json.dumps([list(comments)]), + "OLD_BODY": old, + "FAIL_ACTION": fail, + "FINDINGS": findings, + "GH_REPO": "sysprog21/codetrial", + }, + capture_output=True, + text=True, + timeout=60, + ) + calls = [ + json.loads(line) for line in (root / "calls").read_text().splitlines() + ] + self.assertIn("creator=github-actions[bot]", calls[0]) + self.assertIn("--paginate", calls[0]) + self.assertIn("--slurp", calls[0]) + self.assertIn("state=all", calls[0]) + for call in calls[1:]: + if "--body-file" in call: + self.assertEqual( + Path(call[call.index("--body-file") + 1]).read_text(), current + ) + return result, calls + + def test_only_trusted_main_runs_with_issue_write_access(self): + workflow = (support.ROOT / ".github/workflows/audit.yml").read_text() + job = support.job("audit", workflow) + tracking = support.step(job, "Track remediation") + self.assertIn("steps.audit.outputs.findings != ''", tracking) + self.assertIn("if: github.ref == 'refs/heads/main'", job) + triggers = support.section( + workflow, "\non:\n", "\npermissions:", "audit triggers" + ) + self.assertNotIn("pull_request", triggers) + + @unittest.skipUnless(shutil.which("jq"), "jq is not installed") + def test_tracking_filters_real_json_including_other_authors_prs_and_pages(self): + bot = {key: value for key, value in ISSUE.items() if key != "number"} + decoys = [ + {**bot, "number": 1, "user": {"login": "outsider"}}, + {**bot, "number": 2, "pull_request": {}}, + {**bot, "number": 3, "title": "Other issue"}, + ] + for pages, expected in ( + ([[]], None), + ([decoys], None), + ([decoys, [{**bot, "number": 42}, {**bot, "number": 43}]], "42"), + ): + with self.subTest(pages=pages): + result, calls = self.run_tracking(pages, findings="false") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual( + [call[:3] for call in calls[1:]], + [["issue", "edit", expected], ["issue", "close", expected]] + if expected + else [], + ) + + @unittest.skipUnless(shutil.which("jq"), "jq is not installed") + def test_tracking_notifies_only_when_findings_change_and_preserves_retry(self): + issue = ISSUE + closed = {**issue, "state": "closed"} + unchanged = "Findings fingerprint: `same`\n" + changed = "Findings fingerprint: `new`\n" + bot = {"login": "github-actions[bot]"} + posted = [{"user": bot, "body": "Report\n" + changed}] + stale = [ + {"user": bot, "body": "Report\n" + changed}, + {"user": bot, "body": "Report\n" + unchanged}, + {"user": {"login": "outsider"}, "body": changed}, + ] + notify = ["view", "comments", "comment", "edit"] + for pages, findings, old, current, fail, comments, actions in ( + ([[]], "true", "", unchanged, "", (), ["create"]), + ( + [[issue]], + "true", + unchanged, + unchanged + "New database revision\n", + "", + (), + ["view", "edit"], + ), + ([[issue]], "true", unchanged, changed, "", (), notify), + ([[issue]], "true", "Legacy report", changed, "", (), notify), + ([[issue]], "true", unchanged, changed, "", stale, notify), + # A comment whose follow-up edit failed is not posted twice. + ( + [[issue]], + "true", + unchanged, + changed, + "", + posted, + ["view", "comments", "edit"], + ), + # Recurring findings reopen the issue its subscribers follow. + ( + [[closed]], + "true", + unchanged, + unchanged, + "", + (), + ["view", "edit", "reopen"], + ), + # Findings that return after a clean close notify again, even when + # they match the comment from before the close. + ( + [[closed]], + "true", + "Clean report", + changed, + "", + posted, + ["view", "comment", "edit", "reopen"], + ), + ([[issue]], "false", unchanged, "Clean report", "", (), ["edit", "close"]), + ([[closed]], "false", unchanged, "Clean report", "", (), []), + ([[]], "false", "", "Clean report", "", (), []), + ([[issue]], "true", unchanged, changed, "api", (), []), + ([[issue]], "true", unchanged, changed, "comment", (), notify[:3]), + ([[issue]], "true", unchanged, changed, "edit", (), notify), + ): + with self.subTest( + pages=pages, findings=findings, old=old, fail=fail, actions=actions + ): + result, calls = self.run_tracking( + pages, + findings=findings, + old=old, + current=current, + fail=fail, + comments=comments, + ) + self.assertEqual(result.returncode, int(bool(fail)), result.stderr) + names = [ + "comments" if call[0] == "api" else call[1] for call in calls[1:] + ] + self.assertEqual(names, actions) + for call in calls[1:]: + if call[0] == "api": + self.assertIn( + "repos/sysprog21/codetrial/issues/42/comments", call + ) + elif call[1] != "create": + self.assertEqual(call[2], "42") + + def run_audit(self, base, head, *, full=False, invalid_base=False): + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + (root / "scripts").mkdir() + script = root / "scripts/audit.py" + script.write_bytes((support.ROOT / "scripts/audit.py").read_bytes()) + support.write_executable(root / "cargo", CARGO) + + def git(*args): + return subprocess.check_output(["git", "-C", str(root), *args]) + + git("init", "--quiet") + git("config", "user.name", "Test") + git("config", "user.email", "test@example.org") + git("config", "core.hooksPath", "/dev/null") + lockfile = root / "Cargo.lock" + lockfile.write_text(json.dumps(base)) + git("add", "Cargo.lock") + git("commit", "--quiet", "-m", "base") + sha = git("rev-parse", "HEAD").decode().strip() + lockfile.write_text(json.dumps(head)) + command = [sys.executable, str(script), "--report", str(root / "report.md")] + if not full: + command += ["--base", "missing" if invalid_base else sha] + env = { + **os.environ, + "PATH": str(root) + os.pathsep + os.environ["PATH"], + "AUDIT_CALLS": str(root / "calls"), + "GITHUB_OUTPUT": str(root / "outputs"), + "GITHUB_STEP_SUMMARY": str(root / "summary"), + } + result = subprocess.run( + command, env=env, capture_output=True, text=True, timeout=30 + ) + report = (root / "report.md").read_text() + summary = (root / "summary").read_text() + if len(summary) <= 60000: + self.assertEqual(report, summary) + else: + self.assertTrue(report.startswith(summary[:60000])) + self.assertIn("Report truncated", report) + outputs = ( + (root / "outputs").read_text() if (root / "outputs").exists() else "" + ) + calls = ( + [json.loads(line) for line in (root / "calls").read_text().splitlines()] + if (root / "calls").exists() + else [] + ) + return result, report, outputs, calls + + def test_unchanged_lockfile_reports_inherited_findings_with_one_scan(self): + result, report, outputs, calls = self.run_audit( + {"findings": [A]}, {"findings": [A]} + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("ctutils 0.4.3` (inherited)", report) + self.assertIn("blocking: 0", report) + self.assertNotIn("::error::", result.stderr) + self.assertEqual(outputs, "findings=true\n") + self.assertEqual(len(calls), 1) + + def test_changed_lockfile_uses_one_snapshot_and_allows_inherited_findings(self): + result, report, _, calls = self.run_audit( + {"findings": [A]}, {"findings": [A], "change": True} + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("(inherited)", report) + self.assertEqual(len(calls), 2) + self.assertNotIn("--no-fetch", calls[0]) + self.assertIn("--no-fetch", calls[1]) + self.assertEqual( + calls[0][calls[0].index("--db") + 1], calls[1][calls[1].index("--db") + 1] + ) + + def test_equal_counts_do_not_hide_replacement_or_version_changes(self): + for finding in (B, [*A[:2], "0.4.2"]): + with self.subTest(finding=finding): + result, report, _, _ = self.run_audit( + {"findings": [A]}, {"findings": [finding]} + ) + self.assertEqual(result.returncode, 1, result.stderr) + self.assertIn("blocking: 1", report) + + def test_fixed_current_base_blocks_a_vulnerability_from_an_old_branch(self): + result, report, _, _ = self.run_audit({"findings": []}, {"findings": [A]}) + self.assertEqual(result.returncode, 1, result.stderr) + self.assertIn("(blocking)", report) + self.assertIn("::error::RUSTSEC-2026-0385 ctutils 0.4.3", result.stderr) + + def test_annotations_escape_workflow_command_control_characters(self): + finding = [A[0], "crate%\r\n::warning::injected", "1.0.0"] + result, _, _, _ = self.run_audit( + {"findings": []}, {"findings": [finding]}, full=True + ) + self.assertEqual(result.returncode, 1) + self.assertIn( + "::error::RUSTSEC-2026-0385 crate%25%0D%0A::warning::injected 1.0.0", + result.stderr, + ) + self.assertNotIn("\n::warning::", result.stderr) + + def test_fingerprint_tracks_findings_not_order_or_database_and_warning_changes( + self, + ): + fingerprints = [] + for findings, warnings in ( + ([A, B], {}), + ([B, A], {"unmaintained": ["example"]}), + ([A], {}), + ): + result, report, _, _ = self.run_audit( + {"findings": []}, + {"findings": findings, "warnings": warnings}, + full=True, + ) + self.assertEqual(result.returncode, 1, result.stderr) + match = re.search(r"^Findings fingerprint: `([0-9a-f]{64})`$", report, re.M) + self.assertIsNotNone(match) + fingerprints.append(match[1]) + self.assertEqual(fingerprints[0], fingerprints[1]) + self.assertNotEqual(fingerprints[0], fingerprints[2]) + + def test_full_scan_blocks_findings_but_accepts_a_patch(self): + for findings, status in (([A], 1), ([], 0)): + with self.subTest(findings=findings): + result, _, outputs, calls = self.run_audit( + {"findings": []}, {"findings": findings}, full=True + ) + self.assertEqual(result.returncode, status, result.stderr) + self.assertEqual(outputs, f"findings={str(bool(findings)).lower()}\n") + self.assertEqual(len(calls), 1) + + def test_incomplete_scans_never_claim_a_clean_result(self): + # Every malformed report fails the same schema check, so one per mode + # suffices beyond the diff mode that tries them all. + cases = [ + {"raw": "not JSON"}, + {"raw": "{}"}, + { + "raw": json.dumps( + {"vulnerabilities": {"found": False, "count": 0, "list": {}}} + ), + "exit": 0, + }, + {"findings": [], "warnings": []}, + {"findings": [], "exit": 2}, + {"findings": [A], "exit": 0}, + ] + runs = [(broken, "diff") for broken in cases] + runs += [(cases[0], "full"), (cases[-1], "informational")] + for broken, mode in runs: + with self.subTest(broken=broken, mode=mode): + result, report, outputs, _ = self.run_audit( + broken if mode == "informational" else {"findings": []}, + broken, + full=mode == "full", + ) + self.assertEqual(result.returncode, 0 if mode == "informational" else 2) + self.assertIn("Scan incomplete", report) + self.assertEqual(outputs, "") + + def test_base_scan_errors_and_missing_bases_block(self): + for base, invalid in (({"raw": "broken"}, False), ({"findings": []}, True)): + with self.subTest(base=base, invalid=invalid): + result, report, outputs, _ = self.run_audit( + base, {"findings": []}, invalid_base=invalid + ) + self.assertEqual(result.returncode, 2) + self.assertIn("Scan incomplete", report) + self.assertEqual(outputs, "") + + def test_warnings_remain_visible_without_blocking(self): + result, report, _, _ = self.run_audit( + {"findings": []}, + {"findings": [], "warnings": {"unmaintained": ["example"]}}, + full=True, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("unmaintained", report) + + def test_large_issue_reports_are_bounded_without_losing_the_full_summary(self): + findings = [ + ["RUSTSEC-2026-9999", f"crate-{index}-" + "x" * 200, "1.0.0"] + for index in range(300) + ] + result, report, outputs, _ = self.run_audit( + {"findings": []}, {"findings": findings}, full=True + ) + self.assertEqual(result.returncode, 1, result.stderr) + self.assertLess(len(report), 65536) + self.assertIn("Report truncated", report) + self.assertEqual(outputs, "findings=true\n") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_skill_scripts.py b/tests/test_skill_scripts.py index 433f48984..d585ab47e 100644 --- a/tests/test_skill_scripts.py +++ b/tests/test_skill_scripts.py @@ -1314,6 +1314,30 @@ def test_ci_failure_names_the_gates_and_what_main_shares(self): # indent failed beside cargo-audit, so a rebase would not clear it. self.assertIs(found["stale_base"], False) + def test_ci_failure_reports_the_advisory_from_an_audit_annotation(self): + checks = [ + { + "name": "audit", + "state": "FAILURE", + "bucket": "fail", + "link": "https://github.com/sysprog21/codetrial/actions/runs/7/job/70", + } + ] + message = "RUSTSEC-2026-0385 ctutils 0.4.3" + result = run_fake( + VERIFY / "ci-failure.sh", + [5], + FAKE_GH_CI, + { + "FAKE_CHECKS": json.dumps(checks), + "FAKE_LOG": f"audit\tscan\t2026-10-22T00:00:00.0Z ##[error]{message}\n", + }, + ) + self.assertEqual(result.returncode, 0, result.stderr) + found = json.loads(result.stdout) + self.assertEqual(found["failing"][0]["errors"], [message]) + self.assertFalse(found["stale_base"]) + def test_an_audit_failure_main_has_fixed_is_a_stale_base(self): checks = [ { diff --git a/tests/test_test_gate.py b/tests/test_test_gate.py index 2606f4db3..6d9cd5ba0 100644 --- a/tests/test_test_gate.py +++ b/tests/test_test_gate.py @@ -168,7 +168,8 @@ def test_parallel_lanes_cover_the_default_gate(self): self.assertEqual(rust & browser, shared) self.assertEqual(portable & browser, shared) self.assertEqual(browser - shared, {"browser-tests"}) - self.assertEqual(rust | portable | browser, full) + self.assertEqual(rust | portable | browser | {"cargo-audit"}, full) + self.assertNotIn("cargo-audit", rust | portable | browser) self.assertEqual(self.run_lane()[1], results["all"][1]) def test_new_gate_is_included_in_portable_lane(self): @@ -221,27 +222,27 @@ def test_ci_refuses_skipped_gates(self): self.assertIn("failed gates: skipped-gates", result.stderr) def run_status(self, command, **env): + passing = dict.fromkeys(("GATES", "AUDIT", "PLAN", "MUTANTS"), "success") return subprocess.run( ["bash", "-e", "-c", command], - env={**os.environ, **env}, + env={**os.environ, **passing, **env}, capture_output=True, check=False, ).returncode def test_check_requires_the_lanes_and_explains_every_mutation_skip(self): check = support.job("check") - self.assertIn(" needs: [gates, plan-mutants, mutants]\n", check) + self.assertIn(" needs: [gates, audit, plan-mutants, mutants]\n", check) self.assertIn(f" if: ${{{{ !cancelled() && {NOT_AVATAR} }}}}\n", check) command = support.run_block(check) states = ("success", "failure", "cancelled", "skipped", "") - # The lanes are checked first, so one passing mutation answer stands in - # for the rest. - for gates in states: - with self.subTest(gates=gates): - status = self.run_status( - command, GATES=gates, PLAN="success", MUTANTS="success" - ) - self.assertEqual(status == 0, gates == "success") + # The lanes and audit are checked first, so one passing mutation answer + # stands in for the rest. + for lane in ("GATES", "AUDIT"): + for state in states: + with self.subTest(lane=lane, state=state): + status = self.run_status(command, **{lane: state}) + self.assertEqual(status == 0, state == "success") accepted = { # Shards ran and passed. ("success", "success"), @@ -253,16 +254,93 @@ def test_check_requires_the_lanes_and_explains_every_mutation_skip(self): for plan in states: for mutants in states: with self.subTest(plan=plan, mutants=mutants): - status = self.run_status( - command, GATES="success", PLAN=plan, MUTANTS=mutants - ) + status = self.run_status(command, PLAN=plan, MUTANTS=mutants) self.assertEqual(status == 0, (plan, mutants) in accepted) + def test_audit_dispatches_events_and_stops_on_git_or_scan_errors(self): + audit = support.job("audit") + command = support.run_block(audit) + self.assertIn("GROUP_BASE: ${{ github.event.merge_group.base_sha }}", audit) + self.assertIn("fetch-depth: 2", audit) + self.assertNotIn("Install cargo-audit", gates_job()) + parent = ["git:rev-parse HEAD^1"] + group = ["git:cat-file -e group-base^{commit}"] + fetch = "git:fetch --no-tags --depth=1 origin group-base" + for event, fail, expected_status, expected_calls in ( + ("push", "", 0, ["python:scripts/audit.py"]), + ( + "pull_request", + "", + 0, + parent + ["python:scripts/audit.py --base actual-parent"], + ), + ( + "merge_group", + "", + 0, + group + ["python:scripts/audit.py --base group-base"], + ), + ( + "merge_group", + "missing-base", + 0, + group + [fetch, "python:scripts/audit.py --base group-base"], + ), + ("merge_group", "fetch", 1, group + [fetch]), + ("pull_request", "rev-parse", 1, parent), + ("push", "scan", 2, ["python:scripts/audit.py"]), + ("unknown", "", 2, []), + ): + with self.subTest(event=event, fail=fail): + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + support.write_executable( + root / "git", + """#!/bin/sh +printf 'git:%s\\n' "$*" >> "$CALLS" +case "$1" in + rev-parse) [ "$FAIL" != rev-parse ] || exit 1; echo actual-parent ;; + cat-file) [ "$FAIL" != missing-base ] && [ "$FAIL" != fetch ] ;; + fetch) [ "$FAIL" != fetch ] ;; + *) exit 99 ;; +esac +""", + ) + support.write_executable( + root / "python3", + """#!/bin/sh +printf 'python:%s\\n' "$*" >> "$CALLS" +[ "$FAIL" != scan ] || exit 2 +""", + ) + result = subprocess.run( + ["bash", "-eo", "pipefail", "-c", command], + cwd=root, + env={ + **os.environ, + "PATH": str(root) + os.pathsep + os.environ["PATH"], + "CALLS": str(root / "calls"), + "EVENT": event, + "FAIL": fail, + "GROUP_BASE": "group-base", + }, + capture_output=True, + text=True, + timeout=60, + ) + self.assertEqual(result.returncode, expected_status, result.stderr) + calls = ( + (root / "calls").read_text().splitlines() + if (root / "calls").exists() + else [] + ) + self.assertEqual(calls, expected_calls) + def test_publishing_waits_on_the_lanes_not_on_check(self): # A push skips the mutation jobs `check` waits on, and the default # `success()` would carry that skip through `check` to both of these. - self.assertIn(" needs: gates\n", support.job("build")) - self.assertIn(" needs: [gates, build]\n", support.job("release")) + self.assertIn(" needs: [gates, audit]\n", support.job("build")) + self.assertIn(" needs: [gates, audit, build]\n", support.job("release")) def test_each_lane_has_a_bounded_job(self): timeouts = {lane: int(entry["timeout"]) for lane, entry in lanes().items()} @@ -369,7 +447,7 @@ def test_each_lane_installs_what_its_gates_need(self): set(matrix["portable"]["packages"].split()), {"shellcheck", "shfmt"} ) self.assertEqual(set(matrix["browser"]), {"lane", "timeout"}) - for name in ("ruff", "commentflow", "actionlint", "cargo-audit"): + for name in ("ruff", "commentflow", "actionlint"): with self.subTest(tool=name): self.assertEqual( condition(gates, f"Install {name}"), "matrix.lane == 'portable'" From bc54c23a94400a3ece24e7410ed8022b9a526a26 Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Fri, 23 Oct 2026 21:15:16 +0800 Subject: [PATCH 2/2] Count only audit comments posted since a close A comment whose follow-up edit failed was posted again on a closed tracking issue, because the retry skipped the comment lookup there to avoid matching comments from before the clean scan. Only bot comments newer than the issue's close now count, which covers both cases with one rule. The trusted-main test also checks the job's issue scope. --- .github/workflows/audit.yml | 18 +++++++----------- tests/test_audit.py | 30 ++++++++++++++++++++++++------ 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml index 893ffc62d..6dab2f13d 100644 --- a/.github/workflows/audit.yml +++ b/.github/workflows/audit.yml @@ -42,8 +42,8 @@ jobs: # Closed issues count too: reopening keeps existing subscribers. found=$(gh api --paginate --slurp --method GET "repos/$GH_REPO/issues" \ -f state=all -f 'creator=github-actions[bot]' -f per_page=100 \ - | jq -r --arg title "$title" '[.[][] | select(.pull_request == null and .user.login == "github-actions[bot]" and .title == $title)][0] // empty | "\(.number) \(.state)"') - read -r issue state <<< "$found" + | jq -r --arg title "$title" '[.[][] | select(.pull_request == null and .user.login == "github-actions[bot]" and .title == $title)][0] // empty | "\(.number) \(.state) \(.closed_at // "")"') + read -r issue state closed <<< "$found" if [ "$FINDINGS" != true ]; then # The clean body replaces the old fingerprint, so findings that # return later count as a change and notify subscribers again. @@ -60,16 +60,12 @@ jobs: current=$(fingerprint < "$RUNNER_TEMP/audit.md") previous=$(gh issue view "$issue" --json body --jq .body | fingerprint) # Keep the old fingerprint until notification succeeds so failures - # retry, but skip the comment if a failed edit left it posted. On a - # closed issue the last comment predates the clean scan, so recurring - # findings always notify. + # retry, but skip the comment if a failed edit left it posted. Only + # comments since a close count, so recurring findings notify again. if [ "$previous" != "$current" ]; then - notified= - if [ "$state" = open ]; then - notified=$(gh api --paginate --slurp "repos/$GH_REPO/issues/$issue/comments" \ - | jq -r '[.[][] | select(.user.login == "github-actions[bot]")] | last | .body // ""' \ - | fingerprint) - fi + notified=$(gh api --paginate --slurp "repos/$GH_REPO/issues/$issue/comments" \ + | jq -r --arg since "$closed" '[.[][] | select(.user.login == "github-actions[bot]" and .created_at > $since)] | last | .body // ""' \ + | fingerprint) if [ "$notified" != "$current" ]; then gh issue comment "$issue" --body-file "$RUNNER_TEMP/audit.md" fi diff --git a/tests/test_audit.py b/tests/test_audit.py index bc4ed0662..5b501d705 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -127,6 +127,10 @@ def test_only_trusted_main_runs_with_issue_write_access(self): tracking = support.step(job, "Track remediation") self.assertIn("steps.audit.outputs.findings != ''", tracking) self.assertIn("if: github.ref == 'refs/heads/main'", job) + self.assertIn( + " permissions:\n contents: read\n issues: write\n", job + ) + self.assertIn("\npermissions:\n contents: read\n\n", workflow) triggers = support.section( workflow, "\non:\n", "\npermissions:", "audit triggers" ) @@ -158,15 +162,18 @@ def test_tracking_filters_real_json_including_other_authors_prs_and_pages(self): @unittest.skipUnless(shutil.which("jq"), "jq is not installed") def test_tracking_notifies_only_when_findings_change_and_preserves_retry(self): issue = ISSUE - closed = {**issue, "state": "closed"} + closed = {**issue, "state": "closed", "closed_at": "2026-01-03T00:00:00Z"} unchanged = "Findings fingerprint: `same`\n" changed = "Findings fingerprint: `new`\n" bot = {"login": "github-actions[bot]"} - posted = [{"user": bot, "body": "Report\n" + changed}] + before = "2026-01-02T00:00:00Z" + after = "2026-01-04T00:00:00Z" + posted = [{"user": bot, "created_at": before, "body": "Report\n" + changed}] + reposted = [{**posted[0], "created_at": after}] stale = [ - {"user": bot, "body": "Report\n" + changed}, - {"user": bot, "body": "Report\n" + unchanged}, - {"user": {"login": "outsider"}, "body": changed}, + {"user": bot, "created_at": before, "body": "Report\n" + changed}, + {"user": bot, "created_at": before, "body": "Report\n" + unchanged}, + {"user": {"login": "outsider"}, "created_at": after, "body": changed}, ] notify = ["view", "comments", "comment", "edit"] for pages, findings, old, current, fail, comments, actions in ( @@ -212,7 +219,18 @@ def test_tracking_notifies_only_when_findings_change_and_preserves_retry(self): changed, "", posted, - ["view", "comment", "edit", "reopen"], + notify + ["reopen"], + ), + # A comment posted after the close, whose edit then failed, is not + # posted again. + ( + [[closed]], + "true", + "Clean report", + changed, + "", + reposted, + ["view", "comments", "edit", "reopen"], ), ([[issue]], "false", unchanged, "Clean report", "", (), ["edit", "close"]), ([[closed]], "false", unchanged, "Clean report", "", (), []),