Skip to content

security(workflow-audit): report the unexplained, not the routine - #377

Merged
nedtwigg merged 7 commits into
mainfrom
security/workflow-audit-classify
Aug 16, 2026
Merged

security(workflow-audit): report the unexplained, not the routine#377
nedtwigg merged 7 commits into
mainfrom
security/workflow-audit-classify

Conversation

@nedtwigg

Copy link
Copy Markdown
Member

The problem

The audit filed an issue for every commit touching .github/workflows/. Across the 15 open [workflow-audit] issues, 18 commits were reported:

  • 9 were chore(deps): update github-actions — and 8 of those were the same pending Renovate PR, re-reported nightly because rebasing renovate/github-actions mints a new SHA and the audit deduped on SHA.
  • 6 were the nightly tend regen.
  • The rest were merged PRs that had already been reviewed.

Zero true positives in three months, and the empty signal was buried.

The fix

Report the unexplained. Two routine sources are classified and omitted, each on evidence that the credential this audit exists to watch cannot mint:

Renovate pin bumps — must carry a valid GitHub signature (verification.verified, reason: valid, author renovate[bot]) and change nothing but the ref of an already-referenced action. Renovate's commits are GPG-signed server-side by GitHub; a TEND_BOT_TOKEN pushing over git cannot produce one. The signature is load-bearing because %an is free text from git config, and the name-unchanged test is load-bearing because a pin-only diff can still repoint actions/checkout at evil/action.

tend regenerations — must reproduce byte-for-byte from uvx tend@<version> init at the version in the files' own generated header, run against that commit's own .config/tend.yaml. Identity is worthless here: TEND_BOT_TOKEN is exactly the credential in question, and SECURITY.md:46 names workflow-authoring by a compromised bot as the worst case this audit mitigates. Reproducibility is the only acceptable evidence.

Both classifiers fail open — any error, ambiguity, or unparseable input reports the commit.

Deliberately not done

  • Not skipping commits merged to main. Review is not proof, and SECURITY.md:46 names social-engineering an admin toward a merge as an accepted risk.
  • Not deduplicating by (branch, file set). That would let a benign change be reported once and a later force-push of malicious content to the same branch and files pass unremarked. Every commit is classified on its own content.

Incidental fix

git show --name-only reports nothing for a merge commit, which produced contentless reports and would have hidden an evil merge — content present in the result but in neither parent. Merges are now diffed against every parent and intersected, so a file taken wholesale from one side drops out and only what the merge itself introduced remains.

Verification

Ran end-to-end against the last 10 days — a window that previously produced 6 issues:

Explained (not reported):
- `1c80790` — chore: update tend workflows (0.1.12 → 0.1.14) (reproduces from the tend generator)
- `4c2d3a4` — chore(deps): update github-actions (signed Renovate pin bump)
- `7c49ef6` — chore: update tend workflows (0.1.16 → 0.1.17) (reproduces from the tend generator)
- `ad4d2d4` — chore: update tend workflows (0.1.14 → 0.1.15) (reproduces from the tend generator)
- `d844e6e` — chore: update tend workflows (0.1.15 → 0.1.16) (reproduces from the tend generator)

Unexplained: 0

Negative tests — each still reported:

Attack shape Result
tend regen with one curl exfil line added REPORTED
unsigned commit authored as a bot REPORTED
actions/checkout repointed to evil/action REPORTED
pin bump plus an injected run: line REPORTED

SECURITY.md

Refreshed the Audit visibility paragraph to describe the classification and its rationale, plus three items that today's #340 environment migration made stale:

  • Org-level secrets: BUILDCACHE_USER and NEXUS_USER were narrowed to selected visibility excluding this repo, so the accepted exposure no longer has to be accepted. The FAIL IF now admits none.
  • The environments FAIL IF now names the new tend environment.
  • New FAIL IF (flagging explicitly — happy to drop it): TEND_BOT_TOKEN / CLAUDE_CODE_OAUTH_TOKEN must be in the tend environment and absent at repo level, so today's migration can't silently regress.

Closes the 15 open [workflow-audit] issues.

🤖 Generated with Claude Code

The audit filed an issue for every commit touching .github/workflows/,
which in practice meant a near-daily issue about changes that had
already been reviewed. Half of all reported commits were one pending
Renovate PR, re-reported nightly because rebasing mints a new SHA.
Fifteen open issues, zero true positives.

Classify two routine sources and omit them, each on evidence the
credential this audit exists to watch cannot mint:

- Renovate pin bumps must carry a valid GitHub signature AND change
  nothing but the ref of an already-referenced action. The signature
  because `%an` is free text from git config; the name-unchanged test
  because a pin-only diff can still repoint actions/checkout at an
  attacker's action.
