diff --git a/internal/cmd/config/clear_legacy_test.go b/internal/cmd/config/clear_legacy_test.go new file mode 100644 index 0000000..6399f83 --- /dev/null +++ b/internal/cmd/config/clear_legacy_test.go @@ -0,0 +1,232 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" + + appconfig "github.com/open-cli-collective/google-readonly/internal/config" + "github.com/open-cli-collective/google-readonly/internal/credtest" +) + +// withSyntheticConfigCandidates overrides configFilesForClearFn so Linux CI +// can exercise the macOS/Windows "old != new" branch with explicit paths. +// Same shape as MON-5373 nrq's credentialFileCandidates seam. +func withSyntheticConfigCandidates(t *testing.T, paths []string) { + t.Helper() + orig := configFilesForClearFn + configFilesForClearFn = func() ([]string, error) { return paths, nil } + t.Cleanup(func() { configFilesForClearFn = orig }) +} + +func seedFile(t *testing.T, path, contents string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("MkdirAll %s: %v", filepath.Dir(path), err) + } + if err := os.WriteFile(path, []byte(contents), 0o600); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} + +func TestConfigClearAll_RemovesAllConfigCandidates(t *testing.T) { + credtest.Setup(t) + tmp := t.TempDir() + newYML := filepath.Join(tmp, "new", "config.yml") + newJSON := filepath.Join(tmp, "new", "config.json") + oldYML := filepath.Join(tmp, "old", "config.yml") + oldJSON := filepath.Join(tmp, "old", "config.json") + for _, p := range []string{newYML, newJSON, oldYML, oldJSON} { + seedFile(t, p, "credential_ref: foo/bar\n") + } + withSyntheticConfigCandidates(t, []string{newYML, newJSON, oldYML, oldJSON}) + + _ = capture(t, func() { + if err := runClear(true, false); err != nil { + t.Fatalf("runClear: %v", err) + } + }) + + for _, p := range []string{newYML, newJSON, oldYML, oldJSON} { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Errorf("--all must remove %s (stat err=%v)", p, err) + } + } +} + +func TestConfigClearAll_DryRunReportsAllCandidates(t *testing.T) { + credtest.Setup(t) + tmp := t.TempDir() + newYML := filepath.Join(tmp, "new", "config.yml") + oldJSON := filepath.Join(tmp, "old", "config.json") + seedFile(t, newYML, "credential_ref: foo/bar\n") + seedFile(t, oldJSON, `{"credential_ref":"legacy/bar"}`) + withSyntheticConfigCandidates(t, []string{newYML, oldJSON}) + + out := capture(t, func() { + if err := runClear(true, true); err != nil { + t.Fatalf("runClear dry-run: %v", err) + } + }) + + for _, p := range []string{newYML, oldJSON} { + if !strings.Contains(out, p) && !strings.Contains(out, appconfig.ShortenPath(p)) { + t.Errorf("--dry-run --all should name %q in output, got:\n%s", p, out) + } + if _, err := os.Stat(p); err != nil { + t.Errorf("--dry-run must not remove %s (stat err=%v)", p, err) + } + } +} + +func TestConfigClearAll_LegacyAbsent_OnlyCanonical(t *testing.T) { + credtest.Setup(t) + tmp := t.TempDir() + newYML := filepath.Join(tmp, "new", "config.yml") + oldYML := filepath.Join(tmp, "old", "config.yml") // never seeded + seedFile(t, newYML, "credential_ref: foo/bar\n") + withSyntheticConfigCandidates(t, []string{newYML, oldYML}) + + out := capture(t, func() { + if err := runClear(true, false); err != nil { + t.Fatalf("runClear: %v", err) + } + }) + + if _, err := os.Stat(newYML); !os.IsNotExist(err) { + t.Errorf("--all must remove canonical %s (stat err=%v)", newYML, err) + } + // Absent legacy must not produce a "Removed" line. We accept silence on + // IsNotExist. + if strings.Contains(out, oldYML) { + t.Errorf("absent %s should not appear in --all output, got:\n%s", oldYML, out) + } +} + +func TestConfigFilesForClear_PathIdentityDedup(t *testing.T) { + credtest.Setup(t) + // Engineer oldDir == newDir by pointing XDG_CONFIG_HOME at the same root + // the canonical resolver uses. credtest.Setup put us under a hermetic + // statedirtest env; on Linux that already produces old==new — the test + // is meaningful there. On macOS/Windows the resolvers diverge by design + // and this assertion just verifies no spurious duplication when they + // happen to coincide (the canonical paths are themselves distinct). + paths, err := configFilesForClear() + if err != nil { + t.Fatalf("configFilesForClear: %v", err) + } + seen := map[string]int{} + for _, p := range paths { + seen[p]++ + } + for p, n := range seen { + if n != 1 { + t.Errorf("path %s appeared %d times; want 1 (per-file dedup)", p, n) + } + } + // Both forms must always be present at the canonical dir (regression pin + // for the Codex r2 minor: canonical config.json was originally missed). + newDir, derr := newDirFor(t) + if derr != nil { + t.Fatalf("newDirFor: %v", derr) + } + wantYML := filepath.Join(newDir, "config.yml") + wantJSON := filepath.Join(newDir, "config.json") + if seen[wantYML] == 0 { + t.Errorf("candidate list missing canonical config.yml at %s; got %v", wantYML, paths) + } + if seen[wantJSON] == 0 { + t.Errorf("candidate list missing canonical config.json at %s; got %v", wantJSON, paths) + } +} + +func TestConfigClearAll_BrokenKeyringOpen_StillScrubsFiles(t *testing.T) { + credtest.Setup(t) + tmp := t.TempDir() + newYML := filepath.Join(tmp, "new", "config.yml") + // Force keychain.OpenNoMigrate to fail via an unknown backend value. + seedFile(t, newYML, "credential_ref: foo/bar\n") + t.Setenv("GOOGLE_READONLY_KEYRING_BACKEND", "this-backend-does-not-exist") + withSyntheticConfigCandidates(t, []string{newYML}) + + out := capture(t, func() { + if err := runClear(true, false); err != nil { + t.Fatalf("runClear under --all must soft-degrade on keyring open failure, got: %v", err) + } + }) + + if _, err := os.Stat(newYML); !os.IsNotExist(err) { + t.Errorf("--all must scrub %s even when keyring open fails (stat err=%v)", newYML, err) + } + if !strings.Contains(out, newYML) && !strings.Contains(out, appconfig.ShortenPath(newYML)) { + t.Errorf("expected runClear to print Removed line for %s, got:\n%s", newYML, out) + } +} + +// TestConfigClearAll_MalformedCanonicalConfig_StillScrubsFiles pins that a +// genuinely malformed config.yml on disk does NOT block --all. The fix +// removed LoadConfig from the runClear path entirely (paths come from the +// candidate helper, not from a parsed Config), so this should be trivially +// true — the test pins it against regression. +func TestConfigClearAll_MalformedCanonicalConfig_StillScrubsFiles(t *testing.T) { + credtest.Setup(t) + tmp := t.TempDir() + newYML := filepath.Join(tmp, "new", "config.yml") + // Genuinely malformed YAML (matches the relocate_test.go fixture shape). + seedFile(t, newYML, "[unclosed_array: yes\n") + withSyntheticConfigCandidates(t, []string{newYML}) + + _ = capture(t, func() { + if err := runClear(true, false); err != nil { + t.Fatalf("--all must tolerate malformed canonical YAML, got: %v", err) + } + }) + + if _, err := os.Stat(newYML); !os.IsNotExist(err) { + t.Errorf("--all must scrub malformed %s (stat err=%v)", newYML, err) + } +} + +// TestConfigFilesForClear_ExcludesOAuthClientJSON pins the deployment- +// material exclusion: clear --all must never list oauth_client.json +// among the files it removes. +func TestConfigFilesForClear_ExcludesOAuthClientJSON(t *testing.T) { + credtest.Setup(t) + paths, err := configFilesForClear() + if err != nil { + t.Fatalf("configFilesForClear: %v", err) + } + for _, p := range paths { + if filepath.Base(p) == appconfig.OAuthClientFile { + t.Errorf("clear --all must NOT remove the OAuth client JSON (deployment material); got %s in %v", p, paths) + } + } +} + +func TestConfigClear_PlainStillHardFailsOnBrokenKeyring(t *testing.T) { + credtest.Setup(t) + t.Setenv("GOOGLE_READONLY_KEYRING_BACKEND", "this-backend-does-not-exist") + + _ = capture(t, func() { + err := runClear(false, false) + if err == nil { + t.Fatal("plain `clear` (no --all) must surface a keyring open failure; got nil") + } + }) +} + +// newDirFor returns the canonical config dir as configFilesForClear would +// resolve it (via the live config.GetConfigDirNoCreate). Kept as a helper +// so tests don't have to import internal/config solely for this. +func newDirFor(t *testing.T) (string, error) { + t.Helper() + paths, err := configFilesForClear() + if err != nil { + return "", err + } + if len(paths) == 0 { + t.Fatal("configFilesForClear returned no paths") + } + return filepath.Dir(paths[0]), nil +} diff --git a/internal/cmd/config/config.go b/internal/cmd/config/config.go index 1f7b825..27c8314 100644 --- a/internal/cmd/config/config.go +++ b/internal/cmd/config/config.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "github.com/spf13/cobra" @@ -18,6 +19,45 @@ import ( "github.com/open-cli-collective/google-readonly/internal/output" ) +// configFilesForClear returns the config files `clear --all` should remove, +// deduped by path-identity (Linux collapses old==new). Symmetric with the +// cache side's CacheDirPath + LegacyCacheDir treatment. Both forms +// (config.yml and the legacy config.json) are listed at both the canonical +// and old hand-rolled dirs because LoadConfig still reads new-dir +// config.json when YAML is absent, and the relocation gate can still copy +// either form forward from the old dir. The OAuth client JSON is +// intentionally excluded (deployment material, per the existing trailing +// note in runClear). Resolves WITHOUT creating directories. +func configFilesForClear() ([]string, error) { + newDir, err := config.GetConfigDirNoCreate() + if err != nil { + return nil, err + } + oldDir, _ := config.OldHandRolledConfigDir() // best-effort: no HOME → skip legacy + + var paths []string + seen := map[string]struct{}{} + add := func(p string) { + if _, dup := seen[p]; dup { + return + } + seen[p] = struct{}{} + paths = append(paths, p) + } + add(filepath.Join(newDir, config.ConfigFileYAML)) + add(filepath.Join(newDir, config.ConfigFile)) + if oldDir != "" { + add(filepath.Join(oldDir, config.ConfigFileYAML)) + add(filepath.Join(oldDir, config.ConfigFile)) + } + return paths, nil +} + +// configFilesForClearFn is the package-var test seam: tests inject a +// synthetic distinct-old/new path list so Linux CI can exercise the +// macOS/Windows "old != new" branch without OS-specific paths. +var configFilesForClearFn = configFilesForClear + // NewCommand returns the config command with subcommands. func NewCommand() *cobra.Command { cmd := &cobra.Command{ @@ -210,33 +250,67 @@ func runTest(cmd *cobra.Command, _ []string) error { } func runClear(all, dryRun bool) error { - // OpenNoMigrate: clear is a §1.8 remediation path ("clear the conflicting - // entry, then re-run") — running migration first would block it. - st, err := keychain.OpenNoMigrate() + // Resolve scrub targets BEFORE opening the keyring (§6.6 pattern 7): + // `--all` is the user's primary recovery path and must not itself be + // blocked by the broken state it exists to wipe. Path resolution is + // side-effect-free; failure here is fatal regardless of --all. + cfgPaths, err := configFilesForClearFn() if err != nil { - return err + return fmt.Errorf("resolving config paths: %w", err) } - defer func() { _ = st.Close() }() - hasTok, err := st.HasToken() + // OpenNoMigrate: clear is a §1.8 remediation path ("clear the conflicting + // entry, then re-run") — running migration first would block it. Under + // --all an open failure (e.g. invalid keyring.backend in a malformed + // canonical config) is soft-degraded so the file scrub still runs. + st, err := keychain.OpenNoMigrate() if err != nil { - return fmt.Errorf("checking stored token: %w", err) + if !all { + return err + } + fmt.Fprintf(os.Stderr, "warning: could not open keyring (%v) — proceeding with file scrub only\n", err) + st = nil } - // NoCreate: clear (incl. --dry-run) must not create the config dir as a - // side-effect. os.Remove below tolerates an absent dir/file. - cfgPath, err := config.GetConfigPathNoCreate() - if err != nil { - return fmt.Errorf("resolving config path: %w", err) + // Nil-guard the deferred close: a later HasToken soft-degrade may set + // st = nil after closing it eagerly. Closing nil would panic. + defer func() { + if st != nil { + _ = st.Close() + } + }() + + hasTok := false + if st != nil { + h, herr := st.HasToken() + switch { + case herr == nil: + hasTok = h + case all: + // Symmetric with the OpenNoMigrate soft-degrade: a HasToken + // failure (e.g. partial backend corruption) under --all must + // not block the file scrub. Surface a warning, drop the + // store reference so token branches below skip cleanly. + fmt.Fprintf(os.Stderr, "warning: could not check stored token (%v) — proceeding with file scrub only\n", herr) + _ = st.Close() + st = nil + default: + return fmt.Errorf("checking stored token: %w", herr) + } } if dryRun { - if hasTok { + switch { + case st == nil: + fmt.Println("Would remove: (keyring unavailable — token state unknown)") + case hasTok: fmt.Printf("Would remove: OAuth token at %s\n", st.Ref()) - } else { + default: fmt.Println("Would remove: (no OAuth token present)") } if all { - fmt.Printf("Would remove: %s\n", config.ShortenPath(cfgPath)) + for _, p := range cfgPaths { + fmt.Printf("Would remove: %s\n", config.ShortenPath(p)) + } // Non-creating resolver: --dry-run must not create or migrate. if cacheDir, cerr := config.CacheDirPath(); cerr == nil { fmt.Printf("Would remove: Drive metadata cache at %s\n", config.ShortenPath(cacheDir)) @@ -247,23 +321,28 @@ func runClear(all, dryRun bool) error { return nil } - if hasTok { + switch { + case st == nil: + // Already warned above; skip token cleanup. + case hasTok: if err := st.DeleteToken(); err != nil { return fmt.Errorf("clearing token: %w", err) } fmt.Printf("Cleared OAuth token from %s.\n", st.Ref()) - } else { + default: fmt.Println("No OAuth token found to clear.") } if all { - switch err := os.Remove(cfgPath); { - case err == nil: - fmt.Printf("Removed %s.\n", config.ShortenPath(cfgPath)) - case os.IsNotExist(err): - fmt.Printf("No %s to remove.\n", config.ShortenPath(cfgPath)) - default: - return fmt.Errorf("removing %s: %w", config.ShortenPath(cfgPath), err) + for _, p := range cfgPaths { + switch err := os.Remove(p); { + case err == nil: + fmt.Printf("Removed %s.\n", config.ShortenPath(p)) + case os.IsNotExist(err): + // not present — fine + default: + return fmt.Errorf("removing %s: %w", config.ShortenPath(p), err) + } } // Drive metadata cache: an explicit full reset removes BOTH the diff --git a/internal/config/config.go b/internal/config/config.go index e9e85db..a840e44 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -117,6 +117,13 @@ func GetConfigDir() (string, error) { return configScope.ConfigDirEnsured() } +// GetConfigDirNoCreate resolves the configuration directory WITHOUT creating +// it — for side-effect-free callers such as `config clear --all` (incl. +// `--dry-run`). Symmetric with GetConfigPathNoCreate. +func GetConfigDirNoCreate() (string, error) { + return configDirPath() +} + // CacheDirPath resolves the OS-designated cache directory WITHOUT creating it // (used by `config clear --all --dry-run` and tests). os.UserCacheDir gives // the canonical per-OS root: Linux $XDG_CACHE_HOME or ~/.cache, macOS diff --git a/internal/config/relocate.go b/internal/config/relocate.go index c4f0006..48d2a35 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -60,6 +60,15 @@ func oldHandRolledConfigDir() (string, error) { return filepath.Join(configHome, DirName), nil } +// OldHandRolledConfigDir is the pre-MON-5371 hand-rolled config directory +// (~/.config/google-readonly on macOS/Windows; identical to the canonical +// dir on Linux). Exposed so cleanup commands can scrub legacy config files +// at this location — symmetric with LegacyCacheDir on the cache side. +// Non-creating. Returns ("", error) if HOME is unresolvable. +func OldHandRolledConfigDir() (string, error) { + return oldHandRolledConfigDir() +} + // OldHandRolledTokenPath is the pre-MON-5371 legacy token.json location. // Exported so keychain.migrate's token-source enumeration can probe it with // full §1.8 conflict semantics (per the MON-5371 plan: token.json is excluded