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.

Author: jeremy

internal/cli/owner_unix.go

@@ -0,0 +1,21 @@
+//go:build !windows
+
+package cli
+
+import (
+	"os"
+	"syscall"
+)
+
+// isForeignOwnerWritable reports whether fi is owner-writable and 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. Root-owned dirs are trusted (root already has full access);
+// self-owned dirs are under our control.
+func isForeignOwnerWritable(fi os.FileInfo) bool {
+	st, ok := fi.Sys().(*syscall.Stat_t)
+	if !ok {
+		return false
+	}
+	return fi.Mode()&0o200 != 0 && st.Uid != uint32(os.Geteuid()) && st.Uid != 0 //nolint:gosec // G115: Geteuid() returns a valid non-negative uid that fits in uint32
+}

internal/cli/owner_windows.go

@@ -0,0 +1,9 @@
+//go:build windows
+
+package cli
+
+import "os"
+
+// isForeignOwnerWritable 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 isForeignOwnerWritable(os.FileInfo) bool { return false }

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()
 
@@ -270,6 +297,61 @@ 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
+// with the owner-write bit set, 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 (e.g. mode
+// 0755 owned by another account): its owner can still substitute a path component
+// and win the race even though no group/world-write bit is set. 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 || isForeignOwnerWritable(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 with the owner-write bit set, 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 whose owner-write bit lets that owner 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()

internal/cli/root_test.go

@@ -2,6 +2,8 @@ package cli
 
 import (
 	"bytes"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -59,6 +61,174 @@ func TestLLMEndpointValidation(t *testing.T) {
 	}
 }
 
+// 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")))
+	})
+}
+
+// TestIsForeignOwnerWritable exercises the foreign-owner half of the TOCTOU guard
+// directly. The helper only flags ancestors owned by a DIFFERENT non-root user
+// that still carry the owner-write bit; root-owned and self-owned dirs are trusted
+// regardless of their owner-write bit. Most cases run as any user; the genuine
+// "true" case requires chown'ing to a foreign uid, which only root can do, so it is
+// skipped otherwise.
+func TestIsForeignOwnerWritable(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, isForeignOwnerWritable(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 even with the
+		// owner-write bit set; group/world-writability is a separate concern
+		// handled by hasWritableChain, not this helper.
+		assert.False(t, isForeignOwnerWritable(fi))
+	})
+
+	t.Run("foreign non-root owner with owner-write => true", func(t *testing.T) {
+		// A genuine foreign-owned, owner-writable 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, isForeignOwnerWritable(fi))
+	})
+}
+
 func TestResolvePreferences(t *testing.T) {
 	boolPtr := func(b bool) *bool { return &b }
 	intPtr := func(i int) *int { return &i }

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
 	}
 

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
 }

internal/commands/wizard_agents.go

@@ -321,7 +321,20 @@ func removeStaleClaudePlugins(ctx context.Context, claudePath string, plugins []
 	for _, p := range plugins {
 		if len(p.Scopes) > 0 {
 			anyRemoved := false
+			// allScopesInvalid stays true only when no scope ever passed
+			// validPluginScope (i.e. no scoped uninstall was even attempted).
+			// We must NOT fall back to an unscoped removal just because a
+			// scoped uninstall failed at runtime — that would wrongly strip
+			// every-scope install when the targeted ones merely errored.
+			allScopesInvalid := true
 			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) {
+					continue
+				}
+				allScopesInvalid = false
 				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
@@ -333,6 +346,21 @@ func removeStaleClaudePlugins(ctx context.Context, claudePath string, plugins []
 			}
 			if anyRemoved {
 				removed = append(removed, p.Key)
+			} else if allScopesInvalid {
+				// Every scope was invalid (none passed validPluginScope), so no
+				// scoped uninstall was attempted. Fall back to the unscoped retry
+				// removal so the plugin isn't silently left installed.
+				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
+					}
+					n++
+				}
+				if n > 0 {
+					removed = append(removed, p.Key)
+				}
 			}
 		} else {
 			n := 0
@@ -351,6 +379,24 @@ func removeStaleClaudePlugins(ctx context.Context, claudePath string, plugins []
 	return removed, scopes
 }
 
+// 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 <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 keeps allScopesInvalid true 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)),