From 870a25ce337cd67d82a35f7fcc0260630b2fea72 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 14:18:38 -0500 Subject: [PATCH 01/17] feat(workspacetrust): add exact-match trust store for project config gating --- internal/workspacetrust/trust.go | 228 ++++++++++++++++++++++ internal/workspacetrust/trust_test.go | 266 ++++++++++++++++++++++++++ 2 files changed, 494 insertions(+) create mode 100644 internal/workspacetrust/trust.go create mode 100644 internal/workspacetrust/trust_test.go diff --git a/internal/workspacetrust/trust.go b/internal/workspacetrust/trust.go new file mode 100644 index 000000000..4851dc1c3 --- /dev/null +++ b/internal/workspacetrust/trust.go @@ -0,0 +1,228 @@ +// 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). Normalizing +// both stored and queried roots is security-critical: without it a relative or +// symlinked path could bypass or forge a 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). +func saveStore(s store) error { + path, err := storeFilePath() + if err != nil { + return err + } + + // Normalize, dedupe, and sort so the on-disk form is stable. + seen := make(map[string]struct{}, len(s.Trusted)) + roots := make([]string, 0, len(s.Trusted)) + for _, entry := range s.Trusted { + norm, nerr := normalize(entry) + if nerr != nil { + return nerr + } + if _, ok := seen[norm]; ok { + continue + } + seen[norm] = struct{}{} + roots = append(roots, norm) + } + 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 + } + for _, entry := range s.Trusted { + norm, nerr := normalize(entry) + if nerr != nil { + return false, nerr + } + if norm == 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 { + norm, nerr := normalize(entry) + if nerr != nil { + return nerr + } + if norm == target { + continue + } + kept = append(kept, norm) + } + 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 := make([]string, 0, len(s.Trusted)) + for _, entry := range s.Trusted { + norm, nerr := normalize(entry) + if nerr != nil { + return nil, nerr + } + roots = append(roots, norm) + } + 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..990a1f443 --- /dev/null +++ b/internal/workspacetrust/trust_test.go @@ -0,0 +1,266 @@ +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 +} From 742811f35d0eca0e54a1feacd08329f5bf4613bd Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 14:21:58 -0500 Subject: [PATCH 02/17] feat(hooks): add ExcludeProject option to LoadConfig --- internal/hooks/hooks.go | 6 +++++ internal/hooks/hooks_test.go | 52 ++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) 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") From 0d87fa39e60c77718f2ac27302242b495aaec4b3 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 14:21:58 -0500 Subject: [PATCH 03/17] feat(plugins): add ExcludeProject option to Load --- internal/plugins/plugins.go | 17 +++++ internal/plugins/plugins_test.go | 106 +++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+) 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 { From c7e685efe782ef259889cd1c3cd393796ca4732d Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 14:34:43 -0500 Subject: [PATCH 04/17] feat(cli): gate project hooks and plugins behind workspace trust Trust check lives inside newHookDispatcherWithExtra and activatePlugins (fail-closed by construction); trustRoot is the original launch dir so a --worktree run inherits the source repo's trust. Callers OR the skip reports and emit one combined stderr notice. --- internal/cli/app.go | 12 +- internal/cli/exec.go | 13 +- internal/cli/exec_spec.go | 8 +- internal/cli/hook_dispatch.go | 92 +++++- internal/cli/plugin_activate.go | 41 ++- internal/cli/plugin_activate_test.go | 17 +- internal/cli/trust_gate_test.go | 438 +++++++++++++++++++++++++++ 7 files changed, 599 insertions(+), 22 deletions(-) create mode 100644 internal/cli/trust_gate_test.go diff --git a/internal/cli/app.go b/internal/cli/app.go index b244b413e..ad32e600a 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -711,7 +711,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 +761,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) return deps.runTUI(context.Background(), tui.Options{ Cwd: workspaceRoot, Version: version, @@ -815,7 +823,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/exec.go b/internal/cli/exec.go index e6761698f..2ba95d7f7 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, @@ -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) } @@ -519,6 +523,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) result, err := agent.Run(runCtx, agentPrompt, provider, agent.Options{ MaxTurns: resolved.MaxTurns, ContextWindow: resolveAgentContextWindow(runCtx, modelRegistry, resolved.Provider), @@ -549,7 +558,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_spec.go b/internal/cli/exec_spec.go index 3fb6c7665..3e9dcd12a 100644 --- a/internal/cli/exec_spec.go +++ b/internal/cli/exec_spec.go @@ -96,6 +96,12 @@ func runExecSpecDraft(run execSpecDraftRun) int { var draftInfo execSpecDraftInfo runCtx, stopSignals := signalContext() defer stopSignals() + // The spec-draft path has no --worktree reassignment and no plugin activation, + // so the trust root is the workspace root itself and the plugin skip is empty. + // Emit at most one notice when project hooks were dropped for an untrusted + // workspace. + hookDispatcher, hookSkip := newHookDispatcher(run.workspaceRoot, run.workspaceRoot) + emitTrustNotice(run.stderr, hookSkip, trustSkip{}) 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 +118,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/hook_dispatch.go b/internal/cli/hook_dispatch.go index 25c09d166..9176edf01 100644 --- a/internal/cli/hook_dispatch.go +++ b/internal/cli/hook_dispatch.go @@ -1,17 +1,56 @@ 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. +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 +59,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 +103,34 @@ 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 and/or plugins were skipped in an untrusted workspace. It is computed once +// per session by the caller (each session-setup site runs once), so it is +// naturally once-per-process. When either 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, hookSkip trustSkip, pluginSkip trustSkip) { + if stderr == nil { + return + } + if !hookSkip.excludedProjectConfig && !pluginSkip.excludedProjectConfig { + return + } + if hookSkip.trustCheckErrored || pluginSkip.trustCheckErrored { + _, _ = fmt.Fprintln(stderr, "zero: the workspace-trust store could not be read; ignoring project hooks/plugins (fail-closed). Run 'zero trust' to enable.") + return + } + _, _ = fmt.Fprintln(stderr, "zero: ignoring project hooks/plugins in an untrusted workspace. Run 'zero trust' to enable.") } 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_gate_test.go b/internal/cli/trust_gate_test.go new file mode 100644 index 000000000..518c6d358 --- /dev/null +++ b/internal/cli/trust_gate_test.go @@ -0,0 +1,438 @@ +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. A single-var switch would +// leave the store pointed at the real config dir on some platforms, so it sets the +// same variable the platform's os.UserConfigDir consults. XDG_DATA_HOME is also +// 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) + case "darwin": + t.Setenv("HOME", 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()) + } + }) +} + +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 +} From fc765142f07def5f4973377e828373e1e0e84a75 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 14:39:13 -0500 Subject: [PATCH 05/17] feat(cli): add zero trust command (trust/list/remove) --- internal/cli/app.go | 2 + internal/cli/trust.go | 115 ++++++++++++++++++++++++++++++ internal/cli/trust_test.go | 140 +++++++++++++++++++++++++++++++++++++ 3 files changed, 257 insertions(+) create mode 100644 internal/cli/trust.go create mode 100644 internal/cli/trust_test.go diff --git a/internal/cli/app.go b/internal/cli/app.go index ad32e600a..5c71e2d15 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -406,6 +406,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": diff --git a/internal/cli/trust.go b/internal/cli/trust.go new file mode 100644 index 000000000..d305fd864 --- /dev/null +++ b/internal/cli/trust.go @@ -0,0 +1,115 @@ +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). 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": + writeTrustUsage(stderr) + return exitUsage + 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 and plugins +(./.zero/hooks.json, ./.zero/plugins/). Trust is exact per directory. +`) +} diff --git a/internal/cli/trust_test.go b/internal/cli/trust_test.go new file mode 100644 index 000000000..bda85cea9 --- /dev/null +++ b/internal/cli/trust_test.go @@ -0,0 +1,140 @@ +package cli + +import ( + "bytes" + "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`") + } + + 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(), cwd) { + t.Fatalf("trust list stdout = %q, want it to contain the cwd %q", out.String(), cwd) + } +} + +// 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()) + } +} From efef8cc2fcc313879f20050d81cd8274ebe3e374 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 15:07:34 -0500 Subject: [PATCH 06/17] fix(review): key spec-draft trust on the original launch dir; cover trust CLI edge branches Spec-draft runs (zero exec --use-spec --worktree) passed the reassigned worktree path as trustRoot, dropping a trusted repo's project hooks (fail-closed, but breaks worktree-inheritance). Thread the captured pre-worktree trustRoot into execSpecDraftRun. Add tests for the trust 'remove' too-many-args and --help branches, and a resolveTrust comment noting the per-chokepoint check must not be hoisted to callers. --- internal/cli/exec.go | 1 + internal/cli/exec_spec.go | 14 ++++++++----- internal/cli/hook_dispatch.go | 7 +++++++ internal/cli/trust_test.go | 38 +++++++++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/internal/cli/exec.go b/internal/cli/exec.go index 2ba95d7f7..a5a11d0af 100644 --- a/internal/cli/exec.go +++ b/internal/cli/exec.go @@ -419,6 +419,7 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in stderr: stderr, deps: deps, workspaceRoot: workspaceRoot, + trustRoot: trustRoot, registry: registry, modelRegistry: modelRegistry, resolved: resolved, diff --git a/internal/cli/exec_spec.go b/internal/cli/exec_spec.go index 3e9dcd12a..73a5089e2 100644 --- a/internal/cli/exec_spec.go +++ b/internal/cli/exec_spec.go @@ -26,6 +26,10 @@ type execSpecDraftRun struct { 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 registry *tools.Registry modelRegistry modelregistry.Registry resolved config.ResolvedConfig @@ -96,11 +100,11 @@ func runExecSpecDraft(run execSpecDraftRun) int { var draftInfo execSpecDraftInfo runCtx, stopSignals := signalContext() defer stopSignals() - // The spec-draft path has no --worktree reassignment and no plugin activation, - // so the trust root is the workspace root itself and the plugin skip is empty. - // Emit at most one notice when project hooks were dropped for an untrusted - // workspace. - hookDispatcher, hookSkip := newHookDispatcher(run.workspaceRoot, run.workspaceRoot) + // The spec-draft path activates no plugins, so the plugin skip is empty. 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 were dropped. + hookDispatcher, hookSkip := newHookDispatcher(run.workspaceRoot, run.trustRoot) emitTrustNotice(run.stderr, hookSkip, trustSkip{}) result, err := agent.Run(runCtx, run.prompt, run.provider, agent.Options{ MaxTurns: run.resolved.MaxTurns, diff --git a/internal/cli/hook_dispatch.go b/internal/cli/hook_dispatch.go index 9176edf01..a10e3efd3 100644 --- a/internal/cli/hook_dispatch.go +++ b/internal/cli/hook_dispatch.go @@ -29,6 +29,13 @@ type trustSkip struct { // 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 diff --git a/internal/cli/trust_test.go b/internal/cli/trust_test.go index bda85cea9..7ab3c0146 100644 --- a/internal/cli/trust_test.go +++ b/internal/cli/trust_test.go @@ -138,3 +138,41 @@ func TestRunTrustUnknownSubcommand(t *testing.T) { 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 stderr, +// nothing to stdout, and return 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 != exitUsage { + t.Fatalf("trust %s returned %d, want %d", flag, code, exitUsage) + } + if !strings.Contains(errBuf.String(), "Usage") { + t.Fatalf("trust %s stderr = %q, want it to contain usage text", flag, errBuf.String()) + } + if out.Len() != 0 { + t.Fatalf("trust %s should not write to stdout, got %q", flag, out.String()) + } + } +} From 05e93dab33374aad7690682fc7004ff332bdc9ec Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 15:34:01 -0500 Subject: [PATCH 07/17] test(cli): end-to-end trust-gate coverage through agent.Run and exec --worktree Drives the real agent loop with a fake tool-calling provider so a project beforeTool hook actually fires (or is gated away) through production code: gate blocks a tool call via agent.Run, and --worktree / --use-spec --worktree key trust on the original launch dir. All three are load-bearing (RED when the gate or the trustRoot threading regresses). --- internal/cli/trust_e2e_test.go | 213 +++++++++++++++++++++++++++++++++ 1 file changed, 213 insertions(+) create mode 100644 internal/cli/trust_e2e_test.go diff --git a/internal/cli/trust_e2e_test.go b/internal/cli/trust_e2e_test.go new file mode 100644 index 000000000..f91724a6b --- /dev/null +++ b/internal/cli/trust_e2e_test.go @@ -0,0 +1,213 @@ +package cli + +import ( + "bytes" + "context" + "os" + "path/filepath" + "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. +func writeMarkerHook(t *testing.T, dir, markerPath string) { + t.Helper() + 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) + } +} + +// 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) + } +} From dce271287225c12b7fe6ba4771b0ec9ba1cffc2a Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 15:45:59 -0500 Subject: [PATCH 08/17] style(cli): gofmt struct alignment in exec_spec.go --- internal/cli/exec_spec.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/cli/exec_spec.go b/internal/cli/exec_spec.go index 73a5089e2..b1a8b5c80 100644 --- a/internal/cli/exec_spec.go +++ b/internal/cli/exec_spec.go @@ -21,11 +21,11 @@ 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. From f4ab12b980e254ca592813548b0fdb60a3d032ab Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 16:04:58 -0500 Subject: [PATCH 09/17] fix(workspacetrust): compare stored trust entries literally, no symlink re-resolution External adversarial review (codex) found a false positive: IsTrusted re-normalized stored entries on every check (Abs+EvalSymlinks), so a trusted path later replaced by a symlink to attacker content re-resolved to the new target and matched. Stored entries are already canonical (normalized once at Trust time); compare them as literals and only normalize the incoming query. Add a regression test for the symlink-retarget case. --- internal/workspacetrust/trust.go | 53 ++++++++++++--------------- internal/workspacetrust/trust_test.go | 42 +++++++++++++++++++++ 2 files changed, 65 insertions(+), 30 deletions(-) diff --git a/internal/workspacetrust/trust.go b/internal/workspacetrust/trust.go index 4851dc1c3..c8adb30e3 100644 --- a/internal/workspacetrust/trust.go +++ b/internal/workspacetrust/trust.go @@ -40,9 +40,10 @@ func storeFilePath() (string, error) { // 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). Normalizing -// both stored and queried roots is security-critical: without it a relative or -// symlinked path could bypass or forge a match. +// 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 { @@ -83,25 +84,27 @@ func loadStore() (store, error) { // 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 } - // Normalize, dedupe, and sort so the on-disk form is stable. + // 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 { - norm, nerr := normalize(entry) - if nerr != nil { - return nerr - } - if _, ok := seen[norm]; ok { + if _, ok := seen[entry]; ok { continue } - seen[norm] = struct{}{} - roots = append(roots, norm) + seen[entry] = struct{}{} + roots = append(roots, entry) } sort.Strings(roots) @@ -156,12 +159,13 @@ func IsTrusted(workspaceRoot string) (bool, error) { 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 { - norm, nerr := normalize(entry) - if nerr != nil { - return false, nerr - } - if norm == query { + if entry == query { return true, nil } } @@ -196,14 +200,10 @@ func Untrust(workspaceRoot string) error { } kept := make([]string, 0, len(s.Trusted)) for _, entry := range s.Trusted { - norm, nerr := normalize(entry) - if nerr != nil { - return nerr - } - if norm == target { + if entry == target { continue } - kept = append(kept, norm) + kept = append(kept, entry) } return saveStore(store{Trusted: kept}) } @@ -215,14 +215,7 @@ func List() ([]string, error) { if err != nil { return nil, err } - roots := make([]string, 0, len(s.Trusted)) - for _, entry := range s.Trusted { - norm, nerr := normalize(entry) - if nerr != nil { - return nil, nerr - } - roots = append(roots, norm) - } + 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 index 990a1f443..ae070d5bc 100644 --- a/internal/workspacetrust/trust_test.go +++ b/internal/workspacetrust/trust_test.go @@ -264,3 +264,45 @@ func storePath(t *testing.T) string { } 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)") + } +} From 47a9ecd652e4ca522bf8d49a89ca35a189381991 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 16:26:13 -0500 Subject: [PATCH 10/17] feat(mcp): gate project-scoped MCP servers behind workspace trust Closes the P0 an external adversarial review (codex) found: project ./.zero/config.json stdio MCP servers spawned before the hook/plugin gate, an ungated arbitrary-execution path that could also self-trust the workspace. Add ExcludeProject to config.ResolveMCP (skips the project layer) and thread a fail-closed trust check into every MCP spawn site (exec, TUI setup+refresh, mcp tools list, mcp check); reporting sites stay ungated. Verified the sole server-command exec (mcp/client.go) is reachable only through the now-gated RegisterTools paths. --- internal/cli/app.go | 13 +- internal/cli/app_test.go | 8 +- internal/cli/backends.go | 8 +- internal/cli/backends_test.go | 8 +- internal/cli/deferred_wiring_test.go | 6 +- internal/cli/exec.go | 2 +- internal/cli/exec_protocol_test.go | 4 +- internal/cli/exec_spec_test.go | 2 +- internal/cli/extensions.go | 9 +- internal/cli/extensions_test.go | 4 +- internal/cli/mcp_commands_test.go | 6 +- internal/cli/mcp_config.go | 7 +- internal/cli/mcp_oauth.go | 6 +- internal/cli/mcp_oauth_test.go | 4 +- internal/cli/mcp_tools.go | 11 +- internal/cli/mcp_trust_test.go | 119 ++++++++++++++++++ .../resolve_mcp_exclude_project_test.go | 61 +++++++++ internal/config/resolver.go | 6 + internal/config/types.go | 5 + 19 files changed, 255 insertions(+), 34 deletions(-) create mode 100644 internal/cli/mcp_trust_test.go create mode 100644 internal/config/resolve_mcp_exclude_project_test.go diff --git a/internal/cli/app.go b/internal/cli/app.go index 5c71e2d15..224d3abe4 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) { @@ -671,7 +672,11 @@ 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. + mcpExcludeProject, _ := resolveTrust(workspaceRoot) + mcpConfig, err := deps.resolveMCPConfig(workspaceRoot, mcpExcludeProject) if err != nil { return writeAppError(stderr, err.Error(), 1) } @@ -806,7 +811,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 } diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index 8e3d58bb4..ce0bd4a10 100644 --- a/internal/cli/app_test.go +++ b/internal/cli/app_test.go @@ -404,7 +404,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 +479,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 +546,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 +601,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) { 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 a5a11d0af..0c20abac4 100644 --- a/internal/cli/exec.go +++ b/internal/cli/exec.go @@ -205,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, err := registerMCPToolsForWorkspace(context.Background(), workspaceRoot, registry, deps, execMCPAutonomy(options), trustRoot) if err != nil { return writeExecProviderError(stdout, stderr, options.outputFormat, "mcp_error", err.Error()) } 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_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..2240cdd89 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,7 +288,10 @@ 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, err := registerMCPToolsForWorkspace(ctx, cwd, registry, deps, mcp.AutonomyLow, cwd) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } 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/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..c3bf6d16c 100644 --- a/internal/cli/mcp_config.go +++ b/internal/cli/mcp_config.go @@ -243,7 +243,12 @@ 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, _ := resolveTrust(cwd) + cfg, err := deps.resolveMCPConfig(cwd, mcpExcludeProject) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } diff --git a/internal/cli/mcp_oauth.go b/internal/cli/mcp_oauth.go index e4ffb2b4d..da11eee75 100644 --- a/internal/cli/mcp_oauth.go +++ b/internal/cli/mcp_oauth.go @@ -202,7 +202,11 @@ func resolveOAuthServer(deps appDeps, serverName string) (mcp.Server, error) { if err != nil { return mcp.Server{}, fmt.Errorf("failed to resolve workspace: %w", err) } - cfg, err := deps.resolveMCPConfig(cwd) + // OAuth login runs an HTTP authorization flow against server.URL and never execs a + // stdio server.Command (resolveOAuthServer requires auth:"oauth", which is URL-based), + // so this cannot run a cloned repo's command. Left ungated (excludeProject=false) like + // the other report/lookup sites; the arbitrary-command surface is the stdio spawn paths. + cfg, err := deps.resolveMCPConfig(cwd, false) if err != nil { return mcp.Server{}, err } diff --git a/internal/cli/mcp_oauth_test.go b/internal/cli/mcp_oauth_test.go index 6310bb074..c93fabf0d 100644 --- a/internal/cli/mcp_oauth_test.go +++ b/internal/cli/mcp_oauth_test.go @@ -122,7 +122,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 +194,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 diff --git a/internal/cli/mcp_tools.go b/internal/cli/mcp_tools.go index 25becdcf5..ba7931479 100644 --- a/internal/cli/mcp_tools.go +++ b/internal/cli/mcp_tools.go @@ -19,8 +19,15 @@ 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. +func registerMCPToolsForWorkspace(ctx context.Context, workspaceRoot string, registry *tools.Registry, deps appDeps, autonomy mcp.PermissionAutonomy, trustRoot string) (mcpToolRuntime, error) { + excludeProject, _ := resolveTrust(trustRoot) + cfg, err := deps.resolveMCPConfig(workspaceRoot, excludeProject) if err != nil { return nil, err } diff --git a/internal/cli/mcp_trust_test.go b/internal/cli/mcp_trust_test.go new file mode 100644 index 000000000..b3c3362f6 --- /dev/null +++ b/internal/cli/mcp_trust_test.go @@ -0,0 +1,119 @@ +package cli + +import ( + "context" + "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, 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) + } +} + +// 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, 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") + } +} + +// 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, 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") + } +} 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 { From 1ed68d77d836d138130a2cdfd65608794e02b384 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 17:40:07 -0500 Subject: [PATCH 11/17] test(cli): cover default-mode beforeTool firing through the trust gate Assert a project beforeTool hook fires in the default auto and ask permission modes (not only under unsafe), and is blocked when the workspace is untrusted, so the gate is verified in the mode a normal run actually uses. --- internal/cli/trust_e2e_test.go | 59 ++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/internal/cli/trust_e2e_test.go b/internal/cli/trust_e2e_test.go index f91724a6b..82d6db527 100644 --- a/internal/cli/trust_e2e_test.go +++ b/internal/cli/trust_e2e_test.go @@ -121,6 +121,65 @@ func TestTrustGateBlocksToolHookThroughAgentRun(t *testing.T) { } } +// 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) + } +} + // 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. From d0fad9054b46e0d121073d251e6d07f6341ee3fc Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 17:40:07 -0500 Subject: [PATCH 12/17] docs(cli): note MCP servers in zero trust help text and doc comment The gate covers project hooks, plugins, and MCP servers, but the help string and runTrust doc comment listed only hooks and plugins. --- internal/cli/trust.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/cli/trust.go b/internal/cli/trust.go index d305fd864..6b79c6cb9 100644 --- a/internal/cli/trust.go +++ b/internal/cli/trust.go @@ -9,9 +9,9 @@ import ( ) // runTrust implements `zero trust`, letting the user opt a workspace into running -// its project-scoped executable config (hooks, plugins). Trust is keyed on the -// exact normalized working directory, matching the exact-match trust model and -// cwd-relative project-config discovery. +// 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 @@ -109,7 +109,8 @@ func writeTrustUsage(w io.Writer) { 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 and plugins -(./.zero/hooks.json, ./.zero/plugins/). Trust is exact per directory. +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. `) } From 879d29291e1e221e40e6421b51676aad0ae63ea9 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 20:26:54 -0500 Subject: [PATCH 13/17] test(cli): make workspace-trust tests portable on macOS and Windows setTrustConfigRoot set HOME on darwin, but config.UserConfigDir is XDG-first there (=$HOME/.config), so tests wrote trust.json and user hooks.json where the code never read them; set XDG_CONFIG_HOME on every non-Windows platform instead. writeMarkerHook shells out to /bin/sh, absent on Windows, so its trusted-case marker never appeared; skip it there like writeMarkerHookScript already does. TestRunTrustAddThenList compared `trust list` output (canonicalized via EvalSymlinks) against the raw cwd, which diverges on the Windows runner's 8.3 short names (RUNNER~1 -> runneradmin); compare the resolved path, and add TestRunTrustListShowsCanonicalPath to exercise that via a symlink on any platform. --- internal/cli/trust_e2e_test.go | 8 ++++- internal/cli/trust_gate_test.go | 17 +++++++---- internal/cli/trust_test.go | 52 +++++++++++++++++++++++++++++++-- 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/internal/cli/trust_e2e_test.go b/internal/cli/trust_e2e_test.go index 82d6db527..4999182bf 100644 --- a/internal/cli/trust_e2e_test.go +++ b/internal/cli/trust_e2e_test.go @@ -5,6 +5,7 @@ import ( "context" "os" "path/filepath" + "runtime" "testing" "github.com/Gitlawb/zero/internal/agent" @@ -67,9 +68,14 @@ func (p toolThenTextProvider) StreamCompletion(_ context.Context, req zeroruntim } // writeMarkerHook writes a ./.zero/hooks.json under dir whose enabled beforeTool -// hook touches markerPath when it fires. +// 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) } diff --git a/internal/cli/trust_gate_test.go b/internal/cli/trust_gate_test.go index 518c6d358..e737c1ede 100644 --- a/internal/cli/trust_gate_test.go +++ b/internal/cli/trust_gate_test.go @@ -16,18 +16,23 @@ import ( // 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. A single-var switch would -// leave the store pointed at the real config dir on some platforms, so it sets the -// same variable the platform's os.UserConfigDir consults. XDG_DATA_HOME is also -// redirected so the hook audit store never touches the user's real data dir. +// 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) - case "darwin": - t.Setenv("HOME", root) default: t.Setenv("XDG_CONFIG_HOME", root) } diff --git a/internal/cli/trust_test.go b/internal/cli/trust_test.go index 7ab3c0146..7fa237246 100644 --- a/internal/cli/trust_test.go +++ b/internal/cli/trust_test.go @@ -2,6 +2,8 @@ package cli import ( "bytes" + "os" + "path/filepath" "strings" "testing" @@ -36,13 +38,59 @@ func TestRunTrustAddThenList(t *testing.T) { 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(), cwd) { - t.Fatalf("trust list stdout = %q, want it to contain the cwd %q", out.String(), cwd) + 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) } } From 5b36bf97d23a5dfe2a006fecc51a75ac490ed766 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 20:27:36 -0500 Subject: [PATCH 14/17] feat(cli): surface the project MCP skip in the workspace-trust notice Project MCP servers were dropped silently in an untrusted workspace: the trust notice covered hooks and plugins but not MCP, and app.go discarded the resolveTrust error. registerMCPToolsForWorkspace now returns a trustSkip, gated on a new projectMCPConfigExists check plus the previously-dropped store-read error, and emitTrustNotice is variadic so the exec, spec-draft, interactive, and `mcp tools list` paths all fold in the MCP skip. The skip drives the notice only; the gate stays on resolveTrust/excludeProject, so a trusted workspace is unaffected and an untrusted one still fails closed. --- internal/cli/app.go | 10 +- internal/cli/app_test.go | 47 ++++++++ internal/cli/exec.go | 5 +- internal/cli/exec_spec.go | 13 ++- internal/cli/extensions.go | 9 +- internal/cli/hook_dispatch.go | 24 ++-- internal/cli/mcp_config.go | 21 ++++ internal/cli/mcp_tools.go | 21 +++- internal/cli/mcp_trust_test.go | 191 +++++++++++++++++++++++++++++++- internal/cli/trust_e2e_test.go | 85 ++++++++++++++ internal/cli/trust_gate_test.go | 23 ++++ 11 files changed, 423 insertions(+), 26 deletions(-) diff --git a/internal/cli/app.go b/internal/cli/app.go index 224d3abe4..64d266020 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -675,7 +675,13 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a // 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. - mcpExcludeProject, _ := resolveTrust(workspaceRoot) + // 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) @@ -772,7 +778,7 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a // 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) + emitTrustNotice(stderr, hookSkip, pluginActivation.trustSkip, mcpSkip) return deps.runTUI(context.Background(), tui.Options{ Cwd: workspaceRoot, Version: version, diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index ce0bd4a10..3e8822923 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" ) @@ -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/exec.go b/internal/cli/exec.go index 0c20abac4..86ceade70 100644 --- a/internal/cli/exec.go +++ b/internal/cli/exec.go @@ -205,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), trustRoot) + 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()) } @@ -420,6 +420,7 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in deps: deps, workspaceRoot: workspaceRoot, trustRoot: trustRoot, + mcpSkip: mcpSkip, registry: registry, modelRegistry: modelRegistry, resolved: resolved, @@ -528,7 +529,7 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in // 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) + emitTrustNotice(stderr, hookSkip, pluginActivation.trustSkip, mcpSkip) result, err := agent.Run(runCtx, agentPrompt, provider, agent.Options{ MaxTurns: resolved.MaxTurns, ContextWindow: resolveAgentContextWindow(runCtx, modelRegistry, resolved.Provider), diff --git a/internal/cli/exec_spec.go b/internal/cli/exec_spec.go index b1a8b5c80..2fe910242 100644 --- a/internal/cli/exec_spec.go +++ b/internal/cli/exec_spec.go @@ -29,7 +29,11 @@ type execSpecDraftRun struct { // 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 + 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 @@ -100,12 +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 empty. Trust + // 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 were dropped. + // 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, trustSkip{}) + 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), diff --git a/internal/cli/extensions.go b/internal/cli/extensions.go index 2240cdd89..1a9adae5c 100644 --- a/internal/cli/extensions.go +++ b/internal/cli/extensions.go @@ -291,11 +291,18 @@ func runMCPTools(ctx context.Context, args []string, stdout io.Writer, stderr io // `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, err := registerMCPToolsForWorkspace(ctx, cwd, registry, deps, mcp.AutonomyLow, 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/hook_dispatch.go b/internal/cli/hook_dispatch.go index a10e3efd3..5d122c900 100644 --- a/internal/cli/hook_dispatch.go +++ b/internal/cli/hook_dispatch.go @@ -124,20 +124,28 @@ func projectHooksFileExists(workspaceRoot string) bool { } // emitTrustNotice writes at most one stderr line summarizing that project-scoped -// hooks and/or plugins were skipped in an untrusted workspace. It is computed once -// per session by the caller (each session-setup site runs once), so it is -// naturally once-per-process. When either surface's skip was a trust-store read +// 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, hookSkip trustSkip, pluginSkip trustSkip) { +func emitTrustNotice(stderr io.Writer, skips ...trustSkip) { if stderr == nil { return } - if !hookSkip.excludedProjectConfig && !pluginSkip.excludedProjectConfig { + excluded := false + storeErrored := false + for _, skip := range skips { + excluded = excluded || skip.excludedProjectConfig + storeErrored = storeErrored || skip.trustCheckErrored + } + if !excluded { return } - if hookSkip.trustCheckErrored || pluginSkip.trustCheckErrored { - _, _ = fmt.Fprintln(stderr, "zero: the workspace-trust store could not be read; ignoring project hooks/plugins (fail-closed). Run 'zero trust' to enable.") + 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 in an untrusted workspace. Run 'zero trust' to enable.") + _, _ = 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_config.go b/internal/cli/mcp_config.go index c3bf6d16c..a4cb715be 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 { diff --git a/internal/cli/mcp_tools.go b/internal/cli/mcp_tools.go index ba7931479..3f933e024 100644 --- a/internal/cli/mcp_tools.go +++ b/internal/cli/mcp_tools.go @@ -25,23 +25,32 @@ type mcpToolListItem struct { // 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. -func registerMCPToolsForWorkspace(ctx context.Context, workspaceRoot string, registry *tools.Registry, deps appDeps, autonomy mcp.PermissionAutonomy, trustRoot string) (mcpToolRuntime, error) { - excludeProject, _ := resolveTrust(trustRoot) +// +// 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_test.go b/internal/cli/mcp_trust_test.go index b3c3362f6..aee12f58b 100644 --- a/internal/cli/mcp_trust_test.go +++ b/internal/cli/mcp_trust_test.go @@ -1,7 +1,11 @@ package cli import ( + "bytes" "context" + "os" + "path/filepath" + "strings" "testing" "github.com/Gitlawb/zero/internal/config" @@ -46,7 +50,7 @@ func TestMCPGateUntrustedExcludesProjectServer(t *testing.T) { deps := mcpTrustDeps(&gotExclude, &spawned) registry := tools.NewRegistry() - runtime, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, repo) + runtime, skip, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, repo) if err != nil { t.Fatalf("registerMCPToolsForWorkspace: %v", err) } @@ -61,6 +65,14 @@ func TestMCPGateUntrustedExcludesProjectServer(t *testing.T) { 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 @@ -77,7 +89,7 @@ func TestMCPGateEmptyTrustRootFailsClosed(t *testing.T) { deps := mcpTrustDeps(&gotExclude, &spawned) registry := tools.NewRegistry() - runtime, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, "") + runtime, skip, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, "") if err != nil { t.Fatalf("registerMCPToolsForWorkspace: %v", err) } @@ -89,6 +101,10 @@ func TestMCPGateEmptyTrustRootFailsClosed(t *testing.T) { 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 @@ -104,7 +120,7 @@ func TestMCPGateTrustedSpawnsProjectServer(t *testing.T) { deps := mcpTrustDeps(&gotExclude, &spawned) registry := tools.NewRegistry() - runtime, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, repo) + runtime, skip, err := registerMCPToolsForWorkspace(context.Background(), repo, registry, deps, mcp.AutonomyLow, repo) if err != nil { t.Fatalf("registerMCPToolsForWorkspace: %v", err) } @@ -116,4 +132,173 @@ func TestMCPGateTrustedSpawnsProjectServer(t *testing.T) { 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") + } } diff --git a/internal/cli/trust_e2e_test.go b/internal/cli/trust_e2e_test.go index 4999182bf..e44f3a3cf 100644 --- a/internal/cli/trust_e2e_test.go +++ b/internal/cli/trust_e2e_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "github.com/Gitlawb/zero/internal/agent" @@ -186,6 +187,90 @@ func TestTrustGateFiresInDefaultAutoMode(t *testing.T) { } } +// 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. diff --git a/internal/cli/trust_gate_test.go b/internal/cli/trust_gate_test.go index e737c1ede..5bfdf61ac 100644 --- a/internal/cli/trust_gate_test.go +++ b/internal/cli/trust_gate_test.go @@ -430,6 +430,29 @@ func TestEmitTrustNoticeOneLineWhenSkipped(t *testing.T) { 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 { From 558d88cf442cf28fc659e74117b38bccacafa562 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 5 Jul 2026 20:27:54 -0500 Subject: [PATCH 15/17] fix(cli): print zero trust --help to stdout with a success exit zero trust -h/--help/help routed usage to stderr with the usage exit code, unlike every other subcommand (mcp, sandbox, skills, cron, ...) which prints help to stdout and returns success. Align it; the unknown-subcommand path still writes to stderr with the usage exit code. --- internal/cli/trust.go | 7 +++++-- internal/cli/trust_test.go | 17 +++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/internal/cli/trust.go b/internal/cli/trust.go index 6b79c6cb9..8b025ab65 100644 --- a/internal/cli/trust.go +++ b/internal/cli/trust.go @@ -26,8 +26,11 @@ func runTrust(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) i case "remove", "rm", "untrust": return trustRemove(args[1:], stdout, stderr, deps) case "-h", "--help", "help": - writeTrustUsage(stderr) - return exitUsage + // 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 diff --git a/internal/cli/trust_test.go b/internal/cli/trust_test.go index 7fa237246..9cd807106 100644 --- a/internal/cli/trust_test.go +++ b/internal/cli/trust_test.go @@ -205,22 +205,23 @@ func TestRunTrustRemoveTooManyArgs(t *testing.T) { } } -// TestRunTrustHelp proves the -h / --help / help subcommands print usage to stderr, -// nothing to stdout, and return the usage exit code. +// 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 != exitUsage { - t.Fatalf("trust %s returned %d, want %d", flag, code, exitUsage) + if code := runTrust([]string{flag}, &out, &errBuf, deps); code != exitSuccess { + t.Fatalf("trust %s returned %d, want %d", flag, code, exitSuccess) } - if !strings.Contains(errBuf.String(), "Usage") { - t.Fatalf("trust %s stderr = %q, want it to contain usage text", flag, errBuf.String()) + if !strings.Contains(out.String(), "Usage") { + t.Fatalf("trust %s stdout = %q, want it to contain usage text", flag, out.String()) } - if out.Len() != 0 { - t.Fatalf("trust %s should not write to stdout, got %q", flag, out.String()) + if errBuf.Len() != 0 { + t.Fatalf("trust %s should not write to stderr, got %q", flag, errBuf.String()) } } } From db9460992a93979e6396e80f6855f1908a48b5b4 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Wed, 8 Jul 2026 07:26:01 -0500 Subject: [PATCH 16/17] fix(cli): gate oauth MCP login and surface mcp-check trust notice Two project-scoped MCP activation sites were left outside the workspace trust gate. zero mcp oauth login loaded a project-defined OAuth server from an untrusted workspace and ran the full discovery/registration/token-exchange flow against its configured URL. Gate resolveOAuthServer with resolveTrust so an untrusted clone's ./.zero/config.json OAuth server is dropped before login, and emit the same one-line trust notice the other spawn sites use. zero mcp check already gated the project layer but discarded the trust-check result and never emitted the notice, so a gated project server read as a bare "not configured". Emit the notice on the not-found branch. The three remaining ungated resolveMCPConfig(cwd, false) sites (backends doctor/snapshot and the extensions listing) are enumeration-only and stay ungated. --- internal/cli/mcp_config.go | 10 +- internal/cli/mcp_oauth.go | 44 +++-- internal/cli/mcp_oauth_test.go | 282 ++++++++++++++++++++++++++++ internal/cli/mcp_trust_edge_test.go | 221 ++++++++++++++++++++++ internal/cli/mcp_trust_test.go | 54 ++++++ 5 files changed, 595 insertions(+), 16 deletions(-) create mode 100644 internal/cli/mcp_trust_edge_test.go diff --git a/internal/cli/mcp_config.go b/internal/cli/mcp_config.go index a4cb715be..603fcd50a 100644 --- a/internal/cli/mcp_config.go +++ b/internal/cli/mcp_config.go @@ -268,13 +268,21 @@ func runMCPCheck(ctx context.Context, args []string, stdout io.Writer, stderr io // 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, _ := resolveTrust(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 da11eee75..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,37 +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) } - // OAuth login runs an HTTP authorization flow against server.URL and never execs a - // stdio server.Command (resolveOAuthServer requires auth:"oauth", which is URL-based), - // so this cannot run a cloned repo's command. Left ungated (excludeProject=false) like - // the other report/lookup sites; the arbitrary-command surface is the stdio spawn paths. - cfg, err := deps.resolveMCPConfig(cwd, false) + 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 c93fabf0d..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 @@ -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_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 index aee12f58b..9bfb8c1ee 100644 --- a/internal/cli/mcp_trust_test.go +++ b/internal/cli/mcp_trust_test.go @@ -302,3 +302,57 @@ func TestMCPGateUntrustedNoticesProjectMCPConfig(t *testing.T) { 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) + } +} From 66b7d9afdd7f829f5ee32e0f723d418cb9b1ec9e Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Thu, 9 Jul 2026 10:07:36 +0800 Subject: [PATCH 17/17] test(cli): adapt MCP startup-skip tests to two-arg resolveMCPConfig --- internal/cli/app_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index 3e8822923..d56bd9063 100644 --- a/internal/cli/app_test.go +++ b/internal/cli/app_test.go @@ -180,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 @@ -216,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