Repository navigation
fix(ci): harden report and core workflows - #6904
Conversation
3a8ce67 to
3ae3eb2
Compare
This fix(ci) PR landed without a changelog.d/ entry, unlike the sibling ci-injection-prevention fixes that came before it.
The two-sentence comment explaining the shellcheck scope split was hard-wrapped mid-sentence, which the project's prose-wrapping convention forbids for new comment text.
|
|
||
| if [ -n "$CAT_ID" ]; then | ||
| BODY=$(cat /tmp/report.md | jq -Rs .) | ||
| BODY=$(jq -Rs . < /tmp/report.md) |
There was a problem hiding this comment.
A few lines below this (the title: \"Weekly Report — ${{ steps.report.outputs.week }} ~ ${{ steps.report.outputs.today }}\" line in the GraphQL mutation), the diff still splices ${{ steps.report.outputs.week }}/.today directly into the run: script text. That's the exact pattern this PR hardens everywhere else — BASE_REF and CRATES in ci.yml, and the owner/name GraphQL variables right above this line in this same file.
Not exploitable today, since week/today come from date -u ... +%Y-%m-%d and are always [0-9-] — but the safety is incidental to that formatting, not structural. For consistency with the rest of this PR, consider passing week/today as bound GraphQL variables (-f week="$WEEK" -f today="$TODAY", $week: String! in the query) the same way owner/name are handled above, rather than string-splicing them into the mutation source. Leaving as a comment rather than fixing directly since it changes the query shape, not a pure mechanical swap.
Generated by Claude Code
| - name: Install actionlint | ||
| run: | | ||
| mkdir -p "$RUNNER_TEMP/actionlint-bin" | ||
| GOBIN="$RUNNER_TEMP/actionlint-bin" go install github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 |
There was a problem hiding this comment.
Supply-chain consistency nit: this PR pins the trufflehog installer to an immutable commit SHA a few lines above specifically because a mutable ref (main) is a rewritable supply-chain vector, and every actions/checkout in the repo is pinned by commit SHA rather than tag for the same reason. go install .../actionlint@v1.7.7 pins to a mutable git tag instead — Go's module proxy/GOSUMDB does give tamper-evidence once a version is first fetched, so this is materially safer than the curl main | sh pattern being fixed, but it's not the same guarantee as a SHA pin, and it's inconsistent with the standard this PR is otherwise applying in this file. Leaving as a comment rather than fixing directly — worth a maintainer call on whether to pin actionlint by its release commit SHA (go install github.com/rhysd/actionlint/cmd/actionlint@<sha>) for parity, or whether the module-proxy checksum guarantee is considered sufficient here.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Reviewed the workflow diff and the new test_workflow_report_hardening.py regression suite (ran it locally with actionlint v1.7.7 installed — all 7 tests pass, including the actionlint check against both smaller workflows and ci.yml).
The security-relevant fix (routing needs.changes.outputs.crates and github.base_ref through env: instead of direct run: interpolation, plus tightening the crate-name-extraction regex to [A-Za-z0-9_-] so a maliciously-named changed path can no longer produce a shell-metacharacter-bearing "crate" name in the first place) looks correct and closes the actual script-injection vector at both the source and the consumption sites. if: conditions on no-build-artifacts/quality/etc. are unchanged in their gating logic — only timeout-minutes: was added — so nothing is being accidentally skipped. The /to-issue startsWith + exact-token validation step correctly narrows the substring-match footgun without introducing a new interpolation risk (the untrusted comment body goes through env: COMMENT_BODY before the shell script touches it).
One finding left as an inline comment on scripts/tests/test_workflow_report_hardening.py: the branch's tip commit has an AI author identity, which is exactly what the project's attribution policy is meant to prevent.
Generated by Claude Code
There was a problem hiding this comment.
Commit 262f33e0626608deb770f87b131d08e344f5ff77 ("docs: rewrap CI hardening test comment to sentence boundaries") on this branch has author identity Claude <noreply@anthropic.com>.
That's exactly the case CLAUDE.md's commit-msg hook is meant to reject ("separately rejects a commit whose author identity ... resolves to Claude / Anthropic even when the message itself is clean") — it evidently landed without the hook active. As-is it will show up verbatim in the PR's Commits tab and in main's permanent history if this merges as a merge commit rather than a squash.
Separately (and a little ironic given what the commit fixes), that same commit's message body is itself hard-wrapped at ~68 chars across three lines instead of one-sentence-per-line, which is the exact prose-wrapping violation the project's CLAUDE.md forbids for new prose.
I can't fix either without rewriting this branch's history (amending/rebasing the tip commit means a force-push), which is outside what I'm allowed to do here. Recommending: squash-merge (which discards the intermediate commits' authorship) or rebase-fix the author identity/message before merging. The code content itself is fine — this is purely a git-metadata issue.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Daily automated pass over the current head (262f33e0626608deb770f87b131d08e344f5ff77).
No automated-review marker for this sha was present yet, so this ran as a full pass rather than a skip.
Blocking — still open: commit 262f33e0626608deb770f87b131d08e344f5ff77 itself ("docs: rewrap CI hardening test comment to sentence boundaries") is authored as Claude <noreply@anthropic.com>.
That is precisely the identity CLAUDE.md's commit-msg hook rejects, and it landed on this branch without the hook active.
This is unresolvable via this tool — it requires a maintainer rebase (author-identity rewrite) and force-push, both of which are outside what an automated reviewer should do.
Recommend squash-merge (drops the intermediate authorship) or a manual rebase before merging as-is.
Flagged previously on this PR at the scripts/tests/test_workflow_report_hardening.py thread; still unresolved as of this pass.
Resolved since last look: the missing changelog.d/ fragment noted on weekly-report.yml:16 is now present (changelog.d/fixed/6904-workflow-report-hardening.md, added in commit 3c70656722ea4d3db186300b737f8c11f10e37ee).
Marked that thread resolved.
Still open, non-blocking (already flagged, unchanged in this diff):
weekly-report.yml— the "Post to GitHub Discussions" and "Post to Discord" steps still splice${{ steps.report.outputs.week }},.today,.merged_count,.opened_count,.closed_count,.stars,.gfi_countdirectly intorun:script text (title/body of the GraphQL mutation and the DiscordCONTENTheredoc) rather than throughenv:, the pattern this PR hardens everywhere else. Not exploitable today — every one of those outputs is producer-controlled and numeric/date-shaped — but structurally inconsistent with the rest of the PR's threat model.ci.yml—go install github.com/rhysd/actionlint/cmd/actionlint@v1.7.7pins by mutable git tag rather than commit SHA, unlike the trufflehog installer a few lines below it, which this PR just moved to a SHA pin for exactly that reason. Go's module proxy/GOSUMDB gives tamper-evidence once a version is first fetched, so this is materially safer than acurl main | sh, but it's not the SHA-pin guarantee applied elsewhere in this file.
No new supply-chain issues found in this pass: all actions/checkout and dtolnay/rust-toolchain uses remain commit-SHA pinned, permissions blocks (issues, pull-requests, discussions) stay minimal and unchanged, no pull_request_target is used anywhere in these workflows, and the new manual-promote/backfill steps route all github.event.* values through env: before use in run: (COMMENT_BODY, DISC_TITLE, DISC_URL, etc.) — no raw interpolation into shell text.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Automated review pass over fix/workflow-report-hardening (diff against merge-base 2cf4e8aaff344496012e19730a37dc62ba94176f per origin/main).
Commit hygiene: one commit on this branch (262f33e) is authored/committed as Claude <noreply@anthropic.com> — see inline comment for detail and the rest of the branch's (correct) authorship for comparison.
Security review of the workflow diffs (ci.yml, discussion-to-issue.yml, weekly-report.yml): no script-injection or permissions-scoping issues found. All previously-bare ${{ }} expressions carrying externally-influenceable values (github.base_ref, github.event.comment.body, discussion title/url/author/category, needs.changes.outputs.crates) are now routed through step env: blocks rather than interpolated directly into run: shell text, which is the correct fix for that class of bug. permissions: blocks are unchanged and stay minimally scoped. No other correctness issues found in the diff; test/tooling additions (scripts/tests/test_workflow_report_hardening.py, actionlint wiring, changelog fragment) look consistent with the PR description.
Generated by Claude Code
There was a problem hiding this comment.
Commit attribution policy violation on this branch.
262f33e0626608deb770f87b131d08e344f5ff77 ("docs: rewrap CI hardening test comment to sentence boundaries") is authored and committed as Claude <noreply@anthropic.com>:
262f33e0626608deb770f87b131d08e344f5ff77 Claude <noreply@anthropic.com> / Claude <noreply@anthropic.com> - docs: rewrap CI hardening test comment to sentence boundaries
This is the exact pattern the repo's own commit-msg hook (author-identity check via git var GIT_AUTHOR_IDENT) and the "No AI / Claude attribution" convention in CLAUDE.md are meant to catch — it appears the hook wasn't active for whatever produced this commit, or the commit was pushed through a path that bypassed it.
The other three commits on the branch (693c0087, 3c706567, 3ae3eb2b) are correctly attributed to a human author (Evan Hu / Evan), so this looks like an isolated slip on the last commit rather than a systemic issue with the branch.
Recommend squashing this commit's change into the prior commit (or re-committing it under the correct author identity) before merge, since rewriting the branch's history is outside the scope of an automated review pass. Not flagging any other issues — the workflow-hardening changes themselves (routing github.base_ref, github.event.comment.body, discussion fields, and needs.changes.outputs.crates through step env: blocks instead of interpolating ${{ }} directly into run: shell text; unchanged, minimally-scoped permissions: blocks; the /to-issue exact-token match; the commit-pinned TruffleHog installer) all look correct and don't introduce new injection surface.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Reviewed the CI/workflow hardening changes. The script-injection hardening in ci.yml (env-passthrough for BASE_REF/CRATES instead of inline ${{ }} interpolation, shell-safe crate-name regex, pinned TruffleHog installer and actionlint version, added job timeouts) and the exact /to-issue token-matching fix in discussion-to-issue.yml look correct and well-tested. One finding below on a claim in the PR description that doesn't appear to be reflected in the diff.
Generated by Claude Code
| fi | ||
|
|
||
| failures_file=$(mktemp) | ||
| trap 'rm -f "$failures_file"' EXIT |
There was a problem hiding this comment.
The PR description says this change "replace[s] the invalid discussion REST backfill with bounded GraphQL pagination," but the actual discussion-listing call a few lines below (gh api "repos/${REPO}/discussions" --paginate, line 128) is unchanged from origin/main — confirmed via git diff origin/main...HEAD -- .github/workflows/discussion-to-issue.yml, which shows no edits to that line at all.
GitHub's REST API has no GET /repos/{owner}/{repo}/discussions endpoint for repository Discussions (only the now-closed-down team discussions had a REST surface); listing repo Discussions is GraphQL-only, which is presumably why the "Post to Discord" job elsewhere in this same PR still has to fall back to a GraphQL query for discussionCategories.
If that's right, this gh api call 404s, and since the script runs under set -euo pipefail (line 107, also pre-existing, not new in this PR), the whole backfill job dies right here — before it ever reaches the new per-discussion failure-tracking logic you're adding in this hunk (failures_file, the trap, and the final if [ -s "$failures_file" ] block below). That new logic looks correct in isolation, but appears unreachable given the call it depends on, which undercuts the "records per-discussion failures instead of continuing past them silently" claim in both the PR body and the changelog fragment — the job would still fail wholesale at the first API call, exactly as before this PR.
Could you confirm whether this endpoint actually works (I may be missing something — a preview header, GHES-specific behavior, etc.), and if not, either land the described GraphQL replacement in this PR or adjust the PR description/changelog so they don't claim the REST-endpoint fix happened here?
Generated by Claude Code
* fix(ci): paginate issue automation queries * docs(changelog): add missing fragment for issue workflow pagination fix Repo precedent (#6904, #6916) is to accompany fix(ci) workflow behavior fixes with a changelog.d/ fragment; this one was missing. --------- Co-authored-by: Evan <tonymo2048@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
Changes
/to-issuecommand matchingTests
python3 scripts/tests/test_workflow_report_hardening.py(7 passed)python3 -m py_compile scripts/tests/test_workflow_report_hardening.pygit diff --checkReview
Independent review found no remaining Critical, Important, or Minor issues and marked the branch Ready to merge.