From bff0e18d1a9923e537cb04920f6f012009e37526 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 09:59:28 +0200 Subject: [PATCH 1/9] test(cache): pin refresh-still-writes, key contracts and the unmarshal guard Renames maxSessionAge to sessionTimestampRetention and corrects its comment: CleanupSessions filters on activeIDs membership and never reads the constant. --- internal/cache/cache_test.go | 13 ++- internal/cache/cached_eligibility_test.go | 126 +++++++++++++++++++++- internal/cache/cached_roles_test.go | 35 ++++++ internal/cache/session_tracker.go | 16 ++- internal/cache/session_tracker_test.go | 71 ++++++++++++ 5 files changed, 250 insertions(+), 11 deletions(-) diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 8e3b243..1db35b2 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -64,16 +64,21 @@ func TestGet_CorruptJSON(t *testing.T) { dir := t.TempDir() s := NewStore(dir, 4*time.Hour) - // Write garbage to the cache file + // The envelope must carry a FRESH cached_at: an unparseable file or a + // zero-valued cached_at also produces a miss via the TTL branch, which + // would let a broken unmarshal guard pass this test. A well-formed + // envelope whose "response" has the wrong type leaves the unmarshal + // guard as the only thing that can produce the miss. path := filepath.Join(dir, "corrupt.json") - if err := os.WriteFile(path, []byte("{not valid json"), 0o600); err != nil { + envelope := []byte(`{"cached_at":"` + time.Now().UTC().Format(time.RFC3339Nano) + `","response":12345}`) + if err := os.WriteFile(path, envelope, 0o600); err != nil { t.Fatalf("failed to write corrupt file: %v", err) } - var out string + var out string // dst is a string; the stored "response" is a number ok := Get(s, "corrupt", &out) if ok { - t.Fatal("expected miss for corrupt JSON") + t.Fatal("expected miss for a type-mismatched payload with a fresh cached_at") } } diff --git a/internal/cache/cached_eligibility_test.go b/internal/cache/cached_eligibility_test.go index be085a2..fea179b 100644 --- a/internal/cache/cached_eligibility_test.go +++ b/internal/cache/cached_eligibility_test.go @@ -387,7 +387,129 @@ func TestCachedEligibilityLister_LogsRefreshBypass(t *testing.T) { } } -// writeCorruptCacheFile writes invalid JSON to a cache file. +// writeCorruptCacheFile writes a well-formed cache envelope with a FRESH +// cached_at and a type-mismatched "response" payload. Freshness matters: a +// zero-valued cached_at (as unparseable garbage produces) is also a miss via +// the TTL branch, so it would not exercise the unmarshal guard at all. func writeCorruptCacheFile(dir, key string) error { - return os.WriteFile(dir+"/"+key+".json", []byte("{invalid json"), 0o600) + envelope := []byte(`{"cached_at":"` + time.Now().UTC().Format(time.RFC3339Nano) + `","response":"not-an-object"}`) + return os.WriteFile(dir+"/"+key+".json", envelope, 0o600) +} + +// TestCachedEligibility_RefreshStillWrites pins that --refresh bypasses the +// cache READ but still WRITES the fresh response. Counting inner calls is not +// enough: with the write skipped, the pre-warmed entry survives and a later +// read still hits. Only re-reading and comparing the PAYLOAD detects it, which +// is why the two responses below must stay distinguishable. +func TestCachedEligibility_RefreshStillWrites(t *testing.T) { + t.Parallel() + store := NewStore(t.TempDir(), 4*time.Hour) + ctx := t.Context() + + inner := &mockEligibilityLister{ + response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{{WorkspaceID: "ws-stale"}}, + Total: 1, + }, + } + + // Pre-warm the cache with the stale payload. + if _, err := NewCachedEligibilityLister(inner, nil, store, false, nil).ListEligibility(ctx, models.CSPAzure); err != nil { + t.Fatalf("prewarm: %v", err) + } + + // Distinguishable from "ws-stale" on purpose — do not collapse these. + inner.response = &models.EligibilityResponse{ + Response: []models.EligibleTarget{{WorkspaceID: "ws-fresh"}}, + Total: 1, + } + + if _, err := NewCachedEligibilityLister(inner, nil, store, true, nil).ListEligibility(ctx, models.CSPAzure); err != nil { + t.Fatalf("refresh: %v", err) + } + if inner.calls != 2 { + t.Fatalf("refresh should bypass the read: want 2 inner calls, got %d", inner.calls) + } + + // A non-refresh read must now hit the cache AND see the refreshed payload. + resp, err := NewCachedEligibilityLister(inner, nil, store, false, nil).ListEligibility(ctx, models.CSPAzure) + if err != nil { + t.Fatalf("read back: %v", err) + } + if inner.calls != 2 { + t.Errorf("read back should have hit the cache: want 2 inner calls, got %d", inner.calls) + } + if len(resp.Response) != 1 || resp.Response[0].WorkspaceID != "ws-fresh" { + t.Errorf("cache still holds the pre-refresh payload: %+v", resp.Response) + } +} + +// TestCachedGroupsEligibility_RefreshStillWrites is the groups mirror of +// TestCachedEligibility_RefreshStillWrites. +func TestCachedGroupsEligibility_RefreshStillWrites(t *testing.T) { + t.Parallel() + store := NewStore(t.TempDir(), 4*time.Hour) + ctx := t.Context() + + inner := &mockGroupsEligibilityLister{ + response: &models.GroupsEligibilityResponse{ + Response: []models.GroupsEligibleTarget{{GroupID: "g-stale"}}, + Total: 1, + }, + } + + if _, err := NewCachedEligibilityLister(nil, inner, store, false, nil).ListGroupsEligibility(ctx, models.CSPAzure); err != nil { + t.Fatalf("prewarm: %v", err) + } + + // Distinguishable from "g-stale" on purpose — do not collapse these. + inner.response = &models.GroupsEligibilityResponse{ + Response: []models.GroupsEligibleTarget{{GroupID: "g-fresh"}}, + Total: 1, + } + + if _, err := NewCachedEligibilityLister(nil, inner, store, true, nil).ListGroupsEligibility(ctx, models.CSPAzure); err != nil { + t.Fatalf("refresh: %v", err) + } + if inner.calls != 2 { + t.Fatalf("refresh should bypass the read: want 2 inner calls, got %d", inner.calls) + } + + resp, err := NewCachedEligibilityLister(nil, inner, store, false, nil).ListGroupsEligibility(ctx, models.CSPAzure) + if err != nil { + t.Fatalf("read back: %v", err) + } + if inner.calls != 2 { + t.Errorf("read back should have hit the cache: want 2 inner calls, got %d", inner.calls) + } + if len(resp.Response) != 1 || resp.Response[0].GroupID != "g-fresh" { + t.Errorf("cache still holds the pre-refresh payload: %+v", resp.Response) + } +} + +// TestCacheKeys_DistinctPrefixes pins that cloud and group eligibility never +// share a cache file. Collapsing the prefixes makes the two responses +// cross-deserialize into each other's entries. +func TestCacheKeys_DistinctPrefixes(t *testing.T) { + t.Parallel() + for _, csp := range []models.CSP{models.CSPAzure, models.CSPAWS, models.CSPGCP} { + cloud := eligibilityCacheKey(csp) + groups := groupsEligibilityCacheKey(csp) + if cloud == groups { + t.Errorf("csp %s: cloud and groups eligibility share cache key %q", csp, cloud) + } + } +} + +// TestCacheKeys_LowercaseCSP pins the documented on-disk file names. The CSP +// constants are upper-case ("AZURE"), so without strings.ToLower the +// documented eligibility_azure.json becomes eligibility_AZURE.json. +func TestCacheKeys_LowercaseCSP(t *testing.T) { + t.Parallel() + if got, want := eligibilityCacheKey(models.CSPAzure), "eligibility_azure"; got != want { + t.Errorf("eligibilityCacheKey = %q, want %q", got, want) + } + if got, want := groupsEligibilityCacheKey(models.CSPAzure), "groups_eligibility_azure"; got != want { + t.Errorf("groupsEligibilityCacheKey = %q, want %q", got, want) + } } diff --git a/internal/cache/cached_roles_test.go b/internal/cache/cached_roles_test.go index c017450..da96db4 100644 --- a/internal/cache/cached_roles_test.go +++ b/internal/cache/cached_roles_test.go @@ -136,6 +136,41 @@ func TestCachedRolesLister_Refresh(t *testing.T) { } } +// TestCachedRoles_RefreshStillWrites pins that --refresh bypasses the cache +// READ but still WRITES. TestCachedRolesLister_Refresh above cannot see this: +// its pre-warmed entry is identical to the refreshed one, so a skipped write +// still yields a cache hit. Only distinguishable payloads detect it. +func TestCachedRoles_RefreshStillWrites(t *testing.T) { + fake := &fakeRolesLister{roles: []scamodels.OnDemandResource{{ResourceID: "role-stale"}}} + store := newTestStore(t) + req := scamodels.OnDemandRequest{WorkspaceID: "ws-1", PlatformName: "azure_ad", OrgID: "ws-1"} + + if _, err := NewCachedRolesLister(fake, store, false, nil).ListOnDemandResources(t.Context(), req); err != nil { + t.Fatalf("prewarm: %v", err) + } + + // Distinguishable from "role-stale" on purpose — do not collapse these. + fake.roles = []scamodels.OnDemandResource{{ResourceID: "role-fresh"}} + + if _, err := NewCachedRolesLister(fake, store, true, nil).ListOnDemandResources(t.Context(), req); err != nil { + t.Fatalf("refresh: %v", err) + } + if fake.callCount != 2 { + t.Fatalf("refresh should bypass the read: want 2 inner calls, got %d", fake.callCount) + } + + roles, err := NewCachedRolesLister(fake, store, false, nil).ListOnDemandResources(t.Context(), req) + if err != nil { + t.Fatalf("read back: %v", err) + } + if fake.callCount != 2 { + t.Errorf("read back should have hit the cache: want 2 inner calls, got %d", fake.callCount) + } + if len(roles) != 1 || roles[0].ResourceID != "role-fresh" { + t.Errorf("cache still holds the pre-refresh payload: %+v", roles) + } +} + func TestOnDemandRolesCacheKey_HandlesSlashes(t *testing.T) { key := onDemandRolesCacheKey("azure_resource", "/providers/Microsoft.Management/managementGroups/abc") for _, c := range key { diff --git a/internal/cache/session_tracker.go b/internal/cache/session_tracker.go index 0870b3a..897d3b9 100644 --- a/internal/cache/session_tracker.go +++ b/internal/cache/session_tracker.go @@ -9,9 +9,15 @@ type SessionRecord struct { const sessionTimestampsKey = "session_timestamps" -// maxSessionAge is the maximum age for session timestamp entries. -// Entries older than this are filtered out on read and removed on cleanup. -const maxSessionAge = 24 * time.Hour +// sessionTimestampRetention is how long a locally recorded elevation timestamp +// stays useful. Entries older than this are filtered out by SessionTimestamps. +// +// It is purely local retention for the remaining-time DISPLAY. It is not a +// session lifetime, not a session limit, and not an access-control boundary: +// dropping a timestamp only removes grant's ability to show how long a session +// has left, and has no effect on the session itself. CleanupSessions does not +// read this constant at all — it filters on activeIDs membership only. +const sessionTimestampRetention = 24 * time.Hour // RecordSession stores the elevation timestamp for a session ID. // It performs a read-modify-write on the session timestamps cache entry. @@ -22,13 +28,13 @@ func RecordSession(s *Store, sessionID string, now time.Time) error { } // SessionTimestamps returns a map of sessionID -> elevatedAt for all tracked sessions. -// Entries older than maxSessionAge are filtered out. Returns an empty map on error. +// Entries older than sessionTimestampRetention are filtered out. Returns an empty map on error. func SessionTimestamps(s *Store) map[string]time.Time { records := readRecords(s) now := s.now() result := make(map[string]time.Time, len(records)) for id, rec := range records { - if now.Sub(rec.ElevatedAt) <= maxSessionAge { + if now.Sub(rec.ElevatedAt) <= sessionTimestampRetention { result[id] = rec.ElevatedAt } } diff --git a/internal/cache/session_tracker_test.go b/internal/cache/session_tracker_test.go index dc66a99..0abf730 100644 --- a/internal/cache/session_tracker_test.go +++ b/internal/cache/session_tracker_test.go @@ -104,6 +104,77 @@ func TestSessionTimestamps_StaleFiltered(t *testing.T) { } } +// TestSessionTimestamps_RetentionBoundary pins the exact retention window used +// to filter session timestamps on read. The store TTL is set far above the +// retention window so that only sessionTimestampRetention can produce the +// filtering — otherwise the cache-entry expiry would mask it. +func TestSessionTimestamps_RetentionBoundary(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + age time.Duration + wantKey bool + }{ + {name: "one nanosecond inside the window is kept", age: sessionTimestampRetention - time.Nanosecond, wantKey: true}, + {name: "exactly at the window is kept", age: sessionTimestampRetention, wantKey: true}, + {name: "one nanosecond past the window is filtered", age: sessionTimestampRetention + time.Nanosecond, wantKey: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + elevatedAt := time.Date(2026, 2, 21, 12, 0, 0, 0, time.UTC) + now := elevatedAt + + // Store TTL deliberately dwarfs the retention window. + s := &Store{dir: t.TempDir(), ttl: 10000 * time.Hour, now: func() time.Time { return now }} + if err := RecordSession(s, "sess-1", elevatedAt); err != nil { + t.Fatalf("RecordSession() error = %v", err) + } + + now = elevatedAt.Add(tt.age) + _, got := SessionTimestamps(s)["sess-1"] + if got != tt.wantKey { + t.Errorf("age %v: present = %v, want %v", tt.age, got, tt.wantKey) + } + }) + } +} + +// TestSessionTimestampRetention_IsTwentyFourHours hardcodes the literal so the +// value cannot drift by restating the symbol under test. +func TestSessionTimestampRetention_IsTwentyFourHours(t *testing.T) { + t.Parallel() + if sessionTimestampRetention != 24*time.Hour { + t.Errorf("sessionTimestampRetention = %v, want 24h", sessionTimestampRetention) + } +} + +// TestCleanupSessions_IgnoresRetention pins that CleanupSessions filters purely +// on activeIDs membership and never consults sessionTimestampRetention: an +// entry far older than the retention window survives cleanup as long as its +// session is still active. +func TestCleanupSessions_IgnoresRetention(t *testing.T) { + t.Parallel() + elevatedAt := time.Date(2026, 2, 21, 12, 0, 0, 0, time.UTC) + now := elevatedAt + s := &Store{dir: t.TempDir(), ttl: 10000 * time.Hour, now: func() time.Time { return now }} + + if err := RecordSession(s, "sess-old", elevatedAt); err != nil { + t.Fatalf("RecordSession() error = %v", err) + } + + now = elevatedAt.Add(10 * sessionTimestampRetention) + if err := CleanupSessions(s, []string{"sess-old"}); err != nil { + t.Fatalf("CleanupSessions() error = %v", err) + } + + if _, ok := readRecords(s)["sess-old"]; !ok { + t.Error("CleanupSessions removed an active session by age; it must filter on activeIDs only") + } +} + func TestRecordSession_Append(t *testing.T) { t.Parallel() s := NewStore(t.TempDir(), 25*time.Hour) From 24cbac32636bbc8badfd585d14610127d13912e1 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:06:06 +0200 Subject: [PATCH 2/9] fix(config): reject any explicitly-invalid cache_ttl at config load ParseCacheTTL now returns (time.Duration, error). Absent still means the 4h default; unparseable, zero and negative values are all rejected, consistently. Validation runs in config.Load so a bad value surfaces at startup rather than when a command happens to build a cache. buildCachedLister and its seven command call sites propagate the error. Adds config coverage for partial-YAML defaults, non-nil Favorites, invalid YAML, ConfigDir naming, Save's 0600/0700 modes, MkdirAll failure, and a portable read-error test that also runs on the Windows CI leg. --- CHANGELOG.md | 10 +- CLAUDE.md | 4 +- cmd/env.go | 5 +- cmd/favorites.go | 5 +- cmd/list.go | 5 +- cmd/request_submit.go | 10 +- cmd/revoke.go | 5 +- cmd/root.go | 18 ++- cmd/root_test.go | 49 +++++++ cmd/status.go | 5 +- docs/mutation-ledger.md | 48 +++++++ internal/config/config.go | 23 +++- internal/config/config_test.go | 228 ++++++++++++++++++++++++++++++++- 13 files changed, 391 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c814d7a..ca9500b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,9 @@ All notable changes to this project will be documented in this file. ## [Unreleased] -### Security +### Changed -- `grant update` now refuses to install a zero-length binary from a release archive -- `grant update` now rejects non-regular zip entries, matching the existing tar behaviour +- An invalid `cache_ttl` in `~/.grant/config.yaml` is now rejected at startup; `0s` (previously never-read-still-write) and unparseable values like `garbage` (previously a silent 4h default) fail instead ### Fixed @@ -15,6 +14,11 @@ All notable changes to this project will be documented in this file. - `grant favorites add`'s non-interactive error now mentions the required favorite name, not only the flags - Interactive selectors now elevate the row you picked, not another target or Entra ID group that happens to render the same way +### Security + +- `grant update` now refuses to install a zero-length binary from a release archive +- `grant update` now rejects non-regular zip entries, matching the existing tar behaviour + ## [0.9.0] - 2026-08-14 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 819acb2..49fc11b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -145,10 +145,12 @@ Custom `SCAAccessService` follows SDK conventions: ## Cache - Eligibility responses cached in `~/.grant/cache/` as JSON files (e.g., `eligibility_azure.json`, `groups_eligibility_azure.json`) - Default TTL: 4 hours, configurable via `cache_ttl` in `~/.grant/config.yaml` (Go duration syntax: `2h`, `30m`) +- `config.ParseCacheTTL` returns `(time.Duration, error)`. **Absent** means "use the default"; **any explicitly supplied** value that cannot serve as a TTL — unparseable, zero or negative — is an error. Treating those two the same way is the point: silently defaulting `garbage` while rejecting `0s` would validate one field by two opposite rules. `config.Load` validates it so a bad value surfaces at load, not when some command happens to build a cache. `buildCachedLister` (`cmd/root.go`) therefore returns an error too — its bad-TTL arm is reachable only for a `Config` assembled in memory - `--refresh` flag on `grant` and `grant env` bypasses cache reads but still writes fresh data - `internal/cache/cache.go` — generic `Store` with `Get[T]`/`Set[T]`, injectable clock for testing - `internal/cache/cached_eligibility.go` — `CachedEligibilityLister` decorator implementing `eligibilityLister` + `groupsEligibilityLister` -- `internal/cache/session_tracker.go` — `RecordSession`, `SessionTimestamps`, `CleanupSessions` for tracking elevation timestamps in `session_timestamps.json` (25h TTL, auto-cleanup of inactive sessions) +- `internal/cache/session_tracker.go` — `RecordSession`, `SessionTimestamps`, `CleanupSessions` for tracking elevation timestamps in `session_timestamps.json` + - `sessionTimestampRetention` (24h) is **local retention for the remaining-time display only** — not a session lifetime, not a session limit, not an access-control boundary. Dropping a timestamp only costs grant the ability to show how long a session has left. `SessionTimestamps` filters on it; `CleanupSessions` does **not** read it at all — that filters purely on `activeIDs` membership - `buildCachedLister()` in `cmd/root.go` — shared factory used by all commands (root, env, status, revoke, favorites add) - Commands without `--refresh` (status, revoke, favorites add) always pass `refresh: false` — they use eligibility for display only - Cache failures (read/write) silently fall through to the live API diff --git a/cmd/env.go b/cmd/env.go index 199f3d5..d7badb3 100644 --- a/cmd/env.go +++ b/cmd/env.go @@ -60,7 +60,10 @@ func NewEnvCommand() *cobra.Command { return err } - cachedLister := buildCachedLister(cfg, flags.refresh, scaService, nil) + cachedLister, err := buildCachedLister(cfg, flags.refresh, scaService, nil) + if err != nil { + return err + } return runEnvWithDeps(cmd, flags, profile, ispAuth, cachedLister, scaService, &uiSelector{}, cfg) }) diff --git a/cmd/favorites.go b/cmd/favorites.go index f53f68c..eca6046 100644 --- a/cmd/favorites.go +++ b/cmd/favorites.go @@ -164,7 +164,10 @@ func runFavoritesAddProduction(cmd *cobra.Command, args []string) error { return err } - cachedLister := buildCachedLister(cfg, false, scaService, scaService) + cachedLister, err := buildCachedLister(cfg, false, scaService, scaService) + if err != nil { + return err + } return runFavoritesAddWithDeps(cmd, args, cachedLister, &uiUnifiedSelector{}, &surveyNamePrompter{}, cfg, cachedLister) } diff --git a/cmd/list.go b/cmd/list.go index 1baa0e0..b84f791 100644 --- a/cmd/list.go +++ b/cmd/list.go @@ -89,7 +89,10 @@ func NewListCommand() *cobra.Command { } refresh, _ := cmd.Flags().GetBool("refresh") - cachedLister := buildCachedLister(cfg, refresh, svc, svc) + cachedLister, err := buildCachedLister(cfg, refresh, svc, svc) + if err != nil { + return err + } return runList(cmd, ispAuth, cachedLister, cachedLister) }) diff --git a/cmd/request_submit.go b/cmd/request_submit.go index 4360485..f74a64c 100644 --- a/cmd/request_submit.go +++ b/cmd/request_submit.go @@ -394,7 +394,10 @@ func resolveSubmitTarget(ctx context.Context, provider, targetName string, refre if cfg == nil { cfg = config.DefaultConfig() } - cachedLister := buildCachedLister(cfg, refresh, scaSvc, nil) + cachedLister, err := buildCachedLister(cfg, refresh, scaSvc, nil) + if err != nil { + return nil, err + } fetchCtx, fetchCancel := context.WithTimeout(ctx, apiTimeout) defer fetchCancel() @@ -544,7 +547,10 @@ func resolveSubmitRole(ctx context.Context, ws *submitWorkspace, refresh bool) ( var lister cache.OnDemandRolesLister = scaSvc cacheDir, cacheErr := cache.CacheDir() if cacheErr == nil { - ttl := config.ParseCacheTTL(cfg) + ttl, ttlErr := config.ParseCacheTTL(cfg) + if ttlErr != nil { + return "", "", ttlErr + } store := cache.NewStore(cacheDir, ttl) lister = cache.NewCachedRolesLister(scaSvc, store, refresh, common.GetLogger("grant", -1)) } diff --git a/cmd/revoke.go b/cmd/revoke.go index 9091bd5..457de60 100644 --- a/cmd/revoke.go +++ b/cmd/revoke.go @@ -78,7 +78,10 @@ func NewRevokeCommand() *cobra.Command { return err } - cachedLister := buildCachedLister(cfg, false, svc, nil) + cachedLister, err := buildCachedLister(cfg, false, svc, nil) + if err != nil { + return err + } return runRevoke(cmd, args, ispAuth, svc, cachedLister, svc, &uiSessionSelector{}, &uiConfirmPrompter{}, profile) }) diff --git a/cmd/root.go b/cmd/root.go index 1abd49f..6fef39b 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -232,22 +232,30 @@ func runElevateProduction(cmd *cobra.Command, args []string) error { return err } - cachedLister := buildCachedLister(cfg, flags.refresh, scaService, scaService) + cachedLister, err := buildCachedLister(cfg, flags.refresh, scaService, scaService) + if err != nil { + return err + } return runElevateWithDeps(cmd, flags, profile, ispAuth, cachedLister, scaService, &uiUnifiedSelector{}, cachedLister, scaService, cfg) } // buildCachedLister creates a CachedEligibilityLister wrapping the given services. // If the cache directory cannot be resolved, it falls back to the unwrapped services. -func buildCachedLister(cfg *config.Config, refresh bool, cloudInner cache.EligibilityLister, groupsInner cache.GroupsEligibilityLister) *cache.CachedEligibilityLister { +// An invalid cache_ttl is an error: config.Load already rejects one, so reaching +// this with a bad value means the config was built in memory, not read from disk. +func buildCachedLister(cfg *config.Config, refresh bool, cloudInner cache.EligibilityLister, groupsInner cache.GroupsEligibilityLister) (*cache.CachedEligibilityLister, error) { cacheLog := common.GetLogger("grant", -1) cacheDir, err := cache.CacheDir() if err != nil { - return cache.NewCachedEligibilityLister(cloudInner, groupsInner, cache.NewStore("", 0), true, nil) + return cache.NewCachedEligibilityLister(cloudInner, groupsInner, cache.NewStore("", 0), true, nil), nil + } + ttl, err := config.ParseCacheTTL(cfg) + if err != nil { + return nil, err } - ttl := config.ParseCacheTTL(cfg) store := cache.NewStore(cacheDir, ttl) - return cache.NewCachedEligibilityLister(cloudInner, groupsInner, store, refresh, cacheLog) + return cache.NewCachedEligibilityLister(cloudInner, groupsInner, store, refresh, cacheLog), nil } // NewRootCommandWithDeps creates a root command with injected dependencies for testing. diff --git a/cmd/root_test.go b/cmd/root_test.go index 2618e93..ca99136 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + grantconfig "github.com/aaearon/grant-cli/internal/config" "github.com/aaearon/grant-cli/internal/sca/models" "github.com/aaearon/grant-cli/internal/ui" sdkauth "github.com/cyberark/idsec-sdk-golang/pkg/auth" @@ -59,6 +60,54 @@ func TestBootstrapISPAuth_MemoizesRepeatCalls(t *testing.T) { } } +// TestBuildCachedLister_TTL covers both arms of the cache_ttl handling in the +// shared factory: a usable value builds a lister, an unusable one is reported +// rather than silently defaulted. Load already rejects a bad value, so +// this arm is reachable only for a Config assembled in memory. +func TestBuildCachedLister_TTL(t *testing.T) { + tests := []struct { + name string + cacheTTL string + // wantErrContains empty means the call must succeed. + wantErrContains string + }{ + {name: "absent ttl uses the default", cacheTTL: ""}, + {name: "valid ttl", cacheTTL: "30m"}, + {name: "unparseable ttl", cacheTTL: "garbage", wantErrContains: `invalid cache_ttl "garbage"`}, + {name: "zero ttl", cacheTTL: "0s", wantErrContains: "must be greater than zero"}, + {name: "negative ttl", cacheTTL: "-1h", wantErrContains: "must be greater than zero"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := grantconfig.DefaultConfig() + cfg.CacheTTL = tt.cacheTTL + + lister, err := buildCachedLister(cfg, false, nil, nil) + + if tt.wantErrContains != "" { + if err == nil { + t.Fatalf("buildCachedLister() = nil error, want one containing %q", tt.wantErrContains) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %q, want it to contain %q", err, tt.wantErrContains) + } + if lister != nil { + t.Error("expected a nil lister alongside the error") + } + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if lister == nil { + t.Error("expected a lister") + } + }) + } +} + func TestNewRootCommand_SilenceFlags(t *testing.T) { cmd := newRootCommand(nil) diff --git a/cmd/status.go b/cmd/status.go index d5527ea..7224e2f 100644 --- a/cmd/status.go +++ b/cmd/status.go @@ -42,7 +42,10 @@ func NewStatusCommand() *cobra.Command { return err } - cachedLister := buildCachedLister(cfg, false, svc, svc) + cachedLister, err := buildCachedLister(cfg, false, svc, svc) + if err != nil { + return err + } // Build session timestamp tracker (best-effort) var tracker *cache.Store diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index 34c71b1..435e1b0 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -226,6 +226,54 @@ premise does not hold). | UI-08 | internal/ui | `internal/ui/group_selector.go:79` | Delete the `if len(groups) == 0` guard in `SelectGroup`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestSelectGroup_EmptyList`; guard **order** pinned by `TestSelectGroup_NonTTYEmptyList` | PR7 | done | | UI-09 | internal/ui | `internal/ui/role_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRole` | CONFIRMED | wont-fix | none — defensive-only and unreachable through `survey`, which can only return a string it was given. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | | UI-10 | internal/ui | `internal/ui/request_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRequest` | CONFIRMED | wont-fix | none — same rationale as UI-09. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | +| SFU-01 | internal/selfupdate | `internal/selfupdate/selfupdate.go:345` | `case path.IsAbs(cleaned):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath` — one guard-specific `wantErrContains` per arm, with a valid `grant` entry beside each malicious one so the "no binary" fallback cannot be the reason for the error | PR2 | todo | +| SFU-02 | internal/selfupdate | `internal/selfupdate/selfupdate.go:347` | `case strings.HasPrefix(normalized, "//"):` → `case false:`. **Production change (PR2):** move this arm *before* `path.IsAbs` — `path.Clean` collapses `//host/share/x` → `/host/share/x`, so `IsAbs` always wins and the UNC arm is unreachable. Rejection is unchanged; only the message differs | CONFIRMED | test + prod-fix | `TestCheckArchivePath/unc_path` | PR2 | todo | +| SFU-03 | internal/selfupdate | `internal/selfupdate/selfupdate.go:349` | `case hasDriveLetter(normalized):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/drive_absolute` | PR2 | todo | +| SFU-04 | internal/selfupdate | `internal/selfupdate/selfupdate.go:343` | `case name == "":` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/empty_name` | PR2 | todo | +| SFU-05 | internal/selfupdate | `internal/selfupdate/selfupdate.go:339` | `normalized := strings.ReplaceAll(name, "\\", "/")` → `normalized := name` (backslash traversal and backslash UNC then slip through) | CONFIRMED | test | `TestCheckArchivePath/backslash_traversal`, `.../backslash_unc` | PR2 | todo | +| SFU-06 | internal/selfupdate | `internal/selfupdate/selfupdate.go:359` (`hasDriveLetter`) | Narrow the check to uppercase drive letters only, so lowercase `c:\...` passes | CONFIRMED | test | `TestCheckArchivePath/lowercase_drive`, `.../forward_slash_drive` | PR2 | todo | +| SFU-07 | internal/selfupdate | `internal/selfupdate/selfupdate.go:392` | `if hdr.Typeflag != tar.TypeReg \|\| !isBinaryEntry(hdr.Name) {` → drop the `hdr.Typeflag != tar.TypeReg` operand. Probe (`Typeflag: tar.TypeSymlink, Name: "grant", Size: 0, Linkname: "/etc/passwd"`): baseline `bytes=0 err=archive does not contain a grant binary`; mutated `bytes=0 err=` — i.e. a **zero-byte self-destruct**. No symlink case exists anywhere in the package | CONFIRMED | test + prod-fix | `TestExtractBinary_RejectsNonRegularEntries` (symlink / hardlink / directory named `grant`, plus a zip directory entry). **Production:** reject a zero-length extracted binary *and* add a second non-empty check at the apply boundary (`internal/selfupdate/apply.go:50`). CHANGELOG `### Security` | PR2 | todo | +| SFU-08 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389-391` | Delete the tar declared-size guard (`if hdr.Size > maxDownloadBytes`) | CONFIRMED | test | `TestExtractFromTarGz_RejectsOversizeDecoy` — oversized **decoy** beside a valid binary | PR2 | todo | +| SFU-09 | internal/selfupdate | `internal/selfupdate/selfupdate.go:430-432` | Delete the zip declared-size guard (`if maxDownloadBytes >= 0 && f.UncompressedSize64 > uint64(maxDownloadBytes)`) | CONFIRMED | test | `TestExtractFromZip_RejectsOversizeDecoy` | PR2 | todo | +| SFU-10 | internal/selfupdate | `internal/selfupdate/apply.go:74` | Delete the `if err := syncStagedFile(target); err != nil { ... }` call. Per the consistency review this is **not** a production gap — `applyWithOptions` already returns a wrapped sync error before commit and `syncStagedFile` (`:110`) already returns `f.Sync()` errors. Scope is a seam plus tests; **no CHANGELOG entry** | CONFIRMED | test | `TestApplyWithOptions_SyncsBeforeCommit` via a `syncStagedFileFn` seam (call-order + abort-before-commit) | PR3 | todo | +| SFU-11 | internal/selfupdate | `internal/selfupdate/apply.go:110-115` | In `syncStagedFile`, ignore the `f.Sync()` error: `_ = f.Sync(); return nil` | CONFIRMED | test | `TestApplyWithOptions_AbortsOnSyncError` | PR3 | todo | +| SFU-12 | internal/selfupdate | `internal/selfupdate/apply.go:154-156` | In `InterruptedUpdate`, delete the target-exists guard (`if _, err := os.Stat(targetPath); err == nil \|\| !errors.Is(err, os.ErrNotExist) { return "", false }`). The untested case is target **present** and `.old` present — the documented Windows steady state | CONFIRMED | test | `TestInterruptedUpdate_TargetPresentWithOldBackup` | PR3 | todo | +| SFU-13 | internal/selfupdate | `internal/selfupdate/selfupdate.go:197-199` | Delete the non-200 check `if resp.StatusCode != http.StatusOK { ... }` in `fetchLatestRelease` | CONFIRMED | test | `newFixtureServerWith(t, opts)` → `TestFetchLatestRelease_Non200` | PR3 | todo | +| SFU-14 | internal/selfupdate | `internal/selfupdate/selfupdate.go:202-204` | In `fetchLatestRelease`, swallow the `json.Unmarshal` error on an empty body: `_ = json.Unmarshal(body, &rel)` | CONFIRMED | test | `TestFetchLatestRelease_EmptyBody` | PR3 | todo | +| SFU-15 | internal/selfupdate | `internal/selfupdate/selfupdate.go:205-207` | Delete `if rel.TagName == "" { return nil, errors.New("GitHub release response has no tag_name") }` | CONFIRMED | test | `TestFetchLatestRelease_EmptyTagName`; also non-200 on the **asset** and **checksums** downloads (`:229`) | PR3 | todo | +| SFU-16 | internal/selfupdate | `internal/selfupdate/version.go` (`comparePreRelease`, numeric-vs-numeric branch) | Invert the numeric-vs-numeric comparison so `rc.10` sorts before `rc.2` | CONFIRMED | test | `TestCompareVersions_NumericPrereleaseOrdering` (`rc.10` vs `rc.2`) | PR3 | todo | +| SFU-17 | internal/selfupdate | `internal/selfupdate/version.go:194` | `if !isAllDigits(part) {` → `if false {` in the core `MAJOR.MINOR.PATCH` loop, so `"1.+5.3"` is accepted | CONFIRMED | test | `TestParseVersion/invalid_core_segment` (`"1.+5.3"`) | PR3 | todo | +| SFU-18 | internal/selfupdate | `internal/selfupdate/selfupdate.go:284-287` | `if len(fields) != 2 { return fmt.Errorf("malformed line in %s: %q", ...) }` → `continue` | CONFIRMED | test | `TestVerifyChecksum_MalformedLine` | PR3 | todo | +| SFU-19 | internal/selfupdate | `internal/selfupdate/selfupdate.go:316-317` | In `extractBinary`, replace the `default:` unsupported-format error with `return extractFromTarGz(archive)` | CONFIRMED | test | `TestExtractBinary_UnsupportedFormat` | PR3 | todo | +| SFU-20 | internal/selfupdate | `internal/selfupdate/selfupdate.go:402` | Delete `if int64(len(data)) != hdr.Size { ... }` (tar truncation cross-check). **Unreachable by construction**: a successful capped read returns exactly `hdr.Size`, and earlier exhaustion returns `io.ErrUnexpectedEOF`. Hand-patched proof: header declares 40 with body `"bin"` → `bytes=40 err=`; header declares 2000 → `bytes=0 err=... unexpected EOF` | CONFIRMED | wont-fix | none — keep as defense-in-depth, comment it as unreachable, and claim no coverage. Rename `TestExtractBinaryRejectsTruncatedEntry` → `...TruncatedArchive` | PR2 | todo | +| SFU-21 | internal/selfupdate | `internal/selfupdate/selfupdate.go:437` | Delete `if uint64(len(data)) != f.UncompressedSize64 { ... }` (zip truncation cross-check). Same unreachability argument as SFU-20 | CONFIRMED | wont-fix | none — defense-in-depth, no coverage claimed | PR2 | todo | +| SFU-22 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389` vs `:430` | tar/zip size-check asymmetry. The original "zip decompression bomb" framing is **overstated**: the structural asymmetry is real (`maxDownloadBytes=10`, 5000-byte decoy → `TAR bytes=0 err=` vs `ZIP bytes=3 err=`), but `zip.NewReader` parses only the central directory and never opens skipped entries. The tar guard is load-bearing; the zip placement is a consistency point, not a vulnerability | OVERSTATED | wont-fix | Pin the asymmetry as **intentional** with a comment and a test asserting a skipped zip entry is never inflated | PR2 | todo | +| CACHE-01 | internal/cache | `internal/cache/cached_eligibility.go:84` | When `c.refresh` is true, skip the write: guard `Set(c.store, key, *resp)` with `if !c.refresh`. `--refresh` must bypass the **read** but still **write** | CONFIRMED | test | `TestCachedEligibility_RefreshStillWrites` | PR6 | done | +| CACHE-02 | internal/cache | `internal/cache/cached_eligibility.go:117` | Same mutation on the groups-eligibility write | CONFIRMED | test | `TestCachedGroupsEligibility_RefreshStillWrites` | PR6 | done | +| CACHE-03 | internal/cache | `internal/cache/cached_roles.go:53` | Same mutation on the on-demand-roles write | CONFIRMED | test | `TestCachedRoles_RefreshStillWrites` | PR6 | done | +| CACHE-04 | internal/cache | `internal/cache/cache.go:39-41` | `if err := json.Unmarshal(data, &e); err != nil { return false }` → ignore the error and fall through. `TestGet_CorruptJSON` passes today via the zero-`CachedAt` TTL branch, not the unmarshal guard | CONFIRMED | test | `TestGet_CorruptJSON` and `TestCachedEligibilityLister_CorruptCache_Fallthrough`, both rewritten to use a **fresh** `cached_at` with a type-mismatched payload so only the unmarshal guard can produce the miss | PR6 | done | +| CACHE-05 | internal/cache | `internal/cache/cached_eligibility.go:131` | `"groups_eligibility_" + ...` → `"eligibility_" + ...` (key collision with the cloud-eligibility prefix) | CONFIRMED | test | `TestCacheKeys_DistinctPrefixes` | PR6 | done | +| CACHE-06 | internal/cache | `internal/cache/cached_eligibility.go:127` and `:131` | Drop `strings.ToLower(string(csp))` from both key builders | CONFIRMED | test | `TestCacheKeys_LowercaseCSP` | PR6 | done | +| CACHE-07 | internal/cache | `internal/cache/session_tracker.go:14` | `const maxSessionAge = 24 * time.Hour` → `25 * time.Hour`. No test pins either value; code says 24h and CLAUDE.md says 25h | CONFIRMED | test + prod-fix | `TestSessionTimestamps_RetentionBoundary`, `TestSessionTimestampRetention_IsTwentyFourHours`, `TestCleanupSessions_IgnoresRetention`. **Production:** rename to `sessionTimestampRetention` (keep 24h) with a comment stating it is local retention for remaining-time display — not a session limit or access-control boundary. Also fix the factually wrong "removed on cleanup" comment: `CleanupSessions` filters on active IDs and never reads it. Drop the 25h claim from CLAUDE.md | PR6 | done | +| CFG-01 | internal/config | `internal/config/config.go:54-58` | In `Load`, return the default config for **any** read error: `if err != nil { return DefaultConfig(), nil }`. Killed on Linux by `config_test.go:197`, but `config_test.go:184-186` **skips on Windows**, so there is zero coverage on the windows-latest leg | CONFIRMED | test | `TestLoad_UnreadableIsNotTreatedAsMissing`: `Load()` errors as EISDIR on POSIX / ERROR_ACCESS_DENIED on Windows, and `errors.Is(err, os.ErrNotExist)` is false on both. (Windows half reasoned, not measured — verify on the CI leg) | PR6 | done | +| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-1h`, `garbage` plus a `30m` positive control; `TestLoad_InvalidCacheTTLErrors`; `TestBuildCachedLister_TTL` in `cmd`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go:541`. CHANGELOG `### Changed` | PR6 | done | +| CFG-03 | internal/config | `internal/config/config.go:20` | `const DefaultCacheTTL = 4 * time.Hour` → `400 * time.Hour`. The existing assertion is the tautology `want: DefaultCacheTTL` | CONFIRMED | test | `TestParseCacheTTL_DefaultIsFourHours` (assert the literal `4 * time.Hour`) | PR6 | done | +| CFG-04 | internal/config | `internal/config/config.go:60` | `cfg := DefaultConfig()` → `cfg := &Config{}` (defaults no longer survive a partial YAML file) | CONFIRMED | test | `TestLoad_PartialYAMLKeepsDefaults` | PR6 | done | +| CFG-05 | internal/config | `internal/config/config.go:65-67` | Delete `if cfg.Favorites == nil { cfg.Favorites = make(map[string]Favorite) }` | CONFIRMED | test | `TestLoad_FavoritesNeverNil` | PR6 | done | +| CFG-06 | internal/config | `internal/config/config.go:61-63` | Swallow the YAML error: `_ = yaml.Unmarshal(data, cfg)` | CONFIRMED | test | `TestLoad_InvalidYAMLErrors` | PR6 | done | +| CFG-07 | internal/config | `internal/config/config.go:103` | `filepath.Join(home, ".grant")` → `".grantx"` | CONFIRMED | test | `TestConfigDir_EndsInDotGrant` | PR6 | done | +| CFG-08 | internal/config | `internal/config/config.go:84` | `os.WriteFile(path, data, 0o600)` → `0o644` | CONFIRMED | test | `TestSave_FileAndDirModes` (file 0600 and dir 0700) with the `runtime.GOOS == "windows"` skip | PR6 | done | +| CFG-09 | internal/config | `internal/config/config.go:75` | `if err := os.MkdirAll(dir, 0o700); err != nil { ... }` → ignore the error | CONFIRMED | test | `TestSave_MkdirAllFailure` — force it portably by pointing at a path whose parent component is an existing **regular file** (ENOTDIR / ERROR_DIRECTORY), never a hardcoded `/dev/null/...`. A bare "did it error" check does **not** kill this: with the error swallowed the later `WriteFile` fails for the same reason, so the test asserts the `*fs.PathError` `Op` is `mkdir` | PR6 | done | +| UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | todo | +| UI-02 | internal/ui | `internal/ui/group_selector.go:60` | Delete the `sort.Slice(sorted, ...)` call in the group selector | CONFIRMED | test + prod-fix | Extract `sortGroupsForDisplay` so ordering is testable without a TTY; `TestSortGroupsForDisplay_CollisionOrdering` | PR7 | todo | +| UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | todo | +| UI-04 | internal/ui | `internal/ui/request_selector.go:18` | Delete the `time.Parse(time.RFC3339Nano, ts)` branch in the timestamp formatter | CONFIRMED | test | `TestFormatRequestOption_RFC3339Nano` | PR7 | todo | +| UI-05 | internal/ui | `internal/ui/role_selector.go:36` | Make the role sort case-**sensitive** (drop the `strings.ToLower` normalization in the `sort.SliceStable` less-func). Note: the raw mutation orphans the `strings` import — remove it too | CONFIRMED | test | `TestSortRolesForDisplay_MixedCase` | PR7 | todo | +| UI-06 | internal/ui | `internal/ui/selector.go:49` | Delete the `if len(targets) == 0` guard in `SelectTarget`. (`SelectRole`/`SelectRequest` equivalents are **killed**; these three are not) | CONFIRMED | test | `TestSelectTarget_EmptyList` | PR7 | todo | +| UI-07 | internal/ui | `internal/ui/session_selector.go:112` | Delete the `if len(sessions) == 0` guard in `SelectSessions` | CONFIRMED | test | `TestSelectSessions_EmptyList` | PR7 | todo | +| UI-08 | internal/ui | `internal/ui/group_selector.go:54` | Delete the `if len(groups) == 0` guard in `SelectGroup`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestSelectGroup_EmptyList` | PR7 | todo | +| UI-09 | internal/ui | `internal/ui/role_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRole` | CONFIRMED | wont-fix | none — defensive-only and unreachable through `survey`, which can only return a string it was given | PR7 | todo | +| UI-10 | internal/ui | `internal/ui/request_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRequest` | CONFIRMED | wont-fix | none — same rationale as UI-09 | PR7 | todo | --- diff --git a/internal/config/config.go b/internal/config/config.go index 15eafee..413a41e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -66,6 +66,12 @@ func Load(path string) (*Config, error) { cfg.Favorites = make(map[string]Favorite) } + // Validate here so an unusable cache_ttl surfaces at load rather than + // later, when some command happens to build a cache. + if _, err := ParseCacheTTL(cfg); err != nil { + return nil, err + } + return cfg, nil } @@ -108,16 +114,23 @@ func ConfigDir() (string, error) { } // ParseCacheTTL returns the configured cache TTL duration. -// Falls back to DefaultCacheTTL if the config value is empty or unparseable. -func ParseCacheTTL(cfg *Config) time.Duration { +// +// An absent value means "use the default". Any explicitly supplied value that +// cannot serve as a TTL — unparseable, zero or negative — is an error. The two +// are deliberately treated the same way: silently defaulting one while +// rejecting the other would validate a single field by two opposite rules. +func ParseCacheTTL(cfg *Config) (time.Duration, error) { if cfg.CacheTTL == "" { - return DefaultCacheTTL + return DefaultCacheTTL, nil } d, err := time.ParseDuration(cfg.CacheTTL) if err != nil { - return DefaultCacheTTL + return 0, fmt.Errorf("invalid cache_ttl %q: %w", cfg.CacheTTL, err) + } + if d <= 0 { + return 0, fmt.Errorf("invalid cache_ttl %q: must be greater than zero", cfg.CacheTTL) } - return d + return d, nil } // ConfigPath returns the config file path, respecting the GRANT_CONFIG env var. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index bb2beba..d276f32 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,8 @@ package config import ( + "errors" + "io/fs" "os" "path/filepath" "runtime" @@ -493,24 +495,244 @@ func TestSaveConfig_CacheTTL_OmitsEmpty(t *testing.T) { func TestParseCacheTTL(t *testing.T) { t.Parallel() tests := []struct { - name string + name string + // value is the raw cache_ttl string. value string want time.Duration + // wantErrContains empty means the call must succeed. + wantErrContains string }{ {name: "empty uses default", value: "", want: DefaultCacheTTL}, {name: "custom 2h", value: "2h", want: 2 * time.Hour}, + // Positive control: the guard below must not widen to swallow valid values. {name: "custom 30m", value: "30m", want: 30 * time.Minute}, - {name: "invalid falls back to default", value: "garbage", want: DefaultCacheTTL}, + {name: "unparseable is rejected", value: "garbage", wantErrContains: `invalid cache_ttl "garbage"`}, + {name: "zero is rejected", value: "0s", wantErrContains: "must be greater than zero"}, + {name: "negative is rejected", value: "-1h", wantErrContains: "must be greater than zero"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() cfg := &Config{CacheTTL: tt.value} - got := ParseCacheTTL(cfg) + got, err := ParseCacheTTL(cfg) + + if tt.wantErrContains != "" { + if err == nil { + t.Fatalf("ParseCacheTTL(%q) = %v, want error containing %q", tt.value, got, tt.wantErrContains) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("ParseCacheTTL(%q) error = %q, want it to contain %q", tt.value, err, tt.wantErrContains) + } + return + } + + if err != nil { + t.Fatalf("ParseCacheTTL(%q) unexpected error: %v", tt.value, err) + } if got != tt.want { t.Errorf("ParseCacheTTL(%q) = %v, want %v", tt.value, got, tt.want) } }) } } + +// TestParseCacheTTL_DefaultIsFourHours hardcodes the literal. Asserting +// `want: DefaultCacheTTL` restates the symbol under test and cannot detect a +// change to it. +func TestParseCacheTTL_DefaultIsFourHours(t *testing.T) { + t.Parallel() + got, err := ParseCacheTTL(&Config{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != 4*time.Hour { + t.Errorf("default cache TTL = %v, want 4h", got) + } +} + +// TestLoad_InvalidCacheTTLErrors pins that an unusable cache_ttl surfaces at +// config load, not later when some command happens to build a cache. +func TestLoad_InvalidCacheTTLErrors(t *testing.T) { + t.Parallel() + for _, value := range []string{"garbage", "0s", "-1h"} { + t.Run(value, func(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), "config.yaml") + content := []byte("profile: p\ncache_ttl: " + value + "\n") + if err := os.WriteFile(path, content, 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + _, err := Load(path) + if err == nil { + t.Fatalf("Load() with cache_ttl %q = nil error, want a validation error", value) + } + if !strings.Contains(err.Error(), "cache_ttl") { + t.Errorf("error = %q, want it to name cache_ttl", err) + } + }) + } +} + +// TestLoad_PartialYAMLKeepsDefaults pins that a file setting only some keys +// leaves the rest at their defaults — dropping the DefaultConfig() seed would +// silently lose `profile: grant`. +func TestLoad_PartialYAMLKeepsDefaults(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), "config.yaml") + // Deliberately sets only default_provider. + if err := os.WriteFile(path, []byte("default_provider: aws\n"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if cfg.Profile != "grant" { + t.Errorf("profile = %q, want the default %q to survive a partial file", cfg.Profile, "grant") + } + if cfg.DefaultProvider != "aws" { + t.Errorf("default_provider = %q, want %q", cfg.DefaultProvider, "aws") + } +} + +// TestLoad_FavoritesNeverNil pins the nil-map backfill. `favorites:` with no +// value unmarshals to a nil map, and writing to a nil map panics. +func TestLoad_FavoritesNeverNil(t *testing.T) { + t.Parallel() + for name, content := range map[string]string{ + "explicit null": "profile: p\nfavorites:\n", + "no favorites key": "profile: p\n", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), "config.yaml") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if cfg.Favorites == nil { + t.Fatal("Favorites is nil; a write to it would panic") + } + // Prove it is writable, which is the behavior that actually matters. + cfg.Favorites["x"] = Favorite{Provider: "azure"} + }) + } +} + +// TestLoad_InvalidYAMLErrors pins that a malformed file is reported rather +// than silently yielding a half-populated config. +func TestLoad_InvalidYAMLErrors(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), "config.yaml") + if err := os.WriteFile(path, []byte("profile: [unterminated\n"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + if _, err := Load(path); err == nil { + t.Fatal("Load() = nil error for malformed YAML, want an error") + } +} + +// TestLoad_UnreadableIsNotTreatedAsMissing is the portable sibling of +// TestLoadConfig_PermissionError, which skips on Windows and so leaves Load's +// missing-vs-unreadable distinction with zero coverage on that CI leg. A +// directory read fails as EISDIR on POSIX and ERROR_ACCESS_DENIED on Windows, +// and is os.ErrNotExist on neither — so it drives the same branch everywhere. +func TestLoad_UnreadableIsNotTreatedAsMissing(t *testing.T) { + t.Parallel() + dir := t.TempDir() + + if _, err := os.ReadFile(dir); errors.Is(err, os.ErrNotExist) { + t.Fatalf("premise broken: reading a directory reported ErrNotExist: %v", err) + } + + _, err := Load(dir) + if err == nil { + t.Fatal("Load() = nil error, want a read error rather than the default config") + } + if !strings.Contains(err.Error(), "failed to read config") { + t.Errorf("error = %q, want the read-error wrapping", err) + } +} + +// TestConfigDir_EndsInDotGrant pins the directory name users are told to look in. +func TestConfigDir_EndsInDotGrant(t *testing.T) { + dir, err := ConfigDir() + if err != nil { + t.Fatalf("ConfigDir() error = %v", err) + } + if filepath.Base(dir) != ".grant" { + t.Errorf("ConfigDir() = %q, want its last element to be %q", dir, ".grant") + } +} + +// TestSave_FileAndDirModes pins the 0600/0700 modes. The config file holds no +// secrets today, but it is the sibling of the cache and profile directories +// and users reasonably expect it to be private. +func TestSave_FileAndDirModes(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("Go synthesizes 0666/0777 for Windows files and os.Chmod there only toggles the read-only attribute, so POSIX mode bits carry no information") + } + dir := filepath.Join(t.TempDir(), "grantcfg") + path := filepath.Join(dir, "config.yaml") + + if err := Save(DefaultConfig(), path); err != nil { + t.Fatalf("Save() error = %v", err) + } + + fi, err := os.Stat(path) + if err != nil { + t.Fatalf("stat file: %v", err) + } + if got := fi.Mode().Perm(); got != 0o600 { + t.Errorf("config file mode = %#o, want 0600", got) + } + + di, err := os.Stat(dir) + if err != nil { + t.Fatalf("stat dir: %v", err) + } + if got := di.Mode().Perm(); got != 0o700 { + t.Errorf("config dir mode = %#o, want 0700", got) + } +} + +// TestSave_MkdirAllFailure pins that a directory-creation failure propagates. +// The failure is forced portably: a path component that is an existing regular +// file makes MkdirAll fail with ENOTDIR on POSIX and ERROR_DIRECTORY on +// Windows. A hardcoded /dev/null/... path would not work — on Windows that is +// an ordinary writable location. +func TestSave_MkdirAllFailure(t *testing.T) { + t.Parallel() + base := t.TempDir() + blocker := filepath.Join(base, "not-a-dir") + if err := os.WriteFile(blocker, []byte("regular file"), 0o600); err != nil { + t.Fatalf("write blocker: %v", err) + } + + path := filepath.Join(blocker, "sub", "config.yaml") + err := Save(DefaultConfig(), path) + if err == nil { + t.Fatal("Save() = nil error when the parent directory cannot be created") + } + + // A bare "did it error" check is not enough: with the MkdirAll error + // swallowed, the subsequent WriteFile fails for the same underlying + // reason and Save still returns an error. Pinning the syscall op is what + // proves the directory-creation error is the one that propagated. + var pathErr *fs.PathError + if !errors.As(err, &pathErr) { + t.Fatalf("error = %v (%T), want an *fs.PathError", err, err) + } + if pathErr.Op != "mkdir" { + t.Errorf("error op = %q, want %q — the MkdirAll failure must be the one reported", pathErr.Op, "mkdir") + } +} From 1780b5759feba82c802e5f9c6601081505419d13 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 11:05:03 +0200 Subject: [PATCH 3/9] fix(request): fail on an unloadable config in request submit Both resolution steps discarded the config.LoadDefaultWithPath error and substituted DefaultConfig(). Since Load started rejecting an invalid cache_ttl, that made `request submit` the one command where a bad value was neither honored nor reported. Propagate the error, and load the config before authenticating so the failure does not require a working auth cycle first. Extract the on-demand cache construction into buildCachedRolesLister, mirroring buildCachedLister, so its bad-TTL arm is reachable from a test. Also pin that `grant configure` still works over a broken config: it never calls Load, which is what keeps it a recovery path. --- cmd/configure_test.go | 32 ++++++++ cmd/request_submit.go | 54 ++++++++----- cmd/request_submit_config_test.go | 130 ++++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 18 deletions(-) create mode 100644 cmd/request_submit_config_test.go diff --git a/cmd/configure_test.go b/cmd/configure_test.go index aa60438..5f4e123 100644 --- a/cmd/configure_test.go +++ b/cmd/configure_test.go @@ -514,3 +514,35 @@ func TestConfigureLongHelpHasNoLegacyPath(t *testing.T) { t.Errorf("configure Long help still mentions legacy path .idsec_profiles:\n%s", long) } } + +// TestConfigure_RecoversFromInvalidCacheTTL pins that `grant configure` stays +// usable when the on-disk config is unloadable. Now that config.Load rejects an +// invalid cache_ttl, configure is the recovery path for rewriting the broken +// file — it must never read the old config, only overwrite it. +// +// Not parallel: sets GRANT_CONFIG and IDSEC_PROFILES_FOLDER for the process. +func TestConfigure_RecoversFromInvalidCacheTTL(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(cfgPath, []byte("profile: grant\ncache_ttl: garbage\n"), 0o600); err != nil { + t.Fatalf("write broken config: %v", err) + } + t.Setenv("GRANT_CONFIG", cfgPath) + t.Setenv("IDSEC_PROFILES_FOLDER", filepath.Join(dir, "profiles")) + + cmd := NewConfigureCommand() + cmd.SetOut(&strings.Builder{}) + err := runConfigure(cmd, &mockProfileSaver{}, "https://example.cyberark.cloud", "test.user@example.com") + if err != nil { + t.Fatalf("runConfigure() error = %v, want nil; configure must not read the broken config", err) + } + + // The broken value must be gone and the rewritten file must now load. + cfg, err := config.Load(cfgPath) + if err != nil { + t.Fatalf("Load() after configure = %v, want the rewritten config to be valid", err) + } + if cfg.CacheTTL != "" { + t.Errorf("cache_ttl = %q after configure, want it rewritten away", cfg.CacheTTL) + } +} diff --git a/cmd/request_submit.go b/cmd/request_submit.go index f74a64c..cee7f86 100644 --- a/cmd/request_submit.go +++ b/cmd/request_submit.go @@ -385,15 +385,19 @@ func rejectGCPWorkspace(ws *submitWorkspace) error { } func resolveSubmitTarget(ctx context.Context, provider, targetName string, refresh bool) (*submitWorkspace, error) { + // Load the config before authenticating. An unusable config — an invalid + // cache_ttl, say — must fail the command, not be quietly replaced by + // defaults, and there is no point authenticating first to find that out. + cfg, _, err := config.LoadDefaultWithPath() + if err != nil { + return nil, err + } + _, scaSvc, _, err := bootstrapSCAService() if err != nil { return nil, fmt.Errorf("failed to bootstrap SCA service: %w", err) } - cfg, _, _ := config.LoadDefaultWithPath() - if cfg == nil { - cfg = config.DefaultConfig() - } cachedLister, err := buildCachedLister(cfg, refresh, scaSvc, nil) if err != nil { return nil, err @@ -526,6 +530,24 @@ func buildRequestDetails(ws *submitWorkspace, roleID, roleName string, f *submit } } +// buildCachedRolesLister wraps an on-demand roles lister in the file cache. +// It mirrors buildCachedLister: an unresolvable cache directory falls back to +// the unwrapped service, and an invalid cache_ttl is an error. config.Load +// already rejects a bad value, so that arm is reachable only for a Config +// assembled in memory. +func buildCachedRolesLister(cfg *config.Config, refresh bool, inner cache.OnDemandRolesLister) (cache.OnDemandRolesLister, error) { + cacheDir, err := cache.CacheDir() + if err != nil { + return inner, nil + } + ttl, err := config.ParseCacheTTL(cfg) + if err != nil { + return nil, err + } + store := cache.NewStore(cacheDir, ttl) + return cache.NewCachedRolesLister(inner, store, refresh, common.GetLogger("grant", -1)), nil +} + // resolveSubmitRole fetches on-demand roles for the selected workspace and // prompts the user to choose one. Returns the role's resource_id and resource_name. func resolveSubmitRole(ctx context.Context, ws *submitWorkspace, refresh bool) (roleID, roleName string, _ error) { @@ -534,25 +556,21 @@ func resolveSubmitRole(ctx context.Context, ws *submitWorkspace, refresh bool) ( return "", "", err } - _, scaSvc, _, err := bootstrapSCAService() + // Load the config before authenticating, for the reason given in + // resolveSubmitTarget. + cfg, _, err := config.LoadDefaultWithPath() if err != nil { - return "", "", fmt.Errorf("failed to bootstrap SCA service: %w", err) + return "", "", err } - cfg, _, _ := config.LoadDefaultWithPath() - if cfg == nil { - cfg = config.DefaultConfig() + _, scaSvc, _, err := bootstrapSCAService() + if err != nil { + return "", "", fmt.Errorf("failed to bootstrap SCA service: %w", err) } - var lister cache.OnDemandRolesLister = scaSvc - cacheDir, cacheErr := cache.CacheDir() - if cacheErr == nil { - ttl, ttlErr := config.ParseCacheTTL(cfg) - if ttlErr != nil { - return "", "", ttlErr - } - store := cache.NewStore(cacheDir, ttl) - lister = cache.NewCachedRolesLister(scaSvc, store, refresh, common.GetLogger("grant", -1)) + lister, err := buildCachedRolesLister(cfg, refresh, scaSvc) + if err != nil { + return "", "", err } fetchCtx, cancel := context.WithTimeout(ctx, apiTimeout) diff --git a/cmd/request_submit_config_test.go b/cmd/request_submit_config_test.go new file mode 100644 index 0000000..5cd1a29 --- /dev/null +++ b/cmd/request_submit_config_test.go @@ -0,0 +1,130 @@ +package cmd + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + grantconfig "github.com/aaearon/grant-cli/internal/config" + scamodels "github.com/aaearon/grant-cli/internal/sca/models" +) + +// writeBadCacheTTLConfig points GRANT_CONFIG at a config whose cache_ttl is +// unusable and returns the path. +func writeBadCacheTTLConfig(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "config.yaml") + if err := os.WriteFile(path, []byte("profile: grant\ncache_ttl: garbage\n"), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + t.Setenv("GRANT_CONFIG", path) + return path +} + +// TestResolveSubmit_ConfigLoadErrorPropagates pins that `grant request submit` +// reports an unloadable config instead of substituting DefaultConfig(). Both +// resolution steps used to discard the load error, which made request submit +// the one command where an invalid cache_ttl was neither honored nor +// reported. +// +// The assertion that the error is NOT errTestBootstrapDisabled is what proves +// the config is read first: if the load moved back after bootstrapSCAService, +// the stubbed bootstrap would fail first and the config error would never be +// produced at all. +// +// Not parallel: sets GRANT_CONFIG for the process. +func TestResolveSubmit_ConfigLoadErrorPropagates(t *testing.T) { + ws := &submitWorkspace{ + WorkspaceID: "dir-1", + WorkspaceName: "Contoso Directory", + WorkspaceType: scamodels.WorkspaceType("DIRECTORY"), + OrganizationID: "org-1", + } + + tests := []struct { + name string + call func(t *testing.T) error + }{ + { + name: "resolveSubmitTarget", + call: func(t *testing.T) error { + _, err := resolveSubmitTarget(t.Context(), "azure", "anything", false) + return err + }, + }, + { + name: "resolveSubmitRole", + call: func(t *testing.T) error { + _, _, err := resolveSubmitRole(t.Context(), ws, false) + return err + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + writeBadCacheTTLConfig(t) + + err := tt.call(t) + if err == nil { + t.Fatal("got nil error, want the config load failure to propagate") + } + if errors.Is(err, errTestBootstrapDisabled) { + t.Fatalf("got the bootstrap sentinel (%v); the config must be validated before authenticating", err) + } + if !strings.Contains(err.Error(), `invalid cache_ttl "garbage"`) { + t.Errorf("error = %q, want it to name the invalid cache_ttl", err) + } + }) + } +} + +// TestBuildCachedRolesLister_TTL covers both arms of the cache_ttl handling in +// the on-demand roles factory, mirroring TestBuildCachedLister_TTL. config.Load +// already rejects a bad value, so the error arm is reachable only for a Config +// assembled in memory — which is exactly what this test builds. +func TestBuildCachedRolesLister_TTL(t *testing.T) { + tests := []struct { + name string + cacheTTL string + // wantErrContains empty means the call must succeed. + wantErrContains string + }{ + {name: "absent ttl uses the default", cacheTTL: ""}, + {name: "valid ttl", cacheTTL: "30m"}, + {name: "unparseable ttl", cacheTTL: "garbage", wantErrContains: `invalid cache_ttl "garbage"`}, + {name: "zero ttl", cacheTTL: "0s", wantErrContains: "must be greater than zero"}, + {name: "negative ttl", cacheTTL: "-1h", wantErrContains: "must be greater than zero"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := grantconfig.DefaultConfig() + cfg.CacheTTL = tt.cacheTTL + + lister, err := buildCachedRolesLister(cfg, false, nil) + + if tt.wantErrContains != "" { + if err == nil { + t.Fatalf("buildCachedRolesLister() = nil error, want one containing %q", tt.wantErrContains) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %q, want it to contain %q", err, tt.wantErrContains) + } + if lister != nil { + t.Error("expected a nil lister alongside the error") + } + return + } + + if err != nil { + t.Fatalf("buildCachedRolesLister() error = %v, want nil", err) + } + if lister == nil { + t.Fatal("buildCachedRolesLister() = nil lister without an error") + } + }) + } +} From 6239fe17b342cf642ce6614771fd60ff90d4c0fe Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 11:05:07 +0200 Subject: [PATCH 4/9] fix(config): name the config file in the load error "invalid cache_ttl" alone leaves a user with a non-default GRANT_CONFIG no indication of which file to edit. Also tighten TestLoad_InvalidYAMLErrors to assert the yaml parse text: Load now has a second error source (the cache_ttl validation) that a bare "did it error" check would accept. --- internal/config/config.go | 4 +++- internal/config/config_test.go | 44 ++++++++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 413a41e..282429b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -99,7 +99,9 @@ func LoadDefaultWithPath() (*Config, string, error) { } cfg, err := Load(cfgPath) if err != nil { - return nil, "", fmt.Errorf("failed to load config: %w", err) + // Name the file: with GRANT_CONFIG set, the value alone leaves the + // user guessing which config to edit. + return nil, "", fmt.Errorf("failed to load config %s: %w", cfgPath, err) } return cfg, cfgPath, nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d276f32..8adbb23 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -635,9 +635,43 @@ func TestLoad_InvalidYAMLErrors(t *testing.T) { t.Fatalf("write: %v", err) } - if _, err := Load(path); err == nil { + _, err := Load(path) + if err == nil { t.Fatal("Load() = nil error for malformed YAML, want an error") } + // Assert the parse error specifically. "did it error" would also be + // satisfied by an unrelated failure — a read error, or the cache_ttl + // validation that now also runs inside Load. + if !strings.Contains(err.Error(), "yaml:") { + t.Errorf("error = %q, want the YAML parse error", err) + } + if !strings.Contains(err.Error(), "did not find expected ',' or ']'") { + t.Errorf("error = %q, want it to describe the unterminated sequence", err) + } +} + +// TestLoadDefaultWithPath_ErrorNamesTheFile pins that a load failure names the +// config file. With GRANT_CONFIG pointed somewhere non-default, naming only +// the offending value leaves the user with no idea which file to edit. +// +// Not parallel: sets GRANT_CONFIG for the process. +func TestLoadDefaultWithPath_ErrorNamesTheFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "custom-grant.yaml") + if err := os.WriteFile(path, []byte("cache_ttl: garbage\n"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + t.Setenv("GRANT_CONFIG", path) + + _, _, err := LoadDefaultWithPath() + if err == nil { + t.Fatal("LoadDefaultWithPath() = nil error, want the invalid cache_ttl reported") + } + if !strings.Contains(err.Error(), path) { + t.Errorf("error = %q, want it to name the config path %q", err, path) + } + if !strings.Contains(err.Error(), `invalid cache_ttl "garbage"`) { + t.Errorf("error = %q, want it to name the offending value", err) + } } // TestLoad_UnreadableIsNotTreatedAsMissing is the portable sibling of @@ -707,9 +741,11 @@ func TestSave_FileAndDirModes(t *testing.T) { // TestSave_MkdirAllFailure pins that a directory-creation failure propagates. // The failure is forced portably: a path component that is an existing regular -// file makes MkdirAll fail with ENOTDIR on POSIX and ERROR_DIRECTORY on -// Windows. A hardcoded /dev/null/... path would not work — on Windows that is -// an ordinary writable location. +// file makes MkdirAll fail with ENOTDIR on both platforms. No Windows error +// code is involved — os.MkdirAll (os/path.go) stats the parent itself and +// synthesizes &PathError{Op: "mkdir", Err: syscall.ENOTDIR} in platform- +// independent Go. A hardcoded /dev/null/... path would not work — on Windows +// that is an ordinary writable location. func TestSave_MkdirAllFailure(t *testing.T) { t.Parallel() base := t.TempDir() From a64653d19f6919261f9e7ed115284241659bc681 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 11:05:12 +0200 Subject: [PATCH 5/9] docs: correct the cache_ttl changelog entry and the MkdirAll claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changelog entry was 203 chars against the documented ~120, carried mechanism notes that belong in the PR body, and said "at startup" — which is wrong in both directions: nothing fails before a command reaches config.Load, and `configure` never reaches it at all. The "ERROR_DIRECTORY on Windows" claim was also wrong. os.MkdirAll (os/path.go) stats the parent itself and synthesizes &PathError{Op: "mkdir", Err: syscall.ENOTDIR} in platform-independent Go, so it is ENOTDIR on both platforms. Corrected in CLAUDE.md, the test comment and the ledger. Record in the ledger that the seven buildCachedLister call sites' error propagation remains unpinned, and why it is not worth restructuring production code to fix. --- CHANGELOG.md | 2 +- CLAUDE.md | 2 +- docs/mutation-ledger.md | 8 +++++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca9500b..06ba15a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. ### Changed -- An invalid `cache_ttl` in `~/.grant/config.yaml` is now rejected at startup; `0s` (previously never-read-still-write) and unparseable values like `garbage` (previously a silent 4h default) fail instead +- An invalid `cache_ttl` (unparseable, zero or negative) now fails the command instead of silently defaulting ### Fixed diff --git a/CLAUDE.md b/CLAUDE.md index 49fc11b..ef05ae0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -230,7 +230,7 @@ make clean # Clean build artifacts - `.golangci.yml` sets `run.build-tags: [integration, selfupdate_e2e]` so both tagged files are linted. Side effect: with `integration` set, `cmd/main_test.go` (`//go:build !integration`) is excluded from linting - Lint (`golangci-lint-action`) runs on Linux only — a second pass on Windows adds minutes and finds nothing new - Tests must be OS-portable. Never assert POSIX permission bits without a `runtime.GOOS == "windows"` skip: Go synthesizes `0666`/`0777` for Windows files and `os.Chmod` there only toggles the read-only attribute. Current skips: `internal/config/config_test.go` (`TestLoadConfig_PermissionError`, `TestConfigDir_Error` — chmod 0000 and `HOME`) and `internal/cache/cache_test.go` (`TestSet_FilePermissions`) -- Prefer a portable construction over a skip where one exists. To force a write failure, point at a path whose parent component is an existing regular file (`MkdirAll` fails with ENOTDIR on POSIX and ERROR_DIRECTORY on Windows) rather than a hardcoded `/dev/null/...` path, which is an ordinary writable location on Windows +- Prefer a portable construction over a skip where one exists. To force a write failure, point at a path whose parent component is an existing regular file rather than a hardcoded `/dev/null/...` path, which is an ordinary writable location on Windows. `MkdirAll` fails with `ENOTDIR` on **both** platforms — no Windows error code is involved: `os.MkdirAll` (`os/path.go`) stats the parent itself and synthesizes `&PathError{Op: "mkdir", Err: syscall.ENOTDIR}` in platform-independent Go, which is also why asserting `Op == "mkdir"` is portable ## CHANGELOG Style Entries are short and concise. This applies to `[Unreleased]` and everything added from now on; already-released sections are published history and are not rewritten. diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index 435e1b0..961f7d1 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -256,14 +256,14 @@ premise does not hold). | CACHE-06 | internal/cache | `internal/cache/cached_eligibility.go:127` and `:131` | Drop `strings.ToLower(string(csp))` from both key builders | CONFIRMED | test | `TestCacheKeys_LowercaseCSP` | PR6 | done | | CACHE-07 | internal/cache | `internal/cache/session_tracker.go:14` | `const maxSessionAge = 24 * time.Hour` → `25 * time.Hour`. No test pins either value; code says 24h and CLAUDE.md says 25h | CONFIRMED | test + prod-fix | `TestSessionTimestamps_RetentionBoundary`, `TestSessionTimestampRetention_IsTwentyFourHours`, `TestCleanupSessions_IgnoresRetention`. **Production:** rename to `sessionTimestampRetention` (keep 24h) with a comment stating it is local retention for remaining-time display — not a session limit or access-control boundary. Also fix the factually wrong "removed on cleanup" comment: `CleanupSessions` filters on active IDs and never reads it. Drop the 25h claim from CLAUDE.md | PR6 | done | | CFG-01 | internal/config | `internal/config/config.go:54-58` | In `Load`, return the default config for **any** read error: `if err != nil { return DefaultConfig(), nil }`. Killed on Linux by `config_test.go:197`, but `config_test.go:184-186` **skips on Windows**, so there is zero coverage on the windows-latest leg | CONFIRMED | test | `TestLoad_UnreadableIsNotTreatedAsMissing`: `Load()` errors as EISDIR on POSIX / ERROR_ACCESS_DENIED on Windows, and `errors.Is(err, os.ErrNotExist)` is false on both. (Windows half reasoned, not measured — verify on the CI leg) | PR6 | done | -| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-1h`, `garbage` plus a `30m` positive control; `TestLoad_InvalidCacheTTLErrors`; `TestBuildCachedLister_TTL` in `cmd`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go:541`. CHANGELOG `### Changed` | PR6 | done | +| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-1h`, `garbage` plus a `30m` positive control; `TestLoad_InvalidCacheTTLErrors`; `TestBuildCachedLister_TTL` in `cmd`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go`. CHANGELOG `### Changed`. **Ripple coverage, honestly:** the two `request_submit.go` sites are covered — they discarded the `Load` error and substituted `DefaultConfig()`, which made `request submit` the one command where an invalid `cache_ttl` was neither honored nor reported; they now propagate it (`TestResolveSubmit_ConfigLoadErrorPropagates`), and the cache construction is extracted into `buildCachedRolesLister` so its bad-TTL arm is reachable (`TestBuildCachedRolesLister_TTL`). The **seven `buildCachedLister` call sites' error propagation is NOT pinned**: mutating each to `cachedLister, _ := ...` all SURVIVED. They sit in production `RunE` closures that unit tests cannot reach because `bootstrapImpl` is stubbed to fail first. Static reading confirms all eight sites are correct — no `if err != nil { ttl = Default }` anywhere, and no remaining discard. Deliberately **not** fixed by restructuring production code to make them testable: the payoff is a duplicated propagation check on a path `config.Load` validation already covers. Note also that `config.Load` is not reached "at startup" — commands that authenticate first (`status`, `list`, root, `env`, `request submit`) surface an auth failure before the config error when unauthenticated, and `configure` never calls `Load` at all (deliberately: it is the recovery path, pinned by `TestConfigure_RecoversFromInvalidCacheTTL`) | PR6 | done | | CFG-03 | internal/config | `internal/config/config.go:20` | `const DefaultCacheTTL = 4 * time.Hour` → `400 * time.Hour`. The existing assertion is the tautology `want: DefaultCacheTTL` | CONFIRMED | test | `TestParseCacheTTL_DefaultIsFourHours` (assert the literal `4 * time.Hour`) | PR6 | done | | CFG-04 | internal/config | `internal/config/config.go:60` | `cfg := DefaultConfig()` → `cfg := &Config{}` (defaults no longer survive a partial YAML file) | CONFIRMED | test | `TestLoad_PartialYAMLKeepsDefaults` | PR6 | done | | CFG-05 | internal/config | `internal/config/config.go:65-67` | Delete `if cfg.Favorites == nil { cfg.Favorites = make(map[string]Favorite) }` | CONFIRMED | test | `TestLoad_FavoritesNeverNil` | PR6 | done | -| CFG-06 | internal/config | `internal/config/config.go:61-63` | Swallow the YAML error: `_ = yaml.Unmarshal(data, cfg)` | CONFIRMED | test | `TestLoad_InvalidYAMLErrors` | PR6 | done | +| CFG-06 | internal/config | `internal/config/config.go:61-63` | Swallow the YAML error: `_ = yaml.Unmarshal(data, cfg)` | CONFIRMED | test | `TestLoad_InvalidYAMLErrors`, asserting the yaml parse text (`yaml:` plus `did not find expected ',' or ']'`) rather than merely that an error occurred — `Load` now has a second error source (the `cache_ttl` validation) that a bare check would accept | PR6 | done | | CFG-07 | internal/config | `internal/config/config.go:103` | `filepath.Join(home, ".grant")` → `".grantx"` | CONFIRMED | test | `TestConfigDir_EndsInDotGrant` | PR6 | done | | CFG-08 | internal/config | `internal/config/config.go:84` | `os.WriteFile(path, data, 0o600)` → `0o644` | CONFIRMED | test | `TestSave_FileAndDirModes` (file 0600 and dir 0700) with the `runtime.GOOS == "windows"` skip | PR6 | done | -| CFG-09 | internal/config | `internal/config/config.go:75` | `if err := os.MkdirAll(dir, 0o700); err != nil { ... }` → ignore the error | CONFIRMED | test | `TestSave_MkdirAllFailure` — force it portably by pointing at a path whose parent component is an existing **regular file** (ENOTDIR / ERROR_DIRECTORY), never a hardcoded `/dev/null/...`. A bare "did it error" check does **not** kill this: with the error swallowed the later `WriteFile` fails for the same reason, so the test asserts the `*fs.PathError` `Op` is `mkdir` | PR6 | done | +| CFG-09 | internal/config | `internal/config/config.go:75` | `if err := os.MkdirAll(dir, 0o700); err != nil { ... }` → ignore the error | CONFIRMED | test | `TestSave_MkdirAllFailure` — force it portably by pointing at a path whose parent component is an existing **regular file**, never a hardcoded `/dev/null/...`. `os.MkdirAll` (`os/path.go`) stats the parent itself and synthesizes `&PathError{Op: "mkdir", Err: syscall.ENOTDIR}` in platform-independent Go, so it is ENOTDIR on **both** platforms — no Windows error code is involved, which is also why the `Op` assertion is portable. A bare "did it error" check does **not** kill this: with the error swallowed the later `WriteFile` fails for the same reason, so the test asserts the `*fs.PathError` `Op` is `mkdir` | PR6 | done | | UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | todo | | UI-02 | internal/ui | `internal/ui/group_selector.go:60` | Delete the `sort.Slice(sorted, ...)` call in the group selector | CONFIRMED | test + prod-fix | Extract `sortGroupsForDisplay` so ordering is testable without a TTY; `TestSortGroupsForDisplay_CollisionOrdering` | PR7 | todo | | UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | todo | @@ -321,6 +321,8 @@ The seven production changes from the plan's table, plus two added by the PR7 re | SFU-02 | UNC check before `path.IsAbs` (message only) | PR2 | no | | OUT-26 | `favorites add` early non-interactive guard, favorites-specific message | PR1 | `### Fixed` | | CFG-02 | `ParseCacheTTL` errors on any explicitly-invalid value | PR6 | `### Changed` | +| CFG-02 | `request submit` propagates the config-load error instead of substituting `DefaultConfig()`, and loads the config before authenticating | PR6 | covered by the CFG-02 entry | +| CFG-02 | `LoadDefaultWithPath` names the config file in its error | PR6 | no (error-message detail) | | CACHE-07 | `maxSessionAge` → `sessionTimestampRetention` | PR6 | no (internal) | | UI-02 | `sortGroupsForDisplay` extraction | PR7 | no (refactor; since removed with `SelectGroup`) | | SFU-10 | `syncStagedFileFn` seam | PR3 | no (test seam; **not** a production gap) | From 22b86ef8092ad4a09154e2a3c97ff2935f64a88d Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:29:51 +0200 Subject: [PATCH 6/9] fix(config): point the invalid cache_ttl error at a remedy Name the expected duration syntax on the unparseable arm (still wrapping the time.ParseDuration error) and name --refresh on the non-positive arm, so someone who used 0s as a cache kill-switch has a replacement. --- CHANGELOG.md | 2 +- internal/config/config.go | 4 +-- internal/config/config_test.go | 54 +++++++++++++++++++++++++++++----- 3 files changed, 49 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06ba15a..704ba07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. ### Changed -- An invalid `cache_ttl` (unparseable, zero or negative) now fails the command instead of silently defaulting +- An invalid `cache_ttl` (unparseable, zero or negative) now fails the command instead of silently defaulting; the error names the config file, the expected duration syntax and `--refresh` ### Fixed diff --git a/internal/config/config.go b/internal/config/config.go index 282429b..2d38e49 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -127,10 +127,10 @@ func ParseCacheTTL(cfg *Config) (time.Duration, error) { } d, err := time.ParseDuration(cfg.CacheTTL) if err != nil { - return 0, fmt.Errorf("invalid cache_ttl %q: %w", cfg.CacheTTL, err) + return 0, fmt.Errorf("invalid cache_ttl %q: must be a positive Go duration such as 4h or 30m: %w", cfg.CacheTTL, err) } if d <= 0 { - return 0, fmt.Errorf("invalid cache_ttl %q: must be greater than zero", cfg.CacheTTL) + return 0, fmt.Errorf("invalid cache_ttl %q: must be greater than zero; use --refresh to bypass the cache for a single command", cfg.CacheTTL) } return d, nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 8adbb23..882eef3 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -499,16 +499,43 @@ func TestParseCacheTTL(t *testing.T) { // value is the raw cache_ttl string. value string want time.Duration - // wantErrContains empty means the call must succeed. - wantErrContains string + // wantErrContains empty means the call must succeed. Every entry must + // appear in the error: rejecting a value is only half the job, the + // message must also say what a valid one looks like. + wantErrContains []string }{ {name: "empty uses default", value: "", want: DefaultCacheTTL}, {name: "custom 2h", value: "2h", want: 2 * time.Hour}, // Positive control: the guard below must not widen to swallow valid values. {name: "custom 30m", value: "30m", want: 30 * time.Minute}, - {name: "unparseable is rejected", value: "garbage", wantErrContains: `invalid cache_ttl "garbage"`}, - {name: "zero is rejected", value: "0s", wantErrContains: "must be greater than zero"}, - {name: "negative is rejected", value: "-1h", wantErrContains: "must be greater than zero"}, + { + name: "unparseable is rejected with the expected syntax", + value: "garbage", + wantErrContains: []string{ + `invalid cache_ttl "garbage"`, + "must be a positive Go duration such as 4h or 30m", + // The wrapped time.ParseDuration error must survive. + `time: invalid duration "garbage"`, + }, + }, + { + name: "zero is rejected and names the --refresh alternative", + value: "0s", + wantErrContains: []string{ + `invalid cache_ttl "0s"`, + "must be greater than zero", + "use --refresh to bypass the cache for a single command", + }, + }, + { + name: "negative is rejected and names the --refresh alternative", + value: "-1h", + wantErrContains: []string{ + `invalid cache_ttl "-1h"`, + "must be greater than zero", + "use --refresh to bypass the cache for a single command", + }, + }, } for _, tt := range tests { @@ -517,12 +544,14 @@ func TestParseCacheTTL(t *testing.T) { cfg := &Config{CacheTTL: tt.value} got, err := ParseCacheTTL(cfg) - if tt.wantErrContains != "" { + if len(tt.wantErrContains) > 0 { if err == nil { t.Fatalf("ParseCacheTTL(%q) = %v, want error containing %q", tt.value, got, tt.wantErrContains) } - if !strings.Contains(err.Error(), tt.wantErrContains) { - t.Errorf("ParseCacheTTL(%q) error = %q, want it to contain %q", tt.value, err, tt.wantErrContains) + for _, want := range tt.wantErrContains { + if !strings.Contains(err.Error(), want) { + t.Errorf("ParseCacheTTL(%q) error = %q, want it to contain %q", tt.value, err, want) + } } return } @@ -672,6 +701,15 @@ func TestLoadDefaultWithPath_ErrorNamesTheFile(t *testing.T) { if !strings.Contains(err.Error(), `invalid cache_ttl "garbage"`) { t.Errorf("error = %q, want it to name the offending value", err) } + if !strings.Contains(err.Error(), "must be a positive Go duration such as 4h or 30m") { + t.Errorf("error = %q, want it to say what a valid value looks like", err) + } + // The remedy is to edit the named file. `grant configure` rewrites the + // config from scratch and drops favorites and default_provider, so the + // error must never send the user there. + if strings.Contains(err.Error(), "grant configure") { + t.Errorf("error = %q, must not suggest `grant configure` as the remedy", err) + } } // TestLoad_UnreadableIsNotTreatedAsMissing is the portable sibling of From 12400160666acab552407847717f5e2d0a89507f Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:29:51 +0200 Subject: [PATCH 7/9] test(configure): stop reading configure as the endorsed recovery path runConfigure rebuilds the config from scratch, dropping favorites and default_provider. Rename the test, pin that loss, and record the sharp edge in CLAUDE.md; the remedy is to edit the file the error names. --- CLAUDE.md | 3 ++- cmd/configure_test.go | 32 ++++++++++++++++++++++++++------ docs/mutation-ledger.md | 2 +- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ef05ae0..dc18bc6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -145,7 +145,7 @@ Custom `SCAAccessService` follows SDK conventions: ## Cache - Eligibility responses cached in `~/.grant/cache/` as JSON files (e.g., `eligibility_azure.json`, `groups_eligibility_azure.json`) - Default TTL: 4 hours, configurable via `cache_ttl` in `~/.grant/config.yaml` (Go duration syntax: `2h`, `30m`) -- `config.ParseCacheTTL` returns `(time.Duration, error)`. **Absent** means "use the default"; **any explicitly supplied** value that cannot serve as a TTL — unparseable, zero or negative — is an error. Treating those two the same way is the point: silently defaulting `garbage` while rejecting `0s` would validate one field by two opposite rules. `config.Load` validates it so a bad value surfaces at load, not when some command happens to build a cache. `buildCachedLister` (`cmd/root.go`) therefore returns an error too — its bad-TTL arm is reachable only for a `Config` assembled in memory +- `config.ParseCacheTTL` returns `(time.Duration, error)`. **Absent** means "use the default"; **any explicitly supplied** value that cannot serve as a TTL — unparseable, zero or negative — is an error. Treating those two the same way is the point: silently defaulting `garbage` while rejecting `0s` would validate one field by two opposite rules. `config.Load` validates it so a bad value surfaces at load, not when some command happens to build a cache. `buildCachedLister` (`cmd/root.go`) therefore returns an error too — its bad-TTL arm is reachable only for a `Config` assembled in memory. Both rejection messages must name a remedy: the unparseable arm names the expected syntax (`must be a positive Go duration such as 4h or 30m`) and still wraps the `time.ParseDuration` error with `%w`; the non-positive arm names `--refresh` as the way to bypass the cache for one command, since `0s` used to work as an accidental kill-switch. Neither may point at `grant configure` (see Config) - `--refresh` flag on `grant` and `grant env` bypasses cache reads but still writes fresh data - `internal/cache/cache.go` — generic `Store` with `Get[T]`/`Set[T]`, injectable clock for testing - `internal/cache/cached_eligibility.go` — `CachedEligibilityLister` decorator implementing `eligibilityLister` + `groupsEligibilityLister` @@ -170,6 +170,7 @@ Custom `SCAAccessService` follows SDK conventions: ## Config - App config: `~/.grant/config.yaml` - SDK profile: `~/.idsec/profiles/grant` (default; override via `IDSEC_PROFILES_FOLDER`) +- **`runConfigure` rebuilds the config from scratch** (`cmd/configure.go`): it never reads the existing file, it constructs a fresh `&config.Config{}` with a hardcoded `default_provider` and an empty `Favorites` map and `Save`s that, so every favorite and the user's `default_provider` are silently destroyed. That is also what keeps `grant configure` reachable when the on-disk config is unloadable (the no-lockout property, pinned by `TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites`), so it is not purely a bug — but it means **`grant configure` must never be advertised as the remedy for a bad config value**. The user-facing remedy is to edit the file, whose path every load error names. Known sharp edge, follow-up, deliberately not fixed here - Always resolve the profile directory with `profiles.GetProfilesFolder()` (SDK) — never hand-roll it. The SDK reads `os.Getenv("HOME")`, not `os.UserHomeDir()`; on Windows `HOME` is frequently unset, so it resolves to a **relative** `.idsec/profiles` under the process CWD. Any code that prints or computes the profile path must agree with the loader, so reproduce the SDK's behavior rather than "correcting" it ## Keyring diff --git a/cmd/configure_test.go b/cmd/configure_test.go index 5f4e123..2695b81 100644 --- a/cmd/configure_test.go +++ b/cmd/configure_test.go @@ -515,16 +515,26 @@ func TestConfigureLongHelpHasNoLegacyPath(t *testing.T) { } } -// TestConfigure_RecoversFromInvalidCacheTTL pins that `grant configure` stays -// usable when the on-disk config is unloadable. Now that config.Load rejects an -// invalid cache_ttl, configure is the recovery path for rewriting the broken -// file — it must never read the old config, only overwrite it. +// TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites pins what +// `grant configure` actually does to a config it cannot load: it never reads the +// old file, it builds a fresh Config from scratch and writes that over the top. +// +// That keeps configure reachable with a broken config — the no-lockout property +// — but it is NOT an endorsed recovery path for a bad cache_ttl, and no error +// text, doc or help string should point a user at it. runConfigure discards +// every favorite and resets default_provider, so using it to fix a +// one-character typo silently destroys unrelated config. The documented remedy +// is to edit the file named in the load error. The favorites assertion below +// pins that loss so it stays visible; it is recorded behavior, not desired +// behavior, and is flagged as a follow-up in CLAUDE.md. // // Not parallel: sets GRANT_CONFIG and IDSEC_PROFILES_FOLDER for the process. -func TestConfigure_RecoversFromInvalidCacheTTL(t *testing.T) { +func TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites(t *testing.T) { dir := t.TempDir() cfgPath := filepath.Join(dir, "config.yaml") - if err := os.WriteFile(cfgPath, []byte("profile: grant\ncache_ttl: garbage\n"), 0o600); err != nil { + broken := "profile: grant\ndefault_provider: aws\ncache_ttl: garbage\n" + + "favorites:\n prod:\n provider: azure\n target: Prod-EastUS\n role: Contributor\n" + if err := os.WriteFile(cfgPath, []byte(broken), 0o600); err != nil { t.Fatalf("write broken config: %v", err) } t.Setenv("GRANT_CONFIG", cfgPath) @@ -545,4 +555,14 @@ func TestConfigure_RecoversFromInvalidCacheTTL(t *testing.T) { if cfg.CacheTTL != "" { t.Errorf("cache_ttl = %q after configure, want it rewritten away", cfg.CacheTTL) } + + // The sharp edge: everything else in the file went with it. Pinned so a + // future change to runConfigure has to acknowledge this, and so nobody + // mistakes configure for a safe repair tool. + if len(cfg.Favorites) != 0 { + t.Errorf("favorites = %v after configure, want them clobbered (pinned pre-existing behavior)", cfg.Favorites) + } + if cfg.DefaultProvider != "azure" { + t.Errorf("default_provider = %q after configure, want the hardcoded %q (the user's \"aws\" is lost)", cfg.DefaultProvider, "azure") + } } diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index 961f7d1..fb74612 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -256,7 +256,7 @@ premise does not hold). | CACHE-06 | internal/cache | `internal/cache/cached_eligibility.go:127` and `:131` | Drop `strings.ToLower(string(csp))` from both key builders | CONFIRMED | test | `TestCacheKeys_LowercaseCSP` | PR6 | done | | CACHE-07 | internal/cache | `internal/cache/session_tracker.go:14` | `const maxSessionAge = 24 * time.Hour` → `25 * time.Hour`. No test pins either value; code says 24h and CLAUDE.md says 25h | CONFIRMED | test + prod-fix | `TestSessionTimestamps_RetentionBoundary`, `TestSessionTimestampRetention_IsTwentyFourHours`, `TestCleanupSessions_IgnoresRetention`. **Production:** rename to `sessionTimestampRetention` (keep 24h) with a comment stating it is local retention for remaining-time display — not a session limit or access-control boundary. Also fix the factually wrong "removed on cleanup" comment: `CleanupSessions` filters on active IDs and never reads it. Drop the 25h claim from CLAUDE.md | PR6 | done | | CFG-01 | internal/config | `internal/config/config.go:54-58` | In `Load`, return the default config for **any** read error: `if err != nil { return DefaultConfig(), nil }`. Killed on Linux by `config_test.go:197`, but `config_test.go:184-186` **skips on Windows**, so there is zero coverage on the windows-latest leg | CONFIRMED | test | `TestLoad_UnreadableIsNotTreatedAsMissing`: `Load()` errors as EISDIR on POSIX / ERROR_ACCESS_DENIED on Windows, and `errors.Is(err, os.ErrNotExist)` is false on both. (Windows half reasoned, not measured — verify on the CI leg) | PR6 | done | -| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-1h`, `garbage` plus a `30m` positive control; `TestLoad_InvalidCacheTTLErrors`; `TestBuildCachedLister_TTL` in `cmd`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go`. CHANGELOG `### Changed`. **Ripple coverage, honestly:** the two `request_submit.go` sites are covered — they discarded the `Load` error and substituted `DefaultConfig()`, which made `request submit` the one command where an invalid `cache_ttl` was neither honored nor reported; they now propagate it (`TestResolveSubmit_ConfigLoadErrorPropagates`), and the cache construction is extracted into `buildCachedRolesLister` so its bad-TTL arm is reachable (`TestBuildCachedRolesLister_TTL`). The **seven `buildCachedLister` call sites' error propagation is NOT pinned**: mutating each to `cachedLister, _ := ...` all SURVIVED. They sit in production `RunE` closures that unit tests cannot reach because `bootstrapImpl` is stubbed to fail first. Static reading confirms all eight sites are correct — no `if err != nil { ttl = Default }` anywhere, and no remaining discard. Deliberately **not** fixed by restructuring production code to make them testable: the payoff is a duplicated propagation check on a path `config.Load` validation already covers. Note also that `config.Load` is not reached "at startup" — commands that authenticate first (`status`, `list`, root, `env`, `request submit`) surface an auth failure before the config error when unauthenticated, and `configure` never calls `Load` at all (deliberately: it is the recovery path, pinned by `TestConfigure_RecoversFromInvalidCacheTTL`) | PR6 | done | +| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-1h`, `garbage` plus a `30m` positive control; `TestLoad_InvalidCacheTTLErrors`; `TestBuildCachedLister_TTL` in `cmd`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go`. CHANGELOG `### Changed`. **Ripple coverage, honestly:** the two `request_submit.go` sites are covered — they discarded the `Load` error and substituted `DefaultConfig()`, which made `request submit` the one command where an invalid `cache_ttl` was neither honored nor reported; they now propagate it (`TestResolveSubmit_ConfigLoadErrorPropagates`), and the cache construction is extracted into `buildCachedRolesLister` so its bad-TTL arm is reachable (`TestBuildCachedRolesLister_TTL`). The **seven `buildCachedLister` call sites' error propagation is NOT pinned**: mutating each to `cachedLister, _ := ...` all SURVIVED. They sit in production `RunE` closures that unit tests cannot reach because `bootstrapImpl` is stubbed to fail first. Static reading confirms all eight sites are correct — no `if err != nil { ttl = Default }` anywhere, and no remaining discard. Deliberately **not** fixed by restructuring production code to make them testable: the payoff is a duplicated propagation check on a path `config.Load` validation already covers. Note also that `config.Load` is not reached "at startup" — commands that authenticate first (`status`, `list`, root, `env`, `request submit`) surface an auth failure before the config error when unauthenticated, and `configure` never calls `Load` at all (deliberately, so it stays reachable with a broken file — but it is **not** the advertised remedy: it rebuilds the config from scratch and drops favorites and `default_provider`, pinned by `TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites`. The user-facing remedy is to edit the file named in the load error) | PR6 | done | | CFG-03 | internal/config | `internal/config/config.go:20` | `const DefaultCacheTTL = 4 * time.Hour` → `400 * time.Hour`. The existing assertion is the tautology `want: DefaultCacheTTL` | CONFIRMED | test | `TestParseCacheTTL_DefaultIsFourHours` (assert the literal `4 * time.Hour`) | PR6 | done | | CFG-04 | internal/config | `internal/config/config.go:60` | `cfg := DefaultConfig()` → `cfg := &Config{}` (defaults no longer survive a partial YAML file) | CONFIRMED | test | `TestLoad_PartialYAMLKeepsDefaults` | PR6 | done | | CFG-05 | internal/config | `internal/config/config.go:65-67` | Delete `if cfg.Favorites == nil { cfg.Favorites = make(map[string]Favorite) }` | CONFIRMED | test | `TestLoad_FavoritesNeverNil` | PR6 | done | From 126809d69197e3421cff08a5220c6056eb9b0c56 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:29:51 +0200 Subject: [PATCH 8/9] docs(readme): state the cache_ttl validation rule --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 4070f39..b5bd225 100644 --- a/README.md +++ b/README.md @@ -189,6 +189,8 @@ favorites: role: "AdministratorAccess" ``` +`cache_ttl` must be a positive Go duration (`4h`, `30m`); omit it for the 4h default. A zero, negative or unparseable value is a fatal error at config load — edit the file named in the error to fix it. To bypass the cache for a single command, use `--refresh`. + ### Environment Variables | Variable | Description | Default | From e9889e59063fc14c1f8bdfc332e7c32d9be9bcc1 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:29:51 +0200 Subject: [PATCH 9/9] docs(request): scope the config-ordering comment to the function request submit bootstraps its service in RunE first, so an unauthenticated user hits the auth prompt before the config error. --- cmd/request_submit.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/cmd/request_submit.go b/cmd/request_submit.go index cee7f86..a1bc453 100644 --- a/cmd/request_submit.go +++ b/cmd/request_submit.go @@ -385,9 +385,14 @@ func rejectGCPWorkspace(ws *submitWorkspace) error { } func resolveSubmitTarget(ctx context.Context, provider, targetName string, refresh bool) (*submitWorkspace, error) { - // Load the config before authenticating. An unusable config — an invalid - // cache_ttl, say — must fail the command, not be quietly replaced by - // defaults, and there is no point authenticating first to find that out. + // Load the config first, ahead of the SCA service bootstrap below. An + // unusable config — an invalid cache_ttl, say — must fail the command + // rather than be quietly replaced by defaults. + // + // This orders the work inside this function only; it is not a fail-fast + // guarantee for the command. `request submit` bootstraps the access-request + // service in its RunE wrapper before resolveSubmitTarget runs, so an + // unauthenticated user hits the auth prompt and never reaches this error. cfg, _, err := config.LoadDefaultWithPath() if err != nil { return nil, err