diff --git a/internal/cli/app.go b/internal/cli/app.go index b244b413e..64d266020 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -51,7 +51,7 @@ type appDeps struct { stdin io.Reader userConfigPath func() (string, error) resolveConfig func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) - resolveMCPConfig func(workspaceRoot string) (config.MCPConfig, error) + resolveMCPConfig func(workspaceRoot string, excludeProject bool) (config.MCPConfig, error) newProvider func(config.ProviderProfile) (zeroruntime.Provider, error) // exportActiveProvider pins spawned children to the run's provider (production: // config.SetActiveProviderEnv, set in defaultAppDeps — deliberately NOT filled @@ -125,11 +125,12 @@ func defaultAppDeps() appDeps { options.Overrides = overrides return config.Resolve(options) }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, excludeProject bool) (config.MCPConfig, error) { options, err := config.DefaultResolveOptions(workspaceRoot) if err != nil { return config.MCPConfig{}, err } + options.ExcludeProject = excludeProject return config.ResolveMCP(options) }, newProvider: func(profile config.ProviderProfile) (zeroruntime.Provider, error) { @@ -406,6 +407,8 @@ func runWithDeps(args []string, stdout io.Writer, stderr io.Writer, deps appDeps return runWorktrees(args[1:], stdout, stderr, deps) case "verify": return runVerifyCommand(args[1:], stdout, stderr, deps) + case "trust": + return runTrust(args[1:], stdout, stderr, deps) case "eval": return runAgentEvalCommand(args[1:], stdout, stderr, deps) case "changes", "change": @@ -669,7 +672,17 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a return writeAppError(stderr, "failed to initialize specialist tools: "+err.Error(), 1) } defer closeSpecialistRuntime(stderr, specialistRuntime) - mcpConfig, err := deps.resolveMCPConfig(workspaceRoot) + // The TUI has no --worktree reassignment, so trustRoot == workspaceRoot here. + // Gate the project MCP layer behind the workspace-trust check (fail-closed): an + // untrusted workspace must not spawn its ./.zero/config.json stdio MCP servers. + // Keep the store-read error so the notice below can distinguish a fail-closed + // store error from a clean untrusted verdict. + mcpExcludeProject, mcpTrustErrored := resolveTrust(workspaceRoot) + mcpSkip := trustSkip{ + excludedProjectConfig: mcpExcludeProject && projectMCPConfigExists(workspaceRoot), + trustCheckErrored: mcpTrustErrored, + } + mcpConfig, err := deps.resolveMCPConfig(workspaceRoot, mcpExcludeProject) if err != nil { return writeAppError(stderr, err.Error(), 1) } @@ -711,7 +724,10 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a // collect their hooks + skill roots for the dispatcher and skill tool below. // Done after specialist + MCP registration so plugin tools are part of the // deferral count, and it fails OPEN — a malformed plugin is warned and skipped. - pluginActivation := activatePlugins(workspaceRoot, registry, deps, stderr) + // The interactive TUI is not worktree-reassigned, so the trust root is the + // launch directory itself. + trustRoot := workspaceRoot + pluginActivation := activatePlugins(workspaceRoot, registry, deps, stderr, trustRoot) // Ask (not Auto) is the interactive default: in Auto, ToolAdvertised exposes // only PermissionAllow tools, so prompt-gated tools (write_file/edit_file/bash/ // apply_patch) would never be offered to the model — the TUI could neither edit @@ -758,6 +774,11 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a if userConfigPath != "" { sttDownloadRoot = filepath.Join(filepath.Dir(userConfigPath), "stt") } + // Build the hooks dispatcher out of the AgentOptions literal so its trust skip + // report can be combined with the plugin activation's, and emit at most one + // notice when project hooks/plugins were dropped for an untrusted workspace. + hookDispatcher, hookSkip := newHookDispatcherWithExtra(workspaceRoot, pluginActivation.hooks, trustRoot) + emitTrustNotice(stderr, hookSkip, pluginActivation.trustSkip, mcpSkip) return deps.runTUI(context.Background(), tui.Options{ Cwd: workspaceRoot, Version: version, @@ -796,7 +817,7 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a var stdout, stderr bytes.Buffer exitCode := runMCPWithContext(ctx, args, &stdout, &stderr, deps) nextConfig := lastKnownMCPConfig - if refreshed, err := deps.resolveMCPConfig(workspaceRoot); err == nil { + if refreshed, err := deps.resolveMCPConfig(workspaceRoot, mcpExcludeProject); err == nil { lastKnownMCPConfig = refreshed nextConfig = refreshed } @@ -815,7 +836,7 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a Autonomy: "low", Sandbox: sandboxEngine, FileTracker: fileTracker, - Hooks: newHookDispatcherWithExtra(workspaceRoot, pluginActivation.hooks), + Hooks: hookDispatcher, DeferThreshold: resolved.Tools.DeferThreshold, Specialists: specialistRuntime.specialists, Skills: pluginActivation.skillInfos(deps.skillsDir()), diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index 8e3d58bb4..d56bd9063 100644 --- a/internal/cli/app_test.go +++ b/internal/cli/app_test.go @@ -20,6 +20,7 @@ import ( "github.com/Gitlawb/zero/internal/tools" "github.com/Gitlawb/zero/internal/tui" "github.com/Gitlawb/zero/internal/update" + "github.com/Gitlawb/zero/internal/workspacetrust" "github.com/Gitlawb/zero/internal/zeroruntime" ) @@ -179,7 +180,7 @@ func TestTUIStartupSuppressesWarningForUnconfiguredDefaultServer(t *testing.T) { resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { return config.ResolvedConfig{MaxTurns: 8}, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, excludeProject bool) (config.MCPConfig, error) { return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "firecrawl": config.DefaultMCPServers()["firecrawl"], }}, nil @@ -215,7 +216,7 @@ func TestTUIStartupWarnsForUserConfiguredServerSkip(t *testing.T) { resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { return config.ResolvedConfig{MaxTurns: 8}, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, excludeProject bool) (config.MCPConfig, error) { return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "custom": {Type: "stdio", Command: "custom-mcp"}, }}, nil @@ -404,7 +405,7 @@ func TestRunNoArgsLaunchesTUIWithMCPState(t *testing.T) { resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { return config.ResolvedConfig{MaxTurns: 8}, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { if workspaceRoot != cwd { t.Fatalf("workspaceRoot = %q, want %q", workspaceRoot, cwd) } @@ -479,7 +480,7 @@ func TestTUIMCPCommandUsesLastGoodConfigOnRefreshError(t *testing.T) { resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { return config.ResolvedConfig{MaxTurns: 8}, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { resolveCalls++ switch resolveCalls { case 1: @@ -546,7 +547,7 @@ func TestRunNoArgsClosesPartialMCPRuntimeWhenRegistrationFails(t *testing.T) { resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { return config.ResolvedConfig{MaxTurns: 8}, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "docs": {Type: "stdio", Command: "docs-mcp"}, }}, nil @@ -601,7 +602,7 @@ func TestRunNoArgsSoftFailsMCPTokenStoreInit(t *testing.T) { resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { return config.ResolvedConfig{MaxTurns: 8}, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { return config.MCPConfig{}, nil }, newMCPStore: func() (*mcp.PermissionStore, error) { @@ -740,6 +741,52 @@ func TestRunNoArgsLaunchesTUIInAskPermissionMode(t *testing.T) { } } +// TestRunInteractiveSurfacesMCPTrustNotice executes the interactive (TUI) path's MCP +// trust notice (app.go), the third notice site alongside exec and spec-draft. An +// untrusted repo whose only project config is MCP must print the notice before runTUI; +// trusting it silences it. runTUI is stubbed so nothing renders; resolveMCPConfig +// returns no servers so nothing spawns -- the notice depends only on the trust verdict +// and the real ./.zero/config.json. +func TestRunInteractiveSurfacesMCPTrustNotice(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + if err := os.MkdirAll(filepath.Join(repo, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + body := `{"mcp":{"servers":{"proj":{"type":"stdio","command":"proj-cmd"}}}}` + if err := os.WriteFile(filepath.Join(repo, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + run := func() string { + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return repo, nil }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { + return config.ResolvedConfig{MaxTurns: 3}, nil + }, + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{}, nil }, + runTUI: func(context.Context, tui.Options) int { return 0 }, + }) + if code != 0 { + t.Fatalf("interactive launch exit = %d, stderr=%q", code, stderr.String()) + } + return stderr.String() + } + + untrusted := run() + if !strings.Contains(untrusted, "MCP servers") || !strings.Contains(untrusted, "zero trust") { + t.Fatalf("untrusted interactive launch must surface the MCP trust notice, stderr=%q", untrusted) + } + + if err := workspacetrust.Trust(repo); err != nil { + t.Fatal(err) + } + if trusted := run(); strings.Contains(trusted, "ignoring project") { + t.Fatalf("trusted interactive launch must not emit a trust notice, stderr=%q", trusted) + } +} + func TestRunSkipPermissionsUnsafeLaunchesTUIInUnsafeMode(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer diff --git a/internal/cli/backends.go b/internal/cli/backends.go index 1d5aa550d..7431157e4 100644 --- a/internal/cli/backends.go +++ b/internal/cli/backends.go @@ -125,7 +125,9 @@ func backendLifecycleSnapshot(deps appDeps) (zerocommands.BackendLifecycleSnapsh return zerocommands.BackendLifecycleSnapshot{}, fmt.Errorf("failed to resolve workspace: %w", err) } - cfg, err := deps.resolveMCPConfig(cwd) + // Reporting/enumeration only, never spawns a server, so it is left ungated + // (excludeProject=false) to mirror the doctor/status hooks and plugins reports. + cfg, err := deps.resolveMCPConfig(cwd, false) if err != nil { return zerocommands.BackendLifecycleSnapshot{}, err } @@ -152,7 +154,9 @@ func backendDoctorReport(deps appDeps) (zerocommands.BackendDoctorReport, error) return zerocommands.BackendDoctorReport{}, fmt.Errorf("failed to resolve workspace: %w", err) } - cfg, err := deps.resolveMCPConfig(cwd) + // Reporting/enumeration only, never spawns a server, so it is left ungated + // (excludeProject=false) to mirror the doctor/status hooks and plugins reports. + cfg, err := deps.resolveMCPConfig(cwd, false) if err != nil { return zerocommands.BackendDoctorReport{}, err } diff --git a/internal/cli/backends_test.go b/internal/cli/backends_test.go index 23b45a5fa..3a1f7fb4d 100644 --- a/internal/cli/backends_test.go +++ b/internal/cli/backends_test.go @@ -26,7 +26,7 @@ func TestRunBackendsJSONUsesLifecycleSnapshotWithoutConnectingMCP(t *testing.T) secret := "sk-proj-" + strings.Repeat("a", 24) deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { if workspaceRoot != cwd { t.Fatalf("workspaceRoot = %q, want %q", workspaceRoot, cwd) } @@ -123,7 +123,7 @@ func TestRunBackendsJSONUsesLifecycleSnapshotWithoutConnectingMCP(t *testing.T) func TestRunBackendsTextAndHelp(t *testing.T) { deps := appDeps{ getwd: func() (string, error) { return t.TempDir(), nil }, - resolveMCPConfig: func(string) (config.MCPConfig, error) { + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{}, nil }, loadHooks: func(hooks.LoadOptions) (hooks.LoadResult, error) { @@ -164,7 +164,7 @@ func TestRunBackendsDoctorJSONAndTextWithoutConnectingMCP(t *testing.T) { secret := "sk-proj-" + strings.Repeat("b", 24) deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { if workspaceRoot != cwd { t.Fatalf("workspaceRoot = %q, want %q", workspaceRoot, cwd) } @@ -301,7 +301,7 @@ func TestRunBackendsDoctorDoesNotConnectOrExecuteConfiguredBackends(t *testing.T deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, - resolveMCPConfig: func(string) (config.MCPConfig, error) { + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "remote": {Type: "http", URL: server.URL + "/mcp"}, }}, nil diff --git a/internal/cli/deferred_wiring_test.go b/internal/cli/deferred_wiring_test.go index c57405555..c8af2c9d5 100644 --- a/internal/cli/deferred_wiring_test.go +++ b/internal/cli/deferred_wiring_test.go @@ -142,7 +142,7 @@ func TestRunExecListToolsAdvertisesMCPToolsWithoutToolSearch(t *testing.T) { resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { return execResolvedConfig(), nil }, - resolveMCPConfig: func(string) (config.MCPConfig, error) { + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "docs": {Type: "stdio", Command: "docs-mcp"}, }}, nil @@ -177,7 +177,7 @@ func TestRunExecListToolsHonorsJSONFormat(t *testing.T) { resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { return execResolvedConfig(), nil }, - resolveMCPConfig: func(string) (config.MCPConfig, error) { return config.MCPConfig{}, nil }, + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{}, nil }, newMCPStore: func() (*mcp.PermissionStore, error) { return nil, nil }, registerMCPTools: func(_ context.Context, _ *tools.Registry, _ config.MCPConfig, _ mcp.RegisterOptions) (mcpToolRuntime, error) { return closeFunc(func() error { return nil }), nil @@ -234,7 +234,7 @@ func TestTUIRunThreadsDeferThresholdAndRegistersToolSearch(t *testing.T) { Tools: config.ToolsConfig{DeferThreshold: 2}, }, nil }, - resolveMCPConfig: func(string) (config.MCPConfig, error) { + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "docs": {Type: "stdio", Command: "docs-mcp"}, }}, nil diff --git a/internal/cli/exec.go b/internal/cli/exec.go index e6761698f..86ceade70 100644 --- a/internal/cli/exec.go +++ b/internal/cli/exec.go @@ -155,6 +155,10 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in if err != nil { return writeExecFormatUsageError(stdout, stderr, options.outputFormat, err.Error()) } + // trustRoot is the ORIGINAL launch directory, captured before any --worktree + // reassignment below, so a worktree of a trusted repo inherits that repo's + // trust instead of being seen as a fresh untrusted path. + trustRoot := workspaceRoot if options.worktree { preparedWorktree, err := deps.prepareWorktree(context.Background(), worktrees.Options{ Cwd: workspaceRoot, @@ -201,7 +205,7 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in if options.useSpec { permissionMode = agent.PermissionModeSpecDraft } - mcpRuntime, err := registerMCPToolsForWorkspace(context.Background(), workspaceRoot, registry, deps, execMCPAutonomy(options)) + mcpRuntime, mcpSkip, err := registerMCPToolsForWorkspace(context.Background(), workspaceRoot, registry, deps, execMCPAutonomy(options), trustRoot) if err != nil { return writeExecProviderError(stdout, stderr, options.outputFormat, "mcp_error", err.Error()) } @@ -210,7 +214,7 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in // registry and collect their hooks + skill roots for the dispatcher and skill // tool below. Done before --list-tools and filter validation so plugin tools // are listable and filter-validatable; it fails OPEN (a bad plugin is skipped). - pluginActivation := activatePlugins(workspaceRoot, registry, deps, stderr) + pluginActivation := activatePlugins(workspaceRoot, registry, deps, stderr, trustRoot) if options.useSpec { specmode.RegisterDraftTools(registry, workspaceRoot, deps.now) } @@ -415,6 +419,8 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in stderr: stderr, deps: deps, workspaceRoot: workspaceRoot, + trustRoot: trustRoot, + mcpSkip: mcpSkip, registry: registry, modelRegistry: modelRegistry, resolved: resolved, @@ -519,6 +525,11 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in writer.warning(notice) } } + // Build the hooks dispatcher out of the struct literal so its trust skip report + // can be combined with the plugin activation's, and emit at most one notice when + // project hooks/plugins were dropped for an untrusted workspace. + hookDispatcher, hookSkip := newHookDispatcherWithExtra(workspaceRoot, pluginActivation.hooks, trustRoot) + emitTrustNotice(stderr, hookSkip, pluginActivation.trustSkip, mcpSkip) result, err := agent.Run(runCtx, agentPrompt, provider, agent.Options{ MaxTurns: resolved.MaxTurns, ContextWindow: resolveAgentContextWindow(runCtx, modelRegistry, resolved.Provider), @@ -549,7 +560,7 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in RequireCompletionSignal: true, Sandbox: sandboxEngine, FileTracker: fileTracker, - Hooks: newHookDispatcherWithExtra(workspaceRoot, pluginActivation.hooks), + Hooks: hookDispatcher, EnabledTools: options.enabledTools, DisabledTools: options.disabledTools, OnText: writer.text, diff --git a/internal/cli/exec_protocol_test.go b/internal/cli/exec_protocol_test.go index f9500dc85..4750572d4 100644 --- a/internal/cli/exec_protocol_test.go +++ b/internal/cli/exec_protocol_test.go @@ -150,7 +150,7 @@ func TestRunExecListsMCPToolsWithoutProviderConstruction(t *testing.T) { providerBuilt = true return nil, errors.New("provider should not be constructed for --list-tools") }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { if workspaceRoot != cwd { t.Fatalf("workspaceRoot = %q, want %q", workspaceRoot, cwd) } @@ -193,7 +193,7 @@ func TestRunExecLogsMCPRuntimeCloseError(t *testing.T) { getwd: func() (string, error) { return cwd, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { if workspaceRoot != cwd { t.Fatalf("workspaceRoot = %q, want %q", workspaceRoot, cwd) } diff --git a/internal/cli/exec_spec.go b/internal/cli/exec_spec.go index 3fb6c7665..2fe910242 100644 --- a/internal/cli/exec_spec.go +++ b/internal/cli/exec_spec.go @@ -21,11 +21,19 @@ import ( ) type execSpecDraftRun struct { - options execOptions - stdout io.Writer - stderr io.Writer - deps appDeps - workspaceRoot string + options execOptions + stdout io.Writer + stderr io.Writer + deps appDeps + workspaceRoot string + // trustRoot is the ORIGINAL launch directory (captured before any --worktree + // reassignment), so a --use-spec run inside a --worktree of a trusted repo + // still keys the trust check on the source repo, not the generated worktree path. + trustRoot string + // mcpSkip is the trust verdict from the workspace MCP registration in runExec + // (which runs before this spec-draft path), so the spec-draft notice can report a + // dropped project MCP layer instead of leaving it silent. + mcpSkip trustSkip registry *tools.Registry modelRegistry modelregistry.Registry resolved config.ResolvedConfig @@ -96,6 +104,13 @@ func runExecSpecDraft(run execSpecDraftRun) int { var draftInfo execSpecDraftInfo runCtx, stopSignals := signalContext() defer stopSignals() + // The spec-draft path activates no plugins, so the plugin skip is omitted. Trust + // keys on run.trustRoot (the original launch dir), not run.workspaceRoot, which + // may be a --worktree path; this keeps a --use-spec --worktree run of a trusted + // repo trusted. Emit at most one notice when project hooks or the project MCP + // layer (registered earlier in runExec, carried in run.mcpSkip) were dropped. + hookDispatcher, hookSkip := newHookDispatcher(run.workspaceRoot, run.trustRoot) + emitTrustNotice(run.stderr, hookSkip, run.mcpSkip) result, err := agent.Run(runCtx, run.prompt, run.provider, agent.Options{ MaxTurns: run.resolved.MaxTurns, ContextWindow: resolveAgentContextWindow(runCtx, run.modelRegistry, run.resolved.Provider), @@ -112,7 +127,7 @@ func runExecSpecDraft(run execSpecDraftRun) int { Autonomy: "low", Sandbox: run.sandboxEngine, FileTracker: tools.NewFileTracker(), - Hooks: newHookDispatcher(run.workspaceRoot), + Hooks: hookDispatcher, EnabledTools: run.options.enabledTools, DisabledTools: run.options.disabledTools, OnText: writer.text, diff --git a/internal/cli/exec_spec_test.go b/internal/cli/exec_spec_test.go index 7dc684c78..106cfd2fa 100644 --- a/internal/cli/exec_spec_test.go +++ b/internal/cli/exec_spec_test.go @@ -51,7 +51,7 @@ func TestRunExecUseSpecCreatesDraftSession(t *testing.T) { MaxTurns: 3, }, nil }, - resolveMCPConfig: func(string) (config.MCPConfig, error) { + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{}, nil }, newProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { diff --git a/internal/cli/extensions.go b/internal/cli/extensions.go index 9a4e2180e..1a9adae5c 100644 --- a/internal/cli/extensions.go +++ b/internal/cli/extensions.go @@ -205,7 +205,9 @@ func runMCPLegacyList(args []string, stdout io.Writer, stderr io.Writer, deps ap if err != nil { return writeAppError(stderr, "failed to resolve workspace: "+err.Error(), exitCrash) } - cfg, err := deps.resolveMCPConfig(cwd) + // Enumeration for the extensions listing, never spawns a server, so it is left + // ungated (excludeProject=false) like the other doctor/status report sites. + cfg, err := deps.resolveMCPConfig(cwd, false) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } @@ -286,11 +288,21 @@ func runMCPTools(ctx context.Context, args []string, stdout io.Writer, stderr io return writeAppError(stderr, "failed to resolve workspace: "+err.Error(), exitCrash) } registry := tools.NewRegistry() - mcpRuntime, err := registerMCPToolsForWorkspace(ctx, cwd, registry, deps, mcp.AutonomyLow) + // `mcp tools list` connects to (spawns) each server to enumerate its live tools, + // so it is a spawn site and gates the project layer behind the trust check. No + // --worktree reassignment on this command path, so trustRoot == cwd. + mcpRuntime, mcpSkip, err := registerMCPToolsForWorkspace(ctx, cwd, registry, deps, mcp.AutonomyLow, cwd) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } defer closeMCPRuntime(stderr, mcpRuntime) + // Surface the trust skip on stderr so an empty (or short) list in an untrusted + // workspace is explained rather than read as "nothing configured". The notice + // goes to stderr, leaving the list (text or --json) on stdout intact. + emitTrustNotice(stderr, mcpSkip) + // Surface the trust skip on stderr so an empty (or short) list in an untrusted + // workspace is explained rather than read as "nothing configured". The notice + // goes to stderr, leaving the list (text or --json) on stdout intact. items := mcpToolList(registry) if options.json { payload := struct { diff --git a/internal/cli/extensions_test.go b/internal/cli/extensions_test.go index cab17d748..00af4dd9b 100644 --- a/internal/cli/extensions_test.go +++ b/internal/cli/extensions_test.go @@ -204,7 +204,7 @@ func TestRunMCPToolsListJSONAndText(t *testing.T) { closeCalls := 0 deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { if workspaceRoot != cwd { t.Fatalf("workspaceRoot = %q, want %q", workspaceRoot, cwd) } @@ -264,7 +264,7 @@ func TestRunMCPLegacyListAliases(t *testing.T) { closeCalls := 0 deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { if workspaceRoot != cwd { t.Fatalf("workspaceRoot = %q, want %q", workspaceRoot, cwd) } diff --git a/internal/cli/hook_dispatch.go b/internal/cli/hook_dispatch.go index 25c09d166..5d122c900 100644 --- a/internal/cli/hook_dispatch.go +++ b/internal/cli/hook_dispatch.go @@ -1,17 +1,63 @@ package cli import ( + "fmt" + "io" + "os" + "path/filepath" + "github.com/Gitlawb/zero/internal/hooks" + "github.com/Gitlawb/zero/internal/workspacetrust" ) +// trustSkip reports whether a runtime chokepoint dropped the project layer because +// the workspace was not trusted, so the caller can emit a single combined notice. +// Both chokepoints (hooks and plugins) return one, and the caller ORs them. +type trustSkip struct { + // excludedProjectConfig is true when the project layer was excluded AND a + // project config for this surface actually exists on disk (so a notice is + // worth showing). It stays false in a trusted workspace, or when the + // workspace has no project config to skip. + excludedProjectConfig bool + // trustCheckErrored is true when the exclusion was caused by a trust-store + // read error rather than a clean untrusted verdict, so the caller can name + // the error in its notice. + trustCheckErrored bool +} + +// resolveTrust computes the fail-closed trust verdict for a chokepoint. It treats +// any error OR an empty trustRoot as untrusted, so a forgotten or future call site +// cannot fail open. It returns whether the project layer should be excluded and +// whether the decision was driven by a store-read error. +// +// Each chokepoint (hooks and plugins) calls this itself rather than the caller +// resolving trust once and passing a bool down. That is deliberate: keeping the +// check inside the chokepoint is what makes the gate fail-closed by construction. +// Do NOT hoist it to the callers behind an excludeProject bool whose zero value +// includes the project layer, that reintroduces a fail-open default. The extra +// store read per session is negligible. +func resolveTrust(trustRoot string) (excludeProject bool, trustCheckErrored bool) { + if trustRoot == "" { + return true, false + } + trusted, err := workspacetrust.IsTrusted(trustRoot) + if err != nil { + return true, true + } + return !trusted, false +} + // newHookDispatcher builds the per-session hooks dispatcher for a workspace, // merging user + project hooks.json and wiring the audit store. It fails OPEN: // any load or setup error yields a nil dispatcher, which Dispatch treats as a // no-op, so a malformed hooks config can never wedge tool execution. With no // hooks configured the dispatcher selects nothing and runs no commands, so the // hot path stays free of overhead until a user opts in via hooks.json. -func newHookDispatcher(workspaceRoot string) *hooks.Dispatcher { - return newHookDispatcherWithExtra(workspaceRoot, nil) +// +// trustRoot is the original launch directory (resolved before any --worktree +// reassignment); the project layer loads only when that root is trusted. +func newHookDispatcher(workspaceRoot string, trustRoot string) (*hooks.Dispatcher, trustSkip) { + return newHookDispatcherWithExtra(workspaceRoot, nil, trustRoot) } // newHookDispatcherWithExtra builds the dispatcher like newHookDispatcher but also @@ -20,10 +66,22 @@ func newHookDispatcher(workspaceRoot string) *hooks.Dispatcher { // appended after the configured hooks; their ids are plugin-namespaced (plugin // id + hook name) so they never collide with hooks.json ids. A nil/empty extra // slice is byte-equivalent to newHookDispatcher. -func newHookDispatcherWithExtra(workspaceRoot string, extra []hooks.Definition) *hooks.Dispatcher { - loaded, err := hooks.LoadConfig(hooks.LoadOptions{Cwd: workspaceRoot}) +// +// The trust check lives here, inside the chokepoint, so no caller can bypass it: +// the project layer is dropped (ExcludeProject) whenever trustRoot is empty, the +// trust store cannot be read, or the workspace is not trusted (fail-closed). The +// returned trustSkip lets the caller emit one combined notice; the notice itself +// is NOT emitted here. +func newHookDispatcherWithExtra(workspaceRoot string, extra []hooks.Definition, trustRoot string) (*hooks.Dispatcher, trustSkip) { + excludeProject, trustCheckErrored := resolveTrust(trustRoot) + skip := trustSkip{ + excludedProjectConfig: excludeProject && projectHooksFileExists(workspaceRoot), + trustCheckErrored: trustCheckErrored, + } + + loaded, err := hooks.LoadConfig(hooks.LoadOptions{Cwd: workspaceRoot, ExcludeProject: excludeProject}) if err != nil { - return nil + return nil, skip } var audit *hooks.AuditStore if store, err := hooks.NewAuditStore(hooks.AuditStoreOptions{}); err == nil { @@ -52,5 +110,42 @@ func newHookDispatcherWithExtra(workspaceRoot string, extra []hooks.Definition) Config: config, Audit: audit, Cwd: workspaceRoot, - }) + }), skip +} + +// projectHooksFileExists reports whether a ./.zero/hooks.json is present under +// workspaceRoot, so the caller only notices about config it actually skipped. +func projectHooksFileExists(workspaceRoot string) bool { + if workspaceRoot == "" { + return false + } + info, err := os.Stat(filepath.Join(workspaceRoot, ".zero", "hooks.json")) + return err == nil && !info.IsDir() +} + +// emitTrustNotice writes at most one stderr line summarizing that project-scoped +// hooks, plugins, and/or MCP servers were skipped in an untrusted workspace. It +// takes the skip report from each trust-gated surface (hooks, plugins, MCP) and ORs +// them, so a workspace that only drops one surface still gets the notice. It is +// computed once per session by the caller (each session-setup site runs once), so it +// is naturally once-per-process. When any surface's skip was a trust-store read +// error, the notice names that so a transient config-dir problem is diagnosable. +func emitTrustNotice(stderr io.Writer, skips ...trustSkip) { + if stderr == nil { + return + } + excluded := false + storeErrored := false + for _, skip := range skips { + excluded = excluded || skip.excludedProjectConfig + storeErrored = storeErrored || skip.trustCheckErrored + } + if !excluded { + return + } + if storeErrored { + _, _ = fmt.Fprintln(stderr, "zero: the workspace-trust store could not be read; ignoring project hooks/plugins/MCP servers (fail-closed). Run 'zero trust' to enable.") + return + } + _, _ = fmt.Fprintln(stderr, "zero: ignoring project hooks/plugins/MCP servers in an untrusted workspace. Run 'zero trust' to enable.") } diff --git a/internal/cli/mcp_commands_test.go b/internal/cli/mcp_commands_test.go index 7be290c4c..df2b0c228 100644 --- a/internal/cli/mcp_commands_test.go +++ b/internal/cli/mcp_commands_test.go @@ -519,7 +519,7 @@ func TestRunMCPListRedactsURLCredentialsAndSensitiveQueryParams(t *testing.T) { commandSecret := "sk-proj-" + strings.Repeat("a", 24) deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { if workspaceRoot != cwd { t.Fatalf("workspaceRoot = %q, want %q", workspaceRoot, cwd) } @@ -566,7 +566,7 @@ func TestRunMCPCheckRegistersOnlyRequestedServer(t *testing.T) { exitCode := runWithDeps([]string{"mcp", "check", "docs", "--json"}, &stdout, &stderr, appDeps{ getwd: func() (string, error) { return cwd, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { if workspaceRoot != cwd { t.Fatalf("workspaceRoot = %q, want %q", workspaceRoot, cwd) } @@ -618,7 +618,7 @@ func TestRunMCPCheckClosesRuntimeReturnedWithError(t *testing.T) { exitCode := runWithDeps([]string{"mcp", "check", "docs"}, &stdout, &stderr, appDeps{ getwd: func() (string, error) { return cwd, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "docs": {Type: "stdio", Command: "docs-mcp"}, }}, nil diff --git a/internal/cli/mcp_config.go b/internal/cli/mcp_config.go index eaf2cf69f..603fcd50a 100644 --- a/internal/cli/mcp_config.go +++ b/internal/cli/mcp_config.go @@ -29,6 +29,27 @@ type mcpWritableConfig struct { serverRaw map[string]json.RawMessage } +// projectMCPConfigExists reports whether the workspace's project ./.zero/config.json +// declares any MCP servers, so the trust notice fires only when project MCP config was +// actually skipped (mirroring projectHooksFileExists / projectPluginsDirExists). A +// missing or unparseable file, or one that declares no servers, returns false: there +// is nothing to notice about. This only reads the file; it never spawns a server, so +// it is safe to call on an untrusted workspace. +func projectMCPConfigExists(workspaceRoot string) bool { + if workspaceRoot == "" { + return false + } + data, err := os.ReadFile(filepath.Join(workspaceRoot, ".zero", "config.json")) + if err != nil { + return false + } + var fc config.FileConfig + if err := json.Unmarshal(data, &fc); err != nil { + return false + } + return len(fc.MCP.Servers) > 0 +} + func runMCPAdd(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { options, help, err := parseMCPAddArgs(args) if err != nil { @@ -243,12 +264,25 @@ func runMCPCheck(ctx context.Context, args []string, stdout io.Writer, stderr io if err != nil { return writeAppError(stderr, "failed to resolve workspace: "+err.Error(), exitCrash) } - cfg, err := deps.resolveMCPConfig(cwd) + // `mcp check` connects to (spawns) the named server below to enumerate its tools, + // so it is a spawn site: gate the project layer behind the trust check (fail-closed) + // so a cloned repo cannot have `zero mcp check ` run its command. No + // --worktree reassignment on this command path, so trustRoot == cwd. + mcpExcludeProject, trustCheckErrored := resolveTrust(cwd) + cfg, err := deps.resolveMCPConfig(cwd, mcpExcludeProject) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } raw, ok := cfg.Servers[serverName] if !ok { + // The server may be missing because the project layer was gated out in an + // untrusted workspace; emit the same one-line notice the other spawn sites use so + // a dropped project server reads as "run zero trust", not a bare miss. The notice + // self-gates: it stays silent unless a project MCP config was actually excluded. + emitTrustNotice(stderr, trustSkip{ + excludedProjectConfig: mcpExcludeProject && projectMCPConfigExists(cwd), + trustCheckErrored: trustCheckErrored, + }) return writeAppError(stderr, fmt.Sprintf("MCP server %q is not configured", serverName), exitCrash) } if raw.Disabled { diff --git a/internal/cli/mcp_oauth.go b/internal/cli/mcp_oauth.go index e4ffb2b4d..67e4d59e8 100644 --- a/internal/cli/mcp_oauth.go +++ b/internal/cli/mcp_oauth.go @@ -55,8 +55,12 @@ func runMCPOAuthLogin(args []string, stdout io.Writer, stderr io.Writer, deps ap } serverName := positional[0] - server, err := resolveOAuthServer(deps, serverName) + server, skip, err := resolveOAuthServer(deps, serverName) if err != nil { + // When the named server was dropped because the workspace is untrusted, explain + // that with the same one-line notice the other trust-gated MCP paths emit, so a + // gated project OAuth server reads as "run zero trust", not a bare miss. + emitTrustNotice(stderr, skip) return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } @@ -193,33 +197,47 @@ func filterTokenStatuses(statuses []mcp.TokenStatus, serverName string) []mcp.To } // resolveOAuthServer loads the workspace MCP config and returns the named server -// after verifying it declares OAuth authentication. -func resolveOAuthServer(deps appDeps, serverName string) (mcp.Server, error) { +// after verifying it declares OAuth authentication. It also returns a trustSkip so the +// caller can emit the workspace-trust notice when a project server was dropped. +// +// Login is gated behind workspace trust (fail-closed) like the stdio spawn paths: it +// loads a project-scoped MCP server and drives its OAuth flow (endpoint discovery, +// dynamic client registration, code exchange, token persist) against the server's +// configured URL. That is a project-scoped server being activated and contacted, so an +// untrusted clone's ./.zero/config.json OAuth server must be dropped rather than loaded — +// leaving it ungated would let a cloned repo initiate outbound OAuth I/O and persist a +// token. There is no --worktree reassignment on this command path, so trustRoot == cwd. +func resolveOAuthServer(deps appDeps, serverName string) (mcp.Server, trustSkip, error) { if err := mcp.ValidateServerName(serverName); err != nil { - return mcp.Server{}, err + return mcp.Server{}, trustSkip{}, err } cwd, err := deps.getwd() if err != nil { - return mcp.Server{}, fmt.Errorf("failed to resolve workspace: %w", err) + return mcp.Server{}, trustSkip{}, fmt.Errorf("failed to resolve workspace: %w", err) } - cfg, err := deps.resolveMCPConfig(cwd) + excludeProject, trustCheckErrored := resolveTrust(cwd) + skip := trustSkip{ + excludedProjectConfig: excludeProject && projectMCPConfigExists(cwd), + trustCheckErrored: trustCheckErrored, + } + cfg, err := deps.resolveMCPConfig(cwd, excludeProject) if err != nil { - return mcp.Server{}, err + return mcp.Server{}, skip, err } servers, err := mcp.NormalizeConfig(cfg) if err != nil { - return mcp.Server{}, err + return mcp.Server{}, skip, err } for _, server := range servers { if server.Name != serverName { continue } if !strings.EqualFold(server.Auth, mcp.ServerAuthOAuth) { - return mcp.Server{}, fmt.Errorf("MCP server %q does not declare auth: \"oauth\"", serverName) + return mcp.Server{}, skip, fmt.Errorf("MCP server %q does not declare auth: \"oauth\"", serverName) } - return server, nil + return server, skip, nil } - return mcp.Server{}, fmt.Errorf("MCP server %q is not configured", serverName) + return mcp.Server{}, skip, fmt.Errorf("MCP server %q is not configured", serverName) } func oauthConfigForServer(server mcp.Server) mcp.OAuthConfig { diff --git a/internal/cli/mcp_oauth_test.go b/internal/cli/mcp_oauth_test.go index 6310bb074..66106363b 100644 --- a/internal/cli/mcp_oauth_test.go +++ b/internal/cli/mcp_oauth_test.go @@ -6,14 +6,17 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" "path/filepath" "strings" "sync" + "sync/atomic" "testing" "time" "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/workspacetrust" ) // syncBuffer is a goroutine-safe writer used when a background goroutine reads @@ -122,7 +125,7 @@ func TestRunMCPOAuthLoginStoresTokens(t *testing.T) { deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, newMCPTokenStore: func() (*mcp.TokenStore, error) { return store, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "remote": { Type: "http", @@ -194,7 +197,7 @@ func TestRunMCPOAuthLoginRejectsNonOAuthServer(t *testing.T) { deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, newMCPTokenStore: func() (*mcp.TokenStore, error) { return store, nil }, - resolveMCPConfig: func(workspaceRoot string) (config.MCPConfig, error) { + resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "plain": {Type: "http", URL: "https://plain.invalid/mcp"}, }}, nil @@ -210,6 +213,285 @@ func TestRunMCPOAuthLoginRejectsNonOAuthServer(t *testing.T) { } } +// TestRunMCPOAuthLoginGatedInUntrustedWorkspace is the load-bearing security test for +// the OAuth login gate: an OAuth MCP server defined ONLY in an untrusted workspace's +// ./.zero/config.json must be dropped before login. `zero mcp oauth login` must refuse +// with "not configured", surface the trust notice, and store NO token — so the +// discovery/registration/token-exchange flow never fires against the project URL. If the +// gate is reverted to resolveMCPConfig(cwd, false), the server survives, login proceeds, +// a token is stored, and every assertion below fails. +func TestRunMCPOAuthLoginGatedInUntrustedWorkspace(t *testing.T) { + setTrustConfigRoot(t) // isolates the trust store; the workspace stays untrusted + // A live server standing in for the untrusted project's OAuth server. A gated login + // must never reach it, so hits must stay 0 — a direct assertion of no outbound I/O + // (discovery/registration/token exchange) against a cloned repo's configured URL. If + // the gate leaks, mcp.Login contacts this URL and the hit count rises. + var hits atomic.Int64 + oauthSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.WriteHeader(http.StatusNotFound) + })) + defer oauthSrv.Close() + + cwd := t.TempDir() + // A real project MCP config on disk so projectMCPConfigExists() is true and the + // notice fires. resolveMCPConfig is faked, but the notice detector reads disk. + if err := os.MkdirAll(filepath.Join(cwd, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + body := `{"mcp":{"servers":{"remote":{"type":"http","url":"` + oauthSrv.URL + `","auth":"oauth"}}}}` + if err := os.WriteFile(filepath.Join(cwd, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + store, err := mcp.NewTokenStore(mcp.TokenStoreOptions{ + FilePath: filepath.Join(t.TempDir(), "mcp-oauth-tokens.json"), + }) + if err != nil { + t.Fatalf("NewTokenStore() error = %v", err) + } + // Points every OAuth endpoint at the live server: if the gate leaks and login + // proceeds, the flow contacts oauthSrv and hits.Load() becomes non-zero. + projectServer := config.MCPServerConfig{ + Type: "http", + URL: oauthSrv.URL, + Auth: "oauth", + OAuth: &config.MCPOAuthConfig{ + ClientID: "client-123", + AuthorizationEndpoint: oauthSrv.URL + "/authorize", + TokenEndpoint: oauthSrv.URL + "/token", + Scopes: []string{"read"}, + }, + } + deps := appDeps{ + getwd: func() (string, error) { return cwd, nil }, + newMCPTokenStore: func() (*mcp.TokenStore, error) { return store, nil }, + resolveMCPConfig: func(_ string, excludeProject bool) (config.MCPConfig, error) { + servers := map[string]config.MCPServerConfig{} + if !excludeProject { + servers["remote"] = projectServer + } + return config.MCPConfig{Servers: servers}, nil + }, + now: time.Now, + } + + // The gate must short-circuit BEFORE the interactive OAuth flow. Bound the run so a + // regression that drops the gate (login proceeds and blocks on the loopback callback) + // fails here in 10s instead of hanging until the package test timeout. + stdout := &syncBuffer{} + stderr := &syncBuffer{} + done := make(chan int, 1) + go func() { + done <- runWithDeps([]string{"mcp", "oauth", "login", "remote"}, stdout, stderr, deps) + }() + var exitCode int + select { + case exitCode = <-done: + case <-time.After(10 * time.Second): + t.Fatalf("login was not gated; it blocked on the OAuth flow — the untrusted gate is missing. stderr=%q", stderr.String()) + } + if exitCode == exitSuccess { + t.Fatalf("login on a project-only OAuth server in an untrusted workspace must fail; stdout=%q stderr=%q", stdout.String(), stderr.String()) + } + if !strings.Contains(stderr.String(), "not configured") { + t.Fatalf("stderr must report the server as not configured, got %q", stderr.String()) + } + if !strings.Contains(stderr.String(), "MCP servers") || !strings.Contains(stderr.String(), "zero trust") { + t.Fatalf("stderr must surface the workspace-trust notice, got %q", stderr.String()) + } + if _, ok, _ := store.Load("remote"); ok { + t.Fatal("no token must be stored when login is gated") + } + if got := hits.Load(); got != 0 { + t.Fatalf("gated login must never contact the project OAuth server URL, got %d hit(s)", got) + } +} + +// TestResolveOAuthServerTrustedWorkspaceReturnsServer proves the gate does not +// over-block: in a TRUSTED workspace the project OAuth server resolves normally with a +// clean skip, so login proceeds as before. Calls resolveOAuthServer directly to avoid +// driving the full interactive login flow. +func TestResolveOAuthServerTrustedWorkspaceReturnsServer(t *testing.T) { + setTrustConfigRoot(t) + cwd := t.TempDir() + if err := workspacetrust.Trust(cwd); err != nil { + t.Fatalf("Trust(cwd): %v", err) + } + var gotExclude bool + deps := appDeps{ + getwd: func() (string, error) { return cwd, nil }, + resolveMCPConfig: func(_ string, excludeProject bool) (config.MCPConfig, error) { + gotExclude = excludeProject + servers := map[string]config.MCPServerConfig{} + if !excludeProject { + servers["remote"] = oauthTestServerConfig() + } + return config.MCPConfig{Servers: servers}, nil + }, + } + + server, skip, err := resolveOAuthServer(deps, "remote") + if err != nil { + t.Fatalf("trusted workspace must resolve the project OAuth server, got err %v", err) + } + if gotExclude { + t.Fatalf("trusted workspace must resolve MCP config with excludeProject=false") + } + if server.Name != "remote" { + t.Fatalf("server.Name = %q, want remote", server.Name) + } + if skip.excludedProjectConfig { + t.Fatalf("trusted workspace must not report an excluded project config") + } +} + +// TestResolveOAuthServerGatedInUntrustedWorkspace is the fast load-bearing guard for the +// gate: in an untrusted workspace a project-only OAuth server is dropped, so resolution +// fails with "not configured" and flags the excluded project config for the notice. +// Reverting the gate to resolveMCPConfig(cwd, false) makes the server resolve and this +// test fails immediately (no 10s login-flow timeout needed). +func TestResolveOAuthServerGatedInUntrustedWorkspace(t *testing.T) { + setTrustConfigRoot(t) + cwd := t.TempDir() + if err := os.MkdirAll(filepath.Join(cwd, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + body := `{"mcp":{"servers":{"remote":{"type":"http","url":"https://remote.invalid/mcp","auth":"oauth"}}}}` + if err := os.WriteFile(filepath.Join(cwd, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + deps := appDeps{ + getwd: func() (string, error) { return cwd, nil }, + resolveMCPConfig: func(_ string, excludeProject bool) (config.MCPConfig, error) { + servers := map[string]config.MCPServerConfig{} + if !excludeProject { + servers["remote"] = oauthTestServerConfig() + } + return config.MCPConfig{Servers: servers}, nil + }, + } + + _, skip, err := resolveOAuthServer(deps, "remote") + if err == nil { + t.Fatal("untrusted workspace must not resolve a project-only OAuth server") + } + if !strings.Contains(err.Error(), "not configured") { + t.Fatalf("want not-configured error, got %v", err) + } + if !skip.excludedProjectConfig { + t.Fatal("a dropped project OAuth config must flag excludedProjectConfig for the notice") + } + if skip.trustCheckErrored { + t.Fatal("a clean untrusted verdict is not a store-read error") + } +} + +func oauthTestServerConfig() config.MCPServerConfig { + return config.MCPServerConfig{ + Type: "http", + URL: "https://remote.invalid/mcp", + Auth: "oauth", + OAuth: &config.MCPOAuthConfig{ + ClientID: "client-123", + AuthorizationEndpoint: "https://remote.invalid/authorize", + TokenEndpoint: "https://remote.invalid/token", + Scopes: []string{"read"}, + }, + } +} + +// TestRunMCPOAuthLoginTrustedProjectServerStoresToken proves the gate does not over-block +// end-to-end: in a TRUSTED workspace, a project-only OAuth server (returned by the fake +// ONLY when excludeProject is false) is resolved, the full login flow runs, and the token +// is persisted with exitSuccess. This exercises the trusted-resolve + login-success path +// as one execution; if trust were dropped, excludeProject would be true, the fake would +// drop the server, and login would fail with "not configured". +func TestRunMCPOAuthLoginTrustedProjectServerStoresToken(t *testing.T) { + setTrustConfigRoot(t) + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "access-final", + "refresh_token": "refresh-final", + "token_type": "Bearer", + "expires_in": 3600, + }) + })) + defer tokenServer.Close() + + cwd := t.TempDir() + if err := workspacetrust.Trust(cwd); err != nil { + t.Fatalf("Trust(cwd): %v", err) + } + store, err := mcp.NewTokenStore(mcp.TokenStoreOptions{ + FilePath: filepath.Join(t.TempDir(), "mcp-oauth-tokens.json"), + }) + if err != nil { + t.Fatalf("NewTokenStore() error = %v", err) + } + projectServer := config.MCPServerConfig{ + Type: "http", + URL: "https://remote.invalid/mcp", + Auth: "oauth", + OAuth: &config.MCPOAuthConfig{ + ClientID: "client-123", + AuthorizationEndpoint: "https://remote.invalid/authorize", + TokenEndpoint: tokenServer.URL, + Scopes: []string{"read"}, + }, + } + var gotExclude bool + deps := appDeps{ + getwd: func() (string, error) { return cwd, nil }, + newMCPTokenStore: func() (*mcp.TokenStore, error) { return store, nil }, + resolveMCPConfig: func(_ string, excludeProject bool) (config.MCPConfig, error) { + gotExclude = excludeProject + servers := map[string]config.MCPServerConfig{} + if !excludeProject { + servers["remote"] = projectServer + } + return config.MCPConfig{Servers: servers}, nil + }, + now: time.Now, + } + + stdout := &syncBuffer{} + stderr := &syncBuffer{} + go func() { + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + callbackURL := extractCallbackURL(stdout.String()) + if callbackURL != "" { + _, _ = http.Get(callbackURL) + return + } + time.Sleep(5 * time.Millisecond) + } + }() + done := make(chan int, 1) + go func() { + done <- runWithDeps([]string{"mcp", "oauth", "login", "remote"}, stdout, stderr, deps) + }() + var exitCode int + select { + case exitCode = <-done: + case <-time.After(10 * time.Second): + t.Fatalf("trusted login did not complete within 10s; stderr=%s stdout=%s", stderr.String(), stdout.String()) + } + if exitCode != exitSuccess { + t.Fatalf("trusted login must succeed; exit=%d stderr=%s", exitCode, stderr.String()) + } + if gotExclude { + t.Fatal("trusted workspace must resolve MCP config with excludeProject=false") + } + token, ok, err := store.Load("remote") + if err != nil || !ok { + t.Fatalf("token must be stored on trusted login; ok=%v err=%v", ok, err) + } + if token.AccessToken != "access-final" { + t.Fatalf("stored token = %#v", token) + } +} + func TestRunMCPOAuthUnknownSubcommand(t *testing.T) { var stdout, stderr bytes.Buffer exitCode := runWithDeps([]string{"mcp", "oauth", "bogus"}, &stdout, &stderr, appDeps{}) diff --git a/internal/cli/mcp_tools.go b/internal/cli/mcp_tools.go index 25becdcf5..3f933e024 100644 --- a/internal/cli/mcp_tools.go +++ b/internal/cli/mcp_tools.go @@ -19,22 +19,38 @@ type mcpToolListItem struct { Permission string `json:"permission"` } -func registerMCPToolsForWorkspace(ctx context.Context, workspaceRoot string, registry *tools.Registry, deps appDeps, autonomy mcp.PermissionAutonomy) (mcpToolRuntime, error) { - cfg, err := deps.resolveMCPConfig(workspaceRoot) +// registerMCPToolsForWorkspace resolves and registers the workspace's MCP servers. +// This spawns stdio servers, so it gates the project config layer behind the +// workspace-trust check: trustRoot is the ORIGINAL launch directory (resolved before +// any --worktree reassignment) so a worktree of a trusted repo inherits that trust. +// resolveTrust fails closed, so an empty trustRoot or a store-read error excludes the +// project layer and a cloned repo cannot spawn its ./.zero/config.json MCP servers. +// +// It returns a trustSkip alongside the runtime so the caller can fold the MCP gate +// into the one-line trust notice (mirroring the hooks and plugins chokepoints); +// otherwise a workspace whose only project config is MCP would be gated silently. +func registerMCPToolsForWorkspace(ctx context.Context, workspaceRoot string, registry *tools.Registry, deps appDeps, autonomy mcp.PermissionAutonomy, trustRoot string) (mcpToolRuntime, trustSkip, error) { + excludeProject, trustCheckErrored := resolveTrust(trustRoot) + skip := trustSkip{ + excludedProjectConfig: excludeProject && projectMCPConfigExists(workspaceRoot), + trustCheckErrored: trustCheckErrored, + } + cfg, err := deps.resolveMCPConfig(workspaceRoot, excludeProject) if err != nil { - return nil, err + return nil, skip, err } if len(cfg.Servers) == 0 { - return noopMCPRuntime{}, nil + return noopMCPRuntime{}, skip, nil } store, err := deps.newMCPStore() if err != nil { - return nil, err + return nil, skip, err } - return deps.registerMCPTools(ctx, registry, cfg, mcp.RegisterOptions{ + runtime, err := deps.registerMCPTools(ctx, registry, cfg, mcp.RegisterOptions{ PermissionStore: store, Autonomy: autonomy, }) + return runtime, skip, err } func execMCPAutonomy(options execOptions) mcp.PermissionAutonomy { diff --git a/internal/cli/mcp_trust_edge_test.go b/internal/cli/mcp_trust_edge_test.go new file mode 100644 index 000000000..e2c6960c7 --- /dev/null +++ b/internal/cli/mcp_trust_edge_test.go @@ -0,0 +1,221 @@ +package cli + +// Edge-case coverage for the MCP trust gate on the oauth-login and check paths: +// the error-return contract of the gated resolveOAuthServer, the trust-store-read-error +// notice variant, and the accepted (advisory) notice over-emission on the not-oauth path. + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// writeProjectMCPConfig drops a ./.zero/config.json under dir declaring one MCP server, +// so projectMCPConfigExists() reads it as present. +func writeProjectMCPConfig(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(dir, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + body := `{"mcp":{"servers":{"proj":{"type":"stdio","command":"proj-cmd"}}}}` + if err := os.WriteFile(filepath.Join(dir, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} + +// breakTrustStore makes the trust store unreadable (creates trust.json as a directory), +// so workspacetrust.IsTrusted returns an error and resolveTrust fails closed with +// trustCheckErrored=true. +func breakTrustStore(t *testing.T) { + t.Helper() + configRoot := setTrustConfigRoot(t) + if err := os.MkdirAll(filepath.Join(configRoot, "zero", "trust.json"), 0o700); err != nil { + t.Fatal(err) + } +} + +// TestResolveOAuthServerValidationErrorReturnsZeroSkip locks the new 3-value contract on +// the pre-resolve error path: an invalid server name errors before trust is resolved, so +// the returned skip is zero (nothing was excluded yet). +func TestResolveOAuthServerValidationErrorReturnsZeroSkip(t *testing.T) { + deps := appDeps{getwd: func() (string, error) { return t.TempDir(), nil }} + _, skip, err := resolveOAuthServer(deps, "bad name with spaces") + if err == nil { + t.Fatal("invalid server name must error") + } + if skip.excludedProjectConfig || skip.trustCheckErrored { + t.Fatalf("a validation error must return a zero skip, got %+v", skip) + } +} + +// TestResolveOAuthServerGetwdErrorPropagates covers the getwd error path of the gated +// resolver. +func TestResolveOAuthServerGetwdErrorPropagates(t *testing.T) { + deps := appDeps{getwd: func() (string, error) { return "", errors.New("boom") }} + _, _, err := resolveOAuthServer(deps, "remote") + if err == nil || !strings.Contains(err.Error(), "failed to resolve workspace") { + t.Fatalf("getwd error must propagate, got %v", err) + } +} + +// TestRunMCPOAuthLoginStoreErrorNotice proves the store-read-error branch: when the trust +// store cannot be read, login fails closed AND the notice names the store error (the +// "could not be read" variant) rather than the plain untrusted wording. +func TestRunMCPOAuthLoginStoreErrorNotice(t *testing.T) { + breakTrustStore(t) + cwd := t.TempDir() + writeProjectMCPConfig(t, cwd) + deps := appDeps{ + getwd: func() (string, error) { return cwd, nil }, + resolveMCPConfig: func(_ string, excludeProject bool) (config.MCPConfig, error) { + servers := map[string]config.MCPServerConfig{} + if !excludeProject { + servers["proj"] = config.MCPServerConfig{Type: "http", URL: "https://x.invalid", Auth: "oauth"} + } + return config.MCPConfig{Servers: servers}, nil + }, + } + var out, errBuf bytes.Buffer + if code := runWithDeps([]string{"mcp", "oauth", "login", "proj"}, &out, &errBuf, deps); code == exitSuccess { + t.Fatal("login must fail closed on a trust-store read error") + } + if got := errBuf.String(); !strings.Contains(got, "could not be read") || !strings.Contains(got, "not configured") { + t.Fatalf("store-error login must emit the store-errored notice AND not-configured, got %q", got) + } +} + +// TestRunMCPCheckStoreErrorNotice is the mcp-check sibling of the store-error case. +func TestRunMCPCheckStoreErrorNotice(t *testing.T) { + breakTrustStore(t) + cwd := t.TempDir() + writeProjectMCPConfig(t, cwd) + deps := appDeps{ + getwd: func() (string, error) { return cwd, nil }, + resolveMCPConfig: func(_ string, excludeProject bool) (config.MCPConfig, error) { + servers := map[string]config.MCPServerConfig{} + if !excludeProject { + servers["proj"] = config.MCPServerConfig{Type: "stdio", Command: "proj-cmd"} + } + return config.MCPConfig{Servers: servers}, nil + }, + } + var out, errBuf bytes.Buffer + if code := runWithDeps([]string{"mcp", "check", "proj"}, &out, &errBuf, deps); code == exitSuccess { + t.Fatal("mcp check must fail closed on a trust-store read error") + } + if got := errBuf.String(); !strings.Contains(got, "could not be read") { + t.Fatalf("mcp check store-error must emit the store-errored notice, got %q", got) + } +} + +// TestRunMCPOAuthLoginNoticeCoFiresWithNonOAuthError documents accepted behavior: in an +// untrusted workspace with project MCP config on disk, a login that fails because the +// named USER-config server is not OAuth still co-emits the trust notice (project MCP was +// excluded this run, which is true). The notice is advisory and keyed on "project config +// was dropped", not on "this server was the dropped one" — the same coarse scoping the +// mcp-check path uses. If this behavior changes intentionally, update this test. +func TestRunMCPOAuthLoginNoticeCoFiresWithNonOAuthError(t *testing.T) { + setTrustConfigRoot(t) // untrusted + cwd := t.TempDir() + writeProjectMCPConfig(t, cwd) + deps := appDeps{ + getwd: func() (string, error) { return cwd, nil }, + // "foo" is a user-config server (survives the gate) that does not declare oauth. + resolveMCPConfig: func(_ string, _ bool) (config.MCPConfig, error) { + return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "foo": {Type: "http", URL: "https://foo.invalid"}, + }}, nil + }, + } + var out, errBuf bytes.Buffer + if code := runWithDeps([]string{"mcp", "oauth", "login", "foo"}, &out, &errBuf, deps); code == exitSuccess { + t.Fatal("login on a non-oauth server must fail") + } + got := errBuf.String() + if !strings.Contains(got, "oauth") { + t.Fatalf("want the not-oauth error, got %q", got) + } + if !strings.Contains(got, "zero trust") { + t.Fatalf("expected the advisory trust notice to co-fire (accepted behavior), got %q", got) + } +} + +// TestResolveOAuthServerResolveConfigErrorPropagates covers the resolveMCPConfig-error +// return of the gated resolver: the error propagates with the computed skip. +func TestResolveOAuthServerResolveConfigErrorPropagates(t *testing.T) { + setTrustConfigRoot(t) + deps := appDeps{ + getwd: func() (string, error) { return t.TempDir(), nil }, + resolveMCPConfig: func(_ string, _ bool) (config.MCPConfig, error) { + return config.MCPConfig{}, errors.New("resolve boom") + }, + } + _, _, err := resolveOAuthServer(deps, "remote") + if err == nil || !strings.Contains(err.Error(), "resolve boom") { + t.Fatalf("resolveMCPConfig error must propagate, got %v", err) + } +} + +// TestResolveOAuthServerNormalizeErrorPropagates covers the NormalizeConfig-error return: +// a malformed server (stdio with no command) fails normalization. +func TestResolveOAuthServerNormalizeErrorPropagates(t *testing.T) { + setTrustConfigRoot(t) + deps := appDeps{ + getwd: func() (string, error) { return t.TempDir(), nil }, + resolveMCPConfig: func(_ string, _ bool) (config.MCPConfig, error) { + return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "remote": {Type: "stdio"}, // no command -> NormalizeConfig rejects + }}, nil + }, + } + _, _, err := resolveOAuthServer(deps, "remote") + if err == nil || !strings.Contains(err.Error(), "requires command") { + t.Fatalf("NormalizeConfig error must propagate, got %v", err) + } +} + +// TestRunMCPCheckDisabledServer covers the found-but-disabled branch of runMCPCheck. +func TestRunMCPCheckDisabledServer(t *testing.T) { + setTrustConfigRoot(t) + deps := appDeps{ + getwd: func() (string, error) { return t.TempDir(), nil }, + resolveMCPConfig: func(_ string, _ bool) (config.MCPConfig, error) { + return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "proj": {Type: "stdio", Command: "c", Disabled: true}, + }}, nil + }, + } + var out, errBuf bytes.Buffer + if code := runWithDeps([]string{"mcp", "check", "proj"}, &out, &errBuf, deps); code == exitSuccess { + t.Fatal("mcp check on a disabled server must fail") + } + if got := errBuf.String(); !strings.Contains(got, "is disabled") { + t.Fatalf("want disabled error, got %q", got) + } +} + +// TestRunMCPCheckNormalizeError covers the found-but-malformed branch of runMCPCheck: the +// scoped NormalizeConfig fails before any spawn. +func TestRunMCPCheckNormalizeError(t *testing.T) { + setTrustConfigRoot(t) + deps := appDeps{ + getwd: func() (string, error) { return t.TempDir(), nil }, + resolveMCPConfig: func(_ string, _ bool) (config.MCPConfig, error) { + return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "proj": {Type: "stdio"}, // no command + }}, nil + }, + } + var out, errBuf bytes.Buffer + if code := runWithDeps([]string{"mcp", "check", "proj"}, &out, &errBuf, deps); code == exitSuccess { + t.Fatal("mcp check on a malformed server must fail") + } + if got := errBuf.String(); !strings.Contains(got, "requires command") { + t.Fatalf("want normalize error, got %q", got) + } +} diff --git a/internal/cli/mcp_trust_test.go b/internal/cli/mcp_trust_test.go new file mode 100644 index 000000000..9bfb8c1ee --- /dev/null +++ b/internal/cli/mcp_trust_test.go @@ -0,0 +1,358 @@ +package cli + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/workspacetrust" +) + +// mcpTrustDeps builds an appDeps whose resolveMCPConfig HONORS excludeProject: it +// returns a project stdio server only when excludeProject is false, mirroring the +// real ResolveMCP gate. It records the excludeProject value it was called with and +// whether registerMCPTools (which spawns servers) actually fired. +func mcpTrustDeps(gotExclude *bool, spawned *bool) appDeps { + return appDeps{ + resolveMCPConfig: func(_ string, excludeProject bool) (config.MCPConfig, error) { + *gotExclude = excludeProject + servers := map[string]config.MCPServerConfig{} + if !excludeProject { + servers["proj-srv"] = config.MCPServerConfig{Type: "stdio", Command: "proj-cmd"} + } + return config.MCPConfig{Servers: servers}, nil + }, + newMCPStore: func() (*mcp.PermissionStore, error) { return nil, nil }, + registerMCPTools: func(_ context.Context, _ *tools.Registry, _ config.MCPConfig, _ mcp.RegisterOptions) (mcpToolRuntime, error) { + *spawned = true + return closeFunc(func() error { return nil }), nil + }, + } +} + +// TestMCPGateUntrustedExcludesProjectServer proves the P0 fix: an untrusted trustRoot +// makes registerMCPToolsForWorkspace resolve MCP config with excludeProject=true and, +// because that drops the project server, it never spawns anything. This test is +// load-bearing: if the gate is removed (a hardcoded excludeProject=false passed to +// resolveMCPConfig), gotExclude is false, the project server survives, spawned flips +// true, and every assertion below fails. +func TestMCPGateUntrustedExcludesProjectServer(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() // never trusted + + var gotExclude, spawned bool + deps := mcpTrustDeps(&gotExclude, &spawned) + registry := tools.NewRegistry() + + runtime, skip, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, repo) + if err != nil { + t.Fatalf("registerMCPToolsForWorkspace: %v", err) + } + defer func() { _ = runtime.Close() }() + + if !gotExclude { + t.Fatalf("untrusted workspace must resolve MCP config with excludeProject=true") + } + if spawned { + t.Fatalf("untrusted workspace must not spawn the project MCP server") + } + if _, ok := runtime.(noopMCPRuntime); !ok { + t.Fatalf("with the project server dropped, the runtime should be the noop runtime, got %T", runtime) + } + // This repo has no ./.zero/config.json, so there is no project MCP config to + // notice about even though it is untrusted; the skip must stay clean. + if skip.excludedProjectConfig { + t.Fatalf("no project MCP config on disk, so the skip must not flag an excluded config") + } + if skip.trustCheckErrored { + t.Fatalf("a clean untrusted verdict is not a store-read error") + } +} + +// TestMCPGateEmptyTrustRootFailsClosed proves fail-closed-by-construction: a caller +// that forgot to resolve trustRoot (empty) still excludes the project layer. +func TestMCPGateEmptyTrustRootFailsClosed(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + // Even trusting the repo must not help when the caller passes an empty root. + if err := workspacetrust.Trust(repo); err != nil { + t.Fatalf("Trust(repo): %v", err) + } + + var gotExclude, spawned bool + deps := mcpTrustDeps(&gotExclude, &spawned) + registry := tools.NewRegistry() + + runtime, skip, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, "") + if err != nil { + t.Fatalf("registerMCPToolsForWorkspace: %v", err) + } + defer func() { _ = runtime.Close() }() + + if !gotExclude { + t.Fatalf("empty trustRoot must fail closed (excludeProject=true)") + } + if spawned { + t.Fatalf("empty trustRoot must not spawn the project MCP server") + } + // Empty trustRoot is a clean fail-closed verdict, not a store-read error. + if skip.trustCheckErrored { + t.Fatalf("empty trustRoot is not a store-read error") + } +} + +// TestMCPGateTrustedSpawnsProjectServer proves R3 for MCP: after Trust(repo) the +// project layer is included (excludeProject=false) and the project server spawns. +func TestMCPGateTrustedSpawnsProjectServer(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + if err := workspacetrust.Trust(repo); err != nil { + t.Fatalf("Trust(repo): %v", err) + } + + var gotExclude, spawned bool + deps := mcpTrustDeps(&gotExclude, &spawned) + registry := tools.NewRegistry() + + runtime, skip, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, repo) + if err != nil { + t.Fatalf("registerMCPToolsForWorkspace: %v", err) + } + defer func() { _ = runtime.Close() }() + + if gotExclude { + t.Fatalf("trusted workspace must resolve MCP config with excludeProject=false") + } + if !spawned { + t.Fatalf("trusted workspace must spawn the project MCP server") + } + if skip.excludedProjectConfig { + t.Fatalf("trusted workspace must not report the project MCP layer excluded") + } +} + +// TestMCPToolsListSurfacesTrustNotice proves `zero mcp tools list` no longer drops the +// project MCP layer silently in an untrusted workspace: the gated skip is surfaced on +// stderr (the list stays on stdout), so an empty list is explained rather than read as +// "nothing configured". Trusting the repo silences it. +func TestMCPToolsListSurfacesTrustNotice(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + if err := os.MkdirAll(filepath.Join(repo, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + body := `{"mcp":{"servers":{"proj":{"type":"stdio","command":"proj-cmd"}}}}` + if err := os.WriteFile(filepath.Join(repo, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + run := func() string { + var out, errBuf bytes.Buffer + deps := appDeps{ + getwd: func() (string, error) { return repo, nil }, + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{}, nil }, + } + if code := runWithDeps([]string{"mcp", "tools", "list"}, &out, &errBuf, deps); code != exitSuccess { + t.Fatalf("mcp tools list exit = %d, stderr=%q", code, errBuf.String()) + } + return errBuf.String() + } + + // Untrusted: the gated project MCP layer must be explained on stderr. + if errUntrusted := run(); !strings.Contains(errUntrusted, "MCP servers") || !strings.Contains(errUntrusted, "zero trust") { + t.Fatalf("untrusted `mcp tools list` must surface the trust notice on stderr, got %q", errUntrusted) + } + + // Trusted: nothing is skipped, so no notice. + if err := workspacetrust.Trust(repo); err != nil { + t.Fatal(err) + } + if errTrusted := run(); strings.Contains(errTrusted, "ignoring project") { + t.Fatalf("trusted `mcp tools list` must not emit a trust notice, got %q", errTrusted) + } +} + +// TestProjectMCPConfigExists exercises every branch of the notice-gating detector: +// only a ./.zero/config.json that parses AND declares at least one server is true. +func TestProjectMCPConfigExists(t *testing.T) { + writeCfg := func(t *testing.T, dir, body string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(dir, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + if projectMCPConfigExists("") { + t.Fatal("empty workspace root must be false") + } + cases := []struct { + name string + body string // "" means write no config.json at all + want bool + }{ + {"no file", "", false}, + {"declares a server", `{"mcp":{"servers":{"a":{"type":"stdio","command":"x"}}}}`, true}, + {"empty servers map", `{"mcp":{"servers":{}}}`, false}, + {"config without mcp key", `{"model":"x"}`, false}, + {"unparseable json", `{not valid`, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + if tc.body != "" { + writeCfg(t, dir, tc.body) + } + if got := projectMCPConfigExists(dir); got != tc.want { + t.Fatalf("projectMCPConfigExists = %v, want %v", got, tc.want) + } + }) + } +} + +// TestMCPGateFailClosedOnStoreError proves the MCP surface reports a store-read error +// (trust.json created as a directory) as trustCheckErrored, so the caller's notice can +// name the fail-closed reason -- the same error path the hook and plugin gates cover. +func TestMCPGateFailClosedOnStoreError(t *testing.T) { + configRoot := setTrustConfigRoot(t) + trustPath := filepath.Join(configRoot, "zero", "trust.json") + if err := os.MkdirAll(trustPath, 0o700); err != nil { + t.Fatalf("create trust.json as a directory: %v", err) + } + repo := t.TempDir() + if err := os.MkdirAll(filepath.Join(repo, ".zero"), 0o700); err != nil { + t.Fatalf("mkdir project .zero: %v", err) + } + body := `{"mcp":{"servers":{"proj":{"type":"stdio","command":"proj-cmd"}}}}` + if err := os.WriteFile(filepath.Join(repo, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatalf("write project config.json: %v", err) + } + + var gotExclude, spawned bool + deps := mcpTrustDeps(&gotExclude, &spawned) + runtime, skip, err := registerMCPToolsForWorkspace(context.Background(), repo, tools.NewRegistry(), deps, mcp.AutonomyLow, repo) + if err != nil { + t.Fatalf("registerMCPToolsForWorkspace: %v", err) + } + defer func() { _ = runtime.Close() }() + if !gotExclude { + t.Fatalf("a store-read error must fail closed (excludeProject=true)") + } + if spawned { + t.Fatalf("a store-read error must not spawn the project MCP server") + } + if !skip.trustCheckErrored { + t.Fatalf("skip must mark the store-read error so the notice can name it") + } + if !skip.excludedProjectConfig { + t.Fatalf("the project MCP layer must be reported excluded on the error path") + } +} + +// TestMCPGateUntrustedNoticesProjectMCPConfig proves the notice-surfacing fix: when an +// untrusted workspace actually declares project MCP servers in ./.zero/config.json, +// registerMCPToolsForWorkspace reports excludedProjectConfig=true so the caller can +// warn (the CodeRabbit finding: project MCP was gated silently). Trusting the repo +// clears the skip. +func TestMCPGateUntrustedNoticesProjectMCPConfig(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + // A real project MCP config on disk: projectMCPConfigExists reads this file. + if err := os.MkdirAll(filepath.Join(repo, ".zero"), 0o700); err != nil { + t.Fatalf("mkdir project .zero: %v", err) + } + body := `{"mcp":{"servers":{"proj":{"type":"stdio","command":"proj-cmd"}}}}` + if err := os.WriteFile(filepath.Join(repo, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatalf("write project config.json: %v", err) + } + + var gotExclude, spawned bool + deps := mcpTrustDeps(&gotExclude, &spawned) + registry := tools.NewRegistry() + + // Untrusted: the project MCP layer is dropped AND flagged for the notice. + runtime, skip, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, repo) + if err != nil { + t.Fatalf("registerMCPToolsForWorkspace (untrusted): %v", err) + } + defer func() { _ = runtime.Close() }() + if !skip.excludedProjectConfig { + t.Fatalf("untrusted workspace with project MCP config must flag excludedProjectConfig for the notice") + } + if skip.trustCheckErrored { + t.Fatalf("a clean untrusted verdict is not a store-read error") + } + + // Trusted: nothing is skipped, so no notice. + if err := workspacetrust.Trust(repo); err != nil { + t.Fatalf("Trust(repo): %v", err) + } + _, trustedSkip, err := registerMCPToolsForWorkspace(context.Background(), repo, tools.NewRegistry(), deps, mcp.AutonomyLow, repo) + if err != nil { + t.Fatalf("registerMCPToolsForWorkspace (trusted): %v", err) + } + if trustedSkip.excludedProjectConfig { + t.Fatalf("trusted workspace must not flag an excluded project MCP layer") + } +} + +// TestMCPCheckSurfacesTrustNotice proves the `zero mcp check` notice fix: in an untrusted +// workspace whose only definition of a server lives in ./.zero/config.json, the gate +// drops the server and the command must emit the one-line trust notice before the +// "not configured" error, instead of a bare miss that hides the trust exclusion. The R4 +// half proves the notice self-gates: a genuinely-absent server in a workspace with no +// project config on disk prints "not configured" with no notice. +func TestMCPCheckSurfacesTrustNotice(t *testing.T) { + setTrustConfigRoot(t) + + // resolveMCPConfig fake that HONORS excludeProject: `proj` survives only when the + // project layer is included, mirroring the real gate. + dropFake := func(_ string, excludeProject bool) (config.MCPConfig, error) { + servers := map[string]config.MCPServerConfig{} + if !excludeProject { + servers["proj"] = config.MCPServerConfig{Type: "stdio", Command: "proj-cmd"} + } + return config.MCPConfig{Servers: servers}, nil + } + + // Untrusted workspace whose ONLY definition of `proj` is project config on disk. + repo := t.TempDir() + if err := os.MkdirAll(filepath.Join(repo, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + body := `{"mcp":{"servers":{"proj":{"type":"stdio","command":"proj-cmd"}}}}` + if err := os.WriteFile(filepath.Join(repo, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + var out, errBuf bytes.Buffer + deps := appDeps{getwd: func() (string, error) { return repo, nil }, resolveMCPConfig: dropFake} + if code := runWithDeps([]string{"mcp", "check", "proj"}, &out, &errBuf, deps); code == exitSuccess { + t.Fatalf("mcp check on a gated project server must fail, got success; stderr=%q", errBuf.String()) + } + if got := errBuf.String(); !strings.Contains(got, "not configured") || + !strings.Contains(got, "MCP servers") || !strings.Contains(got, "zero trust") { + t.Fatalf("untrusted mcp check must report not-configured AND surface the trust notice, got %q", got) + } + + // R4: an untrusted workspace with NO project MCP config on disk has nothing to + // notice about, so an absent server prints "not configured" with no trust notice. + bare := t.TempDir() + var out2, errBuf2 bytes.Buffer + deps2 := appDeps{getwd: func() (string, error) { return bare, nil }, resolveMCPConfig: dropFake} + if code := runWithDeps([]string{"mcp", "check", "ghost"}, &out2, &errBuf2, deps2); code == exitSuccess { + t.Fatalf("mcp check on an absent server must fail") + } + if got := errBuf2.String(); !strings.Contains(got, "not configured") { + t.Fatalf("stderr must report not configured, got %q", got) + } + if got := errBuf2.String(); strings.Contains(got, "ignoring project") || strings.Contains(got, "zero trust") { + t.Fatalf("no project config on disk means no trust notice, got %q", got) + } +} diff --git a/internal/cli/plugin_activate.go b/internal/cli/plugin_activate.go index c802241c4..2215af4e6 100644 --- a/internal/cli/plugin_activate.go +++ b/internal/cli/plugin_activate.go @@ -3,6 +3,8 @@ package cli import ( "fmt" "io" + "os" + "path/filepath" "strings" "github.com/Gitlawb/zero/internal/agent" @@ -15,9 +17,13 @@ import ( // later dispatcher + skill wiring can consume it: the plugin hook definitions and // the plugin skill search roots. The zero value (no plugins) is inert — the // dispatcher gets no extra hooks and the skill tool keeps only the default dir. +// The embedded trustSkip reports whether the project plugin layer was dropped for +// an untrusted workspace, so the caller can fold it into the single combined +// trust notice. type pluginActivation struct { hooks []hooks.Definition skillRoots []string + trustSkip } // activatePlugins loads the workspace's plugins and makes their declared @@ -27,12 +33,26 @@ type pluginActivation struct { // // It fails OPEN: any load error (or a malformed plugin) is surfaced as a warning // on stderr and otherwise skipped, so a broken plugin can never wedge startup — -// mirroring how newHookDispatcher and skills.Load tolerate bad input. -func activatePlugins(workspaceRoot string, registry *tools.Registry, deps appDeps, stderr io.Writer) pluginActivation { - loaded, err := deps.loadPlugins(plugins.LoadOptions{Cwd: workspaceRoot}) +// mirroring how newHookDispatcher and skills.Load tolerate bad input. That +// fail-OPEN handling of MALFORMED plugins is separate from the trust gate below. +// +// The trust check lives here, inside the chokepoint, so no caller can bypass it: +// the project plugin root is dropped (ExcludeProject) whenever trustRoot is empty, +// the trust store cannot be read, or the workspace is not trusted (fail-closed). +// trustRoot is the original launch directory (resolved before any --worktree +// reassignment). The returned activation carries the skip report so the caller can +// emit one combined notice; the notice itself is NOT emitted here. +func activatePlugins(workspaceRoot string, registry *tools.Registry, deps appDeps, stderr io.Writer, trustRoot string) pluginActivation { + excludeProject, trustCheckErrored := resolveTrust(trustRoot) + skip := trustSkip{ + excludedProjectConfig: excludeProject && projectPluginsDirExists(workspaceRoot), + trustCheckErrored: trustCheckErrored, + } + + loaded, err := deps.loadPlugins(plugins.LoadOptions{Cwd: workspaceRoot, ExcludeProject: excludeProject}) if err != nil { writePluginActivationWarning(stderr, "failed to load plugins: "+err.Error()) - return pluginActivation{} + return pluginActivation{trustSkip: skip} } // Load fails OPEN per plugin: a malformed manifest is recorded as a diagnostic @@ -56,7 +76,18 @@ func activatePlugins(workspaceRoot string, registry *tools.Registry, deps appDep registry.Register(plugins.NewSkillTool(deps.skillsDir(), result.SkillRoots)) } - return pluginActivation{hooks: result.Hooks, skillRoots: result.SkillRoots} + return pluginActivation{hooks: result.Hooks, skillRoots: result.SkillRoots, trustSkip: skip} +} + +// projectPluginsDirExists reports whether a ./.zero/plugins directory is present +// under workspaceRoot, so the caller only notices about config it actually +// skipped. +func projectPluginsDirExists(workspaceRoot string) bool { + if workspaceRoot == "" { + return false + } + info, err := os.Stat(filepath.Join(workspaceRoot, ".zero", "plugins")) + return err == nil && info.IsDir() } // skillInfos resolves the reusable skills the model can load via the skill tool — diff --git a/internal/cli/plugin_activate_test.go b/internal/cli/plugin_activate_test.go index 02fe03f94..ff2e30abb 100644 --- a/internal/cli/plugin_activate_test.go +++ b/internal/cli/plugin_activate_test.go @@ -51,7 +51,8 @@ func TestActivatePluginsRegistersToolAndCollectsHooks(t *testing.T) { registry := tools.NewRegistry() var stderr bytes.Buffer - activation := activatePlugins(t.TempDir(), registry, fakePluginDeps(t, loaded), &stderr) + workspace := t.TempDir() + activation := activatePlugins(workspace, registry, fakePluginDeps(t, loaded), &stderr, workspace) if _, ok := registry.Get("demo_lookup"); !ok { t.Fatalf("plugin tool not registered into the bootstrap registry") @@ -90,7 +91,8 @@ func TestActivatePluginsRegistersPluginSkillTool(t *testing.T) { registry := tools.NewRegistry() registry.Register(tools.NewSkillTool(t.TempDir())) // core skill tool, like the real bootstrap var stderr bytes.Buffer - activation := activatePlugins(t.TempDir(), registry, fakePluginDeps(t, loaded), &stderr) + workspace := t.TempDir() + activation := activatePlugins(workspace, registry, fakePluginDeps(t, loaded), &stderr, workspace) if len(activation.skillRoots) != 1 { t.Fatalf("expected one plugin skill root, got %#v", activation.skillRoots) @@ -124,7 +126,8 @@ func TestActivatePluginsSurfacesLoadDiagnostics(t *testing.T) { } registry := tools.NewRegistry() var stderr bytes.Buffer - activatePlugins(t.TempDir(), registry, deps, &stderr) + workspace := t.TempDir() + activatePlugins(workspace, registry, deps, &stderr, workspace) if stderr.Len() == 0 { t.Fatal("a load diagnostic must be surfaced on stderr") @@ -143,7 +146,8 @@ func TestActivatePluginsFailsOpenOnLoadError(t *testing.T) { } registry := tools.NewRegistry() var stderr bytes.Buffer - activation := activatePlugins(t.TempDir(), registry, deps, &stderr) + workspace := t.TempDir() + activation := activatePlugins(workspace, registry, deps, &stderr, workspace) if len(activation.hooks) != 0 || len(activation.skillRoots) != 0 { t.Fatalf("a load error must yield an inert activation, got %#v", activation) @@ -173,7 +177,8 @@ func TestActivatePluginsWarnsOnMalformedPluginButKeepsGood(t *testing.T) { registry := tools.NewRegistry() var stderr bytes.Buffer - activatePlugins(t.TempDir(), registry, fakePluginDeps(t, []plugins.LoadedPlugin{bad, good}), &stderr) + workspace := t.TempDir() + activatePlugins(workspace, registry, fakePluginDeps(t, []plugins.LoadedPlugin{bad, good}), &stderr, workspace) if _, ok := registry.Get("good_tool"); !ok { t.Fatal("the good plugin tool must still register despite a malformed sibling") @@ -198,7 +203,7 @@ func TestNewHookDispatcherWithExtraFoldsPluginHooks(t *testing.T) { Enabled: true, }} - dispatcher := newHookDispatcherWithExtra(workspace, extra) + dispatcher, _ := newHookDispatcherWithExtra(workspace, extra, workspace) if dispatcher == nil { t.Fatal("dispatcher should never be nil for a clean workspace") } diff --git a/internal/cli/trust.go b/internal/cli/trust.go new file mode 100644 index 000000000..8b025ab65 --- /dev/null +++ b/internal/cli/trust.go @@ -0,0 +1,119 @@ +package cli + +import ( + "fmt" + "io" + + "github.com/Gitlawb/zero/internal/redaction" + "github.com/Gitlawb/zero/internal/workspacetrust" +) + +// runTrust implements `zero trust`, letting the user opt a workspace into running +// its project-scoped executable config (hooks, plugins, MCP servers). Trust is +// keyed on the exact normalized working directory, matching the exact-match trust +// model and cwd-relative project-config discovery. +// +// zero trust trust the current working directory +// zero trust list print the trusted roots, one per line +// zero trust remove [path] untrust the current directory, or a named path +func runTrust(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { + if len(args) == 0 { + return trustCurrentDir(stdout, stderr, deps) + } + switch args[0] { + case "list": + return trustList(stdout, stderr) + case "remove", "rm", "untrust": + return trustRemove(args[1:], stdout, stderr, deps) + case "-h", "--help", "help": + // Explicit help is a success path: write usage to stdout and exit 0, matching + // the other subcommands (mcp, sandbox, skills, cron, ...). Only the unknown- + // subcommand error path below writes usage to stderr with a usage exit code. + writeTrustUsage(stdout) + return exitSuccess + default: + if _, err := fmt.Fprintf(stderr, "zero trust: unknown subcommand %q\n\n", args[0]); err != nil { + return exitCrash + } + writeTrustUsage(stderr) + return exitUsage + } +} + +// trustCurrentDir trusts the exact current working directory. +func trustCurrentDir(stdout io.Writer, stderr io.Writer, deps appDeps) int { + cwd, err := deps.getwd() + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(fmt.Errorf("resolve workspace: %w", err), redaction.Options{}), exitCrash) + } + if err := workspacetrust.Trust(cwd); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + if _, err := fmt.Fprintf(stdout, "Trusted %s\n", cwd); err != nil { + return exitCrash + } + return exitSuccess +} + +// trustList prints each trusted root, one per line, or a friendly line when none +// are trusted. +func trustList(stdout io.Writer, stderr io.Writer) int { + roots, err := workspacetrust.List() + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + if len(roots) == 0 { + if _, err := fmt.Fprintln(stdout, "No trusted workspaces."); err != nil { + return exitCrash + } + return exitSuccess + } + for _, root := range roots { + if _, err := fmt.Fprintln(stdout, root); err != nil { + return exitCrash + } + } + return exitSuccess +} + +// trustRemove untrusts the current directory, or a named path when one is given. +// The path argument is passed to workspacetrust.Untrust verbatim, which normalizes +// it (filepath.Abs + filepath.EvalSymlinks) the same way the store does, so a +// relative or trailing-slash argument still matches the stored entry. +func trustRemove(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { + var target string + switch len(args) { + case 0: + cwd, err := deps.getwd() + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(fmt.Errorf("resolve workspace: %w", err), redaction.Options{}), exitCrash) + } + target = cwd + case 1: + target = args[0] + default: + if _, err := fmt.Fprintln(stderr, "usage: zero trust remove [path]"); err != nil { + return exitCrash + } + return exitUsage + } + if err := workspacetrust.Untrust(target); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + if _, err := fmt.Fprintf(stdout, "Untrusted %s\n", target); err != nil { + return exitCrash + } + return exitSuccess +} + +func writeTrustUsage(w io.Writer) { + _, _ = fmt.Fprint(w, `Usage: + zero trust Trust the current working directory + zero trust list List trusted workspace roots + zero trust remove [path] Untrust the current directory, or a named path + +Trust lets Zero run a workspace's project-scoped hooks, plugins, and MCP +servers (./.zero/hooks.json, ./.zero/plugins/, project MCP config). Trust is +exact per directory. +`) +} diff --git a/internal/cli/trust_e2e_test.go b/internal/cli/trust_e2e_test.go new file mode 100644 index 000000000..e44f3a3cf --- /dev/null +++ b/internal/cli/trust_e2e_test.go @@ -0,0 +1,363 @@ +package cli + +import ( + "bytes" + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/workspacetrust" + "github.com/Gitlawb/zero/internal/worktrees" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// These tests close the end-to-end gap the chokepoint unit tests leave open: +// they drive the REAL agent loop (and, for the worktree cases, the real exec +// entry point) with a fake provider that emits a tool call, so a project +// beforeTool hook actually fires (or is gated away) through production code, not +// a direct dispatcher call. + +// markerTool is a minimal, always-allowed tool the fake provider "calls" so the +// agent loop reaches dispatchBeforeTool (which only fires for a registered, +// permitted tool). Its own Run is a no-op; the observable effect is the project +// beforeTool hook the gate did or did not load. +type markerTool struct{} + +func (markerTool) Name() string { return "marker_tool" } +func (markerTool) Description() string { return "test-only no-op tool" } +func (markerTool) Parameters() tools.Schema { + return tools.Schema{Type: "object", AdditionalProperties: false} +} +func (markerTool) Safety() tools.Safety { return tools.Safety{Permission: tools.PermissionAllow} } +func (markerTool) Run(context.Context, map[string]any) tools.Result { + return tools.Result{Status: tools.StatusOK, Output: "ok"} +} + +// toolThenTextProvider calls toolName on the first turn, then answers with text so +// the loop terminates. It detects "first turn" by the absence of a prior tool +// result in the message history. +type toolThenTextProvider struct{ toolName string } + +func (p toolThenTextProvider) StreamCompletion(_ context.Context, req zeroruntime.CompletionRequest) (<-chan zeroruntime.StreamEvent, error) { + toolAlreadyCalled := false + for _, m := range req.Messages { + if m.Role == zeroruntime.MessageRoleTool { + toolAlreadyCalled = true + break + } + } + ch := make(chan zeroruntime.StreamEvent, 8) + go func() { + defer close(ch) + if toolAlreadyCalled { + ch <- zeroruntime.StreamEvent{Type: zeroruntime.StreamEventText, Content: "done"} + ch <- zeroruntime.StreamEvent{Type: zeroruntime.StreamEventDone} + return + } + ch <- zeroruntime.StreamEvent{Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "c1", ToolName: p.toolName} + ch <- zeroruntime.StreamEvent{Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "c1", ArgumentsFragment: `{"pattern":"*"}`} + ch <- zeroruntime.StreamEvent{Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "c1"} + ch <- zeroruntime.StreamEvent{Type: zeroruntime.StreamEventDone} + }() + return ch, nil +} + +// writeMarkerHook writes a ./.zero/hooks.json under dir whose enabled beforeTool +// hook touches markerPath when it fires. The hook shells out to /bin/sh, so the +// trusted-case assertion (the marker must appear) is meaningless on Windows, where +// that interpreter does not exist; skip there, matching writeMarkerHookScript. +func writeMarkerHook(t *testing.T, dir, markerPath string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("marker hook is POSIX-shell based (/bin/sh)") + } + if err := os.MkdirAll(filepath.Join(dir, ".zero"), 0o755); err != nil { + t.Fatal(err) + } + body := `{"enabled":true,"hooks":[{"id":"m","event":"beforeTool","command":"/bin/sh","args":["-c","touch '` + markerPath + `'"],"enabled":true}]}` + if err := os.WriteFile(filepath.Join(dir, ".zero", "hooks.json"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } +} + +// TestTrustGateBlocksToolHookThroughAgentRun closes gap #2: the gate blocks a +// real tool call's beforeTool hook through the production agent.Run loop, not +// just a direct dispatcher call. Untrusted => the project hook is not in the +// dispatcher, so the tool call fires nothing. Trusted => it fires. +func TestTrustGateBlocksToolHookThroughAgentRun(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + marker := filepath.Join(t.TempDir(), "marker") + writeMarkerHook(t, repo, marker) + + runOnce := func() { + reg := tools.NewRegistry() + reg.Register(markerTool{}) + disp, _ := newHookDispatcherWithExtra(repo, nil, repo) + if _, err := agent.Run(context.Background(), "go", toolThenTextProvider{toolName: "marker_tool"}, agent.Options{ + Registry: reg, + Hooks: disp, + PermissionMode: agent.PermissionModeUnsafe, + MaxTurns: 3, + }); err != nil { + t.Fatalf("agent.Run: %v", err) + } + } + + // Untrusted: gate excludes the project layer; the beforeTool hook must not run. + _ = os.Remove(marker) + runOnce() + if _, err := os.Stat(marker); err == nil { + t.Fatal("UNTRUSTED: project beforeTool hook ran through agent.Run (marker exists) -- gate failed OPEN") + } + + // Trusted: the hook is in the dispatcher; the tool call fires it. + if err := workspacetrust.Trust(repo); err != nil { + t.Fatal(err) + } + _ = os.Remove(marker) + runOnce() + if _, err := os.Stat(marker); err != nil { + t.Fatalf("TRUSTED: project beforeTool hook did NOT run (marker absent): %v", err) + } +} + +// TestTrustGateFiresInDefaultAutoMode is the in-scope proof for the security +// report: the beforeTool hook fires in the DEFAULT permission mode (auto), NOT +// only under --skip-permissions-unsafe. marker_tool has PermissionAllow safety, +// so effectivePermission grants it at loop.go without a prompt regardless of +// mode; dispatchBeforeTool then runs the project hook. This means the +// vulnerability is reachable in normal operation with the sandbox ON, so it is +// not the "requires the user to disable the sandbox" out-of-scope case. +// +// Trusted => the hook fires in auto mode without any unsafe flag; untrusted => +// the gate blocks it, also in auto mode. No OnPermissionRequest is wired: an +// auto-allowed tool needs no approval callback, which is the whole point. +func TestTrustGateFiresInDefaultAutoMode(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + marker := filepath.Join(t.TempDir(), "marker") + writeMarkerHook(t, repo, marker) + + runOnce := func(mode agent.PermissionMode) { + reg := tools.NewRegistry() + reg.Register(markerTool{}) + disp, _ := newHookDispatcherWithExtra(repo, nil, repo) + if _, err := agent.Run(context.Background(), "go", toolThenTextProvider{toolName: "marker_tool"}, agent.Options{ + Registry: reg, + Hooks: disp, + PermissionMode: mode, + MaxTurns: 3, + }); err != nil { + t.Fatalf("agent.Run (%s): %v", mode, err) + } + } + + // Untrusted, default auto mode: the gate excludes the project layer, so the + // hook must NOT fire even though the tool call is permitted. + _ = os.Remove(marker) + runOnce(agent.PermissionModeAuto) + if _, err := os.Stat(marker); err == nil { + t.Fatal("UNTRUSTED auto mode: project beforeTool hook ran -- gate failed OPEN in the default mode") + } + + // Trusted, default auto mode (sandbox on, NO --skip-permissions-unsafe): the + // auto-allowed tool is granted with no prompt and its beforeTool hook fires. + if err := workspacetrust.Trust(repo); err != nil { + t.Fatal(err) + } + _ = os.Remove(marker) + runOnce(agent.PermissionModeAuto) + if _, err := os.Stat(marker); err != nil { + t.Fatalf("TRUSTED auto mode: project beforeTool hook did NOT fire without an unsafe flag (marker absent): %v", err) + } + + // And it is not an auto-only quirk: ask mode (also sandboxed, non-unsafe) + // fires the same way for an auto-allowed tool. + _ = os.Remove(marker) + runOnce(agent.PermissionModeAsk) + if _, err := os.Stat(marker); err != nil { + t.Fatalf("TRUSTED ask mode: project beforeTool hook did NOT fire (marker absent): %v", err) + } +} + +// TestExecSurfacesMCPTrustNotice proves the exec path actually threads the MCP trust +// skip into the one-line notice (the CodeRabbit finding), not just that the unit +// pieces work in isolation: an untrusted repo whose only project config is MCP prints +// the notice through the real runExec wiring; trusting it silences it. resolveMCPConfig +// is stubbed to return no servers so nothing spawns -- the notice depends only on the +// trust verdict and the real ./.zero/config.json that projectMCPConfigExists reads. +func TestExecSurfacesMCPTrustNotice(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("exec fake-provider harness assumes a POSIX process environment") + } + setTrustConfigRoot(t) + repo := t.TempDir() + if err := os.MkdirAll(filepath.Join(repo, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + body := `{"mcp":{"servers":{"proj":{"type":"stdio","command":"proj-cmd"}}}}` + if err := os.WriteFile(filepath.Join(repo, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + run := func() string { + var out, errBuf bytes.Buffer + code := runWithDeps([]string{"exec", "--skip-permissions-unsafe", "--max-turns", "3", "go"}, &out, &errBuf, appDeps{ + getwd: func() (string, error) { return repo, nil }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { return execResolvedConfig(), nil }, + newProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return toolThenTextProvider{toolName: "glob"}, nil + }, + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{}, nil }, + }) + if code != exitSuccess { + t.Fatalf("exec exit = %d, stderr=%q", code, errBuf.String()) + } + return errBuf.String() + } + + // Untrusted: the project MCP layer is dropped, so the notice must name it. The repo + // has no project hooks/plugins, so the MCP skip is the ONLY thing that can fire it. + untrusted := run() + if !strings.Contains(untrusted, "MCP servers") || !strings.Contains(untrusted, "zero trust") { + t.Fatalf("untrusted exec must surface the MCP trust notice, stderr=%q", untrusted) + } + + // Trusted: nothing is skipped, so no notice at all. + if err := workspacetrust.Trust(repo); err != nil { + t.Fatal(err) + } + if trusted := run(); strings.Contains(trusted, "ignoring project") { + t.Fatalf("trusted exec must not emit a trust notice, stderr=%q", trusted) + } +} + +// TestExecSpecSurfacesMCPTrustNotice is the --use-spec analogue of the test above: +// the spec-draft path (exec_spec.go) must also thread the MCP skip into its notice. +// The fake provider never submits a spec, so the run exits non-zero; that is +// orthogonal to trust, so we assert only the notice, not the exit code. +func TestExecSpecSurfacesMCPTrustNotice(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("exec fake-provider harness assumes a POSIX process environment") + } + setTrustConfigRoot(t) + repo := t.TempDir() + if err := os.MkdirAll(filepath.Join(repo, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + body := `{"mcp":{"servers":{"proj":{"type":"stdio","command":"proj-cmd"}}}}` + if err := os.WriteFile(filepath.Join(repo, ".zero", "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + var out, errBuf bytes.Buffer + _ = runWithDeps([]string{"exec", "--use-spec", "--skip-permissions-unsafe", "--max-turns", "3", "go"}, &out, &errBuf, appDeps{ + getwd: func() (string, error) { return repo, nil }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { return execResolvedConfig(), nil }, + newProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return toolThenTextProvider{toolName: "glob"}, nil + }, + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { return config.MCPConfig{}, nil }, + }) + if !strings.Contains(errBuf.String(), "MCP servers") || !strings.Contains(errBuf.String(), "zero trust") { + t.Fatalf("untrusted --use-spec exec must surface the MCP trust notice, stderr=%q", errBuf.String()) + } +} + +// runExecTrust drives the full exec entry point with a fake worktree and a +// tool-calling provider, returning the exit code. The provider calls the core +// "glob" tool so dispatchBeforeTool fires inside the real exec-built registry. +func runExecTrust(t *testing.T, extraArgs []string, launchDir, worktreeDir string) int { + t.Helper() + t.Setenv("XDG_DATA_HOME", t.TempDir()) + args := append([]string{"exec", "--worktree", "--skip-permissions-unsafe", "--max-turns", "3"}, extraArgs...) + args = append(args, "go") + var out, errBuf bytes.Buffer + return runWithDeps(args, &out, &errBuf, appDeps{ + getwd: func() (string, error) { return launchDir, nil }, + prepareWorktree: func(context.Context, worktrees.Options) (worktrees.Result, error) { + return worktrees.Result{Path: worktreeDir}, nil + }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil + }, + newProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return toolThenTextProvider{toolName: "glob"}, nil + }, + }) +} + +// TestExecWorktreeInheritsTrustEndToEnd closes gap #1 for the exec path: trust is +// keyed on the ORIGINAL launch dir, not the generated worktree path. The worktree +// checkout carries the committed .zero/hooks.json; the gate must load it only when +// the SOURCE repo (launch dir) is trusted, proving exec.go captures trustRoot +// before the --worktree reassignment and threads it into the chokepoint. +func TestExecWorktreeInheritsTrustEndToEnd(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() // original launch dir -- the trust key + worktree := t.TempDir() // simulated worktree checkout (workspaceRoot after reassignment) + marker := filepath.Join(t.TempDir(), "marker") + writeMarkerHook(t, worktree, marker) // the checkout carries the committed hook + + // Untrusted source repo: worktree hooks must NOT run. + _ = os.Remove(marker) + if code := runExecTrust(t, nil, repo, worktree); code != exitSuccess { + t.Fatalf("exec --worktree (untrusted) exit = %d", code) + } + if _, err := os.Stat(marker); err == nil { + t.Fatal("UNTRUSTED worktree: project hook ran -- exec keyed trust on the worktree path or failed open") + } + + // Trusted source repo: the worktree inherits its trust, so the hook runs. + if err := workspacetrust.Trust(repo); err != nil { + t.Fatal(err) + } + _ = os.Remove(marker) + if code := runExecTrust(t, nil, repo, worktree); code != exitSuccess { + t.Fatalf("exec --worktree (trusted) exit = %d", code) + } + if _, err := os.Stat(marker); err != nil { + t.Fatalf("TRUSTED worktree: project hook did NOT run (marker absent) -- worktree trust inheritance broken: %v", err) + } +} + +// TestExecSpecWorktreeInheritsTrustEndToEnd closes gap #1 for the spec-draft path +// (the exact --use-spec --worktree combination the review fix addressed): the +// spec-draft chokepoint must also key trust on the original launch dir. +func TestExecSpecWorktreeInheritsTrustEndToEnd(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + worktree := t.TempDir() + marker := filepath.Join(t.TempDir(), "marker") + writeMarkerHook(t, worktree, marker) + + // The spec-draft flow itself exits non-zero here (the fake provider does not + // submit a real spec), which is orthogonal to trust: the hook dispatcher is + // built (keyed on run.trustRoot) before the agent runs, and glob (a read-only + // allow tool) is advertised in spec-draft, so beforeTool still fires. We assert + // only the marker, the trust behavior, not the spec-flow exit code. + + // Untrusted: spec-draft in a worktree of an untrusted repo runs no project hook. + _ = os.Remove(marker) + _ = runExecTrust(t, []string{"--use-spec"}, repo, worktree) + if _, err := os.Stat(marker); err == nil { + t.Fatal("UNTRUSTED spec-draft worktree: project hook ran -- spec-draft keyed trust on the worktree path") + } + + // Trusted: the spec-draft path inherits the source repo's trust. + if err := workspacetrust.Trust(repo); err != nil { + t.Fatal(err) + } + _ = os.Remove(marker) + _ = runExecTrust(t, []string{"--use-spec"}, repo, worktree) + if _, err := os.Stat(marker); err != nil { + t.Fatalf("TRUSTED spec-draft worktree: project hook did NOT run -- spec-draft trust inheritance broken: %v", err) + } +} diff --git a/internal/cli/trust_gate_test.go b/internal/cli/trust_gate_test.go new file mode 100644 index 000000000..5bfdf61ac --- /dev/null +++ b/internal/cli/trust_gate_test.go @@ -0,0 +1,466 @@ +package cli + +import ( + "bytes" + "context" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/Gitlawb/zero/internal/hooks" + "github.com/Gitlawb/zero/internal/plugins" + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/workspacetrust" +) + +// setTrustConfigRoot redirects both the workspace-trust store and the user-level +// hooks/plugins config to a fresh temp dir with a GOOS-aware env switch, mirroring +// setUserConfigRoot in internal/config/paths_test.go, and returns that dir so tests +// can build the exact paths the code resolves to. +// +// XDG_CONFIG_HOME is the lever on macOS as well as Linux: config.UserConfigDir is +// XDG-first there (it only falls back to $HOME/.config when XDG_CONFIG_HOME is +// unset), and the hooks loader resolves its user layer from XDG_CONFIG_HOME too. An +// earlier version set HOME on darwin and returned the raw root; that left the store +// at /.config/zero while the tests wrote to /zero, so the store-error +// and user-hook cases silently missed. Only Windows needs its own variable (APPDATA, +// what os.UserConfigDir consults). XDG_DATA_HOME is redirected so the hook audit +// store never touches the user's real data dir. +func setTrustConfigRoot(t *testing.T) string { + t.Helper() + root := t.TempDir() + switch runtime.GOOS { + case "windows": + t.Setenv("APPDATA", root) + default: + t.Setenv("XDG_CONFIG_HOME", root) + } + t.Setenv("XDG_DATA_HOME", t.TempDir()) + return root +} + +// writeMarkerHookScript writes an executable script that creates markerPath when +// run, and returns its absolute path. The hook dispatcher runs commands directly +// (no shell), so a real executable is needed to observe execution. +func writeMarkerHookScript(t *testing.T, markerPath string) string { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("marker-hook script is POSIX-shell based") + } + dir := t.TempDir() + script := filepath.Join(dir, "mark.sh") + body := "#!/bin/sh\n: > " + shellQuote(markerPath) + "\n" + if err := os.WriteFile(script, []byte(body), 0o700); err != nil { + t.Fatalf("write marker hook script: %v", err) + } + return script +} + +func shellQuote(path string) string { + return "'" + path + "'" +} + +// writeProjectHooks writes a project ./.zero/hooks.json under repo with the given +// hooks and top-level enabled flag. +func writeProjectHooks(t *testing.T, repo string, enabled bool, defs ...hooks.Definition) { + t.Helper() + path := filepath.Join(repo, ".zero", "hooks.json") + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("mkdir project .zero: %v", err) + } + if err := hooks.WriteConfig(path, hooks.Config{Enabled: enabled, Hooks: defs}); err != nil { + t.Fatalf("write project hooks.json: %v", err) + } +} + +// writeUserHooks writes a user-level hooks.json into the redirected config root. +func writeUserHooks(t *testing.T, configRoot string, enabled bool, defs ...hooks.Definition) { + t.Helper() + path := filepath.Join(configRoot, "zero", "hooks.json") + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("mkdir user config dir: %v", err) + } + if err := hooks.WriteConfig(path, hooks.Config{Enabled: enabled, Hooks: defs}); err != nil { + t.Fatalf("write user hooks.json: %v", err) + } +} + +func dispatchBeforeTool(dispatcher *hooks.Dispatcher) hooks.DispatchOutcome { + return dispatcher.Dispatch(context.Background(), hooks.DispatchInput{ + Event: hooks.EventBeforeTool, + ToolName: "bash", + }) +} + +func fileExists(path string) bool { + _, err := os.Stat(path) + return err == nil +} + +// TestHookGateUntrustedExcludesProjectLayer proves R1: an untrusted trustRoot with +// an enabled project beforeTool hook runs nothing and writes no marker. +func TestHookGateUntrustedExcludesProjectLayer(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + marker := filepath.Join(t.TempDir(), "ran") + script := writeMarkerHookScript(t, marker) + writeProjectHooks(t, repo, true, hooks.Definition{ + ID: "proj.mark", Event: hooks.EventBeforeTool, Command: script, Enabled: true, + }) + + dispatcher, skip := newHookDispatcherWithExtra(repo, nil, repo) + outcome := dispatchBeforeTool(dispatcher) + + if outcome.Ran != 0 { + t.Fatalf("untrusted workspace should run 0 project hooks, Ran=%d", outcome.Ran) + } + if fileExists(marker) { + t.Fatalf("untrusted workspace must not execute the project hook (marker exists)") + } + if !skip.excludedProjectConfig { + t.Fatalf("skip report should flag the excluded project hooks file") + } + if skip.trustCheckErrored { + t.Fatalf("a clean untrusted verdict is not a store-read error") + } +} + +// TestHookGateTrustedRunsProjectHook proves R3: after Trust(repo) the project hook +// runs and the marker appears. +func TestHookGateTrustedRunsProjectHook(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + marker := filepath.Join(t.TempDir(), "ran") + script := writeMarkerHookScript(t, marker) + writeProjectHooks(t, repo, true, hooks.Definition{ + ID: "proj.mark", Event: hooks.EventBeforeTool, Command: script, Enabled: true, + }) + + if err := workspacetrust.Trust(repo); err != nil { + t.Fatalf("Trust(repo): %v", err) + } + + dispatcher, skip := newHookDispatcherWithExtra(repo, nil, repo) + outcome := dispatchBeforeTool(dispatcher) + + if outcome.Ran != 1 { + t.Fatalf("trusted workspace should run the project hook, Ran=%d", outcome.Ran) + } + if !fileExists(marker) { + t.Fatalf("trusted workspace must execute the project hook (marker missing)") + } + if skip.excludedProjectConfig { + t.Fatalf("a trusted workspace must not report the project layer as excluded") + } +} + +// TestHookGateEmptyTrustRootFailsClosed proves the fail-closed-by-construction +// guard: a caller that forgot to resolve trustRoot (empty) still excludes the +// project layer, so no marker appears. +func TestHookGateEmptyTrustRootFailsClosed(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + marker := filepath.Join(t.TempDir(), "ran") + script := writeMarkerHookScript(t, marker) + writeProjectHooks(t, repo, true, hooks.Definition{ + ID: "proj.mark", Event: hooks.EventBeforeTool, Command: script, Enabled: true, + }) + // Even trusting the repo must not help when the caller passes an empty root. + if err := workspacetrust.Trust(repo); err != nil { + t.Fatalf("Trust(repo): %v", err) + } + + dispatcher, skip := newHookDispatcherWithExtra(repo, nil, "") + outcome := dispatchBeforeTool(dispatcher) + + if outcome.Ran != 0 { + t.Fatalf("empty trustRoot must fail closed, Ran=%d", outcome.Ran) + } + if fileExists(marker) { + t.Fatalf("empty trustRoot must not execute the project hook (marker exists)") + } + if !skip.excludedProjectConfig { + t.Fatalf("empty trustRoot should report the project layer excluded") + } +} + +// TestHookGateFailClosedOnStoreError proves R5: a real IsTrusted error (trust.json +// created as a directory, per U1) is treated as untrusted, the project hook does +// not run, and the skip report marks the store-read error. +func TestHookGateFailClosedOnStoreError(t *testing.T) { + configRoot := setTrustConfigRoot(t) + // Create the trust store path as a DIRECTORY so os.ReadFile fails with a + // non-ErrNotExist error, forcing IsTrusted to return (false, non-nil). + trustPath := filepath.Join(configRoot, "zero", "trust.json") + if err := os.MkdirAll(trustPath, 0o700); err != nil { + t.Fatalf("create trust.json as a directory: %v", err) + } + + repo := t.TempDir() + marker := filepath.Join(t.TempDir(), "ran") + script := writeMarkerHookScript(t, marker) + writeProjectHooks(t, repo, true, hooks.Definition{ + ID: "proj.mark", Event: hooks.EventBeforeTool, Command: script, Enabled: true, + }) + + dispatcher, skip := newHookDispatcherWithExtra(repo, nil, repo) + outcome := dispatchBeforeTool(dispatcher) + + if outcome.Ran != 0 { + t.Fatalf("a store-read error must fail closed, Ran=%d", outcome.Ran) + } + if fileExists(marker) { + t.Fatalf("a store-read error must not execute the project hook (marker exists)") + } + if !skip.trustCheckErrored { + t.Fatalf("skip report must mark the store-read error") + } + if !skip.excludedProjectConfig { + t.Fatalf("the project layer must be reported excluded on the error path") + } +} + +// TestHookGateUserHookStillRunsWhenProjectExcluded proves R4/R10: with the project +// layer excluded, a user-level hook still fires, and a project hooks.json that sets +// global enabled:false or defines a same-ID hook cannot disable or override it. +func TestHookGateUserHookStillRunsWhenProjectExcluded(t *testing.T) { + configRoot := setTrustConfigRoot(t) + repo := t.TempDir() + marker := filepath.Join(t.TempDir(), "ran") + script := writeMarkerHookScript(t, marker) + + // User hook that writes the marker. + writeUserHooks(t, configRoot, true, hooks.Definition{ + ID: "shared.mark", Event: hooks.EventBeforeTool, Command: script, Enabled: true, + }) + // Project config that would, if loaded, disable the whole surface (enabled:false) + // AND override the user hook by ID with a disabled no-op. The gate drops the + // whole project layer, so neither takes effect. + writeProjectHooks(t, repo, false, hooks.Definition{ + ID: "shared.mark", Event: hooks.EventBeforeTool, Command: "true", Enabled: false, + }) + + // Untrusted: the project layer is excluded. + dispatcher, _ := newHookDispatcherWithExtra(repo, nil, repo) + outcome := dispatchBeforeTool(dispatcher) + + if outcome.Ran != 1 { + t.Fatalf("user hook must still run when the project layer is excluded, Ran=%d", outcome.Ran) + } + if !fileExists(marker) { + t.Fatalf("user hook must fire (marker missing); project config must not disable/override it") + } +} + +// TestHookGateWorktreeInheritsTrust proves the worktree case: with repo trusted, +// calling the chokepoint with a different workspaceRoot (the worktree path) but +// trustRoot=repo loads the project hooks, so trust keys on trustRoot not +// workspaceRoot. +func TestHookGateWorktreeInheritsTrust(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + worktree := t.TempDir() // a distinct, never-trusted path + marker := filepath.Join(t.TempDir(), "ran") + script := writeMarkerHookScript(t, marker) + // The project hooks live under the worktree (that is where config is read from). + writeProjectHooks(t, worktree, true, hooks.Definition{ + ID: "proj.mark", Event: hooks.EventBeforeTool, Command: script, Enabled: true, + }) + + if err := workspacetrust.Trust(repo); err != nil { + t.Fatalf("Trust(repo): %v", err) + } + + // workspaceRoot is the worktree, but trustRoot is the original repo. + dispatcher, skip := newHookDispatcherWithExtra(worktree, nil, repo) + outcome := dispatchBeforeTool(dispatcher) + + if outcome.Ran != 1 { + t.Fatalf("a worktree of a trusted repo should load project hooks, Ran=%d", outcome.Ran) + } + if !fileExists(marker) { + t.Fatalf("worktree run should execute the project hook (marker missing)") + } + if skip.excludedProjectConfig { + t.Fatalf("worktree of a trusted repo must not report the project layer excluded") + } +} + +// TestPluginGateUntrustedExcludesProject proves R2/R5: an untrusted trustRoot makes +// activatePlugins load with ExcludeProject=true and report the skip. +func TestPluginGateUntrustedExcludesProject(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + // A ./.zero/plugins dir so the skip report flags an excluded project config. + if err := os.MkdirAll(filepath.Join(repo, ".zero", "plugins"), 0o700); err != nil { + t.Fatalf("mkdir project plugins dir: %v", err) + } + + var gotExclude bool + deps := appDeps{ + loadPlugins: func(opts plugins.LoadOptions) (plugins.LoadResult, error) { + gotExclude = opts.ExcludeProject + return plugins.LoadResult{}, nil + }, + skillsDir: func() string { return t.TempDir() }, + } + registry := tools.NewRegistry() + var stderr bytes.Buffer + activation := activatePlugins(repo, registry, deps, &stderr, repo) + + if !gotExclude { + t.Fatalf("untrusted workspace must pass ExcludeProject=true to loadPlugins") + } + if !activation.excludedProjectConfig { + t.Fatalf("skip report should flag the excluded ./.zero/plugins dir") + } + if activation.trustCheckErrored { + t.Fatalf("a clean untrusted verdict is not a store-read error") + } +} + +// TestPluginGateTrustedIncludesProject proves R3: after Trust(repo), activatePlugins +// loads with ExcludeProject=false. +func TestPluginGateTrustedIncludesProject(t *testing.T) { + setTrustConfigRoot(t) + repo := t.TempDir() + if err := workspacetrust.Trust(repo); err != nil { + t.Fatalf("Trust(repo): %v", err) + } + + var gotExclude bool + deps := appDeps{ + loadPlugins: func(opts plugins.LoadOptions) (plugins.LoadResult, error) { + gotExclude = opts.ExcludeProject + return plugins.LoadResult{}, nil + }, + skillsDir: func() string { return t.TempDir() }, + } + registry := tools.NewRegistry() + var stderr bytes.Buffer + activation := activatePlugins(repo, registry, deps, &stderr, repo) + + if gotExclude { + t.Fatalf("trusted workspace must pass ExcludeProject=false to loadPlugins") + } + if activation.excludedProjectConfig { + t.Fatalf("trusted workspace must not report the project config excluded") + } +} + +// TestPluginGateFailClosedOnStoreError proves R5 for plugins: a store-read error +// forces ExcludeProject=true and marks the error. +func TestPluginGateFailClosedOnStoreError(t *testing.T) { + configRoot := setTrustConfigRoot(t) + trustPath := filepath.Join(configRoot, "zero", "trust.json") + if err := os.MkdirAll(trustPath, 0o700); err != nil { + t.Fatalf("create trust.json as a directory: %v", err) + } + repo := t.TempDir() + if err := os.MkdirAll(filepath.Join(repo, ".zero", "plugins"), 0o700); err != nil { + t.Fatalf("mkdir project plugins dir: %v", err) + } + + var gotExclude bool + deps := appDeps{ + loadPlugins: func(opts plugins.LoadOptions) (plugins.LoadResult, error) { + gotExclude = opts.ExcludeProject + return plugins.LoadResult{}, nil + }, + skillsDir: func() string { return t.TempDir() }, + } + registry := tools.NewRegistry() + var stderr bytes.Buffer + activation := activatePlugins(repo, registry, deps, &stderr, repo) + + if !gotExclude { + t.Fatalf("a store-read error must fail closed (ExcludeProject=true)") + } + if !activation.trustCheckErrored { + t.Fatalf("skip report must mark the store-read error") + } +} + +// TestEmitTrustNoticeOneLineWhenSkipped proves R6: exactly one notice when either +// surface skips project config, none when trusted, and distinct error text on a +// store-read failure. +func TestEmitTrustNoticeOneLineWhenSkipped(t *testing.T) { + t.Run("both surfaces skipped yields one line", func(t *testing.T) { + var buf bytes.Buffer + emitTrustNotice(&buf, + trustSkip{excludedProjectConfig: true}, + trustSkip{excludedProjectConfig: true}) + lines := nonEmptyLines(buf.String()) + if len(lines) != 1 { + t.Fatalf("expected exactly one notice line, got %d: %q", len(lines), buf.String()) + } + if !bytes.Contains(buf.Bytes(), []byte("zero trust")) { + t.Fatalf("notice should point at 'zero trust', got %q", buf.String()) + } + }) + + t.Run("trusted yields no notice", func(t *testing.T) { + var buf bytes.Buffer + emitTrustNotice(&buf, trustSkip{}, trustSkip{}) + if buf.Len() != 0 { + t.Fatalf("a trusted session must emit no notice, got %q", buf.String()) + } + }) + + t.Run("store-read error uses distinct text", func(t *testing.T) { + var buf bytes.Buffer + emitTrustNotice(&buf, + trustSkip{excludedProjectConfig: true, trustCheckErrored: true}, + trustSkip{}) + lines := nonEmptyLines(buf.String()) + if len(lines) != 1 { + t.Fatalf("expected exactly one notice line, got %d: %q", len(lines), buf.String()) + } + if !bytes.Contains(buf.Bytes(), []byte("could not be read")) { + t.Fatalf("error-path notice should name the store read failure, got %q", buf.String()) + } + }) + + t.Run("only one surface skipped still yields one line", func(t *testing.T) { + var buf bytes.Buffer + emitTrustNotice(&buf, trustSkip{excludedProjectConfig: true}, trustSkip{}) + lines := nonEmptyLines(buf.String()) + if len(lines) != 1 { + t.Fatalf("expected exactly one notice line, got %d: %q", len(lines), buf.String()) + } + }) + + t.Run("mcp skip alone yields one line naming mcp", func(t *testing.T) { + var buf bytes.Buffer + // Hooks and plugins clean; only the MCP surface (third arg) dropped project config. + emitTrustNotice(&buf, trustSkip{}, trustSkip{}, trustSkip{excludedProjectConfig: true}) + lines := nonEmptyLines(buf.String()) + if len(lines) != 1 { + t.Fatalf("expected exactly one notice line, got %d: %q", len(lines), buf.String()) + } + if !bytes.Contains(buf.Bytes(), []byte("MCP")) { + t.Fatalf("notice should name MCP when the MCP surface is skipped, got %q", buf.String()) + } + }) + + t.Run("store error with nothing excluded yields no notice", func(t *testing.T) { + // trustCheckErrored but no surface had project config to skip (excludedProjectConfig + // false): there is nothing to warn about, so the excluded-gate wins over the error. + var buf bytes.Buffer + emitTrustNotice(&buf, trustSkip{trustCheckErrored: true}, trustSkip{trustCheckErrored: true}) + if buf.Len() != 0 { + t.Fatalf("a store error with no skipped project config must emit nothing, got %q", buf.String()) + } + }) +} + +func nonEmptyLines(s string) []string { + out := []string{} + for _, line := range bytes.Split([]byte(s), []byte("\n")) { + if len(bytes.TrimSpace(line)) > 0 { + out = append(out, string(line)) + } + } + return out +} diff --git a/internal/cli/trust_test.go b/internal/cli/trust_test.go new file mode 100644 index 000000000..9cd807106 --- /dev/null +++ b/internal/cli/trust_test.go @@ -0,0 +1,227 @@ +package cli + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/workspacetrust" +) + +// trustDeps builds a test appDeps whose getwd returns a fixed directory, so the +// trust command can be exercised without touching the real cwd. +func trustDeps(cwd string) appDeps { + return appDeps{getwd: func() (string, error) { return cwd, nil }} +} + +// TestRunTrustAddThenList proves `trust` trusts the cwd and `trust list` shows it. +func TestRunTrustAddThenList(t *testing.T) { + setTrustConfigRoot(t) + cwd := t.TempDir() + deps := trustDeps(cwd) + + var out, errBuf bytes.Buffer + if code := runTrust(nil, &out, &errBuf, deps); code != exitSuccess { + t.Fatalf("trust returned %d, want %d; stderr=%q", code, exitSuccess, errBuf.String()) + } + if !strings.Contains(out.String(), "Trusted "+cwd) { + t.Fatalf("trust stdout = %q, want it to contain %q", out.String(), "Trusted "+cwd) + } + + trusted, err := workspacetrust.IsTrusted(cwd) + if err != nil { + t.Fatalf("IsTrusted after trust: %v", err) + } + if !trusted { + t.Fatalf("cwd should be trusted after `trust`") + } + + // The store canonicalizes with filepath.EvalSymlinks, so `list` prints the + // resolved path. On platforms where the temp dir is a symlink (macOS /var) or a + // Windows 8.3 short name (RUNNER~1 -> runneradmin), that differs from the raw + // cwd; compare against the same canonical form the store records. + wantListed := cwd + if resolved, err := filepath.EvalSymlinks(cwd); err == nil { + wantListed = resolved + } + + out.Reset() + errBuf.Reset() + if code := runTrust([]string{"list"}, &out, &errBuf, deps); code != exitSuccess { + t.Fatalf("trust list returned %d, want %d; stderr=%q", code, exitSuccess, errBuf.String()) + } + if !strings.Contains(out.String(), wantListed) { + t.Fatalf("trust list stdout = %q, want it to contain the trusted root %q", out.String(), wantListed) + } +} + +// TestRunTrustListShowsCanonicalPath executes the canonical-path behavior that broke +// TestRunTrustAddThenList on the Windows runner (an 8.3 short name expanded by +// EvalSymlinks). It uses a symlinked cwd -- the portable, Linux-runnable analogue of +// that divergence -- and proves `trust list` prints the resolved path the store +// records, never the raw pre-resolution path. +func TestRunTrustListShowsCanonicalPath(t *testing.T) { + setTrustConfigRoot(t) + realDir := t.TempDir() + link := filepath.Join(t.TempDir(), "link") + if err := os.Symlink(realDir, link); err != nil { + t.Skipf("symlinks unavailable on this platform: %v", err) + } + resolved, err := filepath.EvalSymlinks(link) + if err != nil { + t.Fatalf("EvalSymlinks(link): %v", err) + } + if resolved == link { + t.Skip("symlink did not change the path on this platform") + } + + deps := trustDeps(link) + if code := runTrust(nil, &bytes.Buffer{}, &bytes.Buffer{}, deps); code != exitSuccess { + t.Fatalf("trust returned %d", code) + } + + var out, errBuf bytes.Buffer + if code := runTrust([]string{"list"}, &out, &errBuf, deps); code != exitSuccess { + t.Fatalf("trust list returned %d; stderr=%q", code, errBuf.String()) + } + if !strings.Contains(out.String(), resolved) { + t.Fatalf("trust list = %q, want the canonical path %q", out.String(), resolved) + } + if strings.Contains(out.String(), link) { + t.Fatalf("trust list = %q, must not contain the raw pre-resolution path %q", out.String(), link) + } +} + +// TestRunTrustRemoveCurrentDir proves `trust remove` (no arg) untrusts the cwd and +// `trust list` then reports none. +func TestRunTrustRemoveCurrentDir(t *testing.T) { + setTrustConfigRoot(t) + cwd := t.TempDir() + deps := trustDeps(cwd) + + if code := runTrust(nil, &bytes.Buffer{}, &bytes.Buffer{}, deps); code != exitSuccess { + t.Fatalf("trust setup returned %d", code) + } + + var out, errBuf bytes.Buffer + if code := runTrust([]string{"remove"}, &out, &errBuf, deps); code != exitSuccess { + t.Fatalf("trust remove returned %d, want %d; stderr=%q", code, exitSuccess, errBuf.String()) + } + if !strings.Contains(out.String(), "Untrusted "+cwd) { + t.Fatalf("trust remove stdout = %q, want it to contain %q", out.String(), "Untrusted "+cwd) + } + + trusted, err := workspacetrust.IsTrusted(cwd) + if err != nil { + t.Fatalf("IsTrusted after remove: %v", err) + } + if trusted { + t.Fatalf("cwd should not be trusted after `trust remove`") + } + + out.Reset() + errBuf.Reset() + if code := runTrust([]string{"list"}, &out, &errBuf, deps); code != exitSuccess { + t.Fatalf("trust list returned %d, want %d", code, exitSuccess) + } + if !strings.Contains(out.String(), "No trusted workspaces.") { + t.Fatalf("trust list stdout = %q, want %q for an empty store", out.String(), "No trusted workspaces.") + } +} + +// TestRunTrustRemoveNamedPathNonCanonical proves `trust remove ` untrusts a +// specific path, and that a non-canonical argument (a trailing slash) still matches +// the stored normalized entry. +func TestRunTrustRemoveNamedPathNonCanonical(t *testing.T) { + setTrustConfigRoot(t) + // The cwd is a different directory than the one we trust-and-remove, so this + // exercises the named-path branch, not the bare-cwd branch. + cwd := t.TempDir() + target := t.TempDir() + deps := trustDeps(cwd) + + if err := workspacetrust.Trust(target); err != nil { + t.Fatalf("Trust(target): %v", err) + } + trusted, err := workspacetrust.IsTrusted(target) + if err != nil || !trusted { + t.Fatalf("target should be trusted before remove (trusted=%v err=%v)", trusted, err) + } + + // Pass the target with a trailing slash: normalization must still match the + // stored canonical entry. + var out, errBuf bytes.Buffer + if code := runTrust([]string{"remove", target + "/"}, &out, &errBuf, deps); code != exitSuccess { + t.Fatalf("trust remove returned %d, want %d; stderr=%q", code, exitSuccess, errBuf.String()) + } + + trusted, err = workspacetrust.IsTrusted(target) + if err != nil { + t.Fatalf("IsTrusted after named remove: %v", err) + } + if trusted { + t.Fatalf("target should be untrusted after `trust remove /` despite the trailing slash") + } +} + +// TestRunTrustUnknownSubcommand proves an unknown subcommand returns exit code 2 and +// writes usage to stderr (not stdout). +func TestRunTrustUnknownSubcommand(t *testing.T) { + setTrustConfigRoot(t) + deps := trustDeps(t.TempDir()) + + var out, errBuf bytes.Buffer + if code := runTrust([]string{"bogus"}, &out, &errBuf, deps); code != exitUsage { + t.Fatalf("unknown subcommand returned %d, want %d", code, exitUsage) + } + if errBuf.Len() == 0 { + t.Fatalf("unknown subcommand should write usage to stderr, stderr was empty") + } + if !strings.Contains(errBuf.String(), "trust") { + t.Fatalf("stderr usage = %q, want it to mention `trust`", errBuf.String()) + } + if out.Len() != 0 { + t.Fatalf("unknown subcommand should not write to stdout, got %q", out.String()) + } +} + +// TestRunTrustRemoveTooManyArgs proves `trust remove a b` (more than one path) is a +// usage error: exit code 2, usage on stderr, nothing on stdout, and no store change. +func TestRunTrustRemoveTooManyArgs(t *testing.T) { + setTrustConfigRoot(t) + deps := trustDeps(t.TempDir()) + + var out, errBuf bytes.Buffer + if code := runTrust([]string{"remove", "a", "b"}, &out, &errBuf, deps); code != exitUsage { + t.Fatalf("remove with two args returned %d, want %d", code, exitUsage) + } + if errBuf.Len() == 0 { + t.Fatalf("remove with two args should write usage to stderr, stderr was empty") + } + if out.Len() != 0 { + t.Fatalf("remove with two args should not write to stdout, got %q", out.String()) + } +} + +// TestRunTrustHelp proves the -h / --help / help subcommands print usage to stdout, +// nothing to stderr, and return success, matching the other CLI help entrypoints +// (unknown subcommands, by contrast, write to stderr with the usage exit code). +func TestRunTrustHelp(t *testing.T) { + setTrustConfigRoot(t) + deps := trustDeps(t.TempDir()) + + for _, flag := range []string{"-h", "--help", "help"} { + var out, errBuf bytes.Buffer + if code := runTrust([]string{flag}, &out, &errBuf, deps); code != exitSuccess { + t.Fatalf("trust %s returned %d, want %d", flag, code, exitSuccess) + } + if !strings.Contains(out.String(), "Usage") { + t.Fatalf("trust %s stdout = %q, want it to contain usage text", flag, out.String()) + } + if errBuf.Len() != 0 { + t.Fatalf("trust %s should not write to stderr, got %q", flag, errBuf.String()) + } + } +} diff --git a/internal/config/resolve_mcp_exclude_project_test.go b/internal/config/resolve_mcp_exclude_project_test.go new file mode 100644 index 000000000..e86872854 --- /dev/null +++ b/internal/config/resolve_mcp_exclude_project_test.go @@ -0,0 +1,61 @@ +package config + +import ( + "os" + "path/filepath" + "testing" +) + +// TestResolveMCPExcludeProjectDropsProjectServers proves that ExcludeProject drops +// the project config layer from MCP resolution (fail-closed for an untrusted +// workspace) while keeping the built-in defaults and the user config server. +func TestResolveMCPExcludeProjectDropsProjectServers(t *testing.T) { + userPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(userPath, []byte(`{"mcp":{"servers":{"user-srv":{"type":"stdio","command":"user-cmd"}}}}`), 0o600); err != nil { + t.Fatal(err) + } + projectPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(projectPath, []byte(`{"mcp":{"servers":{"proj-srv":{"type":"stdio","command":"proj-cmd"}}}}`), 0o600); err != nil { + t.Fatal(err) + } + + t.Run("included when ExcludeProject is false", func(t *testing.T) { + cfg, err := ResolveMCP(ResolveOptions{ + UserConfigPath: userPath, + ProjectConfigPath: projectPath, + ExcludeProject: false, + }) + if err != nil { + t.Fatalf("ResolveMCP: %v", err) + } + if _, ok := cfg.Servers["proj-srv"]; !ok { + t.Fatal("a trusted resolve (ExcludeProject=false) must include the project server") + } + if _, ok := cfg.Servers["user-srv"]; !ok { + t.Fatal("the user server must always be present") + } + if _, ok := cfg.Servers["firecrawl"]; !ok { + t.Fatal("the built-in default must always be present") + } + }) + + t.Run("excluded when ExcludeProject is true", func(t *testing.T) { + cfg, err := ResolveMCP(ResolveOptions{ + UserConfigPath: userPath, + ProjectConfigPath: projectPath, + ExcludeProject: true, + }) + if err != nil { + t.Fatalf("ResolveMCP: %v", err) + } + if _, ok := cfg.Servers["proj-srv"]; ok { + t.Fatal("an untrusted resolve (ExcludeProject=true) must drop the project server") + } + if _, ok := cfg.Servers["user-srv"]; !ok { + t.Fatal("the user server must survive when the project layer is dropped") + } + if _, ok := cfg.Servers["firecrawl"]; !ok { + t.Fatal("the built-in default must survive when the project layer is dropped") + } + }) +} diff --git a/internal/config/resolver.go b/internal/config/resolver.go index 83d7f141c..b40874c28 100644 --- a/internal/config/resolver.go +++ b/internal/config/resolver.go @@ -167,6 +167,12 @@ func ResolveMCP(options ResolveOptions) (MCPConfig, error) { if path == "" { continue } + // Drop the project layer when the workspace is untrusted, so a cloned repo's + // ./.zero/config.json cannot register (and spawn) stdio MCP servers. Fail-closed: + // only a trusted workspace clears ExcludeProject. Defaults and user config still load. + if options.ExcludeProject && path == options.ProjectConfigPath { + continue + } fileConfig, err := loadConfigFile(path) if err != nil { return MCPConfig{}, err diff --git a/internal/config/types.go b/internal/config/types.go index 1b0e94a11..2ac4274ec 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -398,6 +398,11 @@ type ResolveOptions struct { ProviderCommand string Env map[string]string Overrides Overrides + // ExcludeProject drops the project config layer (ProjectConfigPath) from MCP + // resolution when the workspace is untrusted, so a cloned repo's ./.zero/config.json + // cannot spawn stdio MCP servers. It is fail-closed: only a trusted workspace sets + // it false. Mirrors the ExcludeProject option hooks and plugins already honor. + ExcludeProject bool } type Overrides struct { diff --git a/internal/hooks/hooks.go b/internal/hooks/hooks.go index 34ca6ed53..f7dd79cea 100644 --- a/internal/hooks/hooks.go +++ b/internal/hooks/hooks.go @@ -98,6 +98,9 @@ type LoadOptions struct { Env map[string]string UserConfigPath string ProjectConfigPath string + // ExcludeProject drops the project layer (./.zero/hooks.json) so only the + // user layer loads. Its zero value (false) preserves the user+project merge. + ExcludeProject bool } type StoreOptions struct { @@ -220,6 +223,9 @@ func LoadConfig(options LoadOptions) (LoadResult, error) { {source: SourceUser, path: userConfigPath}, {source: SourceProject, path: projectConfigPath}, } { + if candidate.source == SourceProject && options.ExcludeProject { + continue + } layer, ok := readLayer(candidate.source, candidate.path, &diagnostics) if ok { layers = append(layers, layer) diff --git a/internal/hooks/hooks_test.go b/internal/hooks/hooks_test.go index ef7d4cd85..a12580512 100644 --- a/internal/hooks/hooks_test.go +++ b/internal/hooks/hooks_test.go @@ -86,6 +86,58 @@ func TestLoadConfigLayersProjectOverridesAndDiagnostics(t *testing.T) { } } +func TestLoadConfigExcludeProjectSkipsProjectLayer(t *testing.T) { + userConfig := map[string]any{ + "enabled": true, + "hooks": []any{map[string]any{ + "id": "zero.user", + "event": "beforeTool", + "command": "node", + "args": []string{"user.mjs"}, + "enabled": true, + }}, + } + projectConfig := map[string]any{ + "enabled": true, + "hooks": []any{map[string]any{ + "id": "zero.project", + "event": "beforeTool", + "command": "node", + "args": []string{"project.mjs"}, + "enabled": true, + }}, + } + + for _, tc := range []struct { + name string + excludeProject bool + wantIDs []string + }{ + {name: "include project (default)", excludeProject: false, wantIDs: []string{"zero.project", "zero.user"}}, + {name: "exclude project", excludeProject: true, wantIDs: []string{"zero.user"}}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + userConfigPath := filepath.Join(dir, "user-hooks.json") + projectConfigPath := filepath.Join(dir, "project-hooks.json") + writeHookJSON(t, userConfigPath, userConfig) + writeHookJSON(t, projectConfigPath, projectConfig) + + result, err := LoadConfig(LoadOptions{ + UserConfigPath: userConfigPath, + ProjectConfigPath: projectConfigPath, + ExcludeProject: tc.excludeProject, + }) + if err != nil { + t.Fatalf("LoadConfig returned error: %v", err) + } + if got := hookIDs(result.Config.Hooks); !reflect.DeepEqual(got, tc.wantIDs) { + t.Fatalf("hook ids = %#v, want %#v", got, tc.wantIDs) + } + }) + } +} + func TestLoadConfigPreservesUserDisabledStateWhenProjectOmitsEnabled(t *testing.T) { dir := t.TempDir() userConfigPath := filepath.Join(dir, "user-hooks.json") diff --git a/internal/plugins/plugins.go b/internal/plugins/plugins.go index e96b99af5..4e9370a3f 100644 --- a/internal/plugins/plugins.go +++ b/internal/plugins/plugins.go @@ -140,6 +140,12 @@ type LoadOptions struct { Cwd string Env map[string]string AllowManifestToolAutoApproval bool + // ExcludeProject drops every SourceProject root before discovery. It is + // applied to the resolved root slice regardless of whether the roots came + // from Roots or ResolveRoots, so a caller passing an explicit SourceProject + // root cannot bypass the exclusion. The zero value (false) preserves the + // default behavior of loading both user and project roots. + ExcludeProject bool } type ParseManifestOptions struct { @@ -200,6 +206,17 @@ func Load(options LoadOptions) (LoadResult, error) { roots = resolvedRoots } + if options.ExcludeProject { + filtered := roots[:0:0] + for _, root := range roots { + if root.Source == SourceProject { + continue + } + filtered = append(filtered, root) + } + roots = filtered + } + diagnostics := []Diagnostic{} discovered := []LoadedPlugin{} for _, root := range roots { diff --git a/internal/plugins/plugins_test.go b/internal/plugins/plugins_test.go index 1de5a2c38..367f416cb 100644 --- a/internal/plugins/plugins_test.go +++ b/internal/plugins/plugins_test.go @@ -380,6 +380,103 @@ func TestLoadPluginsDiscoversDiagnosticsAndProjectPrecedence(t *testing.T) { } } +func TestLoadDiscoversProjectPluginWhenNotExcluded(t *testing.T) { + dir := t.TempDir() + writePluginManifest(t, filepath.Join(dir, ".zero", "plugins", "demo"), map[string]any{ + "schemaVersion": 1, + "id": "zero.project", + "name": "Project Plugin", + "version": "0.1.0", + }) + + result, err := Load(LoadOptions{ + Cwd: dir, + Env: map[string]string{"XDG_CONFIG_HOME": filepath.Join(dir, "xdg")}, + }) + if err != nil { + t.Fatalf("Load returned error: %v", err) + } + if !hasPlugin(result.Plugins, "zero.project") { + t.Fatalf("expected project plugin to be discovered, got %#v", result.Plugins) + } +} + +func TestLoadExcludesProjectPlugin(t *testing.T) { + dir := t.TempDir() + writePluginManifest(t, filepath.Join(dir, ".zero", "plugins", "demo"), map[string]any{ + "schemaVersion": 1, + "id": "zero.project", + "name": "Project Plugin", + "version": "0.1.0", + }) + + result, err := Load(LoadOptions{ + Cwd: dir, + Env: map[string]string{"XDG_CONFIG_HOME": filepath.Join(dir, "xdg")}, + ExcludeProject: true, + }) + if err != nil { + t.Fatalf("Load returned error: %v", err) + } + if hasPlugin(result.Plugins, "zero.project") { + t.Fatalf("project plugin should be excluded, got %#v", result.Plugins) + } +} + +func TestLoadKeepsUserPluginWhenProjectExcluded(t *testing.T) { + dir := t.TempDir() + // User plugin lives under the resolved XDG config home; project plugin under ./.zero. + writePluginManifest(t, filepath.Join(dir, "xdg", "zero", "plugins", "user"), map[string]any{ + "schemaVersion": 1, + "id": "zero.user", + "name": "User Plugin", + "version": "0.1.0", + }) + writePluginManifest(t, filepath.Join(dir, ".zero", "plugins", "demo"), map[string]any{ + "schemaVersion": 1, + "id": "zero.project", + "name": "Project Plugin", + "version": "0.1.0", + }) + + result, err := Load(LoadOptions{ + Cwd: dir, + Env: map[string]string{"XDG_CONFIG_HOME": filepath.Join(dir, "xdg")}, + ExcludeProject: true, + }) + if err != nil { + t.Fatalf("Load returned error: %v", err) + } + if !hasPlugin(result.Plugins, "zero.user") { + t.Fatalf("user plugin should still be discovered, got %#v", result.Plugins) + } + if hasPlugin(result.Plugins, "zero.project") { + t.Fatalf("project plugin should be excluded, got %#v", result.Plugins) + } +} + +func TestLoadExcludeProjectFiltersExplicitProjectRoots(t *testing.T) { + dir := t.TempDir() + projectRoot := filepath.Join(dir, "project-plugins") + writePluginManifest(t, filepath.Join(projectRoot, "demo"), map[string]any{ + "schemaVersion": 1, + "id": "zero.project", + "name": "Project Plugin", + "version": "0.1.0", + }) + + result, err := Load(LoadOptions{ + Roots: []Root{{Source: SourceProject, Path: projectRoot}}, + ExcludeProject: true, + }) + if err != nil { + t.Fatalf("Load returned error: %v", err) + } + if len(result.Plugins) != 0 { + t.Fatalf("explicit project root should be filtered when ExcludeProject is set, got %#v", result.Plugins) + } +} + func TestResolveRootsUsesConfigHomeAndProjectRoot(t *testing.T) { dir := t.TempDir() roots, err := ResolveRoots(ResolveRootOptions{ @@ -411,6 +508,15 @@ func writePluginManifest(t *testing.T, pluginDir string, manifest map[string]any } } +func hasPlugin(plugins []LoadedPlugin, pluginID string) bool { + for _, plugin := range plugins { + if plugin.ID == pluginID { + return true + } + } + return false +} + func hasPluginDiagnostic(diagnostics []Diagnostic, kind DiagnosticKind, pluginID string) bool { for _, diagnostic := range diagnostics { if diagnostic.Kind != kind { diff --git a/internal/workspacetrust/trust.go b/internal/workspacetrust/trust.go new file mode 100644 index 000000000..c8adb30e3 --- /dev/null +++ b/internal/workspacetrust/trust.go @@ -0,0 +1,221 @@ +// Package workspacetrust records which workspace roots the user has explicitly +// trusted, so Zero can gate project-scoped executable config (hooks, plugins) +// behind an opt-in per workspace and fail closed on any error. +// +// Trust is keyed on the normalized absolute workspace root (filepath.Abs then +// filepath.EvalSymlinks), and membership is an EXACT match: a nested repo or +// subdirectory under a trusted root is NOT trusted, so trusting a monorepo root +// does not implicitly trust a vendored dependency or submodule that ships its +// own .zero/. The store is a plaintext JSON file at +// /zero/trust.json; it holds workspace paths, not secrets, so it +// is not encrypted, but it is written atomically with restrictive permissions +// (dir 0o700, file 0o600). +package workspacetrust + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "sort" + + "github.com/Gitlawb/zero/internal/config" +) + +// store is the on-disk JSON shape: {"trusted": ["", ...]}. +type store struct { + Trusted []string `json:"trusted"` +} + +// storeFilePath returns /zero/trust.json, reusing the config +// package's XDG resolution rather than re-implementing it. +func storeFilePath() (string, error) { + dir, err := config.UserConfigDir() + if err != nil { + return "", fmt.Errorf("resolve user config directory: %w", err) + } + return filepath.Join(dir, "zero", "trust.json"), nil +} + +// normalize resolves a workspace root to its canonical absolute form: +// filepath.Abs then filepath.EvalSymlinks, falling back to the Abs path when +// EvalSymlinks errors (typically because the path does not exist). Each incoming +// path is normalized once, at its Trust/Untrust/IsTrusted entry point; stored +// entries are then compared as canonical literals and never re-resolved (so a +// retargeted symlink cannot drift or forge a stored match). +func normalize(workspaceRoot string) (string, error) { + abs, err := filepath.Abs(workspaceRoot) + if err != nil { + return "", fmt.Errorf("resolve absolute path for %q: %w", workspaceRoot, err) + } + resolved, err := filepath.EvalSymlinks(abs) + if err != nil { + // The path may not exist yet; the absolute form is still a stable key. + return abs, nil + } + return resolved, nil +} + +// loadStore reads the trust store. A missing store is not an error: it returns +// an empty store and a nil error. A store that exists but cannot be read or +// parsed returns a non-nil error so callers can fail closed. +func loadStore() (store, error) { + path, err := storeFilePath() + if err != nil { + return store{}, err + } + data, err := os.ReadFile(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return store{}, nil + } + return store{}, fmt.Errorf("read trust store %s: %w", path, err) + } + var s store + if err := json.Unmarshal(data, &s); err != nil { + return store{}, fmt.Errorf("parse trust store %s: %w", path, err) + } + return s, nil +} + +// saveStore writes the trust store atomically: normalize and dedupe entries, +// marshal with indentation, then write to a temp file (mode 0o600) in the same +// directory and rename it into place. The parent directory is created with mode +// 0o700. This mirrors the atomic-write-with-perms convention in +// internal/securefile/securefile.go (a plaintext write, no encryption here). +// +// Entries are NOT re-normalized here: they are already canonical (each incoming +// path is normalized once at its Trust/Untrust entry point). Re-resolving stored +// entries through the filesystem would re-follow symlinks at write time and let a +// retargeted symlink drift the stored value. +func saveStore(s store) error { + path, err := storeFilePath() + if err != nil { + return err + } + + // Dedupe and sort so the on-disk form is stable. Entries are treated as + // already-canonical literals, never re-resolved. + seen := make(map[string]struct{}, len(s.Trusted)) + roots := make([]string, 0, len(s.Trusted)) + for _, entry := range s.Trusted { + if _, ok := seen[entry]; ok { + continue + } + seen[entry] = struct{}{} + roots = append(roots, entry) + } + sort.Strings(roots) + + data, err := json.MarshalIndent(store{Trusted: roots}, "", " ") + if err != nil { + return fmt.Errorf("marshal trust store: %w", err) + } + + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { + return fmt.Errorf("create trust store directory %s: %w", dir, err) + } + + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".*.tmp") + if err != nil { + return fmt.Errorf("create trust store temp file: %w", err) + } + tmpPath := tmp.Name() + defer os.Remove(tmpPath) + + if err := tmp.Chmod(0o600); err != nil { + _ = tmp.Close() + return fmt.Errorf("chmod trust store temp file: %w", err) + } + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return fmt.Errorf("write trust store: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("write trust store: %w", err) + } + if err := os.Rename(tmpPath, path); err != nil { + return fmt.Errorf("publish trust store: %w", err) + } + return nil +} + +// IsTrusted reports whether workspaceRoot's own normalized root is in the trust +// store. It is an EXACT membership check: no ancestor or subtree walk, so a +// nested repo under a trusted root is not trusted. An empty root returns +// (false, nil). A missing store returns (false, nil). A store that exists but +// cannot be read returns (false, non-nil) so callers fail closed. +func IsTrusted(workspaceRoot string) (bool, error) { + if workspaceRoot == "" { + return false, nil + } + query, err := normalize(workspaceRoot) + if err != nil { + return false, err + } + s, err := loadStore() + if err != nil { + return false, err + } + // Compare stored entries literally: they are already canonical (normalized at + // Trust time). Re-normalizing them here would re-run EvalSymlinks on every + // check, so a trusted path later replaced by a symlink to attacker content + // would re-resolve to the new target and match (a false positive). Only the + // incoming query is normalized. + for _, entry := range s.Trusted { + if entry == query { + return true, nil + } + } + return false, nil +} + +// Trust adds workspaceRoot's normalized root to the store. It is idempotent: +// trusting an already-trusted root is a no-op that returns nil. +func Trust(workspaceRoot string) error { + norm, err := normalize(workspaceRoot) + if err != nil { + return err + } + s, err := loadStore() + if err != nil { + return err + } + s.Trusted = append(s.Trusted, norm) + return saveStore(s) +} + +// Untrust removes workspaceRoot's normalized root from the store. Removing an +// absent path is a no-op and returns nil. +func Untrust(workspaceRoot string) error { + target, err := normalize(workspaceRoot) + if err != nil { + return err + } + s, err := loadStore() + if err != nil { + return err + } + kept := make([]string, 0, len(s.Trusted)) + for _, entry := range s.Trusted { + if entry == target { + continue + } + kept = append(kept, entry) + } + return saveStore(store{Trusted: kept}) +} + +// List returns the sorted normalized trusted roots. A missing store returns an +// empty slice and a nil error. +func List() ([]string, error) { + s, err := loadStore() + if err != nil { + return nil, err + } + roots := append([]string(nil), s.Trusted...) + sort.Strings(roots) + return roots, nil +} diff --git a/internal/workspacetrust/trust_test.go b/internal/workspacetrust/trust_test.go new file mode 100644 index 000000000..ae070d5bc --- /dev/null +++ b/internal/workspacetrust/trust_test.go @@ -0,0 +1,308 @@ +package workspacetrust + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +// setUserConfigRoot redirects config.UserConfigDir() (via os.UserConfigDir) to a +// throwaway temp dir on every platform, so the trust store never touches the real +// user config directory. It mirrors internal/config/paths_test.go: os.UserConfigDir +// reads APPDATA on Windows, HOME on darwin, and XDG_CONFIG_HOME on Linux, so a single +// env var is not portable. +func setUserConfigRoot(t *testing.T) { + t.Helper() + + root := t.TempDir() + switch runtime.GOOS { + case "windows": + t.Setenv("APPDATA", root) + case "darwin": + t.Setenv("HOME", root) + default: + t.Setenv("XDG_CONFIG_HOME", root) + } +} + +func TestIsTrustedFreshStore(t *testing.T) { + setUserConfigRoot(t) + dir := t.TempDir() + + trusted, err := IsTrusted(dir) + if err != nil { + t.Fatalf("IsTrusted() error = %v, want nil", err) + } + if trusted { + t.Fatalf("IsTrusted() = true, want false for a fresh store") + } + + list, err := List() + if err != nil { + t.Fatalf("List() error = %v, want nil", err) + } + if len(list) != 0 { + t.Fatalf("List() = %v, want empty for a fresh store", list) + } +} + +func TestTrustThenIsTrusted(t *testing.T) { + setUserConfigRoot(t) + dir := t.TempDir() + + if err := Trust(dir); err != nil { + t.Fatalf("Trust() error = %v", err) + } + + trusted, err := IsTrusted(dir) + if err != nil { + t.Fatalf("IsTrusted() error = %v", err) + } + if !trusted { + t.Fatalf("IsTrusted() = false, want true after Trust()") + } +} + +func TestIsTrustedExactMatchNoInheritance(t *testing.T) { + setUserConfigRoot(t) + repo := t.TempDir() + if err := Trust(repo); err != nil { + t.Fatalf("Trust() error = %v", err) + } + + // Real subdirectories of the trusted repo must NOT be trusted. + vendorEvil := filepath.Join(repo, "vendor", "evil") + src := filepath.Join(repo, "src") + if err := os.MkdirAll(vendorEvil, 0o700); err != nil { + t.Fatalf("mkdir %s: %v", vendorEvil, err) + } + if err := os.MkdirAll(src, 0o700); err != nil { + t.Fatalf("mkdir %s: %v", src, err) + } + + other := t.TempDir() + + for _, path := range []string{vendorEvil, src, other} { + trusted, err := IsTrusted(path) + if err != nil { + t.Fatalf("IsTrusted(%q) error = %v", path, err) + } + if trusted { + t.Fatalf("IsTrusted(%q) = true, want false (exact match only, no inheritance)", path) + } + } +} + +func TestTrustIdempotent(t *testing.T) { + setUserConfigRoot(t) + dir := t.TempDir() + + if err := Trust(dir); err != nil { + t.Fatalf("first Trust() error = %v", err) + } + if err := Trust(dir); err != nil { + t.Fatalf("second Trust() error = %v", err) + } + + list, err := List() + if err != nil { + t.Fatalf("List() error = %v", err) + } + if len(list) != 1 { + t.Fatalf("List() = %v, want exactly one entry after two Trust() calls", list) + } +} + +func TestUntrust(t *testing.T) { + setUserConfigRoot(t) + dir := t.TempDir() + + if err := Trust(dir); err != nil { + t.Fatalf("Trust() error = %v", err) + } + if err := Untrust(dir); err != nil { + t.Fatalf("Untrust() error = %v", err) + } + + trusted, err := IsTrusted(dir) + if err != nil { + t.Fatalf("IsTrusted() error = %v", err) + } + if trusted { + t.Fatalf("IsTrusted() = true, want false after Untrust()") + } +} + +func TestUntrustAbsentIsNoOp(t *testing.T) { + setUserConfigRoot(t) + dir := t.TempDir() + + // Untrust on an absent path (empty store) must not error. + if err := Untrust(dir); err != nil { + t.Fatalf("Untrust() on absent path error = %v, want nil", err) + } + + // And also when the store exists but does not contain the path. + other := t.TempDir() + if err := Trust(other); err != nil { + t.Fatalf("Trust() error = %v", err) + } + if err := Untrust(dir); err != nil { + t.Fatalf("Untrust() on absent-but-nonempty store error = %v, want nil", err) + } +} + +func TestNormalizationTrailingDot(t *testing.T) { + setUserConfigRoot(t) + dir := t.TempDir() + + if err := Trust(dir); err != nil { + t.Fatalf("Trust() error = %v", err) + } + + // Query with a non-canonical trailing "/." that resolves to the same target. + query := filepath.Join(dir, ".") + trusted, err := IsTrusted(query) + if err != nil { + t.Fatalf("IsTrusted(%q) error = %v", query, err) + } + if !trusted { + t.Fatalf("IsTrusted(%q) = false, want true (non-canonical form of a trusted path)", query) + } +} + +func TestNormalizationSymlinkAlias(t *testing.T) { + setUserConfigRoot(t) + target := t.TempDir() + if err := Trust(target); err != nil { + t.Fatalf("Trust() error = %v", err) + } + + // A symlink that resolves to the trusted target must match. + link := filepath.Join(t.TempDir(), "alias") + if err := os.Symlink(target, link); err != nil { + t.Skipf("os.Symlink unsupported on this platform: %v", err) + } + + trusted, err := IsTrusted(link) + if err != nil { + t.Fatalf("IsTrusted(%q) error = %v", link, err) + } + if !trusted { + t.Fatalf("IsTrusted(%q) = false, want true (symlink alias of a trusted path)", link) + } +} + +func TestIsTrustedEmptyRoot(t *testing.T) { + setUserConfigRoot(t) + + trusted, err := IsTrusted("") + if err != nil { + t.Fatalf("IsTrusted(\"\") error = %v, want nil", err) + } + if trusted { + t.Fatalf("IsTrusted(\"\") = true, want false") + } +} + +func TestIsTrustedFailClosedOnReadError(t *testing.T) { + setUserConfigRoot(t) + dir := t.TempDir() + + // Create trust.json as a DIRECTORY so os.ReadFile fails with a non-ErrNotExist + // error. This is portable (unlike chmod 0o000, a no-op under root and on Windows). + if err := os.MkdirAll(filepath.Dir(storePath(t)), 0o700); err != nil { + t.Fatalf("mkdir store parent: %v", err) + } + if err := os.MkdirAll(storePath(t), 0o700); err != nil { + t.Fatalf("mkdir store path as directory: %v", err) + } + + trusted, err := IsTrusted(dir) + if err == nil { + t.Fatalf("IsTrusted() error = nil, want non-nil for an unreadable store") + } + if trusted { + t.Fatalf("IsTrusted() = true, want false (fail-closed) on a read error") + } +} + +func TestPersistedFileAndDirModes(t *testing.T) { + setUserConfigRoot(t) + dir := t.TempDir() + + if err := Trust(dir); err != nil { + t.Fatalf("Trust() error = %v", err) + } + + path := storePath(t) + fileInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("stat store file: %v", err) + } + if runtime.GOOS != "windows" { + if got := fileInfo.Mode().Perm(); got != 0o600 { + t.Fatalf("store file mode = %o, want 0600", got) + } + dirInfo, err := os.Stat(filepath.Dir(path)) + if err != nil { + t.Fatalf("stat store dir: %v", err) + } + if got := dirInfo.Mode().Perm(); got != 0o700 { + t.Fatalf("store dir mode = %o, want 0700", got) + } + } +} + +// storePath returns the on-disk trust store path under the redirected config root. +func storePath(t *testing.T) string { + t.Helper() + p, err := storeFilePath() + if err != nil { + t.Fatalf("storeFilePath() error = %v", err) + } + return p +} + +// TestIsTrustedSymlinkRetargetNotTrusted is a regression test for the false +// positive an external adversarial review found: IsTrusted must compare stored +// entries as canonical literals and only normalize the incoming query, so a +// trusted path later replaced by a symlink to attacker content does NOT stay +// trusted. If IsTrusted re-normalizes stored entries, the stored path re-resolves +// through the new symlink and matches (fail-open). +func TestIsTrustedSymlinkRetargetNotTrusted(t *testing.T) { + setUserConfigRoot(t) + base := t.TempDir() + real := filepath.Join(base, "repo") + attacker := filepath.Join(base, "attacker") + if err := os.Mkdir(real, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(attacker, 0o755); err != nil { + t.Fatal(err) + } + + if err := Trust(real); err != nil { + t.Fatalf("Trust(real): %v", err) + } + if ok, err := IsTrusted(real); err != nil || !ok { + t.Fatalf("real dir should be trusted before retarget: ok=%v err=%v", ok, err) + } + + // Replace the trusted real directory with a symlink to attacker content. + if err := os.Remove(real); err != nil { + t.Fatal(err) + } + if err := os.Symlink(attacker, real); err != nil { + t.Skipf("symlink unsupported on this platform: %v", err) + } + + ok, err := IsTrusted(real) + if err != nil { + t.Fatalf("IsTrusted after retarget: %v", err) + } + if ok { + t.Fatal("SECURITY: a trusted path replaced by a symlink to other content is still trusted (false positive)") + } +}