fix(link-check,queue-monitor-liveness,tool-version-drift-check): restrict tracking-issue adoption to the workflow's own token author - #235
Conversation
The marker and title strings are public in workflow source, so any issue author could craft a decoy the lookup adopts, overwrites, or closes — or a duplicate decoy that trips the fail-closed ambiguity guard to suppress a real report. Filter candidates to the workflow token's own bot identity (github-actions[bot], type Bot) before marker and title matching, mirroring the standards-sync-stuck-automerge-alert lookup. The close path reuses the single lookup's resolved issue-number, so this covers both paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GsyNHuFaUowCwAyGWDT55N
…ng-issue adoption to own token author The marker and title strings are public in workflow source, so any issue author could craft a decoy the shared resolver adopts, overwrites, or closes — or a duplicate decoy that trips the fail-closed ambiguity guard to suppress a real report. Restrict candidates in find-tracking-issue.sh to issues authored by the workflow's own token identity (ISSUE_AUTHOR_LOGIN, a Bot) before marker and title matching, and pass github-actions[bot] from both generated-block consumers. ISSUE_AUTHOR_LOGIN is a required input, so an unset value fails closed rather than silently dropping the restriction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GsyNHuFaUowCwAyGWDT55N
Droppable process meta for the orchestrator to surface at PR time; not intended to land on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GsyNHuFaUowCwAyGWDT55N
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GsyNHuFaUowCwAyGWDT55N
|
Claude finished @kyle-sexton's task in 2m 8s —— View job PR Review
SummaryThis is a well-scoped security hardening PR. The author-restriction filter closes a real attack surface (crafted decoys abusing public marker/title strings) and the implementation is correct across all three consumers. No blocking issues. Core logic —
|
| Requirement | Status |
|---|---|
No pull_request_target / workflow_run triggers touched |
✓ |
All uses: remain SHA-pinned (no new uses: added) |
✓ |
| No secrets echoed or widened | ✓ |
ISSUE_AUTHOR_LOGIN is a non-secret env value, not a secret |
✓ |
| Fail-closed on missing input | ✓ |
| Filter applies before any marker/title comparison in all paths | ✓ |
Approved. No action required.
|
Claude finished @kyle-sexton's task in 2m 45s —— View job Security Review
Reviewed: No exploitable security vulnerabilities found. Rationale below. Author-filter correctnessThe filter is applied at the right place in both execution paths (marker match and title-fallback). An attacker who opens a decoy issue with the public marker string or known title is excluded before either
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GsyNHuFaUowCwAyGWDT55N
|
Claude finished @kyle-sexton's task in 1m 45s —— View job PR Review
This is a clean, well-scoped security hardening. No blocking issues.
|
| # | Case | Expected |
|---|---|---|
| 9 | Marker decoy by human user | empty (not adopted) |
| 10 | Marker decoy by different bot | empty (login mismatch) |
| 11 | Title decoy by human user | empty (not adopted) |
| 12 | Decoy cannot trip ambiguity guard | own issue still adopted |
| 13 | Unset ISSUE_AUTHOR_LOGIN |
non-zero exit |
Case 13 correctly expands run_unset to cover all four required inputs, each omitted in isolation while the other three are provided.
link-check-tracking-author.test.cjs (new) — 8 cases exercise the live github-script body extracted directly from link-check.yml via extractLookupScript(). The markerFor() helper replicates the exact SHA-256 algorithm used in the workflow, so the marker it generates is the real one the step would compute. Both positive controls (marker match, title-fallback match) and all decoy/ambiguity cases are covered.
One known fragility: extractLookupScript() anchors to "- name: Find existing tracking issue" and exactly 10 spaces of indentation. If the step name or YAML indentation changes, the extraction fails loudly at assert.notEqual(stepIndex, -1) — a loud failure, not a silent pass-through. This matches the established pattern in the repo and is acceptable as-is.
Workflow YAML changes
link-check.yml — ISSUE_AUTHOR_LOGIN is a YAML string literal ('github-actions[bot]'), not an expression, so it cannot be overridden by a workflow_call caller. The candidates filter is applied once and drives both the marker scan and the title-fallback; the close/recovery paths consume the issue-number output rather than doing their own lookup. ✓
queue-monitor-liveness.yml / tool-version-drift-check.yml — Both inline blocks are byte-identical regenerations of the updated source script; confirmed by visual inspection against the find-tracking-issue.sh diff. The ISSUE_AUTHOR_LOGIN env line and comment are present in each. ✓
Security / CLAUDE.md alignment
| Requirement | Status |
|---|---|
No pull_request_target / workflow_run triggers touched |
✓ |
All uses: remain SHA-pinned; no new uses: added |
✓ |
| No secrets echoed or widened | ✓ |
ISSUE_AUTHOR_LOGIN is a literal, not expression-derived |
✓ |
Fail-closed on missing input (bash :? and JS empty-string guard) |
✓ |
| Author filter applied before content matching in every path | ✓ |
Approved. No action required.
|
Claude finished @kyle-sexton's task in 2m 14s —— View job Security Review
Reviewed: No exploitable security vulnerabilities found. Reasoning below. Author-filter placement and completenessThe filter is applied in all four candidate-selection paths — bash marker path, bash title-fallback path, JS marker path, JS title-fallback path — before either matching condition is evaluated. There is no path where an attacker-controlled string (marker or title) is tested against an issue before the
|
…ompts already instruct (#319) ## Summary The default review prompt (`claude-review.yml`'s `prompt` input) tells the model "Use \`gh pr diff\` to identify what changed," but the default `claude-args` `--allowedTools` grants no Bash at all — only the inline-comment MCP tool. On a large PR the model has no sanctioned way to enumerate the diff. `claude-security-review.yml`'s default prompt gives the identical "use \`gh pr diff\`" instruction with the same MCP-only default allowlist, so the security lane hits the same denial. ## Verification Fetched run 30503910653 (melodic-software/provisioning#235, reusable at c136b27, v0.9.1): - The composed SDK options log the effective `allowedTools`: `Glob, Grep, LS, Read, mcp__github_comment__update_claude_comment, mcp__github_ci__get_ci_status, mcp__github_ci__get_workflow_run_details, mcp__github_ci__download_job_log, mcp__github_inline_comment__create_inline_comment, Bash(git add:*), Bash(git commit:*), Bash(<git-push.sh>:*), Bash(git rm:*)` — no `Bash(gh:*)` of any kind. - The result entry for that run records `"permission_denials_count": 4`, consistent with the model attempting and being denied tool calls the prompt instructed it to make. - Confirmed the prompt's own text is unchanged at `gh pr diff` in the current `main` default. - Confirmed via `anthropics/claude-code-action`'s own `docs/security.md` (line 18) that the upstream-recommended pattern for exactly this situation is a scoped grant like `Bash(gh issue view:*)` — not rewriting the prompt to use `git diff`, which would face its own problem: the checkout step uses `fetch-depth: 1` (single commit, no base ref available locally), so `git diff <base>...HEAD` would not work today either even if granted. ## Fix Added `Bash(gh pr diff:*)` to the default `claude-args` `--allowedTools` of both `claude-review.yml` and `claude-security-review.yml`, aligning each lane's grant with what its prompt already instructs (the security lane was this PR's own review finding — same root cause, fixed here rather than deferred). Read-only command, so it adds no exposure for a fork PR's read-only token (fork PRs aren't reviewed at all per this repo's `CLAUDE.md`). `claude-e2e-verify.yml` is deliberately untouched: its default `claude-args` sets no `--allowedTools` at all, a different grant model whose defaults this fix does not own. ## Test plan - `yq -e '.' .github/workflows/claude-review.yml` / `yq -e '.' .github/workflows/claude-security-review.yml` — valid YAML. - `node --test .github/scripts/claude-lane-incident.test.cjs .github/scripts/claude-lane-retry-gate.test.cjs .github/scripts/claude-review-superseded-guard.test.cjs` — 89/89 pass. ## Related - `melodic-software/provisioning`#235 (run 30503910653) — the review run whose composed SDK options supplied the `allowedTools` evidence above. - #310 / #320 — the companion prompt-side defect surfaced by that same review run. Fixes #309 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…tion as the review's own (#320) ## Summary A posted review (provisioning#235, run 30503910653) closed with: "No findings — this is an accurate, well-scoped correction backed by verified evidence (\`gh variable list\`, \`gh api orgs/.../actions/variables\`, and direct \`ci.yml\` reads). Nothing blocking." ## Verification - The reviewed run's effective `allowedTools` (confirmed in #309's verification) grants no `Bash(gh:*)` at all, so the review could not have run either `gh` command itself. - The PR body's own Test plan section (melodic-software/provisioning#235) reads verbatim: - `gh variable list --repo melodic-software/provisioning` → empty. - `gh api orgs/melodic-software/actions/variables` → no `CI_PESTER_ENABLED`. - The posted review's closing sentence names exactly those two commands as "verified evidence" it is claiming credit for, despite having no tool grant to run them. It restated the author's self-reported verification as its own. ## Fix Added one instruction to the default review prompt: never restate a PR author's own claimed verification as the review's own conclusion; label anything not independently run as author-claimed and unverified. Kept minimal — no other prompt or grant changes (the companion tool-grant gap is #309 / #319). ## Test plan - `yq -e '.' .github/workflows/claude-review.yml` — valid YAML. - `node --test .github/scripts/claude-lane-incident.test.cjs .github/scripts/claude-lane-retry-gate.test.cjs .github/scripts/claude-review-superseded-guard.test.cjs` — 89/89 pass. ## Related - `melodic-software/provisioning`#235 (run 30503910653) — the reviewed PR whose posted review restated the author's claimed verification as its own. - #309 / #319 — the companion tool-grant gap surfaced by that same review run. Fixes #310 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Closes #221
Summary
link-check.yml,queue-monitor-liveness.yml, andtool-version-drift-check.yml: candidates are filtered touser.login == ISSUE_AUTHOR_LOGIN && user.type == "Bot"BEFORE any marker/title matching, in every lookup path (close paths reuse the single filtered lookup). Marker/title strings are public in workflow source, so without this filter any issue author could craft a decoy that gets adopted (then closed on recovery) or a duplicate decoy that trips the fail-closed ambiguity guard to suppress a real alert — the pattern ported from the stuck-automerge alert (feat(standards-sync): arm auto-merge at PR creation and alert on stuck armed PRs #213).find-tracking-issue.sh; both generated consumer blocks are re-rendered and stay byte-identical (render-find-tracking-issue.cjs --checkgreen).ISSUE_AUTHOR_LOGINis a required input (:?), so an unset value fails closed rather than silently dropping the restriction; a null/missingusernever matches and never throws.tool-version-drift-check.ymlis a third consumer of the same shared script with the identical exposure, so the port covers it too (its diff is exactly the regenerated block plus the oneISSUE_AUTHOR_LOGINenv line — no pins, versions, or checksums touched).GITHUB_TOKEN(create-issue-from-file@v6.0.0 defaultstoken: ${{ github.token }}), and the live API confirmsusers/github-actions[bot]→type: "Bot", matching real tracking issues in this repo.Test plan
bash .github/scripts/find-tracking-issue.test.sh: all cases pass, including new decoy-by-marker (User), decoy-by-marker (other bot), decoy-by-title, decoy-cannot-trip-ambiguity, and unset-ISSUE_AUTHOR_LOGIN-fails-closed cases..github/scripts/link-check-tracking-author.test.cjs(executing harness — extracts and runs the inline lookup): 8/8 — decoy-by-marker, decoy-by-different-bot, decoy-by-title, decoy-cannot-suppress-real-report, genuine-double-match-still-fails-closed, PR-never-adopted, plus positive controls.node --test .github/scripts/*.test.cjs: 260/260.node .github/scripts/render-find-tracking-issue.cjs --check: both consumers byte-identical.shellcheckon the shared script + test: clean.actionlint(YAML/expr) on all three workflows: clean.Related
🤖 Generated with Claude Code