Repository navigation
Fetch the pull request's head in the engine, so a calling workflow needs no checkout of untrusted code - #19
Conversation
…eds no checkout of untrusted code The workflow checked the head out with actions/checkout in a privileged workflow, which CodeQL reports as two alerts (untrusted checkout, and its TOCTOU variant) on every repository that adopts the bot. The engine now fetches the exact commit itself, after it has decided to review, with git started without configuration, hooks or a credential helper, and the token passed through the environment. A workflow that still checks the head out keeps working.
![ncode-review[bot]](https://avatars.githubusercontent.com/u/12114478?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 NCode Review at 1c4b602: 9 new findings on the changed lines.
Not on a changed line:
- 🟡 F8 Nit: The Versions section still says v0.1.0 while the example pins v0.3.0
README.md:70. The workflow example pinsNCodeGroup/code-review@v0.3.0, but the Versions section still gives@v0.1.0as its example. The new text also refers to v0.2 workflows. Fix: Use one tag throughout the README, and confirm that v0.3.0 is the release that ships this change.
🤖 Review completeVerdict: 💬 Comment — This PR moves the pull request head checkout out of the calling workflow and into the engine. The engine now fetches the exact commit with git, after it has decided to review, using an empty home folder, no hooks, no credential helper and a token scoped to the remote's origin. It also adds a fork check on the automatic pull_request path. The design holds together and the security choices are sound: the commit is fixed by SHA, the token never appears in argv or in .git/config, and a half-made checkout is removed on failure. What's left are robustness gaps: the network fetches have no timeout, the by-name fetch fallback and the new main.ts guards have no tests, and one test waits on a fixed delay. 8 reviewers run — 7 findings (run details)
Skipped: api-compat, data-model, infrastructure, intent, performance Findings
Trial: findings are posted as review threads and in the check run. $0.51 on Amazon Bedrock, 83s, at 1516681. (how to interact) |
…nd test the fetch's inputs Reviewing the engine fetch (findings F1 to F10): an existing .git is accepted only at the named commit; the fetch asks for the commit by name when a push left it off the fetched refs, and says the head moved and clears the folder when it still cannot get it; the temporary home is removed; the token's header and the remote URL are built by functions the tests exercise; the fetch takes the parsed pull request's head; the tests run without the machine's git configuration and remove their folders; the README names one version and describes current behavior.
|
Fixed in 0848176. F1: an existing checkout is accepted only at the named commit, otherwise the run stops with a message to comment |
|
F8 (the README's Versions section still said v0.1.0) was in the review summary, not a thread: fixed in 0848176, the README now names v0.3.0 in both places. |
|
/review |
There was a problem hiding this comment.
🤖 NCode Review at 0848176: 4 new findings on the changed lines.
3 more were already posted on this pull request.
…t the token reaching the remote Second review of the engine fetch: a pull request from another repository, or from none, is refused by the engine for pull_request_target as it is for a comment; the failure reading HEAD keeps git's message; the git helper binds its folders in one argument; the wiring is two statements; the tests cover a repository with no commit, a header sent to a real server (in its own process, since the fetch blocks this one) and the fork check.
|
The three findings that were not on a changed line:
40 tests and the typecheck pass. |
There was a problem hiding this comment.
🤖 NCode Review at 1516681: 7 new findings on the changed lines.
| // or the checked-out code could hold there is read. | ||
| function git(where: { cwd: string; empty: string }, args: string[], extra: Record<string, string> = {}): string { | ||
| const { cwd, empty } = where; | ||
| return execFileSync('git', ['-C', cwd, '-c', `core.hooksPath=${empty}`, '-c', 'core.fsmonitor=false', '-c', 'advice.detachedHead=false', ...args], { |
There was a problem hiding this comment.
🟠 Should fix: Network git fetches run with no timeout F1 · correctness, rule-check
git() calls execFileSync without a timeout option, including for the two fetch calls that go over the network.
Consequence: If the remote or proxy stalls, the engine blocks synchronously until the job-level timeout kills the runner. Nothing can cancel it, no useful error is printed, and the 'comment /review' hint never appears.
Fix: Pass a timeout from a named constant (for example a few minutes) to execFileSync for the fetch calls. The timeout error then reaches the existing catch, which cleans up and reports a fetch failure.
Evidence
execFileSync('git', [...], { encoding, stdio, env }) sets no timeout; the git(where, ['fetch', ...], auth) calls at lines ~105–107 are network calls. Rule: outbound fetches must always have a timeout.
Confidence 0.45.
| git(remote, 'commit', '-q', '-m', 'three'); | ||
| git(remote, 'update-ref', 'refs/pull/1/head', git(remote, 'rev-parse', 'HEAD')); | ||
| const dir = target(); | ||
| fetchPullRequestHead({ path: dir, remoteUrl: pathToFileURL(remote).href, number: 1, sha: head }); |
There was a problem hiding this comment.
🟠 Should fix: The by-name fetch fallback never runs on its success path F2 · tests
In the 'moved' test, refs/pull/1/head is moved to a child of the named commit, so the first fetch already brings that commit down and if (!has(where, options.sha)) git(fetch origin <sha>) is skipped.
Consequence: A regression that drops or breaks the by-name fetch (the force-push case the code comment describes) still passes the suite, and in production the reviewed commit then fails to check out after a force-push.
Fix: Add a test where the named commit can't be reached from any branch or pull ref (for example, point refs/pull/1/head at an unrelated commit after recording the head) and assert that fetchPullRequestHead still checks out the named SHA. Keep the existing test as the control.
Evidence
test/fetch-head.test.ts lines 65–76 set refs/pull/1/head to commit 'three', whose parent is head; in src/fetch-head.ts the by-name fetch only runs when has() is false.
Confidence 0.70.
| // The server has no repository, so the fetch fails; what matters is what it received before that. | ||
| assert.throws(() => fetchPullRequestHead({ path: dir, remoteUrl: `http://127.0.0.1:${port}/o/r.git`, number: 1, sha: 'b'.repeat(40), token: 'ghs_tokentoken' }), /Couldn't fetch commit bbbbbbb/); | ||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
| const headers = [...output.matchAll(/^H (.*)$/gm)].map((match) => match[1]); |
There was a problem hiding this comment.
🟠 Should fix: A fixed 200 ms sleep comes before the assertion on the server's received headers F3 · tests, rule-check
After the fetch fails, the auth-header test waits a fixed setTimeout(resolve, 200) and then asserts once on the headers the child server printed.
Consequence: On a slow or loaded CI runner the child may not have flushed stdout yet, and the test fails at random with 'the server saw no request'. On a fast runner it waits longer than it needs to.
Fix: Poll output every 20–100 ms until an H line appears or a deadline of several seconds passes, then assert.
Evidence
await new Promise((resolve) => setTimeout(resolve, 200)); followed by assert.ok(headers.length > 0, ...). Rule: poll until a deadline, never a fixed delay followed by one assertion.
Confidence 0.60.
| if (isFromFork(event.pull_request, repository)) { | ||
| console.log(`#${event.pull_request?.number} comes from a fork, and forks aren't reviewed.`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟠 Should fix: The new fork guard on the event path and the engine fetch in main have no tests F4 · tests
main.ts now returns early for a fork pull_request event and calls fetchHeadFor/fetchPullRequestHead before gather, but only the helper isFromFork is tested, and only in isolation.
Consequence: A later edit could remove the fork guard on the automatic path, and fork code would be fetched and reviewed with every test still passing.
Fix: Move the decision (skip forks, skip already-reviewed heads, fetch) into a small pure function that takes the event and returns an action. Test it with a fork event, a same-repo event and a null head.
Evidence
main.ts adds if (isFromFork(event.pull_request, repository)) { ... return; } and the fetchHeadFor(...) and fetchPullRequestHead(headFetch) lines; no test drives main.
Confidence 0.40.
| }); | ||
|
|
||
| test('fetchPullRequestHead sends the token to the remote as a Basic header', async () => { | ||
| // The server runs in its own process, because the fetch blocks this one while git runs. |
There was a problem hiding this comment.
🟡 Nit: The auth-header unit test starts a real HTTP server on loopback F5 · tests, rule-check
The test spawns a node child process that listens on 127.0.0.1 and runs a real git fetch against it, which depends on local ports and on being able to spawn a process.
Consequence: It can fail or flake where loopback networking or process spawning is restricted, which conflicts with the rule that unit tests stay hermetic.
Fix: If this is the only check that the token actually reaches the wire, keep it and add a comment saying why, or move it to an integration suite. Otherwise rely on the existing authEnvironment test.
Evidence
Lines 142–160 spawn process.execPath -e "...createServer...listen(0,'127.0.0.1')". Rule: keep unit tests hermetic, with no network and no machine state.
Confidence 0.30.
| import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; | ||
| import { tmpdir } from 'node:os'; | ||
| import path from 'node:path'; | ||
| import { after, test } from 'node:test'; |
There was a problem hiding this comment.
🟡 Nit: The isFromFork test is in the fetch-head test file, and the imports are out of order F6 · hygiene
The isFromFork test lives in fetch-head.test.ts although the function is in command.ts, and import { pathToFileURL } from 'node:url' sits between the two ../src imports.
Consequence: A reader looking for the command.ts tests won't find this one, and the import block is harder to scan.
Fix: Move the isFromFork test to the file that tests command.ts, and put the node: imports together before the local imports.
Evidence
Line 6 imports ../src/command.ts, line 7 imports node:url, line 8 imports ../src/fetch-head.ts; the isFromFork test is the last test in the file.
Confidence 0.35.
|
|
||
| `pull_request_target` and `issue_comment` both run this file from the default branch, so a pull request can't change | ||
| its own review, and the pull request's code is only read, never built or run. Pushes start nothing; comment `/review` | ||
| its own review, and the pull request's code is only read, never built or run. The workflow doesn't check the head out: the |
There was a problem hiding this comment.
🟡 Nit: The new README paragraph breaks the existing line wrapping F7 · hygiene
The added sentence runs well past the column width the rest of the README is wrapped to.
Consequence: Only the Markdown source looks inconsistent; the rendered page is unaffected.
Fix: Re-wrap the paragraph to the README's usual width.
Evidence
The line ending ...if it is at that commit). Pushes start nothing; comment /review`` is much longer than the lines around it.
Confidence 0.30.
Every repository that adopts the bot gets the same two CodeQL alerts on
code-review.yml: checkout of untrusted code in a privileged context and its TOCTOU variant. They are accepted risks (the checkout is only read), but they repeat for each repository, mark the adoption PR'sCodeQLcheck red, and need a dismissal each time. This removes the cause.What changes
src/fetch-head.ts) after it has decided to review: a/reviewcomment from a writer, a pull request that isn't from a fork, a commit not already reviewed. A caller then needs only the checkout of its default branch.refs/pull/N/head. A push between a/reviewcomment and the checkout can no longer change what is reviewed, which the TOCTOU alert was about.GIT_CONFIG_*environment variables scoped to the remote's origin, never a command line or the remote's URL, and the test checks it isn't left in.git/config. A SHA that isn't 40 hex characters, a bad number and a non-empty folder are refused.What does not change
The risk is the same as before and is mitigated the same way: the head is data on disk, read by an agent with no settings, hooks, MCP servers or
CLAUDE.mdfrom it, read-only tools and a path guard. This moves the fetch into the audited engine, where those mitigations live, and out of every caller's workflow file. It does not make reading an untrusted checkout safer by itself, and I am not claiming it does.Rollout
code-review.ymland the repositories' workflows to v0.3.0 and delete their second checkout.mainare dismissed once; no repository gets new ones.npm test(33 tests, 4 new, against a local repository standing in for the remote) andnpm run typecheckpass. The fetch against github.com itself is untested until the first run of v0.3.0, which I'll watch.🤖 Generated with Claude Code