Skip to content

fix(security): gate the \translate-resync trigger on commenter trust (#192) - #219

Merged
mmcky merged 2 commits into
mainfrom
fix/192-gate-resync-trigger-on-commenter-trust
Jul 26, 2026
Merged

mmcky merged 2 commits into
mainfrom
fix/192-gate-resync-trigger-on-commenter-trust

Conversation

@mmcky

@mmcky mmcky commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Closes rollout step 1 of #192 (canonical templates), and absorbs both items carried over from #138. #192 should stay open — steps 2 and 3 are in other repos, see What is not done below.

The exposure

The issue_comment branch of the source-sync workflow's if: checked only that the comment body contained the magic string. issue_comment workflows run in default-branch context with full access to secrets, and GitHub cannot filter the event by comment body at the trigger level — so that if: is the only gate. Any GitHub account could comment \translate-resync on a merged PR and spend Anthropic credits plus runner minutes, repeatedly. The commenter controls nothing but the trigger (translated content comes from the repo), so this is cost abuse rather than code execution — but it was live in the canonical template and therefore on every deployed instance.

Two smaller holes came with it, absorbed from #138: the condition never required github.event.issue.pull_request, so a comment on a plain issue fired a run too; and the generated workflow carried no permissions: block at all, leaving the ambient GITHUB_TOKEN at whatever the repo default is.

The fix

The issue_comment clause now requires all four conditions, and the job declares permissions: contents: read — the action authenticates to the target repo with the PAT input, and checkout is the ambient token's only consumer.

    if: >
      (github.event_name == 'pull_request' && github.event.pull_request.merged == true) ||
      (github.event_name == 'issue_comment' &&
       github.event.issue.pull_request &&
       contains(github.event.comment.body, '\translate-resync') &&
       contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association))
    runs-on: ubuntu-latest

    permissions:
      contents: read

This is the shape already deployed in QuantEcon/lecture-python.myst, exactly as the issue asked. I checked it actually works before copying it rather than assuming: YAML folds a more-indented continuation by preserving its newline, so the expression GitHub receives contains literal \n — and both trigger branches have fired on that repo since the fix landed on 2026-07-22 (issue_comment runs on 07-23, pull_request runs through 07-26). A mis-folded if: would stop the sync firing on merges entirely and would look exactly like "nothing happened".

The open design question from #138: CONTRIBUTOR stays out

src/inputs.ts has enforced exactly OWNER/MEMBER/COLLABORATOR (TRUSTED_ASSOCIATIONS) inside the action since resync shipped. A workflow admitting CONTRIBUTOR would start a billed, secrets-bearing run that the action then no-ops with a warning nobody reads — two gates on the same decision have to agree, and the outer one has to be at least as tight as the inner one, or it is not a gate. CONTRIBUTOR is also weak on its own terms: GitHub grants it for one merged PR, which on a public lecture repo is a typo fix. RA reviewers who legitimately need to resync should be repo collaborators — a permissions change in the repo, not a loosened trigger. Recorded in .dev/decisions/D-2026-07-26-resync-trust-gate-association-set.md, including the API-permission-check alternative and why it only moves the cost rather than avoiding it.

Scope: the issue named three files; it is fourteen

#192's table scoped step 1 as setup.ts + examples/README.md + docs/user/quickstart.md. A sweep found twelve documented copies plus the scaffolder plus the E2E harness template. Fixing only the three named would have left nine copies teaching the vulnerable shape — and the docs are how the estate gets configured, so those nine are the supply side of the exposure.

Surface Copies
src/cli/commands/setup.ts (scaffolder, generates the workflow) 1
examples/README.md 3
docs/user/action-reference.md 3
docs/user/tutorials/add-language.md 2
README.md, docs/user/quickstart.md, docs/user/tutorials/fresh-setup.md, docs/user/tutorials/connect-existing.md 1 each
tool-test-action-on-github/.../sync-workflow-template.yml (harness) 1

docs/user/faq.md gains the two requirements it was missing (comment must be on a PR; commenter must be trusted), and the pages that explain the trigger now say why the conditions are load-bearing rather than leaving them as decoration a future edit trims.

The guard sweeps rather than enumerates

The existing review-workflow guard in workflow-templates.test.ts uses a hard-coded DOC_PAGES list. That shape is part of why this spread to twelve copies: each new doc page copied an older one, and no list knew it existed. The new sync guard walks README.md, examples/, docs/, tool-test-action-on-github/ and .github/, parses every workflow it finds with js-yaml, and asserts per job on each of the three conditions, on permissions.contents == read, and on any surviving copy of the pre-fix one-line form. A doc page added tomorrow is covered the day it lands. It parses rather than greps because a substring check passes on a copy whose fold silently swallowed a clause. It also asserts the workflow's association set still equals TRUSTED_ASSOCIATIONS in inputs.ts, so widening one side without the other fails CI.

It deliberately does not walk the repo root: the test-translation-sync* clones are gitignored but present in a working tree, and a test whose result depends on what happens to be on disk is worse than no test. And it asserts a minimum job count, so it cannot pass vacuously the day the extractor breaks.

