Skip to content

Improve sandbox write-path handling - #58283

Merged
rtfeldman merged 3 commits into
mainfrom
harden-sandbox-write-path-handling
Jun 2, 2026
Merged

Improve sandbox write-path handling#58283
rtfeldman merged 3 commits into
mainfrom
harden-sandbox-write-path-handling

Conversation

@rtfeldman

Copy link
Copy Markdown
Contributor

Follow-up hardening on top of #57972 (granular sandbox write permissions), based on a review of that branch. The most important fix is that model-requested fs_write_paths were joined but never normalized, so a path containing .. could pass the lexical subtree-containment checks while seatbelt canonicalized it to somewhere else entirely — causing skipped approval prompts and silent runtime write denials. Write paths are now lexically normalized (via util::paths::normalize_lexically) at the point they enter the system, both for model requests and for hand-edited persistent grants, so the containment check, the approval prompt, and the enforced sandbox policy all operate on the same path.

The subtree insert/prune and containment logic had drifted into roughly five near-identical copies across agent, agent_settings, and settings_content; these now share util::paths::insert_subtree and path_within_subtree. The dead SandboxPermissions::covers (only ever exercised by its own tests, duplicating the production covers_with_persistent) is removed, and its tests rewritten to cover the real settings-compilation path including .. normalization.

The remaining changes reduce brittleness: the sandbox permission option ids (allow, allow_thread, allow_always, deny) are now shared constants in acp_thread instead of bare string literals scattered across the agent and UI, persist_sandbox_always_permission logs instead of silently doing nothing when no filesystem is available, and a comment documents why replay always resolves the terminal tool to the non-sandboxed variant.

Release Notes:

  • N/A

@rtfeldman rtfeldman self-assigned this Jun 2, 2026
@cla-bot cla-bot Bot added the cla-signed The user has signed the Contributor License Agreement label Jun 2, 2026
@zed-community-bot zed-community-bot Bot added the staff Pull requests authored by a current member of Zed staff label Jun 2, 2026
Base automatically changed from add-granular-permissions-for-sandbox to main June 2, 2026 01:09
@rtfeldman
rtfeldman force-pushed the harden-sandbox-write-path-handling branch from 4c0694b to 50e6a22 Compare June 2, 2026 01:13
@rtfeldman rtfeldman changed the title Harden sandbox write-path handling and de-duplicate subtree logic Improve sandbox write-path handling Jun 2, 2026
@rtfeldman
rtfeldman marked this pull request as ready for review June 2, 2026 15:11
MartinYe1234
MartinYe1234 approved these changes Jun 2, 2026
@rtfeldman
rtfeldman added this pull request to the merge queue Jun 2, 2026
Merged via the queue into main with commit 89cac49 Jun 2, 2026
32 checks passed
@rtfeldman
rtfeldman deleted the harden-sandbox-write-path-handling branch June 2, 2026 18:18
dandv pushed a commit to dandv/zed that referenced this pull request Jun 3, 2026
Follow-up hardening on top of zed-industries#57972 (granular sandbox write
permissions), based on a review of that branch. The most important fix
is that model-requested `fs_write_paths` were joined but never
normalized, so a path containing `..` could pass the lexical
subtree-containment checks while seatbelt canonicalized it to somewhere
else entirely — causing skipped approval prompts and silent runtime
write denials. Write paths are now lexically normalized (via
`util::paths::normalize_lexically`) at the point they enter the system,
both for model requests and for hand-edited persistent grants, so the
containment check, the approval prompt, and the enforced sandbox policy
all operate on the same path.

The subtree insert/prune and containment logic had drifted into roughly
five near-identical copies across `agent`, `agent_settings`, and
`settings_content`; these now share `util::paths::insert_subtree` and
`path_within_subtree`. The dead `SandboxPermissions::covers` (only ever
exercised by its own tests, duplicating the production
`covers_with_persistent`) is removed, and its tests rewritten to cover
the real settings-compilation path including `..` normalization.

The remaining changes reduce brittleness: the sandbox permission option
ids (`allow`, `allow_thread`, `allow_always`, `deny`) are now shared
constants in `acp_thread` instead of bare string literals scattered
across the agent and UI, `persist_sandbox_always_permission` logs
instead of silently doing nothing when no filesystem is available, and a
comment documents why replay always resolves the terminal tool to the
non-sandboxed variant.

Release Notes:

- N/A

---------

Co-authored-by: Martin Ye <martin@zed.dev>
Co-authored-by: MartinYe1234 <52641447+MartinYe1234@users.noreply.github.com>
TomPlanche pushed a commit to TomPlanche/zed that referenced this pull request Jun 8, 2026
Follow-up hardening on top of zed-industries#57972 (granular sandbox write
permissions), based on a review of that branch. The most important fix
is that model-requested `fs_write_paths` were joined but never
normalized, so a path containing `..` could pass the lexical
subtree-containment checks while seatbelt canonicalized it to somewhere
else entirely — causing skipped approval prompts and silent runtime
write denials. Write paths are now lexically normalized (via
`util::paths::normalize_lexically`) at the point they enter the system,
both for model requests and for hand-edited persistent grants, so the
containment check, the approval prompt, and the enforced sandbox policy
all operate on the same path.

