Skip to content

fix(ci): require maintainer association for /update-snapshots trigger#532

Merged
ochafik merged 1 commit intomainfrom
worktree-agent-a612d54c
Mar 6, 2026
Merged

fix(ci): require maintainer association for /update-snapshots trigger#532
ochafik merged 1 commit intomainfrom
worktree-agent-a612d54c

Conversation

@ochafik
Copy link
Contributor

@ochafik ochafik commented Mar 6, 2026

Problem

.github/workflows/update-snapshots.yml triggers on issue_comment whenever a PR comment contains /update-snapshots, with no check on who posted the comment.

Attack vector

On a public repo, anyone who can comment on a PR can:

  1. Comment /update-snapshots on any open PR
  2. Trigger a job running with permissions: contents: write
  3. That job checks out the PR head branch, runs playwright test --update-snapshots, and pushes a commit with [skip ci]

The [skip ci] tag means the pushed commit bypasses CI. Combined with PR approval rules that don't dismiss on new commits, this lets unreviewed pixel changes land.

Fix

Add github.event.comment.author_association check to the job's existing if: condition. Only OWNER, MEMBER, or COLLABORATOR can trigger the comment path.

workflow_dispatch path is unchanged — it already requires repo write access to invoke.

Diff

if: >
  github.event_name == 'workflow_dispatch' ||
  (github.event.issue.pull_request &&
  contains(github.event.comment.body, '/update-snapshots') &&
  contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association))

Unauthorized comments now silently skip the job (no reaction, no comment — can add a deny-reaction job later if desired).

The issue_comment trigger previously ran for any commenter on a PR.
On a public repo this lets drive-by users trigger a job with
contents:write that checks out the PR branch, runs playwright
--update-snapshots, and pushes a [skip ci] commit.

Gate the issue_comment path on author_association being OWNER,
MEMBER or COLLABORATOR. workflow_dispatch is unchanged (already
requires repo write access).
@ochafik ochafik marked this pull request as ready for review March 6, 2026 17:14
@ochafik ochafik requested a review from jonathanhefner March 6, 2026 17:14
@ochafik ochafik merged commit 2b0d11e into main Mar 6, 2026
4 checks passed
@ochafik ochafik deleted the worktree-agent-a612d54c branch March 10, 2026 17:39
ochafik added a commit that referenced this pull request Mar 10, 2026
Changes since 1.2.0:
- fix: bundle SDK+zod in react-with-deps (was byte-identical to ./react) (#539)
- fix(build): copy schema.json to dist and externalize zod (#534)
- fix: skip debug log for high-frequency tool-input-partial notifications (#546)
- fix(deps): drop @hono/node-server override to patch GHSA-wc8c-qw6v-h7f6 (#535)
- fix(readme): use picture element for theme-aware logo (#545)
- fix(ci): require maintainer association for /update-snapshots trigger (#532)
- fix: pre-commit stages only originally-staged files; add .npmrc (#538)
- ci: use npm ci with caching, validate typedoc links, align Node versions (#533)
- test: exclude screenshot-gen from default E2E run; wire pdf-server tests (#537)
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