fix(sandbox): scrub the command everywhere it is persisted or rendered - #137
fix(sandbox): scrub the command everywhere it is persisted or rendered#137nplusonedev wants to merge 2 commits into
Conversation
![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 — 💬 Comment
Risk tier: lite · 0 critical · 3 warnings · 0 suggestions
Reviewers: security 2 · code-quality 1 · performance 0 · documentation 0
1. ⚠️ Warning — Scrub command-bearing ExecFailed errors before persistence
📍 packages/runtime-cf/src/sandbox-cf.ts:648-656
Only the ExecTimeout branch redacts 'cmd'. Non-timeout failures still construct 'ExecFailed' from the raw underlying error, which can contain the command and is persisted via the workflow error record. Redact command-bearing error text (or explicitly ensure the resulting message cannot include 'cmd') before returning 'ExecFailed'.
2. ⚠️ Warning — Include dispatch environment values in log redaction
📍 runs/offload-test.ts:804-809
The executed environment is '{ ...secretEnv, ...input.env }', but 'redactValues' contains only 'Object.values(secretEnv)'. A credential supplied through 'input.env' can therefore be emitted by the command or logs without scrubbing. Pass the effective environment's values (and include them in 'renderable') or otherwise apply an explicit secret filter to all sensitive dispatch inputs.
3. ⚠️ Warning — Do not pass the redacted command to self-heal
'maybeDispatchSelfHeal' now receives 'renderable(outcome.stage.command)', replacing any inline secret with '***'. If the self-heal child uses this value as the command to rerun or repair, it will receive an invalid command and lose the original secret. Keep the executable command in the operational path and sanitize only the persisted/displayed payload, or explicitly reconstruct the command from a secure secret source in the child workflow. The same change also occurs at line 1115.
Four credential surfaces that `redactValues` was meant to cover and did not. The command is written into the R2 log's meta line verbatim. The streams beside it are scrubbed, so the gap only holds while no command inlines a credential — and `curl -H "Authorization: Bearer …"` is an ordinary thing for a consumer's own CI to run. `ExecTimeout.message` inlines the command too, and Workflows persists that as the attempt record. Both take the same scrub now, on the container Layer and on the facade. The command handed to `execUnderGrant` stays verbatim; that one has to run. Neither leak was reachable on `offload-test` in any case, because that run never passed `redactValues` at all — so `redact` was the identity function on the one run that executes the CONSUMER's own command, where `set -x` and a stray `env` are ordinary rather than exceptional. Both exec sites now pass `Object.values(secretEnv)`, as `check` and `worker-deploy` already did. And the loudest surface was none of those. `offload-test` embeds the raw stage command in `StepFailed.cause`, in two `summaryMd` bodies and in the self-heal incident — six sites. `stepFailedMd` renders `summaryMd` as UNFENCED markdown straight into the GitHub check-run summary, which on a public repo is public, and the self-heal path carries the command into a child execution's `input_json` in D1. A dying stage also never reaches `ExecTimeout`, since it is caught and replaced by `deadFailure`, so the layer-level scrub does not apply there at all. All six now render through a local scrub. Known and deliberately not in this change: - `captureDetachedLog` and `waitForExit` write `proc.command` plus full unredacted streams. `ExecOpts` DOES carry `redactValues` and `runDetached` drops it, so this is a real gap rather than a missing type — but it is a different capability with credential-bearing callers (`self-heal-pr`, `product-demo`, `cdp-acceptance`) and wants its own change. - `playwright-demo` and `demo-reel` inject secrets with no `redactValues` at all — the same shape as the `offload-test` bug fixed here. - `executions.input_json` stores the run input, which for some runs contains the command. That is the input; it is not fixable by scrubbing. adr/0006-credential-boundary, `## Consequences` — the never-store/never-log bullet.
f3f84e8 to
bac792a
Compare
|
Validated all three findings against the code; two led to the follow-up commit, one is refuted with evidence. Finding 1 — refuted. Finding 2 — accepted, fixed in the follow-up commit. Per-dispatch Finding 3 — the defect is real; the proposed remedy is inverted. Keeping the raw command on the operational path would smuggle a live inlined credential into the credential-free agent sandbox and into a pack an injection-steerable LLM reads — |
bac792a to
5903ab3
Compare
… a heal no honest repro can reach Two corrections from validating the PR-review bot's findings against the code. The scrub list held the wrong value on an env collision. Per-dispatch `env` wins over a same-named store secret, so `Object.values(secretEnv)` alone holds the SHADOWED value while the live one prints — the log keeps the real credential and redacts a string that was never emitted. The list is now the effective value for each secret-designated key. Non-secret `input.env` keys are deliberately excluded: dispatch inputs are documented non-sensitive (header note 3), and scrubbing a value like "production" from every log line trades a contract violation nobody has made for garbled logs everybody reads. The self-heal dispatch now refuses a command whose rendering changed. The bot flagged that passing `renderable(command)` hands the healer a `***` it cannot re-run — true, but the raw command is worse: the repro is re-executed in the credential-free agent sandbox (ci-incident.ts header) and rides a pack an injection-steerable LLM reads, so a raw inlined credential would be smuggled into exactly the environment designed to hold none. Neither version is honest, so no heal dispatches and a warn says why — instead of paying for agent spend that can only fail at verify. All three call sites (both staged drivers and the sole-exec path) now pass the raw command and the guard decides.
5903ab3 to
7ce90c7
Compare
1 participant
Problem & Insight
redactValuesexists so an injected credential a command echoes never reaches a durable surface. It was covering the streams and nothing else.The command itself is a persisted surface.
writeLogwrites{stream:"meta", command}into the R2 log, andExecTimeout.messageinlines the command into what Workflows keeps as the attempt record. Both sat in the clear beside streams that had just been scrubbed. That gap holds only while no command inlines a credential, andcurl -H "Authorization: Bearer …"is an ordinary thing for a consumer's own CI to run.On
offload-testneither was reachable anyway, because the scrub was never armed. Both exec sites inject secrets throughenvand passed noredactValuesat all, soredactwas the identity function — on the one run that executes the consumer's arbitrary command, whereset -xand a strayenvare ordinary rather than exceptional.checkandworker-deployalready passedObject.values(secretEnv).And the loudest surface was none of those.
offload-testembeds the raw stage command inStepFailed.cause, in twosummaryMdbodies, inAcceptanceFailed.summaryMd, and in the self-heal incident — six sites.stepFailedMdrenderssummaryMdas unfenced markdown straight into the GitHub check-run summary, which on a public repo is public, and the self-heal path carries the command into a child execution'sinput_jsonin D1.A dying stage never reaches
ExecTimeoutat all — it is caught and replaced bydeadFailure— so the layer-level scrub does not apply on that path even once armed.Take
Scrub at every point the command crosses into something durable or rendered.
sandbox-cf.tsandsandbox-facade.ts:redact(cmd, redactValues)into the meta line. The command handed toexecUnderGrantstays verbatim — that one has to run.sandbox-cf.ts:ExecTimeout.commandtoo.offload-test.ts:redactValueson both exec sites, and a localrenderable()through which all six embed sites pass.sandbox-fake.ts: the fake'sExecTimeoutscrubs to match, so a run-level test cannot pass on a property the live layer establishes and the fake contradicts.ExecOpts.redactValuesdocstring now names the command surfaces, so an implementation scrubbing only streams no longer reads as compliant.Not extracted into a shared
redact: three byte-identical copies already exist and unifying them is its own change with its own blast radius. The helper says so rather than pretending otherwise.Deliberately not in this change
captureDetachedLog/waitForExitwriteproc.commandplus full unredacted streams.ExecOptsdoes carryredactValuesandrunDetacheddrops it, so this is a real gap rather than a missing type — but it is a different capability with credential-bearing callers, and wants its own change.playwright-demoanddemo-reelinject secrets with noredactValuesat all — the same shape as theoffload-testbug fixed here.executions.input_jsonstores the run input, which for some runs contains the command. That is the input; scrubbing cannot fix it.One consequence worth knowing:
repro.commandin a self-heal incident pack can now contain***and not be runnable. That only happens when a secret was inlined, and in that case the healer could never have reproduced it anyway — it has no way to get the value.Key actions
ExecTimeoutand was the site with no coverageExecTimeout.commandpnpm typecheckclean,pnpm lintclean,pnpm test173 files passsandbox-facade.ts's meta-line scrub is covered by construction only; that suite usescommand: "wrangler deploy"throughout, so mutating the hunk kills no testadr/0006-credential-boundary,
## Consequences— the never-store/never-log bullet.