The subtree insert/prune and containment logic had drifted into roughly
five near-identical copies across `agent`, `agent_settings`, and
`settings_content`; these now share `util::paths::insert_subtree` and
`path_within_subtree`. The dead `SandboxPermissions::covers` (only ever
exercised by its own tests, duplicating the production
`covers_with_persistent`) is removed, and its tests rewritten to cover
the real settings-compilation path including `..` normalization.

The remaining changes reduce brittleness: the sandbox permission option
ids (`allow`, `allow_thread`, `allow_always`, `deny`) are now shared
constants in `acp_thread` instead of bare string literals scattered
across the agent and UI, `persist_sandbox_always_permission` logs
instead of silently doing nothing when no filesystem is available, and a
comment documents why replay always resolves the terminal tool to the
non-sandboxed variant.

Release Notes:

- N/A

---------

Co-authored-by: Martin Ye <martin@zed.dev>
Co-authored-by: MartinYe1234 <52641447+MartinYe1234@users.noreply.github.com>
This was referenced Jun 18, 2026
jonx pushed a commit to jonx/zed-aros that referenced this pull request Jul 17, 2026
Follow-up hardening on top of zed-industries#57972 (granular sandbox write
permissions), based on a review of that branch. The most important fix
is that model-requested `fs_write_paths` were joined but never
normalized, so a path containing `..` could pass the lexical
subtree-containment checks while seatbelt canonicalized it to somewhere
else entirely — causing skipped approval prompts and silent runtime
write denials. Write paths are now lexically normalized (via
`util::paths::normalize_lexically`) at the point they enter the system,
both for model requests and for hand-edited persistent grants, so the
containment check, the approval prompt, and the enforced sandbox policy
all operate on the same path.

The subtree insert/prune and containment logic had drifted into roughly
five near-identical copies across `agent`, `agent_settings`, and
`settings_content`; these now share `util::paths::insert_subtree` and
`path_within_subtree`. The dead `SandboxPermissions::covers` (only ever
exercised by its own tests, duplicating the production
`covers_with_persistent`) is removed, and its tests rewritten to cover
the real settings-compilation path including `..` normalization.

The remaining changes reduce brittleness: the sandbox permission option
ids (`allow`, `allow_thread`, `allow_always`, `deny`) are now shared
constants in `acp_thread` instead of bare string literals scattered
across the agent and UI, `persist_sandbox_always_permission` logs
instead of silently doing nothing when no filesystem is available, and a
comment documents why replay always resolves the terminal tool to the
non-sandboxed variant.

Release Notes:

- N/A

---------

Co-authored-by: Martin Ye <martin@zed.dev>
Co-authored-by: MartinYe1234 <52641447+MartinYe1234@users.noreply.github.com>
jolutz pushed a commit to jolutz/zed that referenced this pull request Aug 8, 2026
Follow-up hardening on top of zed-industries#57972 (granular sandbox write
permissions), based on a review of that branch. The most important fix
is that model-requested `fs_write_paths` were joined but never
normalized, so a path containing `..` could pass the lexical
subtree-containment checks while seatbelt canonicalized it to somewhere
else entirely — causing skipped approval prompts and silent runtime
write denials. Write paths are now lexically normalized (via
`util::paths::normalize_lexically`) at the point they enter the system,
both for model requests and for hand-edited persistent grants, so the
containment check, the approval prompt, and the enforced sandbox policy
all operate on the same path.

The subtree insert/prune and containment logic had drifted into roughly
five near-identical copies across `agent`, `agent_settings`, and
`settings_content`; these now share `util::paths::insert_subtree` and
`path_within_subtree`. The dead `SandboxPermissions::covers` (only ever
exercised by its own tests, duplicating the production
`covers_with_persistent`) is removed, and its tests rewritten to cover
the real settings-compilation path including `..` normalization.

The remaining changes reduce brittleness: the sandbox permission option
ids (`allow`, `allow_thread`, `allow_always`, `deny`) are now shared
constants in `acp_thread` instead of bare string literals scattered
across the agent and UI, `persist_sandbox_always_permission` logs
instead of silently doing nothing when no filesystem is available, and a
comment documents why replay always resolves the terminal tool to the
non-sandboxed variant.

Release Notes:

- N/A

---------

Co-authored-by: Martin Ye <martin@zed.dev>
Co-authored-by: MartinYe1234 <52641447+MartinYe1234@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The user has signed the Contributor License Agreement staff Pull requests authored by a current member of Zed staff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants