From b5bb6f68e0aaa2588cc5f4cee12b29d3a7fe7d3f Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 29 Jul 2026 23:47:47 -0400 Subject: [PATCH] fix(reviewer): never post agy's OAuth login prompt to a PR When agy's cached Google OAuth session lapses on the self-hosted runner, `agy --print` emits its INTERACTIVE login flow to stdout instead of a review -- a live-shaped `accounts.google.com/o/oauth2/...` authorization URL plus a "paste the authorization code here" prompt and an "authentication failed or timed out" tail. That text is non-empty, so `have_text()` classified it as a valid review and the script posted it verbatim as a public PR comment. Incident: it landed on PR #330 (a Dependabot bump) as a "github-actions" review comment carrying the OAuth URL -- noise, phishing-shaped content, and an advertisement that the runner's auth had dropped. That comment has been removed. Root cause: the capture pipeline had no notion of "agy produced output, but the output is an auth prompt, not a review." `have_text()` only checks for non-whitespace bytes. Fix -- two independent layers, plus a helper: * is_auth_prompt() (new): true when a captured file carries agy's interactive-login signature -- the accounts.google.com OAuth endpoint URL, "paste the authorization code", "Authentication required. Please visit", "Waiting for authentication", or "authentication failed or timed out". Keyed on agy runtime strings a genuine review never emits, so a review that merely discusses "authentication" is not misclassified (verified against a benign review sample). * Layer 1 (retry loop): after CR-normalizing each attempt's capture and before the have_text() success break, reject an auth-prompt capture as a hard, NON-retryable failure. Re-auth is a human action on the runner host, so retrying only thrashes the backoff. Blank the capture (so no later path can post it), set auth_failed=1, and break. A dedicated post-loop branch then exits non-zero with an actionable message, ahead of the generic empty-output path. * Layer 3 (pre-post): a final hard guard immediately before `gh pr comment` -- if the fully assembled body still trips the auth-prompt signature, refuse to post and fail the job. This is the last line of defense: a public PR comment must never carry an OAuth URL regardless of any upstream change to body assembly. Behavior on a healthy, authenticated runner is unchanged: a real review never matches is_auth_prompt, so it posts exactly as before. Only the lapsed-session path changes -- from "leak the OAuth URL" to "fail loudly in the job log, post nothing." Syntax-checked (`bash -n`) and the signature validated against the exact leaked comment text, a benign authentication-discussing review, and empty output. Synced byte-identical across the shared reviewer template and RustyNES / RustySNES / RustyN64 (single source of truth). Co-Authored-By: Claude Opus 4.8 --- scripts/agy-review.sh | 47 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 4a5b86cd..b291c6cb 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -18,6 +18,20 @@ set -euo pipefail # `log: command not found` under set -e instead of warning — a latent misconfiguration trap.) log() { printf '[agy-review] %s\n' "$*" >&2; } have_text() { [ -s "$1" ] && grep -q '[^[:space:]]' "$1"; } +# True when a captured agy output is its interactive Google OAuth login flow rather than a +# review — i.e. agy's cached session has lapsed on the runner, so `--print` emitted the login +# prompt (a live-shaped OAuth URL + "paste the authorization code here") instead of a review. +# That text is non-empty, so have_text() alone would treat it as a valid review and POST it — +# leaking an OAuth URL + phishing-shaped auth prompt into a public PR comment (exactly the +# incident this guards against). Keyed on agy's own runtime prompt strings and the +# accounts.google.com OAuth-endpoint URL, none of which a genuine code review emits, so a review +# that merely discusses "authentication" is not misclassified. Used twice: to reject such a +# capture in the retry loop, and as a final hard block before posting (defense in depth). +is_auth_prompt() { + [ -s "$1" ] && grep -qiE \ + 'accounts\.google\.com/o/oauth2|paste the authorization code|Authentication required\. Please visit|Waiting for authentication|authentication failed or timed out' \ + "$1" +} # --- configuration (all env-overridable from the workflow) --------------------- AGY_BIN="${AGY_BIN:-agy}" @@ -100,6 +114,9 @@ log "reviewing ${REPO}#${PR}" # Remove every temp file on exit. Pre-declared so the trap is safe under `set -u` even if the # script exits before a given file is created. diff_file= diff_err= meta_file= prompt_file= out_file= raw= body_file= agy_diff_file= agy_work_dir= +# Set to 1 if agy printed its interactive OAuth login flow instead of a review (lapsed session); +# gates the no-post abort below. Pre-declared so `${auth_failed:-0}` is set-u-safe on every path. +auth_failed=0 # Set when the large-diff fallback below creates refs/agy/* so the trap can remove them. agy_refs_created= cleanup() { @@ -409,6 +426,19 @@ for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do # normalize CRs without sed -i (avoid in-place edit footguns) tr -d '\r' < "$out_file" > "$out_file.clean" && mv "$out_file.clean" "$out_file" + # If agy emitted its interactive OAuth login flow, its cached Google session has lapsed on this + # runner and there is NO review — just a live-shaped OAuth URL + "paste the authorization code" + # prompt. That must never reach a public PR comment (noise, phishing-shaped, and it advertises + # that the runner's auth dropped). Treat it as a hard, NON-retryable failure: re-auth is a human + # action on the runner host, so retrying only thrashes the backoff for a minute. Blank the + # capture so no later path (have_text / body assembly) can ever post it, flag it, and stop. + if is_auth_prompt "$out_file"; then + log "agy is NOT authenticated on this runner: it printed its interactive Google OAuth login flow instead of a review. Refusing to post it (it carries an OAuth URL). Re-authenticate agy on the runner host, then re-run with '/agy-review'." + : > "$out_file" + auth_failed=1 + break + fi + # NOTE: there is deliberately no SQLite-conversation-store fallback here. agy's # store is keyed by mtime, not session, and on a shared/multi-user runner the # most-recent `.db` can belong to an UNRELATED concurrent local `agy` session -- @@ -424,6 +454,14 @@ for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do done exec 9>&- 2>/dev/null || true # release the agy lock so the next queued job proceeds +# Lapsed-auth abort takes precedence over the generic empty-output path: it is a specific, +# actionable cause (re-auth agy on the runner), not a transient backend blip, and we already +# blanked $out_file above so nothing is postable. Exit non-zero WITHOUT posting anything. +if [ "${auth_failed:-0}" = "1" ]; then + log "aborting without posting: agy requires re-authentication on the runner host (no review was produced)." + exit 1 +fi + if ! have_text "$out_file"; then log "no review output after ${AGY_RETRIES} attempt(s). Check $LOG and confirm 'agy -p \"hi\"' works for this user." # Surface agy's stderr into the job log. RUNNER_TEMP is wiped between jobs, so a bare @@ -466,5 +504,14 @@ gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ fi done +# Final hard guard — the last line of defense. Layer 1 (the retry loop) already rejects an +# auth-prompt capture, but a public PR comment must NEVER carry a live-shaped OAuth URL or a +# "paste the authorization code" prompt, whatever any upstream change does to the body. If the +# assembled body trips the auth-prompt signature, fail the job instead of leaking it. +if is_auth_prompt "$body_file"; then + log "refusing to post: the assembled comment body contains an agy auth prompt / OAuth URL. Re-authenticate agy on the runner host." + exit 1 +fi + gh pr comment "$PR" --repo "$REPO" --body-file "$body_file" log "posted review to ${REPO}#${PR}"