diff --git a/shared/credstore/credstore.go b/shared/credstore/credstore.go index e789a12..438d334 100644 --- a/shared/credstore/credstore.go +++ b/shared/credstore/credstore.go @@ -25,6 +25,8 @@ import ( "gopkg.in/yaml.v3" + "github.com/open-cli-collective/cli-common/statedir" + "github.com/open-cli-collective/atlassian-go/auth" ) @@ -69,17 +71,20 @@ type Store struct { JTK ToolSection `yaml:"jtk,omitempty"` } -// DefaultPath returns the canonical shared store path, honoring -// $XDG_CONFIG_HOME if set. -func DefaultPath() string { - if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" { - return filepath.Join(xdg, "atlassian-cli", "config.yml") - } - home, err := os.UserHomeDir() +// DefaultPath returns the canonical shared store path. It resolves via +// the shared statedir resolver (os.UserConfigDir()/atlassian-cli), +// which honors $XDG_CONFIG_HOME on Linux and returns the OS-native +// config dir on macOS/Windows. A relative or unresolvable +// $XDG_CONFIG_HOME now returns an error (the §1.1 intentional +// tightening) instead of the prior silent cwd-relative +// `./.atlassian-cli` fallback. Existence-check callers treat the error +// as "no shared file". +func DefaultPath() (string, error) { + dir, err := statedir.Scope{Name: "atlassian-cli"}.ConfigDir() if err != nil { - return filepath.Join(".", ".atlassian-cli", "config.yml") + return "", err } - return filepath.Join(home, ".config", "atlassian-cli", "config.yml") + return filepath.Join(dir, "config.yml"), nil } // Load reads the store at path. An absent file returns an empty Store diff --git a/shared/credstore/credstore_test.go b/shared/credstore/credstore_test.go index 1405aaf..87aecbe 100644 --- a/shared/credstore/credstore_test.go +++ b/shared/credstore/credstore_test.go @@ -8,6 +8,9 @@ import ( "strings" "testing" + "github.com/open-cli-collective/cli-common/statedir" + "github.com/open-cli-collective/cli-common/statedirtest" + "github.com/open-cli-collective/atlassian-go/auth" "github.com/open-cli-collective/atlassian-go/testutil" ) @@ -323,8 +326,36 @@ func TestURLForCFL(t *testing.T) { } } -func TestDefaultPath_HonorsXDG(t *testing.T) { - t.Setenv("XDG_CONFIG_HOME", "/custom/xdg") - got := DefaultPath() - testutil.Equal(t, "/custom/xdg/atlassian-cli/config.yml", got) +func TestDefaultPath_DelegatesToStatedir(t *testing.T) { + root := statedirtest.Hermetic(t) + + got, err := DefaultPath() + if err != nil { + t.Fatalf("DefaultPath: unexpected error: %v", err) + } + dir, derr := statedir.Scope{Name: "atlassian-cli"}.ConfigDir() + if derr != nil { + t.Fatalf("statedir ConfigDir: %v", derr) + } + testutil.Equal(t, filepath.Join(dir, "config.yml"), got) + if !strings.HasPrefix(got, root) { + t.Fatalf("DefaultPath %q escaped hermetic root %q (real-dir leak)", got, root) + } +} + +func TestDefaultPath_RelativeXDGErrorParity(t *testing.T) { + // statedir rejects a relative $XDG_CONFIG_HOME (the §1.1 tightening) + // instead of the prior silent ./.atlassian-cli fallback. The exact + // rejection semantics are owned by cli-common's resolver tests; here + // we only assert DefaultPath faithfully propagates whatever the + // resolver decides on this platform (delegation parity, not a + // hard-coded OS branch). + statedirtest.Hermetic(t) + t.Setenv("XDG_CONFIG_HOME", "relative/not/absolute") + + _, resolverErr := statedir.Scope{Name: "atlassian-cli"}.ConfigDir() + _, gotErr := DefaultPath() + if (gotErr == nil) != (resolverErr == nil) { + t.Fatalf("DefaultPath error parity mismatch: DefaultPath=%v resolver=%v", gotErr, resolverErr) + } } diff --git a/shared/credstore/relocate.go b/shared/credstore/relocate.go new file mode 100644 index 0000000..6763be9 --- /dev/null +++ b/shared/credstore/relocate.go @@ -0,0 +1,349 @@ +package credstore + +import ( + "errors" + "fmt" + "os" + "path/filepath" +) + +// ErrRelocationConflict is the stable identity for a §3.2 old↔new +// shared-config divergence: the prior hand-rolled location and the +// statedir-resolved location both exist but hold different durable +// config. init fails loud naming BOTH absolute paths and mutates +// nothing — it never precedence-picks a winner (no values shown). +var ErrRelocationConflict = errors.New("credstore: prior and current shared config diverge") + +// oldSharedPath reproduces the PRE-statedir hand-rolled shared location +// ($XDG_CONFIG_HOME|~/.config /atlassian-cli/config.yml) so a user who +// configured under the old layout is not silently abandoned by the +// macOS/Windows resolver move. It applies the SAME relative- +// $XDG_CONFIG_HOME rejection as the new resolver: it must NEVER +// reintroduce the old cwd-relative ./.atlassian-cli fallback. A +// relative/unresolvable old base ⇒ ("", nil): the old-shared probe is +// skipped entirely (no enumeration, no copy, no cleanup target), never +// silently cwd-relative. +// "" means the old-shared probe is SKIPPED entirely (relative +// $XDG_CONFIG_HOME, or an unresolvable/relative home) — never a +// cwd-relative fallback. There is intentionally no error return: every +// unusable base collapses to the same "skip" sentinel, and a (string, +// error) signature whose error is structurally always nil would be +// misleading. +func oldSharedPath() string { + if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" { + if !filepath.IsAbs(xdg) { + return "" + } + return filepath.Join(xdg, "atlassian-cli", "config.yml") + } + home, err := os.UserHomeDir() + if err != nil || home == "" || !filepath.IsAbs(home) { + return "" + } + return filepath.Join(home, ".config", "atlassian-cli", "config.yml") +} + +// SharedRelocation is the PURE result of §3.2 old→new shared-config +// detection. It mutates nothing; it is inert until the caller — having +// passed every conflict gate (this relocation check AND the per-tool +// connection-divergence check) — invokes ApplySharedRelocation. OldProj +// is the migration-only projection (same shape as +// LoadSharedLegacyProjection) so the keyring machinery can enumerate / +// scrub a stale plaintext token at the old path without a prior copy. +type SharedRelocation struct { + OldPath string // "" ⇒ no old-shared (skipped / path-identity / absent) + NewPath string // the statedir-resolved canonical path + CopyNeeded bool // old present & new absent ⇒ copy AFTER gates pass + OldProj *SharedLegacyProjection // old file's pre-MON-5328 projection (nil ⇒ no old) +} + +// DetectSharedRelocation is the PURE pre-token detect/enumerate phase. +// It performs NO mutation and NO copy. Contract: +// +// - old skipped (relative XDG / unresolvable home) or path-identical +// to new (Linux: $XDG/~/.config unchanged) ⇒ no-op (dedup; no +// double-read, no self-copy, no double-enumeration). +// - old absent ⇒ no-op. +// - malformed old OR malformed new ⇒ ErrCorruptStore (fail loud, +// mutate nothing; a malformed new is never overwritten). +// - old present, new absent ⇒ CopyNeeded (deferred to the gated apply). +// - both present ⇒ compared on the dedicated relocation projection +// (canonical tool defaults + legacy per-tool conn/token + token +// presence). Identical ⇒ no-op; divergent ⇒ ErrRelocationConflict +// naming BOTH absolute paths, mutating nothing. +func DetectSharedRelocation(newPath string) (*SharedRelocation, error) { + st, err := classifyRelocation(newPath) + if err != nil { + return nil, err + } + r := &SharedRelocation{NewPath: newPath, OldPath: st.oldPath, OldProj: st.oldProj} + switch st.kind { + case relocOldOnly: + r.CopyNeeded = true + case relocBothDivergent: + return nil, relocationConflict(st.oldPath, newPath, "reconcile or remove one, then re-run init") + case relocNone, relocBothEqual: + // no-op: nothing to copy; old (if any) is inert at init + } + return r, nil +} + +// relocKind classifies the old↔new shared-config relationship. The +// divergent case is a KIND (not an error) so the init path can fail +// loud while the runtime path keeps working on the canonical store — +// the policy split lives in the callers, the detection in one place. +type relocKind int + +const ( + relocNone relocKind = iota // no distinct old / old absent / path-identity + relocOldOnly // old present, new absent + relocBothEqual // both present, equal on the projection + relocBothDivergent // both present, divergent +) + +type relocState struct { + oldPath string + oldProj *SharedLegacyProjection + newStore *Store + kind relocKind +} + +// classifyRelocation is the single PURE detection core shared by +// DetectSharedRelocation (init) and loadSharedRuntime (runtime) so a +// fix lands once. Reads only, never mutates. A corrupt old OR new file +// returns ErrCorruptStore (same contract as Load); divergence is a +// kind. Load(oldPath) is deferred to the both-present branch — the +// common old-only first run never reads the old store here. +func classifyRelocation(newPath string) (*relocState, error) { + newStore, err := Load(newPath) + if err != nil { + return nil, err + } + st := &relocState{newStore: newStore, kind: relocNone} + oldPath := oldSharedPath() + if oldPath == "" || oldPath == newPath { + return st, nil + } + oldProj, err := LoadSharedLegacyProjection(oldPath) + if err != nil { + return nil, err + } + if oldProj == nil { + return st, nil + } + st.oldPath = oldPath + st.oldProj = oldProj + + newProj, err := LoadSharedLegacyProjection(newPath) + if err != nil { + return nil, err + } + if newProj == nil { + st.kind = relocOldOnly + return st, nil + } + oldStore, err := Load(oldPath) + if err != nil { + return nil, err + } + if relocationEqual(oldStore, oldProj, newStore, newProj) { + st.kind = relocBothEqual + } else { + st.kind = relocBothDivergent + } + return st, nil +} + +func relocationConflict(oldPath, newPath, remedy string) error { + return fmt.Errorf( + "%w: %s and %s hold different connection or non-secret defaults; "+ + "%s (no values shown; secrets live only in the OS keyring)", + ErrRelocationConflict, oldPath, newPath, remedy) +} + +// relocationEqual is the dedicated relocation-equality projection. It +// covers BOTH (a) the legacy per-tool connection/token fields (which the +// canonical Store drops post-MON-5328, so a pre-migration token/conn +// divergence is not masked) AND (b) the canonical non-secret tool +// defaults (default_space/output_format/default_project, which the +// legacy projection does not carry, so a durable-defaults divergence is +// not masked). Neither projection alone suffices. URLs/auth_method are +// canonicalized so a cosmetic difference does not false-conflict. +func relocationEqual(oS *Store, oP *SharedLegacyProjection, nS *Store, nP *SharedLegacyProjection) bool { + // Token comparison is presence-aware but migration-skew tolerant: a + // token on ONE side only is the EXPECTED pre-/post-migration state + // (the keyring machinery relocates a stale plaintext token), not a + // durable-config divergence. Only TWO DIFFERENT non-empty tokens is + // a true conflict — and the keyring planMigration also fails loud on + // that (defense in depth, consistent fail-loud semantics). + tokenCompatible := func(a, b string) bool { return a == b || a == "" || b == "" } + connEq := func(a, b SharedLegacyConn) bool { + return NormalizeBaseURL(a.URL) == NormalizeBaseURL(b.URL) && + a.Email == b.Email && + tokenCompatible(a.APIToken, b.APIToken) && + canonAuthMethod(a.AuthMethod) == canonAuthMethod(b.AuthMethod) && + a.CloudID == b.CloudID + } + return connEq(oP.Default, nP.Default) && + connEq(oP.CFL, nP.CFL) && + connEq(oP.JTK, nP.JTK) && + oS.CFL.DefaultSpace == nS.CFL.DefaultSpace && + oS.CFL.OutputFormat == nS.CFL.OutputFormat && + oS.JTK.DefaultProject == nS.JTK.DefaultProject +} + +// ApplySharedRelocation is the GATED apply/copy phase. copy-leave-old: +// the old file is intentionally NOT removed (a stale plaintext token +// there is handled by the keyring migration/scrub machinery; a stale +// LATER old write is caught by always-reconcile at the next load rather +// than silently winning). It is a no-op unless detection found an +// old-only file. The new file is written atomically (temp+rename, +// 0700 dir / 0600 file) so a crash never leaves a half-written shared +// config. The caller MUST invoke this only AFTER every conflict gate +// (relocation + per-tool connection divergence) has passed. +func ApplySharedRelocation(r *SharedRelocation) error { + if r == nil || !r.CopyNeeded || r.OldPath == "" { + return nil + } + data, err := os.ReadFile(r.OldPath) //nolint:gosec // CLI relocating its own config + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil + } + return fmt.Errorf("relocating shared config: reading %s: %w", r.OldPath, err) + } + dir := filepath.Dir(r.NewPath) + if err := os.MkdirAll(dir, 0o700); err != nil { + return fmt.Errorf("relocating shared config: creating %s: %w", dir, err) + } + tmp := r.NewPath + ".tmp" + //nolint:gosec // NewPath is the resolver-derived shared config path, not user input + if err := os.WriteFile(tmp, data, 0o600); err != nil { + _ = os.Remove(tmp) + return fmt.Errorf("relocating shared config: writing %s: %w", tmp, err) + } + if err := os.Rename(tmp, r.NewPath); err != nil { + _ = os.Remove(tmp) + return fmt.Errorf("relocating shared config: renaming -> %s: %w", r.NewPath, err) + } + return nil +} + +// LoadSharedRuntime is the READ-ONLY runtime shared-store resolver used +// by the cfl/jtk non-init load paths. It composes the §3.2 relocation +// into normal commands WITHOUT mutating disk (the actual copy is +// init-only and gated behind the per-tool divergence check): +// +// - canonical (new) present, no distinct old / old absent / old≡new +// (Linux) ⇒ the new store. +// - new ABSENT, old present ⇒ the OLD store is read as the effective +// store (transparent read fallback, exactly like a legacy per-tool +// file — no copy on a read path). +// - BOTH present, equal on the relocation projection ⇒ the new store. +// - BOTH present, DIVERGENT ⇒ (new store, ErrRelocationConflict): the +// runtime caller warns once and proceeds on the canonical store +// (commands keep working); `init` is the fail-loud mutating gate. +// - corrupt old OR new ⇒ ErrCorruptStore (same contract as Load; the +// runtime caller warns-once and falls back). +// +// Returning a usable *Store alongside the divergence error lets the +// caller surface the conflict loudly without de-configuring every +// command. +func LoadSharedRuntime() (*Store, error) { + newPath, err := DefaultPath() + if err != nil { + return nil, err + } + return loadSharedRuntime(newPath) +} + +// loadSharedRuntime is LoadSharedRuntime with the canonical path +// injected — the resolve-path vs load-with-relocation split mirrors +// DetectSharedRelocation(newPath) and is what makes the old≠new +// branches hermetically testable on Linux (where the resolver would +// otherwise collapse old≡new). +func loadSharedRuntime(newPath string) (*Store, error) { + st, err := classifyRelocation(newPath) + if err != nil { + return nil, err + } + switch st.kind { + case relocOldOnly: + // Transparent read fallback (no copy on a read path): the old + // store IS the effective config until init materializes it. + oldStore, oerr := Load(st.oldPath) + if oerr != nil { + return nil, oerr + } + return oldStore, nil + case relocBothDivergent: + return st.newStore, relocationConflict(st.oldPath, newPath, "run init to reconcile") + case relocNone, relocBothEqual: + return st.newStore, nil + } + return st.newStore, nil +} + +// OldSharedConnCandidates yields the origin-labeled connection +// candidates contributed by the prior hand-rolled shared file, so the +// per-tool connection-divergence detector COMPOSES with it (a copy is +// gated on this passing — "no copy while a per-tool divergence is +// pending"). It reuses the canonical ConnCandidates assembly (default + +// pre-MON-5328 per-tool effective overrides) over the old file, relabeled +// "prior shared config" so a conflict message names the old path +// distinctly. Empty unless a copy is actually pending (old-only): when +// both files exist and are equal the canonical file already contributes +// its connection, so old-shared would only add redundant duplicates. +func OldSharedConnCandidates(r *SharedRelocation) []NamedConn { + if r == nil || !r.CopyNeeded || r.OldProj == nil || r.OldPath == "" { + return nil + } + oldDef := Section{ + URL: r.OldProj.Default.URL, + Email: r.OldProj.Default.Email, + AuthMethod: r.OldProj.Default.AuthMethod, + CloudID: r.OldProj.Default.CloudID, + } + cands := ConnCandidates(r.OldPath, oldDef, r.OldProj, nil, nil) + for i := range cands { + cands[i].Label = "prior " + cands[i].Label + } + return cands +} + +// OldSharedProjection returns the migration-only projection of the prior +// hand-rolled shared location plus its absolute path, for ADDITIVE +// inclusion in the keyring token-migration source set (so a stale +// plaintext api_token at the old location is enumerated/scrubbed before +// token resolution, never left behind). ("", nil, nil) when there is no +// addressable, distinct, present old-shared file (skipped / path- +// identity with the current resolver / absent). A parse failure returns +// ErrCorruptStore so the secret machinery fails loud rather than +// scrubbing blindly. +func OldSharedProjection(newPath string) (string, *SharedLegacyProjection, error) { + oldPath := oldSharedPath() + if oldPath == "" || oldPath == newPath { + return "", nil, nil + } + proj, err := LoadSharedLegacyProjection(oldPath) + if err != nil { + return "", nil, err + } + if proj == nil { + return "", nil, nil + } + return oldPath, proj, nil +} + +// OldSharedConfigPath returns the prior hand-rolled shared-config path +// when it is addressable AND distinct from the current resolver path +// (path-identity dedup so `config clear --all` does not double-list / +// double-remove the same file on Linux). Existence is the caller's +// concern. "" ⇒ no distinct old-shared path to clear. +func OldSharedConfigPath(newPath string) string { + oldPath := oldSharedPath() + if oldPath == "" || oldPath == newPath { + return "" + } + return oldPath +} diff --git a/shared/credstore/relocate_test.go b/shared/credstore/relocate_test.go new file mode 100644 index 0000000..1492cef --- /dev/null +++ b/shared/credstore/relocate_test.go @@ -0,0 +1,323 @@ +package credstore + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/open-cli-collective/cli-common/statedirtest" +) + +// writeFile writes content creating parent dirs, failing the test on error. +func writeFile(t *testing.T, path, content string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("mkdir %s: %v", filepath.Dir(path), err) + } + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + +// oldBase points oldSharedPath at an absolute temp base distinct from +// whatever newPath the test passes, so the relocation logic is exercised +// on every OS (not only where the resolver actually moved). +func oldBase(t *testing.T) string { + t.Helper() + root := statedirtest.Hermetic(t) + base := filepath.Join(root, "oldbase") + t.Setenv("XDG_CONFIG_HOME", base) + return filepath.Join(base, "atlassian-cli", "config.yml") +} + +func TestOldSharedPath_RelativeXDGSkipped(t *testing.T) { + statedirtest.Hermetic(t) + t.Setenv("XDG_CONFIG_HOME", "relative/not/abs") + if got := oldSharedPath(); got != "" { + t.Fatalf("relative $XDG_CONFIG_HOME must skip the old-shared probe, got %q", got) + } +} + +func TestDetectSharedRelocation_PathIdentityShortCircuit(t *testing.T) { + oldPath := oldBase(t) + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n") + // new == old ⇒ no-op, no double-read, no copy. + rel, err := DetectSharedRelocation(oldPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rel.OldPath != "" || rel.CopyNeeded { + t.Fatalf("path-identity must be a no-op, got %+v", rel) + } +} + +func TestDetectSharedRelocation_OldOnlyCopyNeeded(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n email: u@x.io\n") + + rel, err := DetectSharedRelocation(newPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !rel.CopyNeeded || rel.OldPath != oldPath || rel.OldProj == nil { + t.Fatalf("old-only must set CopyNeeded with OldProj, got %+v", rel) + } + if _, statErr := os.Stat(newPath); !errors.Is(statErr, os.ErrNotExist) { + t.Fatal("detection must not have copied anything (pure phase)") + } + + if err := ApplySharedRelocation(rel); err != nil { + t.Fatalf("apply: %v", err) + } + if _, statErr := os.Stat(newPath); statErr != nil { + t.Fatalf("apply must materialize new path: %v", statErr) + } + if _, statErr := os.Stat(oldPath); statErr != nil { + t.Fatalf("copy-leave-old: old must remain, got %v", statErr) + } +} + +func TestDetectSharedRelocation_BothEqualNoOp(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + body := "default:\n url: https://acme.atlassian.net\n email: u@x.io\ncfl:\n default_space: ENG\n" + writeFile(t, oldPath, body) + writeFile(t, newPath, body) + + rel, err := DetectSharedRelocation(newPath) + if err != nil { + t.Fatalf("identical old/new must be a no-op, got err %v", err) + } + if rel.CopyNeeded { + t.Fatal("identical old/new must not copy") + } +} + +func TestDetectSharedRelocation_BothDivergentConflict(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "default:\n url: https://OLD.atlassian.net\n") + writeFile(t, newPath, "default:\n url: https://NEW.atlassian.net\n") + + _, err := DetectSharedRelocation(newPath) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("divergent old/new must fail loud with ErrRelocationConflict, got %v", err) + } +} + +func TestDetectSharedRelocation_TokenSkewTolerated(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + // Same durable config; token only on old (the expected pre-migration + // state). Must NOT false-conflict. + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n api_token: SECRET\n") + writeFile(t, newPath, "default:\n url: https://acme.atlassian.net\n") + + rel, err := DetectSharedRelocation(newPath) + if err != nil { + t.Fatalf("token-only-on-old must be tolerated, got %v", err) + } + if rel.CopyNeeded { + t.Fatal("both present ⇒ no copy") + } +} + +func TestDetectSharedRelocation_TwoDifferentTokensConflict(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n api_token: TOK_A\n") + writeFile(t, newPath, "default:\n url: https://acme.atlassian.net\n api_token: TOK_B\n") + + _, err := DetectSharedRelocation(newPath) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("two different non-empty tokens must conflict, got %v", err) + } +} + +func TestDetectSharedRelocation_MalformedFailsLoud(t *testing.T) { + t.Run("old", func(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "{not: valid: yaml: ::::") + _, err := DetectSharedRelocation(newPath) + if !errors.Is(err, ErrCorruptStore) { + t.Fatalf("malformed old must fail loud, got %v", err) + } + }) + t.Run("new", func(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n") + writeFile(t, newPath, "{not: valid: yaml: ::::") + _, err := DetectSharedRelocation(newPath) + if !errors.Is(err, ErrCorruptStore) { + t.Fatalf("malformed new must fail loud (never overwritten), got %v", err) + } + // new untouched + b, _ := os.ReadFile(newPath) //nolint:gosec // test reads its own temp file + if string(b) != "{not: valid: yaml: ::::" { + t.Fatal("malformed new must not be mutated") + } + }) +} + +func TestOldSharedConnCandidates_RelabelAndDefaultsCovered(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n email: u@x.io\n") + + rel, err := DetectSharedRelocation(newPath) + if err != nil { + t.Fatalf("detect: %v", err) + } + cands := OldSharedConnCandidates(rel) + if len(cands) == 0 { + t.Fatal("old-shared must contribute a connection candidate") + } + for _, c := range cands { + if c.Label != "prior shared config" { + t.Fatalf("candidate must be relabeled, got %q", c.Label) + } + if c.Path != oldPath { + t.Fatalf("candidate path must name the old file, got %q", c.Path) + } + } +} + +// On Linux oldSharedPath() ≡ DefaultPath() (both honor +// $XDG_CONFIG_HOME), so old-shared MUST NOT be enumerated as a second +// token/clear source for the very same file (Codex r2: "old==new not +// double-enumerated"). These assert the dedup at the seams the keyring +// migrate/clear sets consume. +func TestOldSharedProjection_PathIdentityNotEnumerated(t *testing.T) { + oldPath := oldBase(t) + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n api_token: SECRET\n") + // new == old: the keyring source set already covers this file via + // the canonical DefaultPath; old-shared must contribute nothing. + gotPath, proj, err := OldSharedProjection(oldPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gotPath != "" || proj != nil { + t.Fatalf("path-identity must not double-enumerate, got path=%q proj=%v", gotPath, proj) + } +} + +func TestOldSharedConfigPath_PathIdentityDeduped(t *testing.T) { + oldPath := oldBase(t) + if got := OldSharedConfigPath(oldPath); got != "" { + t.Fatalf("path-identity must dedup the clear path set, got %q", got) + } +} + +func TestOldSharedProjection_EnumeratesDistinctOldToken(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n api_token: STALE\n") + + gotPath, proj, err := OldSharedProjection(newPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gotPath != oldPath || proj == nil || proj.Default.APIToken != "STALE" { + t.Fatalf("distinct old-shared with a token must be enumerated, got path=%q proj=%v", gotPath, proj) + } +} + +// Minor 1 (Codex r2 architectural requirement): the relocation +// projection MUST also catch a durable tool-default divergence — the +// legacy projection alone (conn/token only) would mask it. +func TestDetectSharedRelocation_ToolDefaultDivergenceConflict(t *testing.T) { + for _, tc := range []struct{ name, oldBody, newBody string }{ + {"cfl.default_space", + "cfl:\n default_space: ENG\n", + "cfl:\n default_space: OPS\n"}, + {"cfl.output_format", + "cfl:\n output_format: json\n", + "cfl:\n output_format: table\n"}, + {"jtk.default_project", + "jtk:\n default_project: MON\n", + "jtk:\n default_project: INT\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n"+tc.oldBody) + writeFile(t, newPath, "default:\n url: https://acme.atlassian.net\n"+tc.newBody) + if _, err := DetectSharedRelocation(newPath); !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("durable %s divergence must fail loud, got %v", tc.name, err) + } + }) + } +} + +func TestLoadSharedRuntime_OldOnlyReadFallbackNoCopy(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n email: u@x.io\n") + // new absent: runtime reads old transparently, mutating nothing. + st, err := loadSharedRuntime(newPath) + if err != nil { + t.Fatalf("old-only runtime read: %v", err) + } + if st.Default.URL != "https://acme.atlassian.net" || st.Default.Email != "u@x.io" { + t.Fatalf("old-only must be read as the effective store, got %+v", st.Default) + } + if _, statErr := os.Stat(newPath); !errors.Is(statErr, os.ErrNotExist) { + t.Fatal("runtime read path must NOT copy/create the new file") + } +} + +func TestLoadSharedRuntime_DivergentReturnsCanonicalPlusError(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + writeFile(t, oldPath, "default:\n url: https://OLD.atlassian.net\n") + writeFile(t, newPath, "default:\n url: https://NEW.atlassian.net\n") + + st, err := loadSharedRuntime(newPath) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("divergence must be surfaced, got %v", err) + } + if st == nil || st.Default.URL != "https://NEW.atlassian.net" { + t.Fatalf("divergence must still yield the canonical store so commands work, got %+v", st) + } +} + +func TestLoadSharedRuntime_BothEqualUsesCanonical(t *testing.T) { + oldPath := oldBase(t) + newPath := filepath.Join(t.TempDir(), "new", "config.yml") + body := "default:\n url: https://acme.atlassian.net\n email: u@x.io\n" + writeFile(t, oldPath, body) + writeFile(t, newPath, body) + st, err := loadSharedRuntime(newPath) + if err != nil { + t.Fatalf("equal old/new must be clean, got %v", err) + } + if st.Default.Email != "u@x.io" { + t.Fatalf("unexpected store %+v", st.Default) + } +} + +func TestLoadSharedRuntime_PathIdentityUsesCanonical(t *testing.T) { + // Linux reality: old≡new ⇒ no fallback, no double-handling. + oldPath := oldBase(t) + writeFile(t, oldPath, "default:\n url: https://acme.atlassian.net\n") + st, err := loadSharedRuntime(oldPath) + if err != nil { + t.Fatalf("path-identity must be clean, got %v", err) + } + if st.Default.URL != "https://acme.atlassian.net" { + t.Fatalf("unexpected store %+v", st.Default) + } +} + +func TestApplySharedRelocation_NoOpWhenNotNeeded(t *testing.T) { + if err := ApplySharedRelocation(nil); err != nil { + t.Fatalf("nil ⇒ no-op, got %v", err) + } + if err := ApplySharedRelocation(&SharedRelocation{}); err != nil { + t.Fatalf("not-needed ⇒ no-op, got %v", err) + } +} diff --git a/shared/credtest/credtest.go b/shared/credtest/credtest.go index 9c99c9a..ea85bf9 100644 --- a/shared/credtest/credtest.go +++ b/shared/credtest/credtest.go @@ -8,11 +8,14 @@ package credtest import ( "os" + "path/filepath" "sort" "testing" cccredstore "github.com/open-cli-collective/cli-common/credstore" + "github.com/open-cli-collective/cli-common/statedirtest" + "github.com/open-cli-collective/atlassian-go/credstore" "github.com/open-cli-collective/atlassian-go/keyring" ) @@ -106,14 +109,19 @@ var tokenEnvVars = []string{ } // Hermetic isolates the process credential environment for the duration -// of t and returns the temp directory used as HOME/XDG_CONFIG_HOME. -// All mutations use t.Setenv / t.Cleanup, so they auto-revert. +// of t and returns the temp ROOT. Directory isolation delegates to the +// canonical cli-common statedirtest 7-var harness (HOME/USERPROFILE/ +// AppData/LocalAppData/XDG_CONFIG_HOME/XDG_CACHE_HOME/XDG_DATA_HOME) so +// os.UserConfigDir/os.UserCacheDir never resolve to the developer's real +// directories on ANY OS — HOME/XDG-only isolation was a macOS/Windows +// real-dir leak. Callers MUST derive the shared config path from +// SharedConfigPath(t) (the resolver), never hand-build a layout. All +// mutations use t.Setenv / t.Cleanup, so they auto-revert. Not usable +// under t.Parallel (t.Setenv). func Hermetic(t *testing.T) string { t.Helper() - dir := t.TempDir() + root := statedirtest.Hermetic(t) - t.Setenv("HOME", dir) - t.Setenv("XDG_CONFIG_HOME", dir) // Force the portable encrypted-file backend so tests never touch (or // prompt for) the real OS keychain. t.Setenv(keyring.BackendEnvVar, "file") @@ -126,7 +134,24 @@ func Hermetic(t *testing.T) string { keyring.ResetCorruptWarnOnce() t.Cleanup(keyring.ResetMigrationNotice) t.Cleanup(keyring.ResetCorruptWarnOnce) - return dir + return root +} + +// SharedConfigPath resolves the shared store path through the production +// resolver (credstore.DefaultPath) under the active hermetic env and +// ensures its parent dir exists. Tests seed/inspect the shared store +// here instead of hand-building "/atlassian-cli/config.yml", which +// only matched the resolver on Linux. Call after Hermetic(t). +func SharedConfigPath(t *testing.T) string { + t.Helper() + p, err := credstore.DefaultPath() + if err != nil { + t.Fatalf("credtest.SharedConfigPath: resolving shared path: %v", err) + } + if err := os.MkdirAll(filepath.Dir(p), 0o700); err != nil { + t.Fatalf("credtest.SharedConfigPath: mkdir %s: %v", filepath.Dir(p), err) + } + return p } // SeedToken stores the shared api_token in the hermetic keyring (the same diff --git a/shared/go.mod b/shared/go.mod index 987ad25..b014ffd 100644 --- a/shared/go.mod +++ b/shared/go.mod @@ -5,7 +5,7 @@ go 1.24.0 require ( github.com/fatih/color v1.18.0 github.com/gohugoio/hugo-goldmark-extensions/extras v0.6.0 - github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 + github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d github.com/yuin/goldmark v1.7.16 golang.org/x/term v0.3.0 gopkg.in/yaml.v3 v3.0.1 diff --git a/shared/go.sum b/shared/go.sum index 2ea6c95..f9bbd9d 100644 --- a/shared/go.sum +++ b/shared/go.sum @@ -29,8 +29,8 @@ github.com/mtibben/percent v0.2.1 h1:5gssi8Nqo8QU/r2pynCm+hBQHpkB/uNK7BJCFogWdzs github.com/mtibben/percent v0.2.1/go.mod h1:KG9uO+SZkUp+VkRHsCdYQV3XSZrrSpR3O9ibNBTZrns= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e h1:fD57ERR4JtEqsWbfPhv4DMiApHyliiK5xCTNVSPiaAs= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= -github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 h1:78EW5uCbAzbAO32+oY4HDaFOqS2sPYnc4AT+G5UjdL0= -github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds= +github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d h1:fiyfxc/Wvpuen/u6Mk3bCUG0DLyOylF0K+eol4y9C64= +github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= diff --git a/shared/keyring/clear.go b/shared/keyring/clear.go index 03034b5..5a3cddd 100644 --- a/shared/keyring/clear.go +++ b/shared/keyring/clear.go @@ -39,6 +39,13 @@ type ClearPlan struct { // itself is unopenable (the recovery path). SharedConfigPath string LegacyPaths []string + + // OldSharedConfigPath is the prior hand-rolled shared-config + // location (§3.2), distinct from SharedConfigPath only on the + // macOS/Windows resolver move (path-identity deduped on Linux). + // `--all` deletes it wholesale like SharedConfigPath so a stale + // plaintext token there is not stranded after relocation. + OldSharedConfigPath string } // PlanClear computes the ClearPlan for tool and, on success, returns the @@ -72,8 +79,13 @@ func PlanClear(tool string, all bool) (ClearPlan, *Store, error) { p.EnvActive = append(p.EnvActive, name) } } - if sp := credstore.DefaultPath(); fileExists(sp) { - p.SharedConfigPath = sp + if sp, perr := credstore.DefaultPath(); perr == nil { + if fileExists(sp) { + p.SharedConfigPath = sp + } + if op := credstore.OldSharedConfigPath(sp); op != "" && fileExists(op) { + p.OldSharedConfigPath = op + } } for _, lp := range []string{credstore.LegacyCFLPath(), credstore.LegacyJTKPath()} { if lp != "" && fileExists(lp) { @@ -146,9 +158,22 @@ func ClearFiles() error { if err := scrubLegacyFile(credstore.LegacyJTKPath()); err != nil { errs = append(errs, err) } - if sp := credstore.DefaultPath(); fileExists(sp) { - if err := os.Remove(sp); err != nil && !os.IsNotExist(err) { - errs = append(errs, err) + if sp, perr := credstore.DefaultPath(); perr != nil { + // `clear --all` is the recovery path: an unresolvable shared + // path (e.g. relative $XDG_CONFIG_HOME) must NOT report success + // while a plaintext token may still sit at an unreachable + // location — surface it instead of silently dropping it. + errs = append(errs, fmt.Errorf("resolving shared config path: %w", perr)) + } else { + if fileExists(sp) { + if err := os.Remove(sp); err != nil && !os.IsNotExist(err) { + errs = append(errs, err) + } + } + if op := credstore.OldSharedConfigPath(sp); op != "" && fileExists(op) { + if err := os.Remove(op); err != nil && !os.IsNotExist(err) { + errs = append(errs, err) + } } } return errors.Join(errs...) diff --git a/shared/keyring/keyring_e2e_test.go b/shared/keyring/keyring_e2e_test.go index 091af63..d5548b8 100644 --- a/shared/keyring/keyring_e2e_test.go +++ b/shared/keyring/keyring_e2e_test.go @@ -10,19 +10,22 @@ import ( "testing" cccredstore "github.com/open-cli-collective/cli-common/credstore" + "github.com/open-cli-collective/cli-common/statedirtest" "github.com/open-cli-collective/atlassian-go/credstore" ) -// hermetic isolates HOME/XDG and forces the encrypted-file backend so -// these tests never touch (or prompt for) the real OS keychain. It is a -// local copy of credtest.Hermetic — keyring cannot import credtest -// (credtest imports keyring; that would be an import cycle). +// hermetic isolates the full cli-common statedirtest 7-var env set +// (HOME/USERPROFILE/AppData/LocalAppData/XDG_*) so os.UserConfigDir +// never resolves to a real directory on ANY OS, and forces the +// encrypted-file backend so these tests never touch (or prompt for) the +// real OS keychain. keyring cannot import credtest (credtest imports +// keyring — an import cycle), so it composes statedirtest directly. +// Tests MUST derive the shared path from sharedConfigPath(t), never +// hand-build a layout. Returns the temp root. func hermetic(t *testing.T) string { t.Helper() - dir := t.TempDir() - t.Setenv("HOME", dir) - t.Setenv("XDG_CONFIG_HOME", dir) + root := statedirtest.Hermetic(t) t.Setenv(BackendEnvVar, "file") t.Setenv("ATLASSIAN_CLI_KEYRING_PASSPHRASE", "e2e-passphrase") for _, v := range []string{"ATLASSIAN_API_TOKEN", "CFL_API_TOKEN", "JIRA_API_TOKEN"} { @@ -32,7 +35,23 @@ func hermetic(t *testing.T) string { ResetCorruptWarnOnce() t.Cleanup(ResetMigrationNotice) t.Cleanup(ResetCorruptWarnOnce) - return dir + return root +} + +// sharedConfigPath resolves the shared store path through the production +// resolver under the active hermetic env and ensures its parent exists. +// Replaces hand-building "/atlassian-cli/config.yml", which only +// matched the resolver on Linux. +func sharedConfigPath(t *testing.T) string { + t.Helper() + p, err := credstore.DefaultPath() + if err != nil { + t.Fatalf("sharedConfigPath: %v", err) + } + if err := os.MkdirAll(filepath.Dir(p), 0o700); err != nil { + t.Fatalf("sharedConfigPath mkdir: %v", err) + } + return p } //nolint:gosec // G101: test fixture string, not a real credential @@ -165,9 +184,9 @@ func TestSetCredential_Rejections(t *testing.T) { // into the keyring, the file is scrubbed, the signal fires exactly once, // and the secret never appears in the signal text. func TestMigration_EndToEnd_ScrubAndSignal(t *testing.T) { - dir := hermetic(t) + hermetic(t) - sharedPath := filepath.Join(dir, "atlassian-cli", "config.yml") + sharedPath := sharedConfigPath(t) // credstore.Save strips the token, so write a pre-migration file by // hand to stand in for a real legacy plaintext store. if err := os.MkdirAll(filepath.Dir(sharedPath), 0o700); err != nil { @@ -286,8 +305,8 @@ func TestMigration_DuplicateLegacy_CollapsesToSingleKey(t *testing.T) { // a secret winner. Two-phase guarantee: nothing is written and NO source // is scrubbed; the error names every location and never the value. func TestMigration_DivergentSources_ConflictNoMutation(t *testing.T) { - dir := hermetic(t) - sharedPath := filepath.Join(dir, "atlassian-cli", "config.yml") + hermetic(t) + sharedPath := sharedConfigPath(t) if err := os.MkdirAll(filepath.Dir(sharedPath), 0o700); err != nil { t.Fatal(err) } @@ -546,7 +565,7 @@ func TestResolveToken_CorruptSharedConfig_DegradesGracefully(t *testing.T) { if err := SetCredential(strings.NewReader(secret), ""); err != nil { t.Fatalf("seed: %v", err) } - sharedPath := filepath.Join(dir, "atlassian-cli", "config.yml") + sharedPath := sharedConfigPath(t) if err := os.MkdirAll(filepath.Dir(sharedPath), 0o700); err != nil { t.Fatal(err) } @@ -595,12 +614,9 @@ func TestResolveToken_CorruptSharedConfig_DegradesGracefully(t *testing.T) { // writeLegacyDefault writes a pre-migration shared config.yml with one // plaintext default api_token (credstore.Save strips tokens, so the // fixture is hand-written). -func writeLegacyDefault(t *testing.T, dir, token string) string { +func writeLegacyDefault(t *testing.T, token string) string { t.Helper() - p := filepath.Join(dir, "atlassian-cli", "config.yml") - if err := os.MkdirAll(filepath.Dir(p), 0o700); err != nil { - t.Fatal(err) - } + p := sharedConfigPath(t) if err := os.WriteFile(p, []byte("default:\n url: https://acme.atlassian.net\n api_token: "+token+"\n"), 0o600); err != nil { t.Fatal(err) @@ -613,8 +629,8 @@ func writeLegacyDefault(t *testing.T, dir, token string) string { // write hits ErrExists; re-resolve sees an equal value → benign, // migration completes (plaintext scrubbed), nothing clobbered. func TestMigration_ConcurrentWriter_IdenticalValue_Benign(t *testing.T) { - dir := hermetic(t) - shared := writeLegacyDefault(t, dir, secret) + hermetic(t) + shared := writeLegacyDefault(t, secret) migrationApplyHook = func(s *Store) { if err := s.cs.Set(s.profile, KeyAPIToken, secret, cccredstore.WithOverwrite()); err != nil { @@ -656,10 +672,10 @@ func TestMigration_ConcurrentWriter_IdenticalValue_Benign(t *testing.T) { // re-resolve sees a different value → hard conflict naming the keyring // value, the racer's token is NOT clobbered, and no plaintext is scrubbed. func TestMigration_ConcurrentWriter_DivergentValue_ConflictNoClobber(t *testing.T) { - dir := hermetic(t) + hermetic(t) srcTok := "SRC-" + secret raceTok := "RACE-" + secret - shared := writeLegacyDefault(t, dir, srcTok) + shared := writeLegacyDefault(t, srcTok) migrationApplyHook = func(s *Store) { if err := s.cs.Set(s.profile, KeyAPIToken, raceTok, cccredstore.WithOverwrite()); err != nil { @@ -699,11 +715,11 @@ func TestMigration_ConcurrentWriter_DivergentValue_ConflictNoClobber(t *testing. // disagreeing party (not read as "divergence across" one source), no // mutation occurs, and no secret leaks. func TestMigration_ExistingAPIToken_NamedInConflict(t *testing.T) { - dir := hermetic(t) + hermetic(t) existing := "EXISTING-" + secret legacy := "LEGACY-" + secret seedRawAPIToken(t, existing) - shared := writeLegacyDefault(t, dir, legacy) + shared := writeLegacyDefault(t, legacy) err := EnsureMigrated() if !errors.Is(err, ErrMigrationConflict) { @@ -736,9 +752,9 @@ func TestMigration_ExistingAPIToken_NamedInConflict(t *testing.T) { // deprecated key, scrub the plaintext, all in one run; bundle exactly // {api_token}. func TestMigration_DeprecatedKeyAndPlaintext_SameValue_Collapses(t *testing.T) { - dir := hermetic(t) + hermetic(t) seedDeprecatedKey(t, "jtk_api_token", secret) - shared := writeLegacyDefault(t, dir, secret) + shared := writeLegacyDefault(t, secret) if err := EnsureMigrated(); err != nil { t.Fatalf("EnsureMigrated: %v", err) @@ -761,9 +777,9 @@ func TestMigration_DeprecatedKeyAndPlaintext_SameValue_Collapses(t *testing.T) { // B3 upgrade where the deprecated keyring key and the plaintext file // DISAGREE → hard conflict, nothing mutated. func TestMigration_DeprecatedKeyAndPlaintext_Divergent_Conflict(t *testing.T) { - dir := hermetic(t) + hermetic(t) seedDeprecatedKey(t, "jtk_api_token", "DEP-"+secret) - shared := writeLegacyDefault(t, dir, "PLAIN-"+secret) + shared := writeLegacyDefault(t, "PLAIN-"+secret) err := EnsureMigrated() if !errors.Is(err, ErrMigrationConflict) { @@ -783,9 +799,9 @@ func TestMigration_DeprecatedKeyAndPlaintext_Divergent_Conflict(t *testing.T) { // reachable code): an explicit-overwrite migration replaces an existing // api_token with the single legacy source value and scrubs plaintext. func TestMigrateOverwrite_ApplyReplacesExisting(t *testing.T) { - dir := hermetic(t) + hermetic(t) seedRawAPIToken(t, "OLD-"+secret) - shared := writeLegacyDefault(t, dir, "NEW-"+secret) + shared := writeLegacyDefault(t, "NEW-"+secret) s, err := OpenForClearAll() // migrationAllowedKeys, no auto-migrate if err != nil { diff --git a/shared/keyring/migrate.go b/shared/keyring/migrate.go index f3b4038..fcef976 100644 --- a/shared/keyring/migrate.go +++ b/shared/keyring/migrate.go @@ -64,19 +64,46 @@ var ErrMigrationConflict = errors.New("keyring: API token migration sources disa func migrateLegacyOverwrite(s *Store, overwrite bool) error { // ---- Phase 1: collect (no mutation) ---------------------------------- - sharedPath := credstore.DefaultPath() - // Migration-only projection: the canonical Store no longer carries - // per-tool connection/token fields (§2.2/MON-5328), but the §1.8 - // token migration must still SEE a legacy per-tool api_token. Absent - // file → proj == nil. Parse failure → ErrCorruptStore (callers treat - // it as a hard error; never silently overwrite an unreadable file). - proj, err := credstore.LoadSharedLegacyProjection(sharedPath) - if err != nil { - return err + // A resolver error (relative/unresolvable $XDG_CONFIG_HOME) means + // there is no addressable shared file: treat it as absent so the + // migration neither reads nor scrubs a cwd-relative path. + sharedPath, sperr := credstore.DefaultPath() + var proj *credstore.SharedLegacyProjection + if sperr != nil { + proj = &credstore.SharedLegacyProjection{} + } else { + // Migration-only projection: the canonical Store no longer carries + // per-tool connection/token fields (§2.2/MON-5328), but the §1.8 + // token migration must still SEE a legacy per-tool api_token. Absent + // file → proj == nil. Parse failure → ErrCorruptStore (callers treat + // it as a hard error; never silently overwrite an unreadable file). + p, err := credstore.LoadSharedLegacyProjection(sharedPath) + if err != nil { + return err + } + if p == nil { + p = &credstore.SharedLegacyProjection{Path: sharedPath} + } + proj = p } - if proj == nil { - proj = &credstore.SharedLegacyProjection{Path: sharedPath} + // Old-shared (§3.2): the prior hand-rolled shared location is an + // ADDITIVE pre-token legacy source. Enumerate (no copy) any + // plaintext api_token there so the macOS/Windows resolver move does + // not strand a secret on disk — it is consolidated/scrubbed BEFORE + // token resolution, exactly like the canonical shared file. + // Path-identity with the current resolver is deduped (no double + // source label, no double scrub). A parse failure fails loud rather + // than scrubbing blindly. + var oldProj *credstore.SharedLegacyProjection + var oldSharedPath string + if sperr == nil { + op, opProj, oerr := credstore.OldSharedProjection(sharedPath) + if oerr != nil { + return oerr + } + oldSharedPath, oldProj = op, opProj } + cflPath := credstore.LegacyCFLPath() jtkPath := credstore.LegacyJTKPath() legacyCFL, errC := credstore.LoadLegacyCFL(cflPath) @@ -108,6 +135,11 @@ func migrateLegacyOverwrite(s *Store, overwrite bool) error { add(proj.Default.APIToken, "shared config default ("+sharedPath+")") add(proj.CFL.APIToken, "shared config cfl.api_token ("+sharedPath+")") add(proj.JTK.APIToken, "shared config jtk.api_token ("+sharedPath+")") + if oldProj != nil { + add(oldProj.Default.APIToken, "prior shared config default ("+oldSharedPath+")") + add(oldProj.CFL.APIToken, "prior shared config cfl.api_token ("+oldSharedPath+")") + add(oldProj.JTK.APIToken, "prior shared config jtk.api_token ("+oldSharedPath+")") + } if legacyCFL != nil { add(legacyCFL.APIToken, "legacy cfl config ("+legacyCFL.Path+")") } @@ -128,7 +160,9 @@ func migrateLegacyOverwrite(s *Store, overwrite bool) error { sharedHadToken := proj.Default.APIToken != "" || proj.CFL.APIToken != "" || proj.JTK.APIToken != "" - anyPlaintext := sharedHadToken || + oldSharedHadToken := oldProj != nil && (oldProj.Default.APIToken != "" || + oldProj.CFL.APIToken != "" || oldProj.JTK.APIToken != "") + anyPlaintext := sharedHadToken || oldSharedHadToken || (legacyCFL != nil && legacyCFL.APIToken != "") || (legacyJTK != nil && legacyJTK.APIToken != "") @@ -197,6 +231,11 @@ func migrateLegacyOverwrite(s *Store, overwrite bool) error { return fmt.Errorf("migrated to keyring %s but could not scrub %s: %w", s.ref, sharedPath, err) } } + if oldSharedHadToken { + if err := scrubSharedStore(oldSharedPath); err != nil { + return fmt.Errorf("migrated to keyring %s but could not scrub %s: %w", s.ref, oldSharedPath, err) + } + } if legacyCFL != nil && legacyCFL.APIToken != "" { if err := scrubLegacyFile(cflPath); err != nil { return fmt.Errorf("migrated to keyring %s but could not scrub %s: %w", s.ref, cflPath, err) diff --git a/shared/keyring/migrate_conn_test.go b/shared/keyring/migrate_conn_test.go index 19b4019..9df9e28 100644 --- a/shared/keyring/migrate_conn_test.go +++ b/shared/keyring/migrate_conn_test.go @@ -18,11 +18,8 @@ import ( // save on a plain runtime path. Without this, S1's schema strip would // silently drop a user's per-tool url/email on the first command. func TestRuntimeMigration_TokenOnly_PreservesPerToolConn(t *testing.T) { - dir := hermetic(t) - path := filepath.Join(dir, "atlassian-cli", "config.yml") - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { - t.Fatal(err) - } + hermetic(t) + path := sharedConfigPath(t) pre := "default:\n" + " url: https://acme.atlassian.net\n" + " email: d@e\n" + diff --git a/tools/cfl/go.mod b/tools/cfl/go.mod index 4eb3962..b94fb60 100644 --- a/tools/cfl/go.mod +++ b/tools/cfl/go.mod @@ -6,6 +6,7 @@ require ( github.com/JohannesKaufmann/html-to-markdown/v2 v2.5.0 github.com/charmbracelet/huh v0.8.0 github.com/open-cli-collective/atlassian-go v0.0.0-00010101000000-000000000000 + github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d github.com/spf13/cobra v1.8.1 github.com/yuin/goldmark v1.7.16 golang.org/x/text v0.31.0 @@ -50,7 +51,6 @@ require ( github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect github.com/muesli/cancelreader v0.2.2 // indirect github.com/muesli/termenv v0.16.0 // indirect - github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/spf13/pflag v1.0.5 // indirect diff --git a/tools/cfl/go.sum b/tools/cfl/go.sum index 12bf868..328ec7f 100644 --- a/tools/cfl/go.sum +++ b/tools/cfl/go.sum @@ -98,8 +98,8 @@ github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e h1:fD57ERR4JtEqsWbfPhv4DMiApHyliiK5xCTNVSPiaAs= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= -github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 h1:78EW5uCbAzbAO32+oY4HDaFOqS2sPYnc4AT+G5UjdL0= -github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds= +github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d h1:fiyfxc/Wvpuen/u6Mk3bCUG0DLyOylF0K+eol4y9C64= +github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= diff --git a/tools/cfl/internal/cmd/configcmd/clear.go b/tools/cfl/internal/cmd/configcmd/clear.go index 7fb690c..40ebef7 100644 --- a/tools/cfl/internal/cmd/configcmd/clear.go +++ b/tools/cfl/internal/cmd/configcmd/clear.go @@ -104,6 +104,9 @@ func runClear(opts *clearOptions) error { if plan.SharedConfigPath != "" { _, _ = fmt.Fprintf(opts.Stderr, "It will also delete the shared config file: %s\n", plan.SharedConfigPath) } + if plan.OldSharedConfigPath != "" { + _, _ = fmt.Fprintf(opts.Stderr, "It will also delete the prior shared config file: %s\n", plan.OldSharedConfigPath) + } for _, lp := range plan.LegacyPaths { _, _ = fmt.Fprintf(opts.Stderr, "It will scrub the legacy plaintext file: %s\n", lp) } diff --git a/tools/cfl/internal/cmd/configcmd/clear_test.go b/tools/cfl/internal/cmd/configcmd/clear_test.go index e7056dd..eabd0b2 100644 --- a/tools/cfl/internal/cmd/configcmd/clear_test.go +++ b/tools/cfl/internal/cmd/configcmd/clear_test.go @@ -3,7 +3,6 @@ package configcmd import ( "bytes" "os" - "path/filepath" "strings" "testing" @@ -82,11 +81,10 @@ func TestRunClear_Cancelled(t *testing.T) { } func TestRunClear_All(t *testing.T) { - xdg := credtest.Hermetic(t) + credtest.Hermetic(t) credtest.SeedToken(t, "shared-secret") - sharedPath := filepath.Join(xdg, "atlassian-cli", "config.yml") - testutil.RequireNoError(t, os.MkdirAll(filepath.Dir(sharedPath), 0o700)) + sharedPath := credtest.SharedConfigPath(t) testutil.RequireNoError(t, os.WriteFile(sharedPath, []byte("default:\n url: https://x\n"), 0o600)) opts, _, _ := newClearOpts(true, "") diff --git a/tools/cfl/internal/cmd/init/init.go b/tools/cfl/internal/cmd/init/init.go index 36f1157..603b25d 100644 --- a/tools/cfl/internal/cmd/init/init.go +++ b/tools/cfl/internal/cmd/init/init.go @@ -95,7 +95,12 @@ func runInit(ctx context.Context, opts *root.Options, prefillURL, prefillEmail, } legacyPath := config.DefaultConfigPath() - sharedPath := credstore.DefaultPath() + sharedPath, err := credstore.DefaultPath() + if err != nil { + v.Error("Cannot resolve the shared credential store path: %v", err) + v.Error("Set XDG_CONFIG_HOME to an absolute path (or unset it), then re-run cfl init.") + return err + } jtkLegacyPath := credstore.LegacyJTKPath() // §2.2 ordering (MON-5328): detect connection divergence FIRST, diff --git a/tools/cfl/internal/cmd/init/reconcile.go b/tools/cfl/internal/cmd/init/reconcile.go index 293c653..de1a56c 100644 --- a/tools/cfl/internal/cmd/init/reconcile.go +++ b/tools/cfl/internal/cmd/init/reconcile.go @@ -44,6 +44,25 @@ func detectAndReconcile( cflLegacyPath, jtkLegacyPath, sharedPath string, prefillURL, prefillEmail, prefillAuthMethod, prefillCloudID string, ) (*reconcileResult, error) { + // §3.2 PURE pre-token relocation gate: runs BEFORE per-tool + // divergence detection and BEFORE any mutation. A corrupt old/new + // file or a divergent old↔new shared config fails loud here naming + // both paths, mutating nothing — consistent with the fail-loud, + // mutate-nothing invariant. + rel, relErr := credstore.DetectSharedRelocation(sharedPath) + if relErr != nil { + if errors.Is(relErr, credstore.ErrCorruptStore) { + // A corrupt old OR new shared file: same contract/UX as the + // Load path below — unreadable, refuse to overwrite. + v.Error("Shared credential store at %s is unreadable: %v", sharedPath, relErr) + v.Error("Refusing to overwrite. Fix or remove the file, then re-run cfl init.") + } else { + v.Error("Shared credential store relocation check failed: %v", relErr) + v.Error("Refusing to mutate anything. Reconcile the named file(s), then re-run cfl init.") + } + return nil, relErr + } + store, err := credstore.Load(sharedPath) if err != nil { v.Error("Shared credential store at %s is unreadable: %v", sharedPath, err) @@ -81,11 +100,35 @@ func detectAndReconcile( // Build the full named connection candidate set and detect // divergence (pure, secret-free, no IO/keyring — shared with jtk). candidates := credstore.ConnCandidates(sharedPath, store.Default, proj, cflLegacy, jtkLegacy) + // Old-shared is ADDITIVE to the candidate set so a relocation copy + // is gated on the per-tool divergence check passing (no copy while a + // divergence is pending). + candidates = append(candidates, credstore.OldSharedConnCandidates(rel)...) chosen, conflicts := credstore.DetectConnDivergence(candidates) if len(conflicts) > 0 { return nil, credstore.ConnConflictError(conflicts, candidates, "cfl") } + // Gated apply: every conflict gate (relocation + per-tool + // divergence) has now passed — materialize the old shared file at + // the new path (copy-leave-old), then reload so the remainder + // reconciles the materialized file exactly as a returning user. + if rel.CopyNeeded { + if aerr := credstore.ApplySharedRelocation(rel); aerr != nil { + v.Error("Could not relocate the shared credential store: %v", aerr) + return nil, aerr + } + // Reload only the canonical store: the divergence candidates + // (incl. old-shared) were already built and checked above, so + // `proj` is not read again — the materialized store is what the + // fold + preserveDefaults below operate on. + store, err = credstore.Load(sharedPath) + if err != nil { + v.Error("Shared credential store at %s is unreadable: %v", sharedPath, err) + return nil, err + } + } + // affectsSibling must be judged on the ORIGINAL loaded store, BEFORE // folding `chosen` in — otherwise a first-time migration from only a // legacy file looks like it is overwriting an already-usable shared diff --git a/tools/cfl/internal/cmd/init/reconcile_test.go b/tools/cfl/internal/cmd/init/reconcile_test.go index 5a0adc6..63670f8 100644 --- a/tools/cfl/internal/cmd/init/reconcile_test.go +++ b/tools/cfl/internal/cmd/init/reconcile_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" + "github.com/open-cli-collective/cli-common/statedirtest" + "github.com/open-cli-collective/atlassian-go/credstore" "github.com/open-cli-collective/atlassian-go/testutil" "github.com/open-cli-collective/atlassian-go/view" @@ -14,6 +16,65 @@ import ( "github.com/open-cli-collective/confluence-cli/internal/config" ) +// oldSharedFixture writes a fixture at the prior hand-rolled shared +// location (statedirtest sets $XDG_CONFIG_HOME, so oldSharedPath +// resolves there) and returns it. The caller passes a DISTINCT +// sharedPath as "new" so the §3.2 relocation actually engages on Linux +// (where the resolver would otherwise collapse old≡new). +func oldSharedFixture(t *testing.T, body string) string { + t.Helper() + // Explicit $XDG_CONFIG_HOME override (mirrors relocate_test.go's + // oldBase) so the old-shared path is deterministic and does not + // silently void if statedirtest's platform behavior changes. + base := filepath.Join(t.TempDir(), "oldbase") + t.Setenv("XDG_CONFIG_HOME", base) + p := filepath.Join(base, "atlassian-cli", "config.yml") + testutil.RequireNoError(t, os.MkdirAll(filepath.Dir(p), 0o700)) + testutil.RequireNoError(t, os.WriteFile(p, []byte(body), 0o600)) + return p +} + +// Major (Codex): old-only shared config at the prior path is COPIED +// (gated apply) into the new path during init, and reconciled in. +func TestReconcile_OldSharedOnly_CopiedAtInit(t *testing.T) { + statedirtest.Hermetic(t) + tmp := t.TempDir() + oldSharedFixture(t, "default:\n url: https://old-shared.atlassian.net\n email: u@e\ncfl:\n default_space: SP\n") + newPath := filepath.Join(tmp, "newshared", "config.yml") + + v, _, _ := newReconcileView() + r, err := detectAndReconcile(v, + filepath.Join(tmp, "cfl.yml"), filepath.Join(tmp, "jtk.json"), + newPath, "", "", "", "") + testutil.RequireNoError(t, err) + if _, statErr := os.Stat(newPath); statErr != nil { + t.Fatalf("old-only must be COPIED to the new path at init: %v", statErr) + } + testutil.Equal(t, "https://old-shared.atlassian.net", r.store.Default.URL) + testutil.Equal(t, "SP", r.store.CFL.DefaultSpace) +} + +// Major (Codex): a pending per-tool connection divergence must block +// the relocation copy entirely — fail loud, mutate nothing. +func TestReconcile_OldShared_PerToolDivergencePending_NoCopy(t *testing.T) { + statedirtest.Hermetic(t) + tmp := t.TempDir() + oldSharedFixture(t, "default:\n url: https://old-shared.atlassian.net\n email: u@e\n") + newPath := filepath.Join(tmp, "newshared", "config.yml") + // A legacy cfl file whose connection diverges from old-shared. + cflPath := filepath.Join(tmp, "cfl.yml") + testutil.RequireNoError(t, os.WriteFile(cflPath, + []byte("url: https://divergent.atlassian.net\nemail: u@e\napi_token: t\n"), 0o600)) + + v, _, _ := newReconcileView() + _, err := detectAndReconcile(v, cflPath, filepath.Join(tmp, "jtk.json"), + newPath, "", "", "", "") + testutil.RequireError(t, err) + if _, statErr := os.Stat(newPath); !os.IsNotExist(statErr) { + t.Fatal("no copy may occur while a per-tool divergence is pending") + } +} + // Pure: detectAndReconcile does NO keyring I/O (B3 leak-regression // rule), so no hermetic harness is needed here. diff --git a/tools/cfl/internal/config/config.go b/tools/cfl/internal/config/config.go index da2574c..6d71651 100644 --- a/tools/cfl/internal/config/config.go +++ b/tools/cfl/internal/config/config.go @@ -123,11 +123,14 @@ func DefaultConfigPath() string { return filepath.Join(home, ".config", "cfl", "config.yml") } -// Save writes the configuration to the specified path. +// Save writes the configuration atomically (temp + rename) so a crash +// mid-write never leaves a truncated config behind. Dir mode is 0700 +// and file mode 0600 (the §3 on-disk-state standard). On any error the +// temp file is removed best-effort so a failed save leaves no stale +// .tmp. func (c *Config) Save(path string) error { - // Create directory if it doesn't exist dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0750); err != nil { + if err := os.MkdirAll(dir, 0o700); err != nil { return fmt.Errorf("creating config directory: %w", err) } @@ -136,10 +139,15 @@ func (c *Config) Save(path string) error { return fmt.Errorf("marshaling config: %w", err) } - // Write with restricted permissions (user read/write only) - if err := os.WriteFile(path, data, 0600); err != nil { + tmp := path + ".tmp" + if err := os.WriteFile(tmp, data, 0o600); err != nil { + _ = os.Remove(tmp) return fmt.Errorf("writing config file: %w", err) } + if err := os.Rename(tmp, path); err != nil { + _ = os.Remove(tmp) + return fmt.Errorf("finalizing config file: %w", err) + } return nil } @@ -195,6 +203,11 @@ var corruptSharedWarnOnce sync.Once func warnCorruptSharedOnce(err error) { corruptSharedWarnOnce.Do(func() { + if errors.Is(err, credstore.ErrRelocationConflict) { + // Readable, not a fallback: the canonical config is in use. + fmt.Fprintf(os.Stderr, "warning: prior and current shared config diverge (%v); using the current config. Run `cfl init` to reconcile.\n", err) + return + } fmt.Fprintf(os.Stderr, "warning: shared credential store is unreadable (%v); falling back to per-tool config. Run `cfl init` to fix.\n", err) }) } @@ -227,14 +240,18 @@ func LoadWithEnv(path string) (*Config, error) { cfg = &Config{} } - store, sErr := credstore.Load(credstore.DefaultPath()) + // Runtime shared resolver, §3.2 relocation-aware and mutation-free: + // reads the canonical store, transparently falls back to the prior + // hand-rolled location when only it exists, and on an old↔new + // divergence returns the canonical store alongside the error so the + // command keeps working while the conflict is surfaced once. A nil + // store (unresolvable/corrupt) → fall back to legacy + env. `cfl + // init` uses the fail-loud detect/gated-copy path instead. + store, sErr := credstore.LoadSharedRuntime() if sErr != nil { - // Runtime callers can't propagate the error meaningfully — every - // cfl command would die. Warn once on stderr and fall back to - // legacy + env. `cfl init` uses credstore.Load directly so it - // can surface the error and refuse to clobber. warnCorruptSharedOnce(sErr) - } else { + } + if store != nil { cfg.LoadFromShared(store) } diff --git a/tools/cfl/internal/config/config_test.go b/tools/cfl/internal/config/config_test.go index 3a9f3eb..b680926 100644 --- a/tools/cfl/internal/config/config_test.go +++ b/tools/cfl/internal/config/config_test.go @@ -259,6 +259,25 @@ func TestConfig_Save_and_Load(t *testing.T) { testutil.Equal(t, original.OutputFormat, loaded.OutputFormat) } +// Save is atomic (temp+rename) with 0600 file / 0700 dir and leaves no +// stale .tmp on success (the MON-5370 commit-4 hardening). +func TestConfig_Save_AtomicPermsNoStaleTmp(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "cfl") + configPath := filepath.Join(dir, "config.yml") + testutil.RequireNoError(t, (&Config{URL: "https://acme.atlassian.net"}).Save(configPath)) + + fi, err := os.Stat(configPath) + testutil.RequireNoError(t, err) + testutil.Equal(t, os.FileMode(0o600), fi.Mode().Perm()) + di, err := os.Stat(dir) + testutil.RequireNoError(t, err) + testutil.Equal(t, os.FileMode(0o700), di.Mode().Perm()) + if _, statErr := os.Stat(configPath + ".tmp"); !os.IsNotExist(statErr) { + t.Fatal("atomic Save must leave no .tmp on success") + } +} + func TestLoad_FileNotFound(t *testing.T) { t.Parallel() _, err := Load("/nonexistent/path/config.yml") @@ -522,9 +541,9 @@ func TestConfig_LoadFromShared(t *testing.T) { } func TestLoadWithEnv_PrecedenceLegacyToSharedToEnv(t *testing.T) { - // Isolate XDG so credstore.DefaultPath points into a tempdir. - xdg := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", xdg) + // Hermetic 7-var isolation; derive the shared path from the + // resolver (cross-OS correct) rather than hand-building the layout. + credtest.Hermetic(t) // Seed legacy file (cfl-only). legacyDir := t.TempDir() @@ -537,7 +556,7 @@ func TestLoadWithEnv_PrecedenceLegacyToSharedToEnv(t *testing.T) { testutil.RequireNoError(t, legacy.Save(legacyPath)) // Seed shared store (overrides URL + token via default). - sharedPath := filepath.Join(xdg, "atlassian-cli", "config.yml") + sharedPath := credtest.SharedConfigPath(t) store := &credstore.Store{ Default: credstore.Section{ URL: "https://shared.atlassian.net", @@ -567,8 +586,8 @@ func TestLoadWithEnv_CorruptSharedFallsBackToLegacy(t *testing.T) { // Hermetic: deterministic file-backend keyring (so the corrupt-store // fallback resolves an empty token instead of touching the OS // keychain) and isolated XDG. - xdg := credtest.Hermetic(t) - sharedPath := filepath.Join(xdg, "atlassian-cli", "config.yml") + credtest.Hermetic(t) + sharedPath := credtest.SharedConfigPath(t) testutil.RequireNoError(t, os.MkdirAll(filepath.Dir(sharedPath), 0o700)) testutil.RequireNoError(t, os.WriteFile(sharedPath, []byte("default: : :: ["), 0o600)) diff --git a/tools/jtk/internal/cmd/configcmd/configcmd.go b/tools/jtk/internal/cmd/configcmd/configcmd.go index 556828a..d9f01c6 100644 --- a/tools/jtk/internal/cmd/configcmd/configcmd.go +++ b/tools/jtk/internal/cmd/configcmd/configcmd.go @@ -160,6 +160,9 @@ func runClear(ctx context.Context, opts *clearOptions) error { if plan.SharedConfigPath != "" { fmt.Fprintf(opts.Stderr, "It will also delete the shared config file: %s\n", plan.SharedConfigPath) } + if plan.OldSharedConfigPath != "" { + fmt.Fprintf(opts.Stderr, "It will also delete the prior shared config file: %s\n", plan.OldSharedConfigPath) + } for _, lp := range plan.LegacyPaths { fmt.Fprintf(opts.Stderr, "It will scrub the legacy plaintext file: %s\n", lp) } diff --git a/tools/jtk/internal/cmd/configcmd/configcmd_test.go b/tools/jtk/internal/cmd/configcmd/configcmd_test.go index aec6e4a..189f77b 100644 --- a/tools/jtk/internal/cmd/configcmd/configcmd_test.go +++ b/tools/jtk/internal/cmd/configcmd/configcmd_test.go @@ -6,7 +6,6 @@ import ( "net/http" "net/http/httptest" "os" - "path/filepath" "strings" "testing" @@ -51,11 +50,10 @@ func TestShowCmd_TableOutput(t *testing.T) { } func TestRunClear_All(t *testing.T) { - xdg := credtest.Hermetic(t) + credtest.Hermetic(t) credtest.SeedToken(t, "shared-secret") - sharedPath := filepath.Join(xdg, "atlassian-cli", "config.yml") - testutil.RequireNoError(t, os.MkdirAll(filepath.Dir(sharedPath), 0o700)) + sharedPath := credtest.SharedConfigPath(t) testutil.RequireNoError(t, os.WriteFile(sharedPath, []byte("default:\n url: https://x\n"), 0o600)) opts, _, _ := newClearOpts(t, true, "") diff --git a/tools/jtk/internal/cmd/initcmd/initcmd.go b/tools/jtk/internal/cmd/initcmd/initcmd.go index 716703c..d2bbd2a 100644 --- a/tools/jtk/internal/cmd/initcmd/initcmd.go +++ b/tools/jtk/internal/cmd/initcmd/initcmd.go @@ -79,7 +79,12 @@ func runInit(ctx context.Context, opts *root.Options, prefillURL, prefillEmail, v := opts.View() - sharedPath := credstore.DefaultPath() + sharedPath, err := credstore.DefaultPath() + if err != nil { + v.Error("Cannot resolve the shared credential store path: %v", err) + v.Error("Set XDG_CONFIG_HOME to an absolute path (or unset it), then re-run jtk init.") + return err + } jtkLegacyPath := credstore.LegacyJTKPath() cflLegacyPath := credstore.LegacyCFLPath() diff --git a/tools/jtk/internal/cmd/initcmd/reconcile.go b/tools/jtk/internal/cmd/initcmd/reconcile.go index 269d29d..c3c2854 100644 --- a/tools/jtk/internal/cmd/initcmd/reconcile.go +++ b/tools/jtk/internal/cmd/initcmd/reconcile.go @@ -36,6 +36,24 @@ func detectAndReconcile( jtkLegacyPath, cflLegacyPath, sharedPath string, prefillURL, prefillEmail, prefillToken, prefillAuthMethod, prefillCloudID string, ) (*reconcileResult, error) { + // §3.2 PURE pre-token relocation gate: runs BEFORE per-tool + // divergence detection and BEFORE any mutation. A corrupt old/new + // file or a divergent old↔new shared config fails loud here naming + // both paths, mutating nothing. + rel, relErr := credstore.DetectSharedRelocation(sharedPath) + if relErr != nil { + if errors.Is(relErr, credstore.ErrCorruptStore) { + // A corrupt old OR new shared file: same contract/UX as the + // Load path below — unreadable, refuse to overwrite. + v.Error("Shared credential store at %s is unreadable: %v", sharedPath, relErr) + v.Error("Refusing to overwrite. Fix or remove the file, then re-run jtk init.") + } else { + v.Error("Shared credential store relocation check failed: %v", relErr) + v.Error("Refusing to mutate anything. Reconcile the named file(s), then re-run jtk init.") + } + return nil, relErr + } + store, err := credstore.Load(sharedPath) if err != nil { v.Error("Shared credential store at %s is unreadable: %v", sharedPath, err) @@ -67,11 +85,35 @@ func detectAndReconcile( } candidates := credstore.ConnCandidates(sharedPath, store.Default, proj, cflLegacy, jtkLegacy) + // Old-shared is ADDITIVE so a relocation copy is gated on the + // per-tool divergence check passing (no copy while a divergence is + // pending). + candidates = append(candidates, credstore.OldSharedConnCandidates(rel)...) chosen, conflicts := credstore.DetectConnDivergence(candidates) if len(conflicts) > 0 { return nil, credstore.ConnConflictError(conflicts, candidates, "jtk") } + // Gated apply: every conflict gate (relocation + per-tool + // divergence) has passed — materialize the old shared file at the + // new path (copy-leave-old), then reload so the remainder + // reconciles the materialized file exactly as a returning user. + if rel.CopyNeeded { + if aerr := credstore.ApplySharedRelocation(rel); aerr != nil { + v.Error("Could not relocate the shared credential store: %v", aerr) + return nil, aerr + } + // Reload only the canonical store: the divergence candidates + // (incl. old-shared) were already built and checked above, so + // `proj` is not read again — the materialized store is what the + // fold + preserveDefaults below operate on. + store, err = credstore.Load(sharedPath) + if err != nil { + v.Error("Shared credential store at %s is unreadable: %v", sharedPath, err) + return nil, err + } + } + // affectsSibling judged on the ORIGINAL loaded store, BEFORE folding // `chosen` (else a first-time legacy migration falsely looks like it // overwrites a usable shared default). True only when the store diff --git a/tools/jtk/internal/cmd/initcmd/reconcile_test.go b/tools/jtk/internal/cmd/initcmd/reconcile_test.go index cc2038e..8f26ef3 100644 --- a/tools/jtk/internal/cmd/initcmd/reconcile_test.go +++ b/tools/jtk/internal/cmd/initcmd/reconcile_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" + "github.com/open-cli-collective/cli-common/statedirtest" + "github.com/open-cli-collective/atlassian-go/credstore" "github.com/open-cli-collective/atlassian-go/testutil" "github.com/open-cli-collective/atlassian-go/view" @@ -14,6 +16,63 @@ import ( "github.com/open-cli-collective/jira-ticket-cli/internal/config" ) +// oldSharedFixture writes a fixture at the prior hand-rolled shared +// location (statedirtest sets $XDG_CONFIG_HOME, so oldSharedPath +// resolves there); the caller passes a DISTINCT sharedPath as "new" so +// §3.2 relocation engages on Linux (resolver would else collapse them). +func oldSharedFixture(t *testing.T, body string) string { + t.Helper() + // Explicit $XDG_CONFIG_HOME override (mirrors relocate_test.go's + // oldBase) so the old-shared path is deterministic and does not + // silently void if statedirtest's platform behavior changes. + base := filepath.Join(t.TempDir(), "oldbase") + t.Setenv("XDG_CONFIG_HOME", base) + p := filepath.Join(base, "atlassian-cli", "config.yml") + testutil.RequireNoError(t, os.MkdirAll(filepath.Dir(p), 0o700)) + testutil.RequireNoError(t, os.WriteFile(p, []byte(body), 0o600)) + return p +} + +// Major (Codex): old-only shared config at the prior path is COPIED +// (gated apply) into the new path during jtk init and reconciled in. +func TestReconcile_OldSharedOnly_CopiedAtInit(t *testing.T) { + statedirtest.Hermetic(t) + tmp := t.TempDir() + oldSharedFixture(t, "default:\n url: https://old-shared.atlassian.net\n email: u@e\njtk:\n default_project: PROJ\n") + newPath := filepath.Join(tmp, "newshared", "config.yml") + + v, _, _ := newReconcileView() + r, err := detectAndReconcile(v, + filepath.Join(tmp, "jtk.json"), filepath.Join(tmp, "cfl.yml"), + newPath, "", "", "", "", "") + testutil.RequireNoError(t, err) + if _, statErr := os.Stat(newPath); statErr != nil { + t.Fatalf("old-only must be COPIED to the new path at init: %v", statErr) + } + testutil.Equal(t, "https://old-shared.atlassian.net", r.store.Default.URL) + testutil.Equal(t, "PROJ", r.store.JTK.DefaultProject) +} + +// Major (Codex): a pending per-tool connection divergence blocks the +// relocation copy entirely — fail loud, mutate nothing. +func TestReconcile_OldShared_PerToolDivergencePending_NoCopy(t *testing.T) { + statedirtest.Hermetic(t) + tmp := t.TempDir() + oldSharedFixture(t, "default:\n url: https://old-shared.atlassian.net\n email: u@e\n") + newPath := filepath.Join(tmp, "newshared", "config.yml") + jtkPath := filepath.Join(tmp, "jtk.json") + testutil.RequireNoError(t, os.WriteFile(jtkPath, + []byte(`{"url":"https://divergent.atlassian.net","email":"u@e","api_token":"t"}`), 0o600)) + + v, _, _ := newReconcileView() + _, err := detectAndReconcile(v, jtkPath, filepath.Join(tmp, "cfl.yml"), + newPath, "", "", "", "", "") + testutil.RequireError(t, err) + if _, statErr := os.Stat(newPath); !os.IsNotExist(statErr) { + t.Fatal("no copy may occur while a per-tool divergence is pending") + } +} + // These tests are pure: detectAndReconcile does NO keyring I/O (the B3 // leak-regression rule — keyring access lives only in the command // layer), so no hermetic credtest harness is required. diff --git a/tools/jtk/internal/config/config.go b/tools/jtk/internal/config/config.go index eaf4140..acb501f 100644 --- a/tools/jtk/internal/config/config.go +++ b/tools/jtk/internal/config/config.go @@ -21,10 +21,18 @@ import ( // reads. Init has a separate code path that surfaces corruption as a // hard error and refuses to clobber the file. func loadShared() *credstore.Store { - s, err := credstore.Load(credstore.DefaultPath()) + // §3.2 relocation-aware, mutation-free runtime resolver: canonical + // store, transparent read-fallback to the prior hand-rolled location + // when only it exists, and on an old↔new divergence the canonical + // store is returned alongside the error so commands keep working + // while the conflict is surfaced once. `jtk init` is the fail-loud + // mutating gate. + s, err := credstore.LoadSharedRuntime() if err != nil { warnCorruptSharedOnce(err) - return &credstore.Store{} + if s == nil { + return &credstore.Store{} + } } return s } @@ -33,6 +41,11 @@ var corruptSharedWarnOnce sync.Once func warnCorruptSharedOnce(err error) { corruptSharedWarnOnce.Do(func() { + if errors.Is(err, credstore.ErrRelocationConflict) { + // Readable, not a fallback: the canonical config is in use. + fmt.Fprintf(os.Stderr, "warning: prior and current shared config diverge (%v); using the current config. Run `jtk init` to reconcile.\n", err) + return + } fmt.Fprintf(os.Stderr, "warning: shared credential store is unreadable (%v); falling back to per-tool config. Run `jtk init` to fix.\n", err) }) } @@ -122,9 +135,19 @@ func Save(cfg *Config) error { return fmt.Errorf("marshaling config: %w", err) } - if err := os.WriteFile(path, data, configFileMode); err != nil { + // Atomic write (temp + rename) so a crash mid-write never leaves a + // truncated config. Dir/file modes are already 0700/0600 (the §3 + // on-disk-state standard). On any error the temp file is removed + // best-effort so a failed save leaves no stale .tmp. + tmp := path + ".tmp" + if err := os.WriteFile(tmp, data, configFileMode); err != nil { + _ = os.Remove(tmp) return fmt.Errorf("writing config file: %w", err) } + if err := os.Rename(tmp, path); err != nil { + _ = os.Remove(tmp) + return fmt.Errorf("finalizing config file: %w", err) + } return nil } diff --git a/tools/jtk/internal/config/config_test.go b/tools/jtk/internal/config/config_test.go index 5b8d880..ba460ff 100644 --- a/tools/jtk/internal/config/config_test.go +++ b/tools/jtk/internal/config/config_test.go @@ -6,52 +6,32 @@ import ( "testing" "github.com/open-cli-collective/atlassian-go/credstore" + "github.com/open-cli-collective/atlassian-go/credtest" "github.com/open-cli-collective/atlassian-go/keyring" "github.com/open-cli-collective/atlassian-go/testutil" "github.com/open-cli-collective/atlassian-go/url" ) -// setupTestConfig creates a temporary config directory for testing -// Uses t.Setenv for automatic cleanup and t.TempDir for automatic removal +// setupTestConfig hermetically isolates the test environment and returns +// the RESOLVED shared config path (credstore.DefaultPath under the +// hermetic env) plus a no-op cleanup. credtest.Hermetic provides the +// canonical 7-var isolation (cross-OS correct — no hand-built layout), +// the file keyring backend, token-env clearing, and migration-notice +// reset; this only adds the non-token JIRA_*/ATLASSIAN_* clears the +// jtk accessors consult. func setupTestConfig(t *testing.T) (string, func()) { t.Helper() - // Use t.TempDir() which auto-cleans after test - tempDir := t.TempDir() - - // Use t.Setenv which auto-restores after test (Go 1.17+) - // XDG_CONFIG_HOME is used on Linux, HOME+Library/App Support on macOS - t.Setenv("XDG_CONFIG_HOME", tempDir) - t.Setenv("HOME", tempDir) - - // Clear any JIRA and ATLASSIAN env vars that might interfere - t.Setenv("JIRA_URL", "") - t.Setenv("JIRA_DOMAIN", "") - t.Setenv("JIRA_EMAIL", "") - t.Setenv("JIRA_API_TOKEN", "") - t.Setenv("JIRA_AUTH_METHOD", "") - t.Setenv("JIRA_CLOUD_ID", "") - t.Setenv("ATLASSIAN_URL", "") - t.Setenv("ATLASSIAN_EMAIL", "") - t.Setenv("ATLASSIAN_API_TOKEN", "") - t.Setenv("ATLASSIAN_AUTH_METHOD", "") - t.Setenv("ATLASSIAN_CLOUD_ID", "") - - // Create macOS-style dir as well for fallback - libDir := filepath.Join(tempDir, "Library", "Application Support") - err := os.MkdirAll(libDir, 0700) - testutil.RequireNoError(t, err) + credtest.Hermetic(t) - // Force the portable encrypted-file keyring backend so token - // resolution is deterministic and never touches the OS keychain. - t.Setenv(keyring.BackendEnvVar, "file") - t.Setenv("ATLASSIAN_CLI_KEYRING_PASSPHRASE", "jtk-test-passphrase") - t.Setenv("CFL_API_TOKEN", "") - keyring.ResetMigrationNotice() - t.Cleanup(keyring.ResetMigrationNotice) + for _, v := range []string{ + "JIRA_URL", "JIRA_DOMAIN", "JIRA_EMAIL", "JIRA_AUTH_METHOD", "JIRA_CLOUD_ID", + "ATLASSIAN_URL", "ATLASSIAN_EMAIL", "ATLASSIAN_AUTH_METHOD", "ATLASSIAN_CLOUD_ID", + } { + t.Setenv(v, "") + } - // Return empty cleanup since t.TempDir and t.Setenv handle it - return tempDir, func() {} + return credtest.SharedConfigPath(t), func() {} } func TestConfig_SaveAndLoad(t *testing.T) { @@ -145,6 +125,18 @@ func TestConfig_FilePermissions(t *testing.T) { testutil.Equal(t, info.Mode().Perm(), os.FileMode(0600)) } +// Save is atomic (temp+rename) and leaves no stale .tmp on success +// (the MON-5370 commit-5 hardening; modes are covered above). +func TestConfig_Save_AtomicNoStaleTmp(t *testing.T) { + _, cleanup := setupTestConfig(t) + defer cleanup() + + testutil.RequireNoError(t, Save(&Config{URL: "https://acme.atlassian.net"})) + if _, statErr := os.Stat(Path() + ".tmp"); !os.IsNotExist(statErr) { + t.Fatal("atomic Save must leave no .tmp on success") + } +} + func TestGetURL_EnvOverride(t *testing.T) { _, cleanup := setupTestConfig(t) defer cleanup() @@ -545,11 +537,10 @@ func TestGetURL_FullPrecedenceChain(t *testing.T) { } func TestSharedStore_FillsURLBetweenEnvAndLegacy(t *testing.T) { - tempDir, cleanup := setupTestConfig(t) + sharedPath, cleanup := setupTestConfig(t) defer cleanup() // Seed shared store with a URL. - sharedPath := filepath.Join(tempDir, "atlassian-cli", "config.yml") store := &credstore.Store{ Default: credstore.Section{URL: "https://shared.atlassian.net"}, } @@ -580,10 +571,9 @@ func TestSharedStore_LegacyWinsWhenSharedAbsent(t *testing.T) { } func TestSharedStore_DefaultProject(t *testing.T) { - tempDir, cleanup := setupTestConfig(t) + sharedPath, cleanup := setupTestConfig(t) defer cleanup() - sharedPath := filepath.Join(tempDir, "atlassian-cli", "config.yml") store := &credstore.Store{ JTK: credstore.ToolSection{DefaultProject: "MON"}, } @@ -595,10 +585,9 @@ func TestSharedStore_DefaultProject(t *testing.T) { } func TestSharedStore_AuthMethodWithSource(t *testing.T) { - tempDir, cleanup := setupTestConfig(t) + sharedPath, cleanup := setupTestConfig(t) defer cleanup() - sharedPath := filepath.Join(tempDir, "atlassian-cli", "config.yml") store := &credstore.Store{ Default: credstore.Section{AuthMethod: "bearer"}, } @@ -610,7 +599,7 @@ func TestSharedStore_AuthMethodWithSource(t *testing.T) { } func TestSharedStore_FullPrecedenceChain(t *testing.T) { - tempDir, cleanup := setupTestConfig(t) + sharedPath, cleanup := setupTestConfig(t) defer cleanup() // Layer 1 (lowest): legacy file. @@ -618,7 +607,6 @@ func TestSharedStore_FullPrecedenceChain(t *testing.T) { testutil.RequireNoError(t, Save(cfg)) // Layer 2: shared default. - sharedPath := filepath.Join(tempDir, "atlassian-cli", "config.yml") store := &credstore.Store{ Default: credstore.Section{URL: "https://shared.atlassian.net", APIToken: "shared-tok"}, } @@ -641,11 +629,10 @@ func TestSharedStore_FullPrecedenceChain(t *testing.T) { } func TestSharedStore_CorruptDoesNotBlockAccessors(t *testing.T) { - tempDir, cleanup := setupTestConfig(t) + sharedPath, cleanup := setupTestConfig(t) defer cleanup() // Corrupt shared store. - sharedPath := filepath.Join(tempDir, "atlassian-cli", "config.yml") testutil.RequireNoError(t, os.MkdirAll(filepath.Dir(sharedPath), 0o700)) testutil.RequireNoError(t, os.WriteFile(sharedPath, []byte("default: : :: ["), 0o600))