diff --git a/CHANGELOG.md b/CHANGELOG.md index c814d7a..704ba07 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` (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 @@ -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..dc18bc6 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. 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. 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 | diff --git a/cmd/configure_test.go b/cmd/configure_test.go index aa60438..2695b81 100644 --- a/cmd/configure_test.go +++ b/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") + } +} 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..a1bc453 100644 --- a/cmd/request_submit.go +++ b/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) 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") + } + }) + } +} 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..fb74612 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`. 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 | +| 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**, 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 | +| 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 | --- @@ -273,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) | 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) diff --git a/internal/config/config.go b/internal/config/config.go index 15eafee..2d38e49 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 } @@ -93,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 } @@ -108,16 +116,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: 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; use --refresh to bypass the cache for a single command", 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..882eef3 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,318 @@ 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. 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: "invalid falls back to default", value: "garbage", want: DefaultCacheTTL}, + { + 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 { t.Run(tt.name, func(t *testing.T) { t.Parallel() cfg := &Config{CacheTTL: tt.value} - got := ParseCacheTTL(cfg) + got, err := ParseCacheTTL(cfg) + + if len(tt.wantErrContains) > 0 { + if err == nil { + t.Fatalf("ParseCacheTTL(%q) = %v, want error containing %q", tt.value, got, 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 + } + + 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) + } + + _, 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) + } + 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 +// 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 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() + 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") + } +}