Skip to content

fix(ci): gate security review on repo write access - #7986

Merged
wpfleger96 merged 3 commits into
mainfrom
duncan/security-review-write-access
Sep 30, 2026
Merged

wpfleger96 merged 3 commits into
mainfrom
duncan/security-review-write-access

Conversation

@wpfleger96

Copy link
Copy Markdown
Member

🤖 The Codex Security Review gate trusted GitHub's author_association (MEMBER/OWNER), but GitHub only reports MEMBER to viewers who can see the membership. The workflow GITHUB_TOKEN cannot see private block org membership, which is GitHub's default, so most Block employees read as CONTRIBUTOR and their PRs never reviewed automatically. The same applied to the comment path, where private members could not authorize a review with @buzz-security-review <sha>.

Trust is now repo write access, read from GET /repos/{owner}/{repo}/collaborators/{user}/permission via getCollaboratorPermissionLevel, which is independent of membership visibility and works with the authorize job's existing contents: read and pull-requests: read permissions. A user is trusted only when permission is write (which includes maintain) or admin. A successful lookup is not enough: on this public repository, an account with no relationship to it returns 200 with read, not 404. Outside contributors can only push to forks and come back read, so they still need an explicit command from a trusted user.

The check covers the PR author on pull_request_target, the commenter on issue_comment (the job-level author_association filter is removed so the trusted script makes that decision), and the stale-marking path on PR updates. Transient API errors are retried. Authorization fails closed: any lookup error other than 404 fails the job instead of authorizing. Stale-marking is conservative: an existing review is always marked stale without consulting permissions, and a failed lookup counts as untrusted, so a previous range's review can never appear current for a new head. A commenter without write access is logged and ignored instead of failing the run.

Duncan and others added 2 commits September 30, 2026 10:38
author_association only reports MEMBER when org membership is public, so
Block employees with private membership never auto-ran the review. The
collaborator permission endpoint is visibility-independent; write, maintain,
and admin are trusted, 404 is untrusted, and other errors fail closed.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
A failed permission lookup in invalidate() aborted before the previous
review was marked stale, leaving it looking current. The lookup now runs
only without an existing review and treats errors as untrusted; it retries
transient errors, and unauthorized commenters no longer fail the job.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 requested a review from a team as a code owner September 30, 2026 14:40
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: d7a35afa9859a0c666de93ab1cd4f8a433a1cfe8...d652600327baedcb562c9feabcad916aef9d0ccb
  • Model: gpt-5.6-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: LOW

The live permission checks correctly fail closed and preserve separation between untrusted review input and privileged posting. However, the workflow now lets any public commenter allocate a hosted runner before authorization is checked.

Findings

[LOW] Untrusted comments can allocate CI runners

  • Category: Agent/Workflow
  • Location: .github/workflows/codex-security-review.yml:31 (source)
  • Description: Removing the author association condition means any user who can comment on a public pull request can trigger the prepare-review job with a matching prefix. The write-access check occurs only after a runner starts and checks out the repository, and this job has no concurrency control.
  • Impact: An attacker can repeatedly post command-shaped comments to consume workflow concurrency, create run noise, and delay legitimate security reviews or other CI jobs. The later permission check prevents privileged review execution but does not prevent this resource consumption.
  • Recommendation: Add a pre-run trust gate where possible, or move live permission validation to a lightweight external dispatcher. At minimum, add restrictive concurrency and rate limiting for comment-triggered preparation jobs.

Notes

  • No additional limitations were reported.

Generated by Codex Security Review |
Requested by: @wpfleger96 |
Workflow run

@TheSentinel454 TheSentinel454 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.

Approving at a194d23500c5e2b1c7ae64912c086a9125e9eee3. Checking repo write access is the right trust boundary. It doesn't depend on whether org membership is visible, it handles the public-repo case where an unrelated user comes back as 200 read, and it fails closed in both prepare and invalidate. .github/scripts/codex-security-review.test.js passes 18/18 at this SHA. The new tests also fail against the base script, so they catch the old bug.

I left three small, non-blocking suggestions inline. There's one more that couldn't go inline because the line isn't in the diff: .github/workflows/codex-security-review.yml:309 still says # The trusted authorization job already enforced repository membership. That's out of date now. Maybe change it to "…already required repo write access for the PR author or commenter."

One thing to check after merge: every probe of GET /collaborators/{user}/permission so far used a user token, not the workflow GITHUB_TOKEN. On the first internal PR after merge, it's worth confirming the Authorize job reports authorized=true. On the first Renovate PR, confirm it skips cleanly: with a user token, renovate[bot] comes back as 200 none.

Comment thread .github/scripts/codex-security-review.js
Comment thread .github/scripts/codex-security-review.test.js
Comment thread .github/scripts/codex-security-review.test.js Outdated
Untrusted malformed commands now exit quietly instead of failing the run.
Tests pin bot authors with no permission and name the mocked 404 account
accurately; the stale workflow comment now describes the write-access gate.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026

@TheSentinel454 TheSentinel454 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.

Re-approving at d652600327baedcb562c9feabcad916aef9d0ccb. All four nits from the previous review are addressed in d6526003:

  • The commenter write-access check now runs before the format check. The new @buzz-security-review foo test from outside-contributor expects no failures and no outputs.
  • unrelated-user is renamed to nonexistent-user.
  • renovate[bot]: none is pinned as an untrusted author.
  • The yml:309 comment now says repo write access.

node --test .github/scripts/codex-security-review.test.js passes 18/18 at this SHA. The post-merge GITHUB_TOKEN check from the previous review still applies.

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026
@wpfleger96
wpfleger96 merged commit 0ee6093 into main Sep 30, 2026
36 checks passed
@wpfleger96
wpfleger96 deleted the duncan/security-review-write-access branch September 30, 2026 16:19
wpfleger96 added a commit that referenced this pull request Sep 30, 2026
…i-port

* origin/main:
  fix(agents): stop built-in prompts from teaching sleep polling (#7992)
  feat(relay): add direct staff ban/timeout/delete with staff guard (#7883)
  fix(ci): gate security review on repo write access (#7986)
  feat(acp): wrap workers at the subprocess launch boundary (#7985)
  feat(buzz-relay): idempotent owner community deletion with quota reservation (#7969)
  feat(mobile): show contextual names in lists, Search and Pulse (#7896)
  Add Kimi Code's default install path to managed-agent binary discovery (#5997)

Co-authored-by: Will Pfleger <wpfleger@block.xyz>
Signed-off-by: Will Pfleger <wpfleger@block.xyz>

This branch was successfully deployed

1 active deployment
codex-review — d6526003 Deployed Sep 30, 2026 by wpfleger96 via Run Codex Security Review #6208
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants