From cca76e20d2576ef999d5676298d654f162293dcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20Harjam=C3=A4ki?= Date: Fri, 10 Jul 2026 17:16:15 +0300 Subject: [PATCH 1/3] feat(38-01): add fail-closed auto-merge eligibility classifier - classify-automerge-eligibility.ps1: IN-CLASS iff dependabot-manifest-only OR docs-only; workflow/executable denylist applies to ALL authors including dependabot (checker blocker #2 regression fixture covered) - decision derives from gh pr view changed paths + author only, never labels/title - auto-merge-eligibility.yml wires the classifier as a required status check (context: automerge-eligibility) on pull_request events - -SelfTest covers 5 table-driven fixtures: pure-docs, docs+workflow-file, dependabot-manifest-only, dependabot+workflow-file (regression), mixed --- .../classify-automerge-eligibility.ps1 | 268 ++++++++++++++++++ .github/workflows/auto-merge-eligibility.yml | 28 ++ 2 files changed, 296 insertions(+) create mode 100644 .github/scripts/classify-automerge-eligibility.ps1 create mode 100644 .github/workflows/auto-merge-eligibility.yml diff --git a/.github/scripts/classify-automerge-eligibility.ps1 b/.github/scripts/classify-automerge-eligibility.ps1 new file mode 100644 index 0000000..07efb52 --- /dev/null +++ b/.github/scripts/classify-automerge-eligibility.ps1 @@ -0,0 +1,268 @@ +<# +.SYNOPSIS + Fail-closed auto-merge eligibility classifier (REQ-1.5.1 / Phase 38-01 Task 1). +.DESCRIPTION + Classifies a PR's changed-file set as IN-CLASS (safe to auto-merge via the + review-bot flow, see docs/merge-flow-policy.md) or OUT-OF-CLASS (falls back + to ordinary human review). The decision is derived strictly from the actual + changed paths returned by `gh pr view --json files` plus the PR author from + `gh pr view --json author` -- NEVER from labels or title, which are + author-controllable and therefore untrusted input for this decision. + + IN-CLASS iff EITHER: + (a) the author login is 'dependabot[bot]' AND every changed path is a + dependency-manifest file (package*.json, requirements*.txt, + *.csproj, Directory.Packages.props, common lockfiles), OR + (b) every changed path matches the docs allowlist + (**/*.md, **/*.mdx, docs/**, including docs/wiki/assets/** and + docs/wiki/diagrams/**). + + The denylist (workflow files under .github/workflows/**, executables, + scripts, and non-docs config/source files) applies to ALL authors, + including dependabot: a dependabot PR that touches .github/workflows/** + (its github-actions ecosystem PRs legitimately do) is OUT-OF-CLASS, even + though every other changed path is a manifest file. This is checked FIRST, + before the manifest/docs checks, so it cannot be bypassed by author + identity (regression coverage for plan-checker blocker #2). + + Classification never depends on PR labels or PR title. + +.PARAMETER PrNumber + The PR number to classify. +.PARAMETER Repo + The "owner/repo" slug, as accepted by `gh pr view --repo`. +.PARAMETER SelfTest + Runs the built-in table-driven fixtures instead of calling gh. Exits 0 if + all fixtures pass, 1 otherwise. Use this in CI/local verification; it does + not require network access or a gh session. +.EXAMPLE + pwsh -File classify-automerge-eligibility.ps1 -PrNumber 42 -Repo Coding-Autopilot-System/autogen + pwsh -File classify-automerge-eligibility.ps1 -SelfTest +#> +[CmdletBinding()] +param( + [int]$PrNumber, + [string]$Repo, + [switch]$SelfTest +) +$ErrorActionPreference = 'Stop' + +# --- Pattern tables ----------------------------------------------------- + +# Docs allowlist: markdown, mdx, and anything under docs/ (this already +# covers docs/wiki/assets/** and docs/wiki/diagrams/** since those are +# subpaths of docs/). +$script:DocsPatterns = @( + '\.md$', + '\.mdx$', + '^docs/' +) + +# Dependency-manifest patterns: only relevant when the author is +# dependabot[bot]. Deliberately narrow -- these are the files a dependency +# bump legitimately touches. +$script:ManifestPatterns = @( + '(^|/)package(-lock)?\.json$', + '(^|/)requirements.*\.txt$', + '\.csproj$', + '(^|/)Directory\.Packages\.props$', + '(^|/)yarn\.lock$', + '(^|/)pnpm-lock\.yaml$', + '(^|/)Gemfile\.lock$', + '(^|/)poetry\.lock$', + '(^|/)Cargo\.lock$', + '(^|/)composer\.lock$' +) + +# Explicit deny extensions/paths that always force OUT-OF-CLASS, evaluated +# BEFORE the manifest check so dependabot cannot smuggle a workflow-file +# change through as part of a "manifest-only" changeset. +$script:WorkflowDenyPatterns = @( + '^\.github/workflows/' +) + +$script:ExecDenyPatterns = @( + '\.ya?ml$', + '\.ps1$', + '\.py$', + '\.cs$', + '\.ts$', + '\.js$', + '(^|/)Dockerfile' +) + +function Test-PathAgainstPatterns { + param( + [Parameter(Mandatory = $true)][string]$Path, + [Parameter(Mandatory = $true)][string[]]$Patterns + ) + foreach ($pattern in $Patterns) { + if ($Path -match $pattern) { return $true } + } + return $false +} + +function Get-PathClass { + <# + Classifies a single changed path into one of: 'deny', 'docs', 'manifest', + 'other'. Order is significant: workflow-deny is checked first so it can + never be shadowed by a manifest or docs match (blocker #2 fix). + #> + param([Parameter(Mandatory = $true)][string]$Path) + + $normalized = $Path -replace '\\', '/' + + if (Test-PathAgainstPatterns -Path $normalized -Patterns $script:WorkflowDenyPatterns) { + return 'deny' + } + if (Test-PathAgainstPatterns -Path $normalized -Patterns $script:DocsPatterns) { + return 'docs' + } + if (Test-PathAgainstPatterns -Path $normalized -Patterns $script:ManifestPatterns) { + return 'manifest' + } + if (Test-PathAgainstPatterns -Path $normalized -Patterns $script:ExecDenyPatterns) { + return 'deny' + } + if ($normalized -match '\.json$') { + # Non-docs, non-manifest JSON (e.g. tsconfig.json, app config) -- deny. + return 'deny' + } + # Fail-closed: any path we do not explicitly recognize is denied rather + # than silently allowed. + return 'deny' +} + +function Get-Classification { + <# + Returns a pscustomobject with Decision ('IN-CLASS' | 'OUT-OF-CLASS') and + Reason (populated for OUT-OF-CLASS). + #> + param( + [Parameter(Mandatory = $true)][string[]]$Paths, + [Parameter(Mandatory = $true)][AllowEmptyString()][string]$Author + ) + + if (-not $Paths -or @($Paths).Count -eq 0) { + return [pscustomobject]@{ Decision = 'OUT-OF-CLASS'; Reason = 'no changed files reported' } + } + + $classes = @($Paths) | ForEach-Object { [pscustomobject]@{ Path = $_; Class = (Get-PathClass -Path $_) } } + + $denied = @($classes | Where-Object { $_.Class -eq 'deny' }) + if ($denied.Count -gt 0) { + $deniedList = ($denied | ForEach-Object { $_.Path }) -join ', ' + return [pscustomobject]@{ + Decision = 'OUT-OF-CLASS' + Reason = "denylisted path(s) present (workflow/executable/non-docs config): $deniedList" + } + } + + $isDependabot = ($Author -eq 'dependabot[bot]') + $allManifest = -not (@($classes | Where-Object { $_.Class -ne 'manifest' }).Count -gt 0) + $allDocs = -not (@($classes | Where-Object { $_.Class -ne 'docs' }).Count -gt 0) + + if ($isDependabot -and $allManifest) { + return [pscustomobject]@{ Decision = 'IN-CLASS'; Reason = $null } + } + if ($allDocs) { + return [pscustomobject]@{ Decision = 'IN-CLASS'; Reason = $null } + } + + if ($isDependabot -and -not $allManifest) { + return [pscustomobject]@{ Decision = 'OUT-OF-CLASS'; Reason = 'dependabot PR touches non-manifest path(s); manifest-only requirement not met' } + } + return [pscustomobject]@{ Decision = 'OUT-OF-CLASS'; Reason = 'changeset is not pure-docs and author is not dependabot[bot] with manifest-only changes' } +} + +# --- Self-test fixtures -------------------------------------------------- + +function Invoke-SelfTest { + $fixtures = @( + [pscustomobject]@{ + Name = 'pure-docs' + Paths = @('docs/merge-flow-policy.md', 'README.md') + Author = 'someuser' + Expected = 'IN-CLASS' + }, + [pscustomobject]@{ + Name = 'docs-plus-one-workflow-file' + Paths = @('docs/foo.md', '.github/workflows/ci.yml') + Author = 'someuser' + Expected = 'OUT-OF-CLASS' + }, + [pscustomobject]@{ + Name = 'dependabot-manifest-only' + Paths = @('package.json', 'package-lock.json') + Author = 'dependabot[bot]' + Expected = 'IN-CLASS' + }, + [pscustomobject]@{ + Name = 'dependabot-with-workflow-file' + Paths = @('package.json', '.github/workflows/ci.yml') + Author = 'dependabot[bot]' + Expected = 'OUT-OF-CLASS' + }, + [pscustomobject]@{ + Name = 'mixed-docs-and-code' + Paths = @('docs/foo.md', 'src/app.ts') + Author = 'someuser' + Expected = 'OUT-OF-CLASS' + } + ) + + $pass = 0 + $fail = 0 + foreach ($fx in $fixtures) { + $result = Get-Classification -Paths $fx.Paths -Author $fx.Author + if ($result.Decision -eq $fx.Expected) { + Write-Host ("PASS {0} -> {1}" -f $fx.Name, $result.Decision) -ForegroundColor Green + $pass++ + } + else { + Write-Host ("FAIL {0} -> expected {1}, got {2} ({3})" -f $fx.Name, $fx.Expected, $result.Decision, $result.Reason) -ForegroundColor Red + $fail++ + } + } + Write-Host ("SelfTest: {0} passed, {1} failed" -f $pass, $fail) + return ($fail -eq 0) +} + +# --- Entry point ----------------------------------------------------------- + +if ($SelfTest) { + if (Invoke-SelfTest) { exit 0 } else { exit 1 } +} + +if (-not $PrNumber -or -not $Repo) { + Write-Error "PrNumber and Repo are required unless -SelfTest is used." + exit 1 +} + +$ghJson = & gh pr view $PrNumber --repo $Repo --json files, author 2>$null +if (-not $ghJson) { + Write-Output "automerge-eligibility: OUT-OF-CLASS reason=unable to read PR files/author via gh (fail-closed)" + exit 1 +} + +try { + $prData = $ghJson | ConvertFrom-Json +} +catch { + Write-Output "automerge-eligibility: OUT-OF-CLASS reason=unable to parse gh pr view output (fail-closed)" + exit 1 +} + +$changedPaths = @($prData.files | ForEach-Object { $_.path }) +$authorLogin = $prData.author.login + +$decision = Get-Classification -Paths $changedPaths -Author $authorLogin + +if ($decision.Decision -eq 'IN-CLASS') { + Write-Output "automerge-eligibility: IN-CLASS" + exit 0 +} +else { + Write-Output ("automerge-eligibility: OUT-OF-CLASS reason={0}" -f $decision.Reason) + exit 1 +} diff --git a/.github/workflows/auto-merge-eligibility.yml b/.github/workflows/auto-merge-eligibility.yml new file mode 100644 index 0000000..ba724e8 --- /dev/null +++ b/.github/workflows/auto-merge-eligibility.yml @@ -0,0 +1,28 @@ +name: "Auto-merge eligibility" +on: + pull_request: + types: + - opened + - synchronize + - reopened +permissions: + contents: read + pull-requests: read + +jobs: + automerge-eligibility: + name: automerge-eligibility + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Checkout repository + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + + - name: Classify PR eligibility + shell: pwsh + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + pwsh -File .github/scripts/classify-automerge-eligibility.ps1 ` + -PrNumber ${{ github.event.pull_request.number }} ` + -Repo ${{ github.repository }} From 24b9832bc6a538d6da9c94ebef7688a701b7e471 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20Harjam=C3=A4ki?= Date: Fri, 10 Jul 2026 17:21:27 +0300 Subject: [PATCH 2/3] feat(38-01): add review-bot workflow (App approval gated on critic_cli) - pull_request_target trigger; mints a cas-review-bot App installation token via actions/create-github-app-token (pinned SHA bcd2ba4, v3.2.0) - gate 1: eligibility classifier -- OUT-OF-CLASS stops before any approval - gate 2: checks out autogen at pinned commit b0524b7 (verified on origin/main via PR #17; never feat/phase-29-peer-critic) and runs critic_cli --severity-gate blocking against the PR diff; non-zero exit requests changes instead of approving - gate 3: gh pr checks --required must be green - only when all three gates pass: gh pr review --approve as the App token, then gh pr merge --auto --squash -- the App is the sole approving identity, never GITHUB_TOKEN or an agent PAT --- .github/workflows/review-bot.yml | 134 +++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 .github/workflows/review-bot.yml diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml new file mode 100644 index 0000000..570f9a2 --- /dev/null +++ b/.github/workflows/review-bot.yml @@ -0,0 +1,134 @@ +name: "Review Bot" +on: + pull_request_target: + types: + - opened + - synchronize + - reopened +permissions: + contents: read + pull-requests: read + +jobs: + review: + name: review-bot + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + # Base-ref checkout only -- this workflow never checks out or executes + # the untrusted PR head; it only reads a computed diff as text and + # feeds it to the pinned, trusted autogen critic (see below). + - name: Checkout repository (base ref, scripts only) + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + + - name: Set up Python + uses: actions/setup-python@e797f83bcb11b83ae66e0230d6156d7c80228e7c # v6.0.0 + with: + python-version: "3.12" + + - name: Mint review-bot App installation token + id: app-token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ secrets.REVIEW_BOT_APP_ID }} + private-key: ${{ secrets.REVIEW_BOT_PRIVATE_KEY }} + + # --- Gate 1: eligibility classifier ------------------------------- + - name: Classify eligibility + id: classify + shell: pwsh + continue-on-error: true + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + run: | + pwsh -File .github/scripts/classify-automerge-eligibility.ps1 ` + -PrNumber ${{ github.event.pull_request.number }} ` + -Repo ${{ github.repository }} + if ($LASTEXITCODE -eq 0) { + "eligible=true" | Out-File -FilePath $env:GITHUB_OUTPUT -Append -Encoding utf8 + } else { + "eligible=false" | Out-File -FilePath $env:GITHUB_OUTPUT -Append -Encoding utf8 + } + + - name: Out-of-class - no approval, human review path + if: steps.classify.outputs.eligible != 'true' + run: | + echo "PR is OUT-OF-CLASS. review-bot will not approve or merge. Falling back to human review." + + # --- Gate 2: deterministic critic (autogen critic_cli, pinned) ---- + - name: Compute PR diff (base...head) + id: diff + if: steps.classify.outputs.eligible == 'true' + run: | + set -euo pipefail + git fetch origin "${{ github.event.pull_request.base.ref }}" --depth=100 + git fetch origin "pull/${{ github.event.pull_request.number }}/head:pr-head" --depth=100 + git diff "origin/${{ github.event.pull_request.base.ref }}...pr-head" > "${{ github.workspace }}/pr.diff" + + - name: Checkout autogen (pinned commit, for critic_cli) + if: steps.classify.outputs.eligible == 'true' + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + repository: Coding-Autopilot-System/autogen + # Pinned to origin/main commit b0524b7 (merged PR #17, + # "consolidate refresh stack and workflow hardening"), verified + # via `git log origin/main -- maf_starter/critic_cli.py`. NEVER + # point this at feat/phase-29-peer-critic (closed, unmerged PR + # #14 -- see docs/merge-flow-policy.md). + ref: b0524b762024237d047fde25b50679da42e1a5e2 + path: autogen-critic + continue-on-error: true + id: checkout-autogen + + - name: Pin verification guard + if: steps.classify.outputs.eligible == 'true' && steps.checkout-autogen.outcome != 'success' + run: | + echo "autogen checkout at the pinned critic_cli SHA failed -- treating as a hard stop (fail-closed). No approval will be issued." >&2 + exit 1 + + - name: Run critic_cli against PR diff + id: critic + if: steps.classify.outputs.eligible == 'true' + working-directory: autogen-critic + env: + PYTHONPATH: ${{ github.workspace }}/autogen-critic + run: | + set -o pipefail + python -m pip install --quiet -r requirements.txt + python -m maf_starter.critic_cli --diff "${{ github.workspace }}/pr.diff" --severity-gate blocking \ + | tee "${{ github.workspace }}/critic-output.txt" + exit_code=${PIPESTATUS[0]} + echo "critic_exit=${exit_code}" >> "$GITHUB_OUTPUT" + continue-on-error: true + + - name: Request changes - critic findings block auto-merge + if: steps.classify.outputs.eligible == 'true' && steps.critic.outputs.critic_exit != '0' + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + run: | + gh pr review "${{ github.event.pull_request.number }}" --repo "${{ github.repository }}" \ + --request-changes --body-file "${{ github.workspace }}/critic-output.txt" || true + echo "critic_cli reported blocking findings (or could not run). review-bot will not approve." >&2 + + # --- Gate 3: required CI checks green ------------------------------ + - name: Confirm required CI checks are green + id: ci-green + if: steps.classify.outputs.eligible == 'true' && steps.critic.outputs.critic_exit == '0' + continue-on-error: true + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + run: | + gh pr checks "${{ github.event.pull_request.number }}" --repo "${{ github.repository }}" --required + + # --- Approve + auto-merge (App identity, all three gates clean) --- + - name: Approve and enable auto-merge (review-bot App) + if: >- + steps.classify.outputs.eligible == 'true' && + steps.critic.outputs.critic_exit == '0' && + steps.ci-green.outcome == 'success' + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + run: | + gh pr review "${{ github.event.pull_request.number }}" --repo "${{ github.repository }}" \ + --approve --body "cas-review-bot: eligibility=IN-CLASS, critic_cli=0 blocking, CI=green. Auto-approved per docs/merge-flow-policy.md." + gh pr merge "${{ github.event.pull_request.number }}" --repo "${{ github.repository }}" --auto --squash From 548d95ff86afa3037e841b3ddb6c48cae8ef56da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20Harjam=C3=A4ki?= Date: Sat, 11 Jul 2026 13:35:48 +0300 Subject: [PATCH 3/3] fix(security): eliminate pwn-request checkout in review-bot privileged job CodeQL flagged review-bot.yml (pull_request_target, App token minted) for checking out untrusted code in a privileged context: the diff step did `git fetch origin pull//head:pr-head` then diffed against it, pulling attacker-controlled ref content into the runner's git state before the critic gate ran. Replace with an API-only diff: `gh pr diff ` piped directly into critic_cli over stdin (--diff -). No git fetch/checkout of the PR head occurs anywhere in the job; the only actions/checkout calls left are the job's own base-ref self-checkout and a pinned, trusted autogen commit for critic_cli. Also add fail-closed handling if `gh pr diff` itself errors, so a failed API call can no longer be misread as "zero findings". Ref: T-38-SEC-PWN --- .github/workflows/review-bot.yml | 38 ++++++++----- ...C-PWN-review-bot-pwn-request-mitigation.md | 55 +++++++++++++++++++ 2 files changed, 80 insertions(+), 13 deletions(-) create mode 100644 docs/security/T-38-SEC-PWN-review-bot-pwn-request-mitigation.md diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml index 570f9a2..53ea222 100644 --- a/.github/workflows/review-bot.yml +++ b/.github/workflows/review-bot.yml @@ -56,15 +56,16 @@ jobs: echo "PR is OUT-OF-CLASS. review-bot will not approve or merge. Falling back to human review." # --- Gate 2: deterministic critic (autogen critic_cli, pinned) ---- - - name: Compute PR diff (base...head) - id: diff - if: steps.classify.outputs.eligible == 'true' - run: | - set -euo pipefail - git fetch origin "${{ github.event.pull_request.base.ref }}" --depth=100 - git fetch origin "pull/${{ github.event.pull_request.number }}/head:pr-head" --depth=100 - git diff "origin/${{ github.event.pull_request.base.ref }}...pr-head" > "${{ github.workspace }}/pr.diff" - + # SECURITY (T-38-SEC-PWN / CodeQL actions/checkout-of-untrusted-code, + # PR #17 review-bot.yml lines 59-68): this job runs under + # pull_request_target with a minted App token, so the PR head is + # untrusted content in a privileged context. It MUST NOT be fetched, + # checked out, or merged into the local git object database here -- + # not even via a bare `git fetch`. The diff is obtained exclusively + # through the GitHub API (`gh pr diff`, an authenticated HTTPS call + # that returns text, not a git ref) and streamed straight into + # critic_cli over stdin in the "Run critic_cli" step below. No PR-head + # commit ever touches this runner's filesystem or git state. - name: Checkout autogen (pinned commit, for critic_cli) if: steps.classify.outputs.eligible == 'true' uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -86,19 +87,30 @@ jobs: echo "autogen checkout at the pinned critic_cli SHA failed -- treating as a hard stop (fail-closed). No approval will be issued." >&2 exit 1 - - name: Run critic_cli against PR diff + - name: Run critic_cli against PR diff (diff via API, no checkout of PR head) id: critic if: steps.classify.outputs.eligible == 'true' working-directory: autogen-critic env: PYTHONPATH: ${{ github.workspace }}/autogen-critic + # Read-only default token (permissions: contents: read, pull-requests: + # read) is sufficient for `gh pr diff`; the App token is intentionally + # NOT used here (least privilege -- this step never approves/merges). + GH_TOKEN: ${{ github.token }} run: | set -o pipefail python -m pip install --quiet -r requirements.txt - python -m maf_starter.critic_cli --diff "${{ github.workspace }}/pr.diff" --severity-gate blocking \ + gh pr diff "${{ github.event.pull_request.number }}" --repo "${{ github.repository }}" \ + | python -m maf_starter.critic_cli --diff - --severity-gate blocking \ | tee "${{ github.workspace }}/critic-output.txt" - exit_code=${PIPESTATUS[0]} - echo "critic_exit=${exit_code}" >> "$GITHUB_OUTPUT" + diff_exit=${PIPESTATUS[0]} + critic_exit=${PIPESTATUS[1]} + if [ "${diff_exit}" -ne 0 ]; then + echo "gh pr diff failed (exit ${diff_exit}) -- treating as a hard stop (fail-closed). No approval will be issued." >&2 + echo "critic_exit=1" >> "$GITHUB_OUTPUT" + exit 1 + fi + echo "critic_exit=${critic_exit}" >> "$GITHUB_OUTPUT" continue-on-error: true - name: Request changes - critic findings block auto-merge diff --git a/docs/security/T-38-SEC-PWN-review-bot-pwn-request-mitigation.md b/docs/security/T-38-SEC-PWN-review-bot-pwn-request-mitigation.md new file mode 100644 index 0000000..07e53ff --- /dev/null +++ b/docs/security/T-38-SEC-PWN-review-bot-pwn-request-mitigation.md @@ -0,0 +1,55 @@ +# T-38-SEC-PWN: review-bot pwn-request mitigation + +## Finding + +CodeQL (`actions/checkout-of-untrusted-code`, "Checkout of untrusted code in a +privileged context") flagged `.github/workflows/review-bot.yml` on PR #17. +The `review` job runs on `pull_request_target`, which mints a GitHub App +installation token with `pull-requests: write` scope before the diff step +ran. The prior "Compute PR diff (base...head)" step fetched the PR head ref +directly (`git fetch origin pull//head:pr-head`) into the runner's local +git object database and then diffed against it. Fetching attacker-controlled +ref content into a privileged job's git state is the classic GitHub Actions +pwn-request pattern (see GitHub's own docs on "Preventing pwn requests"), +even though the original step never wrote that content to the working tree +or executed it directly. + +## Mitigation + +The privileged `review` job is now checkout-free of any untrusted ref: + +- The eligibility classifier (`.github/scripts/classify-automerge-eligibility.ps1`) + already sourced its input exclusively from `gh pr view --json files,author` + (API text), not from any checked-out ref. +- The diff step now calls `gh pr diff --repo ` (GitHub API over + HTTPS, returning diff text) and streams that text directly into + `critic_cli` over stdin (`--diff -`). No `git fetch`/`git checkout` of the + PR head, and no PR-head commit ever enters the runner's git object + database or filesystem. +- The only `actions/checkout` calls remaining in the job are: (1) the job's + own repository at its default/base ref (never the PR head — no `ref:` + override is supplied, so under `pull_request_target` this resolves to the + base branch), and (2) `Coding-Autopilot-System/autogen` pinned to a fixed, + trusted commit SHA for `critic_cli`. Neither checks out attacker-controlled + content. +- Added fail-closed handling: if `gh pr diff` itself fails (non-zero exit), + the job now hard-stops rather than silently treating an empty/partial diff + as "no findings" and proceeding toward approval. + +## Verification + +- Structural check: `grep` confirms zero occurrences of `git fetch .*pull/`, + `pr-head`, or `actions/checkout` with a PR-head `ref:` in + `.github/workflows/review-bot.yml`. The remaining `actions/checkout` uses + are the base-ref self-checkout and the pinned trusted `autogen` checkout. +- The change was pushed to PR #17 (`feat/merge-flow-review-bot`); GitHub's + CodeQL check re-runs automatically on `synchronize` and is expected to + clear the "Checkout of untrusted code in a privileged context" alert on + the updated lines. + +## Note on scope + +This note was added directly to PR #17 rather than to PR #18, because PR #18 +("fix(release): resolve reusable workflow reference") is an unrelated +release-engineering fix and does not contain a merge-flow threat table to +update.