From 702f76c104ea3ea99429e53204c1dae89eb86cc9 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 29 May 2026 22:56:12 -0700 Subject: [PATCH] Tighten config-dir perms and validate plugin scope argv - Create the global config dir at 0700 (it can hold credentials.json) in the skill-refresh and update-notice bootstrap paths, and chmod it once early in root.go so an existing 0755 dir from an older version is tightened. The chmod Lstats first and only touches a real directory (never a symlink), since GlobalConfigDir can fall back to TempDir where a local user could plant one. - Whitelist the plugin --scope value {user,project,local,global} read from installed_plugins.json before passing it to 'claude plugin uninstall', preventing a '-'-leading value from injecting a flag into the argv. --- internal/cli/owner_other.go | 10 ++ internal/cli/owner_unix.go | 23 +++ internal/cli/owner_windows.go | 9 ++ internal/cli/root.go | 83 ++++++++++ internal/cli/root_unix_test.go | 199 ++++++++++++++++++++++++ internal/commands/skill.go | 3 +- internal/commands/update_notice.go | 3 +- internal/commands/wizard_agents.go | 79 ++++++++-- internal/commands/wizard_agents_test.go | 173 ++++++++++++++++++++ 9 files changed, 569 insertions(+), 13 deletions(-) create mode 100644 internal/cli/owner_other.go create mode 100644 internal/cli/owner_unix.go create mode 100644 internal/cli/owner_windows.go create mode 100644 internal/cli/root_unix_test.go create mode 100644 internal/commands/wizard_agents_test.go diff --git a/internal/cli/owner_other.go b/internal/cli/owner_other.go new file mode 100644 index 00000000..61d366e4 --- /dev/null +++ b/internal/cli/owner_other.go @@ -0,0 +1,10 @@ +//go:build !windows && !unix + +package cli + +import "os" + +// isForeignOwned is a no-op on non-Unix, non-Windows platforms (e.g. js/wasm, +// plan9), where the Unix owner model and this TOCTOU vector do not apply; the +// group/world-writable check still runs. +func isForeignOwned(os.FileInfo) bool { return false } diff --git a/internal/cli/owner_unix.go b/internal/cli/owner_unix.go new file mode 100644 index 00000000..0855d5fc --- /dev/null +++ b/internal/cli/owner_unix.go @@ -0,0 +1,23 @@ +//go:build unix + +package cli + +import ( + "os" + "syscall" +) + +// isForeignOwned reports whether fi is owned by a user other than the current +// effective user and other than root. Such an ancestor lets its owner +// substitute a path component and win the Lstat->Chmod TOCTOU race. Current +// mode bits are irrelevant: even if the owner-write bit is clear right now +// (e.g. mode 0555), the owner can always chmod their own directory writable +// after we check, then perform the swap. Root-owned dirs are trusted (root +// already has full access); self-owned dirs are under our control. +func isForeignOwned(fi os.FileInfo) bool { + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return false + } + return st.Uid != uint32(os.Geteuid()) && st.Uid != 0 //nolint:gosec // G115: Geteuid() returns a valid non-negative uid that fits in uint32 +} diff --git a/internal/cli/owner_windows.go b/internal/cli/owner_windows.go new file mode 100644 index 00000000..f538eecb --- /dev/null +++ b/internal/cli/owner_windows.go @@ -0,0 +1,9 @@ +//go:build windows + +package cli + +import "os" + +// isForeignOwned is a no-op on Windows, where the Unix owner model and this +// TOCTOU vector do not apply; the group/world-writable check still runs. +func isForeignOwned(os.FileInfo) bool { return false } diff --git a/internal/cli/root.go b/internal/cli/root.go index 25aa253e..9b3e20d1 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -44,6 +44,33 @@ func NewRootCmd() *cobra.Command { return nil } + // Tighten global config dir perms: an older CLI version (or the + // skill/update bootstrap) may have created it world-listable at + // 0755, yet it can hold credentials.json. Best-effort, runs before + // anything writes into the dir. + // + // Lstat first and only chmod a real directory: os.Chmod follows + // symlinks, and GlobalConfigDir() can fall back to TempDir(), so a + // local attacker could plant a symlink there and redirect the chmod. + if cfgDir := config.GlobalConfigDir(); cfgDir != "" { + // Skip the chmod when any ancestor of cfgDir is world- or + // group-writable (or can't be stat'd): a local user with write + // access to any path component could swap a component for a symlink + // between our Lstat and Chmod, making the pair racy (Lstat→Chmod + // TOCTOU). Checking only the immediate parent is insufficient — a + // world-writable ancestor higher up (e.g. /shared above a 0755 + // /shared/.config) lets the same substitution win the race. Group + // members with write access can win it too, so guard on 0o022 (group + // + world write), not just the world-writable bit. This covers both + // the os.TempDir() fallback and XDG_CONFIG_HOME=/tmp. We only harden + // the dir when no ancestor is attacker-writable. + if !hasWritableAncestor(cfgDir) { + if fi, lstatErr := os.Lstat(cfgDir); lstatErr == nil && fi.IsDir() && fi.Mode()&os.ModeSymlink == 0 { + _ = os.Chmod(cfgDir, 0o700) //nolint:gosec // G302: 0700 is correct for a directory (needs the execute bit) that can hold credentials.json + } + } + } + // Start background update check early so it runs during command execution updateCheck = commands.StartUpdateCheck() @@ -273,6 +300,62 @@ func NewRootCmd() *cobra.Command { return cmd } +// hasWritableChain reports whether any ancestor of path (its parent up to the +// filesystem root) is group- or world-writable, owned by a foreign non-root user, +// or cannot be stat'd. Each ancestor directory is stat'd directly, so a +// world-writable dir that merely contains a symlink component (e.g. /tmp holding +// /tmp/link) is still caught. The foreign-owner check catches an ancestor owned +// by a DIFFERENT non-root local user regardless of its current mode bits (e.g. +// mode 0555 owned by another account): its owner can always chmod the dir +// writable and substitute a path component to win the race, even though no +// write bit is set right now. Root-owned and self-owned ancestors are trusted. +func hasWritableChain(path string) bool { + dir := filepath.Dir(path) + for { + fi, err := os.Stat(dir) + if err != nil || fi.Mode()&0o022 != 0 || isForeignOwned(fi) { + return true + } + parent := filepath.Dir(dir) + if parent == dir { // reached root + return false + } + dir = parent + } +} + +// hasWritableAncestor reports whether the best-effort chmod of path would be unsafe: +// the path is non-absolute, or any ancestor of EITHER the original (lexical) path OR +// the symlink-resolved real path is group- or world-writable, owned by a foreign +// non-root user, or can't be stat'd. Such an ancestor lets another user substitute +// a path component and win the Lstat->Chmod TOCTOU race, so the chmod is skipped +// in that case. The foreign-owner case covers an ancestor that is not +// group/world-writable yet is owned by a DIFFERENT non-root local account: even +// with no write bits set today, that owner can chmod their own dir writable and +// then perform the substitution. +// +// Both chains must be clean. Walking only the resolved chain misses a writable dir +// that holds a symlink component: EvalSymlinks jumps to the symlink target and skips +// the writable dir entirely (e.g. XDG_CONFIG_HOME=/tmp/link pointing into a private +// 0755 tree leaves the world-writable /tmp unexamined), which a local user can swap +// between our Lstat and Chmod. Walking only the lexical chain misses a writable real +// ancestor reached through a symlinked component. A relative path can't be reasoned +// about reliably (its real ancestry depends on cwd), and an unresolvable/missing +// component means EvalSymlinks fails; both are treated conservatively as unsafe. +func hasWritableAncestor(path string) bool { + if !filepath.IsAbs(path) { + return true // can't reason about a relative config dir; treat as unsafe + } + if hasWritableChain(path) { // lexical ancestors of the original path + return true + } + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return true // unresolvable/missing component => be conservative + } + return hasWritableChain(resolved) // ancestors of the real path +} + // Execute runs the root command. func Execute() { cmd := NewRootCmd() diff --git a/internal/cli/root_unix_test.go b/internal/cli/root_unix_test.go new file mode 100644 index 00000000..c544c9d5 --- /dev/null +++ b/internal/cli/root_unix_test.go @@ -0,0 +1,199 @@ +//go:build unix + +package cli + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHasWritableAncestor exercises the TOCTOU guard that gates the best-effort +// chmod of the global config dir in PersistentPreRunE. The chmod is skipped when +// any ancestor — not just the immediate parent — is group/world-writable, since a +// writable ancestor anywhere up the path lets an attacker substitute a path +// component and win the Lstat->Chmod race. +// +// hasWritableAncestor walks the filesystem all the way to root, so a clean +// ("proceeds") case needs a base whose entire ancestry is non-writable. t.TempDir() +// lives under a world-writable /tmp (1777), which would always trip the guard, so +// the tree is built under the user's home dir instead and the proceeds assertion is +// skipped on environments whose HOME itself sits under a writable ancestor. +// +// The helper walks BOTH the lexical ancestor chain of the original path AND the +// chain of the symlink-resolved real path; either chain being writable is unsafe. +// EvalSymlinks requires the path to exist, so cfgDir leaves are created (not just +// their parents) in these cases. It also treats relative and unresolvable paths as +// unsafe. +func TestHasWritableAncestor(t *testing.T) { + home, err := os.UserHomeDir() + require.NoError(t, err) + + // base is a freshly created, 0755 dir under HOME; only its ancestry (HOME and + // above) plus whatever we loosen below can be writable. + base, err := os.MkdirTemp(home, "hwa-test-") + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(base) }) + require.NoError(t, os.Chmod(base, 0o755)) + + t.Run("relative path => unsafe", func(t *testing.T) { + // A relative config dir can't be reasoned about (real ancestry depends on + // cwd), so it must be treated as unsafe regardless of the filesystem. + assert.True(t, hasWritableAncestor(filepath.Join("foo", "basecamp"))) + }) + + t.Run("non-writable ancestors => chmod proceeds", func(t *testing.T) { + if hasWritableAncestor(base) { + t.Skip("HOME has a writable ancestor; cannot demonstrate the proceeds case here") + } + grandparent := filepath.Join(base, "ok-gp") + parent := filepath.Join(grandparent, "parent") + cfgDir := filepath.Join(parent, "basecamp") + require.NoError(t, os.MkdirAll(cfgDir, 0o755)) + + assert.False(t, hasWritableAncestor(cfgDir)) + }) + + t.Run("writable immediate parent => chmod skipped", func(t *testing.T) { + parent := filepath.Join(base, "wp-parent") + cfgDir := filepath.Join(parent, "basecamp") + require.NoError(t, os.MkdirAll(cfgDir, 0o755)) + require.NoError(t, os.Chmod(parent, 0o777)) + + assert.True(t, hasWritableAncestor(cfgDir)) + }) + + t.Run("non-writable parent but writable grandparent => chmod skipped", func(t *testing.T) { + grandparent := filepath.Join(base, "wgp-gp") + parent := filepath.Join(grandparent, "parent") + cfgDir := filepath.Join(parent, "basecamp") + require.NoError(t, os.MkdirAll(cfgDir, 0o755)) + // Loosen the grandparent only; the immediate parent stays 0755. + require.NoError(t, os.Chmod(grandparent, 0o777)) + + assert.True(t, hasWritableAncestor(cfgDir)) + }) + + t.Run("symlink into writable real ancestor => chmod skipped", func(t *testing.T) { + if hasWritableAncestor(base) { + t.Skip("HOME has a writable ancestor; cannot isolate the symlinked-ancestor case here") + } + // Real target lives under a world-writable ancestor (writable/real-cfg), + // but the lexical path to the symlink (safe/link) has only non-writable + // ancestors. Lexical walking would miss the writable ancestor; symlink + // resolution must catch it. + writable := filepath.Join(base, "writable") + realCfg := filepath.Join(writable, "real-cfg") + require.NoError(t, os.MkdirAll(realCfg, 0o755)) + require.NoError(t, os.Chmod(writable, 0o777)) + + safe := filepath.Join(base, "safe") + require.NoError(t, os.MkdirAll(safe, 0o755)) + link := filepath.Join(safe, "link") + if err := os.Symlink(realCfg, link); err != nil { + t.Skipf("symlinks unavailable in this environment: %v", err) + } + + assert.True(t, hasWritableAncestor(link)) + }) + + t.Run("symlink whose lexical parent is writable but target tree is private => chmod skipped", func(t *testing.T) { + if hasWritableAncestor(base) { + t.Skip("HOME has a writable ancestor; cannot isolate the writable-symlink-parent case here") + } + // The dual to the prior case: here the symlink's TARGET tree is entirely + // private (0755) but the world-writable dir holding the symlink is in the + // LEXICAL chain only — EvalSymlinks jumps to the target and skips it, so a + // resolved-only walk returns "safe". A local user with write access to the + // writable dir can swap the symlink between our Lstat and Chmod, so the + // lexical chain must catch it. + realTree := filepath.Join(base, "private-real", "sub") + require.NoError(t, os.MkdirAll(realTree, 0o755)) + + writable := filepath.Join(base, "world-writable") + require.NoError(t, os.MkdirAll(writable, 0o755)) + require.NoError(t, os.Chmod(writable, 0o777)) + link := filepath.Join(writable, "link") + if err := os.Symlink(realTree, link); err != nil { + t.Skipf("symlinks unavailable in this environment: %v", err) + } + + // link/leaf resolves into the private tree, but its lexical parent chain + // runs through the world-writable dir. + assert.True(t, hasWritableAncestor(filepath.Join(link, "leaf"))) + }) +} + +// TestIsForeignOwned exercises the foreign-owner half of the TOCTOU guard +// directly. The helper flags any ancestor owned by a DIFFERENT non-root user +// regardless of its current mode bits (a foreign owner can always chmod their +// own dir writable after the check); root-owned and self-owned dirs are +// trusted. Most cases run as any user; the genuine "true" cases require +// chown'ing to a foreign uid, which only root can do, so they are skipped +// otherwise. +func TestIsForeignOwned(t *testing.T) { + home, err := os.UserHomeDir() + require.NoError(t, err) + base, err := os.MkdirTemp(home, "ifow-test-") + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(base) }) + + t.Run("self-owned 0700 => false", func(t *testing.T) { + dir := filepath.Join(base, "self-0700") + require.NoError(t, os.Mkdir(dir, 0o700)) + fi, err := os.Stat(dir) + require.NoError(t, err) + assert.False(t, isForeignOwned(fi)) + }) + + t.Run("self-owned 0755 => false (uid==self; group/world bits handled elsewhere)", func(t *testing.T) { + dir := filepath.Join(base, "self-0755") + require.NoError(t, os.Mkdir(dir, 0o755)) + require.NoError(t, os.Chmod(dir, 0o755)) + fi, err := os.Stat(dir) + require.NoError(t, err) + // Owned by us, so the foreign-owner check is false; mode bits are + // irrelevant to this helper, and group/world-writability is a separate + // concern handled by hasWritableChain. + assert.False(t, isForeignOwned(fi)) + }) + + t.Run("foreign non-root owner => true", func(t *testing.T) { + // A genuine foreign-owned dir can only be produced by chown'ing to + // another uid, which requires root. Skip otherwise. + if os.Geteuid() != 0 { + t.Skip("need root to chown a dir to a foreign uid") + } + dir := filepath.Join(base, "foreign-0755") + require.NoError(t, os.Mkdir(dir, 0o755)) + // Chown to a non-root, non-self uid (nobody-ish). Skip if it fails. + const foreignUID = 65534 // conventionally "nobody" + if err := os.Chown(dir, foreignUID, foreignUID); err != nil { + t.Skipf("cannot chown to foreign uid %d: %v", foreignUID, err) + } + fi, err := os.Stat(dir) + require.NoError(t, err) + assert.True(t, isForeignOwned(fi)) + }) + + t.Run("foreign non-root owner without write bits (0555) => true", func(t *testing.T) { + // Even with no write bits set, a foreign owner can chmod their own dir + // writable after the check, so the helper must still flag it. + if os.Geteuid() != 0 { + t.Skip("need root to chown a dir to a foreign uid") + } + dir := filepath.Join(base, "foreign-0555") + require.NoError(t, os.Mkdir(dir, 0o755)) + const foreignUID = 65534 // conventionally "nobody" + if err := os.Chown(dir, foreignUID, foreignUID); err != nil { + t.Skipf("cannot chown to foreign uid %d: %v", foreignUID, err) + } + require.NoError(t, os.Chmod(dir, 0o555)) + fi, err := os.Stat(dir) + require.NoError(t, err) + assert.True(t, isForeignOwned(fi)) + }) +} diff --git a/internal/commands/skill.go b/internal/commands/skill.go index 8ce645b8..aa727ef0 100644 --- a/internal/commands/skill.go +++ b/internal/commands/skill.go @@ -344,7 +344,8 @@ func RefreshSkillsIfVersionChanged() bool { // On transient failure, leave the sentinel stale so the next run retries. needsRefresh := baselineSkillInstalled() if !needsRefresh || refreshed { - _ = os.MkdirAll(filepath.Dir(sentinelPath), 0o755) //nolint:gosec // G301: config dir + // 0o700: GlobalConfigDir can hold credentials.json; keep it owner-only. + _ = os.MkdirAll(filepath.Dir(sentinelPath), 0o700) _ = os.WriteFile(sentinelPath, []byte(version.Version), 0o644) //nolint:gosec // G306: not a secret } diff --git a/internal/commands/update_notice.go b/internal/commands/update_notice.go index 71557bfc..a2cc8d90 100644 --- a/internal/commands/update_notice.go +++ b/internal/commands/update_notice.go @@ -130,6 +130,7 @@ func writeUpdateCache(latestVersion string) { return } dir := filepath.Dir(updateCachePath()) - _ = os.MkdirAll(dir, 0o755) //nolint:gosec // G301: config dir + // 0o700: GlobalConfigDir can hold credentials.json; keep it owner-only. + _ = os.MkdirAll(dir, 0o700) _ = os.WriteFile(updateCachePath(), data, 0o644) //nolint:gosec // G306: not a secret } diff --git a/internal/commands/wizard_agents.go b/internal/commands/wizard_agents.go index afa91514..c69d8af8 100644 --- a/internal/commands/wizard_agents.go +++ b/internal/commands/wizard_agents.go @@ -321,7 +321,22 @@ func removeStaleClaudePlugins(ctx context.Context, claudePath string, plugins [] for _, p := range plugins { if len(p.Scopes) > 0 { anyRemoved := false + // someScopeInvalid tracks whether any scope failed validPluginScope + // (i.e. no scoped uninstall could be attempted for it). Those entries + // are unreachable by scoped uninstalls, so the unscoped fallback must + // run even when another, valid scope was removed successfully. We must + // NOT fall back just because a VALID scope's uninstall failed at + // runtime — that would wrongly strip every-scope install when the + // targeted ones merely errored. + someScopeInvalid := false for _, scope := range p.Scopes { + // scope comes from installed_plugins.json (not first-party). + // Whitelist it so a "-"-leading value can't inject a flag into + // the uninstall argv. + if !validPluginScope(scope) { + someScopeInvalid = true + continue + } c := exec.CommandContext(ctx, claudePath, "plugin", "uninstall", p.Key, "--scope", scope) //nolint:gosec // G204: claudePath from FindClaudeBinary if err := c.Run(); err == nil { anyRemoved = true @@ -331,26 +346,68 @@ func removeStaleClaudePlugins(ctx context.Context, claudePath string, plugins [] } } } - if anyRemoved { - removed = append(removed, p.Key) - } - } else { - n := 0 - for i := 0; i < 10; i++ { - c := exec.CommandContext(ctx, claudePath, "plugin", "uninstall", p.Key) //nolint:gosec // G204: claudePath from FindClaudeBinary - if err := c.Run(); err != nil { - break + if someScopeInvalid { + // At least one scope was invalid, so its entry was never targeted + // by a scoped uninstall. Fall back to the unscoped retry removal + // so the plugin isn't silently left installed under that scope. + if uninstallUnscoped(ctx, claudePath, p.Key) { + anyRemoved = true + // The unscoped removal strips EVERY install of the key, + // including valid-scoped ones whose scoped uninstall failed + // at runtime (and thus were never recorded above). Record + // those valid scopes so runClaudeSetup reinstalls them + // instead of silently dropping the user's install. + for _, scope := range p.Scopes { + if validPluginScope(scope) && !scopeSeen[scope] { + scopeSeen[scope] = true + scopes = append(scopes, scope) + } + } } - n++ } - if n > 0 { + if anyRemoved { removed = append(removed, p.Key) } + } else if uninstallUnscoped(ctx, claudePath, p.Key) { + removed = append(removed, p.Key) } } return removed, scopes } +// uninstallUnscoped removes every installation of key by retrying an unscoped +// `claude plugin uninstall` until it fails (entry gone) or a safety cap of 10 +// iterations is reached. Reports whether at least one uninstall succeeded. +func uninstallUnscoped(ctx context.Context, claudePath, key string) bool { + n := 0 + for i := 0; i < 10; i++ { + c := exec.CommandContext(ctx, claudePath, "plugin", "uninstall", key) //nolint:gosec // G204: claudePath from FindClaudeBinary + if err := c.Run(); err != nil { + break + } + n++ + } + return n > 0 +} + +// validPluginScope reports whether scope is one of Claude's accepted plugin +// scopes. Used to gate untrusted scope values from installed_plugins.json +// before they reach `claude plugin uninstall --scope `. +// +// `claude plugin uninstall --scope` only accepts user/project/local, so a +// "global"-scoped entry is invalid here: leaving it valid would make a scoped +// uninstall fail silently while suppressing the unscoped fallback, stranding +// the plugin. Treating "global" as invalid sets someScopeInvalid so the +// unscoped fallback removes it. +func validPluginScope(scope string) bool { + switch scope { + case "user", "project", "local": + return true + default: + return false + } +} + // claudeManualInstallHint returns the two-line manual install instructions. func claudeManualInstallHint(styles *tui.Styles) (string, string) { return styles.Bold.Render(fmt.Sprintf(" claude plugin marketplace add %s", harness.ClaudeMarketplaceSource)), diff --git a/internal/commands/wizard_agents_test.go b/internal/commands/wizard_agents_test.go new file mode 100644 index 00000000..c0e28ff4 --- /dev/null +++ b/internal/commands/wizard_agents_test.go @@ -0,0 +1,173 @@ +package commands + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/harness" +) + +// TestValidPluginScope guards the argv-injection whitelist: scope values come +// from installed_plugins.json (not first-party), so a "-"-leading value must +// not reach `claude plugin uninstall --scope `. +func TestValidPluginScope(t *testing.T) { + valid := []string{"user", "project", "local"} + for _, s := range valid { + if !validPluginScope(s) { + t.Errorf("validPluginScope(%q) = false, want true", s) + } + } + + // "global" is not a scope `claude plugin uninstall --scope` accepts, so it + // must be treated as invalid: keeping it valid would make a scoped uninstall + // fail silently while suppressing the unscoped fallback, stranding the plugin. + invalid := []string{"", "global", "-rf", "--force", "User", "system", "/etc", "user ", " user"} + for _, s := range invalid { + if validPluginScope(s) { + t.Errorf("validPluginScope(%q) = true, want false", s) + } + } +} + +// stubClaudeUninstall writes a stub `claude` binary that logs every invocation +// to logFile and exits with a non-zero code for scoped uninstall calls when +// failScoped is true (otherwise they succeed). Unscoped uninstalls succeed once +// per key then fail (hard-coded), so the retry loop runs once before ending, +// mirroring real "entry gone" behavior. Returns its absolute path. +func stubClaudeUninstall(t *testing.T, failScoped bool) string { + t.Helper() + dir := t.TempDir() + logFile := filepath.Join(dir, "calls.log") + markerDir := filepath.Join(dir, "markers") + require.NoError(t, os.MkdirAll(markerDir, 0o755)) + + scopedExit := "0" + if failScoped { + scopedExit = "1" + } + script := "#!/bin/sh\n" + + "echo \"$*\" >> \"" + logFile + "\"\n" + + "case \"$1 $2\" in\n" + + " \"plugin uninstall\")\n" + + " if [ \"$4\" = \"--scope\" ]; then exit " + scopedExit + "; fi\n" + + // unscoped: succeed once per key, then fail so the retry loop ends. + " MARKER=\"" + markerDir + "/$3.removed\"\n" + + " if [ ! -f \"$MARKER\" ]; then > \"$MARKER\"; exit 0; fi\n" + + " exit 1\n" + + " ;;\n" + + " *) exit 0 ;;\n" + + "esac\n" + path := filepath.Join(dir, "claude") + require.NoError(t, os.WriteFile(path, []byte(script), 0o755)) //nolint:gosec // G306: test helper + return path +} + +func readClaudeCalls(t *testing.T, claudePath string) string { + t.Helper() + data, err := os.ReadFile(filepath.Join(filepath.Dir(claudePath), "calls.log")) + if os.IsNotExist(err) { + return "" + } + require.NoError(t, err) + return string(data) +} + +// TestRemoveStaleClaudePluginsAllScopesInvalid verifies the YL7 fix: when every +// recorded scope fails validPluginScope (no scoped uninstall is attempted), we +// fall back to an unscoped removal so the plugin isn't silently left installed. +func TestRemoveStaleClaudePluginsAllScopesInvalid(t *testing.T) { + claude := stubClaudeUninstall(t, false) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"-rf", "--force"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.NotContains(t, calls, "--scope", "no scoped uninstall should be attempted for invalid scopes") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals", "unscoped fallback should run") + assert.Equal(t, []string{"basecamp@37signals"}, removed) + assert.Empty(t, scopes) +} + +// TestRemoveStaleClaudePluginsGlobalScopeFallsBack verifies that a stale entry +// whose only recorded scope is "global" (which `claude plugin uninstall --scope` +// rejects) is treated as all-invalid, so the unscoped fallback removes it rather +// than leaving it silently installed behind a failing scoped uninstall. +func TestRemoveStaleClaudePluginsGlobalScopeFallsBack(t *testing.T) { + claude := stubClaudeUninstall(t, false) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"global"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.NotContains(t, calls, "--scope", "no scoped uninstall should be attempted for a global scope") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals", "unscoped fallback should run") + assert.Equal(t, []string{"basecamp@37signals"}, removed) + assert.Empty(t, scopes) +} + +// TestRemoveStaleClaudePluginsMixedScopes verifies that when a plugin key has +// BOTH a valid scope and an invalid legacy scope (e.g. "global"), the valid +// scope's successful uninstall does not suppress the unscoped fallback: the +// entry installed under the invalid scope must still be removed, and the key +// must appear in removed exactly once. +func TestRemoveStaleClaudePluginsMixedScopes(t *testing.T) { + claude := stubClaudeUninstall(t, false) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"user", "global"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.Contains(t, calls, "plugin uninstall basecamp@37signals --scope user", + "valid scope should be uninstalled explicitly") + assert.NotContains(t, calls, "--scope global", "invalid scope must never reach the argv") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals\n", + "unscoped fallback should run for the invalid-scope entry") + assert.Equal(t, []string{"basecamp@37signals"}, removed, "key reported once despite two removal paths") + assert.Equal(t, []string{"user"}, scopes, + "valid scope recorded exactly once: the scoped-uninstall success and the fallback must not double-add it") +} + +// TestRemoveStaleClaudePluginsMixedScopesValidUninstallFails verifies that when +// a plugin mixes a VALID scope whose scoped uninstall fails at runtime with an +// INVALID scope, the unscoped fallback (which strips every install of the key, +// including the valid-scoped one) still records the valid scope in the returned +// scopes list so runClaudeSetup reinstalls it instead of silently dropping it. +func TestRemoveStaleClaudePluginsMixedScopesValidUninstallFails(t *testing.T) { + claude := stubClaudeUninstall(t, true) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"project", "global"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.Contains(t, calls, "plugin uninstall basecamp@37signals --scope project", + "valid scope should be attempted explicitly even though it fails") + assert.NotContains(t, calls, "--scope global", "invalid scope must never reach the argv") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals\n", + "unscoped fallback should run for the invalid-scope entry") + assert.Equal(t, []string{"basecamp@37signals"}, removed) + assert.Equal(t, []string{"project"}, scopes, + "valid scope stripped by the unscoped fallback must be recorded exactly once for reinstall") +} + +// TestRemoveStaleClaudePluginsValidScopeUninstallFails verifies the regression +// fix: when scopes are VALID but the scoped uninstall fails at runtime, we must +// NOT fall back to an unscoped removal (which would wrongly strip every scope). +func TestRemoveStaleClaudePluginsValidScopeUninstallFails(t *testing.T) { + claude := stubClaudeUninstall(t, true) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"user", "project"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.Contains(t, calls, "plugin uninstall basecamp@37signals --scope user") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals --scope project") + assert.NotContains(t, calls, "plugin uninstall basecamp@37signals\n", + "no unscoped fallback when valid scopes were attempted but failed") + assert.Empty(t, removed) + assert.Empty(t, scopes) +}