- tend regenerations must reproduce byte-for-byte from
  `uvx tend@<version> init` at the version in the files' own header.
  Identity proves nothing here — TEND_BOT_TOKEN is the credential in
  question — so reproducibility is the only acceptable evidence.

Both fail open: any error or ambiguity reports the commit.

Commits merged to main are still reported; review is not proof, and
SECURITY.md names social-engineering an admin toward a merge as an
accepted risk. Not deduplicated by branch or file set either, which
would let a benign change be reported once and a later force-push of
malicious content to the same files pass unremarked.

Also fixes a pre-existing blind spot: `git show --name-only` reports
nothing for a merge, which produced contentless reports and would have
hidden an evil merge. Merges are now diffed against every parent and
intersected, leaving only what the merge itself introduced.

When nothing is unexplained, no issue is filed — the run summary
carries the record. The SECURITY.md liveness check keys on a successful
run, not on an issue existing.

Verified against the last 10 days: 5 commits, all explained, 0 reported
(previously 6 issues). Negative tests confirm a tampered tend regen, an
unsigned commit, an action repointed to evil/*, and a pin bump with an
injected `run:` line are all still reported.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 15, 2026

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 280632f
Status: ✅  Deploy successful!
Preview URL: https://b5e34c09.mouseterm.pages.dev
Branch Preview URL: https://security-workflow-audit-clas.mouseterm.pages.dev

View logs

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The classification idea is right and the fail-open discipline is applied consistently. Two things about the evidence the classifiers rest on, both of which land inside the threat model this audit is written against (SECURITY.md:46TEND_BOT_TOKEN holds repo + workflow write).

1. is_tend_regen moves the trust boundary onto .config/tend.yaml, which nothing audits. The reproducibility proof runs the generator against that commit's own .config/tend.yaml, so a commit that edits the config and regenerates reproduces byte-for-byte by construction and is classified "explained". .config/tend.yaml is not a passive manifest — its values land verbatim in the generated YAML: bot_name appears inside GitHub expression strings (contains(github.event.issue.body, '@dormouse-bot') in tend-mention.yaml) and workflows.ci-fix.watched_workflows becomes the on: workflow_run: workflows: list in tend-ci-fix.yaml. So "reproducible" only implies "safe" if the upstream generator escapes its config inputs — an assumption about someone else's code, holding up a control that exists precisely because the bot can author workflows. Before this PR that commit was reported; now it isn't. Inline suggestion below closes it at zero false-positive cost — the regen commits in your verification window are version bumps with .config/tend.yaml untouched.

2. The Renovate signature is GitHub's, not Renovate's, so it doesn't carry the weight the comment gives it. The comment says the signature is "the load-bearing half" because .author.login resolves from a settable email. But checking the actual commit shows the signature is web-flow:

$ gh api repos/diffplug/dormouse/commits/4c2d3a4 --jq '{verified: .commit.verification.verified, reason: .commit.verification.reason, committer: .commit.committer, committer_login: .committer.login, author_login: .author.login}'
{"verified":true,"reason":"valid","committer":{"name":"GitHub","email":"noreply@github.com"},"committer_login":"web-flow","author_login":"renovate[bot]"}

GitHub applies that signature to anything created through its API, and the author name/email on such a call is caller-supplied. A token with workflow write can therefore create a commit under .github/workflows/ via the contents API with author.email set to 29139614+renovate[bot]@users.noreply.github.com, and it comes back verified: true, reason: "valid", .author.login == "renovate[bot]" — all three identity checks pass. I have not executed that against a real repo, so treat the mechanism as demonstrated and the end-to-end as unverified; but the metadata above is enough to say the signature is not evidence of Renovate authorship.

That leaves the name-unchanged content test as the only real control, and it does bound the damage to "same action, different ref" — the classifier isn't broken, but the stated basis for it is. Worth either correcting the claim in the workflow comment and the SECURITY.md paragraph, or adding a check that isn't mintable: gh api repos/$GITHUB_REPOSITORY/commits/$sha/pulls --jq '.[].user.login' returns renovate[bot] for a genuine bump, and PR authorship is assigned server-side from the authenticated identity, so TEND_BOT_TOKEN cannot forge it.

Third, smaller item inline: the ${REFS:-none — unreferenced commit} fallback can't fire as written — under set -euo pipefail the empty case aborts the step instead.

Unrelated to the code: I can't verify the BUILDCACHE_USER / NEXUS_USER claim from CI (the org secrets endpoint 403s on the workflow token), so the "every diffplug org secret is selected and none lists diffplug/dormouse" sentence is taken as given.

Holding approval on point 1 — that's my own read, not repo policy: it's a real reduction in what the audit reports, and whether delegating to .config/tend.yaml is acceptable is your call to make explicitly rather than mine to wave through.

Comment thread .github/workflows/workflow-audit.yaml
Comment thread .github/workflows/workflow-audit.yaml
Comment thread .github/workflows/workflow-audit.yaml Outdated
…the tend config

Review found the stated basis for the Renovate classifier was wrong.
The signature on a Renovate commit is GitHub's web-flow key
(committer.login == "web-flow"), applied to anything created through
the API with a caller-supplied author — it attests that GitHub made
the commit, not that Renovate did. Verified on 4c2d3a4. So a token
with `workflow` write could mint all three identity signals via the
contents API.

The content test was and remains the actual control. Says so now, in
both the workflow comment and SECURITY.md, and adds PR authorship —
assigned server-side, unforgeable by the caller, though a push onto an
existing renovate/* branch inherits that branch's PR, so it narrows
rather than proves.

Also guards the tend classifier against a commit that edits
.config/tend.yaml and regenerates: that reproduces byte-for-byte by
construction, which would make "reproducible" contingent on the
upstream generator escaping its config inputs. Probed both documented
paths against 0.1.17 — bot_name is rejected by username validation and
watched_workflows is correctly escaped into the workflow_run list — so
this is hardening rather than a live hole, but it removes an
assumption about someone else's sanitizer from a control that exists
because the bot can author workflows.

Third: the REFS assignment could abort the step. `git branch -a
--contains` exits 0 with no output for a commit reachable only from a
tag, making `grep -v` exit 1, which under `set -euo pipefail` fails the
run rather than falling through to the fallback — precisely on the
commit shape the fallback was written for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nedtwigg

Copy link
Copy Markdown
Member Author

All three taken — the second one was a straight error on my part. Pushed as 9233c8a.

1. Renovate signature. You're right, and I verified it before changing anything:

$ gh api repos/diffplug/dormouse/commits/4c2d3a4 --jq '{verified, reason, committer_login, author_login}'
{"verified":true,"reason":"valid","committer_login":"web-flow","author_login":"renovate[bot]"}

committer.login == "web-flow" — GitHub's key, applied to anything created through the API with a caller-supplied author. My comment claimed the signature was the load-bearing half; it attests that GitHub made the commit, not that Renovate did. Corrected in both the workflow comment and the SECURITY.md paragraph, and added your PR-authorship check.

One qualification on that check, now documented alongside it: PR authorship is unforgeable by the caller, but a token with repo write can push onto the existing renovate/github-actions branch and inherit its open PR. So it narrows the field rather than proving origin. The content test remains the only real bound, exactly as you said — and the residual it leaves is a ref the attacker controls inside that action's own repo, which is the trust every Renovate bump already rests on.

2. Config guard. Taken as suggested. I did probe whether it closes a live hole, and it doesn't — tend 0.1.17 defends both paths you named:

  • bot_nameError: bot_name '...' is not a valid GitHub username (only letters, digits, and hyphens)
  • watched_workflows → escaped into the list as "CI\"]\\n branches: [\"**\"]\\n #", contained inside the string

So this is hardening against an assumption, not a fix for an exploitable path. Worth keeping on your reasoning rather than mine: it costs nothing on real regen commits (version bumps leave the config untouched), and a control that exists because the bot can author workflows shouldn't rest on someone else's sanitizer continuing to behave across releases. Flagging the distinction so the commit message doesn't overstate what was found.

3. set -euo pipefail on REFS. Confirmed and fixed with || true. Good catch — it would have failed the run on exactly the commit shape the fallback exists for.

On the org-secret sentence you couldn't verify: correct that the workflow token 403s there. Verified from an admin session — all ten diffplug org secrets are visibility: selected and none lists diffplug/dormouse. BUILDCACHE_USER and NEXUS_USER were all until today; that's what the SECURITY.md edit records.

Re-ran the full matrix on the final code: genuine regen and Renovate bump still skip; regen-plus-exfil-line, config-edit-plus-regen, action repointed to evil/*, pin bump with an injected run:, and an unsigned bot commit all report. The 10-day window is still 5 explained, and it now correctly reports one unexplained commit — my own edit to the audit workflow.

@dormouse-bot

Copy link
Copy Markdown
Collaborator

Confirmed on 1 and 3. The web-flow correction is right, and the qualification you added on PR authorship is the accurate framing — unforgeable by the caller, but inheritable by pushing onto the existing renovate/* branch, so it narrows rather than proves. || true closes the REFS abort.

On 2 the guard doesn't hold as written, and my suggestion didn't say enough: splitting the edit across two commits walks past it.

Commit A edits .config/tend.yaml and nothing else. It never enters the audit at all — COMMITS is git log --all ... -- .github/workflows/, so a commit touching no workflow file isn't in the window. Commit B then regenerates, touching only tend-*.yaml. The new guard asks whether B touched the config; it didn't, so it passes — and the worktree B reproduces in is checked out at B, which already contains A's config. Byte-for-byte reproduction, classified explained. The guard catches the single-commit shape and only that.

Closing it looks like widening the window rather than adding a check: put .config/tend.yaml in the pathspec at L72 and in both own_changes branches (L201, L205), so A is an auditable commit reported on its own content. Both classifiers already fail open on it — a config-only commit leaves files and changed empty — so widening the window is the whole change. The same gap is why an edited config sits latent even with no attacker-authored regen: the next legitimate nightly regen picks it up, and that commit is explained too.

Separately, the file header still describes the classifiers as resting "on evidence an attacker cannot mint" and names GitHub-signing as the Renovate half (L13–L18) — the claim this commit retracts 70 lines below, and it doesn't mention the PR-authorship check or the config guard. The skip label (signed Renovate pin bump) at L226 leans on the same retracted word.

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config guard and the corrected Renovate rationale both land as intended, so my earlier hold is resolved — the reproducibility proof is self-contained again, and the PR-authorship check plus the "narrows the field, doesn't prove origin" qualification is the right characterization. I verified the new gate returns what the code expects on the real commits: gh api repos/diffplug/dormouse/commits/4c2d3a4/pullsrenovate[bot], and the two tend regens → dormouse-bot, so the Renovate classifier still admits genuine bumps and the regen path is unaffected by it.

Two non-blocking items, neither a reason to hold.

The file-header comment still carries the claim this push corrected. Lines 12–22 are the first thing a reader of this workflow sees, and they still say the two sources are skipped "each on evidence an attacker cannot mint" with "Renovate pin bumps — GitHub-signed (verification.verified)" as the first bullet — which is exactly what the body comment 70 lines down now spends 20 lines refuting, and SECURITY.md no longer says either. The tend bullet is also missing the new .config/tend.yaml-untouched requirement, so the header describes a classifier that's now strictly narrower than what it claims. Inline suggestion below.

permissions: doesn't grant pull-requests: read, and the new PR-authorship call is the only thing in this workflow that needs it. The block declares exactly the three scopes the other API calls use (contents: read for /commits/{sha}, actions: read for the workflow-runs query, issues: write for gh issue create) — pull-requests was never needed until this push added /commits/{sha}/pulls. If the scoped github.token 403s there, gh api fails, is_renovate_pin_bump returns 1, and every Renovate bump gets reported again — the fail-open direction is the safe one, but it silently undoes the PR's main effect until someone reads a nightly summary. I could not verify which way it goes: a GITHUB_TOKEN with pull-requests unset may still read public data on a public repo, and I have no way to exercise a scope-restricted token from this session. Declaring it costs nothing either way:

permissions:
  contents: read
  issues: write
  actions: read
  pull-requests: read

That's outside the diff (the permissions: block is unchanged in this PR), so no inline suggestion — say the word and I'll push it.

Also checked, no finding
  • The new awk -F'\t' '$1 == "true"' drops the separate reason = "valid" test, but verification.verified is only true when reason is valid, so no behavior change; login stays empty on false and the classifier returns 1.
  • Both gh api calls keep their exit status: login/pr_authors are declared in the local line and assigned separately, so || return 1 sees the pipeline's status under pipefail rather than local's.
  • The new .config/tend.yaml guard uses plain git show --name-only, which reports nothing for a merge — but is_tend_regen already bails at [ -n "$files" ] || return 1 on the same call for .github/workflows/, so a merge never reaches the guard. Fine as written; the dependency is just non-obvious.
  • if length == 0 then "" and a multi-author join(",") both land on the reported side.

Comment thread .github/workflows/workflow-audit.yaml Outdated
…s: read

Two follow-ups from review, both correct.

The file-header comment still described the classifiers as resting on
"evidence an attacker cannot mint" with the GitHub signature as the
first bullet — the claim the previous commit spent twenty lines
refuting further down and removed from SECURITY.md. It's the first
thing a reader of this workflow sees. Now describes the content test as
the control and names the .config/tend.yaml requirement the tend bullet
had gained.

`permissions:` never granted `pull-requests`, and declaring the block
sets every unlisted scope to `none`. The PR-authorship call added in
the previous commit reads /commits/{sha}/pulls, so a 403 there would
make is_renovate_pin_bump fail open, report every Renovate bump, and
silently undo this PR's main effect — visible only to whoever reads a
nightly run summary. `pull-requests: read` is within the permission
ceiling SECURITY.md sets for agent-managed workflows (any read).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nedtwigg

Copy link
Copy Markdown
Member Author

Both taken — pushed. The header was describing a classifier two commits out of date, and it's what a reader hits first; it now leads with the content test and carries the .config/tend.yaml-untouched requirement.

On pull-requests: read: you were right to flag that you couldn't determine which way it goes, and I can't exercise a scope-restricted token from here either. Declaring it removes the question rather than resolving it — which is the right trade when the failure mode is silent. A 403 there fails open, reports every Renovate bump, and undoes this PR's entire effect while looking like a working audit; nobody would notice until they read a nightly summary and wondered why Renovate was back. It's also within the permission ceiling SECURITY.md sets for agent-managed workflows (any read), so it costs nothing on that front.

Thanks for the four no-finding checks in the details block — the local-vs-assignment exit-status one in particular is the kind of thing that would have been a silent always-true had I written it the other way.

@dormouse-bot

Copy link
Copy Markdown
Collaborator

Header and permissions both read right at 84003ef — the header now leads with the content test and carries the config-untouched requirement, and declaring pull-requests: read explicitly is the better call than leaving it to a 403 that fails open into "report every bump" while still looking like a working audit.

The one item that doesn't appear in that push is the split-commit gap from my last comment — I think "both" landed on the two items from the review rather than that one. 84003ef touches only the header block and the permissions: entry, and the window is unchanged: COMMITS is still -- .github/workflows/, so commit A editing only .config/tend.yaml never enters the audit, and commit B regenerating from it leaves the config untouched, passes the L173 guard, and reproduces byte-for-byte against the config A already landed. The header's new .config/tend.yaml-untouched sentence describes the guard accurately; it's the window, not the guard, that the split walks past.

It stays a pathspec change in three places rather than a new check — L82 and both own_changes branches (L210, L214) — because both classifiers already fail open on a config-only commit and a genuine regen still classifies under the wider window.

Why widening alone is sufficient, traced through both classifiers

Commit A (.config/tend.yaml only) once it's in the window:

  • own_changes.config/tend.yaml, so FILES is non-empty and the contentless-merge continue doesn't swallow it.
  • is_renovate_pin_bump → its diff is scoped to -- .github/workflows/, so changed is empty and L131 returns 1.
  • is_tend_regen → its files is likewise empty and L157 returns 1.

So A is reported on its own content, which is the point — the config edit is auditable whether or not an attacker-authored regen ever follows it. A legitimate nightly regen is unaffected: its own_changes are tend-*.yaml only, the config is untouched, and both the L158 filename filter and the L173 guard see exactly what they see today. A combined config-plus-regen commit is still caught by L173 as before.

Not verified end-to-end by running the audit — this is read from the script at 84003ef, not an executed run.

Minor, from the same comment and also unpushed: the skip label at L235 still prints (signed Renovate pin bump), resting on the word the rest of the PR retracted.

If you weighed the split shape and decided it's acceptable — the config is a reviewed file and the regen it feeds is reported the moment anything else drifts — that's a call worth making explicitly rather than one I should keep re-raising; say so and I'll drop it.

The config guard added two commits ago catches only the single-commit
shape. Split across two commits it does nothing: commit A edits
.config/tend.yaml and nothing else, so it never enters the audit at all
— the window is `git log --all -- .github/workflows/`. Commit B then
regenerates, touching only tend-*.yaml, leaves the config untouched,
passes the guard, and reproduces byte-for-byte against the config A
already landed.

The fix is the window, not another check. .config/tend.yaml joins the
pathspec in three places — the commit list and both own_changes
branches — so the config edit is an auditable commit reported on its
own content. Both classifiers already fail open on it: each scopes its
own diff to .github/workflows/, which comes back empty for a
config-only commit. A genuine regen is unaffected, and the same-commit
guard still covers the combined shape.

Verified by constructing the split on a real branch: the config commit
is now reported with .config/tend.yaml in its file list, where before
this change neither commit appeared.

Also drops "signed" from the Renovate skip label, which rested on the
word this PR retracted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nedtwigg

Copy link
Copy Markdown
Member Author

You're right that "both" landed on the two review items and skipped the split-commit gap — that was me miscounting, not a decision. Taken now in 7793ec5, and it's not a shape I'd want to accept: the whole point of the reproducibility proof is to be self-contained, and a two-commit split makes it depend on a file nothing looked at.

Applied as the pathspec widening you described, in all three places — the commit list plus both own_changes branches — rather than another check. Also dropped signed from the skip label.

Verified end-to-end rather than by reading, since your trace was explicitly unexecuted. Built the split on a real branch (so git log --all sees it): commit A edits .config/tend.yaml to add an entry to watched_workflows, commit B runs uvx tend@0.1.17 init and touches only tend-ci-fix.yaml. Before this change neither commit appeared in the audit. After:

### `d4dfa5b` — chore: tidy tend config
- **Files:**
  - `.config/tend.yaml`

Explained (not reported):
- `8b1d038` — chore: update tend workflows (reproduces from the tend generator)

Which is the design working as you laid it out — B still classifies as reproducible, and that's fine, because A is now the auditable event. Your trace through both classifiers holds: A's own diff scoped to .github/workflows/ is empty, so is_renovate_pin_bump returns 1 at the changed check and is_tend_regen at the files check, and own_changes reports it on the config file itself. The five genuine commits in the window still classify as explained.

Worth recording that the guard from the previous push isn't redundant now — it still covers the combined config-plus-regen commit, which enters the window on its workflow files and would otherwise pass the filename filter.

One thing from earlier in this thread that I should state plainly, since it bears on how much the config path was ever worth: I probed both injection vectors you named against 0.1.17 and neither lands. bot_name is rejected by username validation, and watched_workflows is escaped into the workflow_run list as "CI\"]\\n branches: [\"**\"]\\n #". So this is hardening against an assumption about the upstream generator, not a live hole — which I think is still the right call for a control whose premise is that the bot can author workflows, but the distinction belongs in the record rather than being left implied.

@dormouse-bot

Copy link
Copy Markdown
Collaborator

The widening lands, and I checked it by running rather than reading this time — a config-only commit is reported on its own content, a genuine pin bump is untouched, and the same-commit guard still covers the combined regen shape. The distinction you drew at the end is the right one to have in the record: hardening an assumption about the upstream generator is a different claim than closing a live hole, and a control whose premise is "the bot can author workflows" shouldn't rest on someone else's escaping either way.

One asymmetry the widening leaves behind, in the other classifier. is_tend_regen has an explicit same-commit config guard; is_renovate_pin_bump has none, and its content test still scopes its diff to .github/workflows/ alone. So a commit that is a clean pin bump plus an arbitrary .config/tend.yaml edit now enters the window — own_changes lists the config — and then classifies as explained, because the half of the diff that carries the config never reaches the uses:-only test at L147. Same latency as the split-commit shape: the config sits unreported until the next legitimate regen picks it up, and that commit is explained.

The identity checks don't bound this one, by this PR's own account — the web-flow signature attests GitHub made the commit, .author.login resolves from a settable email, and PR authorship is inherited by pushing onto the existing renovate/* branch. The content test is the only bound, and its scope is now narrower than the window it runs inside.

It's the same shape of fix as the last one — the pathspec at L139 gains .config/tend.yaml, and the config's lines become non-uses: changed lines that fail the test at L147 and fail open. No new check, and the header bullet at L23–L25 would want the same one-word widening.

Executed: three cases against the content test at `7793ec5`, current pathspec vs. widened

Built each commit on a real branch in a worktree at 7793ec5 and ran the content half of is_renovate_pin_bump verbatim (the identity half needs the API, so it's stubbed out — it's the half this PR already declares non-conclusive).

Commit Current (.github/workflows/) Widened (+ .config/tend.yaml)
actions/checkout ref bump + a line appended to .config/tend.yaml passes → skipped as pin bump fails → reported
actions/checkout ref bump alone (genuine) passes → skipped passes → skipped (no regression)
.config/tend.yaml alone fails → reported fails → reported (fail-open holds)

The mixed commit's own_changes at 7793ec5 is:

.config/tend.yaml
.github/workflows/workflow-audit.yaml

so it is in the window and has attributable files — it's the classifier, not the window, that lets it through. is_tend_regen is unaffected in all three: the L186 guard already returns 1 on any commit touching the config, which is why only the Renovate arm has this gap.

If you'd rather bank the PR as-is and take this separately, that's reasonable — it's a strictly narrower shape than the one you just closed, and everything else here reads right to me.

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on 7793ec5. It doesn't unsettle the approval — the widening is the right shape and the split-commit gap is closed — but putting .config/tend.yaml in the window makes it reachable by the other classifier, and that arm never looks at it.

is_renovate_pin_bump scopes its content test to .github/workflows/ (diff=), so a commit that bumps a pin and edits .config/tend.yaml classifies as explained: the workflow-side diff is uses:-only with matching action names, the classifier returns 0, and the own_changes list — which now correctly includes .config/tend.yaml — is thrown away by the continue. The config edit is never reported, and the next legitimate nightly regen then reproduces byte-for-byte against it and is explained too. Same shape you just closed for the regen arm, arriving through the Renovate arm instead.

It's a residual the PR's own reasoning stops covering at exactly this commit: "what bounds the damage is that the diff can express nothing but a new ref for an already-referenced action" held while the config was out of scope. Now the diff can also express arbitrary .config/tend.yaml content, and the only thing in front of it is the identity trio this PR documents as narrowing rather than proving — author caller-supplied through the contents API, PR authorship inheritable by pushing onto the existing renovate/* branch.

Inline suggestion is the one-line guard is_tend_regen already carries, mirrored into the Renovate arm. Renovate never touches .config/tend.yaml, so it costs nothing on real bumps.

Verified by execution, not by reading

Scratch repo, both functions lifted verbatim from 7793ec55, gh stubbed to return what a genuine-looking Renovate commit returns (verification.verified: true, .author.login: renovate[bot], PR author renovate[bot]) — i.e. the identity checks all pass, which is the assumption the PR already argues can't be relied on.

Commit under test bumps actions/checkout@aaaaaaa@bbbbbbb and flips bot_name: dormouse-botbot_name: evil-bot in .config/tend.yaml.

own_changes (what the window now sees):
  .config/tend.yaml
  .github/workflows/ci.yaml
RESULT: classified EXPLAINED -> skipped, .config/tend.yaml edit never reported

With the suggested guard added, and a third commit that is a clean pin bump with the config untouched:

411f028 (pin bump + config edit) -> REPORTED
381872f (clean pin bump)         -> EXPLAINED (skipped)

So the guard closes it with no effect on genuine bumps.

Minor, second suggestion: the issue body still says the commits touch .github/workflows/, so a config-only commit — the new case this PR exists to catch — gets reported under a sentence that doesn't describe it. The two No workflow file changes since lines further up have the same staleness, but those are log and step-summary text rather than the artifact a human opens.

Comment thread .github/workflows/workflow-audit.yaml
Comment thread .github/workflows/workflow-audit.yaml Outdated
Widening the window in the previous commit made .config/tend.yaml
reachable by both classifiers, but only is_tend_regen refuses it. So a
commit that is a clean pin bump *and* edits the config classified as
explained: the workflow-side diff is uses:-only with matching action
names, the classifier returns 0, and own_changes — which correctly
lists the config — is discarded by the continue. Same shape the
previous commit closed for the regen arm, arriving through the other
one.

The identity checks don't bound it, by this PR's own account: the
signature is web-flow, the author is caller-supplied, and PR authorship
is inherited by pushing onto the existing renovate/* branch. The
content test was the only bound, and its scope was narrower than the
window it ran inside.

Mirrors the one-line guard is_tend_regen already carries. With both
arms refusing it, any commit touching the config must be reported by
one of them — that pairing is the invariant, and there is no third
classifier to fall out of step with the window.

Verified: a clean pin bump still classifies as explained, a pin bump
carrying a config edit is reported, and both genuine commits in the
repo (4c2d3a4, 7c49ef6) are unaffected.

Also refreshes the issue-body, log, and step-summary text, which still
described the window as .github/workflows/ alone — the issue body being
the one a human actually opens.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nedtwigg

Copy link
Copy Markdown
Member Author

Both taken in 4843794 — and this time I checked what my own fix invalidated before pushing, which is the failure mode the last few rounds have had in common.

Verified the guard with the identity trio stubbed to pass, since that's the assumption the finding rests on:

clean pin bump           -> EXPLAINED (skipped)
pin bump + config edit   -> REPORTED

Genuine 4c2d3a4 and 7c49ef6 both still classify as explained, so no regression on real traffic.

Refreshed the issue-body sentence you flagged, plus the two No workflow file changes lines — you were right that those are log and summary text rather than the artifact a human opens, but leaving them stale is how the next reader learns the window from a sentence that's wrong.

One thing I fixed that wasn't in your review: the comment above COMMITS still justified the widening with "both classifiers already fail open on it, since each scopes its own diff to .github/workflows/ and comes back empty." True when written, false the moment either arm gained an explicit guard. It now states the actual invariant — both arms refuse any commit touching the config, so nothing in the widened window can be swallowed by an arm that doesn't inspect it.

That invariant is why I think this closes the class rather than the instance. The last two findings were the same defect reached through different arms: the window covers two paths while each classifier derives its own diff scoped to one. With both arms carrying the guard, a config edit must surface, and there is no third classifier to fall out of step. The structural version — having both classifiers consume the already-computed own_changes instead of re-deriving — would make that hold by construction rather than by two matching guards, but it's a refactor of both functions and the invariant holds without it. Worth doing only if this ever grows a third arm.

Not merging on the approval; nedtwigg has it.

@dormouse-bot

Copy link
Copy Markdown
Collaborator

Confirmed at 4843794, executed rather than read — three commits built on real branches in a worktree at that SHA, with the identity trio stubbed to pass since that's the assumption the finding rested on. Clean pin bump → explained; pin bump carrying a .config/tend.yaml edit → reported; config alone → reported. All three enter the window.

The invariant is the right thing to have stated in the comment above COMMITS, and it does hold at this SHA: the window is one pathspec, both arms refuse any commit touching the config before deriving their own diff, and there are exactly two arms. Your read of the refactor is the one I'd make too — consuming own_changes instead of re-deriving would make it hold by construction, but two matching one-line guards is not where that cost is worth paying until a third classifier exists.

Executed: three cases against is_renovate_pin_bump at 4843794

Identity half (the two gh api calls) stubbed to pass; everything from the config guard down is verbatim from the file.

case 1: clean pin bump
    .github/workflows/workflow-audit.yaml
  -> EXPLAINED (skipped as pin bump)
case 2: pin bump + .config/tend.yaml edit
    .config/tend.yaml
    .github/workflows/workflow-audit.yaml
  -> REPORTED
case 3: .config/tend.yaml alone
  -> REPORTED

And git log --all --pretty='%h %s' -- .github/workflows/ .config/tend.yaml lists all three, so none of them is passing by falling out of the window:

6e592325 chore(deps): update github-actions      (case 2)
ebb8e18b chore: tidy tend config                 (case 3)
a0b547a6 chore(deps): update github-actions      (case 1)

Merge commits stay fail-open on both arms under the widened pathspec, which is worth recording since the guard uses the same git show --name-only that reports nothing for a merge: the guard passes vacuously, but is_renovate_pin_bump then hits [ -n "$changed" ] || return 1 on an empty diff and is_tend_regen hits [ -n "$files" ] || return 1, so a merge can only ever reach the report path — where own_changes gives it the parent-intersected file list.

The distinction you put in the record at the end of the last round is the part I'd keep: hardening an assumption about the upstream generator is a different claim than closing a live hole, and the reason to do it anyway is that the control's premise is "the bot can author workflows." Nothing further from me — approval stands, and the merge is yours.

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My earlier review pushed you the other way on this, so I checked the new claim against GitHub's own documentation instead of taking it as read — it holds. About commit signature verification states that bot signature verification "will only work if the request is verified and authenticated as the GitHub App or bot and contains no custom author information, custom committer information, and no custom signature information, such as Commits API." So a valid signature with committer.login == "web-flow" does exclude a caller-supplied author, and the pair is a provenance control as the comment now says.

One non-blocking prose nit inline: the sentence that gets there — "The REST paths that do allow those fields to be supplied do not add GitHub's signature; they require the caller to supply one" — is right about the Git Commits API (POST /git/commits, which takes a signature) but not about PUT /contents/{path}, which has no signature parameter and does come back GitHub-signed for a bot token when no author or committer is supplied. What actually closes the forgery is the custom-author rule above, not the absence of signing on those paths. The same sentence carries into the SECURITY.md paragraph ("while REST paths that permit those fields require the caller to supply the signature"), which is one long line, so no suggestion there — same swap.

What I ran against the real commits

Ran the new gate verbatim against 4c2d3a4 (chore(deps): update github-actions):

$ gh api "repos/diffplug/dormouse/commits/4c2d3a4" --jq '[.commit.verification.verified, .commit.verification.reason, (.author.login // ""), (.committer.login // "")] | @tsv'
true	valid	renovate[bot]	web-flow
→ awk prints: renovate[bot]
content test: 14 changed lines, all `uses:`, removed/added action-name sets equal → classified explained

That also covers the thing the awk depends on but nothing asserts: gh's embedded jq renders the boolean as the literal true in @tsv, so $1 == "true" matches. It would fail closed (report every bump) if it didn't, but it doesn't.

The committer.login == "web-flow" tightening doesn't false-report genuine bumps: every renovate[bot]-authored commit touching .github/workflows/ in this repo's history is committed by GitHub <noreply@github.com>4c2d3a4, ccb68b3, ef53105, 281e1fe, aa4301f, d27673c, 10c6484, 341a106 — consistent with Renovate's createCommitOnBranch path.

Comment thread .github/workflows/workflow-audit.yaml Outdated
Co-authored-by: dormouse-bot <ned.twigg+dormouse-bot@diffplug.com>
@nedtwigg
nedtwigg merged commit 31fef35 into main Aug 16, 2026
3 checks passed
@nedtwigg
nedtwigg deleted the security/workflow-audit-classify branch August 16, 2026 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants