fix(config): reject invalid cache_ttl instead of silently defaulting

CHANGELOG.md

@@ -4,17 +4,21 @@ 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` (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
 
 - `grant favorites add` now fails immediately without a terminal instead of authenticating first
 - `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

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. 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`
-- `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
@@ -168,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
@@ -228,7 +231,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.

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 |

cmd/configure_test.go

@@ -514,3 +514,55 @@ func TestConfigureLongHelpHasNoLegacyPath(t *testing.T) {
 		t.Errorf("configure Long help still mentions legacy path .idsec_profiles:\n%s", long)
 	}
 }
+
+// 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_OverwritesInvalidCacheTTLAndClobbersFavorites(t *testing.T) {
+	dir := t.TempDir()
+	cfgPath := filepath.Join(dir, "config.yaml")
+	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)
+	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)
+	}
+
+	// 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")
+	}
+}

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)
 	})

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)
 }

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)
 	})

cmd/request_submit.go

@@ -385,16 +385,28 @@ func rejectGCPWorkspace(ws *submitWorkspace) error {
 }
 
 func resolveSubmitTarget(ctx context.Context, provider, targetName string, refresh bool) (*submitWorkspace, error) {
+	// 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
+	}
+
 	_, 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
 	}
-	cachedLister := buildCachedLister(cfg, refresh, scaSvc, nil)
 
 	fetchCtx, fetchCancel := context.WithTimeout(ctx, apiTimeout)
 	defer fetchCancel()
@@ -523,6 +535,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) {
@@ -531,22 +561,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 := config.ParseCacheTTL(cfg)
-		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)

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")
+			}
+		})
+	}
+}

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)
 	})