From 84f7c445890b6796f2a730d205b0972f56167363 Mon Sep 17 00:00:00 2001 From: "Sahil H. Mobaidin" Date: Mon, 5 Oct 2026 00:56:20 +0300 Subject: [PATCH] fix: bind fork reviews to immutable commits --- .github/workflows/fork-review.yml | 75 +++++++++-- .github/workflows/test.yml | 21 +++ README.md | 7 +- tests/fork-review.test.cjs | 209 ++++++++++++++++++++++++++++++ 4 files changed, 298 insertions(+), 14 deletions(-) create mode 100644 .github/workflows/test.yml create mode 100644 tests/fork-review.test.cjs diff --git a/.github/workflows/fork-review.yml b/.github/workflows/fork-review.yml index 93ee416..292d0a9 100644 --- a/.github/workflows/fork-review.yml +++ b/.github/workflows/fork-review.yml @@ -22,9 +22,9 @@ on: default: "high" type: string codex_version: - description: Optional Codex CLI version passed through to openai/codex-action + description: Exact stable version of the Codex CLI and Responses API proxy required: false - default: "" + default: "0.160.0" type: string review_focus: description: Additional review focus instructions @@ -138,7 +138,13 @@ jobs: AZURE_OPENAI_API_KEY: ${{ secrets.azure_openai_api_key }} AZURE_OPENAI_RESPONSES_ENDPOINT: ${{ secrets.azure_openai_responses_endpoint }} AZURE_OPENAI_CODEX_MODEL: ${{ inputs.codex_model }} + CODEX_VERSION: ${{ inputs.codex_version }} run: | + if ! [[ "$CODEX_VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + echo "codex_version must be an exact stable version, such as 0.160.0" >&2 + exit 1 + fi + missing=() [ -n "$AZURE_OPENAI_API_KEY" ] || missing+=("secret azure_openai_api_key") @@ -187,16 +193,27 @@ jobs: return; } - const files = await github.paginate(github.rest.pulls.listFiles, { + // Both responses must describe the authorized commits, even if the + // contributor pushes and resets the branch while requests are in flight. + const comparison = { owner: context.repo.owner, repo: context.repo.repo, - pull_number: prNumber, - per_page: 100, + basehead: `${process.env.EXPECTED_BASE_SHA}...${process.env.EXPECTED_HEAD_SHA}`, + }; + const { data: comparisonData } = await github.rest.repos.compareCommitsWithBasehead({ + ...comparison, + per_page: 1, + page: 1, }); - const diffResponse = await github.request("GET /repos/{owner}/{repo}/pulls/{pull_number}", { - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: prNumber, + // GitHub caps comparison files at 300, even when commits are paginated. + // Refuse a potentially incomplete file list instead of claiming full coverage. + const files = comparisonData.files; + if (!Array.isArray(files) || files.length >= 300) { + core.setFailed("The immutable comparison file list is missing or may be truncated (300-file limit)"); + return; + } + const diffResponse = await github.request("GET /repos/{owner}/{repo}/compare/{basehead}", { + ...comparison, headers: { accept: "application/vnd.github.v3.diff" }, }); if (typeof diffResponse.data !== "string") { @@ -326,6 +343,7 @@ jobs: if: ${{ needs.codex-review.result == 'success' && needs.codex-review.outputs.final_message != '' }} runs-on: ubuntu-latest permissions: + contents: read # Read the immutable comparison for inline comment locations. issues: write # Upsert the summary comment from this fresh job. pull-requests: write # Post inline review comments from this fresh job. steps: @@ -335,6 +353,7 @@ jobs: CODEX_FINAL_MESSAGE: ${{ needs.codex-review.outputs.final_message }} PR_NUMBER: ${{ needs.resolve-pr.outputs.pr_number }} PR_HEAD_SHA: ${{ needs.resolve-pr.outputs.head_sha }} + PR_BASE_SHA: ${{ needs.resolve-pr.outputs.base_sha }} with: github-token: ${{ github.token }} retries: 3 @@ -345,6 +364,7 @@ jobs: const repo = context.repo.repo; const pull_number = Number(process.env.PR_NUMBER); const commit_id = process.env.PR_HEAD_SHA; + const base_sha = process.env.PR_BASE_SHA; const raw = process.env.CODEX_FINAL_MESSAGE || ""; const summaryMarker = ""; const severityOrder = { high: 0, medium: 1, low: 2 }; @@ -366,12 +386,35 @@ jobs: return; } + const text = [ + review.summary, + ...review.findings.flatMap((finding) => [finding?.title, finding?.body]), + ].filter(Boolean).join("\n").toLowerCase(); + const claimsMissingDiff = + text.includes("unable to inspect") || + text.includes("couldn’t inspect") || + text.includes("could not inspect") || + text.includes("cannot inspect") || + text.includes("don't have the actual changed files") || + text.includes("do not have the actual changed files") || + text.includes("need to inspect the repository diff") || + text.includes("provide the diff output"); + const hasAnchoredFinding = review.findings.some((finding) => { + const path = String(finding?.path || "").trim(); + const line = Number(finding?.line || 0); + return path !== "" && Number.isFinite(line) && line > 0; + }); + if (claimsMissingDiff && !hasAnchoredFinding) { + core.setFailed("Codex did not inspect the supplied diff; refusing to post a non-review"); + return; + } + const { data: currentPr } = await github.rest.pulls.get({ owner, repo, pull_number, }); - if (currentPr.head.sha !== commit_id) { + if (currentPr.head.sha !== commit_id || currentPr.base.sha !== base_sha) { core.setFailed("The pull request changed during review; run /codex-review again"); return; } @@ -429,12 +472,18 @@ jobs: return severityDelta || `${a.path}:${a.line}:${a.title}`.localeCompare(`${b.path}:${b.line}:${b.title}`); }); - const files = await github.paginate(github.rest.pulls.listFiles, { + const { data: comparisonData } = await github.rest.repos.compareCommitsWithBasehead({ owner, repo, - pull_number, - per_page: 100, + basehead: `${base_sha}...${commit_id}`, + per_page: 1, + page: 1, }); + const files = comparisonData.files; + if (!Array.isArray(files) || files.length >= 300) { + core.setFailed("The immutable comparison file list is missing or may be truncated (300-file limit)"); + return; + } const reviewComments = await github.paginate(github.rest.pulls.listReviewComments, { owner, repo, diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..aa49990 --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,21 @@ +name: Test fork review safeguards + +on: + pull_request: + push: + branches: [main] + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 + with: + persist-credentials: false + - uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6.3.0 + with: + node-version: "24" + - run: node --test tests/*.test.cjs diff --git a/README.md b/README.md index 2757d1a..b06cfea 100644 --- a/README.md +++ b/README.md @@ -125,10 +125,15 @@ The fork workflow deliberately does not execute fork-controlled code: - it checks out the trusted base commit, never the fork head - it downloads the pull request diff through the GitHub API and stores it as read-only review data +- the diff, file list, and inline comment locations use the authorized base/head commit SHAs, never a moving PR ref +- it refuses comparisons with 300 or more files because GitHub may truncate their file list - it does not install dependencies, run builds or tests, load fork-controlled Codex configuration, or execute repository scripts - it runs Codex with `permission-profile: ":read-only"` and `safety-strategy: "drop-sudo"` - Codex is the final step in the Azure-secret-bearing job - a fresh job with no Azure credential and no checkout validates the structured result and posts the review +- results claiming the diff could not be inspected without any anchored findings fail before posting + +The fork workflow pins both the Codex CLI and Responses API proxy to `0.160.0` by default. Its `codex_version` override must be an exact stable version (`X.Y.Z`); empty values, tags, and version ranges are rejected. Normal `pull_request` CI for forks can continue installing dependencies and running tests under GitHub's secure defaults, which withhold repository secrets and supply a read-only token. Do not pass those jobs secrets or a write-capable token, and do not reuse their runner or executable artifacts in the fork review workflow. @@ -153,7 +158,7 @@ That means: - `codex_effort`: Codex effort level - `sandbox`: legacy Codex sandbox mode, used only when `permission_profile` is empty - `permission_profile`: Codex permission profile, default `:workspace` -- `codex_version`: optional Codex CLI version passed through to `openai/codex-action` +- `codex_version`: optional Codex CLI version passed through to `openai/codex-action`; the fork workflow defaults to `0.160.0` and requires an exact stable version - `review_focus`: extra review criteria inserted into the prompt - `extra_prompt`: extra prompt text appended after the standard review instructions diff --git a/tests/fork-review.test.cjs b/tests/fork-review.test.cjs new file mode 100644 index 0000000..14365aa --- /dev/null +++ b/tests/fork-review.test.cjs @@ -0,0 +1,209 @@ +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); +const { test } = require('node:test'); + +// Execute the actual trusted workflow scripts with mocked GitHub responses. +// No model requests, credentials, or GitHub writes are used by these tests. +const workflow = fs.readFileSync(path.join(__dirname, '../.github/workflows/fork-review.yml'), 'utf8'); +function stepBlock(name, key, indent) { + const step = workflow.split(`- name: ${name}\n`)[1]; + assert(step, `Missing step ${name}`); + const lines = step.split(`${key}: |\n`)[1].split('\n'); + const result = []; + for (const line of lines) { + if (line.trim() && !line.startsWith(' '.repeat(indent))) break; + result.push(line.slice(indent)); + } + return result.join('\n'); +} +const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor; +function script(name) { + return new AsyncFunction('require', 'process', 'context', 'github', 'core', stepBlock(name, 'script', 12)); +} +const authorize = script('Authorize fork review command'); +const prepare = script('Prepare fork diff as untrusted review data'); +const post = script('Validate and post Codex fork review'); +const base = 'a'.repeat(40), head = 'b'.repeat(40), otherHead = 'c'.repeat(40); +const pr = { + number: 2403, draft: false, title: 'Example', body: 'Example body', + base: { sha: base, repo: { full_name: 'codemod/codemod' } }, + head: { sha: head, repo: { full_name: 'contributor/codemod' } }, +}; +const context = { + eventName: 'issue_comment', repo: { owner: 'codemod', repo: 'codemod' }, + payload: { issue: { number: 2403, pull_request: {} }, comment: { body: '/codex-review', author_association: 'MEMBER' } }, +}; +const file = { filename: 'src/example.js', status: 'modified', additions: 1, deletions: 1, patch: '@@ -1 +1 @@\n-old\n+new' }; +function state() { + const result = { errors: [], outputs: {} }; + result.core = { setFailed: message => result.errors.push(message), setOutput: (key, value) => { result.outputs[key] = value; } }; + return result; +} +function assertComparison(args) { + assert.equal(args.basehead, `${base}...${head}`); + assert.equal(args.owner, 'codemod'); + assert.equal(args.repo, 'codemod'); +} + +for (const association of ['OWNER', 'MEMBER', 'NONE', 'CONTRIBUTOR', 'COLLABORATOR']) { + test(`authorization: ${association}`, async () => { + const input = structuredClone(context); + input.payload.comment.author_association = association; + const result = state(); + await authorize(require, { env: { PR_NUMBER: '2403' } }, input, { rest: { pulls: { get: async () => ({ data: pr }) } } }, result.core); + assert.equal(result.errors.length > 0, !['OWNER', 'MEMBER'].includes(association)); + }); +} + +test('a push/reset during collection cannot change the reviewed diff or file list', async t => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'fork-review-test-')); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + const result = state(); + let liveHead = head; + const github = { + rest: { + pulls: { get: async () => { liveHead = otherHead; return { data: structuredClone(pr) }; } }, + repos: { compareCommitsWithBasehead: async args => { + assert.equal(liveHead, otherHead); + assertComparison(args); + assert.equal(args.page, 1); + return { data: { files: [file] } }; + } }, + }, + request: async (route, args) => { + assert.equal(route, 'GET /repos/{owner}/{repo}/compare/{basehead}'); + assertComparison(args); + liveHead = head; + return { data: 'diff for authorized head' }; + }, + }; + await prepare(require, { env: { PR_NUMBER: '2403', EXPECTED_BASE_SHA: base, EXPECTED_HEAD_SHA: head, REVIEW_ARTIFACT_DIR: dir } }, context, github, result.core); + assert.deepEqual(result.errors, []); + assert.equal(liveHead, head); + const prompt = fs.readFileSync(result.outputs.prompt_file, 'utf8'); + assert(prompt.includes(`Head SHA: ${head}`)); + assert(prompt.includes(file.filename)); + assert(prompt.includes('diff for authorized head')); + assert.equal(fs.readFileSync(path.join(dir, 'pull-request.diff'), 'utf8'), 'diff for authorized head'); +}); + +for (const files of [undefined, Array(300).fill(file)]) { + test(`collection rejects ${files ? 'potentially truncated' : 'missing'} comparison files`, async () => { + const result = state(); + await prepare(require, { env: { PR_NUMBER: '2403', EXPECTED_BASE_SHA: base, EXPECTED_HEAD_SHA: head } }, context, + { rest: { pulls: { get: async () => ({ data: pr }) }, repos: { compareCommitsWithBasehead: async () => ({ data: { files } }) } } }, result.core); + assert.match(result.errors[0], /file list/); + assert.deepEqual(result.outputs, {}); + }); +} + +async function runPost(review, options = {}) { + const result = state(), calls = []; + const github = { + rest: { + pulls: { + get: async () => ({ data: options.currentPr || pr }), + listReviewComments: () => {}, + createReview: async args => calls.push(['review', args]), + }, + repos: { compareCommitsWithBasehead: async args => { + assertComparison(args); + return { data: { files: options.files || [file] } }; + } }, + issues: { + listComments: () => {}, + createComment: async args => calls.push(['summary', args]), + updateComment: async args => calls.push(['update', args]), + }, + }, + paginate: async () => [], + }; + await post(require, { env: { PR_NUMBER: '2403', PR_HEAD_SHA: head, PR_BASE_SHA: base, CODEX_FINAL_MESSAGE: typeof review === 'string' ? review : JSON.stringify(review) } }, context, github, result.core); + return { ...result, calls }; +} + +for (const message of [ + 'Unable to inspect the PR diff; no review was performed.', + 'Couldn’t inspect the changed files.', 'Could not inspect the diff.', 'Cannot inspect the diff.', + "I don't have the actual changed files.", 'I do not have the actual changed files.', + 'I need to inspect the repository diff.', 'Please provide the diff output.', +]) { + test(`reject non-review: ${message}`, async () => { + const result = await runPost({ summary: message, findings: [] }); + assert.match(result.errors[0], /non-review/); + assert.deepEqual(result.calls, []); + }); +} + +test('reject a missing-diff claim hidden in an unanchored finding', async () => { + const result = await runPost({ summary: 'Review result', findings: [{ title: 'Blocked', body: 'Unable to inspect the diff.', path: '', line: 0 }] }); + assert.match(result.errors[0], /non-review/); + assert.deepEqual(result.calls, []); +}); + +test('post a legitimate clean review', async () => { + const result = await runPost({ summary: 'No material findings.', findings: [] }); + assert.deepEqual(result.errors, []); + assert.equal(result.calls.length, 1); + assert.equal(result.calls[0][0], 'summary'); +}); + +test('post an anchored finding even when it describes inspection limitations', async () => { + const result = await runPost({ summary: 'One bug found.', findings: [{ title: 'Missing access check', body: 'The user is unable to inspect their own record.', path: file.filename, line: 1, severity: 'high' }] }); + assert.deepEqual(result.errors, []); + assert.equal(result.calls[0][0], 'review'); + assert.equal(result.calls[0][1].commit_id, head); + assert.equal(result.calls[0][1].event, 'COMMENT'); + assert.equal(result.calls[0][1].comments[0].path, file.filename); +}); + +for (const side of ['base', 'head']) { + test(`reject a changed ${side} before posting`, async () => { + const currentPr = structuredClone(pr); + currentPr[side].sha = otherHead; + const result = await runPost({ summary: 'No material findings.', findings: [] }, { currentPr }); + assert.match(result.errors[0], /changed during review/); + assert.deepEqual(result.calls, []); + }); +} + +test('reject potentially truncated inline location data', async () => { + const result = await runPost({ summary: 'No material findings.', findings: [] }, { files: Array(300).fill(file) }); + assert.match(result.errors[0], /file list/); + assert.deepEqual(result.calls, []); +}); + +for (const raw of ['not JSON', 'null', '{}', JSON.stringify({ summary: 1, findings: [] }), 'x'.repeat(250001)]) { + test(`reject malformed or oversized output (${raw.length} characters)`, async () => { + const result = await runPost(raw); + assert.equal(result.errors.length, 1); + assert.deepEqual(result.calls, []); + }); +} + +test('untrusted output cannot redirect posting or execute shell-like text', async () => { + const result = await runPost({ summary: '$(touch marker) `command`', repo: 'other', pull_number: 1, findings: [{ title: 'Bad path', body: 'Example', path: '../../outside', line: 1, severity: 'high' }] }); + assert.deepEqual(result.errors, []); + assert.equal(result.calls.length, 1); + const [type, args] = result.calls[0]; + assert.equal(type, 'summary'); + assert.equal(args.owner, 'codemod'); + assert.equal(args.repo, 'codemod'); + assert.equal(args.issue_number, 2403); + assert(args.body.includes('$(touch marker) `command`')); +}); + +test('runtime version validation accepts only exact stable versions', () => { + const validation = stepBlock('Validate Azure OpenAI configuration', 'run', 10); + for (const version of ['0.160.0', '1.2.3', '', 'latest', '^0.160.0', '0.160.0 || touch marker']) { + const result = spawnSync('bash', ['-c', validation], { encoding: 'utf8', env: { + PATH: process.env.PATH, CODEX_VERSION: version, + AZURE_OPENAI_API_KEY: 'fake', AZURE_OPENAI_RESPONSES_ENDPOINT: 'fake', AZURE_OPENAI_CODEX_MODEL: 'fake', + } }); + assert.equal(result.status === 0, ['0.160.0', '1.2.3'].includes(version), version); + } + assert.match(workflow, /codex_version:[\s\S]*?default: "0\.160\.0"/); +});