Verified the guard fails before trusting it — regressing one doc page back to the pre-fix shape turns 5 assertions red, then restored.

Verification

  • npm test — 1444 passed, 63 suites (90 of them in workflow-templates.test.ts).
  • npm run lint, npm run format:check, npm run check-dev-refs — all clean.
  • Every edited YAML block parsed with js-yaml and its if: checked post-fold for balanced parens and all four conditions: 14/14.
  • npm run builddist-action/ byte-identical. This is CLI, docs and tests only; the action bundle does not move.

What is not done

  • Step 2 (harness). The harness template is fixed here, so test-translation-sync's three sync workflows pick the gate up on the next harness run (it force-pushes what it renders). The three files as they sit on that repo right now are still ungated.
  • Step 3 (estate). Untouched and still exposed: lecture-python-intro/sync-translations-zh-cn.yml and lecture-python-programming/sync-translations-{zh-cn,fa,fr}.yml. lecture-python.myst already carries the fix — it is where the shape came from.

Refs #192, #138. Surfaced by Copilot on QuantEcon/test-translation-sync#671 and QuantEcon/lecture-python.myst#979.

🤖 Generated with Claude Code

…192)

The `issue_comment` branch of the source-sync workflow's `if:` checked only
that the comment body contained the magic string. `issue_comment` workflows
run in default-branch context with full access to secrets, and GitHub cannot
filter the event by body at the trigger level — so that `if:` is the only
gate. Any GitHub account could comment `\translate-resync` on a merged PR and
spend Anthropic credits and runner minutes, repeatedly. The commenter controls
nothing but the trigger, so this is cost abuse rather than code execution, but
it was live in the canonical template and therefore on every deployed instance.

Two smaller holes came with it, absorbed from #138: the condition never
required `github.event.issue.pull_request`, so a comment on a plain issue
fired a run too; and the generated workflow carried no `permissions:` block at
all, leaving the ambient GITHUB_TOKEN at the repo default.

The `issue_comment` clause now requires all four conditions — a comment on a
PR, the command, and an author in ["OWNER", "MEMBER", "COLLABORATOR"] — and
the job declares `permissions: contents: read`, since the action authenticates
to the target repo with the PAT input and checkout is the ambient token's only
consumer.

CONTRIBUTOR is deliberately excluded. `src/inputs.ts` has always enforced
exactly this three-way set inside the action, so admitting it at the workflow
level would only buy a billed run that then no-ops; the outer gate has to be
at least as tight as the inner one. Recorded in
.dev/decisions/D-2026-07-26-resync-trust-gate-association-set.md.

The fix lands in all fourteen in-repo copies — the scaffolder, twelve
documented copies across README, examples/, quickstart, action-reference and
the three tutorials, and the E2E harness template — plus the FAQ's stated
requirements. The exposure was never one file: each new doc page had copied
the shape from an older one, and the docs are how the estate gets configured.

A new guard in workflow-templates.test.ts sweeps rather than enumerates. It
parses every workflow under README.md, examples/, docs/,
tool-test-action-on-github/ and .github/, plus the scaffolder's output, and
fails per job on a missing condition, a missing `contents: read`, or any
surviving copy of the pre-fix one-line form — so a doc page added tomorrow is
covered the day it lands. It also asserts the workflow's association set still
matches TRUSTED_ASSOCIATIONS in inputs.ts, which keeps the two gates from
drifting apart. Verified it fails before trusting it: regressing one doc page
turns 5 assertions red. It parses rather than greps because a substring check
passes on a copy whose YAML fold silently swallowed a clause.

The folded `if:` shape is the one deployed in QuantEcon/lecture-python.myst
since 2026-07-22, where both trigger branches have fired since — checked
before copying it, because a mis-folded `if:` would stop the sync firing on
merges entirely and would look exactly like "nothing happened".

dist-action/ is byte-identical: this is CLI, docs and tests only.

Rollout steps 2 and 3 remain, in other repos: test-translation-sync's three
sync workflows pick the gate up on the next harness run (the template is fixed
here), and the production source repos still need the same edit by hand.

Refs #192, #138. Surfaced by Copilot on QuantEcon/test-translation-sync#671
and QuantEcon/lecture-python.myst#979.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 26, 2026 04:55
@mmcky mmcky added security Security implications — needs a security-aware review bar bug Something isn't working high-priority Address soon labels Jul 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Hardens the source-sync workflow template and all in-repo documented copies by ensuring the \translate-resync issue_comment trigger only starts for trusted commenters on merged PRs, and by reducing the ambient job GITHUB_TOKEN to read-only permissions. This fits the codebase’s “canonical template + drift guards” approach by updating the scaffolder, docs, and E2E harness template together and adding a sweeping test to prevent future drift.

