fix(sandbox): a checkout with no credential must arrive complete - #116
Conversation
The clone is the container's only authenticated reach at GitHub — the credential scrub rewrites `.git/config` the moment the checkout lands (ADR-0006). The SDK's `gitCheckout` owns the clone's object filter and offers no way to turn it off, so the checkout it produced still needed the network to answer questions about its own history, and had nothing left to ask with. `pr-review` is where that bit. Its three-dot `git diff <base>...<head>` reads MERGE-BASE blobs, which belong to neither tree a clone materialises — the default-branch tip it lands on, nor the head `git checkout` moves to. Git reached for the promisor remote, found no credential, and the step died after ~5 minutes on two `could not read Username for 'https://github.com'` prompts and `unable to read <oid>`. Intermittent by construction: it only bit when a merge-base blob differed from both trees on disk. `git.clone` is now three execs of its own — clone, SHA checkout, scrub — with the clone deliberately complete: no `--filter`, no `--depth`, no `--single-branch`. The URL's userinfo is stripped by the SDK's own log sanitizer before any command string reaches a log, so the token's exposure is unchanged. Completeness is a property of the primitive, not of one recipe: anything cheaper needs a credential that outlives the clone.
![flaredispatch-fractalboxdev[bot]](https://avatars.githubusercontent.com/in/4273665?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI code review — 🛑 Request changes
Risk tier: full · 1 critical · 1 warnings · 0 suggestions
Reviewers: security
1. ⚠️ Warning — Full clone can severely increase execution time and storage
📍 packages/runtime-cf/src/sandbox-clone-url.ts:104-105
'cloneCommand' intentionally omits depth, filter, and branch narrowing, so every run downloads all reachable objects and tags before the review starts. Large repositories can approach or exceed the 600-second clone timeout and consume substantially more sandbox disk than the previous filtered checkout. Consider retaining narrowly scoped credentials for a targeted post-clone fetch, or otherwise adding a bounded fetch strategy that preserves the no-network-after-scrub guarantee.
2. 🛑 Critical — Authenticated clone URL is passed directly to exec
📍 packages/runtime-cf/src/sandbox-cf.ts:530-537
The new implementation embeds the GitHub installation token in the shell command sent to 'box.exec'. Unlike the previous 'gitCheckout' API, this exposes the credential to any sandbox command tracing, process/worker diagnostics, or SDK logging that records the raw command; the nearby comment's claim that exec sanitizes the URL is not enforced by this change. It also risks propagating the token through clone stderr and subsequent error handling. Avoid placing credentials in the command string (use a credential mechanism supported by the sandbox, or explicitly guarantee and test redaction at the exec boundary before shipping).
Review flagged the first cut: moving the clone from the SDK's `gitCheckout` to `exec` put the authenticated URL — token and all — into a command string. It is redacted there, but only by `redactCommand` inside `@cloudflare/sandbox`, i.e. by an internal of a pinned dependency. ADR-0011 is about exactly that: a guarantee held up by an SDK pin can be revoked by a version bump nobody reads as a security change. A URL carrying its own credential leaks by three routes at once — the command string, git's stderr, and `.git/config` — and each one needs its own guard. Git now gets the credential-free URL and a `-c credential.helper` that reads the token from `FLARE_DISPATCH_CLONE_TOKEN`, set on the clone `exec` alone. The command holds only the variable's name, nothing git prints can contain a token, and `.git/config` never receives one. `redactCloneFailure` and `scrubCloneCredential` stay, now as backstops rather than as the only thing between an installation token and the workload. Verified against a private repo: the clone fails `Authentication failed` with the variable unset and succeeds with it set, `remote.origin.url` is credential-free from the start, no promisor config, and the three-dot diff that started this reproduces green over historical blobs with an unreachable remote. `authenticateCloneUrl` is deleted rather than left exported — nothing calls it, and leaving the URL-embedding shape available invites its return.
|
Both findings addressed in d1307d7. Critical — token in the exec command. Correct, and the fix is not a redaction test. The SDK does sanitize ( Verified against a private repo: Warning — full clone cost. Kept, deliberately. There is no bounded middle: |
There was a problem hiding this comment.
AI code review — 💬 Comment
Risk tier: full · 0 critical · 1 warnings · 0 suggestions
Reviewers: security
1. ⚠️ Warning — Authenticated clones now download the entire repository history and object database
📍 packages/runtime-cf/src/sandbox-clone-url.ts:78-111
'cloneCommand' explicitly removes filtering, depth, and branch narrowing, so every authenticated run transfers all refs and blobs up front. For large or long-lived repositories this can substantially increase network, disk, and clone latency, and the fixed 600-second timeout may turn repositories that previously completed into failures. Consider preserving a complete-history fallback only when the review workload requires it, or otherwise adding a bounded/conditional clone strategy with a credential that remains available for lazy fetches.
1 participant
pr-reviewhas been failing intermittently onprepare-diff— three of nine runs on one consumer repo over 24h, each burning ~5 minutes before it gave up. The cause is in the clone, not the diff.Problem & Insight
The clone is the container's only authenticated reach at GitHub — by design, nothing after it holds a credential (ADR-0006). The SDK's
gitCheckoutowns the clone's object filter and exposes no way to turn it off, so it handed back a checkout that still needed the network to answer questions about its own history, and had nothing left to ask with.pr-review's three-dotgit diff <base>...<head>reads merge-base blobs. Those belong to neither tree a clone materialises: not the default-branch tip it lands on, and not the headgit checkoutmoves to. Git fell through to the promisor remote, found no credential, and the step died:Intermittent by construction, which is why it read as flakiness: it only bit when a merge-base blob differed from both trees on disk — i.e. when the PR sat behind its base by a commit touching the same file. Runs whose PR was level with the default branch passed on the same code path.
Take
git.cloneis now three execs of its own — clone, SHA checkout, scrub — with the clone deliberately complete: no--filter, no--depth, no--single-branch. Completeness belongs to the primitive, not to one recipe; routingpr-reviewaround its own checkout (fetching the diff from the compare API) would have left the nextgitconsumer to rediscover this.Doing the clone ourselves means owning the credential, so the token no longer travels in the URL. Git gets the credential-free URL plus a
-c credential.helperthat reads it fromFLARE_DISPATCH_CLONE_TOKEN, set on the cloneexecalone. A URL carrying its own credential leaks by three routes at once — the command string, git's stderr,.git/config— each needing its own guard, and the command-string one was held up by an internal of the pinned@cloudflare/sandbox, which is what ADR-0011 says not to lean on. Reading from the environment closes all three at the source:redactCloneFailureandscrubCloneCredentialstay as backstops rather than as the only thing between an installation token and the workload.authenticateCloneUrlis deleted rather than left exported.Verified against a private repo: clone fails
Authentication failedwith the variable unset, succeeds with it set,remote.origin.urlis credential-free from the start, no promisor config, and the three-dot diff that started this runs green over historical blobs with an unreachable remote.Cost is the historical blobs a filtered clone skipped — 30 MB for the repo that hit this, against a clone budget unchanged at the SDK's 600s. The substrate's own
depth: 1clone (apps/substrate/src/sandbox-do.ts) is untouched: it checks out one named ref for a shell, never diffs across history, and says so.Key actions
cloneCommand, replacingbox.gitCheckoutexec's environment;authenticateCloneUrlremoved--filter/--depth/--single-branch/--bare/--sparse; no token, nox-access-token:in the command; inherited credential helpers reset firstpnpm test2149 passed,pnpm typecheck+pnpm lintclean