mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 13:11:31 -10:00
fix/opencode-session-header
79
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Vasanth T
|
3b43c43eca |
fix(sandbox): keep a derived ACL target inside the write root it came from (#1040)
* fix(sandbox): keep a derived ACL target inside the write root it came from FILE_FLAG_OPEN_REPARSE_POINT refuses a reparse point at the final path component and resolves every component above it, which is right for a path the operator named and not enough for one this package derived. The write-root carveouts are derived: <root>/.git/hooks and <root>/.git/config are constructed from a root, and <root>/.git is a name an unprivileged workspace writer can create before setup runs. mklink /J needs no privilege, so a junction there had the apply open <junction-target>/hooks, an ordinary directory with nothing wrong about its final component, and zero sandbox setup wrote a deny ACE on it as Administrator, outside the workspace. A derived entry now carries the root it came from, and the apply requires the object it finally holds to still live under it. The check is on the handle rather than the name: GetFinalPathNameByHandle answers where the open object actually is, and both sides go through it so the spelling normalizes the same way. Materialization is checked before it creates, since os.MkdirAll follows the same reparse points. Deliberately not a refusal of every reparse point on the path. Above the write root the path is the operator's, who may keep a workspace under a junction or a mapped directory; only the tail the sandbox appended is held strict. Closes #1024 * fix(sandbox): reject the reparse ancestor itself before materializing The pre-create check asked where the deepest existing ancestor lives, and a junction answers with its own path: the open does not follow a final-component reparse point, so <root>/.git came back as <root>/.git and matched. os.MkdirAll does follow it. What disqualifies that ancestor is that it IS a reparse point, not where it reports living. Found by reverting the check and watching the test still pass, which said the guard was doing nothing rather than that the test was weak. Pinned against the function now, because through the whole apply the create is made and then removed on the failure path, so the filesystem afterwards looks identical either way. The plan wiring gets its own test for the same reason: every apply-level case here hands the group an anchor directly, so none of them would notice the builder never setting one. * fix(sandbox): anchor only the carveouts that are under the root, and put the DACL back Two from review, both right. ReadOnlySubpaths is a profile field an operator can set to any path, and one placed outside the write root is a configuration that works today; anchoring it unconditionally turned that into a containment refusal. Only paths lexically under the root are anchored now, which is where the derived carveouts are anyway, and anything else keeps the final-component guard it always had. The successful-apply tests left their deny ACE in place, so t.TempDir could fail to remove the tree. Restoring the snapshot exposed the sharper half: the ACE denied the group the test runs as, which revoked its own WRITE_DAC and left the rollback unable to reopen the target. The tests deny a group this process is not a member of instead, which is what a capability SID is in production, and roll back afterwards. * test(sandbox): check the plan anchoring on every platform, out-of-root case included The plan builder is cross-platform and its anchor tests were in a Windows-only file, so nothing checked the wiring on Linux or macOS. Moved beside the other BuildWindowsACLPlan tests, with the case review raised: an out-of-root ReadOnlySubpath stays unanchored, and an in-root one is still held to its write root. * test(sandbox): give the cross-platform anchor tests native paths Moving these beside the other BuildWindowsACLPlan tests made them run on Linux and macOS, where their Windows path literals stop meaning what they say: pathWithinRoot is filepath.Rel underneath, a backslash is an ordinary character off Windows, and C:\workspace\.git\hooks is then one component that is not under C:\workspace, so every anchor came back empty. The neighbouring tests get away with such literals because they only compare strings they built the same way and never ask whether one contains another. Paths are built with filepath.Join from temp roots now, so the containment question is asked in the separator the running platform actually uses. |
||
|
|
0207f2b269 |
fix(sandbox): keep Windows restricted-token SIDs narrow and fail closed on DenyRead (#1006)
* fix(sandbox): keep Windows restricted-token SIDs narrow and fail closed on DenyRead * fix(sandbox): address CodeRabbit review feedback on PR 640 * refactor(sandbox): Address CodeRabbit nitpicks on PR 640. Co-Authored-By: cairn-code <[email protected]> * fix(sandbox): skip inherited ACEs and reject DenyRead upfront during setup Skip inherited ACEs when preserving read deny entries, correct syntax in the command runner error message, and evaluate unsupported DenyRead profiles before the elevation check in Windows sandbox setup. Refs #640 * Reject unsupported DenyRead profiles upfront during sandbox setup Refs #640 * Migrate legacy Windows DenyWrite ACEs, randomize smoke probes, and clean dead descendant machinery. Refs #640 * Use /d /c for denied-write probe command and verify ownership on cleanup. Refs #640 * Honor NoInherit in legacy ACL migration and verify probe ownership. Refs #639 * Preserve DenyRead bits when migrating combined legacy deny ACEs. Refs #639 --------- Co-authored-by: euxaristia <[email protected]> Co-authored-by: cairn-code <[email protected]> |
||
|
|
f30f550e60 |
fix(sandbox): protect worktree git pointers (#805)
* fix(sandbox): resolve git metadata carveouts for worktrees .git is a file (gitdir: ...) in worktrees/submodules, not a dir. The hooks/config write-deny carveout assumed a directory and pointed at <root>/.git/hooks, which has no mkdir-able parent under the .git file. bwrap then failed 'Can't mkdir parents ... Not a directory', blocking every sandboxed tool in worktree checkouts. Resolve the real (common) git dir via the gitdir pointer + commondir so worktrees deny the shared hooks/config like a plain clone. Plain checkouts (.git dir) and .git-absent roots keep the literal paths. * fix(sandbox): bound worktree git metadata carveouts to the write root Refuse gitdir/commondir pointers that escape the workspace instead of carving outside confinement. Invalid, stale, or non-directory pointers return no carveouts so bwrap does not mkdir under a .git file. * fix(sandbox): evaluate gitdir/commondir through symlinks Containment must use the real path so an in-root symlink cannot punch hooks/config carveouts outside the write root. * fix(sandbox): fail closed on gitdir eval and protect the pointer file Refuse carveouts when EvalSymlinks cannot produce a real in-root path, compare evaluated roots and targets, and write-deny the .git pointer so it cannot redirect hooks/config between commands. * fix(sandbox): deny worktree-local gitdir metadata and parse first gitdir line Keep hooks/config write-denied on both the common dir and the in-root worktree gitdir, emit those paths under the lexical write root, and accept a gitdir: pointer that has trailing comment lines. * fix(sandbox): skip special git metadata files and re-check carveouts at apply Read .git and commondir only when they are regular files so a FIFO cannot block profile construction. At sandbox apply, drop ReadOnlySubpaths that no longer sit inside the write root after a symlink swap. * fix(sandbox): follow directory .git symlinks and unix-tag FIFO tests A .git symlink to a directory is a normal checkout, so keep lexical hooks/config carveouts. FIFO coverage stays on unix so Windows still compiles. * fix(sandbox): protect worktree git pointer directly * test(sandbox): skip unavailable symlink fixture * fix(sandbox): preserve git symlink semantics * fix(sandbox): limit git pointer carveout to regular files --------- Co-authored-by: jatmn <[email protected]> Co-authored-by: jatmn <[email protected]> |
||
|
KRATOS
|
7acac7acee |
fix(sandbox): refcount temporary roots so one holder's cleanup cannot revoke another's (#911)
* fix(sandbox): a temporary root is refcounted, so one holder's cleanup cannot revoke another's Split out of #829 as an independent fix, per @Vasanthdev2004's review asking for the self-contained pieces to arrive separately. A temporary grant was add-then-remove with no notion of who still needed the root. The second caller to ask for a root already present got a NO-OP undo, and the first caller's cleanup then removed the root out from under it — so a live grant lost its access while its holder still believed it had it. Two read-only tools in the same parallel batch, both blocked on the same directory, is exactly that shape, and read-only tools are precisely the ones the batch runs concurrently. tempReads/tempWrites now count live holders, and the root is only stripped when the last one releases. The release also performs both mutations under ONE hold of the lock. Dropping it between them left a window where the root was still in readRoots but no longer in tempReads: a concurrent AddTemporaryRead landing there reads it as a PERMANENT root, hands its caller a no-op undo, and this call then strips the root — so that caller believes it holds access it has already silently lost. Independent of the remaining #829 work; builds and tests against current main on its own. Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * fix(sandbox): a read covered by a TEMPORARY write root takes a reference too Found by a self-review of this branch: the refcount closed the defect within reads and within writes, but not across them. AddTemporaryRead returned a no-op undo whenever writeRootCoversLocked said a write root already covered the path, and the comment there claimed that cover was permanent. It is not: writeRootCoversLocked scans extraRoots, and AddTemporaryWrite appends TEMPORARY write roots into that same slice. So a read covered by another holder's temporary write took no reference, and that holder's cleanup silently revoked the reader's access — the very defect the refcount exists to close, reached across the read/write boundary instead of within one side of it. Demonstrated before fixing, and the regression fails without it: ReadRoots before release = [ws, …/outer] ReadRoots after release = [ws] the write holder's cleanup revoked a live read grant on …/outer/inner The read now takes a reference on the covering temporary write root, and the genuinely permanent case — the workspace root, or a session-scoped grant — still returns a no-op undo. The regression also asserts the other direction: once the reader releases too, the root is gone, so the reference does not leak the grant past its last holder. Origin-Session: local-0484ba | Claude Code | 6 prompts Origin-Snapshot: e6d90ff3ce46 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * test(sandbox): make the concurrency test concurrent, and portable off macOS Both raised by CodeRabbit. The third finding on this PR — a read covered by a TEMPORARY write root taking no reference — was already fixed in 87a722cb with the regression it asked for; this covers the two that were still live, and both are cases of a test reporting more than it checked. THE CONCURRENCY TEST WAS NOT CONCURRENT. The gate spun until hasReadRoot saw somebody holding the root, which is satisfied by the FIRST holder, then opened the release. The spin wins that race against goroutines still being scheduled: instrumented over 200 runs, 194 peaked at a single simultaneous holder and not one ever reached eight. A test named for concurrent holders was exercising one holder at a time. Against a deliberately broken refcount, where the first release strips the root, it passed 33 times in 40 — it was a flaky test that mostly agreed with broken code. Counting all eight acquisitions makes the overlap real: 200 of 200 runs now peak at eight, and the same broken refcount is caught 40 times in 40. Holders signal the barrier on every path including failure, so a holder that returns early fails the test rather than hanging it. THE HELPER SKIPPED ON WINDOWS. scopeOutsideRoots reached for /Users/Shared and fell back to /var/empty, neither of which exists there, so every test built on it skipped and reported green having asserted nothing. It now uses the home directory, which no platform lists as a default write root, matching what scope_extra_read_test.go already does — and then PROVES the choice against defaultTempWriteRoots rather than assuming it, so a future default root turns these tests into an honest skip instead of a silent no-op. Origin-Session: local-abff1c | Claude Code | 2 prompts Origin-Snapshot: d2f269b81f33 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * fix(sandbox): a permanent grant outlives the temporary one, and an undo is per holder Both P1s from @anandh8x, both reproduced before changing anything, and both are the same mistake in different places: treating "something covers this path" as though it answered "and will it still be there afterwards". A PERMANENT GRANT WAS REVOKED BY A TEMPORARY HOLDER. extraRoots holds temporary write roots alongside permanent ones, so Add asked only whether the path was already covered. A session-scoped grant made while a temporary holder happened to cover it recorded nothing at all, and disappeared the moment that holder released. Outliving the request that prompted it is the entire difference between Add and AddTemporaryWrite, and it was not happening. AddRead had it too, including across the boundary: a permanent read covered only by a temporary WRITE root died with that root. Permanent coverage is now looked for first and across every root, so a temporary cover cannot decide the answer when a permanent one is also present. The same root held temporarily is promoted in place — its holders' releases become no-ops, which is what permanence means here. A BROADER temporary root over a narrower permanent grant cannot be fixed by promotion, so the narrower root is recorded in its own right; that is the case he specifically called out, and it is the one a promotion-only fix would have missed. ONE HOLDER'S UNDO IS ONE HOLDER'S REFERENCE. The comment on releaseTemporaryRead claimed each undo is called exactly once and that flooring the count at zero covered the rest. Flooring stops the count going negative; it does nothing about a holder's second call consuming a DIFFERENT holder's reference. With two readers, calling the first's undo twice took the count 2 -> 1 -> 0 and removed a root the second was still using — the exact revocation this refcount exists to prevent, reached through a duplicate call rather than a sibling's cleanup, and a deferred cleanup in a retry path is how a caller does that by accident. Every undo handed out is now wrapped so it drops its own reference at most once, and the comment says what is actually true. Both mutation-checked: unwrapping the undo, and ignoring permanence in Add, each fail the test that covers them. Race detector clean. Origin-Session: local-abff1c | Claude Code | 3 prompts Origin-Snapshot: 07900397c62e Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * chore(sandbox): re-request an automated review of this head No code change. CodeRabbit's verdict on this PR is pinned to ff85dbcc, a commit that no longer exists on the branch, and a stale changes-requested keeps the PR at mergeStateStatus=BLOCKED exactly as a human one would. It re-reviews on push rather than on request, and repeated @coderabbitai triggers over five days produced nothing, so this is the only lever available without maintainer rights. The cost is that this dismisses @Vasanthdev2004's and @anandh8x's approvals of 4a7063aa. The tree they approved is byte-identical to this one. Origin-Session: local-8cd239 | Claude Code | 6 prompts Origin-Snapshot: 149ab98453ad Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * test(sandbox): a fixture failure fails, a report cannot deadlock, and AddRead's own branches are covered All raised by CodeRabbit on the full review. Every one verified before changing anything. ## The concurrent test could hang instead of failing failures was buffered at one slot per holder, but each goroutine can report at BOTH hasReadRoot checks — its own grant going missing while held, and again after the release gate. Nothing drains the channel until after wg.Wait(), so the ninth report blocks forever, wg.Done() is never reached and the wait hangs. The test would deadlock in exactly the case it exists to catch: a refcount bug severe enough to fail more than eight of eight holders. Buffered at 2*holders. ## A broken fixture reported itself as a pass TestANarrowerRequestHoldsTheCoveringGrant called t.Skip when os.MkdirAll failed. The directory is the fixture, not a precondition the environment may reasonably withhold, and the sibling test above it already uses t.Fatal for the same call. ## Two tests assumed something they never checked Both tests in scope_extra_read_test.go place their grant under $HOME precisely because NewScope seeds /tmp and $TMPDIR as write roots — a grant that collapsed into one of those would be writable for a reason unrelated to the property under test. $HOME is normally clear of them, but a sandboxed or CI environment that points HOME at a temp directory turns both tests into something that proves nothing while still reporting PASS. homeGrantOutsideTempRoots now builds the fixture for both and skips with the reason when the assumption does not hold. Verified by running with HOME set to a temp directory: both tests SKIP naming the covering root, where before they passed. The path is resolved before comparing, because the temp roots are resolved and /var is a symlink to /private/var on macOS. ## AddRead's branches were only covered from the write side Two subtests added: a permanent read under a temporary WRITE root, and a narrow permanent read under a BROAD temporary read root. Both mutation-checked by removing the corresponding temporary-coverage guard in AddRead — each fails. The first mutation I ran did not fail its test, and the finding looked wrong: the pattern being edited occurs in Add, AddRead and AddTemporaryWrite alike, and a first-match edit had changed Add. Re-run against the line inside AddRead, it fails as claimed. Recorded because a mutation that misses its target is indistinguishable from a test that holds. ## Also asserted: the refcount table is empty, not merely invisible hasReadRoot answers what a caller can see. An entry left at zero in tempReads is not visible that way and would still leak one map entry per acquire/release cycle, so the contention test now checks the terminal state directly. go test -race ./internal/sandbox/ -count=5: clean. Unrelated and pre-existing here: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-8cd239 | Claude Code | 12 prompts Origin-Snapshot: dd397730a138 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * fix(sandbox): a reader outliving its writer keeps read authority, not write Reported by @jatmn, and it is a privilege escalation rather than a lifetime bug. AddTemporaryRead, given a path already covered by a temporary WRITE root, took a reference on that write root and returned an undo that released it. The lifetime was then correct — the reader no longer lost its access when the writer cleaned up — but the capability was wrong. extraRoots feeds validate(), which authorises WRITES, so once the write holder released first the path stayed in extraRoots for as long as the read holder lived. A caller that asked only to READ could write anywhere below the former write root after the write grant had ended. His diagnosis names the root cause exactly: one write-capability reference was doing two jobs, tracking a lifetime and standing in for a read-only dependent. The lifetime is now kept as what the caller actually asked for — a temporary READ root on its own path. AddTemporaryRead consults permanentWriteRootCoversLocked rather than writeRootCoversLocked, so a merely temporary write cover no longer short-circuits it into borrowing write authority. Mutation-checked: restoring writeRootCoversLocked in that branch compiles and fails the regression on three separate assertions — the reader keeps no read root of its own, ReadRoots() loses the path, and the writer's cleanup revokes a live read grant. Two things a verification pass turned up that are worth recording: The comment justifying this design said readRoots "feeds ReadRoots() and nothing else". That was false — it also feeds ExtraReadRoots(). The security argument survives, because neither confers write authority and validate() reads workspaceRoot plus extraRoots, which this path never enters. But that sentence IS the written justification for the whole choice, so it now lists both and says why neither grants write. The "the reader releases first" subtest passes on the unfixed tree: the escalation needs the WRITER to go first. It is a real no-regression control for the opposite ordering, not evidence the finding is closed, and it is renamed to say so rather than padding the count. Rebased onto |
||
|
|
e59bf78cab |
fix(sandbox): Make WSL backend tests deterministic. (#948)
Co-authored-by: euxaristia <[email protected]> Co-authored-by: cairn-code <[email protected]> |
||
|
Kevin CodexandClaude Fable 5
|
2749447cdc |
fix(tools): make apply_patch tolerant and re-expose edit_file (#956)
* fix(tools): make apply_patch tolerant and re-expose edit_file
Head-to-head benchmarking against Grok Build on grok-4.6 (3 tasks × 3 runs)
showed Zero solving every task but spending 2.7× the tokens and 2.3× the
time on the multi-file feature task. Every one of the 20 apply_patch calls
in those runs failed, after which the model degraded to whole-file
write_file rewrites and read→write verification loops.
Root causes and fixes:
- The sandbox and the tool both rejected an absolute path that lies inside
the workspace ("must stay inside the workspace"). Models routinely echo
the absolute path read_file showed them. Absolute paths now flow through
the normal workspace-scope validation (inside → allowed, outside → still
denied); unified-diff headers are rewritten to a/ b/ relative form before
git apply, and platform symlink prefixes (/var → /private/var) are
normalised the same way the other file tools do.
- The structured parser demanded a byte-exact "*** Begin Patch" /
"*** End Patch"; grok-4.6 writes "*** Begin Patch ***". Decorated marker
spellings are accepted, and a unified range after "@@" ("-12,4 +12,6",
optionally with a heading) no longer becomes a literal context search.
- Parse errors now carry a one-line format reminder so a failed call is
recoverable in one retry instead of a format-guessing loop.
- edit_file (exact search/replace with uniqueness guard, CRLF and fuzzy
fallbacks) is advertised to the model again alongside apply_patch, and
the apply_patch description shows the structured format.
- Writers (edit_file, write_file, apply_patch) recorded a file's line total
with lineCount, which is one higher than read_file's count for a file
ending in a newline. RecordSeenRange resets every observation when the
total changes, so a partial read_file after a successful edit silently
discarded whole-file knowledge and the next edit was refused with
"content that has not been read exactly in this session". All writers
now record the same total read_file reports (trackedLineTotal).
- read_file's padded " N | " line prefix was ~19% of every read (about
1.5k tokens per 800-line file) and was re-sent on every later call. It is
now the compact "N→" prefix models already know; nothing parsed the old
form.
- The editing-discipline prompt steers: edit_file for a targeted change,
apply_patch for multi-hunk/multi-file changes, write_file only for new
files or full rewrites; a successful edit result is confirmation, so the
file is not re-read to verify it.
Tests: regression tests for decorated markers, writer/reader line-total
agreement (edit → partial read → edit far away still succeeds), range hunk headers,
absolute in-workspace paths (structured + unified), outside-workspace
rejection, header-line-only relativisation, and tool visibility; the
prompt and tool-budget tests updated to the new contract.
Benchmark (grok-4.6, 3 tasks x 3 runs, harness in demo/bench-zero-vs-grok):
Zero 0.8.0 617 s / 1,527k tokens -> with this change 346 s / 493k tokens;
Grok Build 407 s / 713k. Every run of every configuration solved its task.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): keep CRLF line endings when relativizing patch headers
relativizeUnifiedPatchPaths split on a normalized "\n" and rejoined with
"\n", so any CRLF patch that needed a header rewrite lost its line endings
everywhere; git apply could then fail to match a CRLF file. Split on "\n"
only and carry each line's trailing "\r" through, including on rewritten
headers.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(sandbox): share the structured-patch classifier with apply_patch
Review follow-ups for #956:
- The tool accepted decorated structured markers ("***Begin Patch") that
the sandbox's applyPatchPaths did not recognise, so for that spelling the
boundary scanned the patch as a unified diff, extracted no targets and
validated nothing. The marker classifier now lives in the sandbox package
(StructuredPatchMarker / IsStructuredPatch) and the tool delegates to it,
so both sides accept exactly the same set; tests cover inside, traversal
and outside targets for the no-space spelling at the boundary.
- Document the containment model at the apply site: structured patches are
applied through an opened os.Root (descriptor-relative, no-follow);
unified diffs keep the pre-existing git-apply pathname flow, which the
absolute-path rewrite does not widen (they become the same root-relative
form relative paths always used).
- OpenAI addendum: the one-file-per-call rule names read_file, edit_file and
write_file explicitly and allows a single multi-file apply_patch.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(sandbox): compare slash-normalised patch paths so the check passes on Windows
structuredPatchHeaderPaths normalises separators to "/", so the absolute
outside-workspace path in the boundary test must be compared in its slash
form; on Windows the backslash original never matched and the Smoke job
failed.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(sandbox): validate the parsed patch path at the scope boundary
For each accepted marker spelling, pass the exact path applyPatchRequestPaths
produced to scope.validate: an outside path must be denied and an inside
path accepted, so a separator-handling regression between the parser and the
scope cannot slip past the test.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): apply unified diffs in-process through the workspace root
Review follow-up for #956. Unified diffs were validated by pathname and then
handed to git apply, which reopened the targets by name — a check-to-use
window a symlink swap could exploit — and the absolute-path rewrite added in
this PR normalised `diff --git` headers with strings.Fields, which splits a
git C-quoted path containing whitespace.
Both go away by not shelling out at all: parseUnifiedPatch translates a
unified diff into the same operations a structured patch produces, and both
formats are applied by applyPatchOperations through os.OpenRoot — every
stat, read, create and write is descriptor-relative and refuses to follow a
link out of the root. git is no longer required by apply_patch.
Unified-diff support: modifications, creations (--- /dev/null), deletions
(+++ /dev/null), multiple hunks per file, git "rename from/to" (a move,
hunks optional) and "copy from/to" (a new copy operation whose destination
inherits only the tracker credit the source had), "\ No newline at end of
file" on either side, a/ b/ prefixes, C-quoted paths, CRLF patches, and
git's diff/index/mode lines. Hunks are placed at their "@@ -a,b" range when
the expected lines match there and located by context otherwise (so a
stale range still applies and duplicate blocks are disambiguated by the
range). Binary patches are rejected.
Removed: the git apply flow, relativizeUnifiedPatchPaths,
recheckPatchWriteTargets, missingPatchTargets, completeCreatedPatchTargets
and recordCreatedPatchTargets. validatePatchPaths/changedFilesFromPatch stay
for mutation-target listing.
Tests: the existing unified-diff suite now runs through the in-process
applier unchanged; new coverage for quoted whitespace paths (inside applies,
outside is refused untouched), a workspace path swapped for a symlink that
escapes the root (both formats refused, outside file unmodified), create +
delete + no-newline markers, range hint vs context fallback, rename with a
hunk, and binary/empty rejection.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): harden unified-diff parsing and cover the copy operation
Review follow-ups for #956 (in-process unified applier):
- A hunk that ends before its declared line counts is reported as
malformed instead of silently absorbing the next file's ---/+++ header
pair (which then failed later against the wrong file). Both shapes are
caught: leftover counts at a header or at end of input, and a --- line
directly followed by +++ inside a hunk. The element after a patch's final
newline is no longer counted as an empty context line, so a truncated
hunk cannot look complete.
- "\ No newline at end of file" may only follow a file's final hunk; a
marker before another hunk is rejected rather than leaking into that
hunk's result.
- Zero-context insertions (git diff -U0) are documented as
position-trusting by design, matching git apply without fuzz; hunks with
context are verified at the range before it is used.
Tests: copy from/to (source kept, destination created with the hunk
applied and tracked with the source's whole-file state; existing
destination refused untouched; copy onto itself refused), over-declared and
truncated hunk counts, and the no-newline marker before vs after the final
hunk.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): require an @@ line before treating ---/+++ inside a hunk as a file boundary
A removed "-- x" line directly followed by an added "++ y" line is valid
hunk content that also matches the ---/+++ header shape; the boundary check
now additionally requires the following line to be a hunk header, and a
regression test keeps such a pair intact. End-of-input strictness for a hunk
cut short is documented as deliberate: applying a truncated hunk would make
a partial change (removals without their replacements).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* refactor(tools): share one line-count implementation between reader and writer
sourceLineCount and trackedLineTotal computed the same count; keeping two
copies invites exactly the reader/writer drift this PR fixed. The reader now
delegates to trackedLineTotal, which carries the tracker contract.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): verify a unified deletion's expected content before removing the file
A unified diff's "+++ /dev/null" hunks state the content being removed, but
the in-process applier dropped them and deleted unconditionally, so a stale
deletion removed a file whose current content the patch never described.
Unified deletions now keep their hunks, require at least one, and run them
through the same verifier as updates: the hunks must match the current file
and leave nothing behind, otherwise the deletion is refused and the file is
untouched. A structured "*** Delete File" keeps its unconditional meaning.
Hardening from the same review: every non-create change re-reads its source
through the opened root immediately before commit and refuses to proceed if
the bytes differ from what was planned.
Tests: matching deletion, stale deletion (file byte-for-byte unchanged),
partial deletion, header-only deletion, multi-hunk deletion with one stale
hunk (nothing removed) and all-good hunks, structured delete unchanged, and
a source changed between planning and commit for update and delete.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tools): a move with a missing source is refused before publishing
The pre-commit recheck now refuses a move whose source vanished after
planning instead of publishing the destination and then failing; update the
test to the safer contract (error names the recheck, nothing is created).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): byte-exact unified deletions and git's header-only empty-file forms
Review follow-ups for #956:
- A unified deletion is verified byte for byte: the removed lines (with the
removed side's "\ No newline at end of file" honoured) must equal the
current file exactly, with only the file's own line-ending style
normalised. None of the whitespace tolerance used to locate update hunks
applies, and a whitespace-only remainder no longer passes; a deletion is
not recoverable, so it fails closed.
- git describes an empty file's deletion or creation with header lines only
("diff --git", "deleted file mode"/"new file mode", "index") and no
hunks. Both forms are recognised: the deletion is applied only when the
current file is empty, the creation writes an empty file, and they compose
with hunk-carrying files in the same patch.
- The pre-commit recheck test also covers the copy operation and asserts no
destination is created on refusal.
Tests: whitespace/indentation/blank-line/space-only/missing-newline
mismatches all refused with the file unchanged, matching no-newline marker
and CRLF file accepted, header-only deletion of a non-empty file refused,
multi-file patch with header-only delete + create + a normal hunk.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tools): remove the move source after planning via a deterministic hook
The refused-move test now creates source.txt, plans the move, and removes
the file through a test-only pre-commit hook (structuredPatchBeforeCommit)
so it exercises the post-planning disappearance path rather than a source
that was never there. Assertions unchanged: refused before publishing, no
destination created.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tui): read_file fixtures use the compact N→ prefix
Three TUI fixtures still built fake read_file output in the legacy
" N | text" shape; they pass either way but pinned an outdated contract.
They now mirror what appendReadFileRange emits, and the shape comment and
the expanded-card assertion are updated.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): name the committed files when a patch is partially applied
Review follow-up for #956. When a later change in a multi-file patch fails
(for example the pre-commit recheck refuses a file that changed after
planning), the error now lists exactly which files were already committed
and states that the rest are unchanged, instead of a generic "partially
applied" warning. The model can then re-read only what actually changed.
Test: a three-file structured patch whose second file is altered after
planning fails with "already committed: first.txt", first.txt holds the
patched content, and the other two files are untouched.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
---------
Co-authored-by: Claude Fable 5 <[email protected]>
|
||
|
beardthelion
|
6edf9a8b78 |
fix(sandbox): normalize launcher names before the command-prefix denylist (#934)
* fix(sandbox): retire recorded approvals through a policy bump Ahead of tightening which command prefixes may be approved, make the upgrade path safe. A grant already on disk that the narrower rules reject fails validation on read, and that path rejects the entire grants file rather than the one entry. Every caller treats a read error as "no grant" (engine.go LookupCommandPrefix, ApprovedCommandPrefixes, and the Lookup in Evaluate), so one stale prefix would silently disable the user's persisted deny grants too, with the error text never reaching them. Bump the approval policy version instead and let the store's existing migration handle it: it backs the file up, keeps deny grants, invalidates prior approvals, and reports the counts once through the startup notice that both frontends already print. That is what the mechanism was built for. The v1-migration test pinned the policy version as a literal, so it now derives the expected value from the constant and will not need editing on the next bump. * fix(sandbox): normalize launcher names before the command-prefix denylist unsafeCommandPrefixLauncher matched the program name as written, so python3.11, python2.7, python.exe, node.exe and git.exe all validated as ordinary commands and could be persisted as command-prefix grants. Lookup is a string-prefix match, so a single approval then auto-allowed every later invocation: granting [python3.11 -c] covers `python3.11 -c "import os; os.system(...)"` with no further prompt. internal/agent's commandName already normalizes case and Windows executable extensions for the allow side of the same decision; the deny side did not, and the two disagreeing is the bug. Reduce the program to the launcher it actually runs and match on that: executable extension, Windows' trailing dots and spaces (python. starts python.exe), CPython ABI flags, then a trailing version, testing the digit-stripped form as well so python3.11 and python3 both arrive as python. A name is only ever narrowed toward an existing entry, so the failure direction is an extra permission prompt, never a silent grant. Digits without a separator stay part of the name, which keeps base64, 7z and sha256sum grantable. Two Windows spellings cannot be resolved by name at all, so the shape is refused alongside a path: 8.3 short names truncate the stem, which makes POWERS~1.EXE unrecognizable, and python.exe::$DATA reaches the same executable through its default stream. Normalizing the first token before the banned-suggestion comparison keeps git.exe at parity with git. The list also gains launchers of the same class it already covers and that normalization cannot reach: cmd, wsl, the remaining POSIX shells, busybox, uv and uvx. Entries normalization now covers (/bin/bash, /bin/zsh, powershell.exe, python3, pypy3) are gone; the first two were already unreachable behind the path check above them. Covers the persisted store, the session-scoped grants the agent takes during a turn, the legacy-schema migration that prunes such a grant per entry, and the Windows spellings hermetically. * fix(sandbox): treat free-threaded CPython as a launcher too The ABI flags came from the pre-3.13 set, so "python3.13t" and "python3.13t.exe" normalized to themselves and stayed grantable as command prefixes while the GIL build "python3.13" was refused. A free-threaded interpreter runs arbitrary code just the same. Add "t" to the flags trimmed under the existing digit guard, which is what keeps ordinary names intact: "zstd" and "cat" have no digit beneath the trimmed letters, so they keep their own names and stay grantable. Reported by CodeRabbit on #934. * fix(sandbox): refuse the remaining spellings of listed launchers Same defect as the free-threaded build, found by sweeping the families already on the list for names it cannot reach: - nodejs is node on Debian and Ubuntu, and was grantable. - python3-dbg, python3.11-dbg and python3.13t-dbg are interpreters that run arbitrary code; so are bash-static, pwsh-preview and node-nightly. A closed set of build-channel suffixes now normalizes them. The set stays closed deliberately: a general "-word" strip would also swallow node-gyp, python3-config and ruby-lsp, which are ordinary tools a user should still be able to approve, and those are pinned as must-stay-grantable. - pnpm, yarn and bunx run package scripts exactly as npm and npx do, and npm and npx are already refused. - sudoedit escalates like sudo. The launcher-wrapper family (winpty, chroot, unshare, nsenter, parallel, script, unbuffer, taskset, flock) has the same inconsistency against the env/nohup/timeout/setsid entries already on the list, and is left for a separate decision rather than widened here. * test(sandbox): cover the -debug and -beta launcher suffixes Four of the six build-channel suffixes had regression cases; -debug and -beta did not, so removing either from the set left every test green. Both now fail RED when their entry is dropped. Reported by CodeRabbit on #934. |
||
|
Anandan
|
d7ac85cbf9 |
Polish Zero's TUI and harden patch, model, and background workflows (#916)
* Checkpoint UI polish * Fix structured apply patch handling * Restore tool previews when resuming sessions * Improve theme-aware code rendering * Replace persistent sidebar with run details * Remove inert transcript interactions * Cover resumed edit previews * Smooth active run status feedback * Preserve semantic colors behind overlays * Normalize ChatGPT model aliases * Render staged image attachments in terminal * Polish transcript and tool feedback * Resolve rebased diff renderer conflict * Address UI polish review findings * Fix smoke test regressions * Harden patch and transcript edge cases * Bind structured patches to workspace root * Use unique structured patch temp files * Retry structured patch temp allocation * Reuse rooted patch temp allocator * Fix Windows CI process checks * Fix structured patch safety boundaries Preserve read-before-write credit only when the model already knew the full source, reject ambiguous hunk matches, and retain CRLF line endings. Add regression coverage for unsafe overwrites, positive tracking cases, duplicate matches, and Windows-style files. * Resolve plan test seam after rebase Drop the obsolete sticky-plan height helper while retaining the PR status test seams brought forward by the UI changes. * Address final review correctness gaps Bound narrow waiting labels, reject malformed plan records, preserve missing final newlines, reject out-of-order EOF hunks, clear stale tracking after failed rollback, and keep deferred tool loading aligned with model visibility. * Harden patch transactions and review edges Publish new patch targets without overwriting raced destinations, journal only completed mutations, and create replacement temps beside their targets. Reject pre-plan diagnostics, cover tool-search transcript suppression, and leave sufficient cleanup time in the Windows process fixture. * Bind patch rollback to committed versions Capture file identity and content before publication, quarantine rollback targets atomically, and refuse destructive undo when a path was replaced or modified in place. Add deterministic coverage for publication and rollback races. * Align structured patch failure semantics Use prevalidated sequential application with explicit partial-success errors instead of unsafe multi-file rollback. Retain atomic per-file publication, no-overwrite creates, unsupported-hardlink fallback, and conservative tracker invalidation. * Preserve patch context bytes Track source offsets for structured-patch context lines and reuse the exact matched source text during replacement. This keeps tolerant matching from changing indentation or mixed line endings outside the requested edit. |
||
|
|
55fa05f0ab |
fix(sandbox): deny reads of Zero credential stores (#681)
* fix(sandbox): deny reads of zero credential stores * fix(sandbox): address PR review comments - Deny reads of mcp-oauth-tokens.json.secret in the default Zero config candidates: file-backed oauth stores keep their encryption secret in a sibling .secret file, so the default MCP token store needs the same protection as the override paths (CodeRabbit). - Clarify that explicit credential-file overrides are still filtered by on-disk existence like every other candidate (Copilot). - Align the zeroUserConfigDir doc comment with config.UserConfigDir: macOS honors XDG_CONFIG_HOME before falling back to ~/.config (Copilot). Co-Authored-By: Claude Fable 5 <[email protected]> * fix(sandbox): deny the whole Zero config dir, not itemized filenames Address code review on PR #681: - Deny credentialDenyReadPaths' Zero-config candidate as the containing directory instead of an itemized filename list. The token/credential/ config stores each publish through a randomly-named .tmp-<pid>-<nanos> sibling before their atomic rename, and the legacy MCP token store leaves a mcp-oauth-tokens.json.migrated backup after importing it — none of those names were covered by the old itemized list, so a sandboxed command could read them under the read-all posture. - Stop dropping default candidates that don't exist yet at profile-build time. A store created later in a long-lived sandboxed session (e.g. a concurrent ) previously got no deny rule at all; every backend already treats a deny rule over a not-yet-existing path as a harmless no-op that still takes effect once the path appears. - Resolve ZERO_OAUTH_TOKENS_PATH / ZERO_MCP_OAUTH_TOKENS_PATH overrides the same way the token stores resolve them (relative-to-cwd, no ~ expansion) instead of through normalizeProfilePath, which tilde-expands and so could derive a deny path different from where the store actually writes. Adds regression coverage for the directory-wide deny (including the migrated backup and synthetic temp-file siblings), for building the profile before the store directory exists, and for the tilde-override resolution mismatch. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(sandbox): address macOS deny-path test and config parity Recompute the expected Zero config deny path after MkdirAll and resolve the temp base with EvalSymlinks so macOS /var -> /private/var does not flake TestPermissionProfileDeniesZeroCredentialFiles. Add a parity test that sandbox.zeroUserConfigDir stays aligned with config.UserConfigDir. Co-authored-by: PierrunoYT <[email protected]> * fix(sandbox): harden credential deny paths * test(cli): normalize credential deny baseline * fix(sandbox): close credential path gaps * test(sandbox): normalize command credential path * fix(sandbox): handle absent credential paths safely * fix(sandbox): protect MCP OAuth siblings * fix(sandbox): protect OAuth lock siblings * docs(sandbox): clarify Linux future-path limits * test(sandbox): probe credential store reads in the Linux smoke test The smoke test wired HOME/XDG_CONFIG_HOME at credential directories but never attempted a read through the sandbox, so it could not catch a real-bwrap regression for the paths this change protects. Write a secret into ~/.aws and $XDG_CONFIG_HOME/zero and add both to the deny-read table. * fix(sandbox): keep the credential deny usable, current, and unguessable Four problems with the credential deny baseline, all in the same mechanism. The whole-directory deny over $XDG_CONFIG_HOME/zero also hid the supported user plugin, specialist, and command roots, whose commands and scripts are executed through this sandbox, so an installed user plugin could no longer start. Narrowing the deny to itemized files was not an option: the stores publish through temporary names and a concurrent login can add a store that did not exist when the profile was built, and only a directory rule covers both. The directory deny therefore stays, and the non-secret subtrees are carved back out through a new DenyReadCarveouts field: seatbelt re-allows them after the deny (SBPL is last-match-wins) plus ancestor metadata so path resolution can reach them, and bubblewrap masks the directory with --perms 111 instead of 000 -- unlistable but traversable -- and re-binds the carveout read-only before the tmpfs is remounted read-only. A relative ZERO_OAUTH_TOKENS_PATH, ZERO_MCP_OAUTH_TOKENS_PATH, or XDG_CONFIG_HOME was resolved against the command directory only, while oauth.ResolveStorePath and mcp.ResolveTokenStorePath call filepath.Abs, i.e. resolve against the Zero process working directory. A command run from a subdirectory therefore had the wrong path denied and the real store stayed readable under the read-all posture. Relative candidates now resolve against both directories, since the process that resolves the value is not necessarily the one that writes the store. The regression test runs the command from a different directory than the process, which the previous tests did not. bubblewrap cannot mount over a path that is absent when the namespace is assembled, so a store created mid-session stayed readable through the live read-only host-root bind for the rest of a long-lived sandbox session. The directories Zero owns -- its config directory and the per-store publication directories -- are now reported as EnsureDenyReadDirs and created (0700) before the namespace is assembled, so their mask always exists. Third-party stores such as ~/.aws are still skipped when absent: Zero must not create them, and that limitation stays documented on the field. The fixed <store>.tmp publication sibling made the plaintext token, and via the same path its encryption key, observable to a same-user sandboxed process with write access to an override parent: it could wait for the known name and rename the opened inode into an allowed path. Both stores now publish through a per-store <store>.publish directory with a randomly-named file inside. The directory name is derived from the store path, so the profile can deny it up front, while the random name inside is what cannot be waited for or renamed away. A test keeps the duplicated suffix in step with oauth.PublicationDirSuffix. Also adds the seatbelt render coverage that was missing: dropping DenyReadIfExists from denyReadRules made every credential store readable again on macOS without failing a single test. * fix(sandbox): close credential deny review gaps Preserve process credential roots alongside command overrides while limiting host precreation and read carveouts to trusted process-derived paths. Canonicalize future credential roots through existing path aliases, reject symlink carveouts, and collapse automatic masks already covered by user or automatic directory denies. Normalize the policy golden and add regression coverage across profile, Seatbelt, and bubblewrap rendering. Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): normalize credential carveout paths Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): canonicalize backend credential masks Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * test(sandbox): normalize backend mask expectations Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): fail closed for replaceable token stores Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): allow keyring token path overrides Amp-Thread-ID: https://ampcode.com/threads/T-019fbdfa-8469-731e-87c0-b26b7641cf59 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): harden command credential roots Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-c723-74d9-a323-026040b94436 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): close credential deny review gaps Reapply credential denies after Seatbelt extension carveouts and model the legacy MCP token path as a migration input rather than an atomic writer. Retain Bubblewrap fail-closed markers for OAuth outputs exposed through readable carveouts, and add regressions for both platform policies. Amp-Thread-ID: https://ampcode.com/threads/T-019fbf55-f6b0-71fc-b384-652ba32b9645 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Claude Fable 5 <[email protected]> Co-authored-by: Cursor Agent <[email protected]> Co-authored-by: PierrunoYT <[email protected]> Co-authored-by: Amp <[email protected]> |
||
|
Vasanth T
|
91b413c514 |
fix(sandbox): stop the Windows write jail honouring Everyone-granted paths (#865)
* fix(sandbox): stop the Windows write jail honouring Everyone-granted paths The restricted token put the World SID (S-1-1-0, Everyone) in its restricted-SID list unconditionally, from the original sandbox baseline and with no comment saying why. A WRITE_RESTRICTED token runs two checks for a write and needs both: the normal one against its enabled SIDs, and a second against the restricted list. The write jail rests entirely on that second check only succeeding where Zero has explicitly ACL'd a capability SID. Everyone is a SID every principal carries, so on any path whose DACL grants Everyone write, the restricted half passed for free and confinement fell back to the ordinary user's own permissions -- the exact boundary this token exists to be stricter than. Reachable with no privilege, no symlink and no race: an Everyone-writable directory is enough, and share roots opened with Everyone:F and loose third-party installer ACLs supply them. Ruled out as the common case: C:\Users\Public\Documents grants BATCH, not Everyone. The runner already stated the rule this broke -- it declines to add the user SID, Administrators or SYSTEM because "each has write access nearly everywhere" -- while listing Everyone among the restricted SIDs. Removed only from the WRITE_RESTRICTED token, because without that flag it is load-bearing rather than gratuitous. The restricted-SID check then applies to reads too, and default Windows DACLs grant BUILTIN\Users rather than anything in this list, so a token with no Everyone cannot open cmd.exe and dies at launch with STATUS_ACCESS_DENIED (0xC0000022). Dropping it there was tried first and takes TestWindowsUnelevatedRealSandboxSmoke with it. That path is taken only when a profile configures DenyRead, which is already the posture trading capability for read-deny enforcement (#612), so the bypass survives there deliberately and narrowly; closing it needs a read-side grant that is not a universal group. Verified against real tokens rather than reasoned about. The new smoke test drives the real runner and asserts three outcomes that only agree when the jail is intact: a granted root is writable, an ordinary outside path is denied, and an Everyone-writable outside path is denied too. Before the change its third case wrote the file with exit 0; the first two were already correct, which is what made the gap invisible. Restoring the SID unconditionally turns it red again. * test(sandbox): make a denied write distinguishable from a runner failure The denial assertions expected exit 1, which the runner also returns for its own errors -- a failed marker validation, a bad argument, a token it could not build. Written that way the test passes both when the sandbox denies the write and when nothing ever ran, and the second case proves nothing while being indistinguishable from success. That is a poor property for any test and a bad one for this test, whose entire job is to notice a confinement regression. The leading "granted root is writable" assertion covered part of it, but said nothing about whether the two later commands launched. The denial commands now report a distinctive exit code the runner cannot produce for itself, so the code proves cmd.exe actually ran and its redirect was refused. Re-checked in both directions rather than assumed: the test passes against the fix, and restoring the World SID unconditionally fails it with "exit code = 0, want 77" -- still catching the bug it was written for. Raised by CodeRabbit on #865. |
||
|
Vasanth T
|
3e7692d8f0 |
fix(sandbox): scrub the daemon token file pointer (#677) (#818)
scrubSensitiveEnv removed ZERO_DAEMON_REMOTE_TOKEN but not its file counterpart. TokenFromEnv accepts either form, so a sandboxed command still inherited the path to the bridge token, and the default posture is read-all, so it could then read the token itself. Scrubbing the pointer is the whole fix rather than half of one: the path comes from this variable alone and TokenFromEnv has no default location to fall back on, so a command that never sees the variable has nothing to guess at. This mirrors GOOGLE_APPLICATION_CREDENTIALS, which is already scrubbed for the same reason. Extends the existing scrub table, which compares the surviving environment exactly, so the case fails if the entry is dropped. Verified: without the fix the assertion reports ZERO_DAEMON_REMOTE_TOKEN_FILE surviving into the scrubbed environment. |
||
|
Vasanth T
|
33c94ec7a3 |
fix(sandbox): deny reads of git's credential stores (#815) (#816)
* fix(sandbox): deny reads of git's credential stores (#815) git's credential-store backend keeps host passwords and personal access tokens in cleartext, at ~/.git-credentials or ~/.config/git/credentials depending on whether the user is on the XDG layout. Neither was on the deny list, so any command the agent runs could read them. Scoped to those two files deliberately. #815 also covers SSH private keys and the GPG keyring, and those are a harder trade: denying ~/.ssh stops a sandboxed git push over SSH from working, and excluding that directory is not even sufficient, since an IdentityFile directive can point key material anywhere. These two cost nothing by comparison, because git reads them to authenticate rather than to know who the user is, so a sandboxed git keeps working and simply cannot act as the user. The XDG one denies the file rather than the directory around it, which is the opposite of the choice made for ~/.config/zero and is deliberate: ~/.config/git also holds the global git config, which userGitConfigReadPaths grants on purpose so a sandboxed git can read user.name and aliases instead of failing outright. Denying the directory would take that away. Tests assert both stores are denied, that the git config beside the XDG one stays readable, and that an explicit read grant still re-includes them like every other entry. Dropping either entry fails the first; widening the XDG entry to the directory fails the config assertion. * test(sandbox): assert the credential deny exists before the grant lifts it The allow-read case only checked that the credential store was absent from the deny list once an explicit grant covered it. A deny rule that was never added satisfies that just as well, so the test passed with the production change reverted and proved nothing about the interaction it was named for. Check both runs: denied with no grant, absent with one. The first leg is what establishes there is something for the grant to override. |
||
|
Anandan
|
81a5e6cb71 |
fix(sandbox): preserve user config and retry denied writes (#801)
* fix(sandbox): preserve user config and retry denied writes Keep user identity and configuration paths discoverable while retaining managed cache and temporary storage. Classify native sandbox failures as typed recoverable denials so approved commands can retry outside the sandbox. Add regression coverage for global Git ignores, typed denial handling, and permission retry behavior. * fix(sandbox): tighten denial retry safeguards * test(sandbox): assert denial contract * fix(sandbox): trust typed denials and restore Cargo state Stop promoting command output keywords into recoverable unrestricted denials, so only structured adapter facts can enter the approved unsandboxed retry path. Restore the managed CARGO_HOME directory and environment override so Cargo remains writable while the caller's real home stays read-only. * fix(sandbox): mask preserved credential configs Preserving caller configuration made credential-bearing tool files discoverable under Linux's read-all sandbox profile. Add default and environment-overridden npm, GitHub CLI, netrc, Docker, Kubernetes, and Zero config paths to the credential deny set, with regression coverage. Clarify the typed-denial retry and platform read-model comments without changing the approval behavior. * fix(sandbox): protect custom gcloud config CLOUDSDK_CONFIG can relocate gcloud credentials outside the XDG-derived default. Include the configured directory in the credential deny profile and cover the override with a regression test. * fix(sandbox): deny zero credential directory * test(sandbox): keep zero credential directory deny path expectations |
||
|
Anandan
|
518d2e53ba |
fix(windows): use PowerShell for command execution (#804)
* fix(windows): prefer PowerShell for command execution Detect PowerShell 7 or Windows PowerShell before falling back to cmd.exe, and share the selected host shell across agent tools and the interactive shell escape. Configure UTF-8 output, preserve restricted-sandbox MSYS safeguards, and add Windows coverage for pipelines, workspace writes, and interactive stdin sessions. * test(windows): align shell coverage with PowerShell Make Windows command fixtures and guidance assertions follow the selected host shell, preserve cmd fallback coverage, and use a genuine MSYS executable when PowerShell aliases are available. Remove the unsupported non-TTY PowerShell stdin assertion; Windows interactive stdin requires a separate PTY transport. * fix(sandbox): classify Windows executable suffixes Normalize .exe, .cmd, .bat, and .com command names before safety and network lookup so PowerShell invocations such as curl.cmd receive the same network permission handling as curl. Add analyzer and engine-level regressions for the Windows PowerShell command shape observed in CI. * fix(sandbox): normalize absolute Windows command paths Extract drive-letter and UNC basenames before removing shell escape characters so fully qualified Windows executables continue matching canonical safety and network classifications. Cover both drive and UNC curl paths with analyzer regressions. * fix(sandbox): normalize relative Windows paths Recognize drive-relative, explicit relative, and suffixed backslash executable paths before generic shell escape removal so Windows commands continue to match canonical sandbox risk entries. * fix(windows): harden PowerShell execution * fix(sandbox): classify PowerShell removal aliases |
||
|
|
88fb7b6192 |
fix(sandbox): add platform-specific write lock around grant state file (#752) (#755)
* fix(sandbox): add platform-specific write lock around grant state file (#752) Add grants_lock_unix.go and grants_lock_windows.go with a lockStateFile method that serializes access to the persisted grant state across processes. Wire the lock into Grant, GrantCommandPrefix, Revoke, RevokePath, and Clear. Also add .gitattributes to force LF line endings for Go files so gofmt checks pass on Windows. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(sandbox): do not lock the grants file when there is no notice to consume Addresses jatmn's #755 finding. ConsumeMigrationNotice took the interprocess lock unconditionally, and lockStateFile MkdirAlls the grants directory and creates <grants>.lockfile. Every startup and every 'zero exec' calls this, so it turned reading the grant state into requiring a WRITABLE grants directory: a user with no grants file whose ZERO_SANDBOX_GRANTS_PATH (or its parent config directory) sits on a read-only mount failed outright with 'failed to migrate sandbox grants', where the pre-lock code read an empty state and wrote nothing. Peek at the state without the lock first and return early when no migration write is required. Only when one is — a schema/policy migration, or clearing NoticePending — take the lock and re-read under it, since the unlocked peek is a snapshot another process may have already consumed or migrated, and is meaningless by contract when needsMigration is set. Both regression tests are verified to fail against the pre-fix code. The read-only-directory test skips on Windows (mode bits do not deny writes) and as root (permissions are bypassed), so a portable companion asserts the same requirement by requiring that nothing is created at all. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <[email protected]> |
||
|
|
583e6530bd |
fix(lint): resolve static analysis baseline (#769)
* fix(lint): resolve static analysis baseline * fix(lint): clear linux staticcheck findings Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-b2ee-7442-8a31-80b2cf9dd270 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Amp <[email protected]> |
||
|
Anandan
|
9fe02ae52e | fix(sandbox): clarify scoped permission prompts (#798) | ||
|
euxaristiaandClaude Sonnet 5
|
97ef90d27a |
chore: remove code unreachable on every supported platform (#706)
* chore: remove code unreachable on every supported platform The Security and code health job's advisory deadcode step reports 152 unreachable functions on main. Running the same check under GOOS=linux, windows, and darwin shows 13 of those are false positives whose callers live in windows build-tagged files, so only functions unreachable on all three platforms are touched. Functions with no references anywhere are deleted, including the internal/reasoning package, which nothing imports. Functions that only tests reference move into per-package export_test.go files so tests keep their seams while the production binary sheds the code. Test call sites of deleted convenience wrappers are ported to the live APIs (scoped tool constructors, tools.DeferredSource, FinishReason checks). Kept despite the report: cron.Store.Runs (only reader of runs.jsonl, which production writes), and agenteval.TraceEventKeys and MissingTraceEvents (used across packages by the trace contract test). * docs(config): correct ValidateFile's errors.As claim Message is a flattened string (.Error() already called on the parse error), not a wrapped error, so callers can't recover the concrete json error type via errors.As. Fix the comment to match. Co-Authored-By: Claude Sonnet 5 <[email protected]> * chore(trace,skills): drop obsolete Sink adapters and fix stale doc references The dead-code pass removed the Emit methods but left Sink, NDJSONSink, and TextSink exported with a contract nothing implements; remove the types with their contract. Also remove the stray blank line at EOF of trace.go flagged by gofmt, and update skills package docs and HOW_ZERO_WORKS.md to stop directing callers to DiscoveryRoots/Duplicates, which are now test seams. Co-Authored-By: Claude Fable 5 <[email protected]> * chore: clean up remaining review nits on dead code removal Replace a pointless fmt.Errorf(...%w...).Error() with fmt.Sprintf using %v, since the wrapped error was discarded immediately anyway. Remove a comment block that was duplicated verbatim. * docs(skills): drop remaining references to test-only List/Get Load and MergedSkills comments still named List/Get as production entry points after they moved to export_test.go. Point callers at Load and ListFromRoots instead. * chore: remove now-unreferenced export_test.go seams execSessionManager.len() and likelySandboxDenied/sandboxDenialKind had no remaining callers after the sandbox unify-command-execution rebase; the production code they wrapped for tests no longer exists. --------- Co-authored-by: Claude Sonnet 5 <[email protected]> |
||
|
Anandan
|
96859c9bd1 |
feat(sandbox): unify command execution and enforcement (#781)
* feat(sandbox): unify command execution and enforcement * test(sandbox): make platform contracts portable * fix(sandbox): close execution lifecycle races Bound retained interactive output, preserve interruption state, and start workspace observation before process launch. Protect active runtime roots with cross-platform leases, capture transient protected-path creation events, and restore process attributes after PTY fallback. * fix(sandbox): restore strict macOS isolation Remove Seatbelt host-local network allowances after native validation showed localhost filters can reach host interfaces. Track process reap completion so retained sessions cannot signal a stale numeric PID. |
||
|
Vasanth T
|
4945684fa2 |
fix(sandbox): bind Windows elevated ACL setup to one no-follow handle (#765)
Closes #728. The Windows ACL setup resolved each target's pathname independently across os.Stat, GetNamedSecurityInfo, and SetNamedSecurityInfo (and again on rollback), so during elevated setup a lower-privileged local user could swap a target for a symlink/junction between operations and redirect the ACL change onto a system object the process never validated. That is a TOCTOU across a privilege boundary. Open each target once with a no-follow handle (FILE_FLAG_OPEN_REPARSE_POINT plus FILE_FLAG_BACKUP_SEMANTICS, READ_CONTROL|WRITE_DAC) and drive both the read (GetSecurityInfo) and the write (SetSecurityInfo) through that one handle, so they hit the same kernel object. isDir is read from the same handle rather than a separate Stat. A target that resolves to a reparse point is refused outright, since a setup target that is a symlink/junction during elevated setup is the swap-attack signature. Rollback re-opens no-follow rather than restoring by pathname; the residual window is small because the target is ACL-restricted by then, and one caller discards the rollback closure so handles are not retained for the whole sandbox lifetime. Tests (windows-tagged, no elevation needed): the handle-based apply plus rollback round-trip on a user-owned temp dir, the materialize-and-remove path, reparse rejection via a junction (needs no admin, so it runs in CI), and the not-exist mapping. The full privilege-boundary race can't be reproduced in CI, so the guard is verified structurally (one handle for read and write) and by junction rejection. |
||
|
KRATOSandKRATOS
|
f079b90f82 |
fix(sandbox): AST second opinion for interactive-command bypasses (#473) (#745)
* fix(sandbox): AST second opinion for interactive-command bypasses (#473) The interactive-command guard split shell commands with a hand-written parser (splitShellSegments), which mis-handles unusual quoting, command substitution, subshells, and newline separators — an interactive program hidden by one slips through and the agent hangs until the per-command timeout. Add astCommandFields(): mvdan.cc/sh/v3/syntax (already used by analyzer.go) to re-extract the real simple commands, then apply the SAME per-program checks the regex path uses (interactivePrograms + hasNonInteractiveFlag). This only ADDS detections and classifies every program exactly as before — ssh with a trailing command, python -c, etc. stay allowed — while catching the splitter's bypasses. Unparseable input (Windows cmd.exe, obfuscation) yields no commands and falls through to the regex path; the guard never hard-blocks on a parse error. * fix(sandbox): AST pass runs the full pipeline + guards dynamic programs (#473 review) Address review on #745: - The AST second opinion now applies the complete detection pipeline via a new inspectCommandFields helper (multi-word interactive segments, sh -c payload recursion, then the per-program lookup), so parser-hidden bypasses like { git rebase -i HEAD~1; } and { sh -c 'vim file.txt'; } are caught, not just bare interactive programs. - astCommandFields now skips a CallExpr whose program word is not a static literal (isLiteralWord), so a substitution concatenated with a literal ($(printf foo)vim -> foovim) can't fabricate an interactive program name. Regression tests added for both boundaries. (The un-braced $(printf foo)vim false positive is a separate, pre-existing hand-parser behavior — present on main before this PR — and is out of scope here.) * fix(sandbox): require literal words for the whole AST call (#473 review) astCommandFields guarded only the program word, then ran every argument through wordText, which keeps literal/quoted parts and silently drops CmdSubst/ParamExp. A dynamic ARGUMENT was therefore reconstructed lossily: `git $(printf foo)rebase -i HEAD~1` runs as `git foorebase -i` (a non-existent, non-interactive subcommand) but collapsed to `git rebase -i` and was blocked -- a false positive on a command the user never wrote, and new on this branch (the base correctly reports not-interactive). Extend the literalness guard to every word in the call: if any word carries an expansion, skip that AST command rather than classify a reconstruction whose runtime value is unknowable. Skipping is the safe direction -- the hand-written passes still run first, and a miss falls through to the normal permission prompt instead of hard-blocking. Genuine detection is unchanged: `git rebase -i HEAD~1` and the #473 bypass cases still classify exactly as before. --------- Co-authored-by: KRATOS <[email protected]> |
||
|
KRATOSandKRATOS
|
684901165d |
fix(sandbox): don't auto-allow shell when re-entrancy skips wrapping (#727) (#744)
The ambient ZERO_SANDBOXED=1 + ZERO_SANDBOX_BACKEND markers are user-controlled at an unsandboxed process boundary. Policy could auto-allow an ordinary shell command (backend reports a native wrapping sandbox) while the runner, seeing the same markers, returned an unwrapped pass-through plan — the command then ran directly on the host with no isolation and no prompt. shellSandboxActive now mirrors the runner: when IsAlreadySandboxed() (the markers are present, so wrapping is skipped) the sandbox is not the boundary for this command, so it reports inactive and the shell command takes the normal approval path instead of auto-allowing. Immediate mitigation per the issue; authenticated launch provenance is the longer-term fix. Co-authored-by: KRATOS <[email protected]> |
||
|
PierrunoYTandClaude Sonnet 5
|
9043baedcf |
fix(sandbox): scrub dynamic credential env vars (#682)
* fix(sandbox): scrub dynamic credential env vars * fix(sandbox): tolerate env assignments in configured sensitive keys A config value mistakenly given as a full assignment (e.g. "COMPANY_LLM_SECRET=...") would never match the real env key during scrubbing, silently re-exposing the credential to sandboxed commands. normalizeSensitiveEnvKeys now keeps only the name part before '=' and re-trims it, per PR review. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(sandbox): scrub credentials on direct/degraded command plans directCommandPlan (used when the native sandbox backend is unavailable, disabled, or not required, including the EnforcementDegraded fallback) left Env nil whenever spec.Env was nil, so exec.Cmd inherited the full caller environment unscrubbed. Configured apiKeyEnv credentials and dynamically named ZERO_OAUTH_<NAME>_CLIENT_SECRET variables leaked through that path even though the wrapped sandbox plans already scrubbed them. Route direct plans through scrubSensitiveEnv the same way wrapped plans do, snapshotting os.Environ() when spec.Env is nil so scrubbing still applies. Adds a regression test covering the degraded-fallback path with both a configured and a dynamically named secret. Co-Authored-By: Claude Sonnet 5 <[email protected]> --------- Co-authored-by: Claude Fable 5 <[email protected]> |
||
|
euxaristia
|
6fc1220f6a |
fix(sandbox): scrub sensitive credentials from sandbox environment (#660)
* fix(windows): retry atomic file renames on Access is denied / Sharing violation * test(swarm): add unit tests for rename retry and non-retryable errors * test(swarm): skip TestMailboxRenameRetry on non-Windows platforms * fix(sandbox): scrub sensitive provider credentials from sandbox env * refactor(sandbox): unify environment creation and reduce lines of code * fix(sandbox): forward workspaceRoot through sandboxEnvironment, gate Windows env on backend * style(sandbox,swarm): remove stray blank lines flagged by gofmt * refactor(fsutil): extract shared rename retry helper, derive scrub list from provider catalog Address CodeRabbit review feedback: - Add internal/fsutil.RenameWithRetry with an injectable rename override and move the four duplicated renameWithRetry/isWindowsSharingViolation copies (cron, sessions, sandbox, swarm) onto it. Mailbox keeps its rename test seam by passing its field through. Add fsutil tests for the Windows retry and non-retryable paths. - Remove the orphaned metadata.json.tmp in cron writeJob when the rename permanently fails, matching the sibling writers. - Derive scrubSensitiveEnv keys from providercatalog AuthEnvVars plus a static list of non-catalog secrets, so new providers are scrubbed automatically. AWS_PROFILE and GOOGLE_APPLICATION_CREDENTIALS stay because they are pointers, not secrets. * fix(sandbox): deny sandboxed reads of cloud credential stores by default Address CodeRabbit finding that scrubbed env pointers still left the underlying credential files readable under the read-all workspace posture: - Add credentialDenyReadPaths: default profile-level deny-read entries for ~/.aws, ~/.config/gcloud, ~/.azure, and the GOOGLE_APPLICATION_CREDENTIALS target file, wired into both permission profile builders so the seatbelt and Linux helper backends enforce them. Entries are stat-filtered and a store nested under a user AllowRead entry is dropped, keeping an explicit opt-out. - Keep the defaults out of Policy.DenyRead, whose emptiness gates escalated execution, and skip Windows so the profile deny list does not flip the runner off the WRITE_RESTRICTED token path. - Scrub GOOGLE_APPLICATION_CREDENTIALS from the sandbox env; keep AWS_PROFILE since the deny-read rule on ~/.aws is the real protection and the SDKs fall back to the default profile regardless. - Add credential deny path tests, extend the scrub test, relax the profile test's exact deny count to containment. * test(cli): pin HOME in sandbox policy golden test The default credential-store deny-read entries depend on what exists in the real home directory (the macOS CI image ships ~/.aws), which leaked host paths into the policy JSON golden comparison. Point HOME at an empty temp directory so the golden stays hermetic. * fix(sandbox): protect GOOGLE_APPLICATION_CREDENTIALS without a home, retry lock violations Address CodeRabbit review feedback: - credentialDenyReadPaths no longer returns early when the home lookup fails; home-based candidates become conditional and the GOOGLE_APPLICATION_CREDENTIALS target is always considered. - fsutil.RenameWithRetry also retries ERROR_LOCK_VIOLATION (errno 33), matching the mcp lock handling; helper renamed to isWindowsSharingOrLockViolation. - Extend tests: homeless GOOGLE_APPLICATION_CREDENTIALS case and a lock violation step in the retry sequence. * fix(sandbox): tighten env-scrub test assertion and add missing secret keys TestScrubSensitiveEnv only checked an allowlist plus a length count, so a duplicate allowed key silently replacing a required one would still pass. Compare the exact expected key/value map instead. Also add ZERO_WEBSEARCH_API_KEY and ZERO_DAEMON_REMOTE_TOKEN to scrubSensitiveEnv's sensitiveKeys list: both are real bearer secrets read from the environment but were missing from the scrub list, leaving them readable by sandboxed commands when set in the parent shell. |
||
|
euxaristia
|
a5d2e327c8 |
fix(sandbox): use WRITE_RESTRICTED token when no DenyRead paths are configured (#658)
Removing the WRITE_RESTRICTED flag in #612 made the restricted-SID check apply to reads as well as writes. Default Windows DACLs grant BUILTIN\Users rather than any SID in the token's restricted list (random capability SIDs, logon SID, Everyone), so the sandboxed process could no longer open any executable or non-KnownDlls DLL: every spawned command failed with exit 1 and no output, including cmd builtins' children (gh, git, where.exe). The existing TestWindowsRestrictedTokenNestedPipeCapture smoke test reproduces the regression. Restore WRITE_RESTRICTED, but only when the permission profile has no DenyRead paths. The flag makes the kernel skip restricted-SID deny ACEs for reads, which is exactly the DenyRead bypass #612 fixed, so profiles that configure DenyRead keep the fully restricted token and trade spawn capability for read-deny enforcement. DenyRead is empty by default, so the common case regains a working sandbox while #612's guarantee holds for the profiles that rely on it. |
||
|
euxaristia
|
5c4815a66e |
fix(sandbox): unblock git fetch/commit/add under the write-restricted sandbox (#654)
* fix(sandbox): stop blocking git fetch/commit/add by unblocking .git writes The sandbox denied every write under .git for shell-executed commands, so any git operation that touches its own metadata (fetch, commit, add, pull, merge, stash) failed under the default sandbox. Narrow the block to .git/hooks (auto-executing scripts) and .git/config (remote URLs, credential.helper, core.hooksPath), the two subpaths that are actually dangerous to write; the rest of .git stays writable to git subprocesses since those writes go through git's own invariants. The app-level auto-allow gate for direct file-tool writes (write_file, edit_file, apply_patch) is unaffected and still treats all of .git as protected, since hand-editing git's internals bypasses those invariants. * test(cli): normalize Windows path separators in sandbox policy golden The sandbox policy golden is shared across platforms, but readOnlySubpaths carve-outs are built with filepath.Join and render as backslash paths on Windows. normalizePortableJSONRootString only converted a bare backslash, so the Windows smoke run diverged from the forward-slash golden. Replace all backslashes with slashes during normalization so the shared golden passes on every runner. |
||
|
euxaristia
|
3d96ac7e55 |
fix(sandbox): remove windowsWriteRestricted flag to fix DenyRead bypass (#612)
On Windows, creating restricted tokens with windowsWriteRestricted (0x08) instructs the kernel to skip checking the restricted SIDs list for read operations. This bypasses DenyRead path policies on NTFS. Remove the flag to enforce restricted SID constraints on both read and write operations, and add integration test assertions to verify that DenyRead paths are correctly blocked. |
||
|
|
ba4c00755d |
fix(sandbox): Windows-appropriate suggestions when blocking interactive commands (#414)
* fix(sandbox): give Windows-appropriate suggestions when blocking interactive commands DetectInteractiveCommand already receives goos, but its remediation text for pagers (less/more/most), process monitors (top/htop/btop/ btm), and `tail -f` hardcoded POSIX-only alternatives (cat, head, tail -n N, ps aux) regardless of platform. On Windows none of those exist either, so blocking e.g. `type file | more` correctly stopped the interactive command but then pointed the model at three more commands cmd.exe doesn't have — directly undermining the guidance this check exists to give. Reported in the wild (screenshot showing `more` blocked with a "use cat, head, or tail -n N" suggestion on a Windows session). Adds an optional windowsSuggestion per program/segment, used in place of the POSIX suggestion when goos == "windows" — `type` for pagers, `tasklist` for process monitors, the read_file tool for tail -f. Non-Windows suggestions are unchanged. Co-Authored-By: Claude Sonnet 5 <[email protected]> * test(sandbox): cover remaining Windows suggestion overrides and fix GOOS-blind assertion TestBashToolBlocksInteractiveCommandThroughSandbox asserted the pager guard message always contains "cat", but on Windows it now correctly suggests "type" — breaking Smoke (windows-latest) CI. Make the assertion GOOS-aware. Also extend the safe_command windowsSuggestion test table to cover most/top/btop/btm, which were missing alongside the already-tested more/less/htop/tail -f. Co-Authored-By: Claude Sonnet 5 <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Vasanthdev2004 <[email protected]> |
||
|
euxaristia
|
066681855b |
fix(tools): block MSYS and WSL shells under the Windows sandbox (#587)
* fix(tools): block MSYS and WSL shells under the Windows sandbox Git-for-Windows bash.exe and sh.exe are MSYS binaries: the MSYS runtime secures its signal pipe and shared-memory sections with explicit DACLs granting only the user, Administrators, and SYSTEM (sigproc.cc sigproc_init -> sec_user_nih), so under the WRITE_RESTRICTED token the second access check can never pass and startup dies with "couldn't create signal pipe" or "CreateFileMapping <SID>.1", Win32 error 5. The System32 WSL bash launcher fails equivalently: the restricted token cannot connect to the WSL service (Bash/Service/CreateInstance/ E_ACCESSDENIED). Every executable a bare bash can resolve to fails, so block bash and sh preflight like the MSYS coreutils from #476, mention the indirect spawn paths (git hooks, git/gh credential helpers) in the shared suggestion, detect the WSL launcher's UTF-16LE service-denied stderr, and document the root cause in the runner's KNOWN LIMITATION comment beside the Schannel note. * fix(tui): align picker_test switchProviderModel call with 4-value return * test(tools): cover sh.exe in Windows shell blocking test |
||
|
euxaristia
|
8b248f4e11 |
fix(tools): platform-specific pager suggestions, quote/caret-safe cd detection (#543)
less/more/most now suggest `type` instead of cat/head/tail on Windows, since those tools don't exist there. Drop `more` from the MSYS sandbox suggestion too, since more is itself blocked as an interactive pager and would send the model straight into that guard next. detectShellCommandIssue also strips double-quoted spans and cmd.exe caret escapes before matching the bash-style `cd /foo` pattern, so a cd-shaped string inside a quoted argument (e.g. a commit message) isn't mistaken for an actual command. |
||
|
PierrunoYTandCopilot Autofix powered by AI
|
008bc9b3f3 |
fix: address bugs found in a multi-agent codebase audit (#481)
* fix: address bugs found in a multi-agent codebase audit Five parallel review agents each swept a different area (self-update, TUI keybindings, agent/session handling, sandbox/shell safety, and auth/credential storage) and surfaced nine concrete defects, all fixed here with regression tests: - sandbox: grant_scope path comparisons were case-sensitive, letting a persisted deny grant be bypassed by spelling a Windows path with different case - securefile/oauth: createSecretFile's Windows ERROR_ACCESS_DENIED handling was only applied to the oauth token store, not the shared credstore backend; both also masked the real lock-contention error behind a misleading "file does not exist" timeout - cli: firstUsableProvider ignored OAuth logins when picking a fallback provider, forcing unnecessary re-onboarding for an already-authenticated user - sessions: exec prompt summarization truncated on a raw byte offset, which could split a multi-byte UTF-8 rune and embed invalid UTF-8 into a resumed session's prompt - tui: configurable keybindings had no collision detection, so a remapped chord could silently make a hardcoded shortcut (or another configured binding) permanently unreachable - tools: the Windows shell-syntax pre-flight check wasn't quote-aware and could block harmless commands whose quoted arguments merely contained operator-like text - update: checksum verification didn't cross-check the archive filename it verified against the one being extracted; extractZip didn't reject symlink-mode entries like extractTarGz already did; replaceBinary's restore-on-failure rename had no retry, risking a permanently missing binary on a transient Windows file lock * fix: close remaining PR 481 review findings * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <[email protected]> * fix(tools): drop stale ; test case after rebasing onto main's MSYS check Rebasing fix/bug-hunt-findings onto main pulled in #476, which replaced this file's regex-based windows_shell_syntax detection with a segment/word-anchored check and deliberately stopped treating ; as a command separator on Windows (cmd.exe doesn't split on ; the way bash does). detectShellCommandIssue itself came out of the rebase unchanged from main (our quote-masking version was fully superseded by #476's more thorough segment-based approach), but the quoted-content test this PR added still asserted the old bash-style semantics for `echo "foo" ; ls -la`, which now correctly returns no issue since it's a single echo invocation with literal arguments under cmd.exe, not a real ls invocation. --------- Co-authored-by: Copilot Autofix powered by AI <[email protected]> |
||
|
PatrickNoFilter
|
0f69d995e9 |
fix: Termux/Android support — PRoot scroll, SIGSYS sandbox, build docs (#509)
* fix(tui): TracerPid PRoot detection, Ctrl+U/D direction, composer guard - Replace Termux env var detection with isRunningUnderPRoot() via /proc/self/status TracerPid, cached with sync.OnceValue - Mouse mode falls back to CellMotion (not AllMotion) under PRoot to fix broken touch scrolling - Add Shift+Up/Shift+Down before plain KeyDown/KeyUp for transcript scroll with permission/askuser cursor + composer guard - Add Ctrl+U (scroll up) and Ctrl+D (scroll down) with proper direction: Ctrl+U=move up, Ctrl+D=move down - Add guards for active modals (wizard, picker, spec review, etc.) to avoid intercepting their Ctrl+U/D keybindings - Tests: TestPermissionCursorCtrlU/D (direction regression), TestShiftUp/DownComposerGuard (composer guard) - References issue #507 * fix(sandbox): avoid faccessat2 for Termux/Android portability Replace exec.LookPath with lookupExecutable that uses os.Stat instead of the faccessat2 syscall. The faccessat2 syscall (439) is blocked by the Samsung seccomp filter on Android, causing SIGSYS termination. os.Stat uses the newfstatat/statx syscalls which are universally permitted. This is part of broader Termux/Android support for Zero. * docs(install): add Termux/Android build and configuration guide Document how to build Zero for native Android/Termux: GOOS=android to avoid the SIGSYS crash, proot bind-mount for DNS resolution, PRoot scroll fix, and free-tier provider setup. * fix(sandbox): skip empty PATH segments, use platform-aware executable check |
||
|
euxaristiaandClaude Fable 5
|
563a6dbe91 |
fix(sandbox): fix nested pipe creation under the Windows restricted token (#456)
* fix(sandbox): fix nested pipe creation under the Windows restricted token
Any subprocess spawned FROM WITHIN a process already running under Zero's
write-restricted token, that captures the child's output via a pipe, failed
with ERROR_ACCESS_DENIED. Reproduces with `gh pr list` (gh shells out to git
and captures its output), with a bare Go program doing exec.Command(...).
Output(), and with cmd.exe's own FOR /F ('...') construct, which uses the
identical CreatePipe+CreateProcess pattern internally with no external tools
involved.
Root cause: a WRITE_RESTRICTED token requires a WRITE-type access check to
match BOTH the normal enabled-SID grant and a separate grant to one of the
token's restricted SIDs. CreateRestrictedToken does not modify the default
DACL it inherits from the base token, so a pipe created with a default
security descriptor (the common case; every language runtime's "spawn a
subprocess and capture output" primitive does this) only carries ACEs for
the base identity, none of which are in the restricted-SID list, so the
second check fails. This is the same failure family already documented for
Schannel/SEC_E_NO_CREDENTIALS, but broader in scope: it affects any pipe,
event, mutex, or other kernel object created with default security by a
process running under the token.
Fix: add the token's own logon SID (already unconditionally present in its
restricted-SID list) to the token's default DACL, so objects it creates
without an explicit security descriptor automatically satisfy the extra
check. This does not touch filesystem access: NTFS write-jailing is
enforced by the explicit ACL grants applied to workspace paths, created
WITH an explicit security descriptor, and the token's default DACL is only
consulted when none is supplied. Exposure is bounded to the same logon
session (only other processes running as the same signed-in user could, in
principle, open a NAMED object the sandboxed process creates with default
security; anonymous pipes have no name and are reachable only via an
inherited handle, so this is a no-op for the actual objects this fixes).
This is not specific to the unelevated tier added in #427 - the affected
token-construction code is shared by both the fully elevated and unelevated
restricted-token paths, so this predates that work.
Verified on Windows 11 (before/after, using an unfixed build for
comparison):
- Before: FOR /F fails with Win32 error 5 (ERROR_ACCESS_DENIED), masked
behind cmd.exe's generic "not recognized" message; `gh pr list` fails
with "failed to run git: pipe: Access is denied."; a bare Go nested
exec.Command(...).Output() fails with "pipe: Access is denied."
- After: all three succeed; `gh pr list` returns real PR data.
- Write-jail regression check: a write outside every granted root is still
denied by the OS after the fix (unchanged from before).
New TestWindowsRestrictedTokenNestedPipeCapture (ZERO_SANDBOX_REAL_SMOKE=1)
pins the FOR /F case using only cmd.exe, no external dependency. go vet and
the full internal/sandbox suite are clean (excluding pre-existing failures
already present identically on unmodified main, unrelated to this change).
Co-Authored-By: Claude Fable 5 <[email protected]>
* fix(sandbox): fix dangling-buffer use-after-scope in default DACL merge
Addresses the blocker from review on this PR (Vasanthdev2004, gnanam1990,
coderabbitai): windowsTokenDefaultDacl returned a *windows.ACL that points
INTO the local buf byte slice backing TOKEN_DEFAULT_DACL - Windows embeds
the ACL data in the same allocation GetTokenInformation fills. The
function's own runtime.KeepAlive(buf) only covered buf up to that
function's own return; nothing kept it alive across the caller's later use.
In broadenWindowsRestrictedTokenDefaultDacl, the returned ACL pointer was
then dereferenced by native code in setEntriesInACL after intervening
allocations (the EXPLICIT_ACCESS literal, TrusteeValueFromSID) that are
GC-safepoint-eligible - a real use-after-free window in code that
constructs part of the sandbox's own restricted token, even though Go's
current non-moving GC meant it didn't manifest in testing.
windowsTokenDefaultDacl now returns the backing buffer alongside the ACL;
the caller holds runtime.KeepAlive(oldDaclBuf) until after
SetEntriesInAclW (the last point that dereferences it) returns.
Also from review: SetEntriesInAclW reports its Win32 error directly in the
return value, not via GetLastError, so setEntriesInACL now uses that
return value directly instead of the (potentially stale/unrelated) syscall
error. And narrowed the default-DACL grant from GENERIC_ALL to GENERIC_READ
| GENERIC_WRITE - the second WRITE_RESTRICTED access check only needs a
read/write match; GENERIC_ALL also implied DELETE/WRITE_DAC/WRITE_OWNER
that the pipe/event/mutex use case never needed.
Re-verified on Windows 11 with the fixed runner: nested pipe capture still
succeeds (pure Go exec.Command(...).Output(), and TestWindowsRestrictedTokenNestedPipeCapture
via go test itself), and the write-jail regression check (write denied
outside every granted root, allowed inside) is unchanged. Full
internal/sandbox suite green.
Co-Authored-By: Claude Fable 5 <[email protected]>
---------
Co-authored-by: Claude Fable 5 <[email protected]>
|
||
|
euxaristiaandClaude Fable 5
|
190241bd59 |
fix(tools): fix cmd.exe /S/C corrupting commands with embedded quotes (#465)
* fix(tools): fix cmd.exe /S/C corrupting commands with embedded quotes Any shell command whose own text contains embedded double quotes - python -c "print(15 / 3)", git commit -m "message", node -e "..." - was silently corrupted on Windows before reaching the target program. Reported live: a model ran python -c "print(15 / 3)" and got back a SyntaxError showing the code Python received as literally "print(15, truncated right before the /. Root cause: bash.go passes the full command text as ONE argv element to cmd.exe /d /s /c, and Go's os/exec escapes it correctly for a standard CommandLineToArgvW-style parser (wrapping it in quotes, backslash-escaping the embedded ones). But cmd.exe's own /C remainder parsing does not follow those rules once /S is present: per its documented behavior, it strips the FIRST and LAST literal quote character in the whole remainder - not a matched pair - before handing the (now corrupted) text along to be re-tokenized. The escaping was correct for one layer of parsing; the command line actually goes through two. Fix: on Windows, write commandText to a temp .cmd script file and point /c at the file's path instead of the raw command text. This sidesteps cmd.exe's /C-remainder heuristic entirely, since the remainder is then just a plain path with no quotes to mis-strip. @echo off is prepended since cmd.exe treats a /c target ending in .cmd as a batch file and echoes each line it runs by default, which would otherwise pollute captured stdout (caught by the existing bash tool test suite before this commit, not left in). buildBashCommand now returns a cleanup func (in addition to plan.Cleanup) for the temp file. Both callers updated: bash.go's one-shot run defers it immediately; exec_command.go's persistent session composes it with startExecProcess's own cleanup and runs it after the session's process actually exits, not before. This fix lives upstream of the sandbox wrapping, in how the Name/Args spec is constructed before either the direct or sandboxed code path consumes it, so both benefit without separate sandbox-specific changes - cmd.exe's own command-line parsing does not depend on what token launched it. New TestBashToolPreservesEmbeddedQuotesOnWindows pins the exact failure shape via a self-dispatch test helper (a quoted argument containing a space and a slash, matching the original bug report's truncation point), with no dependency on python being installed. Full internal/tools suite green, excluding 3 failures pre-existing and identical on unmodified main in my environment (unrelated path-assumption issues under a deeply nested working directory). Co-Authored-By: Claude Fable 5 <[email protected]> * fix(tools): pass the shell command line to cmd.exe raw instead of via a temp script Review on the original fix found real problems with running commandText from a temporary .cmd file: it silently changed cmd.exe command-line syntax into batch-file syntax (for example, for %i loops need %%i only inside an actual .bat/.cmd file), /s was still corrupting the script path itself if it contained spaces, and the script leaked on sandbox planning errors. The actual root cause was narrower than the temp-file approach assumed. cmd.exe's own /C remainder parsing is not CommandLineToArgvW-compatible: it strips the first and last literal quote from the remainder whenever it starts with a quote, without undoing backslash-escaping. Passing commandText as a normal exec.Cmd argument makes Go wrap it in exactly that shape (quoted because it has spaces, inner quotes escaped as \") since it contains embedded quotes, which is what the stripping then corrupts. Fixes it at the source instead: commandText now reaches cmd.exe completely unescaped (via SysProcAttr.CmdLine for the unwrapped path, and the same treatment inside the sandboxed runner's own process creation for the wrapped path), exactly as if it had been typed at an interactive prompt. No outer quotes means the stripping heuristic never triggers for a command like `python -c "print(15 / 3)"`, and a command that legitimately starts with a quoted executable path is handled by cmd.exe's own careful quote-preserving special case for that shape. Also drops /s, which forced the more aggressive of cmd.exe's two stripping behaviors. No temp file, so nothing to leak on error, and no batch-file reinterpretation of the command text. internal/sandbox/windows_runner.go now owns the shared shell-argv shape (WindowsShellArgs/WindowsShellCommandLine) so internal/tools/bash.go and the sandboxed runner's windowsCommandLine build the exact same raw command line instead of duplicating the flag literals in two places. * chore: retrigger CI The Windows smoke job failed on TestLoadOrCreateSecretConcurrentConverges in internal/oauth (a lock-file access-denied flake), unrelated to this change. internal/tools and internal/sandbox, the packages this PR touches, both passed. Retriggering to confirm. --------- Co-authored-by: Claude Fable 5 <[email protected]> |
||
|
euxaristiaandClaude Fable 5
|
f653dcac36 |
fix(sandbox): gate /tmp test assertions on GOOS, not path existence (#426)
* fix(sandbox): gate /tmp test assertions on GOOS, not path existence
TestPermissionProfileFromPolicyIncludesDefaultTempWriteRoots and
TestNewScopeNormalizesAndValidatesExtraRoots guard their /tmp assertion with
pathExists("/tmp"), intending to skip it on Windows. But Go resolves the bare
path /tmp against the current drive on Windows, so on any host where C:\tmp
exists the guard passes while the product code (correctly, by design - see
defaultTempWriteRootCandidatesForGOOS) uses %TEMP%/%TMP% instead, and both
tests fail. Gate the assertion on runtime.GOOS != "windows" as well; POSIX
behavior is unchanged.
Co-Authored-By: Claude Fable 5 <[email protected]>
* chore: retrigger CI
The Windows smoke job failed on TestLoadOrCreateSecretConcurrentConverges in internal/oauth, unrelated to this change. Retriggering to confirm it was a flake.
---------
Co-authored-by: Claude Fable 5 <[email protected]>
|
||
|
euxaristiaandClaude Fable 5
|
8d0c5feccb |
fix(sandbox): self-heal a corrupt unelevated setup marker (#437)
A malformed/truncated windows-unelevated-setup.json returned a hard error from loadWindowsUnelevatedSetupMarker, bricking every unelevated command until the file was hand-deleted, while the unknown-schema path already reset to an empty marker. Treat corrupt JSON the same way: the marker only memoizes idempotent ACL applies, so a reset can only cost redundant work, never skip enforcement. Addresses the review feedback from coderabbitai and @anandh8x on #427. Co-authored-by: Claude Fable 5 <[email protected]> |
||
|
euxaristiaandClaude Fable 5
|
b9ddd6f421 |
feat(sandbox): unelevated Windows fallback tier instead of prompts-only degrade (#427)
Before the one-time elevated `zero sandbox setup` has run, Windows dropped straight from the native sandbox to the in-process policy gate: commands ran with no OS enforcement at all. But only the WFP network filters actually need Administrator rights - the write-restricted token and the workspace ACL grants (DACL edits on user-owned roots) work unelevated. Add an `unelevated` enforcement tier between native and degraded: when the elevated setup marker is missing and the profile restricts the filesystem, the command still wraps through the command runner at a new `unelevated` sandbox level. The runner applies the workspace ACL plan itself (recording applied plans by hash in windows-unelevated-setup.json so repeat commands skip the apply) and launches under the same write-restricted token, so the OS-level write-jail holds. Network enforcement stays with the per-command approval gate at this tier, and the downgrade reason says so; profiles that need the sandbox only for network isolation keep the old degrade behavior, and `--sandbox require` still errors with the setup hint. Verified end to end on Windows 11 without elevation: a write inside the workspace succeeds under the restricted token, a write outside every granted root is denied by the OS (new ZERO_SANDBOX_REAL_SMOKE integration test, using cmd.exe because managed PowerShell cannot initialize its crypto provider under a write-restricted token on some hosts). Co-authored-by: Claude Fable 5 <[email protected]> |
||
|
Gautam ManchandaniandGautam Manchandani
|
960db9660e |
fix(tools): require permission before web_search requests (#382)
* fix(tools): gate web_search behind network permission * chore(tools): clarify web_search permission reason --------- Co-authored-by: Gautam Manchandani <[email protected]> |
||
|
Kunj SHAH
|
33dc7ae2cc |
fix(tools): CRLF line ending mismatch in edit_file tool on Windows (#378)
* fix(tools): CRLF line ending mismatch in edit_file tool on Windows * fix: resolve gofmt error, add robust CRLF guard, and fix Windows sandbox and OAuth test flakiness |
||
|
Anandan
|
80ab238361 |
Align sandbox command approval behavior (#322)
* Align shell approval sandbox execution Make approved shell command-prefix executions use the escalated sandbox path when denied-read policy allows it, so remembered approvals do not still hit the restrictive sandbox. Allow auto sandbox mode to degrade to a direct command plan when native command wrapping is unavailable, while strict sandbox require continues to fail closed. Update regression tests for degraded execution, prefix-approved shell calls, and unavailable native sandbox metadata. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/agent ./internal/sandbox Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/tools -run 'TestRegistryRunsWithDegradedUnavailableNativeSandbox|TestBashToolRunsWithDegradedUnavailableNativeSandbox|TestBashRequireEscalatedBypassesNativeSandboxAfterApproval|TestExecCommandRequireEscalatedBypassesNativeSandboxAfterApproval' Tested: GOCACHE=/tmp/zero-go-cache go test ./... -run '^$' Tested: git diff --check * Align sandbox command approval behavior Evaluate reusable shell approvals across plain command segments so prefix grants can cover commands with safe read-only tails without relying on command-specific sandbox exceptions. Keep process inspection commands sandboxed by default; unsandboxed host access continues to require explicit escalation or a narrow command-prefix approval. Improve shell tool guidance around sandboxed additional permissions, full escalation, justifications, and narrow reusable prefixes. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/sandbox ./internal/tools ./internal/agent Tested: env HOME=/tmp/zero-test-home-clean XDG_CONFIG_HOME=/tmp/zero-test-config-clean GOCACHE=/tmp/zero-go-cache go test ./... * Normalize explicit sandbox backend platforms Infer the platform from a caller-provided sandbox backend name before choosing the manager runtime platform. This keeps Linux, macOS, and Windows backend plans deterministic across host OSes when tests or callers provide a backend without an explicit Platform field. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/sandbox ./internal/tools Tested: env HOME=/tmp/zero-test-home-clean XDG_CONFIG_HOME=/tmp/zero-test-config-clean GOCACHE=/tmp/zero-go-cache go test ./... |
||
|
Anandan
|
f1502f0bfe |
Add local control browser and terminal tools (#319)
* Add local control browser and terminal tools Add configurable local browser, desktop, and terminal control helpers with packaged helper discovery for install and release flows. Improve sandbox scope classification for local control commands and shared temporary paths. Add structured browser actions, terminal session line input/key normalization/read support, and artifact capture for local-control outputs. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/localcontrol ./internal/tools ./internal/cli ./internal/config -run 'Test.*LocalControl|TestTerminal|TestBrowser|TestDesktop|TestCapture' Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/localcontrol ./internal/tools ./internal/cli ./internal/config ./internal/agent Tested: tuistory smoke launched bash, sent interactive input, waited for expected output, and captured a no-cursor snapshot Tested: git diff --cached --check * Fix npm wrapper helper manifest test on macOS Canonicalize expected helper paths in the npm wrapper manifest test so macOS /var and /private/var temp path aliases compare correctly. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/npmwrapper Tested: git diff --check * Harden local control review fixes Respect resolved local-control config before tool listing and filter validation while preserving stream-json usage error ordering. Harden browser, desktop, and artifact tool validation; guard Linux-only app launch behavior; clear stale helper manifests from the npm wrapper. Make helper packaging lockfile-based with npm ci and add installer/package tests for helper staging. Tested: focused local-control/tool/CLI/release/install/sandbox suite Tested: isolated full go test ./... Tested: macOS and Windows compile-only checks for touched packages |
||
|
Anandan
|
644f61fa58 |
Fix shared temp roots in sandbox scope (#314)
Treat platform temp directories as default writable roots so file tools, shell commands, and sandbox profiles share the same temp view. On Linux, bind host temp roots instead of mounting an isolated tmpfs at /tmp so artifacts created under /tmp remain visible across tool and shell calls. Tests: GOCACHE=/tmp/zero-go-cache go test ./internal/sandbox ./internal/tools ./internal/cli ./internal/agent |
||
|
Anandan
|
6be50cd3a7 |
fix(sandbox): retry approved network shell commands (#301)
* fix(sandbox): retry approved network shell commands * test(sandbox): fix portable smoke expectations * fix(sandbox): restore Windows offline egress block |
||
|
KRATOS
|
b776a9281f |
fix(sandbox): grant ancestor metadata so cd into the workspace works (#302)
* fix(sandbox): grant ancestor metadata so cd into the workspace works macOS seatbelt granted file-read* on each read root via (subpath "<root>"), which covers the root and its descendants but NOT its parent directories. An absolute cd (and any path the kernel canonicalises) must stat each ancestor to traverse to a deeply-nested workspace like /Users/me/Downloads/app — those parents weren't granted, so seatbelt denied the chdir and surfaced it as the misleading 'cd: <path>: Not a directory' (ENOTDIR). This blocked the agent from cd-ing into its own workspace (e.g. 'cd /abs/workspace && go test'). Fix: grant file-read-metadata + file-test-existence on the path-ancestors of each read root (metadata only — ancestor contents stay unreadable). Same pattern already used for /opt/homebrew, /usr/local, /System/Volumes/Data/private. Verified end-to-end: 'cd /abs/workspace && ...' now succeeds under the real generated profile. go build/vet/test green. * fix(sandbox): skip path-ancestors for filesystem roots (invalid SBPL) Follow-up to the ancestor-metadata traversal fix. A read root of "/" has no ancestors, and (path-ancestors "/") is invalid SBPL — sandbox-exec aborts with exit 65 and the sandboxed command can't launch at all. This was reachable via the compat builder (ReadRoots=["/"] flipped to restricted under EnforceWorkspace) and any degenerate "/" workspace. seatbeltAncestorMetadataRule now skips any root that is its own parent (filepath.Dir(clean)==clean), neutralizing "/" and volume roots regardless of entry point. Verified: the compat profile now compiles under sandbox-exec (exit 0). Found by adversarial verification of the prior commit. * fix(sandbox): grant the CLT toolchain so git/clang/make work On macOS, /usr/bin/git (and clang, make, etc.) are thin stubs that resolve the real binary under the active developer dir — usually /Library/Developer/CommandLineTools. The sandbox's platform read roots didn't include it, so the stub couldn't reach the real tool and failed with the misleading 'xcode-select: No developer tools were found' error. Add /Library/Developer (read-only) to the platform read roots. * fix(sandbox): allow reading the user's global git config git reads identity/config from the user's global git config, but the sandbox confines reads away from HOME, so it failed with 'unable to access ~/.gitconfig: Operation not permitted'. Grant read-only on the git config FILES specifically (~/.gitconfig and ~/.config/git/config) — not the ~/.config/git directory, which can hold an XDG credential store — so git runs while credentials and the rest of HOME stay unreadable. The path-ancestors traversal grant covers reaching them. * fix(sandbox): let the agent terminate user-owned processes on request The seatbelt profile only allowed signaling the agent's own process group ((target self) (target pgrp)), so a process started by a PREVIOUS session — e.g. a dev server still listening on a port — was in a different group and could not be killed. The agent would thrash through kill/pkill/osascript/sudo, all denied, and never stop the process the user asked it to terminate. Relax to (allow signal). The kernel still enforces UID ownership: a sandboxed command runs as the user, so it can only signal the user's own processes, never root's or another user's. Verified end-to-end: a sandboxed command now kills a different-group user-owned process under the real generated profile. * fix(sandbox): let the agent find processes + guide it to working tools Pairs with the signal fix (8b5018d). Two more gaps blocked process management: - process-info was scoped to (target same-sandbox), so the agent couldn't even inspect processes outside its own sandbox. Relaxed to (allow process-info*). - `ps` is setuid root and macOS sandbox-exec refuses to exec setuid binaries (privilege-escalation guard) — unfixable via profile. `pgrep` needs a blocked system service. So the agent kept thrashing on ps/pgrep/os.kill. Add macOS shell guidance pointing the model at the tools that DO work: lsof to find a process, kill to stop it. With the signal grant, lsof+kill now manages processes reliably (verified end-to-end). * fix(sandbox): grant git config at the seatbelt rule, not the shared profile The git-config read paths (~/.gitconfig, ~/.config/git/config) were added to the cross-platform permissionProfileReadRoots, so they leaked HOME-dependent absolute paths into the platform-agnostic 'sandbox policy --json' snapshot and broke the golden test on every OS (machine-specific paths + Windows separators). Move the grant to seatbeltReadRule (macOS only, where reads are actually confined): the PermissionProfile stays platform-agnostic so the golden is stable, and the seatbelt profile still grants the config files + their ancestor metadata for traversal. Verified: golden test green, full 'go test ./...' green, and git still reads ~/.gitconfig under the real generated profile. * fix(sandbox): grant ancestor metadata for platform read roots too Addresses CodeRabbit review on #302. seatbeltAncestorMetadataRule was passed only fs.ReadRoots + gitConfig, on the assumption that platform read roots are top-level or already covered. /Library/Developer (the CLT toolchain) breaks that: it needs /Library stat-able as an ancestor, so a chdir-style traversal into it (cd /Library/Developer/CommandLineTools) failed with ENOTDIR even though reads succeeded (exec doesn't require ancestor-stat the way chdir does). Include the platform read roots in ancestorRoots when IncludePlatformRoots is set. Verified: cd into the CLT toolchain now succeeds under the real profile; golden + sandbox tests green. Regression test added. |
||
|
Vasanth T
|
1b1abd5bf1 |
feat(sandbox): online/offline Windows network identity — run approved network commands sandboxed (#297)
* feat(sandbox): online/offline Windows network identity (run approved network commands) Approved network commands bricked on Windows with "windows sandbox setup is out of date: network policy changed": the setup marker was locked to network=deny, so any command granted network (curl, git push, npm install) ran with network=allow and failed marker validation. Fix with a two-identity model that fits Zero's restricted-token architecture — no extra logon users, no DPAPI, no admin at runtime: mint one synthetic "offline-marker" SID per sandbox home and scope the persistent WFP block filter to THAT SID only. Network is then gated purely by the restricted token's SID set: - deny (offline): token = write-capability SIDs + offline-marker SID -> block filter matches -> no network; writes jailed by the capability SIDs. - allow (online, approved): token = write-capability SIDs only -> filter never matches -> network reaches out; writes jailed identically. The setup marker becomes mode-AGNOSTIC: it fingerprints the provisioned offline infrastructure (NetworkInfraHash + OfflineFilterSID), not the per-command mode, so one setup validly serves both an allow and a deny command. Schema bumped 3->4 (forces a clean re-setup; old markers scoped the filter to write SIDs). - windows_runner.go: Offline SID in WindowsCapabilitySIDs (schema 2, back-compat upgrade) + WindowsOfflineMarkerSID + windowsRuntimeTokenSIDs. - windows_network.go: BuildWindowsNetworkInfraPlan + WindowsNetworkInfraHash; drop the now-dead mode-coupled BuildWindowsNetworkPlan/*Hash helpers. - windows_setup.go: marker schema 4; validation drops per-command-mode equality. - windows_setup_windows.go: setup always installs the offline-scoped infra filter. - windows_command_runner_windows.go: compose the token SID set per network mode. Cross-platform core fully tested (mode selector, mode-independent infra+hash, back-compat SID upgrade, one-marker-validates-both-modes regression, schema-3 rejected); sandbox/doctor suites + windows cross-compile green. The WFP ALE_USER_ID/restricting-SID behavior needs the gated Windows integration smoke test. * docs(sandbox): note the Schannel HTTPS limitation under the Windows restricted token Document the known SEC_E_NO_CREDENTIALS limitation at the token-composition site: an approved online command reaches the network, but HTTPS via Windows Schannel fails inside the restricted token because Schannel can't acquire its per-user TLS credential under a WRITE_RESTRICTED/LUA token. Fundamental restricted-token ↔ Schannel incompatibility, unsolved even in the reference sandboxes (codex#17459). Workarounds: degraded mode or the in-process web_fetch tool. Comment-only. * docs(sandbox): keep the Schannel-limitation note vendor-neutral |
||
|
Vasanth T
|
ffaa3a9a48 |
fix(sandbox): stop bricking Windows when the sandbox isn't set up (degrade like Linux/macOS) (#295)
* fix(sandbox): degrade Windows to permission-gating when not set up (stop bricking) On Windows the sandbox command runner hard-fails EVERY command until a one-time elevated `zero sandbox setup` writes the setup marker — so a fresh Windows user can't run a single shell command (the "windows sandbox is not initialized" error). Linux/macOS already degrade when their sandbox is unavailable; Windows was the lone outlier that bricks itself. BuildExecutionRequest now, on Windows when the setup marker is missing: - default (auto): DEGRADE to EnforcementDegraded — the command runs unwrapped under the in-process policy gate (workspace path-confinement) + per-command approval, with a one-time downgrade notice pointing at `zero sandbox setup` for full WFP/ACL/network isolation. - `--sandbox require`: still hard-errors with a clear setup hint, so strict callers keep their guarantee. - marker present: unchanged (native, wrapped, fully sandboxed). Matches every reference agent (codex defaults its Windows sandbox off and runs under approval; opencode/openclaude/vix degrade on Windows) — none brick the CLI because a setup step hasn't run. Test covers all three paths; sandbox/doctor/cli suites green; vet/build clean. * fix(sandbox): don't auto-allow shell commands while Windows is degraded Follow-up to the degrade fix in this PR. The permission engine's shellSandboxActive keyed on backend *capabilities* (CommandWrapping && NativeIsolation), which stay true on Windows even when setup hasn't run. So once the degrade fix let commands RUN unwrapped, the engine still auto-allowed ordinary shell commands as if sandboxed — running them with no OS isolation AND no permission prompt, breaking the per-command-approval floor the degrade is supposed to keep (the function's own doc comment promises "ordinary shell auto-allow never runs an unsandboxed command"). shellSandboxActive now returns false on Windows when the sandbox isn't initialized (mirrors manager.BuildExecutionRequest), so a degraded shell command falls through to the normal approval prompt. Network/destructive/out-of-workspace checks already ran earlier and are unaffected; native (post-setup) still auto-allows. Regression test covers degraded->prompt and native->auto-allow. Sandbox suite green. |
||
|
KRATOS
|
b46e952282 |
fix(sandbox): let macOS sandbox run Homebrew/usr-local tools (node, python3) (#296)
A scoped (workspace-enforced) exec gave commands a minimal PATH and restricted
reads, so a Homebrew python3/node either resolved to the broken /usr/bin/python3
stub or could not start. Four coordinated allowances on macOS:
- PATH: prepend /opt/homebrew/{bin,sbin} + /usr/local/{bin,sbin} (ensureMacToolPaths).
- read: broaden platform read roots from .../lib to the whole /opt/homebrew and
/usr/local trees so the interpreter and its Cellar/opt dylibs are readable.
- file-map-executable: add /opt/homebrew and /usr/local so those dylibs can load.
- file-read-metadata on path-ancestors of /opt/homebrew and /usr/local: realpath()
/ getcwd() lstat every ancestor (e.g. /opt), which the subpath read rule does not
cover — without it python3 fails at startup with 'realpath: /opt/homebrew/bin/:
Operation not permitted' while node (exec only) works. Root cause confirmed by
reproducing under the real profile via sandbox-exec.
Writes stay confined to the workspace; exec was already allowed (allow process*).
Binding a listening socket (e.g. python3 -m http.server) remains blocked by the
network policy (deny network*) — that is a separate gate. Local build for manual
verification.
|
||
|
KRATOS
|
3c2cd1d491 |
fix(sandbox): stop the interactive guard from blocking 'node --version' & friends (#294)
DetectInteractiveCommand treated node/python/ruby/... as interactive REPLs unless an argument was a script file or one of a small allow-list of eval/print flags. So 'node --version' (and -v, --check, --help; 'python3 --version', etc.) — all print-and-exit, non-interactive — were wrongly blocked before execution with "Blocked interactive command 'node': node with no script". Add universal print-and-exit flags (--version/--help) that suppress the guard for every repl program, plus node's unambiguous short forms (-v, --check, -h). Kept conservative: --version/--help only universally (e.g. 'mysql -v' is *verbose*, not version, so it still opens a prompt — covered by a new test). Bare 'node'/'python' still blocked. Tests: the exact 'node --version && npm --version' is now allowed; mysql -v stays blocked. |
||
|
Vasanth T
|
990e3c439b |
fix(sandbox): self-dispatch the Windows sandbox helpers so they work in dev (#280)
Zero already had a complete Windows sandbox (restricted token + workspace ACLs + WFP network filters), but it was unreachable in dev and plain go build: the backend was only selected when the standalone helper exes were found adjacent to the binary or on PATH (release layout only), so every bash command hard-failed with 'Windows sandbox command runner is not available'. Add self-dispatch: the running zero binary acts as its own helper via two hidden subcommands, with three-tier discovery (adjacent exe, PATH, then self-dispatch via os.Executable) mirroring the Linux fallback. Also surface the elevation requirement for 'zero sandbox setup' (WFP/ACL need Administrator) instead of a raw ACCESS_DENIED. Tests cover the three discovery tiers, dispatch routing, and the unavailable edge. |
||
|
Anandan
|
f5038151c8 |
Add managed background terminals (#270)
* Add managed background terminal sessions * Stop background terminals on TUI exit * Cover approved local network sandbox mode * Polish background terminal rendering * Tighten exec session cleanup guidance * Address background terminal review feedback * Kill exec session trees on Windows * Trigger CI for Windows cleanup |