Changes:

  • Tighten the issue_comment branch of the sync workflow if: to require (1) comment is on a PR, (2) contains \translate-resync, and (3) commenter is in OWNER/MEMBER/COLLABORATOR.
  • Add permissions: contents: read to the sync job across templates/docs (least-privilege for the ambient GITHUB_TOKEN).
  • Add a new sweeping drift guard that parses published workflow YAML blocks and asserts the gate shape + permissions across all publishable surfaces (plus the scaffolder output).

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tool-test-action-on-github/test-action-on-github-data/sync-workflow-template.yml Updates the harness sync template to include the trusted-commenter gate and read-only job permissions.
src/cli/commands/setup.ts Updates the generated source workflow YAML to gate \translate-resync on trusted commenter + PR-only, and sets read-only job permissions.
src/cli/tests/workflow-templates.test.ts Adds a repo-sweeping, YAML-parsing drift guard to ensure every documented/template copy keeps the hardened resync gate + contents: read.
src/cli/tests/setup.test.ts Extends scaffolder output assertions to include the new PR-only/trusted-commenter clauses and permissions block.
README.md Updates the documented workflow snippet and explains the trust-gated resync behavior and read-only permissions.
examples/README.md Updates example workflow snippets and adds an explicit explanation of the resync trust gate requirements.
docs/user/tutorials/fresh-setup.md Updates the tutorial’s workflow YAML to include PR-only + trusted commenter gating and read-only job permissions.
docs/user/tutorials/connect-existing.md Updates the tutorial’s workflow YAML to include PR-only + trusted commenter gating and read-only job permissions.
docs/user/tutorials/add-language.md Updates multi-job workflow examples so each job carries the same gate and read-only permissions.
docs/user/quickstart.md Updates the quickstart workflow YAML and adds an explicit note that the four conditions are load-bearing.
docs/user/faq.md Documents the additional resync requirements (PR-only + trusted association set) alongside existing requirements.
docs/user/action-reference.md Updates the reference workflow YAML and adds a detailed explanation of why the gating conditions and contents: read matter.
CHANGELOG.md Adds a security entry documenting the hardening, scope (all copies), and the new sweeping drift guard.
.dev/STATE.md Records current project state for #192, including what remains out-of-repo (rollout steps 2 and 3).
.dev/log/2026-07-26-192-resync-trust-gate.md Adds a maintenance log entry describing the change and guard rationale.
.dev/decisions/D-2026-07-26-resync-trust-gate-association-set.md Records the decision to exclude CONTRIBUTOR and keep the workflow gate aligned with TRUSTED_ASSOCIATIONS in the action.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Audited all eight deployed sync workflows rather than trusting #192's list —
an org-wide search for `translate-resync` returns exactly those eight outside
this repo, and each was fetched and checked. #192's list was accurate: four
production workflows ungated, myst already fixed, the three harness ones
carrying `issue.pull_request` but neither the trust gate nor permissions.

Refs #220, #192.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mmcky
mmcky merged commit 35c9b22 into main Jul 26, 2026
1 check passed
@mmcky
mmcky deleted the fix/192-gate-resync-trigger-on-commenter-trust branch July 26, 2026 05:18
mmcky added a commit that referenced this pull request Jul 26, 2026
The docs half is live the moment it is on main — that is where the estate gets
configured from — but the scaffolder's copy reaches users only on the next
release, and nothing already deployed moved: the gate is in the workflow file,
not the action, so an `@v0` pin does not carry it.

E2E status noted as unconfirmed and only partly confirmable: the harness fires
via the `test-translation` label, so a run proves the rewritten `if:` still
parses and fires at all (the risk that would otherwise silently kill the merge
path), but it never exercises the resync clause itself.

Refs #192, #219, #220.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Jul 26, 2026
Scoped harness run (--scenarios 01, all languages, main 59f7c56) deployed the
gate to all three test-translation-sync sync workflows: they now audit
isPR=1 assoc=1 perms=1.

Both branches of the rewritten `if:` exercised on the real Actions path. The
label/merge branch fired three green syncs and three translation PRs — the
load-bearing check, since a mis-folded scalar would silently kill the merge
path rather than erroring. The resync branch was fired by a MEMBER comment and
ADMITTED: three issue_comment runs started, logged the RESYNC line, then exited
on 'not merged' with no model calls. Run against an unmerged PR on purpose, so
it costs nothing and still proves the gate does not lock legitimate users out.

The rejection half stays unverified — it needs an account outside the org.

Also logged against the existing PLAN [L] item: the resync language parser
splits the whole comment body, not the first line, so prose after the command
is read as a language code ('(verifying'). Observed live, not theorised.

Refs #192, #219, #220.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Jul 26, 2026
…flows gated

Estate PRs merged (lecture-python-intro#805, lecture-python-programming#586).
Closing audit re-fetched every deployed sync workflow from its default branch
and PARSED it rather than grepping, so the check is against the expression
GitHub evaluates after folding: 8/8 carry the four conditions with balanced
parens and permissions: contents: read, and no legacy one-line form survives.
An org-wide sweep scoped to .github/workflows/ returns exactly those eight, so
the set is complete rather than merely matching the original report's list.

#192 and #220 closed. STATE entry moved from In flight to Recently landed,
carrying the two residuals: the scaffolder ships the ungated shape until the
next release, and #221 (GitHub Apps report author_association=NONE).

Refs #192, #219, #220, #221.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mmcky mmcky mentioned this pull request Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working high-priority Address soon security Security implications — needs a security-aware review bar

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants