security(ci): drop persisted Git credentials from compiling checkouts#319
Merged
Conversation
Closes #318. actions/checkout defaults to persist-credentials: true, which writes the workflow GITHUB_TOKEN into .git/config as an http.https://github.com/.extraheader entry. Any code the job subsequently executes from the checkout can read it: Cargo build scripts, proc macros, test binaries, Gradle build scripts, scripts/*.sh, the MkDocs build. On a pull request that tree is by definition unreviewed code, and every CI job in this repo compiles and runs it. Raised by CodeRabbit on #317, where the fix was applied to the one new job; this is the repo-wide sweep. Applied to 18 of the 19 checkouts under .github/. Audited rather than applied blanket. Three findings shaped the result: 1. .github/actions/rust-setup performs no checkout of its own (it says so at L5-6 and is used by 12 call sites), so hardening at each call site is complete coverage — there is no hidden checkout to miss. 2. No job actually needed the persisted credential. There is exactly ONE Git network operation in the whole .github/ tree. Nothing pushes commits, tags, or branches; there are no submodules; GitHub Pages uses the OIDC flow (configure-pages/upload-pages-artifact/deploy-pages) rather than a gh-pages branch push, and its deploy job has no checkout at all. The usual false positives were each checked and dismissed: `gh release create` and softprops/action-gh-release authenticate via GH_TOKEN against the REST API, and `fastlane match` clones a DIFFERENT repository using its own MATCH_GIT_* credentials. persist-credentials only affects git network operations relying on the stored credential — not the gh CLI, not the API. 3. The highest-exposure site is not a ci.yml job. web.yml declares pages: write + id-token: write at the WORKFLOW level, so that block also covers its `build` job — which is pull-request-reachable and runs trunk, cargo doc, pip install, and mkdocs build. That is the most valuable token in the repo sitting in the job with the widest untrusted-code surface. The single deliberate exception is release-auto.yml's `prepare`, which keeps its credentials and now carries an inline comment explaining why. It holds the tree's only real Git network operation (git ls-remote --tags origin), it is unreachable from untrusted input (workflow_run, further gated to event == 'push'), and it executes no repository code at all — it reads two text files. Hardening it would buy nothing while risking a release path that is only exercised at a version cut. The comment also records the safe migration order for anyone later wanting uniformity: convert the ls-remote to `gh api .../git/ref/tags/$tag` FIRST, then drop the credentials. That swap additionally fixes a latent bug, since the current `>/dev/null 2>&1` makes a transient failure indistinguishable from "tag does not exist" and would publish a duplicate release. ci.yml's `changes` job is the one place the change touches the git layer at all, since dorny/paths-filter reads local git on push events. It needs no authentication (fetch-depth: 0 means the base commit is already local, and the pull_request path uses the GitHub API), so this is expected to be inert; an inline comment records the reasoning and the benign failure mode. Deliberately NOT included: zizmor's unpinned-uses finding. SHA-pinning every action is a different risk class and a maintenance commitment that needs the github-actions Dependabot ecosystem enabled first, or it trades a supply-chain risk for a stale-action risk. One sub-item is worth doing soon on its own: .github/actions/rust-setup pins dtolnay/rust-toolchain@master — a branch ref that advances on every upstream commit, unlike the @vn tags used everywhere else — and that composite feeds 12 of the 19 checkouts including release.yml (contents: write) and web.yml (pages: write). Tracked in #318. Purely additive CI configuration: no source, build output, or emulation behavior changes.
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGitHub Actions checkout steps now disable credential persistence across build, test, release, and security workflows. The automatic release tag-check exception is documented, and the unreleased changelog records the CI hardening scope. ChangesCI checkout credential hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Hardens GitHub Actions checkouts by preventing actions/checkout from persisting the workflow token into .git/config, reducing token-exfiltration risk from untrusted code paths executed during CI (build scripts, proc macros, tests, Gradle, shell scripts, docs builds).
Changes:
- Add
persist-credentials: falsetoactions/checkout@v7across PR-reachable and release/dispatch workflows. - Preserve the single intentional exception in
release-auto.ymland document why it retains persisted credentials. - Document the change in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| CHANGELOG.md | Adds release-note entry describing the credential persistence hardening. |
| .github/workflows/web.yml | Hardens the Pages build job checkout (high-permission token surface). |
| .github/workflows/security.yml | Hardens checkouts for audit/deny/clippy-security jobs. |
| .github/workflows/release.yml | Hardens checkout for the release build workflow. |
| .github/workflows/release-auto.yml | Keeps persisted credentials for tag-existence check; adds rationale comment. |
| .github/workflows/pgo.yml | Hardens checkout for PGO/BOLT workflows. |
| .github/workflows/ios.yml | Hardens checkout for iOS workflow. |
| .github/workflows/ci.yml | Hardens all CI checkouts (including changes job with fetch-depth: 0). |
| .github/workflows/android.yml | Hardens checkouts for Android cross-build and Gradle packaging jobs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-auto.yml:
- Around line 74-81: Scope the credential exception to the checkout’s origin
credential: in .github/workflows/release-auto.yml lines 74-81, update the
comment to say this is the only operation requiring that credential, not the
only Git network operation. In CHANGELOG.md lines 52-64, change the statement
that no job needed persisted credentials to say all other jobs do not need the
checkout credential, and remove the broader Git-operation claim.
- Around line 83-88: Update the tag existence check in the release workflow to
distinguish a missing tag from lookup failures: either switch the check to the
specified gh api ref endpoint, or preserve git ls-remote while handling nonzero
errors separately and failing the workflow closed. Ensure only a confirmed
missing tag reaches gh release create, and transient authentication or network
failures cannot trigger publication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f120365c-2fbd-4a0e-a5a0-c67bf2455b12
📒 Files selected for processing (9)
.github/workflows/android.yml.github/workflows/ci.yml.github/workflows/ios.yml.github/workflows/pgo.yml.github/workflows/release-auto.yml.github/workflows/release.yml.github/workflows/security.yml.github/workflows/web.ymlCHANGELOG.md
Three accuracy corrections from Copilot and CodeRabbit on #319, all adopted; none change behaviour. 1. "reads two text files" / "Cargo.toml + CHANGELOG.md" undercounted: the job also reads the optional .github/release-notes/vX.Y.Z.md override (L119). Both the pre-existing comment and mine now describe the set without pinning an exact count, so the security rationale cannot go stale the next time a file is added. 2. "the only real Git network operation in the whole .github/ tree" was too broad. fastlane match in ios.yml also clones over the network — it just uses its own MATCH_GIT_* secrets against a different remote, so it is unaffected by persist-credentials. Reworded to the claim that actually justifies the exception: the only operation needing THIS checkout's origin credential. The CHANGELOG had the mirrored problem, asserting no job needed the credential and then naming an exception; it now says every *hardened* job was confirmed not to need it. 3. The CHANGELOG claimed "every CI job compiles and runs" the checked-out tree. The audit and deny jobs do neither — they install prebuilt binaries and parse Cargo.lock — and overstating a security claim in release notes is exactly what should not happen. Also corrected a claim in my own comment: a masked ls-remote failure was described as publishing "a duplicate release". Per gh's documented behaviour it would instead fail outright when a release already exists for the tag, or publish against the pre-existing tag when one does not — noisy or wrong, but not silently duplicated. Declined in this PR (tracked in #318): converting the ls-remote to a fail-closed gh api call. It is a real latent bug, but it changes the release path, cannot be verified without a version cut, and does not belong in a credential-hardening change. The comment records the safe migration order.
This was referenced Jul 21, 2026
doublegate
added a commit
that referenced
this pull request
Jul 21, 2026
CodeRabbit flagged that the entry added here says release-auto.yml joined the sweep while the #319 entry below it still said "18 of 19" and called release-auto.yml a deliberate exception. Correct, and the underlying problem is larger than the numbers: both entries sit in the same [Unreleased] section, so they ship in one release and a reader gets "we hardened 18 with one exception" followed by "...and now the 19th". That is PR history leaking into release notes, which this project explicitly does not want there ("Keep deep engineering/lineage narrative out of the CHANGELOG"). Patching the count would have left two entries narrating one coherent change. Merged into a single entry describing the end state — all 19 checkouts hardened, with the fail-closed tag check and the toolchain pin as the two related items — and dropped the now-purely-historical intermediate narrative. The audit facts worth keeping are retained verbatim: rust-setup performs no checkout of its own, no job needed the credential, and web.yml's build is the highest-exposure site.
doublegate
added a commit
that referenced
this pull request
Jul 21, 2026
#321) * security(ci): fail-closed release tag check + pin the toolchain action Finishes the two items #318 tracked that PR #319 deliberately deferred. Reopened first: #318 had auto-closed one second after #319 merged, purely from a `Closes #318.` keyword in that commit body, while both items were still present on main. Deliberately no closing keyword here — the issue is closed by hand once this is verified. 1. Fail-closed release-tag check ------------------------------- `release-auto.yml` decided whether to publish a release with: git ls-remote --exit-code --tags origin "refs/tags/$tag" >/dev/null 2>&1 That collapses three distinct outcomes — tag present, tag absent, lookup failed — into a two-way answer, reading ANY non-zero exit as "absent". A transient network or auth blip therefore pushed an already-released version down the should_release=true path. This one call decides the entire release, so guessing is the single thing it must not do. Replaced with `gh api repos/$GITHUB_REPOSITORY/git/matching-refs/tags/$tag`. `matching-refs` was chosen over the more obvious `git/ref/tags/$tag` specifically because it answers "absent" with HTTP 200 and an empty array rather than a 404 — a genuine miss can never look like an error, and no error-body parsing is needed to tell the two apart. `matching-refs` matches by PREFIX, so the exact ref is compared in jq. That is load-bearing rather than defensive, and was verified against the live API: `v2.2` prefix-matches two real tags (v2.2.0, v2.2.1) while exact- matching none, so a naive non-empty check would have reported a nonexistent tag as present. Fail-closed behaviour verified under `shell: bash` + `set -euo pipefail`, which is what the step actually runs: a `gh` HTTP error aborts the assignment (exit 1), and a non-array body makes jq error under pipefail (exit 5). Neither can resolve to a release decision. An unexpected match count aborts explicitly. (Verifying this in the local zsh gave a false "passes" result — zsh's `set -e` handling of `var="$(cmd)"` differs from bash's, so the check must be exercised with `bash -c`.) Because the check no longer touches Git, `release-auto.yml`'s checkout drops its persisted credentials as well. It was the single documented exception in #319's sweep, held back only by this ls-remote, so all 19 checkouts in the repo are now uniform and the exception comment is gone rather than stale. 2. Pin dtolnay/rust-toolchain ----------------------------- `.github/actions/rust-setup` used `dtolnay/rust-toolchain@master` — a BRANCH ref that advances on every upstream commit, so every run silently resolved to whatever HEAD happened to be, with no review window and no signal that anything changed. Every other action in this repo is referenced by a `@vN` tag, which is not expected to move. The exposure is disproportionate: this composite is consumed by 12 of the repo's 19 checkouts, including release.yml (contents: write, builds the shipped binaries) and web.yml (pages: write + id-token: write), and it is the action that installs the compiler — a position where a compromise is very hard to detect from build output. Pinned to e97e2d8cc328f1b50210efc529dca0028893a2d9, verified via the API as the current target of upstream's refs/tags/v1. The trailing `# v1` is the convention Dependabot's github-actions ecosystem reads; that ecosystem is already enabled in .github/dependabot.yml, so this does not trade a supply- chain risk for a stale-action one. Deliberately still NOT done: SHA-pinning the remaining 12 actions. Those are `@vN` tags rather than branch refs, so they carry materially less risk than `@master` did, and a blanket pin is a policy decision about pin maintenance rather than a drive-by fix. Addresses #318. * docs(changelog): consolidate the #318 hardening into one entry CodeRabbit flagged that the entry added here says release-auto.yml joined the sweep while the #319 entry below it still said "18 of 19" and called release-auto.yml a deliberate exception. Correct, and the underlying problem is larger than the numbers: both entries sit in the same [Unreleased] section, so they ship in one release and a reader gets "we hardened 18 with one exception" followed by "...and now the 19th". That is PR history leaking into release notes, which this project explicitly does not want there ("Keep deep engineering/lineage narrative out of the CHANGELOG"). Patching the count would have left two entries narrating one coherent change. Merged into a single entry describing the end state — all 19 checkouts hardened, with the fail-closed tag check and the toolchain pin as the two related items — and dropped the now-purely-historical intermediate narrative. The audit facts worth keeping are retained verbatim: rust-setup performs no checkout of its own, no job needed the credential, and web.yml's build is the highest-exposure site.
doublegate
added a commit
that referenced
this pull request
Jul 22, 2026
…sync The template re-sync in 9875dab pulled the upstream reviewer over the hardened version from 0237135 and silently reverted five of its fixes -- three of which CodeRabbit had already marked "Addressed in commit 0237135" on this PR, so the resolved threads no longer described the tree. This restores the hardening on top of the re-sync's genuine improvements (job-level concurrency, the agy flock, the retry loop), and closes the remaining review findings. Trigger and execution gating (workflow) The `pull_request` fork check was dropped, so any fork PR could schedule work on the self-hosted runner. GitHub's "require approval for outside collaborators" is not a substitute: its default covers only FIRST-TIME contributors, so a returning outside contributor gets a runner on the maintainer's own hardware. The `head.repo.full_name == github.repository` condition is restored, alongside the `issue_comment` author_association gate. The checkout now takes the DEFAULT BRANCH rather than the PR head. The job executes the checked-out `scripts/agy-review.sh` with a token in scope, so checking out the PR let the reviewed change rewrite its own reviewer -- code execution on the runner from an untrusted diff. Review content is unaffected: the diff comes from the API via `gh pr diff`, not the working tree. The consequence is worth stating plainly: a PR that edits the reviewer or the style guide is reviewed by the version already on main. `persist-credentials: false` joins it per the repo-wide rule from PR #319, and the action moves to `actions/checkout@v7`, matching the other 19 checkout sites (this was the only `@v4` in the repo). Token exposure at the agy boundary `--dangerously-skip-permissions` stays: it is required for headless operation -- without it agy blocks on an interactive approval no one is there to answer and the run times out. Removing the flag, as suggested, would break the feature rather than secure it. What the flag actually costs is the approval gate, so the agent could act on instructions embedded in the attacker-controlled diff it is reviewing; the exposure that matters is the GitHub token in the job environment. agy is therefore launched through `env -u GH_TOKEN -u GITHUB_TOKEN`. `gh` runs in this script, before and after, and agy never needs the token. `--sandbox` still confines filesystem and network access. Command injection in the script(1) fallback The re-sync replaced the argv-safe `_agy_pty.py` wrapper with `script -qfec "... ${flags[*]}"`. `script -c` takes a command STRING and runs it through `sh -c`, so interpolating the flags array raw makes any space or metacharacter in AGY_MODEL / AGY_EFFORT / AGY_PRINT_TIMEOUT -- all env-settable -- into shell syntax. The command string is now assembled with `printf '%q '`, which emits a shell-safe rendering of each element. Verified: a flag value of `5m; touch /tmp/AGY_PWNED` now arrives as one literal argv element and the injected command does not run. Single-argument size ceiling (a live E2BIG, not a hypothetical) The prompt reaches agy as ONE argv string, and Linux caps a single argument at MAX_ARG_STRLEN = 32 * PAGE_SIZE = 128 KiB -- a separate and far lower ceiling than ARG_MAX (4 MiB here). The workflow shipped `MAX_DIFF_BYTES: "200000"`, which exceeds it outright, and capping the diff alone would not be sufficient anyway since the instruction boilerplate and the style guide ride in the same string. Measured locally on this kernel: 131000 bytes execs, 131073 fails, 200000 fails. The diff budget drops to 90 KB and a new MAX_PROMPT_BYTES (120 KB) caps the ASSEMBLED prompt. Conversation-database fallback could publish an unrelated session The `ls -t | head -1` fallback takes the globally newest conversation DB, so when agy fails BEFORE creating a conversation it posts the last assistant message from whatever else ran on that host -- another repo's review, or the owner's interactive chat -- into a public PR comment. The search is now scoped with `find -newermt` to a DB this attempt actually touched; with none, the fallback yields nothing and the retry/failure path takes over. This also retires the `ls`-parsing shellcheck flagged as SC2012. Prior-comment cleanup deleted by marker alone The HTML marker is plain text in a public comment and the filter ignored the author, so anyone who pasted the marker into a comment had it deleted on the next run -- and the job holds `issues: write`, so the delete succeeded. Deletion is now restricted to comments authored by `github-actions[bot]`. Also: the log path gains a run-ID suffix (a fixed name collides between concurrent jobs whenever RUNNER_TEMP is unset and /tmp is shared), the script-side author_association re-check is restored as defense in depth against a future workflow edit losing the gate, and the bare `|| true` around the agy invocation now records the exit code -- it had erased the difference between "agy failed" and "agy produced nothing". CHANGELOG gains an [Unreleased] Added + Security entry. CI-only: no crate, no shipped artifact, and nothing under crates/rustynes-{cpu,ppu,apu,mappers,core} is touched, so the deterministic chip stack and every golden vector are unchanged by construction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Jul 22, 2026
* ci: add Antigravity PR reviewer (self-hosted, Ultra) * ci(review): harden reviewer — gate triggers, argv-safe PTY, drop racy sqlite fallback * ci(review): re-sync reviewer to template — job-level concurrency, agy flock + retry * ci(review): re-harden the Antigravity reviewer after the template re-sync The template re-sync in 9875dab pulled the upstream reviewer over the hardened version from 0237135 and silently reverted five of its fixes -- three of which CodeRabbit had already marked "Addressed in commit 0237135" on this PR, so the resolved threads no longer described the tree. This restores the hardening on top of the re-sync's genuine improvements (job-level concurrency, the agy flock, the retry loop), and closes the remaining review findings. Trigger and execution gating (workflow) The `pull_request` fork check was dropped, so any fork PR could schedule work on the self-hosted runner. GitHub's "require approval for outside collaborators" is not a substitute: its default covers only FIRST-TIME contributors, so a returning outside contributor gets a runner on the maintainer's own hardware. The `head.repo.full_name == github.repository` condition is restored, alongside the `issue_comment` author_association gate. The checkout now takes the DEFAULT BRANCH rather than the PR head. The job executes the checked-out `scripts/agy-review.sh` with a token in scope, so checking out the PR let the reviewed change rewrite its own reviewer -- code execution on the runner from an untrusted diff. Review content is unaffected: the diff comes from the API via `gh pr diff`, not the working tree. The consequence is worth stating plainly: a PR that edits the reviewer or the style guide is reviewed by the version already on main. `persist-credentials: false` joins it per the repo-wide rule from PR #319, and the action moves to `actions/checkout@v7`, matching the other 19 checkout sites (this was the only `@v4` in the repo). Token exposure at the agy boundary `--dangerously-skip-permissions` stays: it is required for headless operation -- without it agy blocks on an interactive approval no one is there to answer and the run times out. Removing the flag, as suggested, would break the feature rather than secure it. What the flag actually costs is the approval gate, so the agent could act on instructions embedded in the attacker-controlled diff it is reviewing; the exposure that matters is the GitHub token in the job environment. agy is therefore launched through `env -u GH_TOKEN -u GITHUB_TOKEN`. `gh` runs in this script, before and after, and agy never needs the token. `--sandbox` still confines filesystem and network access. Command injection in the script(1) fallback The re-sync replaced the argv-safe `_agy_pty.py` wrapper with `script -qfec "... ${flags[*]}"`. `script -c` takes a command STRING and runs it through `sh -c`, so interpolating the flags array raw makes any space or metacharacter in AGY_MODEL / AGY_EFFORT / AGY_PRINT_TIMEOUT -- all env-settable -- into shell syntax. The command string is now assembled with `printf '%q '`, which emits a shell-safe rendering of each element. Verified: a flag value of `5m; touch /tmp/AGY_PWNED` now arrives as one literal argv element and the injected command does not run. Single-argument size ceiling (a live E2BIG, not a hypothetical) The prompt reaches agy as ONE argv string, and Linux caps a single argument at MAX_ARG_STRLEN = 32 * PAGE_SIZE = 128 KiB -- a separate and far lower ceiling than ARG_MAX (4 MiB here). The workflow shipped `MAX_DIFF_BYTES: "200000"`, which exceeds it outright, and capping the diff alone would not be sufficient anyway since the instruction boilerplate and the style guide ride in the same string. Measured locally on this kernel: 131000 bytes execs, 131073 fails, 200000 fails. The diff budget drops to 90 KB and a new MAX_PROMPT_BYTES (120 KB) caps the ASSEMBLED prompt. Conversation-database fallback could publish an unrelated session The `ls -t | head -1` fallback takes the globally newest conversation DB, so when agy fails BEFORE creating a conversation it posts the last assistant message from whatever else ran on that host -- another repo's review, or the owner's interactive chat -- into a public PR comment. The search is now scoped with `find -newermt` to a DB this attempt actually touched; with none, the fallback yields nothing and the retry/failure path takes over. This also retires the `ls`-parsing shellcheck flagged as SC2012. Prior-comment cleanup deleted by marker alone The HTML marker is plain text in a public comment and the filter ignored the author, so anyone who pasted the marker into a comment had it deleted on the next run -- and the job holds `issues: write`, so the delete succeeded. Deletion is now restricted to comments authored by `github-actions[bot]`. Also: the log path gains a run-ID suffix (a fixed name collides between concurrent jobs whenever RUNNER_TEMP is unset and /tmp is shared), the script-side author_association re-check is restored as defense in depth against a future workflow edit losing the gate, and the bare `|| true` around the agy invocation now records the exit code -- it had erased the difference between "agy failed" and "agy produced nothing". CHANGELOG gains an [Unreleased] Added + Security entry. CI-only: no crate, no shipped artifact, and nothing under crates/rustynes-{cpu,ppu,apu,mappers,core} is touched, so the deterministic chip stack and every golden vector are unchanged by construction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci(review): drop the shared conversation-store fallback; state the trust model Two Major findings from the re-review of e5ccbc9, both adopted. Remove the SQLite conversation-store fallback entirely The previous round scoped it with `find -newermt` against a per-attempt timestamp, which was an improvement but not the fix. mtime bounds a time WINDOW, not ownership: `$HOME/.gemini/antigravity-cli/conversations` is shared across every agy session on the host, and the flock in this script serializes review JOBS only -- it does nothing about the owner's own interactive agy usage on the same machine. Any session that touches a DB during the window could still be selected and posted into a PUBLIC pull-request comment. The sound version of this fallback needs a conversation ID or a private store tied to this invocation, and agy exposes neither. Relocating the store per run means relocating $HOME, which is where the OAuth session lives -- the thing that makes these reviews free under Ultra. Since ownership cannot be established, the fallback fails closed instead of guessing: no usable stdout means retry, then fail the job. Losing a review is recoverable (`/agy-review` re-runs it); publishing someone else's conversation is not. This deletes the fallback block along with the now-unused CONV_DIR and attempt_start. The PTY path (unbuffer, else script(1)) was always the actual mechanism; the store read was belt-and-suspenders for agy issue #76. Stop presenting `--sandbox` as the isolation boundary The prior comment claimed "--sandbox still confines filesystem and network access." That overclaims. Upstream antigravity-cli#36 reports that `--dangerously-skip-permissions` can auto-approve the very prompts required to escape the sandbox, and there is a published prompt-injection -> RCE/sandbox- escape writeup against the CLI. The flag stays (defense in depth, no cost), but the comment no longer rests any argument on it. The invocation site now documents where the boundary actually is: the workflow's `if:`. A fork PR cannot schedule this job, and `/agy-review` requires OWNER/MEMBER/COLLABORATOR, so a same-repo branch means the diff was pushed by someone who already holds write access -- and has far more direct means available than prompt injection. No external party's diff reaches agy. That gate is the entire defense, and the comment says so in the imperative: weaken it (add `pull_request_target`, drop the fork check) and this becomes remote code execution on the maintainer's machine. The reviewer's recommended remedy -- an ephemeral account or VM -- is recorded as the prerequisite for ever opening this to untrusted diffs, rather than silently declined. It is incompatible with the $HOME-resident OAuth session today, which is a constraint worth writing down rather than rediscovering. CHANGELOG's [Unreleased] Security entry is updated to match on both points. CI-only; no crate and no emulation-core file is touched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci(review): close the fork-PR hole on the /agy-review comment path The trust-model comment added in 675fa52 claimed "there is no path by which an external party's diff reaches agy." That claim was false, and the review caught it: the `issue_comment` branch authorizes the COMMENTER, never the DIFF. A collaborator typing `/agy-review` on a fork PR scheduled this job against that fork's diff, which `gh pr diff` fetches by number regardless of where the head branch lives. So an external diff did reach agy -- under --dangerously-skip-permissions, on the maintainer's own machine -- through the single most natural use of the feature: a maintainer asking for a review of a contributor's pull request. The workflow `if:` cannot fix this. An `issue_comment` payload carries no head-repo information at all -- only `.issue.pull_request` as a marker that the comment is on a PR -- so the same-repo condition that guards the `pull_request` trigger has nothing to evaluate. The check has to happen after the PR number is resolved, which is here. `gh pr view` now requests `isCrossRepository` alongside the title, and a fork PR exits 0 with a message pointing at the alternatives (review by hand, or push the branch into this repo). The metadata fetch is also now FAIL-CLOSED: it used to fall back to `{}` on error, which was harmless when the only field read was the title and is not harmless when the same document decides whether an untrusted diff reaches agy. A lookup failure and "same-repo" must not be the same outcome. Note the `jq` filter is `.isCrossRepository`, deliberately NOT `.isCrossRepository // empty`: jq's `//` alternative fires on `false` as well as on null, so the `// empty` form collapses the same-repo case into the unknown case and refuses every legitimate review. Caught by table-testing the gate against all four input shapes (true / false / `{}` / non-JSON) before pushing; the first version had exactly that bug. The trust model at the invocation site is rewritten to describe what is actually enforced: the boundary is "agy only ever sees a same-repo diff", and it takes TWO checks because neither trigger is covered by one -- the workflow for `pull_request`, this script for `issue_comment`. Both are named as load-bearing, with the failure mode spelled out, so a future edit cannot remove either without reading why it exists. The CHANGELOG entry is corrected to match rather than left restating the overstated version. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
added a commit
that referenced
this pull request
Jul 23, 2026
The Antigravity reviewer could not review this repository's larger pull requests. Two independent ceilings stacked, and the second one failed silently, which is the worse of the two. GitHub's API refuses any diff over 20,000 lines with HTTP 406, so `gh pr diff` failed outright on PR #325 (67,770 lines) and the check went red six seconds in. Beneath that sat a second limit: the assembled prompt reaches agy as one argv string, and Linux caps a single argument at MAX_ARG_STRLEN = 32 * PAGE_SIZE = 128 KiB regardless of ARG_MAX, which is where MAX_DIFF_BYTES=90000 came from. A diff over that budget was truncated with `head -c`. For a 2.68 MB diff that is a review of the first 3.3% of the bytes, published under a heading that reads as a review of the pull request. A confident verdict over an arbitrary prefix is worse than no verdict, because nothing in the output says which it is. Rename detection does not rescue this and was measured rather than assumed: `git diff -M` and `-M -C --find-copies-harder` both return 67,770 lines / 2,686,694 bytes, identical to plain `git diff`. The mapper re-split rewrote each module (new `//!` preamble, a subset of the original body), so git sees content change, not renames. Excluding `Cargo.lock` saves about 3 KB. There is no compression available here; the diff is genuinely that large. The fix is to stop treating the argv ceiling as the review's ceiling. agy is an agent with file-reading tools, so argv bounds only what can be INLINED. Above MAX_DIFF_BYTES the diff is now written into agy's own workspace and the prompt points at it: * `.agy-review-work/` sits inside the checkout deliberately, so it is already part of agy's workspace -- no `--add-dir`, no sandbox exception. It is gitignored, created per run, and removed by the exit trap, which is guarded on a flag rather than on the directory existing so it can never delete a pre-existing path. * The diff is split into AGY_DIFF_PART_BYTES (150 KB) parts rather than handed over as one 2.68 MB file. A single read that size is at the mercy of whatever per-read cap the agent applies, and a silent cap is precisely the failure being replaced. The prompt lists every part with its line and byte count, and asks for a `<!-- coverage: read N/M parts -->` header, so incomplete reading is visible in the output instead of invisible in it. * The prompt instructs that the parts are consecutive slices of one file -- a hunk header can straddle a boundary -- and that an honest partial review beats a confident review of an unread diff. * AGY_PRINT_TIMEOUT_LARGE (25m) applies only on this path, and only when the caller did not set AGY_PRINT_TIMEOUT explicitly. An inlined diff arrives with the prompt; a handed-off one must be read first, one tool call per part, before reasoning starts. Timing that out would reproduce the empty-review failure from the other direction. Above that, the 406 itself is now distinguished from a genuine failure (auth, network, bad PR number, which stay fatal) and falls back to a local `git diff`. It fetches `refs/pull/N/head` and the base branch into private `refs/agy/*`, computes the merge base, and diffs -- fetching objects but never checking them out, so the working tree stays on the default branch and a PR still cannot rewrite its own reviewer. The refs are deleted by the same exit trap. That fetch needs credentials without violating the repo-wide `persist-credentials: false` rule (PR #319), and it needs them in two different environments. `AUTHORIZATION: bearer` via `http.extraheader` is what Actions' GITHUB_TOKEN accepts and is tried first; it is rejected outright by a personal token from `gh auth token` ("remote: invalid credentials"), so a hand-run falls through to a plain fetch using git's configured credential helper. Neither path writes anything into .git/config. Verified end-to-end against PR #325 through a new AGY_DRY_RUN escape hatch, which exercises diff acquisition and prompt assembly without spending an agy run or posting a comment: the API 406 is caught, the local fallback produces 67,770 lines / 2,686,694 bytes, 18 parts are written whose sizes sum to exactly the diff, and the assembled prompt is 4,322 bytes against a 120,000-byte cap. shellcheck reports nothing beyond the pre-existing SC1007 on line 93; actionlint is clean. Note on rollout: the workflow checks out the default branch, never the PR head, so this fix does not apply to the check on the PR that introduces it -- that job keeps running main's copy of the script until this merges. That is the security property working as designed, not a regression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #318. Follow-up to #317, where CodeRabbit raised this and the fix was applied to the one new job.
Problem
actions/checkoutdefaults topersist-credentials: true, writing the workflowGITHUB_TOKENinto.git/config. Any code the job then executes from the checkout — Cargo build scripts, proc macros, test binaries, Gradle build scripts,scripts/*.sh, the MkDocs build — can read it. On a pull request that tree is by definition unreviewed code, and every CI job here compiles and runs it.18 of 19 checkouts hardened. One deliberate exception, below.
This was audited, not applied blanket
Three findings shaped the result:
1.
.github/actions/rust-setupperforms no checkout of its own (it documents this at L5-6) despite being used by 12 call sites — so hardening at each call site is complete coverage, with no hidden checkout to miss.2. No job actually needed the persisted credential. There is exactly one Git network operation in the entire
.github/tree. Nothing pushes commits, tags, or branches; there are no submodules; Pages uses the OIDC flow (configure-pages/upload-pages-artifact/deploy-pages), not agh-pagespush, and itsdeployjob has no checkout at all.The usual false positives were each checked and dismissed:
gh release create(release-auto.yml:159)GH_TOKEN→ REST APIsoftprops/action-gh-release@v3(release.yml:150)GITHUB_TOKEN→ REST APIactions/deploy-pages@v5(web.yml:187)fastlane match(ios.yml:128)MATCH_GIT_*secretspersist-credentialsonly affects git network operations relying on the stored credential — not theghCLI, not API calls. That distinction is the main source of false "needs credentials" classifications.3. The highest-exposure site is not a
ci.ymljob.web.ymldeclarespages: write+id-token: writeat the workflow level (L52-55), so that block also covers itsbuildjob — which is PR-reachable and runstrunk,cargo doc,pip install, andmkdocs build. That is the most valuable token in the repo sitting in the job with the widest untrusted-code surface.The one deliberate exception
release-auto.yml'spreparekeeps its credentials, and now carries an inline comment saying why:git ls-remote --tags origin, L88);workflow_run, further gated toevent == 'push');So hardening buys nothing there while risking a release path only exercised at a version cut. The comment records the safe migration order for anyone later wanting uniformity: convert the
ls-remotetogh api .../git/ref/tags/$tagfirst, then drop the credentials. That swap also fixes a latent bug — the current>/dev/null 2>&1makes a transient failure indistinguishable from "tag does not exist", which would publish a duplicate release.Worth a second look during review
ci.yml'schangesjob is the only place this touches the git layer at all, sincedorny/paths-filterreads local git onpushevents. It needs no authentication (fetch-depth: 0means the base commit is already local; thepull_requestpath uses the GitHub API), so this should be inert. An inline comment records the reasoning and the benign failure mode — if it were ever wrong, the filter reports "everything changed" and the full suite runs, costing only the docs-only skip.Post-merge verification: confirm a docs-only push to
mainstill skips the heavy jobs.Not included
zizmor's
unpinned-uses(no action pinned to a commit SHA). Different risk class, and SHA-pinning is a maintenance commitment that needs thegithub-actionsDependabot ecosystem enabled first — otherwise it trades a supply-chain risk for a stale-action risk, which is a live concern in a repo that has already had to move off Node-20-deprecated action majors.One sub-item is worth doing soon on its own, and is called out in #318:
.github/actions/rust-setup:41usesdtolnay/rust-toolchain@master— a branch ref that advances on every upstream commit, unlike the@vNtags used everywhere else. That composite feeds 12 of the 19 checkouts includingrelease.yml(contents: write) andweb.yml(pages: write+id-token: write), and it is the action that installs the compiler. Upstream publishes av1tag, so it is a one-line fix.Scope
Purely additive CI configuration. No source, build output, or emulation behavior changes — AccuracyCoin and every golden vector are untouched by construction.
CI green on this PR exercises 10 of the changed sites directly.
android.ymlispaths-gated andpgo.yml/ios.yml/release.ymlare tag/dispatch-only, so those are covered by inspection plus the reasoning above rather than by this run.Summary by CodeRabbit
Security
Documentation