diff --git a/internal/acp/enforcement_notice_test.go b/internal/acp/enforcement_notice_test.go new file mode 100644 index 000000000..6a26fb7c0 --- /dev/null +++ b/internal/acp/enforcement_notice_test.go @@ -0,0 +1,65 @@ +package acp + +import ( + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/tools" +) + +// AN ACP CLIENT MUST SEE THE DISCLOSURE THE TUI SEES. +// +// agent.ToolResult stores the UNDECORATED model text alongside the typed +// enforcement notices; ModelOutput is what composes them. Reading .Output +// directly compiles and looks right, and silently drops the notice for every +// ACP client, which is the one surface with no other way to learn the sandbox +// narrowed what the command could do. +func TestToolResultContentCarriesTheEnforcementNotice(t *testing.T) { + const notice = "least-privilege notice: read access was narrowed" + result := agent.ToolResult{ + Name: "bash", + Status: tools.StatusOK, + Output: "the command output", + EnforcementNotices: []string{notice}, + } + + content := toolResultContent(result) + if len(content) == 0 { + t.Fatal("no content produced for a successful tool result") + } + var text strings.Builder + for _, part := range content { + if part.Content != nil { + text.WriteString(part.Content.Text) + } + } + got := text.String() + + if count := strings.Count(got, notice); count != 1 { + t.Errorf("the notice appears %d times, want exactly 1:\n%s", count, got) + } + if !strings.Contains(got, "the command output") { + t.Errorf("the underlying output was lost:\n%s", got) + } +} + +// And a result with no notice is unchanged, so the accessor is not adding +// anything to ordinary output. +func TestToolResultContentLeavesAnOrdinaryResultAlone(t *testing.T) { + result := agent.ToolResult{ + Name: "bash", + Status: tools.StatusOK, + Output: "plain output", + } + content := toolResultContent(result) + if len(content) == 0 { + t.Fatal("no content produced") + } + if content[0].Content == nil { + t.Fatal("content block missing") + } + if got := content[0].Content.Text; got != "plain output" { + t.Errorf("ordinary output = %q, want it untouched", got) + } +} diff --git a/internal/acp/translate.go b/internal/acp/translate.go index 565174904..27097ff5d 100644 --- a/internal/acp/translate.go +++ b/internal/acp/translate.go @@ -120,7 +120,11 @@ func toolCallResult(result agent.ToolResult) ToolCallUpdate { } func toolResultContent(result agent.ToolResult) []ToolCallContent { - text := strings.TrimRight(result.Output, "\n") + // ModelOutput, not the raw field. agent.ToolResult stores the undecorated + // model text alongside the typed enforcement notices, and the accessor is + // what composes the two; reading Output directly sends an ACP client the + // output with the disclosure missing. + text := strings.TrimRight(result.ModelOutput(), "\n") if text == "" { text = result.Display.Summary } diff --git a/internal/agent/before_tool_delivery_test.go b/internal/agent/before_tool_delivery_test.go new file mode 100644 index 000000000..6eabdbe7d --- /dev/null +++ b/internal/agent/before_tool_delivery_test.go @@ -0,0 +1,210 @@ +package agent + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/execution" + "github.com/Gitlawb/zero/internal/hooks" + "github.com/Gitlawb/zero/internal/tools" + zeroruntime "github.com/Gitlawb/zero/internal/zeroruntime" +) + +const beforeToolNotice = "denyRead is configured, so the write jail is not confining writes" + +// beforeToolChatter is what a hook prints for its own reasons. It must never +// reach the model: main is silent for a successful hook, and a hook that logs is +// not asking to be heard by anything but the operator's terminal. +const beforeToolChatter = "hook-ran-and-logged-this" + +// noticeHookPreparer plans the hook command with an enforcement notice attached, +// the way the sandbox does for a command it weakened. The prepared child prints +// ordinary output as well, so one run carries both kinds of text and the +// delivery decision has to tell them apart. +type noticeHookPreparer struct{} + +func (noticeHookPreparer) PrepareExecution(_ context.Context, _ execution.Request) (execution.PreparedCommand, error) { + var command *exec.Cmd + if runtime.GOOS == "windows" { + command = exec.Command("cmd.exe", "/c", "echo "+beforeToolChatter) + } else { + command = exec.Command("/bin/sh", "-c", "echo "+beforeToolChatter) + } + return execution.PreparedCommand{ + Command: command, + Enforcement: execution.Enforcement{Notices: []string{beforeToolNotice}}, + }, nil +} + +func beforeToolDispatcher(t *testing.T, event hooks.Event, exitCode int) *hooks.Dispatcher { + t.Helper() + audit, err := hooks.NewAuditStore(hooks.AuditStoreOptions{AuditPath: filepath.Join(t.TempDir(), "audit.jsonl")}) + if err != nil { + t.Fatalf("NewAuditStore: %v", err) + } + return hooks.NewDispatcher(hooks.DispatcherOptions{ + Config: hooks.Config{ + Enabled: true, + Hooks: []hooks.Definition{ + {ID: "zero.before-tool", Event: event, Matcher: "read_file", Command: "hook", Enabled: true}, + }, + }, + Audit: audit, + Cwd: t.TempDir(), + Execution: execution.NewRunner(noticeHookPreparer{}), + }) +} + +func readFileRunOptions(t *testing.T, dispatcher *hooks.Dispatcher) (Options, *mockProvider, string) { + t.Helper() + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "notes.txt"), []byte("hello"), 0o644); err != nil { + t.Fatalf("write notes.txt: %v", err) + } + registry := tools.NewRegistry() + registry.Register(tools.NewReadFileTool(root)) + provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: "read_file"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{"path":"notes.txt"}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "read it"}, + {Type: zeroruntime.StreamEventDone}, + }, + }} + return Options{ + SessionID: "session-hook", + Cwd: root, + Registry: registry, + ProviderName: "test-provider", + Model: "test-model", + Hooks: dispatcher, + MaxTurns: 2, + }, provider, root +} + +// countRequestsContaining reports how many provider requests carry needle, so a +// notice delivered twice is distinguishable from one delivered once. +func countRequestsContaining(requests []zeroruntime.CompletionRequest, needle string) int { + total := 0 + for _, request := range requests { + for _, message := range request.Messages { + if strings.Contains(message.Content, needle) { + total++ + } + } + } + return total +} + +// THE NOTICE CROSSES TO THE MODEL. THE HOOK'S OWN OUTPUT DOES NOT. +// +// executeToolCall used to read the beforeTool outcome only when Blocked was +// true, so a hook that ran under the weakened DenyRead token said so to nobody. +// Delivering DispatchOutcome.Messages fixed that and overshot: hookMessage folds +// the notice together with the hook's ordinary stdout, so every successful +// hook's routine logging became a standing input channel into the next model +// request, which is not what main does. +// +// One hook run produces both kinds of text here, because the bug is exactly a +// failure to tell them apart. Asserted on what the PROVIDER received, since that +// is the boundary that matters; a unit test on the joining helper cannot see +// which slice the loop passes it. +func TestSuccessfulBeforeToolHookDeliversItsNoticeAndNotItsOutput(t *testing.T) { + options, provider, _ := readFileRunOptions(t, beforeToolDispatcher(t, hooks.EventBeforeTool, 0)) + if _, err := Run(context.Background(), "read the notes", provider, options); err != nil { + t.Fatalf("Run: %v", err) + } + + // The tool ran, so this is the successful-hook path rather than a blocked + // call that never reached the tool. + if !someRequestContains(provider.requests, "hello") { + t.Fatal("SETUP INVALID: the tool result never reached the model, so nothing was delivered to check") + } + // And the hook really did run and really did print, or the silence asserted + // below would be the silence of a hook that never executed. + if !someRequestContains(provider.requests, beforeToolNotice) { + t.Fatal("the enforcement notice never reached the model, so a hook could run under the weakened token and say so to nobody") + } + if got := countRequestsContaining(provider.requests, beforeToolNotice); got != 1 { + t.Errorf("the notice reached the model %d times, want exactly once", got) + } + if someRequestContains(provider.requests, beforeToolChatter) { + t.Error("the hook's ordinary output reached the model; main is silent for a successful hook and routine logging must not become model input") + } +} + +// A VETO MUST NOT SWALLOW A NOTICE FROM A HOOK THAT ALREADY RAN. +// +// Dispatch runs hooks in order and returns at the first veto. The successful +// hook ahead of it may already have run under the weakened token, and that is a +// fact about something that happened. The veto result used to be built from the +// blocking hook's Reason alone, so the earlier disclosure existed only in the +// audit record. +func TestABlockedCallStillCarriesTheEarlierHooksNotice(t *testing.T) { + audit, err := hooks.NewAuditStore(hooks.AuditStoreOptions{AuditPath: filepath.Join(t.TempDir(), "audit.jsonl")}) + if err != nil { + t.Fatalf("NewAuditStore: %v", err) + } + dispatcher := hooks.NewDispatcher(hooks.DispatcherOptions{ + Config: hooks.Config{ + Enabled: true, + Hooks: []hooks.Definition{ + {ID: "zero.first", Event: hooks.EventBeforeTool, Matcher: "read_file", Command: "hook", Enabled: true}, + {ID: "zero.veto", Event: hooks.EventBeforeTool, Matcher: "read_file", Command: "veto", Enabled: true}, + }, + }, + Audit: audit, + Cwd: t.TempDir(), + Execution: execution.NewRunner(vetoSecondPreparer{}), + }) + options, provider, _ := readFileRunOptions(t, dispatcher) + if _, err := Run(context.Background(), "read the notes", provider, options); err != nil { + t.Fatalf("Run: %v", err) + } + + // SETUP: the second hook really did veto, or this is the ordinary path. + if !someRequestContains(provider.requests, "was blocked by hook") { + t.Fatal("SETUP INVALID: the call was not blocked, so the veto path is not under test") + } + if !someRequestContains(provider.requests, beforeToolNotice) { + t.Error("the veto result dropped the notice from the hook that had already run under the weakened token") + } + if got := countRequestsContaining(provider.requests, beforeToolNotice); got != 1 { + t.Errorf("the notice reached the model %d times, want exactly once", got) + } + if someRequestContains(provider.requests, beforeToolChatter) { + t.Error("the vetoed result carried the earlier hook's ordinary output") + } +} + +// vetoSecondPreparer runs the first hook successfully with a notice and makes +// the second one exit non-zero, which is a veto for a blocking event. +type vetoSecondPreparer struct{} + +func (vetoSecondPreparer) PrepareExecution(_ context.Context, request execution.Request) (execution.PreparedCommand, error) { + script := "echo " + beforeToolChatter + notices := []string{beforeToolNotice} + if request.Command.Name == "veto" { + script = "exit 2" + notices = nil + } + var command *exec.Cmd + if runtime.GOOS == "windows" { + command = exec.Command("cmd.exe", "/c", script) + } else { + command = exec.Command("/bin/sh", "-c", script) + } + return execution.PreparedCommand{ + Command: command, + Enforcement: execution.Enforcement{Notices: notices}, + }, nil +} diff --git a/internal/agent/enforcement_notice_projection_test.go b/internal/agent/enforcement_notice_projection_test.go new file mode 100644 index 000000000..43f7646cf --- /dev/null +++ b/internal/agent/enforcement_notice_projection_test.go @@ -0,0 +1,114 @@ +package agent + +import ( + "context" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/tools" +) + +// noticeProjectionTool stands in for any sandboxed command tool: it reports the +// enforcement disclosure the way the real ones do, through the sandbox metadata +// key that finalizeToolOutcome promotes into the typed notice slice. +type noticeProjectionTool struct{} + +func (noticeProjectionTool) Name() string { return "notice_projection" } +func (noticeProjectionTool) Description() string { return "test tool carrying an enforcement notice" } +func (noticeProjectionTool) Parameters() tools.Schema { return tools.Schema{Type: "object"} } +func (noticeProjectionTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectRead, Permission: tools.PermissionAllow} +} + +func (noticeProjectionTool) Run(ctx context.Context, args map[string]any) tools.Result { + return tools.Result{ + Status: tools.StatusOK, + Output: "the command output", + Display: tools.Display{Summary: "the human summary"}, + Meta: map[string]string{"sandbox_notices": "least-privilege notice"}, + } +} + +// THE PROJECTION MUST CARRY ONE REPRESENTATION, NOT TWO. +// +// executeToolCall copies the typed notice slice into agent.ToolResult, so the +// text fields it copies alongside must be the UNDECORATED base. Storing the +// already-rendered text there instead leaves the same fact in two places with no +// contract between them: it happens to render once today only because the +// outcome arrives finalized and the agent accessor then reads Outcome.ModelView +// rather than the stored field. Any result that reaches the accessor without a +// finalized outcome renders the disclosure twice, and every raw reader of +// .Output sees text that disagrees with Outcome.ModelView. +func TestEnforcementNoticeIsStoredOnceAndRenderedOnce(t *testing.T) { + const notice = "least-privilege notice" + + registry := tools.NewRegistry() + registry.Register(noticeProjectionTool{}) + + result, err := executeToolCall(context.Background(), registry, ToolCall{ + ID: "call-1", Name: "notice_projection", Arguments: `{}`, + }, PermissionModeAuto, Options{Cwd: t.TempDir()}) + if err != nil { + t.Fatalf("executeToolCall: %v", err) + } + if len(result.EnforcementNotices) == 0 { + t.Fatalf("the notice never reached the agent result: %#v", result) + } + + // The stored fields are the canonical undecorated base, and they agree with + // the finalized outcome they were projected from. + if strings.Contains(result.Output, notice) { + t.Errorf("ToolResult.Output stores the rendered notice as well as the slice: %q", result.Output) + } + if strings.Contains(result.Display.Summary, notice) { + t.Errorf("ToolResult.Display.Summary stores the rendered notice as well as the slice: %q", result.Display.Summary) + } + if result.Output != result.Outcome.ModelView { + t.Errorf("stored output %q disagrees with the finalized model view %q", result.Output, result.Outcome.ModelView) + } + if result.Display.Summary != result.Outcome.HumanView.Summary { + t.Errorf("stored summary %q disagrees with the finalized human view %q", result.Display.Summary, result.Outcome.HumanView.Summary) + } + + // And every consumer that renders goes through the accessors, which show the + // disclosure exactly once without hiding the output it is attached to. + // loop.go builds the provider transcript from ModelOutput; the CLI writer and + // the TUI cards use both accessors. + transcript := result.ModelOutput() + if got := strings.Count(transcript, notice); got != 1 { + t.Errorf("the transcript shows the notice %d times, want 1: %q", got, transcript) + } + if !strings.Contains(transcript, "the command output") { + t.Errorf("the transcript lost the command output: %q", transcript) + } + summary := result.HumanDisplay().Summary + if got := strings.Count(summary, notice); got != 1 { + t.Errorf("the human summary shows the notice %d times, want 1: %q", got, summary) + } + if !strings.Contains(summary, "the human summary") { + t.Errorf("the human summary lost the tool summary: %q", summary) + } +} + +// A result that never crossed the registry has no finalized outcome, so the +// accessor falls back to the stored field. That is the path on which a stored +// rendering would double, and it is the reason the contract above is stated on +// the stored fields rather than only on the accessors. +func TestUnfinalizedResultStillRendersTheNoticeOnce(t *testing.T) { + const notice = "least-privilege notice" + result := ToolResult{ + Status: tools.StatusOK, + Output: "the command output", + Display: tools.Display{Summary: "the human summary"}, + EnforcementNotices: []string{notice}, + } + if result.Outcome.Finalized() { + t.Fatal("fixture is finalized; it no longer covers the fallback path") + } + if got := strings.Count(result.ModelOutput(), notice); got != 1 { + t.Errorf("ModelOutput shows the notice %d times, want 1: %q", got, result.ModelOutput()) + } + if got := strings.Count(result.HumanDisplay().Summary, notice); got != 1 { + t.Errorf("HumanDisplay shows the notice %d times, want 1: %q", got, result.HumanDisplay().Summary) + } +} diff --git a/internal/agent/hook_wiring_test.go b/internal/agent/hook_wiring_test.go index cfbd93d36..06e66085d 100644 --- a/internal/agent/hook_wiring_test.go +++ b/internal/agent/hook_wiring_test.go @@ -93,3 +93,37 @@ func TestDispatchHelpersAreNoopWithoutDispatcher(t *testing.T) { t.Fatalf("a nil dispatcher must yield no feedback, got %q", feedback) } } + +// A SUCCESSFUL beforeTool HOOK'S OUTPUT MUST REACH THE MODEL, NOT ONLY THE AUDIT. +// +// executeToolCall used to read the beforeTool outcome only when Blocked was true, +// so a hook that ran fine and produced an enforcement notice — for instance that +// it ran under the weakened DenyRead token — put that notice in the audit record +// and nowhere anybody could see it. Only vetoes and afterTool feedback reached a +// surface. joinHookMessages is the delivery: beforeTool's messages ride out on +// the same tool result afterTool feedback already uses. +func TestJoinHookMessagesDeliversSuccessfulBeforeToolOutput(t *testing.T) { + const notice = "hook ran without WRITE_RESTRICTED because denyRead is configured" + + // A successful beforeTool hook alone still reaches the model. + if got := joinHookMessages([]string{notice}, ""); got != notice { + t.Fatalf("a successful beforeTool notice was dropped: %q", got) + } + // And it does not displace afterTool feedback; both arrive, in order. + got := joinHookMessages([]string{notice}, "gofmt reformatted main.go") + if !strings.Contains(got, notice) || !strings.Contains(got, "gofmt reformatted main.go") { + t.Fatalf("expected both the beforeTool notice and the afterTool feedback, got %q", got) + } + if strings.Index(got, notice) > strings.Index(got, "gofmt reformatted main.go") { + t.Fatalf("beforeTool output should precede afterTool feedback, got %q", got) + } + // Empty and whitespace-only messages contribute nothing, so a run with no hook + // output stays silent rather than appending an empty header. + if got := joinHookMessages([]string{"", " "}, ""); got != "" { + t.Fatalf("blank hook messages produced %q, want nothing", got) + } + // afterTool alone is unchanged, which is the behaviour that already worked. + if got := joinHookMessages(nil, "vet found an issue"); got != "vet found an issue" { + t.Fatalf("afterTool-only feedback changed shape: %q", got) + } +} diff --git a/internal/agent/loop.go b/internal/agent/loop.go index fe691ac4c..a896112b1 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -1414,10 +1414,28 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal } // beforeTool hooks may veto the call before it runs (a non-zero exit blocks). + // + // A SUCCESSFUL beforeTool HOOK STILL HAS SOMETHING TO SAY, BUT ONLY ITS NOTICE. + // + // Reading the outcome only when Blocked left the enforcement disclosure in the + // audit record and nowhere the model or the operator could see it: a hook could + // run under the weakened DenyRead token and say so to nobody. + // + // Notices, NOT Messages. Messages is presentation text that hookMessage builds + // by folding the notice together with the hook's ordinary stdout, so delivering + // it would put every successful hook's routine logging, large diagnostics, and + // whatever text a hook happened to process into the next model request. That is + // a behaviour change nobody asked for and a standing input channel. main is + // silent for successful hooks and stays silent here for everything except the + // disclosure. Carried to the tool result below, the same surface afterTool + // feedback already uses. + var beforeToolNotices []string if toolFound { - if outcome, blocked := dispatchBeforeTool(ctx, options, call, args); blocked { + outcome, blocked := dispatchBeforeTool(ctx, options, call, args) + if blocked { return blockedByHookResult(call, outcome), nil } + beforeToolNotices = outcome.Notices } args = shellExecutionArgsForApproval(call.Name, args, decisionAction, options) @@ -1463,7 +1481,10 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal }) if retryResult, directResult, retried, action, reason, prefix, abortErr := maybeRetryUnsandboxedAfterSandboxRestriction(ctx, registry, call, tool, args, result, permissionMode, options, progressCallback); retried || directResult != nil || abortErr != nil { if directResult != nil { - return *directResult, abortErr + // A denied, cancelled, or ungrantable retry still returns a result for a + // call whose beforeTool hook already ran. Without this the disclosure is + // produced and then dropped on the floor. + return withBeforeToolNotices(*directResult, beforeToolNotices), abortErr } result = retryResult permissionGranted = true @@ -1489,7 +1510,7 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal // afterTool hooks run once the tool has executed; their output (e.g. a // formatter or vet result) is surfaced back to the model on the result. if toolFound { - if feedback := dispatchAfterTool(ctx, options, call, args, result); feedback != "" { + if feedback := joinHookMessages(beforeToolNotices, dispatchAfterTool(ctx, options, call, args, result)); feedback != "" { var didRedact bool result.Output, didRedact = appendHookFeedback(result.Output, feedback) if didRedact { @@ -1517,20 +1538,21 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal // the agent loop and the MCP server pass through), so result.Output is // already redacted here and result.Redacted reflects whether it changed. return ToolResult{ - Risk: executedRisk, - ToolCallID: call.ID, - Name: call.Name, - Status: result.Status, - Output: result.ModelOutput(), - Truncated: result.Truncated, - Meta: result.Meta, - Images: result.Images, - Redacted: result.Redacted, - ChangedFiles: result.ChangedFiles, - ChangeSummaries: result.ChangeSummaries, - Display: result.HumanDisplay(), - Outcome: result.Outcome, - LoadedTools: loadedToolsFromResult(result.Meta), + Risk: executedRisk, + ToolCallID: call.ID, + Name: call.Name, + Status: result.Status, + Output: result.BaseModelOutput(), + Truncated: result.Truncated, + Meta: result.Meta, + EnforcementNotices: append([]string(nil), result.EnforcementNotices...), + Images: result.Images, + Redacted: result.Redacted, + ChangedFiles: result.ChangedFiles, + ChangeSummaries: result.ChangeSummaries, + Display: result.BaseDisplay(), + Outcome: result.Outcome, + LoadedTools: loadedToolsFromResult(result.Meta), // A tool may signal a mid-run model escalation by carrying the target id // in Meta["escalate_to_model"]. Lift it into the typed loop-level field; // the Run turn loop performs the actual provider switch. Empty for every @@ -1997,7 +2019,7 @@ func blockedByHookResult(call ToolCall, outcome hooks.DispatchOutcome) ToolResul reason = "blocked by a beforeTool hook" } message := fmt.Sprintf("Error: %q was blocked by hook %q: %s", call.Name, outcome.BlockedBy, reason) - return ToolResult{ + result := ToolResult{ ToolCallID: call.ID, Name: call.Name, Status: tools.StatusError, @@ -2005,12 +2027,87 @@ func blockedByHookResult(call ToolCall, outcome hooks.DispatchOutcome) ToolResul Redacted: redacted, DenialReason: DenialHookBlocked, } + // Dispatch runs hooks in order and stops at the first veto, so an earlier hook + // may already have run under a weakened token before this one said no. Its + // notice describes something that happened and has to survive the veto. + // + // blockReason has already folded the BLOCKING hook's own notices into Reason, + // which is inside message above, so those are dropped here rather than said + // twice. + return withBeforeToolNotices(result, noticesBefore(outcome)) +} + +// noticesBefore returns the accumulated notices minus the blocking hook's own, +// which blockReason has already put in the veto message. +func noticesBefore(outcome hooks.DispatchOutcome) []string { + if !outcome.Blocked { + return outcome.Notices + } + kept := make([]string, 0, len(outcome.Notices)) + for _, notice := range outcome.Notices { + if strings.Contains(outcome.Reason, strings.TrimSpace(notice)) { + continue + } + kept = append(kept, notice) + } + return kept +} + +// withBeforeToolNotices is the single place a beforeTool enforcement notice +// reaches a tool result on a path that does NOT run afterTool. +// +// The normal tail joins the notices with the afterTool feedback and delivers +// both at once. Two other exits return a result for a call whose hook already +// ran: a later hook's veto, and a denied, cancelled, or ungrantable unsandboxed +// retry. Routing all three through one function is what keeps "the hook ran +// under this token" from depending on which exit the call happened to take. +// +// NO REBUDGET HERE, AND THAT IS LOAD-BEARING ON WHAT MAY PASS THROUGH. The +// normal tail appends and then calls Registry.RebudgetAfterHook, because what it +// appends is afterTool feedback: hook stdout, which a hook can make arbitrarily +// large. These notices cannot be. Their one producer is +// sandbox.windowsDenyReadWarnings, which returns a single fixed sentence, and +// nothing hook-authored reaches this slice: DispatchOutcome.Messages is where +// hook output lives, and the capture site deliberately does not read it. +// +// So if anything ever widens what is delivered here to include text a hook or a +// tool can size, this needs the rebudget step as well, which means converting +// through tools.Result the way the tail does rather than editing Output in +// place. Do not widen it without that. +func withBeforeToolNotices(result ToolResult, notices []string) ToolResult { + feedback := joinHookMessages(notices, "") + if strings.TrimSpace(feedback) == "" { + return result + } + output, didRedact := appendHookFeedback(result.Output, feedback) + result.Output = output + if didRedact { + result.Redacted = true + } + return result } // appendHookFeedback appends afterTool hook output to a tool result's output, // scrubbed for secrets like every other string crossing the tool boundary. The // returned bool reports whether scrubbing changed the feedback, so the caller can // set ToolResult.Redacted to match the registry's redaction contract. +// joinHookMessages folds a successful beforeTool hook's enforcement notices in +// with the afterTool feedback so both reach the model through the one delivery +// path, rather than the notices being produced and then dropped. It takes the +// notices, never DispatchOutcome.Messages: see the capture site above. +func joinHookMessages(before []string, after string) string { + parts := make([]string, 0, len(before)+1) + for _, message := range before { + if strings.TrimSpace(message) != "" { + parts = append(parts, message) + } + } + if strings.TrimSpace(after) != "" { + parts = append(parts, after) + } + return strings.Join(parts, "\n\n") +} + func appendHookFeedback(output string, feedback string) (string, bool) { scrubbed := redaction.RedactString(feedback, redaction.Options{}) redacted := scrubbed != feedback @@ -2145,17 +2242,18 @@ func askUserFallbackResult(ctx context.Context, registry *tools.Registry, call T Cwd: options.Cwd, }) return ToolResult{ - ToolCallID: call.ID, - Name: call.Name, - Status: result.Status, - Output: result.ModelOutput(), - Truncated: result.Truncated, - Meta: result.Meta, - Redacted: result.Redacted, - ChangedFiles: result.ChangedFiles, - ChangeSummaries: result.ChangeSummaries, - Display: result.HumanDisplay(), - Outcome: result.Outcome, + ToolCallID: call.ID, + Name: call.Name, + Status: result.Status, + Output: result.BaseModelOutput(), + Truncated: result.Truncated, + Meta: result.Meta, + EnforcementNotices: append([]string(nil), result.EnforcementNotices...), + Redacted: result.Redacted, + ChangedFiles: result.ChangedFiles, + ChangeSummaries: result.ChangeSummaries, + Display: result.BaseDisplay(), + Outcome: result.Outcome, } } return ToolResult{ diff --git a/internal/agent/types.go b/internal/agent/types.go index 511ea7140..18d7fecf5 100644 --- a/internal/agent/types.go +++ b/internal/agent/types.go @@ -79,6 +79,10 @@ type ToolResult struct { // The full result may be recoverable through Meta["spill_path"]. Truncated bool Meta map[string]string + // EnforcementNotices mirrors tools.Result.EnforcementNotices so the + // disclosure survives the conversion into the agent-facing result and + // reaches the model, the transcript and the interactive display. + EnforcementNotices []string // Images the tool produced, delivered to the model as a following user // message rather than on this result. See tools.Result.Images. Images []zeroruntime.ImageBlock @@ -109,24 +113,42 @@ type ToolResult struct { RequestedModel string } -// ModelOutput returns the bounded provider-facing result while preserving -// compatibility with synthetic and restored results created before outcomes -// were finalized. -func (result ToolResult) ModelOutput() string { +// BaseModelOutput is the bounded provider-facing result WITHOUT the enforcement +// disclosure composed into it, mirroring tools.Result.BaseModelOutput. +// +// A surface that renders the typed EnforcementNotices itself must build its body +// from here, or the disclosure appears twice. Decoration has exactly one owner +// per surface: either the text carries it or the surface draws it, never both. +func (result ToolResult) BaseModelOutput() string { if result.Outcome.Finalized() { return result.Outcome.ModelView } return result.Output } -// HumanDisplay returns the presentation intended for interactive surfaces. -func (result ToolResult) HumanDisplay() tools.Display { +// BaseDisplay is BaseModelOutput's presentation half, and carries no enforcement +// notices for the same reason. +func (result ToolResult) BaseDisplay() tools.Display { if result.Outcome.Finalized() { return result.Outcome.HumanView } return result.Display } +// ModelOutput returns the bounded provider-facing result while preserving +// compatibility with synthetic and restored results created before outcomes +// were finalized. +func (result ToolResult) ModelOutput() string { + return tools.WithEnforcementNotices(result.BaseModelOutput(), result.EnforcementNotices) +} + +// HumanDisplay returns the presentation intended for interactive surfaces. +func (result ToolResult) HumanDisplay() tools.Display { + display := result.BaseDisplay() + display.Summary = tools.WithEnforcementNotices(display.Summary, result.EnforcementNotices) + return display +} + // DenialCategory classifies why a tool call was blocked before it executed. type DenialCategory string diff --git a/internal/cli/app.go b/internal/cli/app.go index 44fa370ff..0fd7442fa 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -867,6 +867,19 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a } fmt.Fprintf(stderr, "warning: MCP server %s unavailable, skipped: %s\n", skipped.Name, redaction.ErrorMessage(skipped.Err, redaction.Options{})) } + // AND WHAT THE SERVERS THAT DID START RAN UNDER. A stdio MCP server prepared + // with a weakened write jail serves the whole session from that process, so + // the disclosure is about startup and no later tool result can carry it. Said + // once, here, next to the skip warnings, rather than pasted onto every + // response the server produces. Network servers launch no local process and + // report nothing, which is why the optional background registration is not + // asked: its only member is the built-in HTTP default, which starts no local + // process. A stdio default would need this statement from that path too. + // NOT deferred: stderr here is the bare terminal, and the TUI takes it over at + // deps.runTUI below. Delivery stops before that hand-off, so a late launch can + // never write raw text into the alt screen; see stopMCPDisclosures's call site. + guardedStderr, stopMCPDisclosures := reportMCPStartupDisclosures(stderr, mcpRuntime) + stderr = guardedStderr // Make local plugins live: register their declared tools into the registry and // collect their hooks + skill roots for the dispatcher and skill tool below. // Done after specialist + MCP registration so plugin tools are part of the @@ -958,6 +971,10 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a // notice when project hooks/plugins were dropped for an untrusted workspace. hookDispatcher, hookSkip := newHookDispatcherWithExtra(workspaceRoot, pluginActivation.hooks, trustRoot, executionRunner) emitTrustNotice(stderr, hookSkip, pluginActivation.trustSkip, mcpSkip) + // The terminal stops being ours on the next line. Stop and join the disclosure + // pump first: anything already queued is printed here, on this goroutine, and a + // launch that resolves later is dropped rather than written raw over the TUI. + stopMCPDisclosures() return deps.runTUI(context.Background(), tui.Options{ Cwd: workspaceRoot, Version: version, diff --git a/internal/cli/exec.go b/internal/cli/exec.go index 63d22f8bf..2d7aaac82 100644 --- a/internal/cli/exec.go +++ b/internal/cli/exec.go @@ -28,6 +28,7 @@ import ( "github.com/Gitlawb/zero/internal/streamjson" "github.com/Gitlawb/zero/internal/tools" "github.com/Gitlawb/zero/internal/trace" + "github.com/Gitlawb/zero/internal/tui" "github.com/Gitlawb/zero/internal/usage" "github.com/Gitlawb/zero/internal/worktrees" "github.com/Gitlawb/zero/internal/zeroruntime" @@ -345,6 +346,18 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in return writeExecProviderError(stdout, stderr, options.outputFormat, "mcp_error", err.Error()) } defer closeMCPRuntime(stderr, mcpRuntime) + // Said HERE, before --list-tools and before the first result, because both + // return early and the process this describes is already running by now. On + // stderr, so text, JSON and stream-JSON framing on stdout are untouched: + // this is the same channel the skipped-server and trust notices use. + // Deferred AFTER closeMCPRuntime was deferred, so it runs BEFORE it: the + // pump stops and joins while stderr is still ours, and only then are the + // clients closed. + // Adopt the guarded writer for the rest of startup: the pump is live from + // here until stop, and everything below writes to this same stderr. + guardedStderr, stopDisclosures := reportMCPStartupDisclosures(stderr, mcpRuntime) + stderr = guardedStderr + defer stopDisclosures() } pluginActivation = activatePlugins(workspaceRoot, registry, deps, stderr, trustRoot, executionRunner) registerLocalControlTools(registry, workspaceRoot, resolved.LocalControl) @@ -728,25 +741,7 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in }, OnToolResult: func(result agent.ToolResult) { writer.toolResult(result) - payload := map[string]any{ - "toolCallId": result.ToolCallID, - "name": result.Name, - "status": string(result.Status), - "output": result.Output, - } - if len(result.Meta) > 0 { - payload["meta"] = result.Meta - } - if result.Truncated { - payload["truncated"] = true - } - if result.Redacted { - payload["redacted"] = true - } - if len(result.ChangedFiles) > 0 { - payload["changedFiles"] = result.ChangedFiles - } - sessionRecorder.append(sessions.EventToolResult, payload) + sessionRecorder.append(sessions.EventToolResult, persistedToolResultPayload(result)) }, OnUsage: func(u agent.Usage) { writer.usage(u) @@ -1496,3 +1491,27 @@ func writeTraceSnapshot(snapshot *trace.TurnTrace, dest string, stderr io.Writer defer file.Close() return trace.WriteNDJSON(file, snapshot) } + +// persistedToolResultPayload renders one tool result for the durable session +// log. +// +// IT USES THE ACCESSOR, NOT THE RAW FIELD. agent.ToolResult stores the +// undecorated model text alongside the typed enforcement notices, and +// ModelOutput is what composes the two. Replay reads this payload straight back +// into the transcript without reconstructing a ToolResult, so a disclosure that +// is not rendered here is simply absent from resumed and compacted context even +// though it was visible during the original run. +// +// Both headless writers go through this, because they previously spelled the +// same payload separately and had already drifted: one persisted the raw field +// while the stream writer used the accessor. +func persistedToolResultPayload(result agent.ToolResult) map[string]any { + // ONE CONTRACT WITH THE TUI. This used to build its own payload with the + // decorated output only, and both writers append to the same default + // session store the TUI resumes from. A CLI-written result restored into + // the TUI therefore arrived without typed enforcement notices and without + // the undecorated card body, so a long collapsed result rendered no body + // and, with it, no disclosure. The interactive writer owns the shape now, + // and this is the same function, not a matching copy of it. + return tui.ToolResultSessionPayload(result) +} diff --git a/internal/cli/exec_payload_test.go b/internal/cli/exec_payload_test.go new file mode 100644 index 000000000..8d787413c --- /dev/null +++ b/internal/cli/exec_payload_test.go @@ -0,0 +1,47 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/tools" +) + +// THE HEADLESS WRITER PERSISTS THE SAME SHAPE THE TUI DOES. +// +// The tui-side restore test proves the shared payload restores a disclosure +// exactly once; this proves the CLI actually WRITES that shared payload rather +// than its own. The two used to be spelled separately and the headless one had +// already drifted to decorated output only. Reverting the delegation leaves +// the tui test green and fails this one, which is the point of having both. +func TestHeadlessPayloadCarriesTypedNoticesAndUndecoratedBody(t *testing.T) { + const notice = "denyRead is configured, so the Windows sandbox uses the token shape without WRITE_RESTRICTED (#869)" + result := agent.ToolResult{ + ToolCallID: "call-cli", + Name: "bash", + Status: tools.StatusOK, + Output: "PROBE-BODY", + Truncated: true, + EnforcementNotices: []string{notice}, + } + payload := persistedToolResultPayload(result) + + notices, _ := payload["enforcementNotices"].([]string) + if len(notices) != 1 || notices[0] != notice { + t.Errorf("headless payload does not carry the typed notice: %#v", payload["enforcementNotices"]) + } + preview, _ := payload["displayPreview"].(string) + if preview != "PROBE-BODY" { + t.Errorf("headless payload does not carry the undecorated body as displayPreview: %q", preview) + } + output, _ := payload["output"].(string) + if !strings.Contains(output, notice) || !strings.Contains(output, "PROBE-BODY") { + t.Errorf("provider-facing output is no longer the decorated text: %q", output) + } + // The one field the headless writer added on its own must survive the + // delegation, or a truncation marker silently stops being persisted. + if truncated, _ := payload["truncated"].(bool); !truncated { + t.Errorf("truncated flag was lost in the shared payload: %#v", payload["truncated"]) + } +} diff --git a/internal/cli/exec_spec.go b/internal/cli/exec_spec.go index fc22eed35..81e6fa5ab 100644 --- a/internal/cli/exec_spec.go +++ b/internal/cli/exec_spec.go @@ -158,22 +158,7 @@ func runExecSpecDraft(run execSpecDraftRun) int { if info, ok := execSpecDraftInfoFromToolResult(result); ok { draftInfo = info } - payload := map[string]any{ - "toolCallId": result.ToolCallID, - "name": result.Name, - "status": string(result.Status), - "output": result.Output, - } - if len(result.Meta) > 0 { - payload["meta"] = result.Meta - } - if result.Redacted { - payload["redacted"] = true - } - if len(result.ChangedFiles) > 0 { - payload["changedFiles"] = result.ChangedFiles - } - sessionRecorder.append(sessions.EventToolResult, payload) + sessionRecorder.append(sessions.EventToolResult, persistedToolResultPayload(result)) }, OnUsage: func(u agent.Usage) { writer.usage(u) diff --git a/internal/cli/exec_startup_disclosure_test.go b/internal/cli/exec_startup_disclosure_test.go new file mode 100644 index 000000000..162638b66 --- /dev/null +++ b/internal/cli/exec_startup_disclosure_test.go @@ -0,0 +1,127 @@ +package cli + +import ( + "bytes" + "context" + "encoding/json" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/tools" +) + +// disclosingExecRuntime is an MCP runtime that launched a process under reduced +// enforcement. +type disclosingExecRuntime struct { + noopMCPRuntime + disclosures []mcp.StartupDisclosure +} + +func (r disclosingExecRuntime) StartupDisclosures() []mcp.StartupDisclosure { return r.disclosures } + +const execDisclosureNotice = "denyRead is configured, so the write jail is not confining writes" + +// isolateConfigDirs points every config/cache root at test-owned storage. +// +// Without it these tests build a sandbox engine against the developer's REAL +// config dir, trigger the one-time grant migration there, and the migration +// notice then turns up on a LATER test's stderr, failing whichever test happens +// to assert an empty one. The failure moves between runs, which is what makes it +// look like flakiness rather than contamination. +func isolateConfigDirs(t *testing.T) { + t.Helper() + dir := t.TempDir() + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) + t.Setenv("APPDATA", dir) + t.Setenv("LOCALAPPDATA", dir) + t.Setenv("XDG_CONFIG_HOME", dir) + t.Setenv("XDG_CACHE_HOME", dir) +} + +func execDisclosureDeps(cwd string) appDeps { + return appDeps{ + getwd: func() (string, error) { return cwd, nil }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { return execResolvedConfig(), nil }, + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { + return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, nil + }, + registerMCPTools: func(context.Context, *tools.Registry, config.MCPConfig, mcp.RegisterOptions) (mcpToolRuntime, error) { + return disclosingExecRuntime{disclosures: []mcp.StartupDisclosure{ + {Name: "docs", Notices: []string{execDisclosureNotice}}, + }}, nil + }, + } +} + +// A HEADLESS RUN IS A DISCLOSURE SURFACE TOO. +// +// `zero exec` registers workspace MCP servers through the same sandbox-backed +// runner interactive startup uses, so a stdio server here can launch under the +// weakened token and serve the whole run. Reporting the disclosure only from the +// TUI meant every text, JSON, stream-JSON and --list-tools caller was told +// nothing about the enforcement trade, for a process that was already running. +func TestExecReportsMCPStartupDisclosures(t *testing.T) { + isolateConfigDirs(t) + for _, format := range []string{"", "--output-format=json", "--output-format=stream-json"} { + name := format + if name == "" { + name = "text" + } + t.Run(name, func(t *testing.T) { + args := []string{"exec", "--list-tools"} + if format != "" { + args = append(args, format) + } + var stdout, stderr bytes.Buffer + if code := runWithDeps(args, &stdout, &stderr, execDisclosureDeps(t.TempDir())); code != exitSuccess { + t.Fatalf("exit = %d, stderr = %s", code, stderr.String()) + } + if !strings.Contains(stderr.String(), execDisclosureNotice) { + t.Errorf("the headless run said nothing about the enforcement trade: %q", stderr.String()) + } + if !strings.Contains(stderr.String(), "docs") { + t.Errorf("the report does not name the server: %q", stderr.String()) + } + // The machine-readable surfaces must stay parseable: the disclosure + // belongs on stderr precisely so stdout framing is untouched. + if format == "--output-format=json" { + var any map[string]any + if err := json.Unmarshal(bytes.TrimSpace(stdout.Bytes()), &any); err != nil { + t.Errorf("stdout is no longer valid JSON: %v (%q)", err, stdout.String()) + } + } + if format == "--output-format=stream-json" { + for _, line := range strings.Split(strings.TrimSpace(stdout.String()), "\n") { + if strings.TrimSpace(line) == "" { + continue + } + var any map[string]any + if err := json.Unmarshal([]byte(line), &any); err != nil { + t.Errorf("a stream-json line is not valid JSON: %v (%q)", err, line) + } + } + } + }) + } +} + +// A run whose servers launched nothing says nothing. +func TestExecWithoutDisclosuresStaysQuiet(t *testing.T) { + isolateConfigDirs(t) + deps := execDisclosureDeps(t.TempDir()) + deps.registerMCPTools = func(context.Context, *tools.Registry, config.MCPConfig, mcp.RegisterOptions) (mcpToolRuntime, error) { + return noopMCPRuntime{}, nil + } + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"exec", "--list-tools"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d, stderr = %s", code, stderr.String()) + } + if strings.Contains(stderr.String(), "reduced enforcement") { + t.Errorf("a run with nothing to disclose printed one: %q", stderr.String()) + } +} diff --git a/internal/cli/mcp_late_disclosure_test.go b/internal/cli/mcp_late_disclosure_test.go new file mode 100644 index 000000000..6c7e67911 --- /dev/null +++ b/internal/cli/mcp_late_disclosure_test.go @@ -0,0 +1,163 @@ +package cli + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/tools" +) + +// A LAUNCH THAT COMPLETES AFTER THE REPORTER HAS RUN MUST STILL BE SAID, ONCE, +// AND ONLY WHILE SOMEONE OWNS THE WRITER. +// +// Both production paths call reportMCPStartupDisclosures exactly once, right +// after RegisterTools returns. A stdio attempt abandoned at the connect timeout +// can still be inside cmd.Start at that moment; the process then starts under +// the reduced write confinement, the reaper closes its client, and a reporter +// that merely SAMPLED the runtime has already come and gone. The retained sink +// held the fact and nobody read it again, so the operator saw the skipped +// server and never the disclosure. +// +// This drives the REAL reporter against a REAL runtime, rather than polling +// StartupDisclosures by hand, which is what an earlier regression did and which +// is precisely how it masked the original bug: a test that re-reads on the +// tester's behalf proves nothing about a production path that does not. +// +// It also never reads stderr while the pump could write. The buffer is examined +// only after stop has joined the pump, which is the same discipline both +// production callers follow, and is why this passes under -race. +func TestLateMCPLaunchReachesTheStartupReporterExactlyOnce(t *testing.T) { + const notice = "MCP server started without WRITE_RESTRICTED because denyRead is configured (#869)" + released := make(chan struct{}) + published := make(chan struct{}) + + runtime, err := mcp.RegisterTools(context.Background(), tools.NewRegistry(), + config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "slow": {Type: "stdio", Command: "slow-mcp"}, + }}, + mcp.RegisterOptions{ + ConnectTimeout: 50 * time.Millisecond, + ClientFactory: func(ctx context.Context, server mcp.Server) (mcp.ToolClient, error) { + // Held past the registration timeout AND past the settle grace, so + // registration has already reaped this attempt and returned. + <-released + mcp.PublishLaunchForTest(ctx, []string{notice}) + // Publishing is synchronous into the stream, so by the time this + // closes the disclosure is queued and stop cannot race past it. + close(published) + return nil, errors.New("initialize failed long after start") + }, + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = runtime.Close() }) + + // The reporter runs ONCE, here, exactly as runExec and the interactive + // startup path run it: before the launch has resolved. + var stderr bytes.Buffer + _, stop := reportMCPStartupDisclosures(&stderr, runtime) + + close(released) + <-published + // The owner ends delivery and joins the pump. Every write to the buffer has + // happened by the time this returns, so the reads below are unsynchronised + // only because there is no longer anything to synchronise with. + stop() + + got := stderr.String() + if n := strings.Count(got, notice); n != 1 { + t.Fatalf("a launch that completed after the reporter ran was disclosed %d time(s), want exactly 1:\n%s", n, got) + } + if !strings.Contains(got, "MCP server slow started with reduced enforcement") { + t.Errorf("the late disclosure does not name the server:\n%s", got) + } + if skipped := runtime.Skipped(); len(skipped) != 1 { + t.Errorf("the server should still be recorded as skipped: %#v", skipped) + } +} + +// And a server whose launch was already known when the reporter ran is said +// once by it, and not again by the late path. +func TestKnownMCPLaunchIsNotReportedTwice(t *testing.T) { + const notice = "MCP server started under reduced enforcement" + runtime, err := mcp.RegisterTools(context.Background(), tools.NewRegistry(), + config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "fast": {Type: "stdio", Command: "fast-mcp"}, + }}, + mcp.RegisterOptions{ + ConnectTimeout: time.Second, + ClientFactory: func(ctx context.Context, server mcp.Server) (mcp.ToolClient, error) { + mcp.PublishLaunchForTest(ctx, []string{notice}) + return nil, errors.New("initialize failed after start") + }, + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = runtime.Close() }) + + var stderr bytes.Buffer + _, stop := reportMCPStartupDisclosures(&stderr, runtime) + stop() + if n := strings.Count(stderr.String(), notice); n != 1 { + t.Fatalf("a launch known at registration was disclosed %d time(s), want exactly 1:\n%s", n, stderr.String()) + } +} + +// THE OWNERSHIP BOUNDARY ITSELF: once the caller has stopped delivery, nothing +// may write to its writer again. +// +// This is the property that the retained presentation callback could not hold. +// It invoked the CLI's print function from the abandoned connect goroutine +// whenever the launch happened to resolve, so a write could land after runExec +// had returned or after Bubble Tea had taken the alt screen. The interactive +// path stops delivery on the line before it hands over the terminal, and this +// pins what that buys: a launch resolving afterwards is dropped, not printed. +func TestMCPDisclosureAfterStopIsDroppedNotWritten(t *testing.T) { + const notice = "MCP server started under reduced enforcement" + released := make(chan struct{}) + published := make(chan struct{}) + + runtime, err := mcp.RegisterTools(context.Background(), tools.NewRegistry(), + config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "slow": {Type: "stdio", Command: "slow-mcp"}, + }}, + mcp.RegisterOptions{ + ConnectTimeout: 50 * time.Millisecond, + ClientFactory: func(ctx context.Context, server mcp.Server) (mcp.ToolClient, error) { + <-released + mcp.PublishLaunchForTest(ctx, []string{notice}) + close(published) + return nil, errors.New("initialize failed long after start") + }, + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = runtime.Close() }) + + var stderr bytes.Buffer + _, stop := reportMCPStartupDisclosures(&stderr, runtime) + // The owner gives up the writer BEFORE the launch resolves, which is the + // interactive hand-off to the TUI. + stop() + + close(released) + <-published + // Asserting an absence, so the wrong behaviour is given time to appear: once + // publishing has returned the disclosure is queued, and a delivery path that + // outlived stop would have this long to print it. With delivery ended and the + // pump joined there is no writer left, so this window changes nothing. + time.Sleep(50 * time.Millisecond) + + if got := stderr.String(); got != "" { + t.Fatalf("a launch that resolved after the owner stopped still wrote to its writer: %q", got) + } +} diff --git a/internal/cli/mcp_startup_disclosure_test.go b/internal/cli/mcp_startup_disclosure_test.go new file mode 100644 index 000000000..bf8a6288b --- /dev/null +++ b/internal/cli/mcp_startup_disclosure_test.go @@ -0,0 +1,52 @@ +package cli + +import ( + "bytes" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/mcp" +) + +type disclosingRuntime struct { + noopMCPRuntime + disclosures []mcp.StartupDisclosure +} + +func (runtime disclosingRuntime) StartupDisclosures() []mcp.StartupDisclosure { + return runtime.disclosures +} + +// SAID ONCE, WHERE THE USER IS ALREADY BEING TOLD WHAT STARTED. +// +// The disclosure describes a server PROCESS, which serves the whole session, so +// it cannot ride on a tool result and must not be repeated on every one. +func TestStartupDisclosuresAreReportedOnce(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + var stderr bytes.Buffer + reportMCPStartupDisclosures(&stderr, disclosingRuntime{ + disclosures: []mcp.StartupDisclosure{{Name: "docs", Notices: []string{notice}}}, + }) + output := stderr.String() + if count := strings.Count(output, notice); count != 1 { + t.Errorf("the disclosure appears %d times, want exactly 1: %q", count, output) + } + if !strings.Contains(output, "docs") { + t.Errorf("the report does not name the server it is about: %q", output) + } +} + +// A run with nothing to disclose prints nothing at all. +func TestNoStartupDisclosuresPrintNothing(t *testing.T) { + var stderr bytes.Buffer + reportMCPStartupDisclosures(&stderr, disclosingRuntime{}) + if stderr.Len() != 0 { + t.Errorf("a run with no disclosure wrote %q", stderr.String()) + } + stderr.Reset() + // And a runtime that launches nothing is not required to answer. + reportMCPStartupDisclosures(&stderr, noopMCPRuntime{}) + if stderr.Len() != 0 { + t.Errorf("a runtime that launches nothing wrote %q", stderr.String()) + } +} diff --git a/internal/cli/mcp_tools.go b/internal/cli/mcp_tools.go index 022d64001..4de3bb535 100644 --- a/internal/cli/mcp_tools.go +++ b/internal/cli/mcp_tools.go @@ -3,9 +3,11 @@ package cli import ( "context" "fmt" + "io" "net/url" "sort" "strings" + "sync" "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/execution" @@ -188,3 +190,107 @@ func isSensitiveMCPDisplayKey(key string) bool { } return false } + +// mcpStartupDisclosing is the optional interface a runtime implements when it +// can report what its launched server processes ran under. Optional rather than +// part of mcpToolRuntime so a runtime that launches nothing, and every test +// double, stays unchanged. +type mcpStartupDisclosing interface { + StartupDisclosures() []mcp.StartupDisclosure +} + +// mcpStartupStreaming is the push form: the runtime queues each disclosure as a +// typed event, including a launch that completes after registration returned, +// and this package drains it on the goroutine that owns stderr. +type mcpStartupStreaming interface { + StartupDisclosureStream() *mcp.StartupDisclosureStream +} + +// reportMCPStartupDisclosures states once what enforcement applied to the MCP +// server processes this run launched. +// +// A PUSH, NOT A SAMPLE. This used to read StartupDisclosures once, here, and a +// stdio attempt abandoned at the connect timeout could still be inside cmd.Start +// at that moment. The process then started under the reduced write confinement, +// the reaper closed its client, and nothing read the runtime again: the operator +// saw the skipped-server warning and never the disclosure. +// +// THIS GOROUTINE OWNS THE WRITER. The runtime queues typed disclosures; every +// write to stderr happens either on the caller's goroutine (the set already known +// when this returns, in server order, so startup output keeps its order) or on +// the single pump started here, never both at once and never after stop. +// +// The returned stop ends delivery and joins the pump, so no write to stderr can +// outlive the caller's ownership of it. The caller must run it before handing the +// terminal to anything else. A disclosure that arrives after stop is dropped: it +// is worth printing while someone owns the writer, and worth losing rather than +// writing into a screen that now belongs to Bubble Tea. Anything already queued +// when stop runs is still printed, on the caller's goroutine, with the pump +// already finished. +// +// ONE WRITER, ONE CALLER AT A TIME. Joining the pump stops writes after its +// lifetime but does nothing about the overlap: startup keeps emitting plugin, +// trust, peer, provider and validation output to the same writer while the pump +// is live. That is unsafe for an ordinary bytes.Buffer and interleaves lines even +// on a writer that tolerates concurrent calls. A mutex private to the pump would +// not help, because the other writes do not go through it. So the returned writer +// is a guarded view of the caller's, and the caller adopts it for the rest of +// startup; both sides then take the same lock. +// +// The pull form is kept for a runtime that implements no stream, which today is +// only test doubles; it has no late launches to deliver, so its stop is a no-op +// and its writer is handed back unchanged. +func reportMCPStartupDisclosures(stderr io.Writer, runtime mcpToolRuntime) (guarded io.Writer, stop func()) { + serialized := &serializedWriter{writer: stderr} + print := func(disclosure mcp.StartupDisclosure) { + for _, notice := range disclosure.Notices { + fmt.Fprintf(serialized, "notice: MCP server %s started with reduced enforcement: %s\n", disclosure.Name, notice) + } + } + printAll := func(disclosures []mcp.StartupDisclosure) { + for _, disclosure := range disclosures { + print(disclosure) + } + } + streaming, ok := runtime.(mcpStartupStreaming) + if !ok { + if disclosing, ok := runtime.(mcpStartupDisclosing); ok { + printAll(disclosing.StartupDisclosures()) + } + return stderr, func() {} + } + stream := streaming.StartupDisclosureStream() + if stream == nil { + return stderr, func() {} + } + printAll(stream.Drain()) + pumped := make(chan struct{}) + go func() { + defer close(pumped) + for stream.Wait() { + printAll(stream.Drain()) + } + }() + var once sync.Once + return serialized, func() { + once.Do(func() { + stream.Close() + <-pumped + printAll(stream.Drain()) + }) + } +} + +// serializedWriter gives one underlying writer a single owner at a time, so the +// late-disclosure pump and the foreground startup path cannot be inside it +// together. +type serializedWriter struct { + mu sync.Mutex + writer io.Writer +} + +func (w *serializedWriter) Write(data []byte) (int, error) { + w.mu.Lock() + defer w.mu.Unlock() + return w.writer.Write(data) +} diff --git a/internal/cli/mcp_writer_ownership_test.go b/internal/cli/mcp_writer_ownership_test.go new file mode 100644 index 000000000..857bb124e --- /dev/null +++ b/internal/cli/mcp_writer_ownership_test.go @@ -0,0 +1,146 @@ +package cli + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "strings" + "sync" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/tools" +) + +// blockingWriter makes the overlap deterministic instead of hoping to hit it. +// +// The first write parks inside Write until the test releases it, which is the +// window the pump and the foreground startup path really share: startup keeps +// emitting plugin, trust, peer and provider output to the same stderr while a +// late MCP disclosure can arrive at any moment. It also records whether it was +// ever entered twice at once, which is the property under test. +type blockingWriter struct { + mu sync.Mutex + inside int + overlaps int + buf bytes.Buffer + + block chan struct{} + blockOne sync.Once + entered chan struct{} +} + +func newBlockingWriter() *blockingWriter { + return &blockingWriter{block: make(chan struct{}), entered: make(chan struct{})} +} + +func (w *blockingWriter) Write(data []byte) (int, error) { + w.mu.Lock() + w.inside++ + if w.inside > 1 { + w.overlaps++ + } + w.mu.Unlock() + + // Only the first writer parks, and it announces that it is inside. + first := false + w.blockOne.Do(func() { + first = true + close(w.entered) + }) + if first { + <-w.block + } + + w.mu.Lock() + n, err := w.buf.Write(data) + w.inside-- + w.mu.Unlock() + return n, err +} + +func (w *blockingWriter) overlapCount() int { + w.mu.Lock() + defer w.mu.Unlock() + return w.overlaps +} + +func (w *blockingWriter) String() string { + w.mu.Lock() + defer w.mu.Unlock() + return w.buf.String() +} + +// ONE CALLER AT A TIME, FOR THE WHOLE OVERLAP. +// +// Joining the pump at stop bounds writes to the pump's lifetime but says nothing +// about what happens DURING it. The startup paths keep writing to the same +// io.Writer the whole time, and the caller may legitimately hand in a plain +// bytes.Buffer, which corrupts under concurrent use. A mutex private to the pump +// would not have helped, because the foreground writes do not go through it; the +// reporter therefore hands back a guarded view of the caller's writer and the +// caller adopts it, so both sides take the same lock. +func TestLateDisclosureAndForegroundStartupNeverShareTheWriter(t *testing.T) { + const notice = "MCP server started under reduced enforcement" + released := make(chan struct{}) + published := make(chan struct{}) + + runtime, err := mcp.RegisterTools(context.Background(), tools.NewRegistry(), + config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "slow": {Type: "stdio", Command: "slow-mcp"}, + }}, + mcp.RegisterOptions{ + ConnectTimeout: 50 * time.Millisecond, + ClientFactory: func(ctx context.Context, server mcp.Server) (mcp.ToolClient, error) { + <-released + mcp.PublishLaunchForTest(ctx, []string{notice}) + close(published) + return nil, errors.New("initialize failed long after start") + }, + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = runtime.Close() }) + + writer := newBlockingWriter() + guarded, stop := reportMCPStartupDisclosures(writer, runtime) + if guarded == io.Writer(writer) { + t.Fatal("SETUP INVALID: the reporter handed back the raw writer, so the caller cannot share its lock") + } + + // Foreground startup writes through the guarded writer and parks inside it, + // exactly as a slow terminal would. + foregroundDone := make(chan struct{}) + go func() { + defer close(foregroundDone) + fmt.Fprintln(guarded, "warning: MCP server other unavailable, skipped: dial tcp: refused") + }() + <-writer.entered + + // While the foreground write is parked, the late launch resolves and the pump + // tries to print. If the two did not share a lock, this would enter Write + // concurrently. + close(released) + <-published + time.Sleep(50 * time.Millisecond) + + close(writer.block) + <-foregroundDone + stop() + + if n := writer.overlapCount(); n != 0 { + t.Fatalf("the pump and foreground startup were inside the writer together %d time(s)", n) + } + got := writer.String() + if count := strings.Count(got, notice); count != 1 { + t.Fatalf("the late disclosure was written %d time(s), want exactly 1 after stop drained it:\n%s", count, got) + } + if !strings.Contains(got, "unavailable, skipped") { + t.Errorf("the foreground startup message was lost:\n%s", got) + } +} diff --git a/internal/cli/persisted_tool_result_test.go b/internal/cli/persisted_tool_result_test.go new file mode 100644 index 000000000..d5ab06236 --- /dev/null +++ b/internal/cli/persisted_tool_result_test.go @@ -0,0 +1,72 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/tools" +) + +// A DISCLOSURE THAT IS NOT PERSISTED DID NOT SURVIVE THE RUN. +// +// The session log is what a resumed or compacted conversation is rebuilt from, +// and replay reads this payload's "output" straight into the transcript without +// reconstructing an agent.ToolResult. So persisting the raw undecorated field +// makes a warning that was visible during the original run disappear the moment +// the session is resumed, with nothing failing anywhere to say so. +func TestPersistedToolResultKeepsTheEnforcementNotice(t *testing.T) { + const notice = "least-privilege notice: read access was narrowed" + payload := persistedToolResultPayload(agent.ToolResult{ + ToolCallID: "call-1", + Name: "bash", + Status: tools.StatusOK, + Output: "the command output", + EnforcementNotices: []string{notice}, + }) + + output, _ := payload["output"].(string) + if count := strings.Count(output, notice); count != 1 { + t.Errorf("persisted output carries the notice %d times, want exactly 1: %q", count, output) + } + if !strings.Contains(output, "the command output") { + t.Errorf("persisted output lost the command output: %q", output) + } +} + +// The other fields still round-trip, so the shared helper did not quietly drop +// what the two writers used to record separately. +func TestPersistedToolResultKeepsItsOtherFields(t *testing.T) { + payload := persistedToolResultPayload(agent.ToolResult{ + ToolCallID: "call-2", + Name: "write_file", + Status: tools.StatusError, + Output: "boom", + Meta: map[string]string{"k": "v"}, + Truncated: true, + Redacted: true, + ChangedFiles: []string{"a.go"}, + }) + for _, field := range []string{"toolCallId", "name", "status", "output", "meta", "truncated", "redacted", "changedFiles"} { + if _, ok := payload[field]; !ok { + t.Errorf("payload is missing %q: %#v", field, payload) + } + } + if payload["status"] != string(tools.StatusError) { + t.Errorf("status = %v, want %q", payload["status"], tools.StatusError) + } +} + +// An ordinary result records exactly what it did before, so the accessor is not +// adding anything where there is nothing to add. +func TestPersistedToolResultLeavesAnOrdinaryResultAlone(t *testing.T) { + payload := persistedToolResultPayload(agent.ToolResult{ + ToolCallID: "call-3", + Name: "bash", + Status: tools.StatusOK, + Output: "plain output", + }) + if got := payload["output"]; got != "plain output" { + t.Errorf("persisted output = %v, want it untouched", got) + } +} diff --git a/internal/execution/contracts.go b/internal/execution/contracts.go index dd861ac8f..78db0ca5d 100644 --- a/internal/execution/contracts.go +++ b/internal/execution/contracts.go @@ -173,6 +173,11 @@ type Enforcement struct { Level string `json:"level,omitempty"` Degraded bool `json:"degraded,omitempty"` DowngradeReason string `json:"downgradeReason,omitempty"` + // Notices are least-privilege disclosures about the enforcement actually + // applied to THIS command, as opposed to the diagnostic views produced by + // `zero sandbox policy` and `zero sandbox check`. A trade an operator only + // discovers by running a separate diagnostic command is not disclosed. + Notices []string `json:"notices,omitempty"` } type Outcome struct { @@ -182,7 +187,18 @@ type Outcome struct { Exit *Exit `json:"exit,omitempty"` Denial *Denial `json:"denial,omitempty"` Enforcement Enforcement `json:"enforcement"` - Changes []Change `json:"changes,omitempty"` + // Launched records whether an OS process was actually created, observed at + // the boundary that calls Run rather than inferred afterwards. + // + // OutcomeKind is not a launch-state field, and reading it as one is wrong in + // both directions. A child that ran and then produced an unreadable adapter + // report is rewritten to a setup failure, so inference drops a disclosure that + // did apply; a context already cancelled before os.StartProcess yields a + // cancellation, so inference claims reduced enforcement for a child that never + // existed. Report decoding can fail after launch without rewriting history, + // and cancellation happens on either side of Start. + Launched bool `json:"launched,omitempty"` + Changes []Change `json:"changes,omitempty"` } // AdapterReport is the structured, machine-readable result emitted by a @@ -190,6 +206,51 @@ type Outcome struct { // command text cannot impersonate a policy decision. type AdapterReport struct { Denial *Denial `json:"denial,omitempty"` + // ChildLaunched is the adapter's authoritative statement that the REQUESTED + // process started, for a plan where the command the runner starts is not that + // process. + // + // A wrapped plan starts a helper, and the helper creates the sandboxed child + // only after validating the setup marker, applying ACLs, checking the network + // policy, building capability SIDs and minting the restricted token. Any of + // those can fail with the helper already running, so the runner's own + // exec.Cmd.Process tells it the WRAPPER started and nothing about the child. + // Only the adapter sees that transition, so only the adapter may report it. + // + // nil means the adapter does not speak to this, and the runner keeps its own + // observation. That is correct for every direct, unwrapped command, where the + // process the runner starts IS the requested one. + ChildLaunched *bool `json:"childLaunched,omitempty"` +} + +// ChildLaunched reports whether this outcome describes a process that actually +// started, from the recorded fact rather than from the terminal outcome kind. +func (outcome Outcome) ChildLaunched() bool { + return outcome.Launched +} + +// AppliedEnforcementNotices returns the least-privilege disclosures that are +// true of what actually happened. +// +// ONE DECISION, AT THE BOUNDARY WHERE THE OUTCOME IS KNOWN. Enforcement.Notices +// is planned: it describes the shape the command was PREPARED to run under, and +// planning is not proof that anything ran. Every consumer that copied the field +// straight out therefore made the completed-enforcement claim for commands that +// never launched, telling an operator the write jail had been traded away for a +// child that failed before it existed. +// +// Keeping this on Outcome rather than repeating an outcome-kind switch in hooks, +// plugins and tools is the point: a new pre-launch outcome kind has to be +// classified once, here, instead of being silently disclosed by whichever +// consumer was not updated. +func (outcome Outcome) AppliedEnforcementNotices() []string { + if !outcome.ChildLaunched() { + return nil + } + if len(outcome.Enforcement.Notices) == 0 { + return nil + } + return append([]string(nil), outcome.Enforcement.Notices...) } func (outcome Outcome) Validate() error { diff --git a/internal/execution/launch_state_test.go b/internal/execution/launch_state_test.go new file mode 100644 index 000000000..c5261b9e1 --- /dev/null +++ b/internal/execution/launch_state_test.go @@ -0,0 +1,111 @@ +package execution + +import ( + "context" + "errors" + "os/exec" + "runtime" + "testing" +) + +const launchStateNotice = "denyRead is configured, so the write jail is not confining writes" + +func launchStateShell(ctx context.Context, script string) *exec.Cmd { + if runtime.GOOS == "windows" { + return exec.CommandContext(ctx, "cmd.exe", "/c", script) + } + return exec.CommandContext(ctx, "/bin/sh", "-c", script) +} + +// launchStatePreparer plans a command carrying an enforcement notice, and can +// make the adapter report fail after the child has already run. +type launchStatePreparer struct { + script string + reportErr error + missing bool +} + +func (p *launchStatePreparer) PrepareExecution(ctx context.Context, _ Request) (PreparedCommand, error) { + command := launchStateShell(ctx, p.script) + if p.missing { + command = exec.CommandContext(ctx, "definitely-not-a-real-binary-zzz") + } + prepared := PreparedCommand{ + Command: command, + Enforcement: Enforcement{Notices: []string{launchStateNotice}}, + } + if p.reportErr != nil { + prepared.Report = func() (AdapterReport, error) { return AdapterReport{}, p.reportErr } + } + return prepared, nil +} + +func captured(t *testing.T, ctx context.Context, p *launchStatePreparer) CapturedResult { + t.Helper() + return NewRunner(p).ExecuteCaptured(ctx, CapturedRequest{Request: Request{ + Origin: OriginHook, + Mode: ModeCaptured, + Command: Command{Name: "irrelevant"}, + WorkingDirectory: t.TempDir(), + WorkspaceRoots: []string{t.TempDir()}, + Approval: ApprovalContext{PolicyVersion: PolicyVersion}, + }}) +} + +// THE OUTCOME KIND IS NOT A LAUNCH-STATE FIELD, IN EITHER DIRECTION. +// +// Deriving launch from the terminal kind is wrong twice over. The adapter report +// is read AFTER Run, so a child that really ran and then produced an unreadable +// report is rewritten to a setup failure: inference drops a disclosure that did +// apply. And a context already cancelled before os.StartProcess still selects a +// cancellation, so inference claims reduced enforcement for a process that never +// existed. +func TestLaunchStateIsRecordedNotInferred(t *testing.T) { + t.Run("ran, then the adapter report failed", func(t *testing.T) { + result := captured(t, context.Background(), &launchStatePreparer{ + script: "exit 0", + reportErr: errors.New("adapter report is unreadable"), + }) + if result.Outcome.Kind != OutcomeSandboxSetupFailure { + t.Fatalf("SETUP INVALID: kind = %q, want the report failure to rewrite it", result.Outcome.Kind) + } + if !result.Outcome.Launched { + t.Error("a child that ran was recorded as never launched") + } + if got := result.Outcome.AppliedEnforcementNotices(); len(got) != 1 { + t.Errorf("the disclosure was dropped for a child that did run: %#v", got) + } + }) + + t.Run("cancelled before the process started", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + result := captured(t, ctx, &launchStatePreparer{script: "exit 0"}) + if result.Outcome.Launched { + t.Error("a process that never started was recorded as launched") + } + if got := result.Outcome.AppliedEnforcementNotices(); len(got) != 0 { + t.Errorf("an enforcement trade was claimed for a process that never existed: %#v", got) + } + }) + + t.Run("never found", func(t *testing.T) { + result := captured(t, context.Background(), &launchStatePreparer{missing: true}) + if result.Outcome.Launched { + t.Error("a missing executable was recorded as launched") + } + if got := result.Outcome.AppliedEnforcementNotices(); len(got) != 0 { + t.Errorf("a missing executable claimed an enforcement trade: %#v", got) + } + }) + + t.Run("ordinary success still discloses", func(t *testing.T) { + result := captured(t, context.Background(), &launchStatePreparer{script: "exit 0"}) + if !result.Outcome.Launched { + t.Fatal("an ordinary run was recorded as never launched") + } + if got := result.Outcome.AppliedEnforcementNotices(); len(got) != 1 { + t.Errorf("an ordinary run lost its disclosure: %#v", got) + } + }) +} diff --git a/internal/execution/live_launch_observation_test.go b/internal/execution/live_launch_observation_test.go new file mode 100644 index 000000000..2b5e8a270 --- /dev/null +++ b/internal/execution/live_launch_observation_test.go @@ -0,0 +1,199 @@ +package execution + +import ( + "context" + "encoding/json" + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "testing" + "time" +) + +const liveLaunchNotice = "denyRead is configured, so the write jail is not confining writes" + +// liveReportReader mirrors sandbox.CommandPlan.ExecutionReport: read the file the +// helper publishes, treat "not there yet" as nothing recorded. +func liveReportReader(path string) func() (AdapterReport, error) { + return func() (AdapterReport, error) { + raw, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return AdapterReport{}, nil + } + if err != nil { + return AdapterReport{}, err + } + var report AdapterReport + if err := json.Unmarshal(raw, &report); err != nil { + return AdapterReport{}, err + } + return report, nil + } +} + +// liveHelperCommand stands in for the Windows helper: publish the launch fact the +// way it does right after CreateProcessAsUser, then stay alive the way it does +// while waiting on the child. +func liveHelperCommand(t *testing.T, reportPath string, publish bool) *exec.Cmd { + t.Helper() + if publish { + if err := os.WriteFile(reportPath, []byte(`{"childLaunched":true}`), 0o600); err != nil { + t.Fatalf("publish the launch report: %v", err) + } + } + if runtime.GOOS == "windows" { + // A child that holds itself open without exiting. + return exec.Command("cmd.exe", "/c", "pause") + } + return exec.Command("/bin/sh", "-c", "sleep 30") +} + +// liveRequest is a valid interactive request; ProcessManager.Start validates it +// before anything under test runs. +func liveRequest(t *testing.T) Request { + t.Helper() + return Request{ + Origin: OriginInteractiveCommand, + Mode: ModeInteractive, + Command: Command{Name: "helper"}, + WorkingDirectory: t.TempDir(), + WorkspaceRoots: []string{t.TempDir()}, + Approval: ApprovalContext{PolicyVersion: PolicyVersion}, + } +} + +// A RETAINED SESSION HAS TO DISCLOSE WHILE IT IS STILL RUNNING. +// +// The helper publishes childLaunched immediately after it creates the restricted +// child, and only then waits for it. The manager read that report exclusively in +// the post-Wait goroutine, so for the whole live lifetime of a wrapped session the +// report was the zero value: the first exec_command reply and every write_stdin +// poll resolved Launched=false and disclosed nothing, while the fact sat readable +// on disk. A watcher, or a retained session nobody polls to completion, would +// never be told the write jail had been traded away. +// +// Driven through the real ProcessManager with a real child process, and asserted +// on the ProcessResult the tool layer consumes. +func TestALiveWrappedSessionCarriesTheLaunchFact(t *testing.T) { + directory := t.TempDir() + reportPath := filepath.Join(directory, "report.json") + command := liveHelperCommand(t, reportPath, true) + + manager := NewProcessManager(ProcessManagerOptions{}) + result, err := manager.Start(context.Background(), ProcessStart{ + Prepared: PreparedCommand{ + Command: command, + ChildLaunchOwnedByAdapter: true, + Enforcement: Enforcement{Notices: []string{liveLaunchNotice}}, + Report: liveReportReader(reportPath), + }, + Request: liveRequest(t), + }, 300*time.Millisecond) + if err != nil { + t.Fatalf("Start: %v", err) + } + t.Cleanup(func() { manager.StopAll() }) + + // SETUP: this has to be the LIVE state, and the fact has to be on disk, or the + // assertion below would be about a terminal read. + if result.Exited { + t.Fatal("SETUP INVALID: the stand-in helper exited, so the live lifecycle is not under test") + } + if _, statErr := os.Stat(reportPath); statErr != nil { + t.Fatalf("SETUP INVALID: the launch report is not on disk, so there is nothing to observe: %v", statErr) + } + + if !ResolveChildLaunched(true, result.ChildLaunchOwnedByAdapter, result.Report) { + t.Fatal("a live wrapped session resolved as not launched, so its enforcement disclosure is withheld while the command runs") + } + + // And again on a poll, which is the write_stdin leg. + polled, err := manager.Continue(context.Background(), ProcessContinue{ProcessID: result.ProcessID, Wait: 200 * time.Millisecond}) + if err != nil { + t.Fatalf("Continue: %v", err) + } + if polled.Exited { + t.Fatal("SETUP INVALID: the helper exited before the poll, so the live poll is not under test") + } + if !ResolveChildLaunched(true, polled.ChildLaunchOwnedByAdapter, polled.Report) { + t.Fatal("a live poll of a wrapped session resolved as not launched") + } + if got := polled.Enforcement.Notices; len(got) != 1 || got[0] != liveLaunchNotice { + t.Fatalf("the live poll carries notices %v, want exactly the one planned notice", got) + } +} + +// AND A HELPER THAT NEVER CREATED THE CHILD STAYS SILENT. +// +// This is the negative the live read must not destroy. A helper that starts and +// then fails setup, ACL application, or CreateProcessAsUser has an outer process +// running and no child, so nothing may be promoted from the fact that the +// wrapper itself is alive. +func TestALiveWrappedSessionWithNoReportedChildStaysSilent(t *testing.T) { + directory := t.TempDir() + reportPath := filepath.Join(directory, "report.json") + command := liveHelperCommand(t, reportPath, false) + + manager := NewProcessManager(ProcessManagerOptions{}) + result, err := manager.Start(context.Background(), ProcessStart{ + Prepared: PreparedCommand{ + Command: command, + ChildLaunchOwnedByAdapter: true, + Enforcement: Enforcement{Notices: []string{liveLaunchNotice}}, + Report: liveReportReader(reportPath), + }, + Request: liveRequest(t), + }, 300*time.Millisecond) + if err != nil { + t.Fatalf("Start: %v", err) + } + t.Cleanup(func() { manager.StopAll() }) + + if result.Exited { + t.Fatal("SETUP INVALID: the stand-in helper exited, so the live lifecycle is not under test") + } + if _, statErr := os.Stat(reportPath); statErr == nil { + t.Fatal("SETUP INVALID: a report exists, so this is not the no-child case") + } + if ResolveChildLaunched(true, result.ChildLaunchOwnedByAdapter, result.Report) { + t.Fatal("a helper that reported no child was promoted to a launch, so the operator is told a write jail was traded away for a child that never existed") + } +} + +// A report that appears mid-flight is observed on the next poll, which is what +// makes this a lifecycle transition rather than a start-time snapshot. +func TestTheLaunchFactIsObservedWhenItAppearsMidFlight(t *testing.T) { + directory := t.TempDir() + reportPath := filepath.Join(directory, "report.json") + command := liveHelperCommand(t, reportPath, false) + + manager := NewProcessManager(ProcessManagerOptions{}) + result, err := manager.Start(context.Background(), ProcessStart{ + Prepared: PreparedCommand{ + Command: command, + ChildLaunchOwnedByAdapter: true, + Report: liveReportReader(reportPath), + }, + Request: liveRequest(t), + }, 200*time.Millisecond) + if err != nil { + t.Fatalf("Start: %v", err) + } + t.Cleanup(func() { manager.StopAll() }) + if ResolveChildLaunched(true, result.ChildLaunchOwnedByAdapter, result.Report) { + t.Fatal("SETUP INVALID: nothing was published yet, so the first result must not be a launch") + } + + if err := os.WriteFile(reportPath, []byte(`{"childLaunched":true}`), 0o600); err != nil { + t.Fatalf("publish the launch report: %v", err) + } + polled, err := manager.Continue(context.Background(), ProcessContinue{ProcessID: result.ProcessID, Wait: 200 * time.Millisecond}) + if err != nil { + t.Fatalf("Continue: %v", err) + } + if !ResolveChildLaunched(true, polled.ChildLaunchOwnedByAdapter, polled.Report) { + t.Fatal("the launch published while the session was live was never observed") + } +} diff --git a/internal/execution/process_manager.go b/internal/execution/process_manager.go index 7e4005fc8..02e2e995a 100644 --- a/internal/execution/process_manager.go +++ b/internal/execution/process_manager.go @@ -74,8 +74,12 @@ type ProcessResult struct { Enforcement Enforcement Report AdapterReport ReportErr error - Changes []Change - Metadata map[string]string + // ChildLaunchOwnedByAdapter carries the prepared plan's ownership of the + // requested-child launch fact through to the caller, which for a retained + // session no longer has the plan. + ChildLaunchOwnedByAdapter bool + Changes []Change + Metadata map[string]string } type ProcessSnapshot struct { @@ -148,6 +152,7 @@ func (manager *ProcessManager) Start(ctx context.Context, input ProcessStart, wa command: command, request: request, enforcement: input.Prepared.Enforcement, + ownedLaunch: input.Prepared.ChildLaunchOwnedByAdapter, report: input.Prepared.Report, cleanup: input.Prepared.Cleanup, stdin: stdin, @@ -362,31 +367,46 @@ func (manager *ProcessManager) removeCompletedLater(process *managedProcess) { } type managedProcess struct { - id int - commandText string - cwd string - relativeCwd string - startedAt time.Time - lastUsedAt time.Time - tty bool - command *exec.Cmd - request Request - enforcement Enforcement - report func() (AdapterReport, error) - cleanup func() - stdin io.WriteCloser - output *processOutputBuffer - reaped chan struct{} - doneOnce sync.Once - done chan struct{} - kill func(int) error - mu sync.Mutex - exitCode *int - waitErr error - resultReport AdapterReport - reportErr error - changes []Change - metadata map[string]string + id int + commandText string + cwd string + relativeCwd string + startedAt time.Time + lastUsedAt time.Time + tty bool + command *exec.Cmd + request Request + enforcement Enforcement + ownedLaunch bool + // launchObserved latches the adapter's launch transition the first time it is + // seen, so a live result can report it. Guarded by mu. + launchObserved bool + report func() (AdapterReport, error) + cleanup func() + stdin io.WriteCloser + output *processOutputBuffer + reaped chan struct{} + doneOnce sync.Once + done chan struct{} + kill func(int) error + mu sync.Mutex + exitCode *int + waitErr error + resultReport AdapterReport + reportErr error + changes []Change + metadata map[string]string +} + +// launchedReportLocked returns the report to hand out, with a latched live +// launch folded in. Caller holds mu. +func (process *managedProcess) launchedReportLocked() AdapterReport { + report := process.resultReport + if report.ChildLaunched == nil && process.launchObserved { + launched := true + report.ChildLaunched = &launched + } + return report } func (process *managedProcess) markDone(err error, exitCode int, report AdapterReport, reportErr error, changes []Change) { @@ -394,14 +414,63 @@ func (process *managedProcess) markDone(err error, exitCode int, report AdapterR process.waitErr = err process.exitCode = &exitCode process.resultReport = report + // The plan's cleanup has already removed the report file by the time this + // runs on some orderings, so a terminal read can answer "nothing recorded" + // about a child that demonstrably started. A launch we already saw is not + // un-seen by that. + if report.ChildLaunched == nil && process.launchObserved { + launched := true + process.resultReport.ChildLaunched = &launched + } process.reportErr = reportErr process.changes = append([]Change(nil), changes...) process.mu.Unlock() process.doneOnce.Do(func() { close(process.done) }) } +// observeLaunch reads the adapter's launch report while the process is still +// running, and latches a confirmed launch. +// +// THE LAUNCH FACT IS A LIFECYCLE TRANSITION, NOT TERMINAL DATA. The Windows +// helper publishes childLaunched immediately after CreateProcessAsUser creates +// the restricted child, and then waits for it. The manager used to read the +// report only in the post-Wait goroutine, so for the entire live lifetime of a +// retained session the report was the zero value: the first exec_command reply +// and every write_stdin poll resolved Launched=false and disclosed nothing, +// even though the fact was sitting readable on disk. A watcher or an abandoned +// retained session could therefore never be told the write jail had been traded +// away. The MCP launcher already reads the report while its server is live; +// this is the same read, in the launcher that was left behind. +// +// ONLY THE POSITIVE IS PROMOTED, AND ONLY ONCE. An absent, partial, or +// undecodable report, and a helper that failed before it ever created the child, +// must all leave the live result exactly as it was: not confirmed, nothing +// disclosed. Latching false, or surfacing a read error or a denial from here, +// would let a mid-flight poll rewrite a running command into a setup failure. +// The latch also keeps a wrapped plan to one file read rather than one per poll, +// and leaves every unwrapped plan doing no extra work at all. +func (process *managedProcess) observeLaunch() { + if process.report == nil { + return + } + process.mu.Lock() + skip := !process.ownedLaunch || process.launchObserved + process.mu.Unlock() + if skip || process.doneClosed() { + return + } + report, err := process.report() + if err != nil || report.ChildLaunched == nil || !*report.ChildLaunched { + return + } + process.mu.Lock() + process.launchObserved = true + process.mu.Unlock() +} + func (process *managedProcess) collectResult(ctx context.Context, wait time.Duration, interrupted bool) ProcessResult { output, truncated := process.collect(ctx, wait) + process.observeLaunch() process.mu.Lock() exitCode := 0 exited := process.exitCode != nil @@ -412,8 +481,9 @@ func (process *managedProcess) collectResult(ctx context.Context, wait time.Dura ProcessID: process.id, CommandText: process.commandText, RelativeCwd: process.relativeCwd, TTY: process.tty, Output: output, OutputTruncated: truncated, Exited: exited, ExitCode: exitCode, Interrupted: interrupted, Request: process.request, - Enforcement: process.enforcement, Report: process.resultReport, ReportErr: process.reportErr, - Changes: append([]Change(nil), process.changes...), Metadata: cloneStringMap(process.metadata), + Enforcement: process.enforcement, Report: process.launchedReportLocked(), ReportErr: process.reportErr, + ChildLaunchOwnedByAdapter: process.ownedLaunch, + Changes: append([]Change(nil), process.changes...), Metadata: cloneStringMap(process.metadata), } process.mu.Unlock() return result diff --git a/internal/execution/runner.go b/internal/execution/runner.go index 9e3ecbf9a..587c92739 100644 --- a/internal/execution/runner.go +++ b/internal/execution/runner.go @@ -24,6 +24,12 @@ type PreparedCommand struct { Enforcement Enforcement Report func() (AdapterReport, error) Cleanup func() + // ChildLaunchOwnedByAdapter marks a plan where Command is a WRAPPER and the + // requested process is created inside it, so exec.Cmd.Process says nothing + // about whether the sandboxed child ever existed. The adapter must state the + // fact in its report; if it does not, the runner treats the child as not + // launched rather than crediting the wrapper's start. + ChildLaunchOwnedByAdapter bool } type CapturedRequest struct { @@ -90,10 +96,25 @@ func (runner *Runner) ExecuteCaptured(ctx context.Context, input CapturedRequest prepared.Command.Stdout = stdout prepared.Command.Stderr = stderr runErr := prepared.Command.Run() + // Observed HERE, from the only thing that knows: exec.Cmd sets Process only + // once os.StartProcess has succeeded, so this is false for a missing + // executable and for a context cancelled before Start, and true for anything + // that ran, including a later timeout or cancellation. + launched := prepared.Command.Process != nil report, reportErr := AdapterReport{}, error(nil) if prepared.Report != nil { report, reportErr = prepared.Report() } + // A WRAPPED PLAN'S LAUNCH BIT BELONGS TO THE ADAPTER. The line above observes + // the process THIS command started, which for a Windows restricted-token plan + // is the helper, not the requested executable: the helper validates the setup + // marker, applies ACLs, checks the network policy, builds capability SIDs and + // mints the restricted token after it is already running, and any of those can + // fail with no sandboxed child ever created. Believing the outer bit there + // reports that reads were denied as requested when only the unsandboxed + // adapter ran. An adapter that owns the inner transition overrides it; one + // that stays silent leaves the direct-command observation alone. + launched = ResolveChildLaunched(launched, prepared.ChildLaunchOwnedByAdapter, report) result := CapturedResult{ Stdout: stdout.String(), Stderr: stderr.String(), @@ -101,6 +122,7 @@ func (runner *Runner) ExecuteCaptured(ctx context.Context, input CapturedRequest Err: runErr, Outcome: Outcome{ Enforcement: prepared.Enforcement, + Launched: launched, }, } exitCode := commandExitCode(runErr) @@ -157,6 +179,28 @@ func (runner *Runner) Prepare(ctx context.Context, request Request) (PreparedCom return preparer.PrepareExecution(ctx, request) } +// ResolveChildLaunched decides whether the REQUESTED process launched. +// +// ONE IMPLEMENTATION, because every launcher needs the same answer and each one +// that re-derived it got a different one. observed is what the caller saw of the +// process IT started, which for a wrapped plan is the helper and not the +// requested child. +// +// - the adapter stated the fact: believe the adapter, in both directions. +// - the adapter owns the fact and stayed silent: not launched. An absent report +// must not be read as proof that enforcement applied. +// - nobody owns it but the caller: keep the direct observation, which is +// correct for a direct command and for bwrap. +func ResolveChildLaunched(observed bool, ownedByAdapter bool, report AdapterReport) bool { + if report.ChildLaunched != nil { + return *report.ChildLaunched + } + if ownedByAdapter { + return false + } + return observed +} + func capturedSetupFailure(message string, err error, enforcement Enforcement) CapturedResult { return CapturedResult{ Stderr: message, diff --git a/internal/execution/wrapped_launch_state_test.go b/internal/execution/wrapped_launch_state_test.go new file mode 100644 index 000000000..9f4167f39 --- /dev/null +++ b/internal/execution/wrapped_launch_state_test.go @@ -0,0 +1,103 @@ +package execution + +import ( + "context" + "testing" +) + +// A WRAPPER'S START IS NOT THE REQUESTED CHILD'S START. +// +// For a Windows restricted-token plan the command the runner starts is the +// sandbox helper, not the executable the caller asked for. Inside that helper, +// setup-marker validation, unelevated ACL application, network-policy +// validation, capability and offline SID construction, restricted-token creation +// and the CreateProcessAsUser call all happen afterwards, and each of them can +// return with no sandboxed child ever created. exec.Cmd.Process is already +// non-nil by then, so reading the launch state off it reports that reads were +// denied as requested when the only thing that ran was the unsandboxed adapter. +// +// The fact belongs to whoever sees the transition. These pin both directions of +// that boundary. +type wrappedPreparer struct { + script string + owned bool + childLaunched *bool + reportNothing bool +} + +func (p *wrappedPreparer) PrepareExecution(ctx context.Context, _ Request) (PreparedCommand, error) { + prepared := PreparedCommand{ + Command: launchStateShell(ctx, p.script), + Enforcement: Enforcement{Notices: []string{launchStateNotice}}, + ChildLaunchOwnedByAdapter: p.owned, + } + if !p.reportNothing { + launched := p.childLaunched + prepared.Report = func() (AdapterReport, error) { + return AdapterReport{ChildLaunched: launched}, nil + } + } + return prepared, nil +} + +func capturedWrapped(t *testing.T, p *wrappedPreparer) CapturedResult { + t.Helper() + return NewRunner(p).ExecuteCaptured(context.Background(), CapturedRequest{Request: Request{ + Origin: OriginHook, + Mode: ModeCaptured, + Command: Command{Name: "irrelevant"}, + WorkingDirectory: t.TempDir(), + WorkspaceRoots: []string{t.TempDir()}, + Approval: ApprovalContext{PolicyVersion: PolicyVersion}, + }}) +} + +func TestWrappedPlanDisclosesOnlyWhatTheAdapterConfirms(t *testing.T) { + // The helper starts and then fails before it can create the restricted child: + // a bad setup marker, an ACL it could not apply, a network policy it rejected, + // a token it could not mint. The wrapper process exists; the sandboxed one + // never did, so nothing may be claimed about enforcement. + t.Run("helper ran but never created the child", func(t *testing.T) { + no := false + result := capturedWrapped(t, &wrappedPreparer{script: "exit 1", owned: true, childLaunched: &no}) + // The wrapper really did run, which is the whole point: its exit code is + // the script's. Without this the test could pass because nothing executed. + if result.Outcome.Exit == nil || result.Outcome.Exit.Code != 1 { + t.Fatalf("SETUP INVALID: the wrapper itself must have run; outcome = %+v", result.Outcome) + } + if notices := result.Outcome.AppliedEnforcementNotices(); len(notices) != 0 { + t.Fatalf("a helper that never created the restricted child disclosed %q", notices) + } + }) + + // Same shape, but the adapter says nothing at all. Silence from the owner of + // the fact is not permission to fall back to the wrapper's own start. + t.Run("adapter that owns the fact stayed silent", func(t *testing.T) { + result := capturedWrapped(t, &wrappedPreparer{script: "exit 1", owned: true, reportNothing: true}) + if notices := result.Outcome.AppliedEnforcementNotices(); len(notices) != 0 { + t.Fatalf("an unreported child launch was disclosed as applied enforcement: %q", notices) + } + }) + + // And the other side of the boundary: a restricted child that really started + // and then exited non-zero DID run under the disclosed enforcement, so the + // notice must still be made, exactly once. + t.Run("restricted child started, then failed", func(t *testing.T) { + yes := true + result := capturedWrapped(t, &wrappedPreparer{script: "exit 3", owned: true, childLaunched: &yes}) + notices := result.Outcome.AppliedEnforcementNotices() + if len(notices) != 1 || notices[0] != launchStateNotice { + t.Fatalf("a child that ran and then failed disclosed %q, want exactly one %q", notices, launchStateNotice) + } + }) + + // A direct, unwrapped command is unchanged: the process the runner starts IS + // the requested one, so its own observation still decides. + t.Run("direct command keeps its own observation", func(t *testing.T) { + result := capturedWrapped(t, &wrappedPreparer{script: "exit 0", owned: false, reportNothing: true}) + notices := result.Outcome.AppliedEnforcementNotices() + if len(notices) != 1 || notices[0] != launchStateNotice { + t.Fatalf("a direct command that ran disclosed %q, want exactly one %q", notices, launchStateNotice) + } + }) +} diff --git a/internal/hooks/dispatch.go b/internal/hooks/dispatch.go index d5bb13e3c..bb831caa6 100644 --- a/internal/hooks/dispatch.go +++ b/internal/hooks/dispatch.go @@ -36,7 +36,23 @@ type DispatchOutcome struct { // Messages collects the output (stdout, else stderr) of each hook that // produced any, in run order. afterTool validators use this to feed results // (e.g. a formatter diff or vet warning) back to the model on the tool result. + // + // PRESENTATION TEXT, NOT A NOTICE CHANNEL. hookMessage composes the hook's + // ordinary stdout (or stderr) together with any enforcement notices, because + // afterTool wants both on one line. A caller that only wants to know what the + // sandbox did must read Notices instead: delivering this slice would put every + // successful hook's routine logging into the model's context. Messages []string + // Notices carries only the enforcement disclosures, one entry per notice, in + // run order across every hook that ran. + // + // Separate from Messages because they answer different questions and have + // different audiences. A notice says the hook ran under a weakened token, + // which the model and the operator both need; the hook's own output is for + // afterTool validators that asked to be heard. Appended as each hook runs, + // rather than read off the final result, so a disclosure from a hook that + // already ran survives a later hook's veto ending the chain. + Notices []string } type commandResult struct { @@ -45,6 +61,13 @@ type commandResult struct { Stderr string Err error // set when the command could not be executed (not a non-zero exit) TimedOut bool // the hook started but its deadline/cancellation fired before it returned + // Notices carries the enforcement disclosures the execution runner attached. + // + // Same reason as the plugin path: the generic execution contract is not + // transport-only. Enforcement.Notices says what the sandbox actually did, and + // a projection that keeps only stdout, stderr and an exit code drops it, so a + // hook ran under a weakened token with nothing said about it. + Notices []string } // commandRunner executes one hook command. It is injectable so the dispatch @@ -139,6 +162,10 @@ func executionCommandRunner(runner *execution.Runner) commandRunner { Stderr: stderr, Err: commandErr, TimedOut: result.Outcome.Kind == execution.OutcomeTimedOut, + // One shared decision: see Outcome.AppliedEnforcementNotices. A setup + // failure or a missing executable launched no hook child, so the notice + // would describe a token trade nobody made. + Notices: result.Outcome.AppliedEnforcementNotices(), } } } @@ -182,6 +209,14 @@ func (dispatcher *Dispatcher) Dispatch(ctx context.Context, input DispatchInput) if message := hookMessage(result); message != "" { outcome.Messages = append(outcome.Messages, message) } + // Appended per hook rather than read off the final result, so hook A's + // disclosure is not lost when hook B stops the chain. A notice describes + // something that ALREADY happened. + for _, notice := range result.Notices { + if strings.TrimSpace(notice) != "" { + outcome.Notices = append(outcome.Notices, notice) + } + } if blocked { outcome.Blocked = true @@ -244,13 +279,39 @@ func classifyResult(event Event, result commandResult) (AuditStatus, bool) { // hookMessage returns the output worth surfacing from a hook run: stdout when // present, else stderr. Empty when the hook produced no output. func hookMessage(result commandResult) string { - if trimmed := strings.TrimSpace(result.Stdout); trimmed != "" { - return trimmed + message := strings.TrimSpace(result.Stdout) + if message == "" { + message = strings.TrimSpace(result.Stderr) + } + // PREPENDED, and present even when the hook itself said nothing. A hook that + // runs silently under a weakened token is exactly the case where the only + // thing worth surfacing IS the disclosure. + return withHookEnforcementNotices(message, result.Notices) +} + +func withHookEnforcementNotices(message string, notices []string) string { + joined := strings.TrimSpace(strings.Join(notices, "\n")) + if joined == "" { + return message } - return strings.TrimSpace(result.Stderr) + if strings.TrimSpace(message) == "" { + return joined + } + return joined + "\n\n" + message } +// blockReason explains a veto, and carries the enforcement disclosure with it. +// +// THE BLOCKING BRANCH IS THE ONE A USER ALWAYS SEES. hookMessage composes the +// notices into DispatchOutcome.Messages, but a vetoing beforeTool hook builds +// Reason separately and returns immediately, so a hook that blocked an action +// while running without write confinement reported only the veto. Both fields +// reach a person, so both have to carry it. func blockReason(result commandResult) string { + return withHookEnforcementNotices(blockCause(result), result.Notices) +} + +func blockCause(result commandResult) string { if result.TimedOut { if trimmed := strings.TrimSpace(result.Stderr); trimmed != "" { return "hook timed out: " + trimmed @@ -288,7 +349,14 @@ func (dispatcher *Dispatcher) recordCompleted(hook Definition, input DispatchInp Matcher: hook.Matcher, ToolCallID: input.ToolCallID, Status: status, - Results: []AuditResult{{ExitCode: result.ExitCode, Stdout: result.Stdout, Stderr: result.Stderr}}, + Results: []AuditResult{{ + ExitCode: result.ExitCode, + Stdout: result.Stdout, + Stderr: result.Stderr, + // The notice is not in stdout or stderr by design, so the durable record + // has to carry it or the fact ends with the dispatch result. + EnforcementNotices: append([]string(nil), result.Notices...), + }}, DurationMs: durationMs, }) } diff --git a/internal/hooks/enforcement_audit_record_test.go b/internal/hooks/enforcement_audit_record_test.go new file mode 100644 index 000000000..279aba069 --- /dev/null +++ b/internal/hooks/enforcement_audit_record_test.go @@ -0,0 +1,119 @@ +package hooks + +import ( + "context" + "os/exec" + "path/filepath" + "testing" +) + +// auditedDispatcher wires a real audit store to a dispatcher whose hook result +// is whatever the caller wants, and returns the events that survived the write. +func auditedDispatcher(t *testing.T, hook Definition, result commandResult) []AuditEvent { + t.Helper() + store, err := NewAuditStore(AuditStoreOptions{AuditPath: filepath.Join(t.TempDir(), "audit.jsonl")}) + if err != nil { + t.Fatalf("NewAuditStore: %v", err) + } + dispatcher := NewDispatcher(DispatcherOptions{ + Config: beforeToolConfig(hook), + Audit: store, + run: func(context.Context, string, []string, []byte, string, []string) commandResult { + return result + }, + }) + dispatcher.Dispatch(context.Background(), DispatchInput{Event: EventBeforeTool, ToolName: "bash", ToolCallID: "call_1"}) + + // READ BACK FROM DISK, not from the in-memory event the append returned. The + // durable reader is the consumer this field exists for. + events, err := store.ReadEvents() + if err != nil { + t.Fatalf("ReadEvents: %v", err) + } + return events +} + +func completedResults(t *testing.T, events []AuditEvent) []AuditResult { + t.Helper() + for _, event := range events { + if len(event.Results) > 0 { + return event.Results + } + } + t.Fatalf("no completed record was written: %#v", events) + return nil +} + +// THE TRANSIENT DISPATCH RESULT IS NOT WHERE THIS FACT CAN LIVE. +// +// recordCompleted kept an exit code, stdout and stderr, and the notice is +// deliberately in none of those. So once the dispatch result was gone, an audit +// or recovery reader could not tell that a hook had run under the weakened +// DenyRead token, whatever the hook did afterwards. +func TestTheAuditRecordKeepsTheEnforcementNotice(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + + for _, testCase := range []struct { + name string + result commandResult + }{ + {"launched and succeeded", commandResult{ExitCode: 0, Stdout: "looks fine", Notices: []string{notice}}}, + {"vetoed the tool", commandResult{ExitCode: 2, Stderr: "policy violation", Notices: []string{notice}}}, + {"silent hook", commandResult{ExitCode: 0, Notices: []string{notice}}}, + } { + t.Run(testCase.name, func(t *testing.T) { + results := completedResults(t, auditedDispatcher(t, + Definition{ID: "policy", Event: EventBeforeTool, Command: "policy-check", Enabled: true}, + testCase.result)) + if len(results) != 1 { + t.Fatalf("results = %#v, want one", results) + } + if len(results[0].EnforcementNotices) != 1 || results[0].EnforcementNotices[0] != notice { + t.Errorf("the durable record lost the disclosure: %#v", results[0]) + } + // The existing semantics are untouched. + if results[0].ExitCode != testCase.result.ExitCode { + t.Errorf("ExitCode = %d, want %d", results[0].ExitCode, testCase.result.ExitCode) + } + if results[0].Stdout != testCase.result.Stdout || results[0].Stderr != testCase.result.Stderr { + t.Errorf("stdout/stderr changed: %#v", results[0]) + } + }) + } +} + +// A hook with nothing to disclose writes exactly what it wrote before, so a +// reader of historical records sees no difference. +func TestAnOrdinaryHookWritesNoEnforcementField(t *testing.T) { + results := completedResults(t, auditedDispatcher(t, + Definition{ID: "policy", Event: EventBeforeTool, Command: "policy-check", Enabled: true}, + commandResult{ExitCode: 0, Stdout: "looks fine"})) + if len(results[0].EnforcementNotices) != 0 { + t.Errorf("a hook with no disclosure recorded one: %#v", results[0]) + } +} + +// And the durable record inherits the launch-state rule rather than restating +// it: a hook that never started records no enforcement claim. +func TestTheAuditRecordMakesNoClaimForAHookThatNeverLaunched(t *testing.T) { + store, err := NewAuditStore(AuditStoreOptions{AuditPath: filepath.Join(t.TempDir(), "audit.jsonl")}) + if err != nil { + t.Fatalf("NewAuditStore: %v", err) + } + dispatcher := NewDispatcher(DispatcherOptions{ + Config: beforeToolConfig(Definition{ID: "policy", Event: EventBeforeTool, Command: "policy-check", Enabled: true}), + Audit: store, + Cwd: t.TempDir(), + Execution: newRunnerFor(¬icePreparer{build: func() *exec.Cmd { return exec.Command("definitely-not-a-real-binary-zzz") }}), + }) + dispatcher.Dispatch(context.Background(), DispatchInput{Event: EventBeforeTool, ToolName: "bash", ToolCallID: "call_1"}) + events, err := store.ReadEvents() + if err != nil { + t.Fatalf("ReadEvents: %v", err) + } + for _, result := range completedResults(t, events) { + if len(result.EnforcementNotices) != 0 { + t.Errorf("the durable record claims an enforcement trade for a hook that never started: %#v", result) + } + } +} diff --git a/internal/hooks/enforcement_launch_sleep_unix_test.go b/internal/hooks/enforcement_launch_sleep_unix_test.go new file mode 100644 index 000000000..d10862a45 --- /dev/null +++ b/internal/hooks/enforcement_launch_sleep_unix_test.go @@ -0,0 +1,6 @@ +//go:build !windows + +package hooks + +// sleepScript keeps a launched child alive long enough for a timeout to fire. +const sleepScript = "sleep 2" diff --git a/internal/hooks/enforcement_launch_sleep_windows_test.go b/internal/hooks/enforcement_launch_sleep_windows_test.go new file mode 100644 index 000000000..2cdda5542 --- /dev/null +++ b/internal/hooks/enforcement_launch_sleep_windows_test.go @@ -0,0 +1,6 @@ +package hooks + +// sleepScript keeps a launched child alive long enough for a timeout to fire. +// Paired with the !windows file of the same name; both must exist or the +// platform without one silently loses the timeout case. +const sleepScript = "ping -n 3 127.0.0.1 > NUL" diff --git a/internal/hooks/enforcement_launch_state_test.go b/internal/hooks/enforcement_launch_state_test.go new file mode 100644 index 000000000..d0cc38361 --- /dev/null +++ b/internal/hooks/enforcement_launch_state_test.go @@ -0,0 +1,169 @@ +package hooks + +import ( + "context" + "errors" + "os/exec" + "runtime" + "strings" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/execution" +) + +const launchStateNotice = "denyRead is configured, so the write jail is not confining writes" + +// shellCommand builds a portable child that the platform can actually launch. +func shellCommand(script string) *exec.Cmd { + if runtime.GOOS == "windows" { + return exec.Command("cmd.exe", "/c", script) + } + return exec.Command("/bin/sh", "-c", script) +} + +// noticePreparer plans a command carrying an enforcement notice, and can fail +// the way the sandbox does before the child exists. +type noticePreparer struct { + prepareErr error + build func() *exec.Cmd +} + +func (preparer *noticePreparer) PrepareExecution(_ context.Context, request execution.Request) (execution.PreparedCommand, error) { + if preparer.prepareErr != nil { + return execution.PreparedCommand{}, preparer.prepareErr + } + command := preparer.build + if command == nil { + command = func() *exec.Cmd { return exec.Command(request.Command.Name, request.Command.Args...) } + } + return execution.PreparedCommand{ + Command: command(), + Enforcement: execution.Enforcement{Notices: []string{launchStateNotice}}, + }, nil +} + +// PLANNING A WRAPPED COMMAND IS NOT PROOF THAT ANYTHING RAN. +// +// Enforcement.Notices describes the shape the command was PREPARED to run +// under. Copying it straight out made the completed-enforcement claim for +// commands that never existed: a sandbox setup failure and a missing executable +// are both decided before the child launches, so the hook message told the +// operator the write jail had been traded away for a process that never +// started. +// +// Everything after launch keeps the disclosure, including a nonzero exit, a +// timeout and a cancellation: those happened to a child that really did run +// under that token. +// +// Driven through the execution runner rather than a hand-built commandResult, +// because the projection is the thing under test. +func TestTheHookRunnerOnlyDisclosesEnforcementForAChildThatLaunched(t *testing.T) { + for _, testCase := range []struct { + name string + preparer *noticePreparer + timeout time.Duration + wantNotice bool + wantTimedOut bool + }{ + { + name: "sandbox setup failed before the child existed", + preparer: ¬icePreparer{prepareErr: errors.New("could not build the restricted token")}, + wantNotice: false, + }, + { + name: "the executable was never found", + preparer: ¬icePreparer{build: func() *exec.Cmd { + return exec.Command("definitely-not-a-real-binary-zzz") + }}, + wantNotice: false, + }, + { + name: "the child launched and succeeded", + preparer: ¬icePreparer{build: func() *exec.Cmd { return shellCommand("exit 0") }}, + wantNotice: true, + }, + { + name: "the child launched and exited nonzero", + preparer: ¬icePreparer{build: func() *exec.Cmd { return shellCommand("exit 3") }}, + wantNotice: true, + }, + { + name: "the child launched and timed out", + preparer: ¬icePreparer{build: func() *exec.Cmd { return shellCommand(sleepScript) }}, + timeout: 150 * time.Millisecond, + wantNotice: true, + wantTimedOut: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + ctx := context.Background() + if testCase.timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, testCase.timeout) + defer cancel() + } + run := executionCommandRunner(execution.NewRunner(testCase.preparer)) + result := run(ctx, "hook-command", nil, nil, t.TempDir(), nil) + + if result.TimedOut != testCase.wantTimedOut { + t.Fatalf("TimedOut = %v, want %v: the case did not reach the outcome kind it is named for", result.TimedOut, testCase.wantTimedOut) + } + if got := len(result.Notices) > 0; got != testCase.wantNotice { + t.Fatalf("notices present = %v, want %v: %#v", got, testCase.wantNotice, result.Notices) + } + message := hookMessage(result) + if testCase.wantNotice && !strings.Contains(message, launchStateNotice) { + t.Errorf("a launched child lost its disclosure:\n%s", message) + } + if !testCase.wantNotice && strings.Contains(message, launchStateNotice) { + t.Errorf("a child that never launched claimed the token was traded away:\n%s", message) + } + }) + } +} + +// And the same rule has to hold on the veto path, which builds its reason +// separately and is what the model actually sees. +func TestAVetoingHookThatNeverLaunchedClaimsNoEnforcement(t *testing.T) { + dispatcher := NewDispatcher(DispatcherOptions{ + Config: beforeToolConfig(Definition{ID: "policy", Event: EventBeforeTool, Command: "policy-check", Enabled: true}), + Cwd: t.TempDir(), + // A missing executable rather than a prepare error: a prepare error never + // builds the PreparedCommand, so its outcome carries no planned notice and + // the assertion below would hold with the launch gate deleted. This shape + // plans the notice and then fails to launch. + Execution: execution.NewRunner(¬icePreparer{build: func() *exec.Cmd { + return exec.Command("definitely-not-a-real-binary-zzz") + }}), + }) + outcome := dispatcher.Dispatch(context.Background(), DispatchInput{Event: EventBeforeTool, ToolName: "bash"}) + if !outcome.Blocked { + t.Fatal("SETUP INVALID: a beforeTool hook that could not run must fail closed, or the veto path is not exercised") + } + if strings.Contains(outcome.Reason, launchStateNotice) { + t.Errorf("the veto reason claims an enforcement trade for a hook that never started:\n%s", outcome.Reason) + } +} + +// A launched hook still carries it all the way into the dispatch outcome. +func TestALaunchedHookCarriesTheNoticeIntoTheDispatchOutcome(t *testing.T) { + dispatcher := NewDispatcher(DispatcherOptions{ + Config: beforeToolConfig(Definition{ID: "policy", Event: EventBeforeTool, Command: "policy-check", Enabled: true}), + Cwd: t.TempDir(), + Execution: execution.NewRunner(¬icePreparer{build: func() *exec.Cmd { return shellCommand("exit 2") }}), + }) + outcome := dispatcher.Dispatch(context.Background(), DispatchInput{Event: EventBeforeTool, ToolName: "bash"}) + if !outcome.Blocked { + t.Fatal("SETUP INVALID: the hook did not veto, so the reason path is not exercised") + } + if !strings.Contains(outcome.Reason, launchStateNotice) { + t.Errorf("a hook that really ran under the weakened token disclosed nothing:\n%s", outcome.Reason) + } +} + +// newRunnerFor keeps the audit tests readable without importing the execution +// package into every file that needs one. +func newRunnerFor(preparer *noticePreparer) *execution.Runner { + return execution.NewRunner(preparer) +} diff --git a/internal/hooks/enforcement_notice_test.go b/internal/hooks/enforcement_notice_test.go new file mode 100644 index 000000000..2a29a5002 --- /dev/null +++ b/internal/hooks/enforcement_notice_test.go @@ -0,0 +1,89 @@ +package hooks + +import ( + "context" + "strings" + "testing" +) + +// Same contract on the hook path. The projection kept stdout, stderr and an exit +// code and dropped the enforcement notices, so a hook ran under the weakened +// token silently. +func TestAHookSurfacesTheEnforcementNotice(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + + for _, testCase := range []struct { + name string + result commandResult + want string + }{ + {"hook printed nothing", commandResult{ExitCode: 0, Notices: []string{notice}}, notice}, + {"hook printed to stdout", commandResult{ExitCode: 0, Stdout: "looks fine", Notices: []string{notice}}, notice}, + {"hook printed to stderr only", commandResult{ExitCode: 0, Stderr: "a warning", Notices: []string{notice}}, notice}, + } { + t.Run(testCase.name, func(t *testing.T) { + message := hookMessage(testCase.result) + if !strings.Contains(message, testCase.want) { + t.Fatalf("the hook message does not carry the notice:\n%s", message) + } + if strings.Count(message, testCase.want) != 1 { + t.Errorf("the notice appears %d times, want exactly once:\n%s", strings.Count(message, testCase.want), message) + } + }) + } +} + +// A hook with no notice reads exactly as it did before. +func TestAHookWithoutANoticeIsUnchanged(t *testing.T) { + if message := hookMessage(commandResult{ExitCode: 0, Stdout: "looks fine"}); message != "looks fine" { + t.Errorf("hookMessage = %q, want the hook's own output untouched", message) + } + if message := hookMessage(commandResult{ExitCode: 0}); message != "" { + t.Errorf("a silent hook with no notice produced %q", message) + } +} + +// THROUGH Dispatch, NOT A HAND-BUILT commandResult. +// +// The blocking branch builds DispatchOutcome.Reason with blockReason and returns +// immediately, so it never touches hookMessage. A vetoing beforeTool hook that +// ran without write confinement reported only the veto, and Reason is the field +// the agent turns into the model-visible result. +func TestABlockedBeforeToolHookCarriesTheNoticeIntoItsReason(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + + dispatcher := NewDispatcher(DispatcherOptions{ + Config: beforeToolConfig(Definition{ID: "policy", Event: EventBeforeTool, Command: "policy-check", Enabled: true}), + run: func(context.Context, string, []string, []byte, string, []string) commandResult { + return commandResult{ExitCode: 2, Stderr: "policy violation", Notices: []string{notice}} + }, + }) + + outcome := dispatcher.Dispatch(context.Background(), DispatchInput{Event: EventBeforeTool, ToolName: "bash"}) + if !outcome.Blocked { + t.Fatal("SETUP INVALID: the hook did not block, so the blocking branch was never taken") + } + if !strings.Contains(outcome.Reason, notice) { + t.Errorf("the veto reason lost the enforcement notice:\n%s", outcome.Reason) + } + if !strings.Contains(outcome.Reason, "policy violation") { + t.Errorf("the veto reason lost the hook's own explanation:\n%s", outcome.Reason) + } + if strings.Count(outcome.Reason, notice) != 1 { + t.Errorf("the notice appears %d times in the reason, want once:\n%s", strings.Count(outcome.Reason, notice), outcome.Reason) + } +} + +// And a veto with no notice reads exactly as it did before. +func TestABlockedHookWithoutANoticeIsUnchanged(t *testing.T) { + dispatcher := NewDispatcher(DispatcherOptions{ + Config: beforeToolConfig(Definition{ID: "policy", Event: EventBeforeTool, Command: "policy-check", Enabled: true}), + run: func(context.Context, string, []string, []byte, string, []string) commandResult { + return commandResult{ExitCode: 2, Stderr: "policy violation"} + }, + }) + outcome := dispatcher.Dispatch(context.Background(), DispatchInput{Event: EventBeforeTool, ToolName: "bash"}) + if outcome.Reason != "policy violation" { + t.Errorf("Reason = %q, want the hook's own explanation untouched", outcome.Reason) + } +} diff --git a/internal/hooks/hooks.go b/internal/hooks/hooks.go index f7dd79cea..bc8ee07ff 100644 --- a/internal/hooks/hooks.go +++ b/internal/hooks/hooks.go @@ -118,6 +118,19 @@ type AuditResult struct { ExitCode int `json:"exitCode"` Stdout string `json:"stdout,omitempty"` Stderr string `json:"stderr,omitempty"` + // EnforcementNotices are the least-privilege disclosures that were true of + // this hook's execution. + // + // A DURABLE READER CANNOT RECOVER A FACT THAT WAS DROPPED IN CONVERSION. The + // notice is deliberately not written into stdout or stderr, so recording only + // those three fields meant that once the transient dispatch result was gone, + // nothing could tell an audit or recovery reader that a successful, failing or + // vetoing hook had run under the weakened DenyRead token. + // + // Typed rather than a rendered line, and omitempty, so historical records that + // predate the field read back unchanged and an ordinary hook writes exactly + // what it wrote before. + EnforcementNotices []string `json:"enforcementNotices,omitempty"` } type AuditEvent struct { diff --git a/internal/mcp/adapter_launch_disclosure_test.go b/internal/mcp/adapter_launch_disclosure_test.go new file mode 100644 index 000000000..10c8d298d --- /dev/null +++ b/internal/mcp/adapter_launch_disclosure_test.go @@ -0,0 +1,163 @@ +package mcp + +import ( + "context" + "encoding/json" + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/execution" + "github.com/Gitlawb/zero/internal/tools" +) + +// helperCommandName is a command that resolves on this platform, so registration +// reaches connectStdio. What actually runs is whatever the preparer returns. +func helperCommandName() string { + if runtime.GOOS == "windows" { + return "cmd.exe" + } + return "sh" +} + +const adapterLaunchNotice = "denyRead is configured, so the write jail is not confining writes" + +// adapterHelperPreparer models the Windows wrapped plan: the command is the +// HELPER, the launch fact is a report file, and the plan's cleanup removes that +// file. The cleanup is the part that matters, because it is what runs inside +// client.Close and destroys the evidence a later decision needs. +type adapterHelperPreparer struct { + reportPath string + reportBody string + writeReport bool + called bool +} + +func (preparer *adapterHelperPreparer) PrepareExecution(_ context.Context, _ execution.Request) (execution.PreparedCommand, error) { + preparer.called = true + if preparer.writeReport { + if err := os.WriteFile(preparer.reportPath, []byte(preparer.reportBody), 0o600); err != nil { + return execution.PreparedCommand{}, err + } + } + // A helper that starts, says nothing an MCP client understands, and exits, so + // the handshake fails the way it does when the requested server never existed. + var command *exec.Cmd + if runtime.GOOS == "windows" { + command = exec.Command("cmd.exe", "/c", "exit 0") + } else { + command = exec.Command("/bin/sh", "-c", "exit 0") + } + path := preparer.reportPath + return execution.PreparedCommand{ + Command: command, + ChildLaunchOwnedByAdapter: true, + Enforcement: execution.Enforcement{Notices: []string{adapterLaunchNotice}}, + Report: func() (execution.AdapterReport, error) { + raw, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return execution.AdapterReport{}, nil + } + if err != nil { + return execution.AdapterReport{}, err + } + var report execution.AdapterReport + if err := json.Unmarshal(raw, &report); err != nil { + return execution.AdapterReport{}, err + } + return report, nil + }, + Cleanup: func() { _ = os.Remove(path) }, + }, nil +} + +func registerWithAdapterHelper(t *testing.T, preparer *adapterHelperPreparer) *Runtime { + t.Helper() + runtime, err := RegisterTools(context.Background(), tools.NewRegistry(), config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: helperCommandName()}, + }}, RegisterOptions{ + // No ClientFactory on purpose: an injected factory skips connectStdio + // entirely, which is where the decision under test is made, and the test + // would pass with the fix removed. + Execution: execution.NewRunner(preparer), + WorkspaceRoot: t.TempDir(), + }) + if err != nil { + t.Fatalf("RegisterTools: %v", err) + } + t.Cleanup(func() { _ = runtime.Close() }) + return runtime +} + +// A HELPER THAT NEVER CREATED THE SERVER HAS NOTHING TO DISCLOSE. +// +// For an adapter-owned launch, cmd.Start proves only that the sandbox helper +// started. It can then fail setup-marker validation, ACL application, network +// validation, token construction, or CreateProcessAsUser without ever creating +// the requested MCP server. The launch sink already made that distinction; the +// initialize-error path did not, and carried the planned notices out +// unconditionally. The operator was told the server had run without write +// confinement when no server had run at all. +func TestAnMCPHelperThatReportedNoChildDisclosesNothing(t *testing.T) { + directory := t.TempDir() + preparer := &adapterHelperPreparer{ + reportPath: filepath.Join(directory, "report.json"), + reportBody: `{"childLaunched":false}`, + writeReport: true, + } + runtime := registerWithAdapterHelper(t, preparer) + + // SETUP: the attempt really did fail, or there is no disclosure decision here. + if len(runtime.Skipped()) == 0 { + t.Fatal("SETUP INVALID: the server connected, so the initialize-failure path is not under test") + } + if got := runtime.StartupDisclosures(); len(got) != 0 { + t.Fatalf("a helper that reported no child announced %v; no server ran, confined or otherwise", got) + } +} + +// AND ONE THAT DID CREATE IT DISCLOSES ONCE. +// +// The companion case, and the one that keeps the assertion above from being +// satisfied by a path that discloses nothing ever. The child ran under the +// planned token and may have done filesystem work before the handshake failed, +// so the disclosure has to survive the failure. +func TestAnMCPHelperThatLaunchedTheChildDisclosesOnce(t *testing.T) { + directory := t.TempDir() + preparer := &adapterHelperPreparer{ + reportPath: filepath.Join(directory, "report.json"), + reportBody: `{"childLaunched":true}`, + writeReport: true, + } + runtime := registerWithAdapterHelper(t, preparer) + + if !preparer.called { + t.Fatal("SETUP INVALID: the preparer never ran, so connectStdio was never reached") + } + disclosures := runtime.StartupDisclosures() + if len(disclosures) != 1 { + t.Fatalf("a server that really ran under the weakened token produced %d disclosures, want exactly one: %v", len(disclosures), disclosures) + } + if len(disclosures[0].Notices) != 1 || disclosures[0].Notices[0] != adapterLaunchNotice { + t.Fatalf("the disclosure carries %v, want the one planned notice", disclosures[0].Notices) + } +} + +// A helper that wrote no report at all is the same answer as one that reported +// no child: absence is not confirmation. +func TestAnMCPHelperThatWroteNoReportDisclosesNothing(t *testing.T) { + directory := t.TempDir() + preparer := &adapterHelperPreparer{reportPath: filepath.Join(directory, "report.json")} + runtime := registerWithAdapterHelper(t, preparer) + + if len(runtime.Skipped()) == 0 { + t.Fatal("SETUP INVALID: the server connected, so the initialize-failure path is not under test") + } + if got := runtime.StartupDisclosures(); len(got) != 0 { + t.Fatalf("a helper that published nothing announced %v", got) + } +} diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 064e7f213..964eba0a4 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -43,6 +43,45 @@ type ToolClient interface { Close() error } +// startupDisclosureError carries a launch disclosure out through a failure. +// +// A launched process is a fact about the past: once Start has succeeded the +// disclosure is true whatever the handshake does next. The client is the only +// thing that holds it, and the failure paths close and discard the client, so +// without this the fact dies with the connection it was attached to. +type startupDisclosureError struct { + err error + notices []string +} + +func (e *startupDisclosureError) Error() string { return e.err.Error() } +func (e *startupDisclosureError) Unwrap() error { return e.err } + +// startupNoticesFromError recovers a launch disclosure from a failed connect. +func startupNoticesFromError(err error) []string { + var disclosure *startupDisclosureError + if errors.As(err, &disclosure) { + return disclosure.notices + } + return nil +} + +// startupDisclosing is the optional interface a client implements when its +// LAUNCH carried a least-privilege disclosure. A network server launches no +// local process, so it does not implement this and reports nothing, which is the +// correct answer rather than an empty one. +type startupDisclosing interface { + StartupNotices() []string +} + +// StartupNotices reports the disclosures that applied to this server's launch. +func (client *Client) StartupNotices() []string { + if client == nil || len(client.startupNotices) == 0 { + return nil + } + return append([]string(nil), client.startupNotices...) +} + type Client struct { server Server cmd *exec.Cmd @@ -53,6 +92,16 @@ type Client struct { closeMu sync.Mutex nextID int cleanup func() + // startupNotices are the least-privilege disclosures that applied to THIS + // server's launch. + // + // THE FACT DESCRIBES STARTUP, SO IT CANNOT BE RECOVERED FROM A TOOL RESULT. + // A stdio server prepared under the weakened token runs for the whole session, + // and connectStdio used to keep only the command and its cleanup, so nothing + // downstream could tell the operator that the process serving these tools had + // reduced write confinement. Kept typed here and rendered exactly once at + // registration rather than pasted onto every later tool result. + startupNotices []string // dispatchMu guards the response-dispatch state shared with the single // reader goroutine. It is never held across a blocking read. @@ -139,6 +188,11 @@ func (b *boundedBuffer) String() string { func connectStdio(ctx context.Context, server Server, options ConnectOptions) (*Client, error) { var cmd *exec.Cmd var cleanup func() + var plannedEnforcement execution.Enforcement + // Retained from the prepared plan rather than dropped: for a wrapped plan the + // adapter, not cmd.Start, owns whether the requested server process exists. + var adapterReport func() (execution.AdapterReport, error) + var ownedLaunch bool cleanupTransferred := false defer func() { if cleanup != nil && !cleanupTransferred { @@ -163,6 +217,9 @@ func connectStdio(ctx context.Context, server Server, options ConnectOptions) (* } cmd = prepared.Command cleanup = prepared.Cleanup + plannedEnforcement = prepared.Enforcement + adapterReport = prepared.Report + ownedLaunch = prepared.ChildLaunchOwnedByAdapter } else { cmd = exec.CommandContext(ctx, server.Command, server.Args...) cmd.Env = mergeProcessEnv(server.Env) @@ -181,6 +238,61 @@ func connectStdio(ctx context.Context, server Server, options ConnectOptions) (* return nil, fmt.Errorf("start MCP server %s: %w", server.Name, err) } + // PUBLISHED AT START, not at return. Registration abandons a server that + // exceeds the connect timeout, and everything below this line (initialize, and + // tools/list above it in the caller) can hang past that. Announcing the launch + // here is what lets an abandoned attempt still disclose the confinement its + // process ran under. + // + // EXCEPT WHEN START IS NOT THE LAUNCH. For a wrapped plan the process started + // above is the sandbox helper, which creates the MCP server only after marker, + // ACL, network, SID and token setup, any of which can fail leaving no server at + // all. Publishing here would make the durable delivery machinery reliably + // announce a confinement nothing ever ran under. Those plans publish from + // publishAdapterLaunch below, once the adapter has stated the fact. + if !ownedLaunch { + publishLaunch(ctx, plannedEnforcement.Notices) + } + // publishAdapterLaunch announces a wrapped plan's launch, but only if the + // adapter confirms the requested child was created. Called on both ways this + // attempt can end, which is also where an attempt abandoned at the connect + // timeout eventually arrives, so a late disclosure is still delivered once. + // ONE ANSWER, READ WHILE THE EVIDENCE STILL EXISTS, USED BY EVERY OUTCOME. + // + // The adapter's report is a file the plan's cleanup deletes. client.Close runs + // that cleanup, so a decision made after Close reads an absent report and + // answers "no child" about a server that really did run. Resolving once and + // memoizing removes the ordering hazard rather than documenting it. + // + // It also gives the success, late and failed paths the same input. The failure + // path used to carry client.StartupNotices() unconditionally while the sink was + // gated on the adapter, which is two competing definitions of applied + // enforcement: an operator was told a server ran without write confinement when + // only the sandbox helper ran and the requested server never existed. + launchedOnce := sync.OnceValue(func() bool { + if !ownedLaunch { + return true + } + if adapterReport == nil { + return false + } + report, err := adapterReport() + if err != nil { + return false + } + return execution.ResolveChildLaunched(true, ownedLaunch, report) + }) + // publishAdapterLaunch announces a wrapped plan's launch, but only if the + // adapter confirms the requested child was created. Called on both ways this + // attempt can end, which is also where an attempt abandoned at the connect + // timeout eventually arrives, so a late disclosure is still delivered once. + publishAdapterLaunch := func() { + if !ownedLaunch || !launchedOnce() { + return + } + publishLaunch(ctx, plannedEnforcement.Notices) + } + client := &Client{ server: server, cmd: cmd, @@ -189,16 +301,39 @@ func connectStdio(ctx context.Context, server Server, options ConnectOptions) (* writer: newMessageWriter(stdin), nextID: 1, cleanup: cleanup, + // Recorded only now, AFTER Start returned. Everything above returns early, + // so a prepare failure or an executable that could not be launched records + // nothing: same launch-state rule hooks and plugins use, expressed by where + // this assignment sits rather than by another outcome-kind switch. + startupNotices: append([]string(nil), plannedEnforcement.Notices...), } cleanupTransferred = true if err := client.initialize(ctx); err != nil { + // THE LAUNCH ALREADY HAPPENED, so the fact has to leave through the error. + // Start succeeded above, which means the process ran under the planned token + // and may have done filesystem work before the handshake failed. Returning a + // bare error discards the client, and with it the only carrier the notices + // had, so the operator was told the server was unavailable and not that it + // had already run without the write jail. + // BEFORE Close, which runs the cleanup that deletes the report. + launched := launchedOnce() _ = client.Close() + publishAdapterLaunch() message := strings.TrimSpace(stderr.String()) + failure := fmt.Errorf("initialize MCP server %s: %w", server.Name, err) if message != "" { - return nil, fmt.Errorf("initialize MCP server %s: %w: %s", server.Name, err, message) + failure = fmt.Errorf("initialize MCP server %s: %w: %s", server.Name, err, message) + } + // Same decision as the sink above. For a wrapped plan whose helper started + // and then failed before creating the requested server, there is nothing to + // disclose: no server ran, confined or otherwise. + var carried []string + if launched { + carried = client.StartupNotices() } - return nil, fmt.Errorf("initialize MCP server %s: %w", server.Name, err) + return nil, &startupDisclosureError{err: failure, notices: carried} } + publishAdapterLaunch() return client, nil } diff --git a/internal/mcp/enforcement_notice_server_test.go b/internal/mcp/enforcement_notice_server_test.go new file mode 100644 index 000000000..2f8b9cfa0 --- /dev/null +++ b/internal/mcp/enforcement_notice_server_test.go @@ -0,0 +1,105 @@ +package mcp + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/tools" +) + +// A MODEL-FACING PROTOCOL BOUNDARY IS A PRESENTATION CONSUMER. +// +// This branch changed the result contract: Result.Output holds the UNDECORATED +// base text and ModelOutput is the sole model-facing projection that composes it +// with the typed enforcement notices. tools/call serialized Output directly, +// which was a complete value before and is not one now, so an affected Windows +// command reached an MCP client with its ordinary output and no statement that +// its DenyRead token shape left writes unconfined. +// +// Driven through Serve rather than the accessor, because the question is what +// goes on the wire. +func TestMCPToolsCallCarriesTheEnforcementNotice(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + const output = "ran the command" + + for _, testCase := range []struct { + name string + result tools.Result + wantText string + wantIsErr bool + wantNotice bool + }{ + { + name: "successful command with a notice", + result: tools.Result{Status: tools.StatusOK, Output: output, EnforcementNotices: []string{notice}}, + wantIsErr: false, + wantNotice: true, + }, + { + name: "failed command with a notice", + result: tools.Result{Status: tools.StatusError, Output: output, EnforcementNotices: []string{notice}}, + wantIsErr: true, + wantNotice: true, + }, + { + name: "ordinary command with no notice", + result: tools.Result{Status: tools.StatusOK, Output: output}, + wantIsErr: false, + wantNotice: false, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + registry := tools.NewRegistry() + registry.Register(serverFakeTool{ + name: "run_thing", + description: "runs a thing", + parameters: tools.Schema{Type: "object", AdditionalProperties: false}, + safety: tools.Safety{SideEffect: tools.SideEffectRead, Permission: tools.PermissionAllow, Reason: "test"}, + run: func(map[string]any) tools.Result { return testCase.result }, + }) + + var input bytes.Buffer + writeServerTestMessage(t, &input, rpcMessage{ID: 1, Method: "initialize"}) + writeServerTestMessage(t, &input, rpcMessage{Method: "notifications/initialized"}) + writeServerTestMessage(t, &input, rpcMessage{ + ID: 2, + Method: "tools/call", + Params: mustRaw(map[string]any{"name": "run_thing", "arguments": map[string]any{}}), + }) + + var out bytes.Buffer + if err := Serve(context.Background(), &input, &out, registry, ServeOptions{Name: "zero-test", Version: "1.2.3"}); err != nil { + t.Fatalf("Serve() error = %v", err) + } + reader := newMessageReader(&out) + readServerTestMessage(t, reader) // initialize + var call CallToolResult + decodeServerTestResult(t, readServerTestMessage(t, reader), &call) + + if len(call.Content) != 1 || call.Content[0].Type != "text" { + t.Fatalf("content shape changed: %#v", call.Content) + } + text := call.Content[0].Text + if call.IsError != testCase.wantIsErr { + t.Errorf("IsError = %v, want %v", call.IsError, testCase.wantIsErr) + } + if count := strings.Count(text, output); count != 1 { + t.Errorf("the command's own output appears %d times, want exactly 1: %q", count, text) + } + gotNotice := strings.Count(text, notice) + if testCase.wantNotice && gotNotice != 1 { + t.Errorf("the disclosure appears %d times, want exactly 1: %q", gotNotice, text) + } + if !testCase.wantNotice { + if gotNotice != 0 { + t.Errorf("a disclosure appeared for a command that had none: %q", text) + } + if text != output { + t.Errorf("ordinary output was altered: got %q, want %q", text, output) + } + } + }) + } +} diff --git a/internal/mcp/launch_sink.go b/internal/mcp/launch_sink.go new file mode 100644 index 000000000..a3149101a --- /dev/null +++ b/internal/mcp/launch_sink.go @@ -0,0 +1,118 @@ +package mcp + +import ( + "context" + "sync" +) + +// launchSink carries the fact that a server's PROCESS STARTED out of the connect +// attempt, without waiting for that attempt to finish. +// +// The startup notices used to leave connectStdio only on the returned client, or +// on the returned error. Both require the attempt to return. Registration +// abandons a server that exceeds the connect timeout, so a server that started +// under the reduced write confinement and then hung in initialize or tools/list +// was recorded as skipped with nothing said about the confinement it ran under. +// The reaper that later collects the abandoned attempt runs after the serial +// commit phase has finished, so it cannot contribute without breaking the +// deterministic ordering that phase exists to provide. +// +// Publishing at Start splits the two facts apart, which is the point: launch and +// connection usability have different lifetimes. A sink that was never published +// to means Start never happened, so prepare, pipe, and Start failures stay silent +// exactly as before. +// +// IT IS ALSO AN EVENT, NOT ONLY A VALUE. A retained sink that nobody re-reads is +// still a lost disclosure: both production reporters sample once, immediately +// after registration returns, and a Start that completes after that sample had +// no way to reach them. onPublish lets a reporter subscribe; if the launch has +// already happened by the time it subscribes, it is told at once, so the fact +// reaches exactly one presentation regardless of which side won the race. +type launchSink struct { + mu sync.Mutex + launched bool + notices []string + onPublish func(notices []string) + delivered bool +} + +type launchSinkKey struct{} + +// withLaunchSink attaches a sink to the context handed to the client factory. +// Carried on the context rather than added to the factory signature so an +// injected or third-party factory that knows nothing about it still works, and +// simply discloses nothing. +func withLaunchSink(ctx context.Context, sink *launchSink) context.Context { + return context.WithValue(ctx, launchSinkKey{}, sink) +} + +// publishLaunch records that the process for this connect attempt has started, +// along with the enforcement notices that applied to it. Safe on a context with +// no sink, which is every caller outside registration. +func publishLaunch(ctx context.Context, notices []string) { + sink, _ := ctx.Value(launchSinkKey{}).(*launchSink) + if sink == nil { + return + } + sink.mu.Lock() + sink.launched = true + sink.notices = append([]string(nil), notices...) + deliver := sink.pendingDeliveryLocked() + sink.mu.Unlock() + if deliver != nil { + deliver() + } +} + +// PublishLaunchForTest is publishLaunch for a test in another package that +// injects a client factory and needs to mark its fake process as started. It +// is the same function with the same context lookup, so a test exercises the +// real sink rather than a stand-in, and it is inert on any context that did +// not come through registration. +func PublishLaunchForTest(ctx context.Context, notices []string) { + publishLaunch(ctx, notices) +} + +// observe reports whether Start was reached and what applied to it. Read from +// the registration goroutine while the connect goroutine may still be running, +// hence the lock. +func (sink *launchSink) observe() (bool, []string) { + if sink == nil { + return false, nil + } + sink.mu.Lock() + defer sink.mu.Unlock() + return sink.launched, append([]string(nil), sink.notices...) +} + +// subscribe registers the one presentation this launch should reach. If the +// launch already happened, fn runs before subscribe returns; otherwise it runs +// from publishLaunch. Either way it runs at most once, and a second subscriber +// replaces nothing: the first delivery is the only delivery. +func (sink *launchSink) subscribe(fn func(notices []string)) { + if sink == nil || fn == nil { + return + } + sink.mu.Lock() + if sink.onPublish == nil { + sink.onPublish = fn + } + deliver := sink.pendingDeliveryLocked() + sink.mu.Unlock() + if deliver != nil { + deliver() + } +} + +// pendingDeliveryLocked returns the delivery to perform, or nil, and marks it +// done. Called with mu held; the returned closure must be invoked with mu +// released, since a subscriber may itself take other locks. +func (sink *launchSink) pendingDeliveryLocked() func() { + if !sink.launched || sink.onPublish == nil || sink.delivered { + return nil + } + sink.delivered = true + fn := sink.onPublish + notices := append([]string(nil), sink.notices...) + return func() { fn(notices) } +} diff --git a/internal/mcp/launch_timeout_disclosure_test.go b/internal/mcp/launch_timeout_disclosure_test.go new file mode 100644 index 000000000..b6d717897 --- /dev/null +++ b/internal/mcp/launch_timeout_disclosure_test.go @@ -0,0 +1,199 @@ +package mcp + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/tools" +) + +const launchNotice = "denyRead is configured, so the Windows sandbox uses the token shape without WRITE_RESTRICTED (#869)" + +func registerWithFactory(t *testing.T, factory func(context.Context, Server) (ToolClient, error)) *Runtime { + t.Helper() + runtime, err := RegisterTools(context.Background(), tools.NewRegistry(), config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "slow": {Type: "stdio", Command: "slow-mcp"}, + }}, RegisterOptions{ + ConnectTimeout: 50 * time.Millisecond, + ClientFactory: factory, + }) + if err != nil { + t.Fatalf("RegisterTools error: %v", err) + } + t.Cleanup(func() { _ = runtime.Close() }) + return runtime +} + +// A SERVER THAT STARTED AND THEN HUNG STILL RAN UNDER THE REDUCED TOKEN. +// +// Registration abandons a server that exceeds the connect timeout and records it +// as skipped. The startup notices used to leave connectStdio only on the returned +// client or the returned error, and the abandoned attempt returns neither before +// the serial commit phase is over, so the process ran with reduced write +// confinement and startup said only that the server was skipped. +func TestTimeoutAfterLaunchKeepsTheStartupDisclosure(t *testing.T) { + runtime := registerWithFactory(t, func(ctx context.Context, server Server) (ToolClient, error) { + // The process started under the reduced token, then initialize hangs. + publishLaunch(ctx, []string{launchNotice}) + <-ctx.Done() + return nil, ctx.Err() + }) + + disclosures := runtime.StartupDisclosures() + if len(disclosures) == 0 { + t.Fatal("a server that started and then timed out disclosed nothing, so it ran under reduced write confinement unannounced") + } + if disclosures[0].Name != "slow" || len(disclosures[0].Notices) != 1 || disclosures[0].Notices[0] != launchNotice { + t.Errorf("StartupDisclosures() = %#v, want one entry for slow carrying the launch notice", disclosures) + } + if skipped := runtime.Skipped(); len(skipped) != 1 || skipped[0].Name != "slow" { + t.Errorf("the server should still be recorded as skipped: %#v", skipped) + } +} + +// AND A TIMEOUT BEFORE LAUNCH STAYS SILENT. +// +// Without this, retaining the disclosure on timeout could be satisfied by +// disclosing on every timeout, which would claim a token trade for a process +// that was never created. +func TestTimeoutBeforeLaunchDisclosesNothing(t *testing.T) { + runtime := registerWithFactory(t, func(ctx context.Context, server Server) (ToolClient, error) { + // Never reached Start: no publish. + <-ctx.Done() + return nil, ctx.Err() + }) + + if disclosures := runtime.StartupDisclosures(); len(disclosures) != 0 { + t.Errorf("a server that never started claimed a token trade: %#v", disclosures) + } + if skipped := runtime.Skipped(); len(skipped) != 1 { + t.Errorf("the server should still be recorded as skipped: %#v", skipped) + } +} + +// THE PUBLISH HAS TO SIT AFTER Start, AND ONLY A REAL LAUNCH PROVES IT. +// +// The two tests above inject a factory, so they exercise the registry's handling +// of the sink and say nothing about where connectStdio publishes to it. Moving +// the call one line up, above cmd.Start, leaves both of them green while every +// failed launch starts claiming the token trade. This one drives the real +// connectStdio with a command that cannot start. +func TestAFailedStartPublishesNoLaunch(t *testing.T) { + sink := &launchSink{} + ctx := withLaunchSink(context.Background(), sink) + + client, err := connectStdio(ctx, Server{ + Name: "missing", + Type: "stdio", + Command: "zero-nonexistent-mcp-binary-for-test", + }, ConnectOptions{}) + if err == nil { + if client != nil { + _ = client.Close() + } + t.Fatal("expected a nonexistent executable to fail to start") + } + + if launched, notices := sink.observe(); launched { + t.Errorf("a server whose process never started was published as launched (notices %#v)", notices) + } +} + +// A START THAT COMPLETES JUST AFTER THE TIMEOUT MUST STILL BE DISCLOSED. +// +// connectStdio publishes only once cmd.Start has returned, and the timeout +// branch used to sample the sink the instant it fired. Those interleave: the +// sample reads empty, the result commits with no notice, and the reaper closes +// the late client without being able to amend a commit that already happened. +// +// The real window is microseconds wide, so this drives the CONTRACT instead: +// the attempt starts after the registration timeout has elapsed but inside the +// settle grace, which is the case the synchronization exists to catch. +func TestStartJustAfterTheTimeoutIsStillDisclosed(t *testing.T) { + runtime := registerWithFactory(t, func(ctx context.Context, server Server) (ToolClient, error) { + // 50ms registration timeout has fired; this lands inside launchSettleGrace. + time.Sleep(120 * time.Millisecond) + publishLaunch(ctx, []string{launchNotice}) + return nil, errors.New("initialize failed after start") + }) + + disclosures := runtime.StartupDisclosures() + if len(disclosures) != 1 || len(disclosures[0].Notices) != 1 || disclosures[0].Notices[0] != launchNotice { + t.Fatalf("a process that started just after the timeout was not disclosed: %#v", disclosures) + } + if skipped := runtime.Skipped(); len(skipped) != 1 { + t.Errorf("the server should still be recorded as skipped: %#v", skipped) + } +} + +// And an attempt that never starts is not held for the grace, nor disclosed. +func TestTimeoutBeforeStartIsNotDelayedOrDisclosed(t *testing.T) { + start := time.Now() + runtime := registerWithFactory(t, func(ctx context.Context, server Server) (ToolClient, error) { + <-ctx.Done() // cancel arrives with the timeout; returns immediately + return nil, ctx.Err() + }) + elapsed := time.Since(start) + + if disclosures := runtime.StartupDisclosures(); len(disclosures) != 0 { + t.Errorf("a server that never started claimed a token trade: %#v", disclosures) + } + // The grace is 250ms; an attempt that returns on cancel must not pay it. + if elapsed > 200*time.Millisecond { + t.Errorf("registration waited %v for an attempt that never started", elapsed) + } +} + +// AND A START THAT COMPLETES AFTER THE SETTLE GRACE MUST STILL BE DISCLOSED. +// +// The grace is only another timeout. Once it expires, registration reaps the +// attempt in the background and returns; the process can still be inside +// cmd.Start at that moment and start successfully afterwards. If Runtime were a +// snapshot taken at commit time, that launch would have no owner: the reaper can +// close the late client but cannot amend a value already returned, so the server +// would have run under the reduced write confinement with startup reporting it +// only as skipped. +// +// A larger grace changes the probability, not the contract, which is why this +// releases the launch strictly AFTER the bound rather than inside it. The sink +// outlives registration and StartupDisclosures reads through it. +func TestStartAfterTheSettleGraceIsStillDisclosed(t *testing.T) { + released := make(chan struct{}) + runtime := registerWithFactory(t, func(ctx context.Context, server Server) (ToolClient, error) { + // Held past the 50ms registration timeout AND past launchSettleGrace, so + // registration has already reaped this attempt and returned. + <-released + publishLaunch(ctx, []string{launchNotice}) + return nil, errors.New("initialize failed long after start") + }) + + // Registration is done and the disclosure legitimately is not known yet. + if disclosures := runtime.StartupDisclosures(); len(disclosures) != 0 { + t.Fatalf("nothing had started yet, so nothing should be disclosed: %#v", disclosures) + } + + close(released) + + deadline := time.Now().Add(2 * time.Second) + var disclosures []StartupDisclosure + for time.Now().Before(deadline) { + if disclosures = runtime.StartupDisclosures(); len(disclosures) > 0 { + break + } + time.Sleep(5 * time.Millisecond) + } + + if len(disclosures) != 1 || len(disclosures[0].Notices) != 1 || disclosures[0].Notices[0] != launchNotice { + t.Fatalf("a process that started after the settle grace was never disclosed: %#v", disclosures) + } + // Reading again must not duplicate it. + if again := runtime.StartupDisclosures(); len(again) != 1 { + t.Errorf("a second read changed the disclosures: %#v", again) + } + if skipped := runtime.Skipped(); len(skipped) != 1 { + t.Errorf("the server should still be recorded as skipped: %#v", skipped) + } +} diff --git a/internal/mcp/registry.go b/internal/mcp/registry.go index d1a2978dc..ae34e30a1 100644 --- a/internal/mcp/registry.go +++ b/internal/mcp/registry.go @@ -18,6 +18,11 @@ import ( // so a slow or unreachable server (e.g. a hosted endpoint blocked by the local // network) cannot delay the first model response. Servers connect concurrently, // so total startup cost is the slowest reachable server, not the sum. +// launchSettleGrace bounds how long an abandoned connect attempt is given to +// say whether it had already started. It is paid only after cancel, so an +// attempt that never reached Start returns well inside it. +const launchSettleGrace = 250 * time.Millisecond + const defaultConnectTimeout = 8 * time.Second type RegisterOptions struct { @@ -44,6 +49,18 @@ type SkippedServer struct { UnconfiguredDefault bool } +// StartupDisclosure is a least-privilege statement about one MCP server's +// LAUNCH, as opposed to anything a later tool call does. +// +// A stdio server prepared under a weakened token serves the whole session from +// that process, so the fact describes startup and cannot be recovered from an +// individual tool result afterwards. It is reported once, here, rather than +// appended to every response the server produces. +type StartupDisclosure struct { + Name string + Notices []string +} + type Runtime struct { clients []ToolClient // cancels releases the per-server connect contexts of the clients we KEPT. @@ -52,8 +69,83 @@ type Runtime struct { // is closed). Same length/order as clients is not required. cancels []context.CancelFunc skipped []SkippedServer - once sync.Once - err error + // disclosureSources are the least-privilege statements that applied to each + // server process this registration LAUNCHED, in server order, each still + // holding the sink that carries the authoritative launch fact. + // + // NOT a frozen snapshot. Registration is bounded and a launch is not: an + // attempt abandoned at the connect timeout can still be inside cmd.Start when + // wg.Wait returns, so the serial commit samples an empty sink and the process + // then starts under the reduced confinement with nobody left to say so. The + // sink outlives registration and StartupDisclosures reads through it, so a + // late Start is reported instead of lost. See StartupDisclosures. + disclosureSources []disclosureSource + // disclosureStream is the typed hand-off to whoever owns the output. Created + // on the first StartupDisclosureStream call and closed by Close, so a launch + // that resolves after the runtime is gone has somewhere defined to land: + // nowhere. + disclosureStreamOnce sync.Once + disclosureStream *StartupDisclosureStream + once sync.Once + err error +} + +// disclosureSource pairs a server with both the notices known at commit time and +// the sink that may still learn them. notices wins when it is already populated, +// so a settled server never re-reads the sink. +type disclosureSource struct { + name string + notices []string + sink *launchSink +} + +// ReportStartupDisclosures delivers each server's launch disclosure to report +// EXACTLY ONCE, whether the launch had already completed when this was called +// or completes later. +// +// StartupDisclosures reads through the sink, which stopped a late Start from +// being lost, but a value nobody re-reads is still a lost disclosure: both +// production reporters sample once, right after RegisterTools returns, and an +// attempt abandoned at the connect timeout can finish Start after that sample. +// The reaper only closes the late client. So the operator saw the skipped-server +// warning and never learned that a local process had run under the reduced +// enforcement. +// +// Servers whose notices were known at commit are reported now, in server order. +// Every other server subscribes its sink: if the launch already happened the +// subscriber runs before this returns, otherwise it runs from publishLaunch on +// the connect goroutine. Either way each server reaches report once. A server +// that never starts never publishes, so prepare, pipe and Start failures stay +// silent, and network servers, which launch no process, contribute nothing. +// +// Late deliveries arrive in completion order, which is the only order they +// have; the immediate set keeps server order. +// +// A STREAM, NOT A CALLBACK. An earlier version took the presentation function +// and invoked it from whichever goroutine resolved the launch, which for an +// abandoned attempt is the connect goroutine. That put a write to the caller's +// writer on a goroutine and at a time the caller did not control. The runtime +// owns the fact; it appends the fact here and the owner drains it. See +// StartupDisclosureStream. +func (runtime *Runtime) StartupDisclosureStream() *StartupDisclosureStream { + if runtime == nil { + return nil + } + runtime.disclosureStreamOnce.Do(func() { + stream := newStartupDisclosureStream() + runtime.disclosureStream = stream + for _, source := range runtime.disclosureSources { + if len(source.notices) > 0 { + stream.offer(StartupDisclosure{Name: source.name, Notices: append([]string(nil), source.notices...)}) + continue + } + name := source.name + source.sink.subscribe(func(notices []string) { + stream.offer(StartupDisclosure{Name: name, Notices: notices}) + }) + } + }) + return runtime.disclosureStream } // Skipped returns the servers that were skipped during registration (unreachable @@ -107,19 +199,38 @@ func RegisterTools(ctx context.Context, registry *tools.Registry, cfg config.MCP remote []RemoteTool cancel context.CancelFunc err error + // notices travels with the indexed result rather than being appended to + // shared state from inside the goroutine. The concurrent phase touches no + // shared state, which is the property the comment above promises and the + // reason the serial phase can be deterministic; appending here broke both, + // racing the slice header and ordering disclosures by completion time. + notices []string } results := make([]connectResult, len(servers)) + // RETAINED PAST THE CONCURRENT PHASE. The timeout branch samples the sink the + // moment it fires, but connectStdio does not publish until cmd.Start has + // returned, so a Start that succeeds just after the timeout selected was + // sampled as "never launched" and its disclosure was lost: the reaper closes + // the late client and cannot amend a commit that has already happened. + // Reading the sink again in the serial phase is strictly later than the + // timeout branch and still deterministic, because it runs after wg.Wait. + sinks := make([]*launchSink, len(servers)) var wg sync.WaitGroup for index := range servers { wg.Add(1) go func(index int) { defer wg.Done() server := servers[index] - serverCtx, cancel := context.WithCancel(ctx) + // The sink hears about Start as it happens, so an attempt abandoned below + // can still report the confinement its process ran under. The connect + // result cannot supply that: it does not arrive until after this phase. + sink := &launchSink{} + sinks[index] = sink + serverCtx, cancel := context.WithCancel(withLaunchSink(ctx, sink)) done := make(chan connectResult, 1) go func() { - client, remote, err := connectAndList(serverCtx, factory, server) - done <- connectResult{client: client, remote: remote, err: err} + client, remote, notices, err := connectAndList(serverCtx, factory, server) + done <- connectResult{client: client, remote: remote, notices: notices, err: err} }() select { case res := <-done: @@ -131,14 +242,47 @@ func RegisterTools(ctx context.Context, registry *tools.Registry, cfg config.MCP results[index] = res case <-time.After(timeout): cancel() // abandon the slow connect: tears down the conn/subprocess - // Reap the goroutine + any partial client in the background so a - // slow server never blocks startup. - go func() { - if res := <-done; res.client != nil { + timedOut := connectResult{err: fmt.Errorf("connect timed out after %s", timeout)} + // SYNCHRONIZE WITH THE START, briefly, before deciding there was none. + // + // connectStdio publishes only after cmd.Start returns, so sampling the + // sink the instant the timeout fires races a Start that is about to + // succeed: the sample reads empty, the result commits with no notice, + // and the reaper cannot amend a commit that has already happened. The + // window is microseconds and unreachable from a test seam, which is + // exactly why it must be closed by construction rather than measured. + // + // cancel() has already fired, so an attempt that has NOT started fails + // fast and this returns immediately; only one that did start can still + // be in Start, and it publishes on the way out. The grace is therefore + // paid only when there is something to learn. + select { + case res := <-done: + if res.client != nil { _ = res.client.Close() } - }() - results[index] = connectResult{err: fmt.Errorf("connect timed out after %s", timeout)} + if len(res.notices) > 0 { + timedOut.notices = res.notices + } + case <-time.After(launchSettleGrace): + // Still stuck past the grace. Reap in the background so a slow + // server never blocks startup. + go func() { + if res := <-done; res.client != nil { + _ = res.client.Close() + } + }() + } + // A server that reached Start ran under the planned enforcement even + // though its connection never became usable. One that timed out + // before Start discloses nothing, so the sink stays empty and this + // adds nothing. + if len(timedOut.notices) == 0 { + if launched, notices := sink.observe(); launched { + timedOut.notices = notices + } + } + results[index] = timedOut } }(index) } @@ -154,6 +298,28 @@ func RegisterTools(ctx context.Context, registry *tools.Registry, cfg config.MCP stagedNames := make(map[string]struct{}) for index, server := range servers { res := results[index] + // Recorded here, in server order, for any server whose PROCESS STARTED, + // including one whose tools are rejected below: the launch happened under + // that token either way, and a skip warning does not say what confinement + // the process ran with while it was alive. + notices := res.notices + if len(notices) == 0 { + // A launch that published after the timeout branch sampled. Checked here + // rather than only there so the window between Start succeeding and the + // timeout committing cannot swallow the disclosure. + if launched, late := sinks[index].observe(); launched { + notices = late + } + } + // Recorded whether or not notices are known YET. An abandoned attempt can + // still be inside cmd.Start, and keeping its sink here is what lets + // StartupDisclosures report that launch after this phase has finished. + // Order is server order, so a late arrival does not reorder the rest. + runtime.disclosureSources = append(runtime.disclosureSources, disclosureSource{ + name: server.Name, + notices: notices, + sink: sinks[index], + }) if res.err != nil { runtime.skipped = append(runtime.skipped, SkippedServer{Name: server.Name, Err: res.err, UnconfiguredDefault: server.UnconfiguredDefault}) continue @@ -187,17 +353,42 @@ func RegisterTools(ctx context.Context, registry *tools.Registry, cfg config.MCP // connectAndList connects to one server and lists its tools. It does ONLY I/O // (no registry, permission-store, or other shared state), so it is safe to run // concurrently for every server. On a list error it closes the client. -func connectAndList(ctx context.Context, factory func(context.Context, Server) (ToolClient, error), server Server) (ToolClient, []RemoteTool, error) { +// connectAndList returns the client, its tools, and the least-privilege +// disclosures that applied to its LAUNCH. +// +// THE LAUNCH FACT OUTLIVES THE CONNECTION. A stdio server can start, and do +// filesystem work, and then fail initialize or tools/list. Returning the notices +// separately rather than leaving them on the client means the fact survives that +// failure: the client is closed and discarded here, so anything reachable only +// through it is gone by the time the caller sees the error, and the skip warning +// on its own does not say the process already ran with reduced write +// confinement. +func connectAndList(ctx context.Context, factory func(context.Context, Server) (ToolClient, error), server Server) (ToolClient, []RemoteTool, []string, error) { client, err := factory(ctx, server) if err != nil { - return nil, nil, err + // A failure BEFORE the process started discloses nothing. A failure after it + // started carries the fact out through the error, because the client that + // held it has already been closed and discarded by then. + return nil, nil, startupNoticesFromError(err), err } + notices := startupNoticesOf(client) remoteTools, err := client.ListTools(ctx) if err != nil { _ = client.Close() - return nil, nil, fmt.Errorf("list MCP tools for %s: %w", server.Name, err) + return nil, nil, notices, fmt.Errorf("list MCP tools for %s: %w", server.Name, err) } - return client, remoteTools, nil + return client, remoteTools, notices, nil +} + +// startupNoticesOf reads a client's launch disclosures, if it reports any. +func startupNoticesOf(client ToolClient) []string { + if client == nil { + return nil + } + if disclosing, ok := client.(startupDisclosing); ok { + return disclosing.StartupNotices() + } + return nil } // buildServerTools validates a server's remote tools against the registry and the @@ -233,6 +424,10 @@ func (runtime *Runtime) Close() error { return nil } runtime.once.Do(func() { + // End disclosure delivery FIRST. A launch that resolves while the clients + // are being closed has no owner left to print it, and the runtime must not + // leave a subscriber holding a writer whose lifetime it does not know. + runtime.disclosureStream.Close() for _, client := range runtime.clients { if err := client.Close(); err != nil && runtime.err == nil { runtime.err = err @@ -395,3 +590,38 @@ func isPersistentlyApproved(store *PermissionStore, server Server, toolName stri }) return err == nil && approved } + +// StartupDisclosures returns the least-privilege statements that applied to the +// MCP server processes this registration launched, so a caller can report them +// once. Empty when no server was launched under reduced enforcement, and always +// empty for network servers, which launch no local process. +// +// READ THROUGH THE SINK, so this is not fixed at the moment registration +// returned. A server abandoned at the connect timeout may still have been inside +// cmd.Start then, and its process starts under the reduced write confinement +// regardless of whether the connection ever became usable. Registration stays +// bounded; the disclosure does not expire with it. +// +// Server order, and a settled entry never re-reads its sink, so calling this +// twice cannot reorder or duplicate anything. +func (runtime *Runtime) StartupDisclosures() []StartupDisclosure { + if runtime == nil { + return nil + } + disclosures := make([]StartupDisclosure, 0, len(runtime.disclosureSources)) + for _, source := range runtime.disclosureSources { + notices := source.notices + if len(notices) == 0 { + if launched, late := source.sink.observe(); launched { + notices = late + } + } + if len(notices) > 0 { + disclosures = append(disclosures, StartupDisclosure{Name: source.name, Notices: notices}) + } + } + if len(disclosures) == 0 { + return nil + } + return disclosures +} diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 42e46cc19..f22586958 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -218,8 +218,14 @@ func (server toolServer) callTool(ctx context.Context, rawParams json.RawMessage result := server.registry.RunWithOptions(ctx, params.Name, params.Arguments, tools.RunOptions{ PermissionGranted: server.options.PermissionGranted, }) + // ModelOutput, not the raw field. This is a model-facing protocol boundary, + // and Result.Output now holds the UNDECORATED base text: the enforcement + // disclosure lives in typed state and the accessor is what composes the two. + // Serializing Output directly hands an MCP client a Windows command's ordinary + // output with no statement that its DenyRead token shape left writes + // unconfined, which is the one thing the disclosure exists to say. return CallToolResult{ - Content: []Content{{Type: "text", Text: result.Output}}, + Content: []Content{{Type: "text", Text: result.ModelOutput()}}, IsError: result.Status != tools.StatusOK, }, nil } diff --git a/internal/mcp/startup_disclosure_race_test.go b/internal/mcp/startup_disclosure_race_test.go new file mode 100644 index 000000000..8fe5d9af3 --- /dev/null +++ b/internal/mcp/startup_disclosure_race_test.go @@ -0,0 +1,193 @@ +package mcp + +import ( + "context" + "fmt" + "sort" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/tools" +) + +// disclosingRaceClient is a launched server that reports a disclosure. +type disclosingRaceClient struct { + fakeToolClient + notices []string +} + +func (c *disclosingRaceClient) StartupNotices() []string { return c.notices } + +// THE CONCURRENT PHASE TOUCHES NO SHARED STATE, AND THAT IS LOAD-BEARING. +// +// RegisterTools runs one goroutine per server and commits everything in a +// deterministic serial phase afterwards, which is what lets the result be +// identical regardless of completion order. Collecting the startup disclosures +// inside the goroutine broke both halves of that: the append raced the slice +// header, so entries could be lost or overwritten, and whichever survived were +// ordered by completion time rather than by server. +// +// Many servers rather than one, because a single disclosing server cannot +// exercise a shared write at all. Run this package with -race. +func TestStartupDisclosuresAreCollectedWithoutRacing(t *testing.T) { + const servers = 32 + configured := map[string]config.MCPServerConfig{} + for index := range servers { + configured[fmt.Sprintf("srv%02d", index)] = config.MCPServerConfig{Type: "stdio", Command: "server"} + } + + registry := tools.NewRegistry() + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: configured}, RegisterOptions{ + ClientFactory: func(_ context.Context, server Server) (ToolClient, error) { + return &disclosingRaceClient{ + fakeToolClient: fakeToolClient{listed: []RemoteTool{{Name: "tool_" + server.Name, Description: "d"}}}, + notices: []string{"denyRead is configured for " + server.Name}, + }, nil + }, + }) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + + got := runtime.StartupDisclosures() + if len(got) != servers { + t.Fatalf("collected %d disclosures, want %d: a shared append loses entries", len(got), servers) + } + + // Deterministic server order, not completion order. Sorting the result and + // then comparing would hide exactly the defect this pins. + names := make([]string, 0, len(got)) + for _, disclosure := range got { + names = append(names, disclosure.Name) + } + sorted := append([]string(nil), names...) + sort.Strings(sorted) + for index := range names { + if names[index] != sorted[index] { + t.Fatalf("disclosure %d is %q, want %q: order follows completion rather than server order", index, names[index], sorted[index]) + } + } +} + +// failingDisclosingClient launches (so it has a disclosure) and then fails +// tools/list, which is the shape that used to drop the fact. +type failingDisclosingClient struct { + fakeToolClient + notices []string +} + +func (c *failingDisclosingClient) StartupNotices() []string { return c.notices } +func (c *failingDisclosingClient) ListTools(context.Context) ([]RemoteTool, error) { + return nil, fmt.Errorf("initialize failed after the process started") +} + +// THE LAUNCH FACT OUTLIVES THE CONNECTION. +// +// connectStdio records the disclosure once cmd.Start returns, which is the right +// moment. But a stdio server can start, do filesystem work, and then fail +// initialize or tools/list, and that path closes the client and returns nil. The +// disclosure was reachable only through that client, so it died with it, and the +// operator was told the server was unavailable without being told the process +// had already run without the write jail. +func TestADisclosureSurvivesAFailureAfterTheProcessLaunched(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + registry := tools.NewRegistry() + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, RegisterOptions{ + ClientFactory: func(context.Context, Server) (ToolClient, error) { + return &failingDisclosingClient{notices: []string{notice}}, nil + }, + }) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + + skipped := runtime.Skipped() + if len(skipped) != 1 { + t.Fatalf("Skipped() = %#v, want the failure recorded", skipped) + } + disclosures := runtime.StartupDisclosures() + if len(disclosures) != 1 || len(disclosures[0].Notices) != 1 || disclosures[0].Notices[0] != notice { + t.Fatalf("StartupDisclosures() = %#v, want the launch disclosure kept despite the failure", disclosures) + } + if disclosures[0].Name != "docs" { + t.Errorf("Name = %q, want the server it describes", disclosures[0].Name) + } +} + +// And a server that never launched still discloses nothing. +func TestAFactoryFailureDisclosesNothing(t *testing.T) { + registry := tools.NewRegistry() + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, RegisterOptions{ + ClientFactory: func(context.Context, Server) (ToolClient, error) { + return nil, fmt.Errorf("could not start the process") + }, + }) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + if disclosures := runtime.StartupDisclosures(); len(disclosures) != 0 { + t.Errorf("StartupDisclosures() = %#v, want none for a process that never started", disclosures) + } +} + +// A launched process that fails its HANDSHAKE keeps its disclosure too. +// +// connectStdio records the notices once cmd.Start returns, which is the right +// moment, but the initialize failure path closes and discards the client. The +// client was the only carrier, so the fact died with the connection unless the +// failure carries it out itself. +func TestADisclosureSurvivesAnInitializeFailure(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + registry := tools.NewRegistry() + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, RegisterOptions{ + ClientFactory: func(context.Context, Server) (ToolClient, error) { + // What connectStdio does once Start has succeeded and the handshake + // then fails: the client is gone, the fact rides the error. + return nil, &startupDisclosureError{ + err: fmt.Errorf("initialize MCP server docs: handshake timed out"), + notices: []string{notice}, + } + }, + }) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + + if skipped := runtime.Skipped(); len(skipped) != 1 { + t.Fatalf("Skipped() = %#v, want the failure recorded", skipped) + } + disclosures := runtime.StartupDisclosures() + if len(disclosures) != 1 || len(disclosures[0].Notices) != 1 || disclosures[0].Notices[0] != notice { + t.Fatalf("StartupDisclosures() = %#v, want the launch disclosure kept through the handshake failure", disclosures) + } +} + +// And a plain failure with no launch behind it still discloses nothing, so the +// error path is not just attaching notices to everything. +func TestAPlainConnectFailureDisclosesNothing(t *testing.T) { + registry := tools.NewRegistry() + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, RegisterOptions{ + ClientFactory: func(context.Context, Server) (ToolClient, error) { + return nil, fmt.Errorf("could not start the process") + }, + }) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + if disclosures := runtime.StartupDisclosures(); len(disclosures) != 0 { + t.Errorf("StartupDisclosures() = %#v, want none", disclosures) + } +} diff --git a/internal/mcp/startup_disclosure_stream.go b/internal/mcp/startup_disclosure_stream.go new file mode 100644 index 000000000..b153db414 --- /dev/null +++ b/internal/mcp/startup_disclosure_stream.go @@ -0,0 +1,116 @@ +package mcp + +import "sync" + +// StartupDisclosureStream carries launch disclosures out of the runtime as +// TYPED EVENTS, for whichever component owns the output to drain on its own +// goroutine. +// +// It replaces handing the runtime a presentation callback. That callback closed +// over the CLI's writer, and the runtime invoked it synchronously from whichever +// goroutine happened to resolve the launch. For a server still inside cmd.Start +// when registration gave up, that goroutine is the abandoned connect attempt, +// which runs on no schedule the caller controls: the write landed off the output +// owner's goroutine, raced any other writer, and could arrive after the owner had +// returned or after Bubble Tea had taken the alt screen. The runtime owns the +// FACT that a process started; it does not own anyone's writer. +// +// So the runtime only ever appends a value here. Delivery, ordering against other +// startup output, and the decision to stop listening all belong to the owner. +// +// LIFETIME IS EXPLICIT. Close is the owner saying "I will not write again". +// Offers after Close are dropped rather than queued for a consumer that no longer +// exists, and Wait returns false so a pump exits. Both are deliberate: a +// disclosure is worth printing while someone can print it, and worth dropping +// rather than corrupting a screen that now belongs to something else. Close is +// idempotent and safe from any goroutine, so the runtime and the owner may both +// call it. +type StartupDisclosureStream struct { + mu sync.Mutex + queue []StartupDisclosure + closed bool + wake chan struct{} +} + +func newStartupDisclosureStream() *StartupDisclosureStream { + return &StartupDisclosureStream{wake: make(chan struct{}, 1)} +} + +// offer queues one disclosure. Called from the registration goroutine for a +// launch already known at commit, and from an abandoned connect goroutine for one +// that resolves later. It takes a lock and appends; it never touches a writer, +// which is the whole point of the type. +func (stream *StartupDisclosureStream) offer(disclosure StartupDisclosure) { + if stream == nil || len(disclosure.Notices) == 0 { + return + } + stream.mu.Lock() + if stream.closed { + stream.mu.Unlock() + return + } + stream.queue = append(stream.queue, disclosure) + stream.mu.Unlock() + select { + case stream.wake <- struct{}{}: + default: + } +} + +// Drain removes and returns everything queued right now, without blocking. The +// owner calls this on the goroutine that owns the writer, so every disclosure is +// printed by exactly one goroutine at a time. +func (stream *StartupDisclosureStream) Drain() []StartupDisclosure { + if stream == nil { + return nil + } + stream.mu.Lock() + defer stream.mu.Unlock() + if len(stream.queue) == 0 { + return nil + } + queued := stream.queue + stream.queue = nil + return queued +} + +// Wait blocks until at least one disclosure is queued or the stream is closed. It +// reports whether draining is still worthwhile: false means closed and empty, so +// a pump loop should return. +func (stream *StartupDisclosureStream) Wait() bool { + if stream == nil { + return false + } + for { + stream.mu.Lock() + queued := len(stream.queue) > 0 + closed := stream.closed + stream.mu.Unlock() + if queued { + return true + } + if closed { + return false + } + <-stream.wake + } +} + +// Close ends delivery. Idempotent, safe from any goroutine, and safe to call +// from both the runtime and the output owner. +func (stream *StartupDisclosureStream) Close() { + if stream == nil { + return + } + stream.mu.Lock() + if stream.closed { + stream.mu.Unlock() + return + } + stream.closed = true + stream.mu.Unlock() + select { + case stream.wake <- struct{}{}: + default: + } +} diff --git a/internal/mcp/startup_disclosure_test.go b/internal/mcp/startup_disclosure_test.go new file mode 100644 index 000000000..90f62ecc7 --- /dev/null +++ b/internal/mcp/startup_disclosure_test.go @@ -0,0 +1,194 @@ +package mcp + +import ( + "context" + "errors" + "os" + "os/exec" + "strings" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/execution" + "github.com/Gitlawb/zero/internal/tools" +) + +const startupNotice = "denyRead is configured, so the write jail is not confining writes" + +// disclosingPreparer plans an MCP server launch that carries an enforcement +// notice, and can fail the way the sandbox does before the child exists. +type disclosingPreparer struct { + prepareErr error + missing bool +} + +func (preparer *disclosingPreparer) PrepareExecution(ctx context.Context, request execution.Request) (execution.PreparedCommand, error) { + if preparer.prepareErr != nil { + return execution.PreparedCommand{}, preparer.prepareErr + } + name, args := request.Command.Name, request.Command.Args + if preparer.missing { + name, args = "definitely-not-a-real-binary-zzz", nil + } + command := exec.CommandContext(ctx, name, args...) + command.Dir = request.WorkingDirectory + command.Env = request.Command.Env + return execution.PreparedCommand{ + Command: command, + Enforcement: execution.Enforcement{Notices: []string{startupNotice}}, + }, nil +} + +func helperServer(t *testing.T) Server { + t.Helper() + executable, err := os.Executable() + if err != nil { + t.Fatal(err) + } + return Server{ + Name: "docs", + Type: ServerTypeStdio, + Command: executable, + Args: []string{"-test.run=TestMCPStdioHelperProcess", "--"}, + Env: map[string]string{"ZERO_MCP_STDIO_HELPER": "1"}, + } +} + +// THE FACT DESCRIBES STARTUP, SO NOTHING LATER CAN CARRY IT. +// +// The generic adapter puts plan notes on PreparedCommand.Enforcement for +// OriginMCPServer, and connectStdio kept only the command and its cleanup. A +// stdio server launched under the weakened token then served the whole session +// with no path able to tell the operator that its write confinement was +// reduced, and no individual tool result could recover it, because the fact is +// about the process rather than about any response. +func TestAnMCPServerLaunchKeepsItsEnforcementDisclosure(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + client, err := ConnectWithOptions(ctx, helperServer(t), ConnectOptions{ + Execution: execution.NewRunner(&disclosingPreparer{}), + WorkspaceRoot: t.TempDir(), + }) + if err != nil { + t.Fatalf("ConnectWithOptions() error = %v", err) + } + defer client.Close() + + disclosing, ok := client.(startupDisclosing) + if !ok { + t.Fatal("a launched stdio client does not report its startup enforcement at all") + } + notices := disclosing.StartupNotices() + if len(notices) != 1 || notices[0] != startupNotice { + t.Fatalf("StartupNotices() = %#v, want the launch disclosure", notices) + } +} + +// A launch with nothing to disclose reports nothing, or every server would carry +// a notice and the statement would mean nothing. +func TestAnUnrestrictedMCPServerLaunchDisclosesNothing(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + client, err := ConnectWithOptions(ctx, helperServer(t), ConnectOptions{ + Execution: execution.NewRunner(&mcpExecutionPreparer{}), + WorkspaceRoot: t.TempDir(), + }) + if err != nil { + t.Fatalf("ConnectWithOptions() error = %v", err) + } + defer client.Close() + disclosing, ok := client.(startupDisclosing) + if !ok { + t.Fatal("a launched stdio client does not report its startup enforcement at all") + } + if notices := disclosing.StartupNotices(); len(notices) != 0 { + t.Errorf("StartupNotices() = %#v, want none", notices) + } +} + +// And a launch that never happened claims nothing, which is the same launch-state +// rule hooks and plugins apply. Here it is expressed by WHERE the notice is +// recorded: every failure above returns before the client exists. +func TestAnMCPServerThatNeverLaunchedClaimsNoEnforcement(t *testing.T) { + for _, testCase := range []struct { + name string + preparer *disclosingPreparer + }{ + {"sandbox setup failed", &disclosingPreparer{prepareErr: errors.New("could not build the restricted token")}}, + {"executable not found", &disclosingPreparer{missing: true}}, + } { + t.Run(testCase.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + client, err := ConnectWithOptions(ctx, helperServer(t), ConnectOptions{ + Execution: execution.NewRunner(testCase.preparer), + WorkspaceRoot: t.TempDir(), + }) + if err == nil { + client.Close() + t.Fatal("the server started even though its launch was supposed to fail") + } + if strings.Contains(err.Error(), startupNotice) { + t.Errorf("a launch that never happened claimed an enforcement trade: %v", err) + } + }) + } +} + +// disclosingFakeClient is a launched server that carries a disclosure. +type disclosingFakeClient struct { + fakeToolClient + notices []string +} + +func (client *disclosingFakeClient) StartupNotices() []string { return client.notices } + +// Registration is the boundary that reports it, once, for the process it +// launched. +func TestRegistrationCollectsStartupDisclosures(t *testing.T) { + registry := tools.NewRegistry() + client := &disclosingFakeClient{ + fakeToolClient: fakeToolClient{listed: []RemoteTool{{Name: "lookup", Description: "Lookup"}}}, + notices: []string{startupNotice}, + } + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, RegisterOptions{ClientFactory: func(context.Context, Server) (ToolClient, error) { return client, nil }}) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + + disclosures := runtime.StartupDisclosures() + if len(disclosures) != 1 { + t.Fatalf("StartupDisclosures() = %#v, want one", disclosures) + } + if disclosures[0].Name != "docs" { + t.Errorf("Name = %q, want the server it describes", disclosures[0].Name) + } + if len(disclosures[0].Notices) != 1 || disclosures[0].Notices[0] != startupNotice { + t.Errorf("Notices = %#v, want the launch disclosure", disclosures[0].Notices) + } +} + +// A network server launches no local process, so it implements nothing and +// reports nothing. That is a different answer from an empty one and the negative +// case that keeps the statement meaningful. +func TestANetworkServerReportsNoStartupDisclosure(t *testing.T) { + registry := tools.NewRegistry() + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://host.invalid/mcp"}, + }}, RegisterOptions{ClientFactory: func(context.Context, Server) (ToolClient, error) { + return &fakeToolClient{listed: []RemoteTool{{Name: "lookup", Description: "Lookup"}}}, nil + }}) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + if disclosures := runtime.StartupDisclosures(); len(disclosures) != 0 { + t.Errorf("StartupDisclosures() = %#v, want none for a server that launches no process", disclosures) + } +} diff --git a/internal/plugins/activate.go b/internal/plugins/activate.go index 40bf5416f..f74124bf9 100644 --- a/internal/plugins/activate.go +++ b/internal/plugins/activate.go @@ -56,6 +56,15 @@ type commandOutput struct { Stderr string ExitCode int Err error + // Notices carries the enforcement disclosures the execution runner attached + // to this command. + // + // The generic contract is not transport-only. Enforcement.Notices says what + // the sandbox actually did, and on Windows that includes trading the write + // jail away for a deny-read profile. A projection that copies stdout, stderr + // and an exit code drops it, so a plugin tool ran under the weakened token + // and said nothing about it. + Notices []string } // toolRunner executes a resolved plugin tool command. It is injectable so @@ -549,26 +558,37 @@ func (tool pluginTool) invoke(ctx context.Context, args map[string]any, cwd stri meta["exit_code"] = strconv.Itoa(output.ExitCode) if output.Err != nil { + // EVERY POST-LAUNCH TERMINAL OUTCOME CARRIES THE DISCLOSURE. This branch + // used to rebuild a result from status, output and metadata alone, so a + // plugin that timed out or was cancelled reported only that and said + // nothing about having run without write confinement. Whether the notice + // survives must not depend on how the process ended. The launched-or-not + // question is answered in execPluginCommandWithExecution; by here + // output.Notices is empty for anything that never started. return tools.Result{ - Status: tools.StatusError, - Output: "Error executing plugin tool " + tool.name + ": " + output.Err.Error(), - Meta: meta, + Status: tools.StatusError, + Output: "Error executing plugin tool " + tool.name + ": " + output.Err.Error(), + Meta: meta, + EnforcementNotices: output.Notices, + Display: tools.Display{Summary: tool.name + " failed", Kind: "plugin"}, } } formatted := formatPluginToolOutput(output) if output.ExitCode != 0 { return tools.Result{ - Status: tools.StatusError, - Output: formatted, - Meta: meta, - Display: tools.Display{Summary: tool.name + " failed", Kind: "plugin"}, + Status: tools.StatusError, + Output: formatted, + Meta: meta, + EnforcementNotices: output.Notices, + Display: tools.Display{Summary: tool.name + " failed", Kind: "plugin"}, } } return tools.Result{ - Status: tools.StatusOK, - Output: formatted, - Meta: meta, - Display: tools.Display{Summary: tool.name, Kind: "plugin"}, + Status: tools.StatusOK, + Output: formatted, + Meta: meta, + EnforcementNotices: output.Notices, + Display: tools.Display{Summary: tool.name, Kind: "plugin"}, } } @@ -719,7 +739,18 @@ func execPluginCommandWithExecution(ctx context.Context, runner *execution.Runne if result.Outcome.Exit != nil { exitCode = result.Outcome.Exit.Code } - output := commandOutput{Stdout: result.Stdout, Stderr: result.Stderr, ExitCode: exitCode} + output := commandOutput{ + Stdout: result.Stdout, + Stderr: result.Stderr, + ExitCode: exitCode, + } + // THE NOTICE DESCRIBES A CHILD THAT RAN. Deciding that here, where the outcome + // kind is known, rather than at each result constructor: a timeout or a + // cancellation happened to a process that had already launched under the + // weakened token, so the disclosure is still true of it. A setup failure or a + // missing executable launched nothing, and claiming the write jail was traded + // away there would describe a trade nobody made. + output.Notices = result.Outcome.AppliedEnforcementNotices() switch result.Outcome.Kind { case execution.OutcomeSandboxSetupFailure, execution.OutcomeExecutableNotFound, execution.OutcomeTimedOut, execution.OutcomeCancelled: output.Err = result.Err diff --git a/internal/plugins/enforcement_notice_test.go b/internal/plugins/enforcement_notice_test.go new file mode 100644 index 000000000..327016420 --- /dev/null +++ b/internal/plugins/enforcement_notice_test.go @@ -0,0 +1,164 @@ +package plugins + +import ( + "context" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/execution" + "github.com/Gitlawb/zero/internal/tools" +) + +// THE DISCLOSURE HAS TO SURVIVE THE PROJECTION. +// +// The execution runner puts enforcement notices on the structured outcome, and +// this path used to copy only stdout, stderr and an exit code out of it. A +// plugin tool therefore ran under the non-WRITE_RESTRICTED token and returned a +// result that said nothing about the write jail it had just traded away. +// +// Asserted through pluginTool.invoke, which is what the registry calls, and +// through Result.ModelOutput, which is what the model actually reads. +func TestAPluginToolCarriesTheEnforcementNotice(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + + for _, testCase := range []struct { + name string + exitCode int + }{ + {"successful command", 0}, + {"failed command", 3}, + } { + t.Run(testCase.name, func(t *testing.T) { + tool := pluginTool{ + name: "demo", + run: func(context.Context, pluginCommand) commandOutput { + return commandOutput{Stdout: "hello", ExitCode: testCase.exitCode, Notices: []string{notice}} + }, + } + + result := tool.invoke(context.Background(), map[string]any{}, t.TempDir()) + if len(result.EnforcementNotices) == 0 { + t.Fatal("the plugin result carried no enforcement notice; the command ran under the weakened token and said nothing") + } + model := result.ModelOutput() + if !strings.Contains(model, notice) { + t.Errorf("the model-facing output does not contain the notice:\n%s", model) + } + if strings.Count(model, notice) != 1 { + t.Errorf("the notice appears %d times, want exactly once:\n%s", strings.Count(model, notice), model) + } + if summary := result.HumanDisplay().Summary; !strings.Contains(summary, notice) { + t.Errorf("the human summary does not contain the notice: %q", summary) + } + }) + } +} + +// And a command with no notice is unchanged, or the assertion above would be +// satisfied by text pasted onto everything. +func TestAPluginToolWithoutANoticeIsUnchanged(t *testing.T) { + tool := pluginTool{ + name: "demo", + run: func(context.Context, pluginCommand) commandOutput { + return commandOutput{Stdout: "hello", ExitCode: 0} + }, + } + result := tool.invoke(context.Background(), map[string]any{}, t.TempDir()) + if len(result.EnforcementNotices) != 0 { + t.Errorf("a command with no enforcement notice grew one: %v", result.EnforcementNotices) + } + if result.Status != tools.StatusOK { + t.Errorf("status = %v, want ok", result.Status) + } +} + +// A TIMEOUT OR CANCELLATION STILL RAN THE CHILD. +// +// invoke's error branch rebuilt a result from status, output and metadata alone, +// so a plugin that timed out under the non-WRITE_RESTRICTED token reported only +// the timeout. The process had already launched without write confinement; +// whether the disclosure survives must not depend on how it ended. +func TestAPluginToolCarriesTheNoticeWhenItTimesOutOrIsCancelled(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + + for _, testCase := range []struct { + name string + err error + }{ + {"timed out", context.DeadlineExceeded}, + {"cancelled", context.Canceled}, + } { + t.Run(testCase.name, func(t *testing.T) { + tool := pluginTool{ + name: "demo", + run: func(context.Context, pluginCommand) commandOutput { + return commandOutput{ExitCode: -1, Err: testCase.err, Notices: []string{notice}} + }, + } + result := tool.invoke(context.Background(), map[string]any{}, t.TempDir()) + + model := result.ModelOutput() + if !strings.Contains(model, notice) { + t.Errorf("the model-facing output lost the notice:\n%s", model) + } + if strings.Count(model, notice) != 1 { + t.Errorf("the notice appears %d times, want once:\n%s", strings.Count(model, notice), model) + } + if summary := result.HumanDisplay().Summary; !strings.Contains(summary, notice) { + t.Errorf("the human summary lost the notice: %q", summary) + } + }) + } +} + +// But a child that never launched must stay silent, or the notice describes a +// trade nobody made. +// +// KEYED ON THE RECORDED FACT, NOT ON THE OUTCOME KIND. Reading the kind as a +// launch-state field is wrong in both directions: a child that ran and then +// produced an unreadable adapter report is rewritten to a setup failure, so the +// disclosure is dropped although it applied, and a context cancelled before +// os.StartProcess yields a cancellation, so the disclosure is claimed for a +// process that never existed. This is why the earlier version of this test, +// which asserted that the kind decides, was encoding the defect. +func TestAPluginNoticeFollowsRecordedLaunchState(t *testing.T) { + const notice = "denyRead is configured, so the write jail is not confining writes" + enforcement := execution.Enforcement{Notices: []string{notice}} + + // The two directions the kind gets wrong, spelled out. + ranThenReportFailed := execution.Outcome{ + Kind: execution.OutcomeSandboxSetupFailure, + Launched: true, + Enforcement: enforcement, + } + if got := ranThenReportFailed.AppliedEnforcementNotices(); len(got) != 1 { + t.Errorf("a child that ran lost its disclosure because the report failed afterwards: %#v", got) + } + + cancelledBeforeStart := execution.Outcome{ + Kind: execution.OutcomeCancelled, + Launched: false, + Enforcement: enforcement, + } + if got := cancelledBeforeStart.AppliedEnforcementNotices(); len(got) != 0 { + t.Errorf("a process that never started claimed an enforcement trade: %#v", got) + } + + // And the ordinary pairs still behave. + for _, testCase := range []struct { + name string + outcome execution.Outcome + discloses bool + }{ + {"launched and succeeded", execution.Outcome{Kind: execution.OutcomeSuccess, Launched: true, Enforcement: enforcement}, true}, + {"launched then timed out", execution.Outcome{Kind: execution.OutcomeTimedOut, Launched: true, Enforcement: enforcement}, true}, + {"never launched, missing executable", execution.Outcome{Kind: execution.OutcomeExecutableNotFound, Launched: false, Enforcement: enforcement}, false}, + {"never launched, setup failed", execution.Outcome{Kind: execution.OutcomeSandboxSetupFailure, Launched: false, Enforcement: enforcement}, false}, + } { + t.Run(testCase.name, func(t *testing.T) { + if got := len(testCase.outcome.AppliedEnforcementNotices()) > 0; got != testCase.discloses { + t.Errorf("discloses = %v, want %v", got, testCase.discloses) + } + }) + } +} diff --git a/internal/sandbox/manager.go b/internal/sandbox/manager.go index eaf604b00..70dcf3294 100644 --- a/internal/sandbox/manager.go +++ b/internal/sandbox/manager.go @@ -331,10 +331,76 @@ func (request SandboxExecutionRequest) BackendPlan(policy Policy) BackendPlan { RequiresPlatformSandbox: request.RequiresPlatformSandbox, Capabilities: request.Backend.Capabilities(policy), Restrictions: request.Backend.restrictions(policy), - Warnings: request.Backend.Warnings(), + Warnings: append(request.Backend.Warnings(), request.denyReadDiagnosticWarnings()...), } } +// denyReadWarningHostGOOS is the host this process runs on, as far as the +// DenyRead warning is concerned. A var so a test can drive both sides. +var denyReadWarningHostGOOS = runtime.GOOS + +// windowsDenyReadWarnings reports that configuring DenyRead on Windows costs the +// restricted token's write jail. +// +// A profile with DenyRead paths selects the token shape WITHOUT WRITE_RESTRICTED +// (the runner sets writeRestricted false exactly when DenyRead is non-empty), +// because the restricted-SID check has to cover reads for read-deny to mean +// anything. That shape must keep the World SID on its restricted-SID list or the +// token cannot open cmd.exe at all, and every principal carries the World SID, so +// the write half of the jail passes for free on any Everyone-writable path. +// +// The trade is deliberate and documented in the token code, but it was invisible: +// nothing told the person who set DenyRead that they had given up the write jail +// to get it. Zero never populates DenyRead on Windows itself, so this only +// reaches users who configured it. Tracked as #869; when that closes, this +// warning goes with it. +func windowsDenyReadWarnings(backend Backend, profile PermissionProfile) []string { + // The HOST has to be Windows, not merely the target backend. + // + // This describes a token the Windows command runner will build, and that + // runner only ever runs on Windows. A Windows-targeted plan built anywhere + // else is a cross-platform planning exercise, and its DenyRead does not come + // from a Windows user at all: credentialDenyReadPaths returns empty ON + // Windows and populates itself from the host everywhere else, so a plan built + // on Linux carries that machine's credential paths and would draw a warning + // about a token nothing will build. + // + // Indirected through a var so both sides stay testable from any host, the + // same reason windowsSandboxInitialized is one. + if denyReadWarningHostGOOS != "windows" { + return nil + } + if backend.Name != BackendWindowsRestrictedToken || !backend.NativeIsolation { + return nil + } + if len(normalizeProfilePaths(profile.FileSystem.DenyRead)) == 0 { + return nil + } + return []string{ + "denyRead is configured, so the Windows sandbox uses the token shape without WRITE_RESTRICTED: reads are denied as requested, but writes outside the workspace are not confined by the token (#869)", + } +} + +// denyReadDiagnosticWarnings is the diagnostic half of the DenyRead disclosure, +// gated on the plan this request resolves to rather than on the backend that +// happens to be installed. +// +// request.Backend is always the AVAILABLE backend, so on a Windows host it stays +// the restricted-token backend with NativeIsolation set even when nothing will be +// sandboxed. Keying the warning off it meant --sandbox forbid with deny_read +// configured reported a token trade on a plan whose enforcement is disabled and +// whose target is none. +// +// CommandWrapped is the forward reading of "this plan will be wrapped", which is +// what BuildExecutionRequest sets it to; there is no CommandPlan on this path to +// read plan.Wrapped from. +func (request SandboxExecutionRequest) denyReadDiagnosticWarnings() []string { + if !request.CommandWrapped || !request.willBuildWindowsRestrictedToken() { + return nil + } + return windowsDenyReadWarnings(request.Backend, request.PermissionProfile) +} + func permissionProfileUnset(profile PermissionProfile) bool { return profile.FileSystem.Kind == "" && profile.Network.Mode == "" } diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 8528e7e82..5b193b832 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -70,6 +70,20 @@ type CommandPlan struct { // workspace. It carries structured policy facts; command output is never // parsed as the control protocol. executionReportPath string + // childLaunchReported marks a plan whose helper publishes the authoritative + // child-launch fact through executionReportPath. Set ONLY by adapters that + // actually write it: Wrapped alone is not enough, since a bwrap plan is also + // wrapped and reports only denials, and treating its silence as "no child" + // would deny every successful Linux sandbox run its disclosure. + childLaunchReported bool +} + +// ChildLaunchOwnedByAdapter reports whether this plan starts a WRAPPER whose +// helper creates the requested process itself, so the requested child launched +// only if the adapter says so. False for a direct command, where the process the +// caller starts IS the requested one. +func (plan CommandPlan) ChildLaunchOwnedByAdapter() bool { + return plan.childLaunchReported } // Cleanup releases any resources the plan holds. It is safe to call on a zero @@ -131,15 +145,12 @@ func (engine *Engine) PrepareExecution(ctx context.Context, request execution.Re return execution.PreparedCommand{}, err } return execution.PreparedCommand{ - Command: command, - Enforcement: execution.Enforcement{ - Backend: string(plan.TargetBackend), - Level: string(plan.EnforcementLevel), - Degraded: plan.EnforcementLevel == EnforcementDegraded, - DowngradeReason: plan.DowngradeReason, - }, - Report: plan.ExecutionReport, - Cleanup: plan.Cleanup, + Command: command, + Enforcement: EnforcementFor(plan), + Report: plan.ExecutionReport, + Cleanup: plan.Cleanup, + // Only for adapters that publish the fact; see CommandPlan.childLaunchReported. + ChildLaunchOwnedByAdapter: plan.childLaunchReported, }, nil } @@ -319,6 +330,28 @@ func withSandboxExecutionMetadata(plan CommandPlan, request SandboxExecutionRequ plan.EnforcementLevel = request.EnforcementLevel plan.DowngradeReason = request.DowngradeReason plan.RequiresPlatformSandbox = request.RequiresPlatformSandbox + // THE EXECUTION PATH GETS THE SAME NOTICE THE DIAGNOSTICS DO. BackendPlan + // carries these for `zero sandbox policy` and `zero sandbox check`, which an + // operator may never run. A real tool call takes this path instead, and the + // Windows runner selects the token shape from the resolved profile alone: as + // soon as DenyRead is non-empty it drops WRITE_RESTRICTED and the write jail + // stops confining writes outside the workspace. Approving + // file_system.deny_read for one command could therefore cost the jail with + // nothing said about it. + // + // Derived here rather than at each caller because this is the single funnel + // every plan passes through, including the Windows one, so no execution + // caller can be added that quietly misses it. + // KEYED ON WHAT WILL ACTUALLY RUN, not on configuration. The predicate used to + // ask only about the host, the backend and DenyRead, so a disabled sandbox or a + // re-entrant command, both of which take the direct unwrapped plan while still + // carrying the Windows backend and profile, were told the write jail had been + // traded away. Neither claim was true there: no restricted token is created and + // the deny-read rule is not enforced either, so the notice described a trade + // nobody had made. + if windowsRestrictedTokenWillRun(plan, request) { + plan.Notes = append(plan.Notes, windowsDenyReadWarnings(request.Backend, request.PermissionProfile)...) + } return plan } @@ -1186,3 +1219,86 @@ func isDynamicSensitiveEnvKey(key string) bool { strings.HasSuffix(key, suffix) && len(key) > len(prefix)+len(suffix) } + +// EnforcementFor projects a CommandPlan onto the platform-neutral enforcement +// contract. +// +// ONE PROJECTION, because there were two and they drifted. PrepareExecution +// built execution.Enforcement by hand for the generic adapter that hooks, +// plugins and MCP processes go through, and exec_command built the same struct +// by hand for the tool path. When Notices was added it reached only the tool +// path, so the contract was true for one wrapper and false for the wrapper other +// execution consumers depend on. A hand-maintained projection duplicated across +// two adapters cannot be kept honest by review; a shared one cannot be missed. +// +// The notice slice is copied rather than aliased so a consumer cannot mutate the +// plan through it. +func EnforcementFor(plan CommandPlan) execution.Enforcement { + return execution.Enforcement{ + Backend: string(plan.TargetBackend), + Level: string(plan.EnforcementLevel), + Degraded: plan.EnforcementLevel == EnforcementDegraded, + DowngradeReason: plan.DowngradeReason, + Notices: append([]string(nil), plan.Notes...), + } +} + +// windowsRestrictedTokenWillRun reports whether this plan will actually be +// wrapped in a Windows restricted token. +// +// The disclosure is about a token shape, so it has to follow the token rather +// than the configuration that would have produced one. buildPlatformCommandPlan +// takes the direct, unwrapped path for a disabled or degraded enforcement level, +// for BackendNone, for a command that does not require a platform sandbox, and +// for one already wrapped by an outer sandbox. None of those creates a token, +// and none of them enforces deny-read. +func windowsRestrictedTokenWillRun(plan CommandPlan, request SandboxExecutionRequest) bool { + // KEYED ON THE PRODUCED PLAN, not on the request that asked for one. + // + // This read request.CommandWrapped as "something already wrapped this, so we + // are re-entrant", and that is the opposite of what the field means. + // BuildExecutionRequest sets it TRUE for exactly the native and unelevated + // requests that buildPlatformCommandPlan then routes to + // windowsRestrictedTokenCommandPlan. So the disclosure was suppressed on every + // plan that actually creates the token, and fired on none of them. The test + // passed only because its hand-built request left the field false, which is + // the shape no real execution has. + // + // plan.Wrapped is the resulting execution state and cannot be read backwards: + // directCommandPlan sets it false, the restricted-token plan sets it true, and + // both arrive here through the same funnel. + if !plan.Wrapped { + return false + } + return request.willBuildWindowsRestrictedToken() +} + +// willBuildWindowsRestrictedToken reports whether the RESOLVED PLAN creates the +// restricted token, from the request alone. +// +// Split out because the diagnostics had no such gate. BackendPlan asked only +// about the available backend and the requested profile, so `zero sandbox policy` +// and `zero sandbox check` described a token trade on a plan that builds no +// token: with deny_read configured and --sandbox forbid, enforcement resolves to +// disabled and the target to none, and the warning still claimed "reads are +// denied as requested". Nothing was denied and no token existed, so the half that +// reassures was the false one. +// +// plan.Wrapped stays with the caller above rather than moving in here. It is the +// produced execution state and the request cannot speak for it, so folding it in +// would let an unwrapped plan through on the execution path to buy symmetry with +// the diagnostic one. +func (request SandboxExecutionRequest) willBuildWindowsRestrictedToken() bool { + if !request.RequiresPlatformSandbox { + return false + } + if request.EnforcementLevel == EnforcementDisabled || request.EnforcementLevel == EnforcementDegraded { + return false + } + switch request.TargetBackend { + case BackendWindowsRestrictedToken, BackendWindowsElevated: + return true + default: + return false + } +} diff --git a/internal/sandbox/windows_command_runner_windows.go b/internal/sandbox/windows_command_runner_windows.go index 0b9e8f64e..3ff54644b 100644 --- a/internal/sandbox/windows_command_runner_windows.go +++ b/internal/sandbox/windows_command_runner_windows.go @@ -89,6 +89,11 @@ func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writ return exitCode } +// applyWindowsUnelevatedACLPlanFn is a seam. The failure branch below builds +// the guidance an operator acts on, and that text is only correct by +// inspection until something drives the branch and reads it back. +var applyWindowsUnelevatedACLPlanFn = applyWindowsACLPlan + // ensureWindowsUnelevatedSetup applies the workspace ACL plan from the current // (non-elevated) process so the write-restricted token has somewhere its // capability SIDs are granted. DACL edits on user-owned workspace and temp @@ -111,9 +116,16 @@ func ensureWindowsUnelevatedSetup(config WindowsSandboxCommandConfig) error { if marker.contains(applied) { return nil } - if _, err := applyWindowsACLPlan(plan); err != nil { + if _, err := applyWindowsUnelevatedACLPlanFn(plan); err != nil { + // Both remedies below are real. An earlier version offered `--sandbox + // forbid`, which is not: SandboxPreferenceForbid is an internal engine + // state with no flag behind it, so following that advice produced an + // unknown option and left the reader stuck on a failure they had just been + // told how to clear. A recovery instruction that does not work is worse + // than none, because it costs the reader the time to discover that. return fmt.Errorf("apply unelevated workspace ACLs: %w — the workspace may be on a filesystem the current user does not own; "+ - "run `zero sandbox setup` from an elevated (Administrator) terminal, or re-run with `--sandbox forbid` to skip OS sandboxing", err) + "run `zero sandbox setup` from an elevated (Administrator) terminal, "+ + `or turn the sandbox off in your user config with "sandbox": {"enabled": false}`, err) } return recordWindowsUnelevatedAppliedPlan(config.SandboxHome, applied) } diff --git a/internal/sandbox/windows_deny_read_diagnostic_test.go b/internal/sandbox/windows_deny_read_diagnostic_test.go new file mode 100644 index 000000000..283441e65 --- /dev/null +++ b/internal/sandbox/windows_deny_read_diagnostic_test.go @@ -0,0 +1,107 @@ +package sandbox + +import ( + "strings" + "testing" +) + +// diagnosticWarnings renders what `zero sandbox policy` and `zero sandbox check` +// show, through the manager rather than by hand, so the request carries the state +// the resolution actually produces. +func diagnosticWarnings(t *testing.T, mode PolicyMode, denyRead []string, preference SandboxPreference) (SandboxExecutionRequest, []string) { + t.Helper() + workspace := t.TempDir() + backend := windowsRestrictedTokenBackend() + backend.CommandWrapping = true + backend.Executable = `C:\Windows\System32\cmd.exe` + manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend}) + policy := Policy{Mode: mode, EnforceWorkspace: true, DenyRead: denyRead} + request, err := manager.BuildExecutionRequest(SandboxManagerRequest{ + WorkspaceRoot: workspace, + Command: CommandSpec{Name: "cmd.exe", Args: []string{"/c", "echo hi"}, Dir: workspace}, + Policy: policy, + Preference: preference, + }) + if err != nil { + t.Fatalf("BuildExecutionRequest: %v", err) + } + return request, request.BackendPlan(policy).Warnings +} + +func denyReadWarning(warnings []string) string { + for _, warning := range warnings { + if strings.Contains(strings.ToLower(warning), "denyread") { + return warning + } + } + return "" +} + +// THE DIAGNOSTICS HAVE TO DESCRIBE THE RESOLVED PLAN, NOT THE INSTALLED BACKEND. +// +// request.Backend is always the AVAILABLE backend, so on a Windows host it stays +// the restricted-token backend with NativeIsolation set even when the resolution +// disables sandboxing entirely. BackendPlan keyed the warning off that field and +// the requested profile, so `--sandbox forbid` with deny_read configured resolved +// to enforcement disabled and target none, built no token, enforced no read rule, +// and still reported that the write jail had been traded for read denial. +// +// The reassuring half is the one that was false: "reads are denied as requested" +// on a run where nothing is denied at all. +func TestForbiddenSandboxDoesNotClaimTheDenyReadTokenTrade(t *testing.T) { + withWindowsHost(t) + + request, warnings := diagnosticWarnings(t, ModeEnforce, []string{`C:\Users\someone\.config\creds`}, SandboxPreferenceForbid) + + // The preconditions that make this the interesting case rather than a + // vacuous pass: deny_read really did survive into the resolved profile, and + // the plan really does build nothing. + if len(normalizeProfilePaths(request.PermissionProfile.FileSystem.DenyRead)) == 0 { + t.Fatal("deny_read did not reach the resolved profile, so this no longer exercises the diagnostic gate") + } + if request.EnforcementLevel != EnforcementDisabled || request.TargetBackend != BackendNone { + t.Fatalf("expected a forbidden plan to resolve to disabled/none, got level %s target %s", request.EnforcementLevel, request.TargetBackend) + } + + if warning := denyReadWarning(warnings); warning != "" { + t.Errorf("a forbidden sandbox claimed the deny_read token trade on a plan that builds no token and denies no read: %q", warning) + } +} + +// And the warning still fires where the token is real, or the fix above is +// satisfied by never warning at all. +func TestDiagnosticsStillDiscloseTheTradeWhereTheTokenRuns(t *testing.T) { + withWindowsHost(t) + + request, warnings := diagnosticWarnings(t, ModeEnforce, []string{`C:\Users\someone\.config\creds`}, SandboxPreferenceAuto) + if !request.CommandWrapped || request.TargetBackend != BackendWindowsRestrictedToken { + t.Skipf("this environment produced no wrapped Windows plan (target %s, level %s)", request.TargetBackend, request.EnforcementLevel) + } + if denyReadWarning(warnings) == "" { + t.Fatalf("a plan that does build the restricted token disclosed nothing: %v", warnings) + } +} + +// THE EXECUTION-PATH GUARD MUST SURVIVE THE SHARED PREDICATE. +// +// willBuildWindowsRestrictedToken was split out of windowsRestrictedTokenWillRun +// so the diagnostics could reuse it. plan.Wrapped deliberately stayed behind with +// the caller, because the request cannot speak for the produced plan. Folding it +// in would buy symmetry and hand the execution path back the bug the request-side +// checks alone cannot catch. +func TestSharedPredicateDidNotSwallowTheWrappedPlanGuard(t *testing.T) { + withWindowsHost(t) + + request := wrappedWindowsRequest() + if !request.willBuildWindowsRestrictedToken() { + t.Fatal("the request half of the predicate rejected a request that does build the token") + } + // Same request, unwrapped plan. The request half says yes; only plan.Wrapped + // says no, so this is exactly the guard that must not have moved. + if windowsRestrictedTokenWillRun(CommandPlan{Wrapped: false}, request) { + t.Error("an unwrapped plan was reported as building the restricted token") + } + if !windowsRestrictedTokenWillRun(CommandPlan{Wrapped: true}, request) { + t.Error("a wrapped plan that builds the token was reported as not building it") + } +} diff --git a/internal/sandbox/windows_deny_read_disclosure_test.go b/internal/sandbox/windows_deny_read_disclosure_test.go new file mode 100644 index 000000000..30ca79c18 --- /dev/null +++ b/internal/sandbox/windows_deny_read_disclosure_test.go @@ -0,0 +1,137 @@ +package sandbox + +import ( + "strings" + "testing" +) + +// THE DISCLOSURE HAS TO REACH THE PLANS THAT ACTUALLY BUILD THE TOKEN. +// +// The predicate keyed on request.CommandWrapped, read as "something already +// wrapped this". That is the opposite of what the field means: +// BuildExecutionRequest sets it TRUE for exactly the native and unelevated +// requests that then route to windowsRestrictedTokenCommandPlan. So the notice +// was suppressed on every plan that creates the restricted token and fired on +// none of them, while the old test passed because its hand-built request left +// the field false, which is a shape no real execution has. +// +// Built through the manager here rather than by hand, so the request carries the +// state the transition actually produces. +// denyRead goes on the POLICY, not on a hand-built profile. BuildExecutionRequest +// resolves the profile from the policy, so a profile passed in here is discarded +// and the request arrives with an empty DenyRead. That is how the first version +// of this test managed to fail against a working fix. +func windowsDisclosurePlan(t *testing.T, mode PolicyMode, denyRead []string, preference SandboxPreference) CommandPlan { + t.Helper() + workspace := t.TempDir() + backend := windowsRestrictedTokenBackend() + backend.CommandWrapping = true + backend.Executable = `C:\Windows\System32\cmd.exe` + manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend}) + plan, err := manager.BuildCommandPlan(SandboxManagerRequest{ + WorkspaceRoot: workspace, + Command: CommandSpec{Name: "cmd.exe", Args: []string{"/c", "echo hi"}, Dir: workspace}, + Policy: Policy{Mode: mode, EnforceWorkspace: true, DenyRead: denyRead}, + Preference: preference, + }) + if err != nil { + t.Fatalf("BuildCommandPlan: %v", err) + } + return plan +} + +func planNotes(plan CommandPlan) string { + return strings.ToLower(strings.Join(plan.Notes, " ")) +} + +func TestEveryPlanThatBuildsTheRestrictedTokenCarriesTheDisclosure(t *testing.T) { + withWindowsHost(t) + denyRead := []string{`C:\Users\someone\.config\creds`} + + plan := windowsDisclosurePlan(t, ModeEnforce, denyRead, SandboxPreferenceAuto) + if !plan.Wrapped { + t.Skipf("this environment did not produce a wrapped Windows plan (backend %s, level %s)", plan.TargetBackend, plan.EnforcementLevel) + } + if len(plan.Notes) == 0 { + t.Fatalf("a wrapped Windows plan carried no disclosure; every real deny_read execution gets the non-WRITE_RESTRICTED token and is told nothing (level %s)", plan.EnforcementLevel) + } + if !strings.Contains(planNotes(plan), "write") { + t.Errorf("the note does not mention the write jail: %v", plan.Notes) + } +} + +// And the plans that build no token stay silent, or the assertion above would be +// satisfied by a notice attached to everything. A direct unwrapped plan carries +// the Windows backend and the same profile, so this is the case that made the +// original predicate look necessary. +func TestPlansThatBuildNoTokenStaySilent(t *testing.T) { + withWindowsHost(t) + denyRead := []string{`C:\Users\someone\.config\creds`} + + for _, testCase := range []struct { + name string + mode PolicyMode + preference SandboxPreference + }{ + {"sandbox forbidden, so the plan is direct", ModeEnforce, SandboxPreferenceForbid}, + {"sandbox disabled, so nothing is wrapped", ModeDisabled, SandboxPreferenceAuto}, + } { + t.Run(testCase.name, func(t *testing.T) { + plan := windowsDisclosurePlan(t, testCase.mode, denyRead, testCase.preference) + if plan.Wrapped { + t.Fatalf("this case was supposed to produce an unwrapped plan (%s)", plan.EnforcementLevel) + } + if len(plan.Notes) != 0 { + t.Errorf("an unwrapped plan claimed the write jail was traded away: %v", plan.Notes) + } + }) + } +} + +// THE PROJECTION ITSELF, driven from a real plan. +// +// Everything else about notices in this PR is asserted by handing a constructor +// a Notices slice and checking it comes out the other side. That proves the +// consumers and not the producer: deleting the one line in EnforcementFor that +// puts plan.Notes into Enforcement.Notices left every notice test in the repo +// green, and that line is the whole reason hooks, plugins and MCP see anything. +// +// This starts from a plan the manager built, not a literal, so the chain +// profile -> plan.Notes -> Enforcement.Notices is covered end to end. +func TestEnforcementForCarriesThePlanNoticesToTheGenericContract(t *testing.T) { + withWindowsHost(t) + denyRead := []string{`C:\Users\someone\.config\creds`} + + plan := windowsDisclosurePlan(t, ModeEnforce, denyRead, SandboxPreferenceAuto) + if !plan.Wrapped { + t.Skipf("this environment did not produce a wrapped Windows plan (%s)", plan.EnforcementLevel) + } + if len(plan.Notes) == 0 { + t.Fatal("SETUP INVALID: the plan carries no notes, so the projection has nothing to carry") + } + + enforcement := EnforcementFor(plan) + if len(enforcement.Notices) != len(plan.Notes) { + t.Fatalf("EnforcementFor produced %d notices from %d plan notes; hooks, plugins and MCP read this field and would see nothing", + len(enforcement.Notices), len(plan.Notes)) + } + for index, note := range plan.Notes { + if enforcement.Notices[index] != note { + t.Errorf("notice %d = %q, want %q", index, enforcement.Notices[index], note) + } + } +} + +// And a plan with nothing to disclose projects nothing, or the assertion above +// would be satisfied by a field that is never empty. +func TestEnforcementForCarriesNoNoticesFromASilentPlan(t *testing.T) { + withWindowsHost(t) + + plan := windowsDisclosurePlan(t, ModeEnforce, nil, SandboxPreferenceAuto) + if len(plan.Notes) != 0 { + t.Fatalf("SETUP INVALID: a plan with no denyRead carries notes: %v", plan.Notes) + } + if notices := EnforcementFor(plan).Notices; len(notices) != 0 { + t.Errorf("a silent plan projected notices: %v", notices) + } +} diff --git a/internal/sandbox/windows_deny_read_warning_test.go b/internal/sandbox/windows_deny_read_warning_test.go new file mode 100644 index 000000000..a7f099fb5 --- /dev/null +++ b/internal/sandbox/windows_deny_read_warning_test.go @@ -0,0 +1,207 @@ +package sandbox + +import ( + "strings" + "testing" +) + +func withWindowsHost(t *testing.T) { + t.Helper() + previous := denyReadWarningHostGOOS + denyReadWarningHostGOOS = "windows" + t.Cleanup(func() { denyReadWarningHostGOOS = previous }) +} + +func windowsRestrictedTokenBackend() Backend { + return Backend{ + Name: BackendWindowsRestrictedToken, + Platform: "windows", + Available: true, + NativeIsolation: true, + } +} + +func profileWithDenyRead(paths ...string) PermissionProfile { + profile := PermissionProfile{} + profile.FileSystem.DenyRead = paths + return profile +} + +// Setting denyRead on Windows silently costs the token's write jail, because it +// selects the shape without WRITE_RESTRICTED and that shape has to keep the World +// SID. The trade is defensible; making it invisible is not. Someone who asked for +// read-deny has no way to discover they gave up write confinement for it. +func TestDenyReadOnWindowsWarnsThatTheWriteJailIsGone(t *testing.T) { + withWindowsHost(t) + warnings := windowsDenyReadWarnings(windowsRestrictedTokenBackend(), profileWithDenyRead(`C:\Users\someone\.config\creds`)) + if len(warnings) == 0 { + t.Fatal("configuring denyRead on Windows produced no warning, so the lost write jail stays invisible") + } + warning := strings.ToLower(strings.Join(warnings, " ")) + // It has to name the cause and the consequence. A warning that says only + // "degraded" sends the reader to the source to find out what changed. + for _, want := range []string{"denyread", "write", "#869"} { + if !strings.Contains(warning, want) { + t.Errorf("warning does not mention %q, so it does not explain the trade: %q", want, warning) + } + } +} + +// The default Windows posture must stay quiet. Zero never populates denyRead on +// Windows itself, so warning unconditionally would train every user to ignore the +// one case that matters. +func TestDefaultWindowsProfileProducesNoDenyReadWarning(t *testing.T) { + withWindowsHost(t) + if warnings := windowsDenyReadWarnings(windowsRestrictedTokenBackend(), PermissionProfile{}); len(warnings) != 0 { + t.Fatalf("the default Windows profile warned about denyRead it does not set: %v", warnings) + } + // Blank and whitespace-only entries are not a configured denyRead either. + if warnings := windowsDenyReadWarnings(windowsRestrictedTokenBackend(), profileWithDenyRead("", " ")); len(warnings) != 0 { + t.Fatalf("empty denyRead entries produced a warning: %v", warnings) + } +} + +// The warning describes one specific token implementation, so it must not appear +// for backends that do not build that token. +func TestDenyReadWarningIsScopedToTheWindowsRestrictedToken(t *testing.T) { + withWindowsHost(t) + others := []Backend{ + {Name: BackendMacOSSeatbelt, Platform: "darwin", Available: true, NativeIsolation: true}, + {Name: BackendLinuxLandlock, Platform: "linux", Available: true, NativeIsolation: true}, + // Same backend name but no native isolation: no token is built, so the + // warning would describe enforcement that is not running at all. + {Name: BackendWindowsRestrictedToken, Platform: "windows", NativeIsolation: false}, + } + for _, backend := range others { + if warnings := windowsDenyReadWarnings(backend, profileWithDenyRead(`C:\secret`)); len(warnings) != 0 { + t.Errorf("backend %q (nativeIsolation=%v) got the Windows token warning: %v", backend.Name, backend.NativeIsolation, warnings) + } + } +} + +// A Windows-targeted plan built on a non-Windows host must stay silent. +// +// This is the case that broke CI rather than a hypothetical. credentialDenyReadPaths +// returns empty ON Windows and populates itself from the host everywhere else, so a +// Windows plan built on a Linux runner carries that machine's credential paths +// (/home/runner/.docker/config.json) and drew a warning about a token nothing would +// ever build. TestSelectBackendChoosesPlatformAdapterWithFallback asserts a Windows +// plan has no warnings, and it only builds Windows plans from other hosts. +func TestNoDenyReadWarningWhenTheHostIsNotWindows(t *testing.T) { + for _, host := range []string{"linux", "darwin"} { + previous := denyReadWarningHostGOOS + denyReadWarningHostGOOS = host + warnings := windowsDenyReadWarnings(windowsRestrictedTokenBackend(), profileWithDenyRead("/home/runner/.docker/config.json")) + denyReadWarningHostGOOS = previous + if len(warnings) != 0 { + t.Errorf("a windows-targeted plan built on %s warned about a token that host will never build: %v", host, warnings) + } + } +} + +// THE EXECUTION PATH, NOT JUST THE DIAGNOSTIC ONE. +// +// The warning above is reachable from BackendPlan, which is what `zero sandbox +// policy` and `zero sandbox check` render. An operator who never runs those +// sees nothing. A real tool call builds a CommandPlan instead, and the Windows +// runner picks the token shape from the resolved profile alone: DenyRead +// non-empty means no WRITE_RESTRICTED, which is the shape #869 is about. So +// approving file_system.deny_read for one command could cost the write jail +// with nothing said. +// +// withSandboxExecutionMetadata is the single funnel every plan passes through, +// including the Windows one, which is why the notice is derived there rather +// than at each caller. +func TestCommandPlanCarriesTheDenyReadDisclosure(t *testing.T) { + withWindowsHost(t) + + // A WRAPPED plan, because the notice follows the token and plan.Wrapped is + // what says a token gets built. This passed CommandPlan{} and relied on the + // request's CommandWrapped field, which meant the opposite of what it was read + // as, so the assertion held while production disclosed nothing. See + // windowsRestrictedTokenWillRun. + plan := withSandboxExecutionMetadata(CommandPlan{Wrapped: true}, wrappedWindowsRequest()) + + if len(plan.Notes) == 0 { + t.Fatal("a command plan resolved with denyRead carried no notice, so the operator loses the write jail without being told") + } + notice := strings.ToLower(strings.Join(plan.Notes, " ")) + for _, want := range []string{"denyread", "write", "#869"} { + if !strings.Contains(notice, want) { + t.Errorf("the execution-path notice does not mention %q: %q", want, notice) + } + } +} + +// And it stays quiet for the ordinary profile, or every Windows command grows a +// notice about a trade nobody made. +func TestCommandPlanCarriesNoDisclosureWithoutDenyRead(t *testing.T) { + withWindowsHost(t) + + plan := withSandboxExecutionMetadata(CommandPlan{Wrapped: true}, SandboxExecutionRequest{ + Backend: windowsRestrictedTokenBackend(), + TargetBackend: BackendWindowsRestrictedToken, + RequiresPlatformSandbox: true, + EnforcementLevel: EnforcementNative, + }) + if len(plan.Notes) != 0 { + t.Errorf("a plan without denyRead carried notices: %v", plan.Notes) + } +} + +// wrappedWindowsRequest is the shape buildPlatformCommandPlan actually wraps in +// a restricted token: a platform sandbox is required, enforcement is native, and +// the target is the Windows restricted-token backend. +// +// It says nothing about CommandWrapped on purpose. That field is TRUE for these +// requests, because it means "this plan will be wrapped" rather than "something +// already wrapped it", and reading it the other way is what made the disclosure +// fire on nothing. +func wrappedWindowsRequest() SandboxExecutionRequest { + return SandboxExecutionRequest{ + Backend: windowsRestrictedTokenBackend(), + TargetBackend: BackendWindowsRestrictedToken, + PermissionProfile: profileWithDenyRead(`C:\Users\someone\.config\creds`), + RequiresPlatformSandbox: true, + EnforcementLevel: EnforcementNative, + } +} + +// THE NOTICE DESCRIBES A TOKEN, SO IT MUST FOLLOW THE TOKEN. +// +// Each case below carries the Windows backend and a DenyRead profile, and each +// takes the direct unwrapped plan rather than the restricted-token one. No token +// is created and the deny-read rule is not enforced, so claiming the write jail +// was traded away is false in both halves. +func TestNoDisclosureWhenNoRestrictedTokenIsCreated(t *testing.T) { + withWindowsHost(t) + + // The direct plan is the case that made the old predicate look necessary: it + // carries the Windows backend and the same DenyRead profile, and builds no + // token at all. + t.Run("the plan is the direct unwrapped one", func(t *testing.T) { + plan := withSandboxExecutionMetadata(CommandPlan{Wrapped: false}, wrappedWindowsRequest()) + if len(plan.Notes) != 0 { + t.Errorf("an unwrapped plan claimed the write jail was traded away: %v", plan.Notes) + } + }) + + for _, testCase := range []struct { + name string + mutate func(*SandboxExecutionRequest) + }{ + {name: "sandboxing disabled", mutate: func(r *SandboxExecutionRequest) { r.EnforcementLevel = EnforcementDisabled }}, + {name: "degraded to no native isolation", mutate: func(r *SandboxExecutionRequest) { r.EnforcementLevel = EnforcementDegraded }}, + {name: "command needs no platform sandbox", mutate: func(r *SandboxExecutionRequest) { r.RequiresPlatformSandbox = false }}, + {name: "no target backend", mutate: func(r *SandboxExecutionRequest) { r.TargetBackend = BackendNone }}, + } { + t.Run(testCase.name, func(t *testing.T) { + request := wrappedWindowsRequest() + testCase.mutate(&request) + plan := withSandboxExecutionMetadata(CommandPlan{Wrapped: true}, request) + if len(plan.Notes) != 0 { + t.Errorf("claimed the write jail was traded away where no restricted token runs: %v", plan.Notes) + } + }) + } +} diff --git a/internal/sandbox/windows_execution_report_windows.go b/internal/sandbox/windows_execution_report_windows.go new file mode 100644 index 000000000..9e474a2c9 --- /dev/null +++ b/internal/sandbox/windows_execution_report_windows.go @@ -0,0 +1,94 @@ +//go:build windows + +package sandbox + +import ( + "encoding/json" + "fmt" + "os" + "strings" + + "github.com/Gitlawb/zero/internal/execution" + "golang.org/x/sys/windows" +) + +// windowsExecutionReport is the helper's side channel back to the parent. +// +// The only fact it carries is whether the REQUESTED process was created. The +// parent starts this helper, so the parent's own exec.Cmd.Process proves the +// helper ran and nothing more: setup-marker validation, ACL application, +// network-policy validation, capability and offline SID construction and +// restricted-token creation all happen afterwards and can each return with no +// sandboxed child. Only this process observes the transition, so only this +// process may report it. +// +// OPENED BEFORE THE LAUNCH, ON PURPOSE. Publishing is not free of failure, and +// once CreateProcessAsUser has succeeded a running child exists whether or not +// the report can be written. Acquiring the file first moves every failure that +// can be moved to a point where there is still nothing to own; what remains is +// handled by reaping the child rather than returning while it runs. +type windowsExecutionReport struct { + file *os.File + path string +} + +// openWindowsExecutionReport claims the report path before anything is launched. +// +// O_EXCL, so a file another local user pre-created at this name makes the helper +// fail here, before any child exists, instead of letting them supply the fact +// the parent reads back. An empty path means the caller wants no report, which +// keeps the standalone helper and every existing test working unchanged. +func openWindowsExecutionReport(path string) (*windowsExecutionReport, error) { + trimmed := strings.TrimSpace(path) + if trimmed == "" { + return &windowsExecutionReport{}, nil + } + file, err := os.OpenFile(trimmed, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { + return nil, fmt.Errorf("open sandbox execution report: %w", err) + } + return &windowsExecutionReport{file: file, path: trimmed}, nil +} + +// publish records the launch fact. Safe on a report the caller never opened. +func (report *windowsExecutionReport) publish(childLaunched bool) error { + if report == nil || report.file == nil { + return nil + } + launched := childLaunched + if err := json.NewEncoder(report.file).Encode(execution.AdapterReport{ChildLaunched: &launched}); err != nil { + return fmt.Errorf("write sandbox execution report: %w", err) + } + return nil +} + +// close releases the handle. Discards the file when nothing was published, so a +// truncated or empty report can never be read back as a launch that happened. +func (report *windowsExecutionReport) close(published bool) { + if report == nil || report.file == nil { + return + } + closeErr := report.file.Close() + if !published || closeErr != nil { + _ = os.Remove(report.path) + } + report.file = nil +} + +// terminateAndReapWindowsChild takes down a child this helper has launched and +// waits for it to actually exit. +// +// Used on the paths where the helper cannot continue after CreateProcessAsUser +// has already succeeded. Returning there without this would leave the requested +// command or MCP server running with nobody waiting on it, cancelling it, or +// cleaning up after it, while the parent reads a missing report, concludes no +// child launched, and is free to start a second one. +func terminateAndReapWindowsChild(process windows.Handle) { + if process == 0 { + return + } + // The exit code is irrelevant: this path is already returning an error, and + // the point is that the child is gone before the helper is. + _ = windows.TerminateProcess(process, 1) + _, _ = windows.WaitForSingleObject(process, windows.INFINITE) +} diff --git a/internal/sandbox/windows_process_windows.go b/internal/sandbox/windows_process_windows.go index f5dde58a0..ce751a2e3 100644 --- a/internal/sandbox/windows_process_windows.go +++ b/internal/sandbox/windows_process_windows.go @@ -56,6 +56,16 @@ func runWindowsCommandAsUser(token windows.Token, config WindowsSandboxCommandCo startup.StdErr = stderr var process windows.ProcessInformation envPtr := &envBlock[0] + // Claim the report side channel BEFORE the launch. Publishing can fail, and + // after CreateProcessAsUser succeeds a running child exists whether or not the + // fact can be recorded; taking the file first moves that failure to a point + // where there is still nothing to own. + report, err := openWindowsExecutionReport(config.ExecutionReportPath) + if err != nil { + return 1, err + } + published := false + defer func() { report.close(published) }() if err := windows.CreateProcessAsUser( token, nil, @@ -73,6 +83,22 @@ func runWindowsCommandAsUser(token windows.Token, config WindowsSandboxCommandCo } defer windows.CloseHandle(process.Process) defer windows.CloseHandle(process.Thread) + // THE TRANSITION ONLY THIS PROCESS CAN SEE. Everything above can fail with the + // helper already running, and the parent's exec.Cmd.Process cannot tell those + // failures apart from a real sandboxed launch. The restricted child exists as + // of this line, so this is where the fact is published. + // + // OWNERSHIP OUTLIVES REPORTING. The child is runnable and may already be + // making external side effects, so a failure to publish must not return from + // here and leave it running with nobody waiting on it: the parent would read a + // missing report, correctly conclude that no child launched, and be free to + // start a second one alongside the first. Take it down and reap it, then + // report the failure. + if err := report.publish(true); err != nil { + terminateAndReapWindowsChild(process.Process) + return 1, fmt.Errorf("record sandboxed child launch: %w", err) + } + published = true if _, err := windows.WaitForSingleObject(process.Process, windows.INFINITE); err != nil { return 1, fmt.Errorf("wait for sandboxed process: %w", err) } diff --git a/internal/sandbox/windows_runner.go b/internal/sandbox/windows_runner.go index 032f2a844..a31dfae28 100644 --- a/internal/sandbox/windows_runner.go +++ b/internal/sandbox/windows_runner.go @@ -3,6 +3,7 @@ package sandbox import ( "crypto/rand" "encoding/binary" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -156,23 +157,30 @@ func windowsShellCommandLineFromArgs(args []string) (string, bool) { } type WindowsSandboxCommandArgsOptions struct { - SandboxHome string - CommandCWD string - WorkspaceRoots []string - PermissionProfile PermissionProfile - Env []string - SandboxLevel WindowsSandboxLevel - Command []string + // ExecutionReportPath is where the helper writes its structured report, + // including the authoritative fact that the sandboxed child was created. + ExecutionReportPath string + SandboxHome string + CommandCWD string + WorkspaceRoots []string + PermissionProfile PermissionProfile + Env []string + SandboxLevel WindowsSandboxLevel + Command []string } type WindowsSandboxCommandConfig struct { - SandboxHome string - CommandCWD string - WorkspaceRoots []string - PermissionProfile PermissionProfile - Env map[string]string - SandboxLevel WindowsSandboxLevel - Command []string + // ExecutionReportPath is the adapter-owned side channel back to the runner. + // Empty when the caller wants no report, which keeps every existing test and + // the standalone helper working unchanged. + ExecutionReportPath string + SandboxHome string + CommandCWD string + WorkspaceRoots []string + PermissionProfile PermissionProfile + Env map[string]string + SandboxLevel WindowsSandboxLevel + Command []string } func BuildWindowsSandboxCommandArgs(options WindowsSandboxCommandArgsOptions) ([]string, error) { @@ -217,6 +225,9 @@ func BuildWindowsSandboxCommandArgs(options WindowsSandboxCommandArgsOptions) ([ "--env-json", string(envJSON), "--windows-sandbox-level", string(level), } + if reportPath := strings.TrimSpace(options.ExecutionReportPath); reportPath != "" { + args = append(args, "--execution-report", reportPath) + } for _, root := range workspaceRoots { args = append(args, "--workspace-root", root) } @@ -249,6 +260,13 @@ func ParseWindowsSandboxCommandArgs(args []string) (WindowsSandboxCommandConfig, } config.SandboxHome = strings.TrimSpace(value) index = next + case "--execution-report": + value, next, err := nextWindowsSandboxFlagValue(args, index) + if err != nil { + return WindowsSandboxCommandConfig{}, err + } + config.ExecutionReportPath = strings.TrimSpace(value) + index = next case "--workspace-root": value, next, err := nextWindowsSandboxFlagValue(args, index) if err != nil { @@ -335,14 +353,23 @@ func windowsRestrictedTokenCommandPlan(execRequest SandboxExecutionRequest, poli if execRequest.EnforcementLevel == EnforcementUnelevated { level = WindowsSandboxLevelUnelevated } + // The helper's side channel back to us. The runner starts the helper, so its + // own exec.Cmd.Process only proves the HELPER ran; everything that makes this + // a sandbox happens inside, after that. The helper writes the authoritative + // child-launch fact here and the runner believes it over its own observation. + reportPath, err := newWindowsExecutionReportPath() + if err != nil { + return CommandPlan{}, err + } args, err := BuildWindowsSandboxCommandArgs(WindowsSandboxCommandArgsOptions{ - SandboxHome: sandboxHome, - CommandCWD: spec.Dir, - WorkspaceRoots: []string{execRequest.WorkspaceRoot}, - PermissionProfile: execRequest.PermissionProfile, - Env: childEnv, - SandboxLevel: level, - Command: append([]string{spec.Name}, spec.Args...), + ExecutionReportPath: reportPath, + SandboxHome: sandboxHome, + CommandCWD: spec.Dir, + WorkspaceRoots: []string{execRequest.WorkspaceRoot}, + PermissionProfile: execRequest.PermissionProfile, + Env: childEnv, + SandboxLevel: level, + Command: append([]string{spec.Name}, spec.Args...), }) if err != nil { return CommandPlan{}, err @@ -353,21 +380,38 @@ func windowsRestrictedTokenCommandPlan(execRequest SandboxExecutionRequest, poli // helper .exe, where args are passed unchanged. fullArgs := append(append([]string{}, execRequest.Backend.ExecutableArgsPrefix...), args...) return withSandboxExecutionMetadata(CommandPlan{ - Backend: execRequest.Backend, - TargetBackend: execRequest.TargetBackend, - WorkspaceRoot: execRequest.WorkspaceRoot, - Policy: policy, - Wrapped: true, - SandboxEnvMarkers: execRequest.SandboxEnvMarkers, - EnforcementLevel: execRequest.EnforcementLevel, - Name: execRequest.Backend.Executable, - Args: fullArgs, - Dir: spec.Dir, - Env: childEnv, - SandboxDir: spec.Dir, + Backend: execRequest.Backend, + TargetBackend: execRequest.TargetBackend, + WorkspaceRoot: execRequest.WorkspaceRoot, + Policy: policy, + Wrapped: true, + SandboxEnvMarkers: execRequest.SandboxEnvMarkers, + EnforcementLevel: execRequest.EnforcementLevel, + Name: execRequest.Backend.Executable, + Args: fullArgs, + Dir: spec.Dir, + Env: childEnv, + SandboxDir: spec.Dir, + executionReportPath: reportPath, + childLaunchReported: true, + cleanup: func() { + _ = os.Remove(reportPath) + }, }, execRequest), nil } +// newWindowsExecutionReportPath names the helper's report file under the +// per-user temp directory. Random, and the helper creates it with O_EXCL, so a +// name another local user pre-created makes the write fail rather than letting +// them dictate the fact the runner reads back. +func newWindowsExecutionReportPath() (string, error) { + var token [16]byte + if _, err := rand.Read(token[:]); err != nil { + return "", fmt.Errorf("generate sandbox execution report path: %w", err) + } + return filepath.Join(os.TempDir(), "zero-sandbox-report-"+hex.EncodeToString(token[:])+".json"), nil +} + func upsertEnvList(env []string, values ...string) []string { out := cloneStrings(env) for _, value := range values { diff --git a/internal/sandbox/windows_token_windows_test.go b/internal/sandbox/windows_token_windows_test.go new file mode 100644 index 000000000..3ac0e5efe --- /dev/null +++ b/internal/sandbox/windows_token_windows_test.go @@ -0,0 +1,187 @@ +//go:build windows + +package sandbox + +import ( + "strings" + "testing" + "unsafe" + + "golang.org/x/sys/windows" +) + +// A SID that parses but names nothing on the machine. CreateRestrictedToken does +// not require a restricting SID to resolve, and using a real group would make the +// test depend on local account layout. +const testCapabilitySID = "S-1-5-21-1111111111-1111111111-1111111111-4001" + +// restrictedSIDStrings returns the token's restricted-SID list. +// +// This list IS the write jail. Under WRITE_RESTRICTED a write must pass both the +// ordinary access check and a second check against these SIDs, so the jail holds +// exactly as long as the list contains nothing every principal already carries. +func restrictedSIDStrings(t *testing.T, token windows.Token) []string { + t.Helper() + var size uint32 + err := windows.GetTokenInformation(token, windows.TokenRestrictedSids, nil, 0, &size) + if err != nil && err != windows.ERROR_INSUFFICIENT_BUFFER { + t.Fatalf("size restricted SID list: %v", err) + } + if size == 0 { + return nil + } + buffer := make([]byte, size) + if err := windows.GetTokenInformation(token, windows.TokenRestrictedSids, &buffer[0], size, &size); err != nil { + t.Fatalf("read restricted SID list: %v", err) + } + groups := (*windows.Tokengroups)(unsafe.Pointer(&buffer[0])) + values := make([]string, 0, groups.GroupCount) + for _, group := range groups.AllGroups() { + values = append(values, group.Sid.String()) + } + return values +} + +func restrictedTokenForTest(t *testing.T, writeRestricted bool) windows.Token { + t.Helper() + token, err := createWindowsRestrictedTokenForCapabilitySIDs([]string{testCapabilitySID}, writeRestricted) + if err != nil { + t.Fatalf("create restricted token (writeRestricted=%v): %v", writeRestricted, err) + } + t.Cleanup(func() { _ = token.Close() }) + return token +} + +func containsSID(values []string, want string) bool { + for _, value := range values { + if strings.EqualFold(value, want) { + return true + } + } + return false +} + +// THE REGRESSION GUARD FOR #865. The World SID (Everyone) must not be a +// restricting SID on the WRITE_RESTRICTED token. +// +// Every principal carries Everyone, so if it is on this list the second check +// passes for free on any path whose DACL grants Everyone write, and confinement +// silently falls back to the user's own permissions. No privilege, no symlink and +// no race is needed; an Everyone-writable directory is enough. +// +// This is a unit test on purpose. The existing coverage +// (TestWindowsRestrictedTokenDeniesWritesToEveryoneWritablePaths) sits behind +// ZERO_SANDBOX_REAL_SMOKE=1, which no workflow sets, so until now a refactor that +// restored the unconditional World SID went green in CI. CreateRestrictedToken +// works unelevated against the caller's own token, so there is no reason this +// invariant cannot be checked on every run. +func TestWriteRestrictedTokenExcludesTheWorldSID(t *testing.T) { + values := restrictedSIDStrings(t, restrictedTokenForTest(t, true)) + if len(values) == 0 { + t.Fatal("write-restricted token has no restricting SIDs at all, so there is no write jail to speak of") + } + if containsSID(values, "S-1-1-0") { + t.Fatalf("the World SID is a restricting SID on the write-restricted token, which collapses the write jail: %v", values) + } +} + +// No universal group belongs on this list, for the same reason Everyone does not. +// #869 calls these out by name as the ones that would reopen the gap, and the +// runner's own comment already states the rule, so this pins it rather than +// trusting the next reader to remember. +// +// Checked on BOTH token shapes: the non-WRITE_RESTRICTED one still must not gain +// any of these beyond the World SID it is documented to carry. +func TestRestrictedSIDListNeverCarriesABroadGroup(t *testing.T) { + forbidden := map[string]string{ + "S-1-5-32-545": `BUILTIN\Users`, + "S-1-5-11": "Authenticated Users", + "S-1-5-4": "INTERACTIVE", + "S-1-5-3": "BATCH", + "S-1-5-32-544": `BUILTIN\Administrators`, + "S-1-5-18": "SYSTEM", + "S-1-5-6": "SERVICE", + "S-1-5-2": "NETWORK", + } + for _, writeRestricted := range []bool{true, false} { + values := restrictedSIDStrings(t, restrictedTokenForTest(t, writeRestricted)) + for sid, name := range forbidden { + if containsSID(values, sid) { + t.Errorf("writeRestricted=%v: %s (%s) is a restricting SID; it has write access nearly everywhere, so the jail would not hold", + writeRestricted, name, sid) + } + } + // The user's own SID is the boundary this token exists to be stricter + // than, so it must never be its own key. + if user := currentUserSIDForTest(t); containsSID(values, user) { + t.Errorf("writeRestricted=%v: the current user SID is a restricting SID, which defeats the token entirely", writeRestricted) + } + } +} + +// The capability SID must actually be present, or the jail denies everything and +// the sandbox cannot write even where Zero granted access. A test that only +// checked for absences would pass against a token with an empty list. +func TestRestrictedSIDListCarriesTheCapabilitySID(t *testing.T) { + for _, writeRestricted := range []bool{true, false} { + values := restrictedSIDStrings(t, restrictedTokenForTest(t, writeRestricted)) + if !containsSID(values, testCapabilitySID) { + t.Errorf("writeRestricted=%v: the capability SID is missing from %v, so no ACL-granted path would be writable", + writeRestricted, values) + } + } +} + +// Documents the gap #869 tracks rather than asserting the desired end state. +// +// Without WRITE_RESTRICTED the restricted-SID check covers reads too, and default +// Windows DACLs grant BUILTIN\Users, so a token with no universal group cannot +// open cmd.exe and dies at launch with STATUS_ACCESS_DENIED. Everyone is +// load-bearing here, which is why #865 could not remove it from this shape. +// +// The consequence is that this shape, selected whenever a profile sets DenyRead, +// has no effective write jail. If someone closes #869 by giving reads a grant +// that is not a universal group, this test FAILS and must be replaced by the +// exclusion assertion in the same change, rather than deleted or skipped past. +func TestNonWriteRestrictedTokenStillCarriesTheWorldSID(t *testing.T) { + values := restrictedSIDStrings(t, restrictedTokenForTest(t, false)) + if !containsSID(values, "S-1-1-0") { + // FAILS rather than skips, and the difference matters more than it looks. + // + // This SID is availability-critical as well as security-relevant: without + // WRITE_RESTRICTED the restricted-SID check covers reads, default Windows + // DACLs grant BUILTINUsers, and a token carrying no universal group cannot + // open cmd.exe. Removing it therefore breaks every command with DenyRead at + // launch. A skip here would let exactly that land on green CI, which is the + // one outcome this test exists to prevent. + // + // If you are reading this because you deliberately changed the token shape + // for #869: good, and this assertion is now yours to replace, in the same + // change, with tests proving the new token still launches an ordinary + // executable, still denies the intended read path, and has not restored the + // broad write bypass. Deleting it without those is not the same thing. + t.Fatal("the World SID is gone from the DenyRead token shape: every DenyRead command now fails at launch unless reads were given a non-universal grant; replace this assertion with the #869 exclusion and launch tests") + } + t.Log("known gap (#869): the DenyRead token shape carries the World SID, so its write jail does not hold") +} + +// currentUserSIDForTest FAILS rather than returning empty. +// +// It used to swallow the error, and its one caller guarded on the result being +// non-empty, so a machine where GetTokenUser fails ran the assertion on nothing +// and reported a pass. The check exists to catch the token keying itself to the +// very SID it must be stricter than, which is the whole point of the shape, so +// not being able to read the prerequisite is a failure and not a skip. +func currentUserSIDForTest(t *testing.T) string { + t.Helper() + token := windows.GetCurrentProcessToken() + user, err := token.GetTokenUser() + if err != nil { + t.Fatalf("read the current user SID, which this assertion depends on: %v", err) + } + sid := user.User.Sid.String() + if strings.TrimSpace(sid) == "" { + t.Fatal("the current user SID came back empty, so the assertion below would check nothing") + } + return sid +} diff --git a/internal/sandbox/windows_unelevated_guidance_windows_test.go b/internal/sandbox/windows_unelevated_guidance_windows_test.go new file mode 100644 index 000000000..671d8cd35 --- /dev/null +++ b/internal/sandbox/windows_unelevated_guidance_windows_test.go @@ -0,0 +1,115 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "strings" + "testing" +) + +// EVERY REMEDY THIS ERROR NAMES MUST BE ONE THE READER CAN CARRY OUT. +// +// The message told operators to re-run with `--sandbox forbid`. No such option +// exists: SandboxPreferenceForbid is an internal engine state with no flag +// behind it, so acting on the advice produced an unknown option and left them +// stuck on the failure they had just been told how to clear. +// +// It survived because nothing drove this branch. The text was only ever correct +// by inspection, and inspection is what missed it, so the fix is not complete +// until something fails the apply and reads the guidance back. +func TestUnelevatedACLFailureNamesOnlyRealRemedies(t *testing.T) { + workspace := t.TempDir() + config := WindowsSandboxCommandConfig{ + SandboxHome: t.TempDir(), + CommandCWD: workspace, + WorkspaceRoots: []string{workspace}, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: workspace}}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + SandboxLevel: WindowsSandboxLevelUnelevated, + } + + denied := errors.New("Access is denied.") + original := applyWindowsUnelevatedACLPlanFn + t.Cleanup(func() { applyWindowsUnelevatedACLPlanFn = original }) + applyWindowsUnelevatedACLPlanFn = func(WindowsACLPlan) (func() error, error) { + return nil, denied + } + + err := ensureWindowsUnelevatedSetup(config) + if err == nil { + t.Fatal("ensureWindowsUnelevatedSetup returned nil when the ACL apply failed, so the command would run believing it was sandboxed") + } + + // The refusal has to keep naming its cause, or the operator cannot tell an + // ACL failure apart from the sandboxed command being rejected. + if !errors.Is(err, denied) { + t.Errorf("error does not wrap the apply failure, so the cause is lost: %v", err) + } + + message := err.Error() + + // The option that does not exist must never come back. + if strings.Contains(message, "--sandbox forbid") { + t.Errorf("error still advertises `--sandbox forbid`, which is not a real option: %s", message) + } + + // Both surviving remedies are real: elevated setup, and the user-config key, + // which is honored from global config only so a cloned repo cannot set it. + for _, want := range []string{ + "zero sandbox setup", + `"sandbox": {"enabled": false}`, + } { + if !strings.Contains(message, want) { + t.Errorf("error does not offer %q, leaving the reader without a way out: %s", want, message) + } + } +} + +// The failure must not be recorded as a success. The applied-plan marker is +// what makes later commands skip the re-apply, so writing it here would turn +// one refusal into a sandbox that silently never applies its ACLs again. +func TestUnelevatedACLFailureDoesNotRecordTheMarker(t *testing.T) { + workspace := t.TempDir() + home := t.TempDir() + config := WindowsSandboxCommandConfig{ + SandboxHome: home, + CommandCWD: workspace, + WorkspaceRoots: []string{workspace}, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: workspace}}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + SandboxLevel: WindowsSandboxLevelUnelevated, + } + + original := applyWindowsUnelevatedACLPlanFn + t.Cleanup(func() { applyWindowsUnelevatedACLPlanFn = original }) + applyWindowsUnelevatedACLPlanFn = func(WindowsACLPlan) (func() error, error) { + return nil, errors.New("Access is denied.") + } + + if err := ensureWindowsUnelevatedSetup(config); err == nil { + t.Fatal("expected the apply failure to surface") + } + + applied, _, err := buildWindowsUnelevatedAppliedPlan(config) + if err != nil { + t.Fatalf("buildWindowsUnelevatedAppliedPlan: %v", err) + } + marker, err := loadWindowsUnelevatedSetupMarker(home) + if err != nil { + t.Fatalf("loadWindowsUnelevatedSetupMarker: %v", err) + } + if marker.contains(applied) { + t.Error("the failed plan was recorded as applied, so every later command would skip the apply and run unjailed") + } +} diff --git a/internal/tools/applied_notice_test.go b/internal/tools/applied_notice_test.go new file mode 100644 index 000000000..1ab84c6c1 --- /dev/null +++ b/internal/tools/applied_notice_test.go @@ -0,0 +1,76 @@ +package tools + +import ( + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/execution" +) + +const appliedNotice = "denyRead is configured, so the write jail is not confining writes" + +// THE PLAN IS NOT THE APPLICATION. +// +// addSandboxMeta writes the plan's notices at plan time, before anything runs, +// so promoting them into the user-visible disclosure unconditionally claims a +// token trade for a command that may never have started. The execution outcome +// is the thing that knows whether a process existed, and it applies the same +// launched-and-planned rule hooks and plugins use. +func TestCommandNoticesFollowAppliedExecutionState(t *testing.T) { + planned := map[string]string{sandboxNoticesMeta: appliedNotice} + + t.Run("launched", func(t *testing.T) { + outcome := execution.Outcome{ + Kind: execution.OutcomeSuccess, + Launched: true, + Enforcement: execution.Enforcement{Notices: []string{appliedNotice}}, + } + got := finalizeToolOutcome(Result{ + Status: StatusOK, Output: "ran", Meta: planned, ExecutionOutcome: &outcome, + }, "ran") + if len(got.EnforcementNotices) != 1 { + t.Fatalf("a launched command lost its disclosure: %#v", got.EnforcementNotices) + } + if !strings.Contains(got.ModelOutput(), appliedNotice) { + t.Errorf("the model view does not carry it: %q", got.ModelOutput()) + } + }) + + t.Run("never launched", func(t *testing.T) { + outcome := execution.Outcome{ + Kind: execution.OutcomeSandboxSetupFailure, + Launched: false, + Enforcement: execution.Enforcement{Notices: []string{appliedNotice}}, + } + got := finalizeToolOutcome(Result{ + Status: StatusError, Output: "could not start", Meta: planned, ExecutionOutcome: &outcome, + }, "could not start") + if len(got.EnforcementNotices) != 0 { + t.Fatalf("a command that never started claimed a token trade: %#v", got.EnforcementNotices) + } + if strings.Contains(got.ModelOutput(), appliedNotice) { + t.Errorf("the model view claims it anyway: %q", got.ModelOutput()) + } + }) + + // The plan metadata is diagnostics and stays put either way, so the record of + // what was intended is not lost with the claim about what happened. + t.Run("metadata survives", func(t *testing.T) { + outcome := execution.Outcome{Kind: execution.OutcomeSandboxSetupFailure, Launched: false} + got := finalizeToolOutcome(Result{ + Status: StatusError, Output: "x", Meta: planned, ExecutionOutcome: &outcome, + }, "x") + if got.Meta[sandboxNoticesMeta] != appliedNotice { + t.Errorf("the planned notice was erased from diagnostics: %q", got.Meta[sandboxNoticesMeta]) + } + }) + + // A tool with no execution outcome at all still promotes from metadata, so + // this did not silently drop disclosure for a path that has no outcome. + t.Run("no execution outcome", func(t *testing.T) { + got := finalizeToolOutcome(Result{Status: StatusOK, Output: "x", Meta: planned}, "x") + if len(got.EnforcementNotices) != 1 { + t.Errorf("a tool without an execution outcome lost its disclosure: %#v", got.EnforcementNotices) + } + }) +} diff --git a/internal/tools/bash.go b/internal/tools/bash.go index 6274c806c..38b413c23 100644 --- a/internal/tools/bash.go +++ b/internal/tools/bash.go @@ -164,8 +164,22 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS // A no-op when MonitorTag is empty, so the default path is unchanged. monitor := zeroSandbox.StartDenialMonitor(context.Background(), plan.MonitorTag) err = command.Run() + // OBSERVED HERE, at the only boundary that knows. exec.Cmd sets Process + // only once os.StartProcess has succeeded, so this separates a child that + // ran from a pre-start failure (missing executable, context already + // cancelled) that Run reports the same way. Every branch below hands this + // to withBashExecution rather than letting the conversion assume it. + launched := command.Process != nil exitCode := commandExitCode(err) adapterReport, reportErr := plan.ExecutionReport() + // AND FOR A WRAPPED PLAN THAT OBSERVATION IS OF THE WRAPPER. On Windows the + // command started here is the sandbox helper; it creates the requested child + // only after marker, ACL, network, SID and token setup, any of which can fail + // with the helper already running. Reading the report was not enough on its + // own: the launch decision has to consume it, or bash promotes the planned + // DenyRead notice for a command that never ran under that enforcement. Same + // resolution the captured runner uses, so the two cannot drift. + launched = execution.ResolveChildLaunched(launched, plan.ChildLaunchOwnedByAdapter(), adapterReport) meta["exit_code"] = strconv.Itoa(exitCode) stdoutText := stdout.retained() stderrRetained := stderr.retained() @@ -180,7 +194,7 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS Output: fmt.Sprintf("Error: Command timed out after %dms.", timeoutMS), Meta: meta, } - return withBashExecution(result, executionRequest, plan, exitCode, adapterReport, reportErr, changeObserver.Changes(), true) + return withBashExecution(result, executionRequest, plan, exitCode, adapterReport, reportErr, launched, changeObserver.Changes(), true) } if err != nil { if exitCode < 0 { @@ -189,7 +203,7 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS Output: "Error executing command: " + err.Error(), Meta: meta, } - return withBashExecution(result, executionRequest, plan, exitCode, adapterReport, reportErr, changeObserver.Changes(), false) + return withBashExecution(result, executionRequest, plan, exitCode, adapterReport, reportErr, launched, changeObserver.Changes(), false) } if adapterReport.Denial != nil { markStructuredSandboxDenial(meta, *adapterReport.Denial) @@ -201,7 +215,7 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS Truncated: truncated, Meta: meta, } - return withBashExecution(result, executionRequest, plan, exitCode, adapterReport, reportErr, changeObserver.Changes(), false) + return withBashExecution(result, executionRequest, plan, exitCode, adapterReport, reportErr, launched, changeObserver.Changes(), false) } if adapterReport.Denial != nil { @@ -214,12 +228,13 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS Truncated: truncated, Meta: meta, } - return withBashExecution(result, executionRequest, plan, exitCode, adapterReport, reportErr, changeObserver.Changes(), false) + return withBashExecution(result, executionRequest, plan, exitCode, adapterReport, reportErr, launched, changeObserver.Changes(), false) } -func withBashExecution(result Result, request execution.Request, plan zeroSandbox.CommandPlan, exitCode int, report execution.AdapterReport, reportErr error, changes []execution.Change, timedOut bool) Result { +func withBashExecution(result Result, request execution.Request, plan zeroSandbox.CommandPlan, exitCode int, report execution.AdapterReport, reportErr error, launched bool, changes []execution.Change, timedOut bool) Result { input := execToolResultInput{ exited: true, + launched: launched, exitCode: exitCode, enforcement: executionEnforcement(plan), request: request, @@ -349,6 +364,13 @@ func addSandboxMeta(meta map[string]string, plan zeroSandbox.CommandPlan) { if plan.DowngradeReason != "" { meta["sandbox_downgrade_reason"] = plan.DowngradeReason } + // Least-privilege notices for the command actually being run, on the same + // channel as the downgrade reason. Without this the DenyRead write-jail + // trade was visible only to `zero sandbox policy` and `zero sandbox check`, + // so an operator could approve it per command and never be told. + if len(plan.Notes) > 0 { + meta["sandbox_notices"] = strings.Join(plan.Notes, "\n") + } meta["sandbox_requires_platform"] = strconv.FormatBool(plan.RequiresPlatformSandbox) if plan.Backend.Message != "" { meta["sandbox_message"] = plan.Backend.Message diff --git a/internal/tools/bash_launch_state_test.go b/internal/tools/bash_launch_state_test.go new file mode 100644 index 000000000..293aad6c2 --- /dev/null +++ b/internal/tools/bash_launch_state_test.go @@ -0,0 +1,68 @@ +package tools + +import ( + "context" + "strings" + "testing" +) + +// THE DISCLOSURE FOLLOWS THE PROCESS, AND BASH IS THE PATH THAT PROVES IT. +// +// execExecutionOutcome is shared between exec_command and bash. It used to set +// Launched unconditionally, which is true for exec_command because a start +// failure returns an errorResult before an execution outcome is ever built. +// bash is different: it hands EVERY Run error to the same conversion, including +// a missing executable and a context cancelled before os.StartProcess. Those +// have a prepared plan, and therefore planned notices, but no child, so the +// hard-coded launch state turned a plan into a claim that reduced enforcement +// had actually been applied. +// +// These drive the real tool rather than constructing an outcome, because the +// bug was precisely that the constructed shape and the real one disagreed. +func TestBashOutcomeCarriesTheRealLaunchState(t *testing.T) { + root := t.TempDir() + tool := NewScopedBashTool(root, nil) + + t.Run("a command whose executable does not exist never launched", func(t *testing.T) { + res := tool.Run(context.Background(), map[string]any{ + "command": "zero-nonexistent-binary-for-launch-state-test --please-fail", + }) + if res.ExecutionOutcome == nil { + t.Fatal("no execution outcome recorded") + } + // The shell itself starts and reports "command not found", so this asserts + // the contract rather than a specific errno: whatever the platform did, + // the notice must agree with whether a process was created. + if got := len(res.ExecutionOutcome.AppliedEnforcementNotices()); got > 0 && !res.ExecutionOutcome.Launched { + t.Errorf("a command that never launched disclosed %d enforcement notices", got) + } + }) + + t.Run("a context cancelled before start never launched", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + res := tool.Run(ctx, map[string]any{"command": "echo hi"}) + if res.ExecutionOutcome == nil { + t.Skip("this platform produced no execution outcome for a pre-cancelled run") + } + if res.ExecutionOutcome.Launched { + t.Error("a run cancelled before start reported a launched child") + } + if got := res.ExecutionOutcome.AppliedEnforcementNotices(); len(got) != 0 { + t.Errorf("a run cancelled before start claimed an enforcement trade: %v", got) + } + }) + + t.Run("an ordinary command that runs does launch", func(t *testing.T) { + res := tool.Run(context.Background(), map[string]any{"command": "echo hello"}) + if res.ExecutionOutcome == nil { + t.Fatal("no execution outcome recorded") + } + if !res.ExecutionOutcome.Launched { + t.Error("a command that ran was not recorded as launched") + } + if !strings.Contains(res.Output, "hello") { + t.Errorf("unexpected output: %q", res.Output) + } + }) +} diff --git a/internal/tools/enforcement_notice_measurement_test.go b/internal/tools/enforcement_notice_measurement_test.go new file mode 100644 index 000000000..c6ba69762 --- /dev/null +++ b/internal/tools/enforcement_notice_measurement_test.go @@ -0,0 +1,43 @@ +package tools + +import "testing" + +// THE BUDGET HAS TO COUNT WHAT THE MODEL ACTUALLY RECEIVES. +// +// The enforcement notices are prepended to the model view on the way out, so a +// disclosed call costs more context than result.Output alone. Measuring the bare +// output undercounts every one of them, and the undercount grows with the notice +// rather than being a fixed slack. +func TestOutcomeMeasuresTheNoticesTheModelReceives(t *testing.T) { + const notice = "denyRead is configured, so the Windows sandbox uses the token shape without WRITE_RESTRICTED (#869)" + const output = "exit status 0" + + bare := finalizeToolOutcome(Result{Status: StatusOK, Output: output}, output) + disclosed := finalizeToolOutcome(Result{Status: StatusOK, Output: output, EnforcementNotices: []string{notice}}, output) + + if disclosed.Outcome.Diagnostics.ModelBytes <= bare.Outcome.Diagnostics.ModelBytes { + t.Errorf("a disclosed result measured %d model bytes, no more than the undisclosed %d, so the notice is uncounted", + disclosed.Outcome.Diagnostics.ModelBytes, bare.Outcome.Diagnostics.ModelBytes) + } + if want := len(WithEnforcementNotices(output, []string{notice})); disclosed.Outcome.Diagnostics.ModelBytes != want { + t.Errorf("model bytes = %d, want %d (the canonical output the model is handed)", + disclosed.Outcome.Diagnostics.ModelBytes, want) + } + if disclosed.Outcome.Diagnostics.EstimatedModelTokens <= bare.Outcome.Diagnostics.EstimatedModelTokens { + t.Errorf("estimated model tokens did not grow with the notice: %d vs %d", + disclosed.Outcome.Diagnostics.EstimatedModelTokens, bare.Outcome.Diagnostics.EstimatedModelTokens) + } +} + +// And the stored view stays bare, or the notices ship twice: ModelOutput +// prepends them to whatever ModelView holds. +func TestOutcomeModelViewDoesNotCarryTheNoticesItself(t *testing.T) { + const notice = "sandbox notice" + const output = "exit status 0" + + result := finalizeToolOutcome(Result{Status: StatusOK, Output: output, EnforcementNotices: []string{notice}}, output) + if result.Outcome.ModelView != output { + t.Errorf("ModelView = %q, want the bare output %q; ModelOutput prepends the notice, so storing it here sends it twice", + result.Outcome.ModelView, output) + } +} diff --git a/internal/tools/exec_command.go b/internal/tools/exec_command.go index c3f643367..9c048d402 100644 --- a/internal/tools/exec_command.go +++ b/internal/tools/exec_command.go @@ -204,6 +204,9 @@ func (tool execCommandTool) run(ctx context.Context, args map[string]any, engine Command: command, Enforcement: executionEnforcement(plan), Report: plan.ExecutionReport, + // A wrapped plan starts a helper; the requested child is created inside + // it and only the adapter sees that transition. + ChildLaunchOwnedByAdapter: plan.ChildLaunchOwnedByAdapter(), Cleanup: func() { plan.Cleanup() cancel() @@ -229,18 +232,14 @@ func (tool execCommandTool) run(ctx context.Context, args map[string]any, engine exitCode: processResult.ExitCode, exited: processResult.Exited, relativeCwd: processResult.RelativeCwd, tty: processResult.TTY, request: processResult.Request, enforcement: processResult.Enforcement, report: processResult.Report, reportErr: processResult.ReportErr, changes: processResult.Changes, - sandboxMeta: processResult.Metadata, - maxOutputTokens: maxOutputTokens, + childLaunchOwnedByAdapter: processResult.ChildLaunchOwnedByAdapter, + sandboxMeta: processResult.Metadata, + maxOutputTokens: maxOutputTokens, }, directBudget) } func executionEnforcement(plan zeroSandbox.CommandPlan) execution.Enforcement { - return execution.Enforcement{ - Backend: string(plan.TargetBackend), - Level: string(plan.EnforcementLevel), - Degraded: plan.EnforcementLevel == zeroSandbox.EnforcementDegraded, - DowngradeReason: plan.DowngradeReason, - } + return zeroSandbox.EnforcementFor(plan) } type writeStdinTool struct { @@ -371,7 +370,8 @@ func (tool writeStdinTool) RunWithOptions(ctx context.Context, args map[string]a exitCode: processResult.ExitCode, exited: processResult.Exited, relativeCwd: processResult.RelativeCwd, tty: processResult.TTY, interrupted: processResult.Interrupted, request: processResult.Request, enforcement: processResult.Enforcement, report: processResult.Report, reportErr: processResult.ReportErr, - changes: processResult.Changes, sandboxMeta: processResult.Metadata, + childLaunchOwnedByAdapter: processResult.ChildLaunchOwnedByAdapter, + changes: processResult.Changes, sandboxMeta: processResult.Metadata, maxOutputTokens: maxOutputTokens, }) } @@ -408,16 +408,25 @@ type execToolResultInput struct { sessionID int exitCode int exited bool - relativeCwd string - tty bool - interrupted bool - request execution.Request - enforcement execution.Enforcement - sandboxMeta map[string]string - report execution.AdapterReport - reportErr error - changes []execution.Change - maxOutputTokens int + // launched records whether an OS process was actually created, observed at + // the boundary that ran it rather than assumed from the outcome shape. The + // exec_command paths set it true because a start failure returns an + // errorResult before reaching here; bash cannot, because it routes a + // pre-start Run error through the same conversion. + launched bool + // childLaunchOwnedByAdapter marks a wrapped plan, where launched above + // describes the helper rather than the requested process. + childLaunchOwnedByAdapter bool + relativeCwd string + tty bool + interrupted bool + request execution.Request + enforcement execution.Enforcement + sandboxMeta map[string]string + report execution.AdapterReport + reportErr error + changes []execution.Change + maxOutputTokens int } func execToolResult(input execToolResultInput) Result { @@ -437,6 +446,15 @@ func execToolResultWithBudget(input execToolResultInput, directBudget bool) Resu for key, value := range input.sandboxMeta { meta[key] = value } + // A process started here by construction, because a start failure returns an + // errorResult above without building an execution outcome. But for a wrapped + // plan that process is the SANDBOX HELPER, which creates the requested child + // only after marker, ACL, network, SID and token setup. So hand the observation + // to the same resolution every other launcher uses instead of asserting it. + // The retained path matters most here: the helper can be returned before it + // has attempted the inner launch, and an absent report then means not yet + // launched rather than launched. + input.launched = execution.ResolveChildLaunched(true, input.childLaunchOwnedByAdapter, input.report) outcome := execExecutionOutcome(input) if input.exited { meta["exit_code"] = strconv.Itoa(input.exitCode) @@ -531,29 +549,40 @@ func execExecutionRequest(command *exec.Cmd, plan zeroSandbox.CommandPlan, cwd s func execExecutionOutcome(input execToolResultInput) execution.Outcome { enforcement := input.enforcement + // EVERY OUTCOME BUILT HERE DESCRIBES A PROCESS THAT STARTED. A command that + // could not be started returns an error result before this point, so there is + // no path in without a process behind it. Stated rather than inferred, so the + // disclosure derived from it does not rest on the terminal outcome kind. + // READ, not assumed. This used to be a const true, documented as safe + // because exec_command returns early on a start failure. That holds for + // exec_command and not for bash, which hands every Run error to this same + // conversion, so a command whose executable did not exist claimed the + // DenyRead token trade had been applied. + launched := input.launched if !input.exited { return execution.Outcome{ State: execution.StateRetained, Kind: execution.OutcomeRunning, + Launched: launched, ProcessID: strconv.Itoa(input.sessionID), Enforcement: enforcement, } } exit := &execution.Exit{Code: input.exitCode} if input.reportErr != nil { - return execution.Outcome{State: execution.StateFailed, Kind: execution.OutcomeSandboxSetupFailure, Exit: exit, Enforcement: enforcement, Changes: input.changes} + return execution.Outcome{State: execution.StateFailed, Kind: execution.OutcomeSandboxSetupFailure, Launched: launched, Exit: exit, Enforcement: enforcement, Changes: input.changes} } if input.report.Denial != nil { denial := *input.report.Denial - return execution.Outcome{State: execution.StateDenied, Kind: execution.OutcomeEnforcementDenied, Exit: exit, Denial: &denial, Enforcement: enforcement, Changes: input.changes} + return execution.Outcome{State: execution.StateDenied, Kind: execution.OutcomeEnforcementDenied, Launched: launched, Exit: exit, Denial: &denial, Enforcement: enforcement, Changes: input.changes} } if input.interrupted { - return execution.Outcome{State: execution.StateCancelled, Kind: execution.OutcomeCancelled, Exit: exit, Enforcement: enforcement, Changes: input.changes} + return execution.Outcome{State: execution.StateCancelled, Kind: execution.OutcomeCancelled, Launched: launched, Exit: exit, Enforcement: enforcement, Changes: input.changes} } if input.exitCode == 0 { - return execution.Outcome{State: execution.StateCompleted, Kind: execution.OutcomeSuccess, Exit: exit, Enforcement: enforcement, Changes: input.changes} + return execution.Outcome{State: execution.StateCompleted, Kind: execution.OutcomeSuccess, Launched: launched, Exit: exit, Enforcement: enforcement, Changes: input.changes} } - return execution.Outcome{State: execution.StateFailed, Kind: execution.OutcomeApplicationFailure, Exit: exit, Enforcement: enforcement, Changes: input.changes} + return execution.Outcome{State: execution.StateFailed, Kind: execution.OutcomeApplicationFailure, Launched: launched, Exit: exit, Enforcement: enforcement, Changes: input.changes} } func executionChangedFiles(changes []execution.Change) []string { diff --git a/internal/tools/exec_launch_contract_test.go b/internal/tools/exec_launch_contract_test.go new file mode 100644 index 000000000..0a8d0625d --- /dev/null +++ b/internal/tools/exec_launch_contract_test.go @@ -0,0 +1,119 @@ +package tools + +import ( + "testing" + + "github.com/Gitlawb/zero/internal/execution" +) + +// THE LAUNCH FACT HAS TO SURVIVE EVERY RESULT SHAPE, NOT JUST THE CAPTURED ONE. +// +// exec_command builds its outcome through its own conversion rather than through +// Runner.ExecuteCaptured, and that conversion used to assert `launched = true` on +// the grounds that a start failure returns earlier. That reasoning holds for the +// process the tool starts, and on Windows a wrapped plan starts the sandbox +// helper: the requested child is created inside it, after marker, ACL, network, +// SID and token setup, any of which can fail with the helper already running. +// Asserting the launch there tells the operator that reads were denied in +// exchange for the write jail when nothing ran under that enforcement. +// +// The retained shape is the one worth pinning hardest: exec_command can return a +// running session before the helper has even attempted the inner launch, so an +// absent report there means "not yet", not "yes". +func TestExecOutcomeTakesTheLaunchFactFromTheAdapter(t *testing.T) { + yes, no := true, false + + cases := []struct { + name string + owned bool + report execution.AdapterReport + exited bool + want bool + because string + }{ + { + name: "wrapped helper failed before creating the child", + owned: true, report: execution.AdapterReport{ChildLaunched: &no}, exited: true, + want: false, because: "only the unsandboxed helper ran", + }, + { + name: "wrapped plan, adapter said nothing", + owned: true, report: execution.AdapterReport{}, exited: true, + want: false, because: "an absent report is not proof that enforcement applied", + }, + { + name: "wrapped plan still running, nothing reported yet", + owned: true, report: execution.AdapterReport{}, exited: false, + want: false, because: "the helper can be returned before it attempts the inner launch", + }, + { + name: "wrapped plan, restricted child confirmed", + owned: true, report: execution.AdapterReport{ChildLaunched: &yes}, exited: true, + want: true, because: "the adapter saw the transition", + }, + { + name: "direct command keeps its own observation", + owned: false, report: execution.AdapterReport{}, exited: true, + want: true, because: "the process the tool started is the requested one", + }, + } + + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + input := execToolResultInput{ + commandText: "echo hi", + sessionID: 7, + exited: testCase.exited, + report: testCase.report, + childLaunchOwnedByAdapter: testCase.owned, + enforcement: execution.Enforcement{Notices: []string{"denyRead is configured, so the write jail is not confining writes"}}, + } + // Through the PRODUCTION conversion, which is what decides the launch + // state. Computing it here instead would pin the shared helper and prove + // nothing about whether exec_command consults it. + result := execToolResult(input) + outcome := result.ExecutionOutcome + if outcome == nil { + t.Fatalf("SETUP INVALID: the conversion produced no execution outcome") + } + if outcome.Launched != testCase.want { + t.Fatalf("Launched = %v, want %v: %s", outcome.Launched, testCase.want, testCase.because) + } + notices := outcome.AppliedEnforcementNotices() + if testCase.want && len(notices) != 1 { + t.Fatalf("a confirmed launch disclosed %q, want the planned notice exactly once", notices) + } + if !testCase.want && len(notices) != 0 { + t.Fatalf("no requested child ran, but the outcome disclosed %q", notices) + } + }) + } +} + +// And the shared resolution itself, since three launchers now depend on it +// answering the same way. +func TestResolveChildLaunchedIsOneAnswerForEveryLauncher(t *testing.T) { + yes, no := true, false + cases := []struct { + name string + observed bool + owned bool + report execution.AdapterReport + want bool + }{ + {"adapter confirms over a false observation", false, true, execution.AdapterReport{ChildLaunched: &yes}, true}, + {"adapter denies over a true observation", true, true, execution.AdapterReport{ChildLaunched: &no}, false}, + {"owned and silent fails closed", true, true, execution.AdapterReport{}, false}, + {"unowned keeps the observation, true", true, false, execution.AdapterReport{}, true}, + {"unowned keeps the observation, false", false, false, execution.AdapterReport{}, false}, + {"an adapter may speak even when unowned", false, false, execution.AdapterReport{ChildLaunched: &yes}, true}, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + if got := execution.ResolveChildLaunched(testCase.observed, testCase.owned, testCase.report); got != testCase.want { + t.Fatalf("ResolveChildLaunched(%v, %v, %+v) = %v, want %v", + testCase.observed, testCase.owned, testCase.report, got, testCase.want) + } + }) + } +} diff --git a/internal/tools/sandbox_notice_meta_test.go b/internal/tools/sandbox_notice_meta_test.go new file mode 100644 index 000000000..7215ef8af --- /dev/null +++ b/internal/tools/sandbox_notice_meta_test.go @@ -0,0 +1,48 @@ +package tools + +import ( + "strings" + "testing" + + zeroSandbox "github.com/Gitlawb/zero/internal/sandbox" +) + +// THE DISCLOSURE HAS TO REACH THE OPERATOR, not just exist on the plan. +// +// The DenyRead write-jail trade was reachable only from BackendPlan, which +// `zero sandbox policy` and `zero sandbox check` render. Someone approving +// file_system.deny_read for a single command never runs those, so they lost the +// write jail silently. addSandboxMeta is the boundary where a tool call's +// sandbox facts become visible, alongside the downgrade reason that already +// travels this way. +func TestSandboxMetaCarriesLeastPrivilegeNotices(t *testing.T) { + meta := map[string]string{} + addSandboxMeta(meta, zeroSandbox.CommandPlan{ + Backend: zeroSandbox.Backend{Name: zeroSandbox.BackendWindowsRestrictedToken}, + Notes: []string{ + "denyRead is set, so the restricted token drops WRITE_RESTRICTED and the workspace write jail no longer confines writes outside it (#869).", + }, + }) + + notices, ok := meta["sandbox_notices"] + if !ok { + t.Fatalf("no sandbox_notices in the tool result metadata, so the trade stays invisible to whoever approved it: %#v", meta) + } + for _, want := range []string{"denyRead", "#869"} { + if !strings.Contains(notices, want) { + t.Errorf("notice does not mention %q: %q", want, notices) + } + } +} + +// A plan with nothing to disclose must not add the key, or every command grows +// an empty field and the presence of one stops meaning anything. +func TestSandboxMetaOmitsNoticesWhenThereAreNone(t *testing.T) { + meta := map[string]string{} + addSandboxMeta(meta, zeroSandbox.CommandPlan{ + Backend: zeroSandbox.Backend{Name: zeroSandbox.BackendWindowsRestrictedToken}, + }) + if value, ok := meta["sandbox_notices"]; ok { + t.Errorf("sandbox_notices present with nothing to say: %q", value) + } +} diff --git a/internal/tools/sandbox_notice_visibility_test.go b/internal/tools/sandbox_notice_visibility_test.go new file mode 100644 index 000000000..7f5a4ae78 --- /dev/null +++ b/internal/tools/sandbox_notice_visibility_test.go @@ -0,0 +1,113 @@ +package tools + +import ( + "context" + "strings" + "testing" + + zeroSandbox "github.com/Gitlawb/zero/internal/sandbox" +) + +const testDenyReadNotice = "denyRead is set, so the restricted token drops WRITE_RESTRICTED and the workspace write jail no longer confines writes outside it (#869)." + +// noticeCarryingTool stands in for a command tool whose plan carried an +// enforcement notice. It writes the notice the way addSandboxMeta does, which is +// the only thing the production paths do with it. +type noticeCarryingTool struct{} + +func (noticeCarryingTool) Name() string { return "bash" } +func (noticeCarryingTool) Description() string { return "test shell tool" } +func (noticeCarryingTool) Parameters() Schema { + return Schema{ + Type: "object", + Properties: map[string]PropertySchema{"command": {Type: "string"}}, + Required: []string{"command"}, + AdditionalProperties: false, + } +} +func (noticeCarryingTool) Safety() Safety { + return Safety{SideEffect: SideEffectRead, Permission: PermissionAllow, Reason: "reads files"} +} +func (noticeCarryingTool) Run(context.Context, map[string]any) Result { + meta := map[string]string{} + addSandboxMeta(meta, zeroSandbox.CommandPlan{ + Backend: zeroSandbox.Backend{Name: zeroSandbox.BackendWindowsRestrictedToken}, + Notes: []string{testDenyReadNotice}, + }) + return Result{ + Status: StatusOK, + Output: "hello from the command", + Meta: meta, + Display: Display{Summary: "ran the command", Kind: "shell"}, + } +} + +// THE DISCLOSURE HAS TO REACH A HUMAN AND A MODEL, NOT A METADATA MAP. +// +// The first version of this wrote sandbox_notices into Result.Meta and stopped +// there. Nothing in production reads those keys, ModelOutput and HumanDisplay +// never consult Meta, and the durable history drops it, so a Windows user who +// configured deny_read could take the non-WRITE_RESTRICTED token, lose write +// confinement, and see nothing but ordinary command output. Metadata is +// side-band data, not a disclosure channel. +func TestEnforcementNoticeReachesTheModelAndTheDisplay(t *testing.T) { + registry := NewRegistry() + registry.Register(noticeCarryingTool{}) + + result := registry.RunWithOptions(context.Background(), "bash", map[string]any{ + "command": "echo hello", + }, RunOptions{PermissionGranted: true}) + + if result.Status != StatusOK { + t.Fatalf("tool failed: %s", result.Output) + } + + model := result.ModelOutput() + if !strings.Contains(model, "#869") { + t.Errorf("the model-facing result does not carry the disclosure, so the agent proceeds unaware:\n%s", model) + } + if !strings.Contains(model, "hello from the command") { + t.Errorf("the notice displaced the actual output:\n%s", model) + } + // PREPENDED, because the output budget trims from the end and a disclosure + // that survives only on short results is not a disclosure. + if !strings.HasPrefix(strings.TrimSpace(model), testDenyReadNotice) { + t.Errorf("the notice is not in front of the output, so a trimmed result can lose it:\n%s", model) + } + + display := result.HumanDisplay() + if !strings.Contains(display.Summary, "#869") { + t.Errorf("the interactive display does not carry the disclosure, so the operator sees nothing: %q", display.Summary) + } + + // Kept in metadata too, for integrations reading the result JSON. + if result.Meta[sandboxNoticesMeta] == "" { + t.Errorf("the metadata copy was dropped: %#v", result.Meta) + } +} + +// A result with nothing to disclose must be untouched, or every command grows a +// blank line and the presence of a notice stops meaning anything. +func TestResultsWithoutNoticesAreUnchanged(t *testing.T) { + result := Result{Status: StatusOK, Output: "plain output", Display: Display{Summary: "did a thing"}} + + if got := result.ModelOutput(); got != "plain output" { + t.Errorf("model output = %q, want it untouched", got) + } + if got := result.HumanDisplay().Summary; got != "did a thing" { + t.Errorf("display summary = %q, want it untouched", got) + } +} + +// Whitespace-only notices are not notices. Guards against a plan that carries an +// empty entry putting a blank line in front of every result. +func TestBlankNoticesDoNotAlterTheResult(t *testing.T) { + result := Result{ + Status: StatusOK, + Output: "plain output", + EnforcementNotices: []string{"", " "}, + } + if got := result.ModelOutput(); got != "plain output" { + t.Errorf("model output = %q, want it untouched", got) + } +} diff --git a/internal/tools/tool_outcome.go b/internal/tools/tool_outcome.go index 439aab58b..13a4fbfcc 100644 --- a/internal/tools/tool_outcome.go +++ b/internal/tools/tool_outcome.go @@ -46,6 +46,28 @@ func (outcome *ToolOutcome) UnmarshalJSON(data []byte) error { // boundaryOutput must already be redacted. It is the text seen immediately // before command reduction and semantic budgeting. func finalizeToolOutcome(result Result, boundaryOutput string) Result { + // PROMOTED HERE, at the one seam every tool result crosses, rather than at + // each construction site. addSandboxMeta already carries the plan's notices + // into metadata for both the bash and the exec_command paths, and any future + // command tool that calls it gets the same treatment for free. Setting the + // field at the call sites instead would be a third hand-maintained projection + // of the same fact, which is exactly how the disclosure went missing from the + // generic execution adapter in the first place. + if len(result.EnforcementNotices) == 0 { + // DERIVED FROM APPLIED STATE, NOT FROM THE PLAN. addSandboxMeta writes the + // plan's notices at plan time, before anything runs, so promoting them + // unconditionally claims a token trade for a command that may never have + // started. The execution outcome is the thing that knows, and it applies + // the same launched-and-planned rule hooks and plugins use. + // + // The metadata stays as diagnostics either way: it records what was + // planned, which is still worth having. + if result.ExecutionOutcome != nil { + result.EnforcementNotices = result.ExecutionOutcome.AppliedEnforcementNotices() + } else if notices := strings.TrimSpace(result.Meta[sandboxNoticesMeta]); notices != "" { + result.EnforcementNotices = strings.Split(notices, "\n") + } + } previous := result.Outcome human := result.Display if human.Preview == "" && result.Meta["command_output_reduced"] == "true" { @@ -65,8 +87,18 @@ func finalizeToolOutcome(result Result, boundaryOutput string) Result { originalBytes = previous.Diagnostics.OriginalBytes originalTokens = previous.Diagnostics.EstimatedOriginalTokens } - modelBytes := len(result.Output) - modelTokens := estimateOutputTokens(result.Output) + // MEASURE WHAT THE MODEL ACTUALLY RECEIVES. + // + // The enforcement notices are prepended to the model view on the way out + // (agent.ToolResult.ModelOutput), so a result carrying them costs more context + // than result.Output alone. Measuring the bare output undercounts every + // disclosed call and hands the budget a figure the model never saw. + // + // ModelView below stays the bare output on purpose: ModelOutput prepends the + // notices itself, so storing them here would send them twice. + canonicalModelOutput := WithEnforcementNotices(result.Output, result.EnforcementNotices) + modelBytes := len(canonicalModelOutput) + modelTokens := estimateOutputTokens(canonicalModelOutput) var artifact *ToolArtifact if previous.Finalized() { diff --git a/internal/tools/types.go b/internal/tools/types.go index 27755d8d4..4a9168581 100644 --- a/internal/tools/types.go +++ b/internal/tools/types.go @@ -2,6 +2,7 @@ package tools import ( "context" + "strings" "github.com/Gitlawb/zero/internal/execution" "github.com/Gitlawb/zero/internal/sandbox" @@ -52,6 +53,11 @@ const ( SandboxDenialKindMeta = "sandbox_denial_kind" SandboxDenialReasonMeta = "sandbox_denial_reason" SandboxDenialKeywordMeta = "sandbox_denial_keyword" + // sandboxNoticesMeta transports the plan notices from addSandboxMeta to + // finalizeToolOutcome, which promotes them onto Result.EnforcementNotices. + // Kept as metadata as well, because integrations reading the result JSON have + // no other way to see them. + sandboxNoticesMeta = "sandbox_notices" ) const ( @@ -113,6 +119,16 @@ type Result struct { // emits them as a following user message, which is also the only shape that // keeps one tool result per tool call. Images []zeroruntime.ImageBlock `json:"-"` + // EnforcementNotices are least-privilege disclosures about the enforcement + // actually applied to this command, and they are USER AND MODEL VISIBLE. + // + // A separate field rather than text baked into Output, so one canonical + // result carries it and every surface reads it through the accessors below. + // The first attempt put this in Meta alongside the sandbox metadata, which + // looked like the established channel and is not one: nothing in production + // reads those keys, ModelOutput and HumanDisplay never consult Meta, and the + // durable history drops it. The disclosure reached nobody. + EnforcementNotices []string `json:"enforcementNotices,omitempty"` // Redacted is set when secret scrubbing altered Output before it left the // tool-execution boundary. Redacted bool @@ -175,24 +191,69 @@ type OutcomeDiagnostics struct { Reason string } -// ModelOutput returns the finalized provider-facing text, falling back to the -// legacy field for direct Tool.Run callers that have not crossed the registry. -func (result Result) ModelOutput() string { +// BaseModelOutput returns the UNDECORATED provider-facing text: the finalized +// model view, falling back to the legacy field for direct Tool.Run callers that +// have not crossed the registry. It carries no enforcement notices. +// +// Callers that PROJECT a result into another carrier (the agent loop building an +// agent.ToolResult) must copy this, not ModelOutput, and copy the typed notice +// slice alongside it. Storing already-rendered text next to the same notices is +// two representations of one fact with no contract between them, and whichever +// side of the projection loses its finalized outcome renders the disclosure +// twice. +func (result Result) BaseModelOutput() string { if result.Outcome.finalized { return result.Outcome.ModelView } return result.Output } -// HumanDisplay returns the finalized presentation, falling back to the legacy -// display for direct Tool.Run callers. -func (result Result) HumanDisplay() Display { +// BaseDisplay is BaseModelOutput for the presentation half, and carries no +// enforcement notices for the same reason. +func (result Result) BaseDisplay() Display { if result.Outcome.finalized { return result.Outcome.HumanView } return result.Display } +// ModelOutput returns the finalized provider-facing text with the enforcement +// disclosure rendered in front of it. This is the only place the model view is +// decorated. +func (result Result) ModelOutput() string { + return WithEnforcementNotices(result.BaseModelOutput(), result.EnforcementNotices) +} + +// HumanDisplay returns the finalized presentation with the enforcement +// disclosure rendered in front of the summary. +func (result Result) HumanDisplay() Display { + display := result.BaseDisplay() + display.Summary = WithEnforcementNotices(display.Summary, result.EnforcementNotices) + return display +} + +// WithEnforcementNotices puts the enforcement disclosure IN FRONT of the text. +// +// PREPENDED, not appended, because the output budget trims from the end: a +// notice at the tail is the first thing a long result loses, and a disclosure +// that survives only on short outputs is not a disclosure. It is also why this +// lives on the accessors rather than at the call sites that build results. +// The previous version wrote it into Result.Meta, and neither ModelOutput nor +// HumanDisplay nor the durable history reads Meta, so it reached nobody at all. +func WithEnforcementNotices(text string, notices []string) string { + if len(notices) == 0 { + return text + } + joined := strings.TrimSpace(strings.Join(notices, "\n")) + if joined == "" { + return text + } + if strings.TrimSpace(text) == "" { + return joined + } + return joined + "\n\n" + text +} + // Display carries a short, structured summary of a tool result for the TUI/stream. type Display struct { Summary string diff --git a/internal/tui/enforcement_notice_card_test.go b/internal/tui/enforcement_notice_card_test.go new file mode 100644 index 000000000..540e4cbe2 --- /dev/null +++ b/internal/tui/enforcement_notice_card_test.go @@ -0,0 +1,260 @@ +package tui + +import ( + "encoding/json" + "fmt" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/sessions" + "github.com/Gitlawb/zero/internal/tools" +) + +const cardNotice = "denyRead is configured, so the Windows sandbox uses the token shape without WRITE_RESTRICTED (#869)" + +func resultWithPreviewAndNotice() agent.ToolResult { + return agent.ToolResult{ + ToolCallID: "call-1", + Name: "edit_file", + Status: tools.StatusOK, + Output: "Successfully edited x.go (replaced 1 occurrence).", + Display: tools.Display{Summary: "Successfully edited x.go.", Kind: "file", Preview: "--- a/x.go\n+++ b/x.go\n@@ -1 +1 @@\n-old\n+new"}, + EnforcementNotices: []string{cardNotice}, + } +} + +func renderedCard(row transcriptRow, expanded bool) string { + row.expanded = expanded + return renderToolResultCard(row, 100, rowContext{}, cardRenderOptions{bodyCap: 20}) +} + +func rowForResult(result agent.ToolResult) transcriptRow { + return transcriptRow{ + kind: rowToolResult, id: "r1", tool: result.Name, status: result.Status, + text: toolResultRowText(result), detail: toolResultDetail(result), + enforcementNotices: result.EnforcementNotices, + } +} + +// THE DISCLOSURE HAS TO REACH THE CARD, NOT JUST THE ROW TEXT. +// +// The notice is prepended to ModelOutput, which toolResultRowText carries into +// row.text, and toolCardHead is handed row.text. But the head renders the action +// and target, so the notice went nowhere. Every result with a rich preview (each +// edit and write card) rendered with no disclosure at all. +// +// Collapsed as well as expanded: a trade the operator has to expand a card to +// discover has not been disclosed. +func TestToolCardShowsTheEnforcementDisclosure(t *testing.T) { + row := rowForResult(resultWithPreviewAndNotice()) + + for _, expanded := range []bool{false, true} { + card := renderedCard(row, expanded) + if !strings.Contains(card, "WRITE_RESTRICTED") { + t.Errorf("expanded=%v: the card rendered no enforcement disclosure:\n%s", expanded, card) + } + } +} + +// THE DISCLOSURE IS DATA, NOT PROSE GLUED ONTO THE DIFF. +// +// row.detail is parsed as a diff by the files panel (planDiffStat, +// perFileDiffStats) and rendered line by line by the file view. Prefixing it +// with the notice would have been the shorter fix and would have corrupted both, +// so the notice travels in its own field and the diff stays a diff. +func TestTheDisclosureDoesNotContaminateTheDiffDetail(t *testing.T) { + result := resultWithPreviewAndNotice() + row := rowForResult(result) + + if strings.Contains(row.detail, "WRITE_RESTRICTED") { + t.Fatalf("the notice leaked into the diff detail, which is parsed as a diff: %q", row.detail) + } + adds, dels := planDiffStat(row.detail) + if adds != 1 || dels != 1 { + t.Errorf("diff stats changed with the disclosure attached: +%d -%d, want +1 -1", adds, dels) + } +} + +// AND IT HAS TO SURVIVE A RESUME. +// +// The session payload carried the notice only inside the "output" string. The +// rich card is rebuilt from displayPreview, which never had it, so a restored +// transcript lost the disclosure even though the row it replaced had shown it. +func TestRestoredSessionKeepsTheEnforcementDisclosure(t *testing.T) { + encoded, err := json.Marshal(toolResultSessionPayload(resultWithPreviewAndNotice())) + if err != nil { + t.Fatalf("marshal session payload: %v", err) + } + + rows := transcriptRowsFromSessionEvents([]sessions.Event{{Type: sessions.EventToolResult, Payload: json.RawMessage(encoded)}}) + if len(rows) != 1 { + t.Fatalf("expected one restored row, got %d", len(rows)) + } + if len(rows[0].enforcementNotices) == 0 { + t.Fatal("the restored row carries no enforcement notices, so the resumed transcript lost the disclosure") + } + for _, expanded := range []bool{false, true} { + if card := renderedCard(rows[0], expanded); !strings.Contains(card, "WRITE_RESTRICTED") { + t.Errorf("expanded=%v: the restored card rendered no disclosure:\n%s", expanded, card) + } + } +} + +// A result with no notice must not grow card furniture, or every card gains a +// blank line and the disclosure stops standing out. +func TestOrdinaryResultsGainNoNoticeLines(t *testing.T) { + result := resultWithPreviewAndNotice() + result.EnforcementNotices = nil + plain := renderedCard(rowForResult(result), true) + + result.EnforcementNotices = []string{"", " "} + blank := renderedCard(rowForResult(result), true) + + if plain != blank { + t.Errorf("blank notices changed the card:\n--- none ---\n%s\n--- blank ---\n%s", plain, blank) + } +} + +// THE NO-PREVIEW CARD IS THE ONE THE PREVIEW TEST CANNOT SEE. +// +// The disclosure travels in two forms: typed EnforcementNotices, which the card +// renders as its own furniture, and ModelOutput, which has the notice composed +// in. A rich preview is undecorated, so an edit card was right. Every bash and +// exec result, and every error, has no preview and fell back to ModelOutput, so +// row.detail already began with the notice and the card drew it twice: once in +// the notice lines and once at the top of the body. +// +// Both halves are asserted, because a body that lost the notice by losing the +// output would also count once. +func resultWithoutPreviewAndNotice(status tools.Status, output string) agent.ToolResult { + return agent.ToolResult{ + ToolCallID: "call-2", + Name: "bash", + Status: status, + Output: output, + EnforcementNotices: []string{cardNotice}, + } +} + +func countNoticeAndBody(t *testing.T, card string, body string) (int, int) { + t.Helper() + return strings.Count(card, "WRITE_RESTRICTED"), strings.Count(card, body) +} + +func TestNoPreviewCardShowsTheDisclosureExactlyOnce(t *testing.T) { + cases := []struct { + name string + status tools.Status + output string + }{ + {"success", tools.StatusOK, "PROBE-BODY-OK"}, + {"error", tools.StatusError, "PROBE-BODY-ERR"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + row := rowForResult(resultWithoutPreviewAndNotice(tc.status, tc.output)) + for _, expanded := range []bool{false, true} { + notices, bodies := countNoticeAndBody(t, renderedCard(row, expanded), tc.output) + if notices != 1 { + t.Errorf("expanded=%v: disclosure rendered %d times, want exactly 1", expanded, notices) + } + if bodies != 1 { + t.Errorf("expanded=%v: command output rendered %d times, want exactly 1", expanded, bodies) + } + } + }) + } +} + +// The durable path had the same mismatch: the payload stores the decorated +// output beside the typed notices, and restoration used that output as the card +// body whenever no distinct preview was stored. +func TestRestoredNoPreviewCardShowsTheDisclosureExactlyOnce(t *testing.T) { + cases := []struct { + name string + status tools.Status + output string + }{ + {"success", tools.StatusOK, "PROBE-BODY-OK"}, + {"error", tools.StatusError, "PROBE-BODY-ERR"}, + // A command that printed nothing under an enforced profile still has a + // real notice. The stored body is empty, which restoration must treat as + // present-and-empty rather than absent. + {"empty output", tools.StatusOK, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + encoded, err := json.Marshal(toolResultSessionPayload(resultWithoutPreviewAndNotice(tc.status, tc.output))) + if err != nil { + t.Fatal(err) + } + rows := transcriptRowsFromSessionEvents([]sessions.Event{{Type: sessions.EventToolResult, Payload: json.RawMessage(encoded)}}) + if len(rows) != 1 { + t.Fatalf("expected one restored row, got %d", len(rows)) + } + for _, expanded := range []bool{false, true} { + card := renderedCard(rows[0], expanded) + if notices := strings.Count(card, "WRITE_RESTRICTED"); notices != 1 { + t.Errorf("expanded=%v: restored card rendered the disclosure %d times, want exactly 1:\n%s", expanded, notices, card) + } + if tc.output != "" && strings.Count(card, tc.output) != 1 { + t.Errorf("expanded=%v: restored card rendered the output %d times, want exactly 1:\n%s", expanded, strings.Count(card, tc.output), card) + } + } + }) + } +} + +// A CLI-WRITTEN RESULT RESUMED IN THE TUI MUST STILL DISCLOSE, ONCE. +// +// The headless writers and the interactive writer append to the same default +// session store, and the TUI resumes from it. The headless payload used to +// carry only the decorated ModelOutput: no typed notices, no undecorated body. +// On restore the transcript found neither, and for a long result the card is +// collapsed by default, so there was no body to carry the decorated text and no +// notice furniture to draw it. The disclosure the run had shown was simply gone +// from the resumed transcript. +// +// Both writers now go through ToolResultSessionPayload, so this exercises the +// exact bytes the CLI persists, restores them the way the TUI does, and renders +// the collapsed card, which is the shape the old CLI test could not reach. +func TestHeadlessWrittenCollapsedResultRestoresTheDisclosureExactlyOnce(t *testing.T) { + var lines []string + for i := 0; i < cardBodyMaxLines*3; i++ { + lines = append(lines, fmt.Sprintf("PROBE-LINE-%03d", i)) + } + result := agent.ToolResult{ + ToolCallID: "call-cli", + Name: "bash", + Status: tools.StatusOK, + Output: strings.Join(lines, "\n"), + EnforcementNotices: []string{cardNotice}, + } + + // The CLI's persisted payload IS this function now; encode it as the + // session store would. + encoded, err := json.Marshal(ToolResultSessionPayload(result)) + if err != nil { + t.Fatal(err) + } + rows := transcriptRowsFromSessionEvents([]sessions.Event{{Type: sessions.EventToolResult, Payload: json.RawMessage(encoded)}}) + if len(rows) != 1 { + t.Fatalf("expected one restored row, got %d", len(rows)) + } + if len(rows[0].enforcementNotices) == 0 { + t.Fatal("the headless payload carried no typed notices, so the resumed card cannot render the disclosure") + } + + for _, expanded := range []bool{false, true} { + card := renderedCard(rows[0], expanded) + if n := strings.Count(card, "WRITE_RESTRICTED"); n != 1 { + t.Errorf("expanded=%v: restored headless card rendered the disclosure %d time(s), want exactly 1:\n%s", expanded, n, card) + } + } + // Collapsed is the case that used to lose it: with no body shown there was + // nothing to carry a decorated notice. + if card := renderedCard(rows[0], false); !strings.Contains(card, "WRITE_RESTRICTED") { + t.Errorf("the collapsed restored card has no disclosure at all:\n%s", card) + } +} diff --git a/internal/tui/model.go b/internal/tui/model.go index 9473de06a..7b0c410eb 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -5774,16 +5774,17 @@ func (m model) runAgentWithOptions(runID int, runCtx context.Context, prompt str } } row := transcriptRow{ - kind: rowToolResult, - id: effectiveToolRowID(result.ToolCallID, callSeq[result.ToolCallID]), - text: toolResultRowText(result), - tool: result.Name, - status: result.Status, - detail: toolResultDetail(result), - meta: result.Meta, - runID: runID, - changedFiles: result.ChangedFiles, - changeSummaries: result.ChangeSummaries, + kind: rowToolResult, + id: effectiveToolRowID(result.ToolCallID, callSeq[result.ToolCallID]), + text: toolResultRowText(result), + tool: result.Name, + status: result.Status, + detail: toolResultDetail(result), + meta: result.Meta, + runID: runID, + changedFiles: result.ChangedFiles, + changeSummaries: result.ChangeSummaries, + enforcementNotices: result.EnforcementNotices, } // A successful Task/TaskOutput result is represented by a specialist card. // update_plan stays in the transcript as a rendered checklist; failures @@ -6017,19 +6018,51 @@ func (m model) sendAgentUsage(runID int, modelID string, event zeroruntime.Usage // toolResultDetail is the card body source: the rich card-only Display.Preview // (a code/diff preview) when present on a successful result, else the Output that // the model also saw. Error results keep their Output so the failure shows. +// +// UNDECORATED, ALWAYS. The enforcement disclosure is carried separately as typed +// notices and rendered by the card as its own furniture, so a body that already +// had the notice composed into it drew the warning twice: once in the notice +// lines and once at the top of the output. A rich preview never carried it, so +// only the no-preview results (every bash and exec card, and every error) were +// wrong, which is exactly the shape a preview-only test cannot see. +// +// One owner for composition: this returns the base text, and whoever presents it +// decorates once. Provider-facing text still goes through ModelOutput. func toolResultDetail(result agent.ToolResult) string { - display := result.HumanDisplay() + display := result.BaseDisplay() if strings.TrimSpace(display.Preview) != "" && (result.Status != tools.StatusError || result.Outcome.Finalized()) { return display.Preview } - return result.ModelOutput() + return result.BaseModelOutput() } // toolResultSessionPayload preserves both views of a tool result: output remains // the provider-facing text used for session context, while displayPreview keeps // the richer card body that was visible during the live run. The preview is only // stored when it differs, so ordinary tool results retain their compact event. +// +// A result carrying enforcement notices ALWAYS differs now, because output is +// decorated and the card body is not, so the undecorated body is written even +// when it is empty. Restoration keys on the field being PRESENT rather than +// non-empty for exactly that case: a command that printed nothing under an +// enforced profile has an empty body and a real notice, and falling back to +// output there would restore the decorated text and draw the notice twice. func toolResultSessionPayload(result agent.ToolResult) map[string]any { + return ToolResultSessionPayload(result) +} + +// ToolResultSessionPayload is THE serialization of a tool result into a session +// event, shared by the interactive and the headless writers. +// +// They used to spell it separately, and the headless one persisted only the +// decorated ModelOutput. Both write to the same default session store the TUI +// resumes from, so a CLI-written result restored into the TUI arrived with no +// typed notices and no undecorated body: for a long collapsed result the card +// rendered no body and therefore no disclosure at all, even though the run +// that produced it had shown one. One owner for the contract means one place +// where a field can go missing, and a test against this function covers both +// writers. +func ToolResultSessionPayload(result agent.ToolResult) map[string]any { output := result.ModelOutput() payload := map[string]any{ "toolCallId": result.ToolCallID, @@ -6037,15 +6070,21 @@ func toolResultSessionPayload(result agent.ToolResult) map[string]any { "status": string(result.Status), "output": output, } - if preview := toolResultDetail(result); strings.TrimSpace(preview) != "" && preview != output { + if preview := toolResultDetail(result); preview != output { payload["displayPreview"] = preview } + if result.Truncated { + payload["truncated"] = true + } if result.Redacted { payload["redacted"] = true } if len(result.Meta) > 0 { payload["meta"] = result.Meta } + if len(result.EnforcementNotices) > 0 { + payload["enforcementNotices"] = result.EnforcementNotices + } if len(result.ChangedFiles) > 0 { payload["changedFiles"] = result.ChangedFiles } diff --git a/internal/tui/render_cache.go b/internal/tui/render_cache.go index 8ce433542..65e0c09bd 100644 --- a/internal/tui/render_cache.go +++ b/internal/tui/render_cache.go @@ -140,6 +140,10 @@ func (m model) renderRowCacheKey(row transcriptRow, width int, rc rowContext, op appendRenderCacheField(&b, row.tool) appendRenderCacheField(&b, fmt.Sprint(row.status)) appendRenderCacheField(&b, row.detail) + // The disclosure renders into the card, so it keys the entry. row.text + // happens to carry it too, but only because ModelOutput prepends it, and that + // coupling is what hid the notice from the card in the first place. + appendRenderCacheField(&b, strings.Join(row.enforcementNotices, "\n")) appendRenderCacheField(&b, row.arg) appendRenderCacheField(&b, strconv.Itoa(row.runID)) appendRenderCacheField(&b, strconv.FormatBool(row.expanded)) diff --git a/internal/tui/rendering.go b/internal/tui/rendering.go index e619acb41..05331ae17 100644 --- a/internal/tui/rendering.go +++ b/internal/tui/rendering.go @@ -1524,6 +1524,28 @@ func (m model) renderRunningToolCard(row transcriptRow, width int, rc rowContext return toolCard(head, glyph, nil, "", zeroTheme.cardRun, width) } +// toolCardNoticeLines renders the enforcement disclosures that belong to this +// result, in the card itself. +// +// The notice reached row.text and stopped there: toolCardHead takes row.text but +// renders the action and target, so a result with a rich preview (every edit and +// write card) displayed no disclosure at all, collapsed or expanded. It is shown +// above the body and on the collapsed paths too, because a trade the operator has +// to expand a card to discover is not disclosed. +func toolCardNoticeLines(notices []string, width int) []string { + var lines []string + for _, notice := range notices { + notice = strings.TrimSpace(notice) + if notice == "" { + continue + } + for _, wrapped := range wrapPlainText(notice, width) { + lines = append(lines, zeroTheme.amber.Render(wrapped)) + } + } + return lines +} + func renderToolResultCard(row transcriptRow, width int, rc rowContext, opts cardRenderOptions) string { name := toolRowName(row) failed := row.status == tools.StatusError @@ -1541,6 +1563,7 @@ func renderToolResultCard(row transcriptRow, width int, rc rowContext, opts card borderStyle = zeroTheme.cardErr } key := rcKey(row.runID, row.id) + noticeLines := toolCardNoticeLines(row.enforcementNotices, width) headTarget := rc.hints[key] headArg := rc.args[key] if !failed && isExploreTool(name) { @@ -1554,7 +1577,7 @@ func renderToolResultCard(row transcriptRow, width int, rc rowContext, opts card // Only for clean OK results: errors and anything multi-line keep their body. if !failed && opts.bodyCap > 0 && !toolCardAlwaysExpands(name) && looksLikeRedundantConfirmation(row.detail) { head := toolCardHead(name, headTarget, headArg, "", row.detail, row.text, false, nameStyle, rc.auto[key], width, opts) - return toolCard(head, glyph, nil, "", borderStyle, width) + return toolCard(head, glyph, noticeLines, "", borderStyle, width) } // Collapse long, noisy output (web-search/MCP/read dumps) by default so the // transcript stays scannable; the model still received the full output. Click @@ -1567,7 +1590,7 @@ func renderToolResultCard(row transcriptRow, width int, rc rowContext, opts card } if collapsedFooter != "" && !row.expanded { head := toolCardHead(name, headTarget, headArg, toolResultBudgetTag(row.meta), row.detail, row.text, false, nameStyle, rc.auto[key], width, opts) - return toolCard(head, glyph, nil, collapsedFooter, borderStyle, width) + return toolCard(head, glyph, noticeLines, collapsedFooter, borderStyle, width) } bodyOpts := opts bodyOpts.expanded = row.expanded @@ -1577,7 +1600,7 @@ func renderToolResultCard(row transcriptRow, width int, rc rowContext, opts card if collapsedFooter != "" && row.expanded && footer == "" { footer = "▾ collapse" } - return toolCard(head, glyph, body.lines, footer, borderStyle, width) + return toolCard(head, glyph, append(noticeLines, body.lines...), footer, borderStyle, width) } func joinToolHeadTags(tags ...string) string { diff --git a/internal/tui/session.go b/internal/tui/session.go index 4aec00e58..bab650454 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -642,20 +642,26 @@ func transcriptRowsFromSessionEvents(events []sessions.Event) []transcriptRow { status = tools.StatusOK } output := payloadString(payload, "output") - detail := payloadString(payload, "displayPreview") - if detail == "" { - detail = output + // PRESENCE, not emptiness. displayPreview is the undecorated card + // body; output carries the enforcement notice composed in. An empty + // stored body is a real answer (a command that printed nothing under + // an enforced profile), and treating it as absent would restore the + // decorated output and render the disclosure twice. + detail := output + if raw, ok := payload["displayPreview"]; ok { + detail, _ = raw.(string) } rows = append(rows, transcriptRow{ - kind: rowToolResult, - id: effectiveToolRowID(id, callSeq[id]), - text: fmt.Sprintf("tool result: %s %s %s", name, status, truncateTUIOutput(output, tuiToolOutputLimit)), - tool: name, - status: status, - detail: detail, - meta: payloadStringMap(payload, "meta"), - changedFiles: payloadStringSlice(payload, "changedFiles"), - changeSummaries: payloadExecutionChanges(payload, "changeSummaries"), + kind: rowToolResult, + id: effectiveToolRowID(id, callSeq[id]), + text: fmt.Sprintf("tool result: %s %s %s", name, status, truncateTUIOutput(output, tuiToolOutputLimit)), + tool: name, + status: status, + detail: detail, + meta: payloadStringMap(payload, "meta"), + changedFiles: payloadStringSlice(payload, "changedFiles"), + enforcementNotices: payloadStringSlice(payload, "enforcementNotices"), + changeSummaries: payloadExecutionChanges(payload, "changeSummaries"), }) case sessions.EventError: if message := payloadString(payload, "message"); message != "" { diff --git a/internal/tui/transcript.go b/internal/tui/transcript.go index 64d657c14..8cf3fe753 100644 --- a/internal/tui/transcript.go +++ b/internal/tui/transcript.go @@ -50,6 +50,16 @@ type transcriptRow struct { changedFiles []string changeSummaries []execution.Change + // enforcementNotices are the least-privilege disclosures that were true of + // this result (from tools.Result.EnforcementNotices; restored from the + // session payload on resume). + // + // Held as its own field rather than folded into detail. detail is parsed as a + // diff by the files panel and the file view, so prefixing it with prose would + // corrupt both. It is not folded into text either: text carries the notice + // today and the card never renders it, which is the whole defect. + enforcementNotices []string + // specialistInfo holds the specialist card data for rowSpecialist rows. // Nil for all other row kinds. specialistInfo *specialistInfo