fix(tools): unblock non-interactive shell + webhook provenance + autonomous filesystem zone (#1244)#1250
Merged
Aaronontheweb merged 9 commits intoMay 31, 2026
Conversation
… provenance (netclaw-dev#1244) Non-interactive channels (Headless/Reminder/Webhook) always failed shell with shell_no_trust_zone_roots. The shell trust zone resolved Personal's WriteFiles.Mode==All to an empty roots list and denied, while file tools read the same Mode.All as allow-all — a semantic divergence inside ScopedFileAccessPolicy. Because shell is Personal-only, the gate could only ever deny. Part A: IShellTrustZonePolicy now authorizes a write path by delegating to ScopedFileAccessPolicy.TryResolveWritePath, so shell and file_write share one Mode.All/Roots/None interpretation (incl. symlink defense). EnforceShellTrustZones validates each path token and the working directory through it; the always-deny shell_no_trust_zone_roots path is removed. Hard-deny, protected-path (ToolPathPolicy), Personal-only, and approval gates are unchanged. Part B: SetWebhookTool inherits the creating context's audience when omitted (instead of hard-defaulting to Public) and rejects minting above creator authority, mirroring the reminder provenance and downgrade-only escalation guard. Config-defined routes keep Public. Includes the OpenSpec change (tool-approval-gates + inbound-webhooks spec deltas), netclaw-operations skill guidance, and tests (netclaw-dev#1244 regression + webhook provenance).
change Code review of the shipped fix found that unifying shell with the file-access policy (Mode.All authorizes any path for Personal) combines with the pre-existing safe-verb short-circuit — which auto-approves a read-only verb on cwd alone and never inspects path arguments — to let an unattended session processing a hostile payload (a webhook) auto-read arbitrary out-of-zone files (e.g. ~/.ssh/id_rsa). Capture the chosen defense in the OpenSpec change: autonomous (non-interactive) channels are confined to a filesystem zone (session + project + operator-configured roots) across all filesystem tools, regardless of audience. The clamp keys on SupportsInteractiveApproval (the missing human backstop), lives at a single seam in ScopedFileAccessPolicy.TryResolvePath, narrows-never-widens, and is non-empty by construction (the corrected form of the original trust-zone roots bug). Adds a new audience-context-filtering spec delta, the design rationale (incl. rejected taint-gating and verb-narrowing alternatives), and task group 3.
…w-dev#1244) Review follow-up. Unifying shell with the file-access policy (Mode.All authorizes any path for Personal) combined with the pre-existing safe-verb short-circuit — which auto-approves a read-only verb on the cwd alone, never inspecting path arguments — let an unattended session processing a hostile payload (a webhook) auto-read arbitrary out-of-zone files (e.g. ~/.ssh/id_rsa). Confine autonomous (non-interactive) channels to a filesystem zone regardless of audience: session dir + current project dir + the workspaces directory (operator-configurable via the existing Workspaces:Directory; projects nest under it), plus the non-sensitive global read roots for reads. The zone is derived from NetclawPaths rather than a new config knob. Reads and writes share the single seam at ScopedFileAccessPolicy.TryResolvePath's Mode.All short-circuit, so shell (via TryResolveWritePath) and every file tool are confined together — confining only shell would move the vector to file_read. The clamp keys on SupportsInteractiveApproval (the missing human backstop), narrows-never-widens (Mode.Roots/None audiences keep their existing, already-tighter behavior; autonomous Public stays session-scoped), and fails closed on an empty zone. Interactive sessions are unchanged. Updates the OpenSpec design/spec delta/tasks to match.
Aaronontheweb
added
bug
…bing (netclaw-dev#1244) Review feedback: the previous approach anchored the zone on NetclawPaths.WorkspacesDirectory, which isn't uniformly available — file_write/file_edit/attach/set_working_directory construct ScopedFileAccessPolicy without paths — so it would have required threading paths through four tool constructors and left the zone inconsistent (shell got workspaces, file_write didn't). Source the zone from data already at the seam instead: session_dir + project_dir (both already on ToolExecutionContext) for write/attach, plus the existing _cachedGlobalReadRoots for reads. An autonomous session can read across the project tree but write only within its session or current project. Same exfil protection, tighter writes, and no new accessor, constructors, or DI threading. Removes ToolAudienceProfileResolver.WorkspacesDirectory; updates the spec delta/design/tasks and AutonomousZoneClampTests (read/write split).
Twice in netclaw-dev#1244 the implementation reached for a new construct (a config knob, then a constructor parameter threaded through four tools) to feed a design, when the data needed was already flowing through the seam being edited (NetclawPaths, then ToolExecutionContext). Codify the lesson in the Universal Quality Bar: check existing constructs first, anchor on data already present at the call site, and treat a parallel new construct as a defect unless nothing existing fits.
…ntext fallback Two reviewer-facing source comments capturing 'why would this surprise me?' decisions: (1) ShellTrustZonePolicy checks shell paths against the stricter WRITE rules even for read-only commands — deliberately over-cautious vs file_read on the global read roots, a known capability gap not a security hole; (2) SetWebhookTool's context-less Empty fallback resolves to Public (the floor), so a missing context can only reduce a webhook's privilege, never escalate it.
Aaronontheweb
commented
May 31, 2026
Collaborator
Author
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
A few inline notes explaining the non-obvious decisions in this change, in plain language for anyone reviewing the security model later.
| // blanket grant — the live approval gate is their backstop. This is the | ||
| // single seam that covers shell (via TryResolveWritePath) and every file | ||
| // tool at once. | ||
| if (context.SupportsInteractiveApproval == false) |
Collaborator
Author
There was a problem hiding this comment.
This one line is the whole fix. When a session has a human who can approve things (Slack, terminal), we let it touch any file — the person is the safety check. When there's no human (a reminder firing, a webhook arriving), we instead box it into a small set of allowed folders. The split is on "is a human watching," not on how trusted the session is.
| out string error) | ||
| { | ||
| var zone = ResolveAutonomousZone(context, accessKind); | ||
| if (zone.Count == 0) |
Collaborator
Author
There was a problem hiding this comment.
Deliberately fail shut: if we somehow end up with no allowed folders at all, deny everything rather than fall back to "allow." A locked door with no key beats a door that opens when the lock is missing. In practice the session folder is always present, so this is a just-in-case guard.
| if (!string.IsNullOrWhiteSpace(context.ProjectDirectory)) | ||
| roots.Add(context.ProjectDirectory); | ||
|
|
||
| if (accessKind == AccessKind.Read) |
Collaborator
Author
There was a problem hiding this comment.
Reading is allowed a bit wider than writing on purpose. An unattended session can read shared things like its own skills and identity files, but can only write inside its session/project folders — so it can consult its instructions but not rewrite them mid-task.
| // allow it (the read zone includes the global read roots, the write zone does | ||
| // not). This is a known capability gap, not a security hole; making the check | ||
| // verb-aware (read-verbs against the read zone) is a possible follow-up. | ||
| public bool IsShellWritePathAuthorized(string fullPath, ToolExecutionContext context) |
Collaborator
Author
There was a problem hiding this comment.
We check shell file paths against the stricter write rules even when the command is only reading. A shell command can write, so we treat it as write-level to be safe. Trade-off: a shell cat of a skills file gets denied even though the read-only file_read tool would allow it. That's intentionally over-cautious, not a security hole — being read/write-smart here is a possible follow-up. (Captured as a source comment just above this line.)
| // escalation guard in TryResolveAudience is keyed off creatorAudience, and | ||
| // Public is the floor. | ||
| protected override Task<string> ExecuteAsync(Params args, CancellationToken ct) | ||
| => ExecuteAsync(args, ToolExecutionContext.Empty, ct); |
Collaborator
Author
There was a problem hiding this comment.
If this tool is ever called without a session context, it falls back to the lowest-privilege identity (Public). So a missing context can only make a webhook less powerful, never accidentally grant it more. (Captured as a source comment just above this line.)
| return false; | ||
| } | ||
|
|
||
| if (parsed > creatorAudience) |
Collaborator
Author
There was a problem hiding this comment.
A webhook can't be created with more privilege than whoever created it. You can dial a webhook down (e.g. a trusted user makes a locked-down public one), but never up. Same rule reminders already use.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1244.
Problem
All non-interactive channels —
Headless(netclaw chat -p),Reminder, andWebhook(SupportsInteractiveApproval == false) — failed every shell command withshell_no_trust_zone_roots, even for pre-approved verbs.Root cause was a semantic divergence inside
ScopedFileAccessPolicy: file tools readWriteFiles.Mode == Allas allow-all, while the non-interactive shell "trust zone" read the sameMode == Allas deny-all (it resolved to an empty roots list). Since shell is Personal-only and Personal isMode == All, the gate was unreachable-success code — it could only ever deny.What changed
A — Unify the shell path check with file-access semantics (closes #1244). The non-interactive shell path validation now delegates to
ScopedFileAccessPolicy.TryResolveWritePath, so shell andfile_writeshare one interpretation ofMode.All/Roots/None(and one symlink defense). The always-denyshell_no_trust_zone_rootspath is removed. Hard-deny, protected-path (ToolPathPolicy), Personal-only, and approval gates are unchanged.B — Webhook audience provenance.
set_webhooknow inherits the creating context's audience when omitted (instead of hard-defaulting toPublic) and rejects minting above the creator's authority (downgrade-only), mirroring the reminder provenance/escalation model. File-defined config routes keepPublicas the fail-closed default.C — Autonomous filesystem zone (defensive clamp). A self-review found that the unified
Mode.Allpath authorization combined with the pre-existing safe-verb short-circuit — which auto-approves a read-only verb on the cwd alone and never inspects path arguments — could let an unattended session processing a hostile payload (a webhook) auto-read arbitrary out-of-zone files (e.g.~/.ssh/id_rsa). So autonomous (non-interactive) channels are now confined to a filesystem zone regardless of audience:ScopedFileAccessPolicy.TryResolvePath'sMode.Allshort-circuit, so shell (viaTryResolveWritePath) and every file tool (file_read/file_write/file_edit/file_list/attach_file) are confined together — confining only shell would move the vector tofile_read.session_dir+ currentproject_dir+WorkspacesDirectory(operator-configurable viaWorkspaces:Directory; projects nest under it), plus the non-sensitive global read roots for reads. No new config knob.Mode.Roots/Nonekeep existing behavior), keys onSupportsInteractiveApproval(the missing human backstop), and fails closed on an empty zone. Interactive sessions are unaffected.Design notes
The decision record lives in the OpenSpec change
fix-noninteractive-shell-and-webhook-provenance(proposal/design/specs/tasks), including the rejected alternatives: deleting the trust-zone mechanism, taint-gating (needs plumbing not yet onToolExecutionContext), and narrowing the safe-verb list (bricks unattended capability and pushes operators to the broadertrust-verb). Spec deltas touchtool-approval-gates,inbound-webhooks, andaudience-context-filtering.Testing
dotnet build Netclaw.slnxclean; 2137Netclaw.Actors.Testspass;dotnet slopwatch analyze0 issues; copyright headers verified.AutonomousZoneClampTests(denied outside zone for read and write; allowed in session/project/workspaces; Public not widened; interactive unrestricted; empty zone fails closed),SetWebhookToolProvenanceTests(inherit / downgrade / upscope-reject), plus an updated Bug: Non-interactive channels always fail withshell_no_trust_zone_roots#1244 regression inToolApprovalGateTests.netclaw-operationssystem skill updated (set_webhook audience inheritance; corrected stale "trust zone path" guidance), version bumped.Follow-ups (not in this PR)
sync/archiveafter merge.