Add gated security reviews

[jmecom]

Add a gated Codex security review for Buzz pull requests.

Reviews run automatically for Block organization members. External PRs require a @buzz-security-review comment from a Block member, and every new external push invalidates the previous result.

Codex reviews the PR SHA in a read-only, network-restricted job with no write GitHub token. A separate trusted job validates and sanitizes structured findings before posting them. The workflow uses the dedicated CODEX_REVIEW_API_KEY secret.

[wpfleger96]

🤖 I found one blocking correctness gap in the review-freshness contract. The trust boundary itself is carefully designed: authorization is re-resolved through GitHub, the exact head is pinned, untrusted code is only inspected from a separate trusted directory, credentials are not persisted, Codex is run with the pinned action's drop-sudo plus :read-only controls, and output is validated and sanitized before the privileged posting job.

This review covered the full workflow and renderer at b806c1957a29f564a2473f254247ae9c99a46a32, the pinned openai/codex-action implementation at 86365089eb2b84e0a8fb0717b304f8bdcb13b20e, the codex-review environment policy/secret scope, and independent adversarial verification of the freshness logic. actionlint passes, and the current PR checks examined are green or still running. The workflow cannot run end to end until it exists on main, so the secret-bearing GitHub-hosted path remains a named post-merge verification requirement.

[wpfleger96]

🤖 IMPORTANT — Correctness: A completed review is considered current from head.sha alone. The posted review was generated for the exact baseSha...headSha range, but this check ignores baseSha; if main advances while the PR head stays unchanged, a later invalidation attempt returns “already has a review for the current head” and leaves the old range displayed as current. The posting path correctly rejects an in-flight result when pullRequest.base.sha !== baseSha, but an already-posted result has no equivalent range check, and pull_request_target does not run merely because the base branch advances.

Please encode both base and head (or the complete range) in the completion marker and make base movement trigger invalidation/re-review—for example by also handling pushes to main and reconciling open PRs, or by using another event/path that re-evaluates affected PRs. The visible “current” state must describe the same exact range Codex reviewed.

[Chessing234]

the two-job split is the right architecture and the gating logic is careful — author_association in ["MEMBER","OWNER"], the stale marker on new external pushes, the concurrency group, pinned action SHAs. a few things i think are still open, in rough order of how much they matter.

the job that checks out untrusted code also holds the API key.

- uses: actions/checkout@…
  with:
    ref: refs/pull/${{ needs.prepare-review.outputs.pr_number }}/head
...
    openai-api-key: ${{ secrets.CODEX_REVIEW_API_KEY }}

the pr says the review runs "in a read-only, network-restricted job with no write GitHub token", and that's true of the GitHub token — but CODEX_REVIEW_API_KEY is a real secret sitting in the environment of a job whose working directory is an outside contributor's branch, under pull_request_target where secrets are available. codex is an agent that runs commands over that checkout. anything that executes from the repo during the review — a config file codex honours, a hook, a tool it's willing to invoke — reads that key.

"network-restricted" is the whole mitigation, so it needs to be stated concretely: what enforces it, and does it apply to the codex process itself (which must reach the openai endpoint, so it isn't a blanket egress block)? if the restriction is an allowlist that includes the API host, an exfil path exists through that host. the alternative shapes are running the review on a fork-isolated runner, or having the untrusted checkout produce only a diff artifact that a second, secret-holding job reads without the working tree.

the comment trigger checks out a moving ref. for the issue_comment path, prepare-review resolves a PR number and security-review checks out refs/pull/<n>/head — which is whatever the head is when the checkout runs, not what the Block member looked at when they typed @buzz-security-review. an outside contributor can push between the two. "every new external push invalidates the previous result" handles a stale posted verdict; it doesn't close this, because here the new code is what gets reviewed and blessed. resolving the head SHA in prepare-review and checking out that SHA — and refusing if it differs from the SHA at comment time — makes the approval refer to a specific tree.

safeText mangles the content it exists to carry. the substitutions replace \, backtick, @, &, <, >, and every URL scheme with lookalike characters. that stops mention-spam and markdown injection, but this is a security review — its findings are about code, and code is mostly those characters. a finding about <script> posts as ‹script›, a && b as a ＆＆ b, and any path it wants to cite loses its backticks and can't be a link. a reader can no longer copy a snippet out of the comment, and can't tell a real homoglyph in the source from one the sanitizer introduced.

the usual answer is to escape for the context rather than substitute: fence the text in a code block (escaping only backtick runs that would close the fence) so github renders it literally and mentions/links are inert inside it. that keeps @, &, <, > intact and still can't ping anyone. the bidi-control and C0/C1 stripping above it is right and should stay.

requireKeys demands an exact key set. any additional property in codex's output — a new field in a future model version, an extra key it decides to add — throws "unexpected or missing properties" and the whole review is discarded rather than degraded. strict is defensible for a security boundary, but the failure is all-or-nothing and silent to the person who asked for the review. worth either allowing (and dropping) unknown keys while still requiring the known ones, or making the parse failure post something so a maintainer knows the review didn't happen rather than assuming clean.

smaller: CODEX_REVIEW_API_KEY_PRESENT: ${{ secrets.CODEX_REVIEW_API_KEY != '' }} is a good touch for a clear "not configured" path — worth making sure the job actually short-circuits on it rather than failing deep inside the action.

[wpfleger96]

🤖 Re-reviewed at exact head 0bf2c7e619dde3cff31d00c4d9f75b1953f2fc7f. The previous stale-range correctness blocker is fixed, and I found no new blocking security or correctness issue.

The completed marker now binds the exact live main SHA and PR head; pushes to main reconcile labeled current reviews in bounded batches; PR updates invalidate old ranges; manual authorization is bound to the full current head SHA; and posting registers itself before final freshness checks, then checks again after the comment write. I adversarially held the PR head constant while moving the base: the old range was invalidated, the head-only mutation failed the tests, and a new exact range remained reviewable. An independent verifier reproduced the same result and found no boundary regression.

just security-review-check passed all 11 contract tests and actionlint passed at this SHA. The relevant CI checks (Security, Unit Tests, Rust Lint, Web, Mobile, both server cross-compiles, macOS build, release candidate, and DCO) are green; unrelated desktop/Windows lanes are still running. The existing post-merge limitation remains: the secret-bearing workflow cannot receive a live GitHub-hosted canary until this workflow exists on main.