diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e2f1963..5d5aebb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,8 +3,10 @@ name: CI on: push: branches: [main] + # No branch filter: a pull_request trigger scoped to [main] silently skips CI + # on stacked PRs (feature branch -> feature branch), which is exactly when a + # green signal matters most. Run on every PR regardless of its base. pull_request: - branches: [main] permissions: contents: read @@ -42,6 +44,24 @@ jobs: if: runner.os == 'Windows' run: go test -race ./... -v + # Integration tests drive the compiled binary as a child process. They + # need no network and take ~2s, and they are the only place the real + # argument parsing, exit codes and error text are exercised end to end. + # Unguarded so both platforms are covered; the harness builds its own + # binary into a temp dir. + # + # -shuffle=on because the 23 untagged cmd/*_test.go files compile into + # this binary too: without it their order dependence is only ever + # shuffled in the untagged build, never here. + - name: Integration tests + run: go test -tags=integration ./cmd -count=1 -shuffle=on -v + + # The suite mutates package globals (bootstrapImpl, log, + # ui.IsTerminalFunc). Randomising order is what surfaces a test that only + # passes because another test ran first. + - name: Test with shuffled order + run: go test -shuffle=on -count=1 ./... + # Runs on BOTH legs so the platforms stay comparable. This is the only # test that replaces a real, running executable, which is where Windows # and POSIX genuinely differ. It needs no network: the fixture binaries diff --git a/.golangci.yml b/.golangci.yml index 0dd4ee9..edde26b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -5,6 +5,13 @@ run: timeout: 3m tests: true + # Without these, cmd/integration_test.go and internal/selfupdate/e2e_test.go + # are invisible to every linter. Note the effect on cmd/: with `integration` + # set, cmd/main_test.go (//go:build !integration) is excluded instead, so the + # two tagged files are linted at the cost of that one. + build-tags: + - integration + - selfupdate_e2e linters: disable-all: true diff --git a/CHANGELOG.md b/CHANGELOG.md index e6d6ee1..f5229a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### 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 + ## [0.9.0] - 2026-08-14 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 3487905..a0da585 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,7 +85,7 @@ Custom `SCAAccessService` follows SDK conventions: - `httptest.NewServer` for service mocks - `httpClient` interface for DI - Test files co-located as `_test.go` -- Tests that swap a package-level var (e.g. `ui.IsTerminalFunc`, `recordSessionTimestamp`, `getAuth`) MUST NOT call `t.Parallel()` — `-race` flags concurrent access to the global. Mark them with a `// Not parallel: mutates the package-global X.` comment. This is why the `cmd` package tests are all serial. +- Tests that swap a package-level var (e.g. `ui.IsTerminalFunc`, `recordSessionTimestamp`, `bootstrapImpl`) MUST NOT call `t.Parallel()` — `-race` flags concurrent access to the global. Mark them with a `// Not parallel: mutates the package-global X.` comment. This is why the `cmd` package tests are all serial. ## CLI - `spf13/cobra` for CLI framework @@ -217,6 +217,8 @@ make clean # Clean build artifacts - Windows runners have no GNU make, so that leg runs the equivalent Go commands directly (`go build -trimpath -o grant.exe .`, `go test -race ./... -v`); Linux keeps `make build` / `make test-race`. Keep the two legs in sync when Makefile targets change - `go test -race` works on windows/amd64 because the runner image ships gcc (the race detector needs cgo) - The `Self-update end-to-end` step runs the `selfupdate_e2e`-tagged tests on **both** legs (no `if:` guard) — it is the only test that replaces a real running executable, and comparing the two platforms is the whole point. It builds its fixtures locally, so it needs no network. Keep it unguarded; guarding it to Linux would defeat its purpose +- `Integration tests` (`go test -tags=integration ./cmd -shuffle=on`) and `Test with shuffled order` (`go test -shuffle=on -count=1 ./...`) run **unguarded on both legs**. Integration needs no network and takes ~2s; shuffling is what catches order dependence in a suite that mutates package globals. The integration step is shuffled too because the untagged `cmd/*_test.go` files compile into that binary as well — without it their order dependence is only ever shuffled in the untagged build +- `.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 @@ -274,35 +276,42 @@ func init() { ### Dependency Injection -Commands use interfaces for testability: +Commands declare their collaborators as interfaces in `cmd/interfaces.go` (`authLoader`, `eligibilityLister`, `groupsEligibilityLister`, `elevateService`, `accessRequestService`, `selfUpdater`, …). There are two ways to substitute them. -```go -// interfaces.go -type authProvider interface { - Authenticate(profile *models.IdsecProfile) (*models.IdsecToken, error) -} +**Preferred — `New*WithDeps` constructors.** Every command has one: `NewRootCommandWithDeps`, `NewEnvCommandWithDeps`, `NewStatusCommandWithDeps`, `NewRevokeCommandWithDeps`, `NewListCommandWithDeps`, `NewRequestCommandWithDeps`, `NewLogoutCommandWithDeps`, `NewUpdateCommandWithDeps`, plus `runFavoritesAddWithDeps`. The production factory is a thin wrapper that bootstraps the real services and calls the same function. Tests build the command with mocks and never touch a global. -type scaService interface { - ListEligibility(ctx context.Context, csp models.CSP) (*models.EligibilityResponse, error) - Elevate(ctx context.Context, req *models.ElevateRequest) (*models.ElevateResponse, error) -} +**Package-var seams**, for the few things a constructor cannot reach: -// Command runtime resolution -var ( - getAuth = func() (authProvider, error) { /* ... */ } - getSCAService = func() (scaService, error) { /* ... */ } -) +| Var | File | Purpose | +|---|---|---| +| `bootstrapImpl` | `cmd/root.go` | Profile load + authenticate. Memoized by `bootstrapISPAuth` via `sync.Once`; clear it with `resetBootstrapCache()` | +| `recordSessionTimestamp` | `cmd/session_tracking.go` | Elevation timestamp writer | +| `resolveRequestIDFn` | `cmd/request_picker.go` | Interactive request picker | +| `submitPromptFn`, `confirmSubmitFn`, `resolveSubmitTargetFn`, `submitWorkspaceSelectorFn`, `resolveRoleFn` | `cmd/request_submit.go` | `request submit` prompt and resolution steps | +| `log` | `cmd/verbose.go` | Verbose logger; tests swap in `spyLogger` | +| `ui.IsTerminalFunc` | `internal/ui/tty.go` | TTY detection | -// Test injection via package vars -func TestMyCommand(t *testing.T) { - originalGetAuth := getAuth - defer func() { getAuth = originalGetAuth }() +There is no `getAuth`/`getSCAService`; those never existed. Every test that swaps one of these globals restores it via `t.Cleanup`, and no such test may call `t.Parallel()`. - getAuth = func() (authProvider, error) { - return &mockAuth{}, nil - } -} -``` +### Test Isolation + +`internal/testenv` is a normal package imported only from `_test.go` files, so it never links into the binary. `testenv.Run(m.Run)` redirects eight variables under one temp root before `m.Run`, then restores them: `HOME`, `USERPROFILE`, `XDG_CONFIG_HOME`, `IDSEC_PROFILES_FOLDER`, `GRANT_CONFIG`, `IDSEC_KEYRING_FOLDER`, `IDSEC_FILE_LOG_PATH` and `IDSEC_BASIC_KEYRING`. + +- `USERPROFILE` is not optional: Go's Windows `os.UserHomeDir` reads `USERPROFILE`, then `HOMEDRIVE`+`HOMEPATH`, and never `HOME`. `HOME` alone leaves the Windows CI leg pointed at the real profile. The SDK profile loader, conversely, reads `HOME` on every platform. +- `GRANT_CONFIG` does **not** cover the cache: `cache.CacheDir()` → `config.ConfigDir()` → `os.UserHomeDir()`. Before this existed the suite wrote the developer's real `~/.grant/cache/session_timestamps.json` on every run. +- `IDSEC_KEYRING_FOLDER` and `IDSEC_FILE_LOG_PATH` are **absolute-path overrides that bypass the `HOME` fallback entirely** (`pkg/common/keyring/idsec_basic_keyring.go`, `pkg/common/idsec_logger.go`, which `MkdirAll`s the log's parent). A value already exported in the developer's or CI environment sends those writes outside the sandbox no matter what `HOME` says. +- `IDSEC_BASIC_KEYRING=1` is set because the OS keyring is a **daemon, not a path**, so no redirect can sandbox it: on a non-WSL Linux box with `DBUS_SESSION_BUS_ADDRESS` set, `GetKeyring` picks the real libsecret store. Forcing the file backend puts keyring state into the sandboxed folder instead. If a future SDK stops honoring the variable, this containment is gone and nothing here detects it. +- `XDG_CONFIG_HOME` is **speculative/defensive** — nothing in grant or the pinned SDK reads it (`rg XDG_` finds only testenv's own comment and tests). It is redirected because it is the conventional escape hatch and costs nothing. +- `IDSEC_PROFILE` and `DEPLOY_ENV` are **unset**, not redirected (`unsetVars`) — they select behavior, not a location, so the only safe state is absent. `IDSEC_PROFILE` picks the SDK's default profile *name*; `DEPLOY_ENV` feeds tenant-env resolution inside `isp.FromISPAuth`, which the sca/workflows retry-policy tests drive for real. `Run` captures set-vs-unset per variable and restores that exact state. +- `testenv` must not import `testing`; `AssertSandboxed` therefore takes a `TB` interface (`Helper`/`Errorf`) that `*testing.T` satisfies. +- `AssertSandboxed` checks the *configured destinations* — `config.ConfigDir`, `config.ConfigPath`, `cache.CacheDir`, `profiles.GetProfilesFolder`, the SDK keyring folder and the SDK file-log path — plus that `IDSEC_BASIC_KEYRING` is non-empty. The last two resolvers are **reimplemented** in testenv rather than called, because the SDK constructor creates the directory as a side effect and an assertion must not write. It does not prove nothing was written outside the sandbox, and cannot see reads. A snapshot-diff gate was considered and rejected: a concurrently running real `grant` false-positives with certainty, and size+mtime misses same-size rewrites. +- Each `AssertSandboxed` block must be individually killable, which means asserting the failure **count and which resolver failed**, not `len(errs) > 0`: any other assertion satisfies a bare "at least one", so the block under test could be deleted outright. `GRANT_CONFIG` pointed outside the sandbox isolates `config.ConfigPath` (1 failure); `HOME`+`USERPROFILE` pointed outside isolates `config.ConfigDir` **and** `cache.CacheDir` (2 failures, because `CacheDir` delegates to `ConfigDir` → `os.UserHomeDir`). `recordingTB` therefore stores the *formatted* message — the resolver name is an argument, not part of the format string. +- The redirect list is validated against an **explicit literal** in `testenv_test.go`, and every entry additionally gets a hostile pre-existing value before `Run` in `TestRun_OverridesPreExistingHostileValues`. Ranging over `redirectedVars` itself is the trap this replaced: dropping an entry merely checked one fewer thing, and four of the original five survived a drop-one mutation because with `HOME` redirected their *fallbacks* already landed in-sandbox. A var's whole value is defending against a pre-existing value, so that is what the test must supply. +- `Run` restores the environment and removes the sandbox from a **`defer`**, so a panic inside `m.Run` (a `-race` detection, a stray panic) cannot leak the directory or leave the process redirected. `sandboxRoot` is saved and restored rather than cleared, so a nested `Run` hands the outer root back. +- `TestMain` lives in `cmd/main_test.go` (`//go:build !integration`, because `cmd/integration_test.go` declares its own), `internal/config/main_test.go`, `internal/cache/main_test.go`, `internal/sca/main_test.go`, `internal/workflows/main_test.go` and `internal/sdkclient/main_test.go`. The `config`/`cache` ones are in the **external** test package (`config_test`/`cache_test`) because `testenv` imports those packages — an in-package test file importing it would be an import cycle. `sca`/`workflows`/`sdkclient` can be in-package: `testenv` imports `internal/sca/models`, not `internal/sca`. Those three earn a `TestMain` because their retry-policy tests drive the **real** service constructors. +- `os.Setenv` in `TestMain` runs before `m.Run`, so it does not collide with the `t.Parallel()` sites in `internal/` — unlike `t.Setenv`, which the stdlib forbids in parallel tests. +- `cmd/bootstrap_stub_test.go` (untagged, so both build configurations get it) points `bootstrapImpl` at `errTestBootstrapDisabled`. Assert it with `errors.Is`, never a bare `wantErr: true` — otherwise a stray bootstrap attempt silently satisfies an unrelated case. `recordSessionTimestamp` is deliberately left live: it is what proves the redirect works. +- `executeCommand`/`executeCommandStreams`/`executeWithHint` call `restoreCommandGlobals` to put back **both** globals Cobra binds to persistent flags: `outputFormat` (`--output`) and `verbose` (`--verbose`). Without that a command built outside the root (e.g. `NewEnvCommandWithDeps`) inherits the previous test's values — an order dependence `go test -shuffle=on` exposes. `outputFormat` is the load-bearing half (a no-op restore fails 5 of 8 fixed shuffle seeds); `verbose` is latent only because pflag rewrites it at registration. ### Testing Patterns @@ -344,19 +353,10 @@ func (m *mockAuthProvider) Authenticate(p *models.IdsecProfile) (*models.IdsecTo ``` #### Integration Tests -```go -//go:build integration - -// integration_test.go - tests compiled binary -func TestMain(m *testing.M) { - // Build binary before tests - cmd := exec.Command("go", "build", "-o", "../grant-test", "../.") - cmd.Run() - code := m.Run() - os.Remove("../grant-test") - os.Exit(code) -} -``` + +`cmd/integration_test.go` (`//go:build integration`) drives the compiled binary as a child process. Its `TestMain` runs inside `testenv.Run` and builds into a unique temp directory — never the shared `../grant-test`, which two concurrent runs would fight over. `GOCACHE`/`GOMODCACHE`/`GOPATH`/`GOENV` are resolved **before** the `HOME` redirect and passed to the build, otherwise it starts from an empty module cache and needs the network. `GOENV` is in the list because the child `go build` finds its env file via `os.UserConfigDir` — `$XDG_CONFIG_HOME/go/env` on Linux (redirected to an empty sandbox) but `%AppData%\go\env` on Windows (not redirected at all), so without it the two CI legs read different files and any `go env -w GOPROXY=…`/`GOFLAGS`/`GOPRIVATE` is silently dropped on Linux. + +Assertions are exact exit codes plus exact error text. Keyword soup (`error|Error|failed|not found`) is banned here: a panic satisfies it. `runGrant` fails the test outright if the child output contains a panic, and closes stdin so no prompt can block. ### Error Handling diff --git a/cmd/bootstrap_stub_test.go b/cmd/bootstrap_stub_test.go new file mode 100644 index 0000000..c75f40b --- /dev/null +++ b/cmd/bootstrap_stub_test.go @@ -0,0 +1,32 @@ +package cmd + +import ( + "errors" + + sdkauth "github.com/cyberark/idsec-sdk-golang/pkg/auth" + sdkmodels "github.com/cyberark/idsec-sdk-golang/pkg/models" +) + +// This file carries NO build tag on purpose. Both TestMain implementations — +// the default one in main_test.go and the integration one in +// integration_test.go — install the same stub, so the in-process cmd tests +// behave identically under `go test ./cmd` and `go test -tags=integration +// ./cmd`. The integration tests themselves exercise a separate child process +// and are unaffected. + +// errTestBootstrapDisabled is returned by the stubbed bootstrapImpl. It is a +// named sentinel rather than an anonymous error so tests can assert on it with +// errors.Is: a bare `wantErr: true` would otherwise be satisfied by an +// accidental bootstrap attempt, hiding the fact that a unit test reached for +// real credentials. +var errTestBootstrapDisabled = errors.New("bootstrapImpl is disabled in unit tests; inject via New...WithDeps instead") + +// installBootstrapStub replaces the real profile-load + authenticate path so +// no in-process test can load the developer's SDK profile or unlock the real +// keyring. Call it from TestMain, before m.Run. +func installBootstrapStub() { + bootstrapImpl = func() (sdkauth.IdsecAuth, *sdkmodels.IdsecProfile, error) { + return nil, nil, errTestBootstrapDisabled + } + resetBootstrapCache() +} diff --git a/cmd/favorites.go b/cmd/favorites.go index 8da7205..f53f68c 100644 --- a/cmd/favorites.go +++ b/cmd/favorites.go @@ -145,6 +145,19 @@ func runFavoritesAddProduction(cmd *cobra.Command, args []string) error { } } + // Fail fast without a terminal. This must stay AFTER the duplicate-name + // check above (moving it earlier makes a duplicate report "not + // interactive" instead of "already exists") and BEFORE the bootstrap + // below: everything past this point authenticates and calls the SCA + // eligibility API, which a scripted `grant favorites add myfav` used to do + // before ui.ErrNotInteractive finally surfaced from PromptName. + // + // The message is favorites-specific on purpose; earlyNonInteractiveCheck + // in request_picker.go would wrongly tell the user to supply a request ID. + if !ui.IsInteractive() { + return fmt.Errorf("%w; pass a favorite name plus --target and --role, or a name plus --type groups with --group", ui.ErrNotInteractive) + } + // Bootstrap auth and SCA service _, scaService, _, err := bootstrapSCAService() if err != nil { diff --git a/cmd/favorites_test.go b/cmd/favorites_test.go index 6028f84..5efac96 100644 --- a/cmd/favorites_test.go +++ b/cmd/favorites_test.go @@ -316,7 +316,10 @@ func TestFavoritesAddCommand(t *testing.T) { _ = config.Save(cfg, path) }, args: []string{"dev"}, - wantErr: true, // Should fail with duplicate error + wantErr: true, + // A bare wantErr passes on ANY error — an auth failure or the + // non-interactive guard would both satisfy it. Name the message. + wantContain: []string{`favorite "dev" already exists`}, }, { name: "success with target and role flags", @@ -1252,3 +1255,100 @@ func TestSurveyNamePrompter_NonTTY(t *testing.T) { t.Errorf("error should suggest providing name as argument, got: %v", err) } } + +// setTTY forces ui.IsInteractive() to the given value for the duration of the +// test. +// +// Not parallel: mutates the package-global ui.IsTerminalFunc. +func setTTY(t *testing.T, interactive bool) { + t.Helper() + original := ui.IsTerminalFunc + t.Cleanup(func() { ui.IsTerminalFunc = original }) + ui.IsTerminalFunc = func(fd uintptr) bool { return interactive } +} + +// TestFavoritesAdd_NonInteractiveGuard pins the BUG that `grant favorites add +// myfav` in a script used to authenticate and call the SCA eligibility API +// before ui.ErrNotInteractive finally surfaced from PromptName. The guard must +// fire before bootstrapSCAService, and must sit AFTER the duplicate-name check +// so a duplicate still reports "already exists". +// +// Not parallel: mutates package-global TTY and bootstrap state. +func TestFavoritesAdd_NonInteractiveGuard(t *testing.T) { + tests := []struct { + name string + // interactive drives ui.IsInteractive(). + interactive bool + setupConfig func(string) + args []string + wantNotInteract bool // error must wrap ui.ErrNotInteractive + wantContain []string // substrings required in the error text + }{ + { + name: "name given but no flags and no terminal", + setupConfig: func(path string) { _ = config.Save(config.DefaultConfig(), path) }, + args: []string{"myfav"}, + // The hint must name the favorites flags, not a request ID — and + // the NAME argument, which is required too and which a + // flags-only hint silently omits. + wantNotInteract: true, + wantContain: []string{"name", "--target", "--role"}, + }, + { + name: "groups type without --group and no terminal", + setupConfig: func(path string) { _ = config.Save(config.DefaultConfig(), path) }, + args: []string{"myfav", "--type", "groups"}, + wantNotInteract: true, + wantContain: []string{"name", "--group"}, + }, + { + name: "duplicate name still reports the duplicate, not the missing terminal", + setupConfig: func(path string) { + cfg := config.DefaultConfig() + _ = config.AddFavorite(cfg, "dev", config.Favorite{ + Provider: "azure", Target: "subscription-123", Role: "Contributor", + }) + _ = config.Save(cfg, path) + }, + args: []string{"dev"}, + wantContain: []string{`favorite "dev" already exists`}, + }, + { + name: "with a terminal the guard does not fire and bootstrap is reached", + interactive: true, + setupConfig: func(path string) { _ = config.Save(config.DefaultConfig(), path) }, + args: []string{"myfav"}, + // errTestBootstrapDisabled proves execution got past the guard. + wantContain: []string{"bootstrapImpl is disabled in unit tests"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setTTY(t, tt.interactive) + resetBootstrapCache() + t.Cleanup(resetBootstrapCache) + + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + tt.setupConfig(configPath) + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommand()) + + _, err := executeCommand(rootCmd, append([]string{"favorites", "add"}, tt.args...)...) + if err == nil { + t.Fatal("expected an error, got nil") + } + if gotNotInteract := errors.Is(err, ui.ErrNotInteractive); gotNotInteract != tt.wantNotInteract { + t.Errorf("errors.Is(err, ui.ErrNotInteractive) = %v, want %v (err = %v)", + gotNotInteract, tt.wantNotInteract, err) + } + for _, want := range tt.wantContain { + if !strings.Contains(err.Error(), want) { + t.Errorf("error missing %q, got: %v", want, err) + } + } + }) + } +} diff --git a/cmd/integration_test.go b/cmd/integration_test.go index ad96343..11a1ea6 100644 --- a/cmd/integration_test.go +++ b/cmd/integration_test.go @@ -3,32 +3,146 @@ package cmd import ( + "errors" + "fmt" "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" + + "github.com/aaearon/grant-cli/internal/testenv" ) -// TestMain builds the binary before running integration tests +// testBinary is the compiled grant binary under test. It lives in a private +// temp directory with a unique name rather than the old shared ../grant-test, +// so two concurrent runs (or a stale artifact from a killed run) cannot +// interfere with each other or leave debris in the repo. +var testBinary string + +// goEnvPassthrough carries the Go tool's own directories into the sandboxed +// build. They are resolved BEFORE testenv.Run redirects HOME, because GOCACHE +// and GOMODCACHE default to locations under the user's home: without this the +// build inside the sandbox would start from an empty module cache and need the +// network. +// +// GOENV is in the list for the same reason and one more. The child `go build` +// locates its env file through os.UserConfigDir: $XDG_CONFIG_HOME/go/env on +// Linux — which the redirect points at an empty sandbox — but %AppData%\go\env +// on Windows, which is not redirected at all. Without pinning GOENV the two CI +// legs resolve different files, and any `go env -w GOPROXY=…` / `GOFLAGS` / +// `GOPRIVATE` the developer or runner configured is silently dropped on Linux. +var goEnvPassthrough []string + func TestMain(m *testing.M) { - // Build the binary - cmd := exec.Command("go", "build", "-o", "../grant-test", "../.") - if err := cmd.Run(); err != nil { - panic("Failed to build binary for integration tests: " + err.Error()) + goEnvPassthrough = resolveGoEnv("GOCACHE", "GOMODCACHE", "GOPATH", "GOENV") + + os.Exit(testenv.Run(func() int { + dir, err := os.MkdirTemp("", "grant-integration-bin-") + if err != nil { + fmt.Fprintf(os.Stderr, "integration: failed to create binary dir: %v\n", err) + return 1 + } + defer func() { _ = os.RemoveAll(dir) }() + + name := "grant-integration-test" + if runtime.GOOS == "windows" { + name += ".exe" + } + testBinary = filepath.Join(dir, name) + + build := exec.Command("go", "build", "-o", testBinary, "..") + build.Env = append(os.Environ(), goEnvPassthrough...) + if out, err := build.CombinedOutput(); err != nil { + fmt.Fprintf(os.Stderr, "integration: failed to build binary: %v\n%s\n", err, out) + return 1 + } + + // The in-process cmd unit tests compile into this binary too; give + // them the same disabled bootstrap they get in the default build. + installBootstrapStub() + + return m.Run() + })) +} + +// resolveGoEnv returns KEY=VALUE strings for the named `go env` keys. +func resolveGoEnv(keys ...string) []string { + out, err := exec.Command("go", append([]string{"env"}, keys...)...).Output() + if err != nil { + return nil + } + lines := strings.Split(strings.ReplaceAll(string(out), "\r\n", "\n"), "\n") + env := make([]string, 0, len(keys)) + for i, k := range keys { + if i >= len(lines) || lines[i] == "" { + continue + } + env = append(env, k+"="+lines[i]) } + return env +} - // Run tests - code := m.Run() +// result is the outcome of one child invocation. +type result struct { + output string + exitCode int +} + +// contains reports whether the combined output contains want. +func (r result) contains(want string) bool { return strings.Contains(r.output, want) } + +// runGrant executes the built binary with the process's (already sandboxed) +// environment plus extra, with stdin closed so no interactive prompt can +// block. It returns the combined output and the real exit code. +func runGrant(t *testing.T, extraEnv []string, args ...string) result { + t.Helper() + + cmd := exec.Command(testBinary, args...) + cmd.Env = append(os.Environ(), extraEnv...) + cmd.Stdin = nil // closed: never a TTY, and never blocks on a prompt - // Clean up - os.Remove("../grant-test") + out, err := cmd.CombinedOutput() + + code := 0 + if err != nil { + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("running %v: %v\noutput:\n%s", args, err, out) + } + code = exitErr.ExitCode() + } - os.Exit(code) + r := result{output: string(out), exitCode: code} + + // A panic satisfies almost any keyword-based assertion, which is exactly + // how the previous version of these tests could pass on a crash. + if r.contains("panic:") || r.contains("goroutine 1 [running]:") { + t.Fatalf("binary panicked on %v (exit %d):\n%s", args, code, out) + } + return r } -func getBinaryPath() string { - return filepath.Join("..", "grant-test") +// isolatedEnv points config and credentials at a fresh per-test directory on +// top of the process-wide testenv sandbox. +func isolatedEnv(t *testing.T) []string { + t.Helper() + dir := t.TempDir() + return []string{ + "HOME=" + dir, + "USERPROFILE=" + dir, + "GRANT_CONFIG=" + filepath.Join(dir, "config.yaml"), + "IDSEC_PROFILES_FOLDER=" + filepath.Join(dir, "profiles"), + // The SDK reads these two absolute-path overrides directly, bypassing + // the HOME fallback, and forces its file keyring on any non-empty + // IDSEC_BASIC_KEYRING. Without them a pre-existing value in the + // developer's or CI environment sends the subprocess's keyring and log + // writes outside the sandbox. + "IDSEC_KEYRING_FOLDER=" + filepath.Join(dir, "keyring"), + "IDSEC_FILE_LOG_PATH=" + filepath.Join(dir, "logs", "idsec.log"), + "IDSEC_BASIC_KEYRING=1", + } } func TestIntegration_Help(t *testing.T) { @@ -66,16 +180,14 @@ func TestIntegration_Help(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd := exec.Command(getBinaryPath(), tt.args...) - output, err := cmd.CombinedOutput() - if err != nil && !strings.Contains(string(output), "Usage:") { - t.Fatalf("Command failed: %v\nOutput: %s", err, output) + got := runGrant(t, isolatedEnv(t), tt.args...) + // Help is a success path: exit 0, exactly. + if got.exitCode != 0 { + t.Fatalf("exit code = %d, want 0\noutput:\n%s", got.exitCode, got.output) } - - outputStr := string(output) for _, want := range tt.wantText { - if !strings.Contains(outputStr, want) { - t.Errorf("Expected output to contain %q, got:\n%s", want, outputStr) + if !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) } } }) @@ -83,184 +195,128 @@ func TestIntegration_Help(t *testing.T) { } func TestIntegration_Version(t *testing.T) { - cmd := exec.Command(getBinaryPath(), "version") - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("Version command failed: %v\nOutput: %s", err, output) + got := runGrant(t, isolatedEnv(t), "version") + if got.exitCode != 0 { + t.Fatalf("exit code = %d, want 0\noutput:\n%s", got.exitCode, got.output) } - outputStr := string(output) - requiredFields := []string{"grant version", "commit:", "built:"} - - for _, field := range requiredFields { - if !strings.Contains(outputStr, field) { - t.Errorf("Expected output to contain %q, got:\n%s", field, outputStr) + for _, field := range []string{"grant version", "commit:", "built:"} { + if !got.contains(field) { + t.Errorf("output missing %q, got:\n%s", field, got.output) } } - - // Should contain at least one of the default values (dev build) - if !strings.Contains(outputStr, "dev") && !strings.Contains(outputStr, "unknown") { - t.Errorf("Expected version output to show dev build info, got:\n%s", outputStr) + // The integration binary is built without -ldflags, so the version stays + // at its compiled-in default. Assert the version line specifically: the + // binary always prints "commit: unknown" for a non-ldflags build, so an + // `|| contains("unknown")` arm would make this assertion true regardless + // of the version string. + if !got.contains("grant version dev") { + t.Errorf("expected a dev build banner, got:\n%s", got.output) } } func TestIntegration_ElevateWithoutLogin(t *testing.T) { - // Set a temporary config path to avoid interfering with real config - tempDir := t.TempDir() + got := runGrant(t, isolatedEnv(t), "--provider", "azure") - cmd := exec.Command(getBinaryPath(), "--provider", "azure") - cmd.Env = append(os.Environ(), "GRANT_CONFIG="+filepath.Join(tempDir, "config.yaml")) - cmd.Env = append(cmd.Env, "HOME="+tempDir) // Isolate from real credentials - - output, err := cmd.CombinedOutput() - - // Command should fail when not authenticated - if err == nil { - t.Errorf("Expected elevation to fail without authentication, but it succeeded.\nOutput: %s", output) + if got.exitCode != 1 { + t.Fatalf("exit code = %d, want 1\noutput:\n%s", got.exitCode, got.output) } - - outputStr := string(output) - - // Should contain an error message about authentication or configuration - errorKeywords := []string{"error", "Error", "failed", "Failed", "not found", "authenticate"} - foundError := false - for _, keyword := range errorKeywords { - if strings.Contains(outputStr, keyword) { - foundError = true - break + // Exact text, not a keyword soup. With an empty sandbox profile directory + // the SDK authenticator refuses before any network call, and the non- + // verbose hint is part of the contract. + for _, want := range []string{ + "authentication failed: either a profile or a specific auth profile must be supplied", + "Hint: re-run with --verbose for more details", + } { + if !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) } } - - if !foundError { - t.Errorf("Expected error output when not authenticated, got:\n%s", outputStr) - } } func TestIntegration_StatusWithoutLogin(t *testing.T) { - // Set a temporary config path to avoid interfering with real config - tempDir := t.TempDir() - - cmd := exec.Command(getBinaryPath(), "status") - cmd.Env = append(os.Environ(), "GRANT_CONFIG="+filepath.Join(tempDir, "config.yaml")) - cmd.Env = append(cmd.Env, "HOME="+tempDir) // Isolate from real credentials - - output, err := cmd.CombinedOutput() + got := runGrant(t, isolatedEnv(t), "status") - // Status command should run but show not authenticated - outputStr := string(output) - - // Should indicate not authenticated state - if !strings.Contains(outputStr, "Not authenticated") && !strings.Contains(outputStr, "not authenticated") { - // If it doesn't explicitly say "not authenticated", it should at least not show a username - if strings.Contains(outputStr, "Username:") && err == nil { - t.Errorf("Expected status to show 'not authenticated', but it showed a username.\nOutput: %s", outputStr) - } + if got.exitCode != 1 { + t.Fatalf("exit code = %d, want 1\noutput:\n%s", got.exitCode, got.output) + } + if want := "authentication failed: either a profile or a specific auth profile must be supplied"; !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) + } + // It must never claim an identity it does not have. + if got.contains("Username:") { + t.Errorf("status printed a username without authentication:\n%s", got.output) } } func TestIntegration_FavoritesList(t *testing.T) { - // Set a temporary config path to avoid interfering with real config - tempDir := t.TempDir() + got := runGrant(t, isolatedEnv(t), "favorites", "list") - cmd := exec.Command(getBinaryPath(), "favorites", "list") - cmd.Env = append(os.Environ(), "GRANT_CONFIG="+filepath.Join(tempDir, "config.yaml")) - - output, err := cmd.CombinedOutput() - - // Command should succeed (empty list is valid) - if err != nil && !strings.Contains(string(output), "No favorites") { - t.Fatalf("Favorites list command failed unexpectedly: %v\nOutput: %s", err, output) + if got.exitCode != 0 { + t.Fatalf("exit code = %d, want 0 (an empty favorites list is a success)\noutput:\n%s", + got.exitCode, got.output) } - - outputStr := string(output) - - // Should either show "No favorites" or be empty - if !strings.Contains(outputStr, "No favorites") && strings.TrimSpace(outputStr) != "" { - // If it's not empty and doesn't say "No favorites", it should at least be valid output - t.Logf("Favorites list output: %s", outputStr) + if !got.contains("No favorites") { + t.Errorf("output missing %q, got:\n%s", "No favorites", got.output) } } func TestIntegration_FavoritesAddWithFlags(t *testing.T) { - tempDir := t.TempDir() - configPath := filepath.Join(tempDir, "config.yaml") - env := append(os.Environ(), "GRANT_CONFIG="+configPath) - - // Add a favorite via flags - cmd := exec.Command(getBinaryPath(), "favorites", "add", "test-fav", "--target", "sub-123", "--role", "Contributor") - cmd.Env = env - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("favorites add with flags failed: %v\nOutput: %s", err, output) - } + env := isolatedEnv(t) - outputStr := string(output) - if !strings.Contains(outputStr, "Added favorite") { - t.Errorf("expected output to contain 'Added favorite', got:\n%s", outputStr) + added := runGrant(t, env, "favorites", "add", "test-fav", "--target", "sub-123", "--role", "Contributor") + if added.exitCode != 0 { + t.Fatalf("favorites add exit code = %d, want 0\noutput:\n%s", added.exitCode, added.output) } - - // Verify via favorites list - cmd = exec.Command(getBinaryPath(), "favorites", "list") - cmd.Env = env - output, err = cmd.CombinedOutput() - if err != nil { - t.Fatalf("favorites list failed: %v\nOutput: %s", err, output) + if !added.contains("Added favorite") { + t.Errorf("output missing %q, got:\n%s", "Added favorite", added.output) } - outputStr = string(output) + listed := runGrant(t, env, "favorites", "list") + if listed.exitCode != 0 { + t.Fatalf("favorites list exit code = %d, want 0\noutput:\n%s", listed.exitCode, listed.output) + } for _, want := range []string{"test-fav", "azure/sub-123/Contributor"} { - if !strings.Contains(outputStr, want) { - t.Errorf("favorites list missing %q, got:\n%s", want, outputStr) + if !listed.contains(want) { + t.Errorf("favorites list missing %q, got:\n%s", want, listed.output) } } } -func TestIntegration_FavoritesAddInteractiveRequiresAuth(t *testing.T) { - tempDir := t.TempDir() - - cmd := exec.Command(getBinaryPath(), "favorites", "add", "test-fav") - cmd.Env = append(os.Environ(), "GRANT_CONFIG="+filepath.Join(tempDir, "config.yaml")) - cmd.Env = append(cmd.Env, "HOME="+tempDir) // Isolate from real credentials - - output, err := cmd.CombinedOutput() +// TestIntegration_FavoritesAddWithoutTTYFailsBeforeAuth is the end-to-end +// counterpart of TestFavoritesAdd_NonInteractiveGuard: with stdin closed the +// command must refuse immediately with the non-interactive hint, and must NOT +// reach the profile load / authentication it used to attempt first. +func TestIntegration_FavoritesAddWithoutTTYFailsBeforeAuth(t *testing.T) { + got := runGrant(t, isolatedEnv(t), "favorites", "add", "test-fav") - // Should fail — interactive mode needs auth - if err == nil { - t.Errorf("Expected favorites add interactive to fail without auth, but it succeeded.\nOutput: %s", output) + if got.exitCode != 1 { + t.Fatalf("exit code = %d, want 1\noutput:\n%s", got.exitCode, got.output) } - - outputStr := string(output) - - // Should contain an auth-related error (profile not found, auth failed, etc.) - errorKeywords := []string{"error", "Error", "failed", "Failed", "not found", "profile"} - foundError := false - for _, keyword := range errorKeywords { - if strings.Contains(outputStr, keyword) { - foundError = true - break + for _, want := range []string{"interactive selection requires a terminal", "--target", "--role"} { + if !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) } } - - if !foundError { - t.Errorf("Expected auth-related error for interactive favorites add, got:\n%s", outputStr) + if got.contains("failed to load profile") || got.contains("authentication failed") { + t.Errorf("favorites add reached authentication before the non-interactive guard:\n%s", got.output) } } func TestIntegration_InvalidCommand(t *testing.T) { - cmd := exec.Command(getBinaryPath(), "nonexistent-command") - output, err := cmd.CombinedOutput() + got := runGrant(t, isolatedEnv(t), "nonexistent-command") - // Should fail for invalid command - if err == nil { - t.Errorf("Expected invalid command to fail, but it succeeded.\nOutput: %s", output) + if got.exitCode != 1 { + t.Fatalf("exit code = %d, want 1\noutput:\n%s", got.exitCode, got.output) } - - outputStr := string(output) - - // Should show an error or help message - if !strings.Contains(outputStr, "unknown command") && - !strings.Contains(outputStr, "Error:") && - !strings.Contains(outputStr, "Usage:") { - t.Errorf("Expected error for invalid command, got:\n%s", outputStr) + if want := `unknown command "nonexistent-command" for "grant"`; !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) } } + +// TestIntegration_SandboxIsolation asserts the harness itself is sandboxed, so +// a regression in TestMain surfaces here rather than as writes to the +// developer's real home directory. +func TestIntegration_SandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} diff --git a/cmd/main_test.go b/cmd/main_test.go new file mode 100644 index 0000000..7f0192c --- /dev/null +++ b/cmd/main_test.go @@ -0,0 +1,78 @@ +//go:build !integration + +// The build tag is load-bearing: cmd/integration_test.go declares its own +// TestMain under `//go:build integration`, and two TestMain symbols in one +// package will not compile. Both harnesses call testenv.Run. +package cmd + +import ( + "errors" + "os" + "strings" + "testing" + + "github.com/aaearon/grant-cli/internal/cache" + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain does two things, in this order: +// +// 1. Redirects HOME/USERPROFILE/XDG_CONFIG_HOME/IDSEC_PROFILES_FOLDER/ +// GRANT_CONFIG at a throwaway directory. Before this existed the suite +// wrote the developer's real ~/.grant/cache/session_timestamps.json on +// every run, because cache.CacheDir resolves through os.UserHomeDir and +// GRANT_CONFIG does not affect it. +// 2. Replaces bootstrapImpl, so no unit test can load the real SDK profile +// or unlock the real keyring. +// +// recordSessionTimestamp is deliberately NOT stubbed: leaving the real writer +// live is what proves the HOME redirect actually works. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(func() int { + installBootstrapStub() + return m.Run() + })) +} + +// TestSandboxIsolation pins the redirect. If TestMain ever stops wrapping +// m.Run, this fails instead of the suite silently writing to the real home. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} + +// TestCacheDirResolvesInsideSandbox is the specific regression for the 25 +// writes to the developer's real ~/.grant/cache/session_timestamps.json. The +// chain is recordSessionTimestamp -> cache.CacheDir -> config.ConfigDir -> +// os.UserHomeDir, which GRANT_CONFIG does not influence at all. +// +// Not parallel: reads process-wide environment state. +func TestCacheDirResolvesInsideSandbox(t *testing.T) { + dir, err := cache.CacheDir() + if err != nil { + t.Fatalf("cache.CacheDir(): %v", err) + } + root := testenv.Root() + if root == "" { + t.Fatal("no testenv sandbox is active; TestMain is not wrapping m.Run") + } + if !strings.HasPrefix(dir, root) { + t.Errorf("cache.CacheDir() = %q, want a path under the sandbox root %q", dir, root) + } +} + +// TestBootstrapSCAServiceIsDisabledInUnitTests asserts the sentinel +// specifically. Asserting merely "some error" would let a cached bootstrap +// failure satisfy unrelated wantErr cases in other tables. +// +// Not parallel: mutates the package-global bootstrap memoization state. +func TestBootstrapSCAServiceIsDisabledInUnitTests(t *testing.T) { + resetBootstrapCache() + t.Cleanup(resetBootstrapCache) + + _, _, _, err := bootstrapSCAService() + if !errors.Is(err, errTestBootstrapDisabled) { + t.Fatalf("bootstrapSCAService() error = %v, want errTestBootstrapDisabled", err) + } +} diff --git a/cmd/test_helpers.go b/cmd/test_helpers.go index 4f47512..45d4f47 100644 --- a/cmd/test_helpers.go +++ b/cmd/test_helpers.go @@ -25,6 +25,8 @@ func newNoOpCommand() *cobra.Command { // When SilenceErrors is true, error text is appended to the output buffer // to match production behavior (where Execute() prints the error). func executeCommand(cmd *cobra.Command, args ...string) (string, error) { + defer restoreCommandGlobals(outputFormat, verbose) + buf := new(bytes.Buffer) cmd.SetOut(buf) cmd.SetErr(buf) @@ -37,12 +39,30 @@ func executeCommand(cmd *cobra.Command, args ...string) (string, error) { return buf.String(), err } +// restoreCommandGlobals puts back the two package-globals that newRootCommand +// binds to persistent flags: outputFormat (--output, StringVarP) and verbose +// (--verbose, BoolVarP). Executing any command carrying those flags leaves both +// globals set for every later test, and a command built without the root flag +// set (NewEnvCommandWithDeps, for instance) then inherits the previous test's +// values — an order dependence that only shows up under `go test -shuffle=on`. +// +// outputFormat is the load-bearing half: making this a no-op fails on 5 of 8 +// fixed shuffle seeds. verbose is latent today, because pflag rewrites it to +// the registered default whenever newRootCommand runs, but it is the same leak +// through the same mechanism and is restored for the same reason. +func restoreCommandGlobals(savedOutput string, savedVerbose bool) { + outputFormat = savedOutput + verbose = savedVerbose +} + // executeCommandStreams executes a command keeping stdout and stderr apart and // writing no error text into either. Use it when a test needs to assert on the // exact stdout payload (e.g. valid JSON) *and* on a returned error, which // executeCommand cannot express because it merges the streams and appends the // error text. func executeCommandStreams(cmd *cobra.Command, args ...string) (stdout, stderr string, err error) { + defer restoreCommandGlobals(outputFormat, verbose) + var outBuf, errBuf bytes.Buffer cmd.SetOut(&outBuf) cmd.SetErr(&errBuf) @@ -55,6 +75,8 @@ func executeCommandStreams(cmd *cobra.Command, args ...string) (stdout, stderr s // executeWithHint simulates Execute() logic without os.Exit, returning the error output. // Used for testing the verbose hint behavior. func executeWithHint(cmd *cobra.Command, args []string) string { + defer restoreCommandGlobals(outputFormat, verbose) + passedArgValidation = false cmd.SetArgs(args) err := cmd.Execute() diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md new file mode 100644 index 0000000..fd3e343 --- /dev/null +++ b/docs/mutation-ledger.md @@ -0,0 +1,321 @@ +# Mutation ledger + +This is the definition of done for the test-remediation effort. Every row is one +mutation that a five-part adversarial mutation audit applied to production code +and that the test suite failed to notice — plus the rows the verifiers **refuted**, +which are recorded here precisely so nobody re-opens a settled question. + +**How a row is closed.** + +1. Apply the mutation in the **Mutation** cell verbatim at the **File:line** shown. +2. Run the test named in **Planned test** and watch it **fail**. +3. Revert the mutation. +4. Run the same test and watch it **pass**. +5. Always `-count=1`. A cached success is not evidence. + +Then flip **Status** to `done`. **A PR is not complete until every one of its rows +is `done`.** `wont-fix` and `refuted` rows are closed by review, not by a test; +mark them `done` when the PR that owns them has landed the comment / rationale. + +**Line numbers are relative to this branch's tree**, not to `main`. They were first +re-verified against `main` post-`f14e2b9`; the nine `cmd/favorites.go` rows were then +shifted by +13 for the non-interactive guard this branch inserts at +`cmd/favorites.go:148`. Several also drifted from the source reports; those corrections +are noted in the Mutation cell with `(was ...)`. **File:line is always the production +site, never the test site.** + +**Verdicts** are the verifiers' conclusions, not the original reports': +`CONFIRMED` (survivor reproduced), `OVERSTATED` (survivor real, stated consequence +weaker than claimed), `REFUTED` (the claim is false — the mutant dies, or the +premise does not hold). + +--- + +## Ledger + +| ID | Area | File:line | Mutation (exact, applyable) | Verdict | Disposition | Planned test | PR | Status | +|---|---|---|---|---|---|---|---|---| +| REQ-01 | cmd/request finalize | `cmd/request_finalize.go:100` | `svc.FinalizeRequest(ctx, requestID, decision, reason)` → `svc.FinalizeRequest(ctx, requestID, "APPROVED", reason)` | CONFIRMED | test | `TestRequestReject_SendsRejectedDecision` | PR4 | todo | +| REQ-02 | cmd/request cancel | `cmd/request_cancel.go:60` | `svc.CancelRequest(ctx, requestID, reason)` → `svc.CancelRequest(ctx, "WRONG-ID", reason)` | CONFIRMED | test | `TestRequestCancel_PassesRequestID` | PR4 | todo | +| REQ-03 | cmd/request get | `cmd/request_get.go:53` | `svc.GetRequest(ctx, requestID)` → `svc.GetRequest(ctx, "WRONG-ID")` | CONFIRMED | test | `TestRequestGet_PassesRequestID` | PR4 | todo | +| REQ-04 | cmd/request list | `cmd/request_list.go:93-98` | Swap the asc/desc branches: `order := "asc"` → `order := "desc"` and `order = "desc"` → `order = "asc"` | CONFIRMED | test | `TestRequestList_SortDirection` | PR4 | todo | +| REQ-05 | cmd/request list | `cmd/request_list.go:82` | `if role != "CREATOR" && role != "APPROVER" {` → `if false {` | CONFIRMED | test | `TestRequestList_RejectsInvalidRole` | PR4 | todo | +| REQ-06 | cmd/request list | `cmd/request_list.go:78` | `params.FreeText = v` → `params.FreeText = ""` | CONFIRMED | test | `TestRequestList_PassesFreeText` | PR4 | todo | +| REQ-07 | cmd/request list | `cmd/request_list.go:58` | Delete `filters = append(filters, fmt.Sprintf("(requestState eq %s)", upper))` | CONFIRMED | test | `TestRequestList_PassesStateFilter` | PR4 | todo | +| REQ-08 | cmd/request submit | `cmd/request_submit.go:306` | `TargetCategory: "CLOUD_CONSOLE"` → `TargetCategory: "WRONG"` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | todo | +| REQ-09 | cmd/request submit | `cmd/request_submit.go:507` | `"workspaceId": ws.WorkspaceID,` → `"workspaceId": "",` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | todo | +| REQ-10 | cmd/request submit | `cmd/request_submit.go:511-512` | Swap the two values: `"timeFrom": f.timeTo,` / `"timeTo": f.timeFrom,` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | todo | +| REQ-11 | cmd/request submit | `cmd/request_submit.go:274` | Delete the `if err := validateSubmitFields(fields); err != nil { return err }` call site | CONFIRMED | test | `TestRunRequestSubmit_InvokesValidation` | PR4 | todo | +| REQ-12 | cmd/request submit | `cmd/request_submit.go:631` | `return errors.New("--date is required")` → `return errors.New("WRONG ERROR MESSAGE")` | CONFIRMED | test | `TestValidateSubmitFields_ErrorMessages` | PR4 | todo | +| REQ-13 | cmd/request get | `cmd/request_picker.go:15` (reached from `cmd/request_get.go`) | In the `get` path, disable the guard: `if requestID == "" && !ui.IsInteractive() {` → `if false {`, keeping `_ = requestID` so it compiles | CONFIRMED | test | `TestRequestGet_NonInteractiveRequiresID` | PR4 | todo | +| REQ-14 | cmd/request approve | `cmd/request_finalize.go:16` | Disable the early non-interactive guard for `approve` (`if false {`), preserving `_ = requestID` — a literal deletion does not compile (`declared and not used: requestID`) | CONFIRMED | test | `TestRequestApprove_NonInteractiveRequiresID` | PR4 | todo | +| REQ-15 | cmd/request reject | `cmd/request_finalize.go:16` | Same as REQ-14 for the `reject` path, with `_ = requestID` | CONFIRMED | test | `TestRequestReject_NonInteractiveRequiresID` | PR4 | todo | +| REQ-16 | cmd/request submit | `cmd/request_submit.go:377` | `if ws.CSP == models.CSPGCP {` → `if false {` inside `rejectGCPWorkspace`. The fixture sets both `WorkspaceType: WorkspaceTypeProject` and `CSP: CSPGCP`, so the workspace-type switch masks loss of the CSP arm | CONFIRMED | test | `TestRejectGCPWorkspace_CSPTagOnly` | PR4 | todo | +| REQ-17 | cmd/request output (text) | `cmd/request.go:79-80` | Swap the table values: `r.DetailString("workspaceName")` / `r.DetailString("roleName")` | CONFIRMED | test | `TestRequestList_TextFieldMapping` | PR5 | todo | +| REQ-18 | cmd/request output (text) | `cmd/request.go:125` | `fmt.Fprintf(w, "Created By: %s\n", r.CreatedBy)` → source from `r.UpdatedBy` | CONFIRMED | test | `TestRequestGet_TextFieldMapping` | PR5 | todo | +| REQ-19 | cmd/request output (JSON) | `cmd/request.go:190-191` | Swap `TimeFrom: r.DetailString("timeFrom")` and `TimeTo: r.DetailString("timeTo")` | CONFIRMED | test | `TestRequestGetJSON_FieldMapping` (`assertJSONEqual`) | PR5 | todo | +| REQ-20 | cmd/login | `cmd/login.go:52` | `if profile == nil {` → `if false {` (auto-configure branch). Feature *is* implemented; `login_test.go` skips it with the factually wrong reason "Auto-configure not yet implemented" — delete the skip | CONFIRMED | test | `TestRunLogin_AutoConfiguresMissingProfile` | PR4 | todo | +| REQ-21 | cmd/login | `cmd/login.go:74` | `auth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true)` → `..., true, false)` (swap `force`/`refreshAuth`) | CONFIRMED | test | `TestRunLogin_AuthenticateFlags` | PR4 | todo | +| REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | todo | +| REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | done | +| OUT-01 | cmd/list flags | `cmd/list.go:73` | Delete `cmd.MarkFlagsMutuallyExclusive("groups", "provider")`. `TestListCommand_MutualExclusivity` passes today on the *unrelated* runtime error `no eligible targets or groups found` | CONFIRMED | test | `TestListCommand_MutualExclusivity` (assert Cobra's `[groups provider] were all set`) | PR5 | todo | +| OUT-02 | cmd/favorites (interactive) | `cmd/favorites.go:258` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `selectFavoriteInteractive` | CONFIRMED | test | `TestFavoritesAddInteractive_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | +| OUT-03 | cmd/favorites (group add) | `cmd/favorites.go:388` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `addGroupFavorite` | CONFIRMED | test | `TestAddGroupFavorite_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | +| OUT-04 | cmd/list | `cmd/list.go:135` | `if provider == "" {` → `if true {` (groups fetched and emitted even when `--provider` is set) | CONFIRMED | test | `TestListCommand_ProviderSuppressesGroups` | PR5 | todo | +| OUT-05 | cmd/status JSON | `cmd/status.go:212` | `Provider: strings.ToLower(string(s.CSP))` → `strings.ToUpper(string(s.CSP))` | CONFIRMED | test | `TestStatusJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| OUT-06 | cmd/status JSON | `cmd/status.go:213` | `WorkspaceID: s.WorkspaceID` → `WorkspaceID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | +| OUT-07 | cmd/status JSON | `cmd/status.go:214` | `Duration: s.SessionDuration` → `Duration: 0` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | +| OUT-08 | cmd/status JSON | `cmd/status.go:215` | `RoleID: s.RoleID` → `RoleID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | +| OUT-09 | cmd/status JSON | `cmd/status.go:217-219` | Delete the `if name, ok := data.nameMap[s.WorkspaceID]; ok { so.WorkspaceName = name }` block | CONFIRMED | test | `TestStatusJSON_ResolvesWorkspaceName` | PR5 | todo | +| OUT-10 | cmd/status JSON | `cmd/status.go:221` | `so.Type = "group"` → `so.Type = "cloud"` | CONFIRMED | test | `TestStatusJSON_GroupSessionType` | PR5 | todo | +| OUT-11 | cmd/list JSON | `cmd/list.go:164` | `WorkspaceID: t.WorkspaceID` → `WorkspaceID: t.OrganizationID` | CONFIRMED | test | `TestListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| OUT-12 | cmd/list JSON | `cmd/list.go:165` | `WorkspaceType: strings.ToLower(string(t.WorkspaceType))` → `WorkspaceType: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | +| OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_RoundTripsToRequestSubmit` | PR5 | todo | +| OUT-14 | cmd/list JSON | `cmd/list.go:175` | `GroupID: g.GroupID` → `GroupID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | +| OUT-15 | cmd/list JSON | `cmd/list.go:176` | `DirectoryID: g.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | +| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:462` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:464` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | +| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:466` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | +| OUT-19 | cmd/favorites | `cmd/favorites.go:333` | `fav.Provider = cfg.DefaultProvider` → `fav.Provider = "azure"`. Every command test uses the azure default, so a non-default `DefaultProvider` (aws/gcp) is unpinned. Secondary, same defect class: `internal/config/favorites.go:21-22` independently defaults empty → `"azure"` | CONFIRMED | test | `TestFavoritesAdd_HonorsNonDefaultProvider` | PR5 | todo | +| OUT-20 | cmd/favorites | `cmd/favorites.go:199-205` | Delete the `--type groups` / `--target`+`--role` pairing validation from `parseFavoritesAddFlags`. Dead-covered: `runFavoritesAddProduction` re-validates, so this is redundancy loss for DI callers, not a current user-facing hole | CONFIRMED | test | `TestParseFavoritesAddFlags_Validation` | PR5 | todo | +| OUT-21 | cmd/status | `cmd/status.go:110-114` | Make the directory-name merge unconditional: drop the `if _, exists := data.nameMap[k]; !exists` guard. Precedence is genuinely unasserted, but in production both lookups read the same cached Azure eligibility response, so a divergence needs colliding IDs or malformed data | OVERSTATED | test | `TestStatus_DirectoryNameMergePrecedence` | PR5 | todo | +| OUT-22 | cmd/status | `cmd/status.go:129` | Delete `_ = cache.CleanupSessions(tracker, activeIDs)` | CONFIRMED | test | `TestStatus_CleansUpStaleSessionTimestamps` | PR5 | todo | +| OUT-23 | cmd/status (test quality) | `cmd/status.go:185-192` (`computeRemainingTime`) | No production defect. `TestStatusCommand_RemainingTime/text_output_shows_remaining_time` asserts `remaining: 4` as a substring, which `remaining: 4h 30m` satisfies — only the JSON sibling killed a sixfold arithmetic error. Signal-poor assertion, not an uncovered defect | CONFIRMED | test | Tighten the text subtest to an exact `remaining: 45m` | PR5 | todo | +| OUT-24 | cmd/favorites | `cmd/favorites.go:432-433` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | todo | +| OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshBypassesCache` | PR5 | todo | +| OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | done | +| OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | refuted | n/a — already killed | — | todo | +| OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | +| OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | +| SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | todo | +| SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactBody` (add `gotBody` to `mockHTTPClient`) | PR8 | todo | +| SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactBody` | PR8 | todo | +| SCA-04 | internal/sca | `internal/sca/service.go:390` | `s.httpClient.Post(ctx, "/api/access/elevate/groups", req)` → `..., nil)` | CONFIRMED | test | `TestElevateGroups_SendsExactBody` | PR8 | todo | +| SCA-05 | internal/sca | `internal/sca/service.go:208` | Route `"/api/access/elevate"` → `"/WRONG"` | CONFIRMED | test | `TestElevate_Route` (add `gotRoute`) | PR8 | todo | +| SCA-06 | internal/sca | `internal/sca/service.go:258` | Route `"/api/access/sessions"` → `"/WRONG"` | CONFIRMED | test | `TestListSessions_Route` | PR8 | todo | +| SCA-07 | internal/sca | `internal/sca/service.go:236` | Route `"/api/access/sessions/revoke"` → `"/WRONG"` | CONFIRMED | test | `TestRevokeSessions_Route` | PR8 | todo | +| SCA-08 | internal/sca | `internal/sca/service.go:285` | `route := fmt.Sprintf("/api/access/%s/eligibility/groups", csp)` → `fmt.Sprintf("/WRONG/%s", csp)` | CONFIRMED | test | `TestListGroupsEligibility_Route` | PR8 | todo | +| SCA-09 | internal/sca | `internal/sca/service.go:390` | Route `"/api/access/elevate/groups"` → `"/WRONG"` | CONFIRMED | test | `TestElevateGroups_Route` | PR8 | todo | +| SCA-10 | internal/sca | `internal/sca/service.go:323` and `:356` | `"pageSize": -1` → `"pageSize": 10` at **both** on-demand call sites. The wire contract is genuinely untested; the *consequence* ("-1 means all, so 10 truncates the role picker") is **unevidenced** — neither the repo, the pinned SDK, nor official docs document this endpoint's `-1` semantics. Assert the sent value; do not assert a truncation story | OVERSTATED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | todo | +| SCA-11 | internal/sca | `internal/sca/service.go:326` and `:360` | `"target_category": "cloud_console"` → `"WRONG"` at **both** on-demand call sites | CONFIRMED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | todo | +| SCA-12 | internal/sca | `internal/sca/service.go:258-262` | `ListSessions`'s `buildParams` closure returns `nil` instead of `map[string]string{"csp": string(*csp)}`. `TestListSessions_WithCSPFilter` is tautological — its canned response is already Azure and it never inspects params. `grant status --provider azure` does no local filtering, so all providers' sessions would display | CONFIRMED | test | Replace `TestListSessions_WithCSPFilter` with `TestListSessions_SendsCSPQueryParam` (add `gotParams`) | PR8 | todo | +| SCA-13 | internal/sca | `internal/sca/service.go:144` | `s.httpClient.Get(ctx, route, p)` → `s.httpClient.Get(context.Background(), route, p)` in `paginate` | CONFIRMED | test | `TestPaginate_PropagatesContextCancellation` | PR8 | todo | +| SCA-14 | internal/sca | `internal/sca/service.go:184` (and the sibling decoders at `:267`, `:291`) | In the `ListEligibility` decode closure, swallow the error: `if err := json.NewDecoder(r).Decode(&page); err != nil { return nil, nil, 0, nil }` | CONFIRMED | test | `TestListEligibility_PropagatesDecodeError` | PR8 | todo | +| SCA-15 | internal/sca | `internal/sca/service.go:74` | `client.SetHeader("X-API-Version", "2.0")` → delete the call, and separately → `"1.0"`. The **only** guard lives inside `TestNewSCAAccessServiceDisablesTransientRetry`, so a retry-motivated rename silently deletes the header assertion. Verifier could not execute it (loopback prohibited in that sandbox); coverage topology confirmed by grep | OVERSTATED | test | Extract `TestNewSCAAccessService_SetsAPIVersionHeader` as its own named test | PR8 | todo | +| SCA-16 | internal/sca models | `internal/sca/models/elevate.go:28` | `RoleID string \`json:"roleId"\`` → `json:"roleIdXX"` on the **request** model | CONFIRMED | test | `TestElevateRequest_JSONTags` | PR8 | todo | +| SCA-17 | internal/sca models | `internal/sca/models/credentials.go:17` (`ParseAWSCredentials`) | Swap `SecretAccessKey` and `SessionToken` in the parser output. **Mutant dies repo-wide**: `env_test.go:77` and `root_elevate_test.go:426`. Nuance: swapping the *struct JSON tags* instead is also caught, by `TestAWSCredentials_JSONUnmarshal`. Recorded so this is not re-filed as a survivor | REFUTED | refuted | n/a — already killed | — | todo | +| SCA-18 | internal/sca | `internal/sca/service.go:75-ish` (`sdkclient.DisableTransientRetry` call) | Claim: deleting the call yields `inbound requests = 4, want 1`. **Not reproducible** in the verifier's sandbox (loopback prohibited); a literal deletion is a compile kill first (`"internal/sdkclient" imported and not used`). Same for the workflows twin. The guard tests exist and are correctly aimed; only their runtime assertion was unverifiable | OVERSTATED | refuted | n/a — `internal/sca/retry_policy_test.go` / `internal/workflows/retry_policy_test.go` already guard this | — | todo | +| WF-01 | internal/workflows | `internal/workflows/service.go:102` | Delete `if err := checkResponse(resp, "request forms"); err != nil { return nil, err }` | CONFIRMED | test | `TestWorkflows_Non200` (table over all six call sites) | PR8 | todo | +| WF-02 | internal/workflows | `internal/workflows/service.go:161` | Delete the `checkResponse(resp, "list requests")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-03 | internal/workflows | `internal/workflows/service.go:196` | Delete the `checkResponse(resp, "get request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-04 | internal/workflows | `internal/workflows/service.go:221` | Delete the `checkResponse(resp, "submit request")` guard. Verified consequence: a 500 carrying `{}` decodes to an empty request, so `grant request submit` prints a blank `Request ID:` / `State:` and exits 0 | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-05 | internal/workflows | `internal/workflows/service.go:245` | Delete the `checkResponse(resp, "cancel request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-06 | internal/workflows | `internal/workflows/service.go:272` | Delete the `checkResponse(resp, "finalize request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-07 | internal/workflows | `internal/workflows/logging_client.go:58` | Delete the `if redacted.Get("Authorization") != ""` redaction block. Real token-in-logs risk; the SCA twin is covered by `TestLoggingClient_DebugLogsHeaders` | CONFIRMED | test | new `internal/workflows/logging_client_test.go`, mirroring the sca one **including Authorization redaction** | PR8 | todo | +| WF-08 | internal/workflows | `internal/workflows/logging_client.go:24-26` | Route `Get` through `c.inner.Post(ctx, route, params)` | CONFIRMED | test | `TestLoggingClient_GetUsesGet` (workflows) | PR8 | todo | +| WF-09 | internal/workflows | `internal/workflows/logging_client.go:24-33` | Swallow the inner error and return a synthetic 200 response with `err = nil` | CONFIRMED | test | `TestLoggingClient_PropagatesInnerError` (workflows) | PR8 | todo | +| WF-10 | internal/workflows models | `internal/workflows/models/submit.go:6` | `RequestDetails map[string]interface{} \`json:"requestDetails"\`` → `json:"requestDetailsXX"` | CONFIRMED | test | `TestSubmitAccessRequest_JSONTags` | PR8 | todo | +| WF-11 | internal/workflows models | `internal/workflows/models/finalize.go:5` | `Result string \`json:"result"\`` → `json:"resultXX"` | CONFIRMED | test | `TestFinalizeAccessRequest_JSONTags` | PR8 | todo | +| WF-12 | internal/workflows models | `internal/workflows/models/cancel.go:5` | `CancelReason *string \`json:"cancelReason"\`` → `json:"cancelReasonXX"` | CONFIRMED | test | `TestCancelAccessRequest_JSONTags` | PR8 | todo | +| WF-13 | internal/workflows | `internal/workflows/service.go:263` | Delete `FinalizationReason: reason,` from the `FinalizeAccessRequest` literal | CONFIRMED | test | `TestFinalizeRequest_SendsFinalizationReason` | PR8 | todo | +| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`service_test.go:274`), which is the contrast that proves the gap | CONFIRMED | test | `TestFinalizeRequest_ExactRoute` | PR8 | todo | +| WF-15 | internal/workflows | `internal/workflows/service.go:141` | Delete `qp["limit"] = strconv.Itoa(limit)` | CONFIRMED | test | `TestListRequests_SendsLimit` | PR8 | todo | +| WF-16 | internal/workflows | `internal/workflows/service.go:124` | `const defaultPageSize = 50` → `= 1` | CONFIRMED | test | `TestListRequests_DefaultPageSize` | PR8 | todo | +| WF-17 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `Get(context.Background(), ...)` in the pagination loop | CONFIRMED | test | `TestListRequests_PropagatesContextCancellation` | PR8 | todo | +| WF-18 | internal/workflows | `internal/workflows/service.go:201` | In `GetRequest`, swallow the decode error: `if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return &result, nil }` | CONFIRMED | test | `TestGetRequest_PropagatesDecodeError` | PR8 | todo | +| WF-19 | internal/workflows | `internal/workflows/service_config.go:9` | `ServiceName: "access-requests"` → `"WRONG"`. There is no workflows `service_config_test.go` at all | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | +| WF-20 | internal/workflows | `internal/workflows/service.go:45` | `base.Authenticator("isp")` → `base.Authenticator("WRONG")` | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | +| ELV-01 | cmd/selection | `cmd/selection.go:78` | `return &items[i], nil` → `return &items[0], nil`. `TestFindItemByDisplay` only checks non-nil/error, so selecting one display value silently elevates the first sorted target and prints a success line naming the wrong one | CONFIRMED | test | `TestFindItemByDisplay_ReturnsMatchingItem` | PR4 | todo | +| ELV-02 | cmd/root (unified elevate builder) | `cmd/root.go:786-793` | In `elevateCloud`, swap `WorkspaceID: selectedTarget.WorkspaceID` and `RoleID: selectedTarget.RoleInfo.ID` | CONFIRMED | test | `TestElevateCloud_RequestPayload` (`mockElevateService` history) | PR4 | todo | +| ELV-03 | cmd/root (unified elevate builder) | `cmd/root.go:786-788` | In `elevateCloud`, blank both `CSP:` and `OrganizationID:` | CONFIRMED | test | `TestElevateCloud_RequestPayload` | PR4 | todo | +| ELV-04 | cmd/root (env/direct elevate builder) | `cmd/root.go:497-506` | In `resolveAndElevate`, swap `WorkspaceID` and `RoleID` in the `ElevateRequest` literal. (Plan calls these "both builders": `:497` and `:786`) | CONFIRMED | test | `TestResolveAndElevate_RequestPayload` | PR4 | todo | +| ELV-05 | cmd/env favorite path | `cmd/root.go:429` | `if flags.favorite != "" {` → `if false {` in `resolveAndElevate`. No env test exercises `--favorite`, yet the flag is registered (`cmd/env.go:39`) and advertised in help (`cmd/env.go:29`) | CONFIRMED | test | `TestEnv_FavoriteMode` | PR4 | todo | +| ELV-06 | cmd/env favorite path | `cmd/root.go:438-440` | Delete the group-favorite rejection (`if fav.ResolvedType() == config.FavoriteTypeGroups { return ... }`) | CONFIRMED | test | `TestEnv_RejectsGroupFavorite` | PR4 | todo | +| ELV-07 | cmd/env favorite path | `cmd/root.go:443-445` | Delete the provider-mismatch check `if flags.provider != "" && !strings.EqualFold(flags.provider, fav.Provider)` | CONFIRMED | test | `TestEnv_FavoriteProviderMismatch` | PR4 | todo | +| ELV-08 | cmd/env direct path | `cmd/root.go:456-458` | Delete the paired `--target`/`--role` validation in `resolveAndElevate` | CONFIRMED | test | `TestEnv_RequiresBothTargetAndRole` | PR4 | todo | +| ELV-09 | cmd/root favorite path | `cmd/root.go:577` | `if fav.ResolvedType() == config.FavoriteTypeGroups {` → `if false {` in `resolveFavoriteFlags`. This is the row that refutes "all root equivalents are covered" — root group-favorite **detection** is also unpinned | CONFIRMED | test | `TestResolveFavoriteFlags_DetectsGroupFavorite` | PR4 | todo | +| ELV-10 | cmd/root group elevate | `cmd/root.go:837-841` | `if result.ErrorInfo != nil {` → `if false {` in `elevateGroup`. Execution then builds a result and returns nil: a policy denial prints as success and exits 0 | CONFIRMED | test | `TestElevateGroup_SurfacesErrorInfo` | PR4 | todo | +| ELV-11 | cmd/env elevate | `cmd/root.go:525-530` | `if result.ErrorInfo != nil {` → `if false {` in `resolveAndElevate` (the env path). Same false-success consequence | CONFIRMED | test | `TestEnv_SurfacesErrorInfo` | PR4 | todo | +| ELV-12 | cmd/env elevate | `cmd/root.go:520-522` | Delete `if len(elevateResp.Response.Results) == 0 { return nil, errors.New("elevation failed: no results returned") }` (env path). Mutant panics on `Results[0]` only if a test supplies an empty slice — none does | CONFIRMED | test | `TestEnv_EmptyResultsGuard` | PR4 | todo | +| ELV-13 | cmd/root cloud elevate | `cmd/root.go:802-805` | Delete the identical empty-`Results` guard in `elevateCloud` | CONFIRMED | test | `TestElevateCloud_EmptyResultsGuard` | PR4 | todo | +| ELV-14 | cmd/root group elevate | `cmd/root.go:832-835` | Delete the identical empty-`Results` guard in `elevateGroup` | CONFIRMED | test | `TestElevateGroup_EmptyResultsGuard` | PR4 | todo | +| ELV-15 | cmd/root JSON | `cmd/root.go:936-937` | In `writeElevationJSON`, swap `Target: cloudRes.target.WorkspaceName` and `Role: cloudRes.target.RoleInfo.Name` | CONFIRMED | test | `TestElevationJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| ELV-16 | cmd/root JSON | `cmd/root.go:923-924` | In `writeElevationJSON`, swap `GroupID: groupRes.group.GroupID` and `DirectoryID: groupRes.group.DirectoryID` | CONFIRMED | test | `TestGroupElevationJSON_Contract` | PR5 | todo | +| ELV-17 | cmd/env JSON | `cmd/env.go:145-147` | Swap `SecretAccessKey: awsCreds.SecretAccessKey` and `SessionToken: awsCreds.SessionToken`. Asymmetry is the point: the identical swap in the **text** export path is killed (`env_test.go:77`) | CONFIRMED | test | `TestEnvJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| ELV-18 | cmd/root JSON | `cmd/root.go:934` | `Provider: strings.ToLower(string(cloudRes.target.CSP))` → drop the `strings.ToLower` | CONFIRMED | test | `TestElevationJSON_Contract` | PR5 | todo | +| ELV-19 | cmd/root | `cmd/root.go:341-344` | Delete `if len(all) == 0 { return nil, errors.New("no eligible targets found, check your SCA policies") }` in `fetchEligibility`'s multi-CSP branch. Callers replace the intended aggregate error with their own message | CONFIRMED | test | `TestFetchEligibility_AllCSPsFail` | PR4 | todo | +| ELV-20 | cmd/env elevate | `cmd/root.go:510-514` | Remove the fresh-context setup and elevate with the original `ctx`: delete `elevCtx, elevCancel := context.WithTimeout(...)` / `defer elevCancel()` and call `elevateService.Elevate(ctx, req)`. **Note:** a raw `elevCtx → ctx` token substitution does *not* compile (`declared and not used: elevCtx`) — use the semantic form above. Root's three interactive dispatch paths *are* covered by `TestRootElevate_SlowPromptTimeout`; env is not | CONFIRMED | test | `TestEnv_SlowPromptTimeout` | PR4 | todo | +| ELV-21 | cmd/env auth | `cmd/root.go:419` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in `resolveAndElevate` (env path) | CONFIRMED | test | `TestEnv_AuthCacheFlag` | PR4 | todo | +| ELV-22 | cmd/root auth | `cmd/root.go:620` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in the root elevate path | CONFIRMED | test | `TestRootElevate_AuthCacheFlag` | PR4 | todo | +| ELV-23 | cmd/env selector | `cmd/root.go:480` | `selector.SelectTarget(allTargets)` → `selector.SelectTarget(nil)`. `mockTargetSelector` returns its canned target without inspecting the slice | CONFIRMED | test | `TestEnv_SelectorReceivesAllTargets` | PR4 | todo | +| ELV-24 | cmd/root Execute | `cmd/root.go:291` | `if !verbose && passedArgValidation {` → `if verbose && passedArgValidation {`. `TestVerboseHintSuppressedForArgErrors` calls Cobra's `root.Execute()` and then *reconstructs* the hint logic; it never invokes the package-level `Execute()` | CONFIRMED | test | `TestExecute_VerboseHintCondition` | PR4 | todo | +| ELV-25 | cmd/root dispatch | `cmd/root.go:631-636` | Swap the dispatch order: test `if flags.groups` before `if flags.group != ""`. `--group` and `--groups` are **not** mutually exclusive (`root.go:136-142` pairs neither), so their precedence is unspecified and unpinned | CONFIRMED | test | `TestRootElevate_GroupAndGroupsPrecedence` | PR4 | todo | +| ELV-26 | cmd test quality | `cmd/root_elevate_test.go:248` | No production site. The `multi-CSP concurrent fetch - parallel execution` case duplicates the line-174 success setup, adds sleeps, and asserts **no** elapsed time. Real concurrency is covered by `TestFetchEligibility_ConcurrentExecution` | CONFIRMED | test | Delete the duplicate case or give it a real elapsed-time assertion | PR4 | todo | +| ELV-27 | cmd test quality | `cmd/root_test.go` (`TestFetchEligibility_ConcurrentExecution`) | Claim: the `<350ms` bound for two concurrent 200ms sleeps is flaky. **Not demonstrated** — 50/50 runs passed. The wall-clock sensitivity remains a plausible overloaded-CI risk, so widen the bound; do not claim an observed flake | OVERSTATED | test | Widen the bound in `TestFetchEligibility_ConcurrentExecution` | PR4 | todo | +| 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 | todo | +| CACHE-02 | internal/cache | `internal/cache/cached_eligibility.go:117` | Same mutation on the groups-eligibility write | CONFIRMED | test | `TestCachedGroupsEligibility_RefreshStillWrites` | PR6 | todo | +| CACHE-03 | internal/cache | `internal/cache/cached_roles.go:53` | Same mutation on the on-demand-roles write | CONFIRMED | test | `TestCachedRoles_RefreshStillWrites` | PR6 | todo | +| 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 | Fix `TestGet_CorruptJSON` to use a **fresh** `cached_at` with a type-mismatched payload, so only the unmarshal guard can produce the miss | PR6 | todo | +| 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 | todo | +| 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 | todo | +| 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`. **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 | todo | +| 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 | Portable replacement: `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 | todo | +| 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`, `-5m`, `garbage`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go:541`. CHANGELOG `### Changed` | PR6 | todo | +| 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 | todo | +| 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 | todo | +| 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 | todo | +| CFG-06 | internal/config | `internal/config/config.go:61-63` | Swallow the YAML error: `_ = yaml.Unmarshal(data, cfg)` | CONFIRMED | test | `TestLoad_InvalidYAMLErrors` | PR6 | todo | +| CFG-07 | internal/config | `internal/config/config.go:103` | `filepath.Join(home, ".grant")` → `".grantx"` | CONFIRMED | test | `TestConfigDir_EndsInDotGrant` | PR6 | todo | +| CFG-08 | internal/config | `internal/config/config.go:84` | `os.WriteFile(path, data, 0o600)` → `0o644` | CONFIRMED | test | `TestSave_FileMode` with the `runtime.GOOS == "windows"` skip | PR6 | todo | +| CFG-09 | internal/config | `internal/config/config.go:75` | `if err := os.MkdirAll(dir, 0o700); err != nil { ... }` → ignore the error | CONFIRMED | test | `TestSave_MkdirAllFailure` — force it portably by pointing at a path whose parent component is an existing **regular file** (ENOTDIR / ERROR_DIRECTORY), never a hardcoded `/dev/null/...` | PR6 | todo | +| 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 | + +--- + +## Summary + +### By PR + +| PR | Rows | Of which CONFIRMED | +|---|---|---| +| PR1 — Test isolation + integration harness | 2 | 0 (both OVERSTATED) | +| PR2 — Archive extraction and path security | 12 | 11 | +| PR3 — Remaining self-update correctness | 10 | 10 | +| PR4 — Argument capture | 42 | 41 | +| PR5 — Output contracts | 32 | 30 | +| PR6 — Cache and config semantics | 16 | 16 | +| PR7 — UI behavior | 10 | 10 | +| PR8 — SCA / workflows / models wire contracts | 36 | 34 | +| *(no PR — settled, recorded only)* | 5 | 0 | +| **Total** | **165** | **152** | + +### By verdict + +| Verdict | Rows | +|---|---| +| CONFIRMED | 152 | +| OVERSTATED | 9 | +| REFUTED | 4 | + +### By disposition + +| Disposition | Rows | +|---|---| +| `test` | 149 | +| `test + prod-fix` | 5 | +| `prod-fix` | 1 | +| `wont-fix` | 5 | +| `refuted` | 5 | +| **Total** | **165** | + +The seven production changes, matching the plan's table: + +| Row | Change | PR | CHANGELOG | +|---|---|---|---| +| SFU-07 | Reject zero-length extracted binary + apply-boundary check | PR2 | `### Security` | +| 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` | +| CACHE-07 | `maxSessionAge` → `sessionTimestampRetention` | PR6 | no (internal) | +| UI-02 | `sortGroupsForDisplay` extraction | PR7 | no (refactor) | +| SFU-10 | `syncStagedFileFn` seam | PR3 | no (test seam; **not** a production gap) | + +### Total CONFIRMED + +**152 CONFIRMED**, plus **9 OVERSTATED** (real survivors whose stated consequence +was weaker than originally claimed) and **4 REFUTED**. **165 rows total.** +5 rows are `wont-fix` (SFU-20, SFU-21, SFU-22, UI-09, UI-10), so **160 rows require +work**, of which 6 carry a production change. + +### Reconciliation against "145" + +The ledger does **not** reconcile cleanly to 145, and the rows were not padded or +truncated to make it. The gap is almost entirely *granularity* — every row traces +to a named, independently reproduced finding in a verification report. + +| Batch | Verifier's stated count | Rows here | Reconciles? | +|---|---|---|---| +| 1 — cmd request/auth | 22 confirmed | 23 (22 CONFIRMED + 1 OVERSTATED) | **Yes.** The 22 CONFIRMED rows are mutation-level and match exactly. REQ-23 (integration suite absent from CI) is reported *outside* the 22. | +| 2 — cmd status/favorites/list | 25 real of 29 claimed | 29 (23 CONFIRMED, 3 OVERSTATED, 3 REFUTED) | **Approximately.** All 29 claimed items are listed so the three refutations stay recorded. 23 + 3 OVERSTATED = 26 actionable, one above the verifier's "25" — its own prose is imprecise about whether OUT-21/OUT-25/OUT-26 count as "real". | +| 3 — internal/sca + workflows | 31 + 4 extra = 35 | 38 (34 CONFIRMED, 3 OVERSTATED, 1 REFUTED) | **Yes.** 34 CONFIRMED + SCA-10 (a real survivor, only its truncation consequence overstated) = exactly 35 survivors. The other 3 rows are SCA-15 (X-API-Version coverage topology, which PR8 acts on) and SCA-17/SCA-18, recorded so they are not re-filed. | +| 4 — cmd root/elevate/env | 22 confirmed | 27 (26 CONFIRMED, 1 OVERSTATED) | **No — +5.** The report's headline groups its own sub-lettered mutations inconsistently: H2.1–2.3, H3.1–3.4, M4.1–2, M5.1–3, M6.1–6.4 and M9.1–3 are enumerated individually in the body, each with its own `go test ./cmd/ -count=1 → ok` transcript, but collapsed in the total. Listing them individually gives 25 production mutations plus 2 test-quality rows (ELV-26, ELV-27). | +| 5 — cache/config/ui/selfupdate | 44 reproduced, 41 actionable | 48 (all CONFIRMED, 4 of them `wont-fix`) | **Partly — +4.** Same granularity problem: M7 and M9 are two mutations each; L1, L3, L4 and L5 are three each; L9 is three survivors. Enumerating every reproduced mutation gives 48; removing the 4 unreachable/defensive `wont-fix` rows (SFU-20, SFU-21, UI-09, UI-10) gives **44 actionable**, matching "44 reproduced" but not "41 actionable" — the report never itemises which 3 it dropped. | + +**Net: 165 rows against a headline of 145.** Roughly 9 of the excess is finer +enumeration in batches 4 and 5 (mutations the reports reproduced individually but +totalled in groups); the rest is the 13 OVERSTATED/REFUTED rows the headline count +deliberately excluded but which belong here as settled questions. Nothing was +invented and nothing was dropped to hit a number. + +There are no `NEEDS-REVIEW` rows: every row's source report is unambiguous about +the mutation applied and the observed result. + +--- + +## PR1 closure evidence + +Both PR1 rows are `done`. Neither has a production mutation of its own (both are +harness rows), so each is closed against the mutation that its new assertion is +supposed to kill. + +**REQ-23** — mutation: `cmd/version.go:31`, `v = "dev"` → `v = "bogus"`. + +| assertion | result | +|---|---| +| old, `contains("dev") \|\| contains("unknown")` | `ok` — **survived**; `commit: unknown` satisfies the second arm regardless of the version string | +| new, `contains("grant version dev")` | `FAIL: expected a dev build banner, got: grant version bogus` | + +Reverted; `go test -tags=integration ./cmd -count=1 -run TestIntegration_Version` → `ok`. +The other half of the row (integration tests absent from CI) is closed by the +`Integration tests` step running `go test -tags=integration ./cmd` on both CI legs. + +**OUT-26** — mutation: `cmd/main_test.go` `TestMain`, unwrap `testenv.Run` so it +calls `installBootstrapStub(); os.Exit(m.Run())` directly. + +``` +--- FAIL: TestSandboxIsolation (0.00s) + main_test.go:40: testenv.AssertSandboxed called outside testenv.Run; no sandbox is active +--- FAIL: TestCacheDirResolvesInsideSandbox (0.00s) + main_test.go:56: no testenv sandbox is active; TestMain is not wrapping m.Run +``` + +Reverted; both tests `ok`. The row's second half (the `favorites add` +non-interactive guard) was mutation-verified when it landed. + +**Redirect-list drop-one.** Every entry of `testenv.redirectedVars` was deleted +in turn and `go test ./cmd ./internal/config ./internal/cache ./internal/testenv +-count=1` run. Before the explicit-literal and hostile-value tests, four of five +survived (`USERPROFILE`, `XDG_CONFIG_HOME`, `IDSEC_PROFILES_FOLDER`, +`GRANT_CONFIG` — with `HOME` redirected their fallbacks already land in-sandbox). +After, all eight fail. diff --git a/internal/cache/main_test.go b/internal/cache/main_test.go new file mode 100644 index 0000000..24b08bc --- /dev/null +++ b/internal/cache/main_test.go @@ -0,0 +1,27 @@ +// This file is in the external test package (cache_test) on purpose: the +// testenv helper imports internal/cache, so an in-package test file importing +// testenv would be an import cycle. TestMain may live in either test package +// and still governs the whole test binary. +package cache_test + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory so tests in this +// package can never touch the developer's real ~/.grant cache. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, or a resolver starts reading a variable testenv does not override, +// this fails instead of silently writing to the developer's home directory. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} diff --git a/internal/config/main_test.go b/internal/config/main_test.go new file mode 100644 index 0000000..0ced660 --- /dev/null +++ b/internal/config/main_test.go @@ -0,0 +1,27 @@ +// This file is in the external test package (config_test) on purpose: the +// testenv helper imports internal/config, so an in-package test file importing +// testenv would be an import cycle. TestMain may live in either test package +// and still governs the whole test binary. +package config_test + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory so tests in this +// package can never touch the developer's real ~/.grant or ~/.idsec. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, or a resolver starts reading a variable testenv does not override, +// this fails instead of silently writing to the developer's home directory. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} diff --git a/internal/sca/main_test.go b/internal/sca/main_test.go new file mode 100644 index 0000000..4b61414 --- /dev/null +++ b/internal/sca/main_test.go @@ -0,0 +1,28 @@ +package sca + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory. This package +// drives the *real* service constructors (see retry_policy_test.go), which +// build an SDK client and can therefore reach the SDK's profile and keyring +// resolution; without the sandbox a new test here could read or write the +// developer's real ~/.idsec with no failing signal. +// +// An in-package (not _test) TestMain is safe here: testenv imports +// internal/sca/models, not internal/sca, so there is no import cycle. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, this fails instead of the suite silently touching real user state. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} diff --git a/internal/sdkclient/main_test.go b/internal/sdkclient/main_test.go new file mode 100644 index 0000000..00c655c --- /dev/null +++ b/internal/sdkclient/main_test.go @@ -0,0 +1,24 @@ +package sdkclient + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory. This package +// constructs real SDK clients, which can reach the SDK's profile and keyring +// resolution; without the sandbox a new test here could read or write the +// developer's real ~/.idsec with no failing signal. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, this fails instead of the suite silently touching real user state. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} diff --git a/internal/testenv/testenv.go b/internal/testenv/testenv.go new file mode 100644 index 0000000..8507e55 --- /dev/null +++ b/internal/testenv/testenv.go @@ -0,0 +1,303 @@ +// Package testenv redirects the environment variables grant and its SDK use to +// locate user state at a throwaway temporary directory, so running the test +// suite can never read or clobber the developer's real ~/.grant or ~/.idsec. +// +// # What it does not cover +// +// The redirect is a list of known variables (see redirectedVars, plus +// unsetVars for the ones that are removed rather than pointed elsewhere), not a +// containment boundary. Anything that reaches user state by another route +// escapes it: +// +// - A direct os.UserHomeDir call is covered only because HOME/USERPROFILE are +// redirected; a hardcoded path or a newly-added SDK variable is not covered +// at all, and nothing here discovers one automatically. +// - The OS keyring is a daemon, not a path, so no environment redirect can +// sandbox it. Run sets IDSEC_BASIC_KEYRING=1 to force the SDK's file +// backend into the sandboxed IDSEC_KEYRING_FOLDER instead. If a future SDK +// stops honoring that variable, keyring access leaves the sandbox again +// and nothing here will notice. +// +// It is a normal (non-test) package so that TestMain functions in several +// packages can share it, but it is imported only from _test.go files and so +// never links into the shipped binary. It deliberately does NOT import +// "testing": that import belongs to test binaries, and keeping it out means +// no importer inherits the testing flag set. +// +// # What the assertions actually prove +// +// AssertSandboxed verifies that the *configured destinations* — the paths +// config.ConfigDir, config.ConfigPath, cache.CacheDir, +// profiles.GetProfilesFolder, the SDK keyring folder and the SDK file-log path +// resolve to — all sit under the sandbox root, and that IDSEC_BASIC_KEYRING is +// set so the file keyring is the one in use. That +// is all it proves. It does NOT prove that no code wrote outside the sandbox: +// it cannot see a future direct os.UserHomeDir call, a hardcoded path, or a +// dependency that writes via some other variable, and it cannot detect reads at +// all. A filesystem snapshot-diff gate was considered and rejected — a +// concurrently running real `grant` false-positives with certainty, size+mtime +// misses same-size rewrites, and reads stay invisible either way. +package testenv + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/aaearon/grant-cli/internal/cache" + "github.com/aaearon/grant-cli/internal/config" + "github.com/cyberark/idsec-sdk-golang/pkg/profiles" +) + +// redirectedVars lists every environment variable Run overrides, in the order +// it sets them. Each one is a way for some resolver to reach the real home: +// +// - HOME — POSIX os.UserHomeDir, and the SDK profile loader, +// which reads os.Getenv("HOME") directly on every platform. +// - USERPROFILE — Go's Windows os.UserHomeDir reads USERPROFILE, then +// HOMEDRIVE+HOMEPATH. It never consults HOME, so +// redirecting HOME alone leaves the Windows CI leg +// pointed at the real profile directory. +// - XDG_CONFIG_HOME — speculative/defensive. Nothing in grant or in the +// pinned SDK reads it today (`rg XDG_` finds only this +// comment and testenv's own tests). It is redirected +// because it is the conventional escape hatch a config +// helper would reach for, and a redirect costs nothing. +// - IDSEC_PROFILES_FOLDER — takes precedence over HOME in the SDK loader. +// - GRANT_CONFIG — overrides config.ConfigPath. +// - IDSEC_KEYRING_FOLDER — overrides the SDK file-keyring folder outright +// (pkg/common/keyring/idsec_basic_keyring.go), bypassing +// its HOME fallback. A pre-existing value in the +// developer's or CI environment would otherwise put +// keyring writes outside the sandbox. +// - IDSEC_FILE_LOG_PATH — overrides the SDK's file-log destination +// (pkg/config, consumed in pkg/common/idsec_logger.go), +// whose parent directory the SDK MkdirAll's. +// - IDSEC_BASIC_KEYRING — not a path: any non-empty value forces the SDK's +// file keyring. Without it, a plain Linux box with +// DBUS_SESSION_BUS_ADDRESS set selects the real +// libsecret store, which no path redirect can sandbox. +var redirectedVars = []string{ + "HOME", + "USERPROFILE", + "XDG_CONFIG_HOME", + "IDSEC_PROFILES_FOLDER", + "GRANT_CONFIG", + "IDSEC_KEYRING_FOLDER", + "IDSEC_FILE_LOG_PATH", + "IDSEC_BASIC_KEYRING", +} + +// unsetVars lists environment variables Run REMOVES for the duration of the +// run instead of redirecting. They select behavior rather than a location, so +// there is no sandbox value to point them at — the only safe state is absent, +// which is what CI has and what a developer's shell may not: +// +// - IDSEC_PROFILE — the SDK profile loader +// (pkg/profiles/idsec_profile_loader.go) reads it to choose the DEFAULT +// PROFILE NAME. A developer with it exported runs the suite against a +// different profile than CI does, from the same source tree. +// - DEPLOY_ENV — feeds tenant-environment resolution inside +// isp.FromISPAuth (pkg/common/isp/idsec_isp_service_client.go), which the +// internal/sca and internal/workflows retry-policy tests now drive for +// real, so an exported value changes what those constructors resolve. +// +// Restoration is exact: a variable that was set comes back with its original +// value, one that was unset stays unset. +var unsetVars = []string{ + "IDSEC_PROFILE", + "DEPLOY_ENV", +} + +// nonPathVars are the entries of redirectedVars whose value is a mode switch +// rather than a filesystem path, so "must resolve under the sandbox root" does +// not apply to them. +var nonPathVars = map[string]bool{ + "IDSEC_BASIC_KEYRING": true, +} + +// sandboxRoot is the active sandbox root, or "" when Run is not executing. +var sandboxRoot string + +// Root returns the active sandbox root directory, or "" outside of Run. +func Root() string { return sandboxRoot } + +// Run creates a temporary sandbox root, redirects every variable in +// redirectedVars beneath it, invokes run, then restores the previous +// environment and removes the sandbox. It returns run's exit code so callers +// can write: +// +// func TestMain(m *testing.M) { os.Exit(testenv.Run(m.Run)) } +// +// Setting the variables here — before m.Run — is what makes this compatible +// with the t.Parallel() call sites throughout internal/: os.Setenv has no +// parallel restriction, whereas t.Setenv panics in a parallel test. +func Run(run func() int) int { + root, err := os.MkdirTemp("", "grant-testenv-") + if err != nil { + fmt.Fprintf(os.Stderr, "testenv: failed to create sandbox root: %v\n", err) + return 1 + } + + home := filepath.Join(root, "home") + if err := os.MkdirAll(home, 0o700); err != nil { + fmt.Fprintf(os.Stderr, "testenv: failed to create sandbox home: %v\n", err) + _ = os.RemoveAll(root) + return 1 + } + + values := map[string]string{ + "HOME": home, + "USERPROFILE": home, + "XDG_CONFIG_HOME": filepath.Join(home, ".config"), + "IDSEC_PROFILES_FOLDER": filepath.Join(home, ".idsec", "profiles"), + "GRANT_CONFIG": filepath.Join(home, ".grant", "config.yaml"), + "IDSEC_KEYRING_FOLDER": filepath.Join(home, ".idsec", "cache", "keyring"), + "IDSEC_FILE_LOG_PATH": filepath.Join(home, ".idsec", "logs", "idsec.log"), + "IDSEC_BASIC_KEYRING": "1", + } + + // Deferred so a panic inside run — a -race detection, a stray panic in a + // test — still restores the environment and removes the sandbox instead of + // leaking the directory and leaving the process redirected. + restore := make(map[string]*string, len(redirectedVars)+len(unsetVars)) + defer func() { + restoreEnv(restore) + _ = os.RemoveAll(root) + }() + + // capture records k's current state so restoreEnv can put it back exactly, + // preserving the set-with-value / unset distinction. + capture := func(k string) { + if v, ok := os.LookupEnv(k); ok { + prev := v + restore[k] = &prev + return + } + restore[k] = nil + } + + for _, k := range redirectedVars { + capture(k) + if err := os.Setenv(k, values[k]); err != nil { + fmt.Fprintf(os.Stderr, "testenv: failed to set %s: %v\n", k, err) + return 1 + } + } + + for _, k := range unsetVars { + capture(k) + if err := os.Unsetenv(k); err != nil { + fmt.Fprintf(os.Stderr, "testenv: failed to unset %s: %v\n", k, err) + return 1 + } + } + + // Save and restore rather than clearing: a nested Run must hand the outer + // run's root back, not "". + prevRoot := sandboxRoot + sandboxRoot = root + defer func() { sandboxRoot = prevRoot }() + + return run() +} + +// restoreEnv puts back the captured values; a nil entry means the variable was +// originally unset and must be unset again rather than set to "". +func restoreEnv(restore map[string]*string) { + for k, v := range restore { + if v == nil { + _ = os.Unsetenv(k) + continue + } + _ = os.Setenv(k, *v) + } +} + +// TB is the subset of *testing.T that AssertSandboxed needs. Accepting an +// interface is what lets this file stay free of the "testing" import. +type TB interface { + Helper() + Errorf(format string, args ...any) +} + +// AssertSandboxed checks that every path grant resolves for user state lands +// under the active sandbox root. See the package comment for exactly what this +// does and does not prove. +func AssertSandboxed(t TB) { + t.Helper() + + root := Root() + if root == "" { + t.Errorf("testenv.AssertSandboxed called outside testenv.Run; no sandbox is active") + return + } + + configDir, err := config.ConfigDir() + if err != nil { + t.Errorf("config.ConfigDir() failed inside sandbox: %v", err) + } else { + assertUnder(t, "config.ConfigDir()", configDir, root) + } + + configPath, err := config.ConfigPath() + if err != nil { + t.Errorf("config.ConfigPath() failed inside sandbox: %v", err) + } else { + assertUnder(t, "config.ConfigPath()", configPath, root) + } + + cacheDir, err := cache.CacheDir() + if err != nil { + t.Errorf("cache.CacheDir() failed inside sandbox: %v", err) + } else { + assertUnder(t, "cache.CacheDir()", cacheDir, root) + } + + assertUnder(t, "profiles.GetProfilesFolder()", profiles.GetProfilesFolder(), root) + assertUnder(t, "SDK keyring folder", keyringFolder(), root) + assertUnder(t, "SDK file log path", fileLogPath(), root) + + if os.Getenv("IDSEC_BASIC_KEYRING") == "" { + t.Errorf("IDSEC_BASIC_KEYRING is empty; the SDK may select the real OS keyring, which no path redirect can sandbox") + } +} + +// keyringFolder mirrors the SDK's keyring folder resolution +// (pkg/common/keyring/idsec_basic_keyring.go NewIdsecBasicKeyring): the +// IDSEC_KEYRING_FOLDER override, else DefaultBasicKeyringFolder under HOME. +// It is reimplemented rather than called because the SDK constructor creates +// the directory as a side effect, which an assertion must not do. +func keyringFolder() string { + if folder := os.Getenv("IDSEC_KEYRING_FOLDER"); folder != "" { + return folder + } + return filepath.Join(os.Getenv("HOME"), ".idsec", "cache", "keyring") +} + +// fileLogPath mirrors the SDK's file-log resolution (pkg/common/idsec_logger.go +// resolveFileLogWriter): the IDSEC_FILE_LOG_PATH override, else a default under +// os.UserHomeDir. The SDK MkdirAll's this path's parent. +func fileLogPath() string { + if p := strings.TrimSpace(os.Getenv("IDSEC_FILE_LOG_PATH")); p != "" { + return p + } + home, err := os.UserHomeDir() + if err != nil { + return "" + } + return filepath.Join(home, ".idsec", "logs", "idsec.log") +} + +// assertUnder reports a failure unless got is root itself or below it. +func assertUnder(t TB, what, got, root string) { + t.Helper() + + cleanGot := filepath.Clean(got) + cleanRoot := filepath.Clean(root) + if cleanGot == cleanRoot || strings.HasPrefix(cleanGot, cleanRoot+string(filepath.Separator)) { + return + } + t.Errorf("%s = %q, which is outside the test sandbox %q; the suite would touch real user state", what, got, root) +} diff --git a/internal/testenv/testenv_test.go b/internal/testenv/testenv_test.go new file mode 100644 index 0000000..76df230 --- /dev/null +++ b/internal/testenv/testenv_test.go @@ -0,0 +1,583 @@ +package testenv + +import ( + "fmt" + "os" + "path/filepath" + "runtime" + "slices" + "strings" + "testing" +) + +// recordingTB implements TB and records the failures reported to it, so the +// assertion helpers can be tested without failing the enclosing test. +type recordingTB struct { + helperCalls int + errs []string +} + +func (r *recordingTB) Helper() { r.helperCalls++ } + +func (r *recordingTB) Errorf(format string, args ...any) { + // Formatted, not the raw format string: the resolver name is an *argument* + // of assertUnder's message, so only the formatted text says WHICH assertion + // failed — and naming the failing assertion is what makes each block in + // AssertSandboxed individually killable. + r.errs = append(r.errs, fmt.Sprintf(format, args...)) +} + +// assertFailedResolvers requires exactly len(want) recorded failures, one +// matching each want substring. The COUNT is the load-bearing half: a bare +// "at least one failure" is satisfied by any *other* assertion inside +// AssertSandboxed, so the block under test could be deleted outright and the +// test would still pass. +func assertFailedResolvers(t *testing.T, rec *recordingTB, want ...string) { + t.Helper() + + if len(rec.errs) != len(want) { + t.Errorf("AssertSandboxed reported %d failures, want exactly %d (%v): %v", + len(rec.errs), len(want), want, rec.errs) + return + } + for _, w := range want { + if !slices.ContainsFunc(rec.errs, func(e string) bool { return strings.Contains(e, w) }) { + t.Errorf("no reported failure mentions %q; got %v", w, rec.errs) + } + } +} + +// wantRedirectedVars is an explicit literal, deliberately NOT derived from +// redirectedVars. The previous version of this test ranged over redirectedVars +// itself, so deleting an entry merely checked one fewer thing; a drop-one +// mutation on four of the five entries then passed the whole suite. +var wantRedirectedVars = []string{ + "HOME", + "USERPROFILE", + "XDG_CONFIG_HOME", + "IDSEC_PROFILES_FOLDER", + "GRANT_CONFIG", + "IDSEC_KEYRING_FOLDER", + "IDSEC_FILE_LOG_PATH", + "IDSEC_BASIC_KEYRING", +} + +// TestRedirectedVarsIsExactlyTheExpectedSet pins the membership of +// redirectedVars. Adding a variable to the production list without adding it +// here is a deliberate speed bump: the new entry needs a hostile-value case in +// TestRun_OverridesPreExistingHostileValues too. +// +// Not parallel: kept serial with the rest of the file. +func TestRedirectedVarsIsExactlyTheExpectedSet(t *testing.T) { + if !slices.Equal(redirectedVars, wantRedirectedVars) { + t.Errorf("redirectedVars = %q, want exactly %q", redirectedVars, wantRedirectedVars) + } +} + +// wantUnsetVars pins the unset list for the same reason wantRedirectedVars +// pins the redirect list: an explicit literal, deliberately not derived from +// the production slice. +var wantUnsetVars = []string{ + "IDSEC_PROFILE", + "DEPLOY_ENV", +} + +// Not parallel: kept serial with the rest of the file. +func TestUnsetVarsIsExactlyTheExpectedSet(t *testing.T) { + if !slices.Equal(unsetVars, wantUnsetVars) { + t.Errorf("unsetVars = %q, want exactly %q", unsetVars, wantUnsetVars) + } +} + +// TestRun_UnsetsSDKBehaviorVarsAndRestoresThem covers both halves of the +// contract: inside Run the variables must be absent (not empty — the SDK's own +// checks distinguish the two), and afterwards each must return to its exact +// prior state, including "was never set". +// +// Not parallel: mutates process-wide environment variables. +func TestRun_UnsetsSDKBehaviorVarsAndRestoresThem(t *testing.T) { + // One var pre-set, one deliberately unset, so a restore that writes "" for + // an originally-absent var is caught. + const preset = "developer-profile" + t.Setenv("IDSEC_PROFILE", preset) + + if err := os.Unsetenv("DEPLOY_ENV"); err != nil { + t.Fatalf("Unsetenv: %v", err) + } + t.Cleanup(func() { _ = os.Unsetenv("DEPLOY_ENV") }) + + inside := map[string]bool{} // var -> was present inside Run + Run(func() int { + for _, k := range wantUnsetVars { + _, ok := os.LookupEnv(k) + inside[k] = ok + } + return 0 + }) + + for _, k := range wantUnsetVars { + if inside[k] { + t.Errorf("%s was still set inside Run; it must be unset, not redirected", k) + } + } + + if got, ok := os.LookupEnv("IDSEC_PROFILE"); !ok || got != preset { + t.Errorf("IDSEC_PROFILE = %q (set=%v) after Run, want the pre-existing %q", got, ok, preset) + } + if v, ok := os.LookupEnv("DEPLOY_ENV"); ok { + t.Errorf("DEPLOY_ENV is set to %q after Run; an originally-unset var must stay unset", v) + } +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_RedirectsAllHomeEnvVars(t *testing.T) { + var ( + gotRoot string + seen = map[string]string{} + ) + + code := Run(func() int { + gotRoot = Root() + for _, k := range wantRedirectedVars { + seen[k] = os.Getenv(k) + } + return 7 + }) + + if code != 7 { + t.Errorf("Run returned %d, want the code from run()", code) + } + if gotRoot == "" { + t.Fatal("Root() was empty inside run()") + } + + for _, k := range wantRedirectedVars { + v := seen[k] + if v == "" { + t.Errorf("%s was empty inside run(); every redirected var must be set", k) + continue + } + if nonPathVars[k] { + continue // a mode switch, not a path: nothing to locate under the root + } + if !strings.HasPrefix(v, gotRoot) { + t.Errorf("%s = %q, want a path under the sandbox root %q", k, v, gotRoot) + } + } +} + +// TestRun_OverridesPreExistingHostileValues is the case that gives each entry +// in redirectedVars its own failing signal. Every var is pre-set to a path +// outside any sandbox before Run — exactly the state a developer or CI runner +// with the variable already exported is in — and each must come back +// overridden. With HOME redirected the other vars' *fallbacks* already land +// in-sandbox, so a pre-existing value is the only thing that distinguishes a +// redirected var from an unredirected one. +// +// Not parallel: mutates process-wide environment variables. +func TestRun_OverridesPreExistingHostileValues(t *testing.T) { + hostile := t.TempDir() // outside any sandbox root, by construction + + for _, k := range wantRedirectedVars { + t.Setenv(k, filepath.Join(hostile, strings.ToLower(k))) + } + + seen := map[string]string{} + var gotRoot string + Run(func() int { + gotRoot = Root() + for _, k := range wantRedirectedVars { + seen[k] = os.Getenv(k) + } + return 0 + }) + + if gotRoot == "" { + t.Fatal("Root() was empty inside run()") + } + + for _, k := range wantRedirectedVars { + got := seen[k] + if strings.HasPrefix(got, hostile) { + t.Errorf("%s = %q inside the sandbox; Run did not override the pre-existing value", k, got) + continue + } + if nonPathVars[k] { + if got == "" { + t.Errorf("%s was empty inside the sandbox, want a non-empty override", k) + } + continue + } + if !strings.HasPrefix(got, gotRoot) { + t.Errorf("%s = %q, want a path under the sandbox root %q", k, got, gotRoot) + } + } +} + +// TestAssertSandboxed_FailsOnAHostileKeyringFolder is the concrete escape the +// package comment used to deny: IDSEC_KEYRING_FOLDER overrides the file-keyring +// folder outright, bypassing the HOME fallback, so before it joined +// redirectedVars the SDK created a keyring folder outside the sandbox while +// AssertSandboxed reported zero failures. +// +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOnAHostileKeyringFolder(t *testing.T) { + escapee := t.TempDir() + + Run(func() int { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv("IDSEC_KEYRING_FOLDER", escapee); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 1 { + t.Errorf("AssertSandboxed reported %d failures, want exactly 1 (the escaped keyring folder): %v", + len(rec.errs), rec.errs) + } + return 0 + }) +} + +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOnAHostileFileLogPath(t *testing.T) { + escapee := filepath.Join(t.TempDir(), "idsec.log") + + Run(func() int { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv("IDSEC_FILE_LOG_PATH", escapee); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 1 { + t.Errorf("AssertSandboxed reported %d failures, want exactly 1 (the escaped file log path): %v", + len(rec.errs), rec.errs) + } + return 0 + }) +} + +// TestAssertSandboxed_FailsOnAHostileConfigPath is what makes the +// config.ConfigPath() block in AssertSandboxed load-bearing. GRANT_CONFIG +// overrides config.ConfigPath outright, bypassing ConfigDir, so it is the only +// input that fails that block and nothing else: deleting the block makes this +// test see zero failures. +// +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOnAHostileConfigPath(t *testing.T) { + escapee := filepath.Join(t.TempDir(), "config.yaml") + + Run(func() int { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv("GRANT_CONFIG", escapee); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + assertFailedResolvers(t, rec, "config.ConfigPath()") + return 0 + }) +} + +// TestAssertSandboxed_FailsOnAHostileHome is what makes the config.ConfigDir() +// and cache.CacheDir() blocks load-bearing. Both resolve through +// os.UserHomeDir (cache.CacheDir delegates to config.ConfigDir), and every +// other resolver has an explicit env override that still points in-sandbox, so +// an escaped home fails exactly those two. Deleting either block drops the +// count to one and this test fails. +// +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOnAHostileHome(t *testing.T) { + escapee := t.TempDir() + + Run(func() int { + // Both variables: POSIX os.UserHomeDir reads HOME, the Windows one + // reads USERPROFILE. Setting both makes the case real on either leg + // instead of skipping half the matrix. + for _, k := range []string{"HOME", "USERPROFILE"} { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv(k, escapee); err != nil { + t.Errorf("Setenv %s: %v", k, err) + return 1 + } + } + rec := &recordingTB{} + AssertSandboxed(rec) + assertFailedResolvers(t, rec, "config.ConfigDir()", "cache.CacheDir()") + return 0 + }) +} + +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsWhenBasicKeyringIsNotForced(t *testing.T) { + Run(func() int { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv("IDSEC_BASIC_KEYRING", ""); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 1 { + t.Errorf("AssertSandboxed reported %d failures, want exactly 1 (basic keyring not forced): %v", + len(rec.errs), rec.errs) + } + return 0 + }) +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_RestoresPreviousEnvironment(t *testing.T) { + const sentinel = "/sentinel-home-value" + t.Setenv("HOME", sentinel) + // GRANT_CONFIG is deliberately left unset so we can prove an unset var + // stays unset rather than being restored as an empty string. + if err := os.Unsetenv("GRANT_CONFIG"); err != nil { + t.Fatalf("Unsetenv: %v", err) + } + t.Cleanup(func() { _ = os.Unsetenv("GRANT_CONFIG") }) + + Run(func() int { return 0 }) + + if got := os.Getenv("HOME"); got != sentinel { + t.Errorf("HOME = %q after Run, want the pre-existing value %q", got, sentinel) + } + if v, ok := os.LookupEnv("GRANT_CONFIG"); ok { + t.Errorf("GRANT_CONFIG is set to %q after Run; an originally-unset var must stay unset", v) + } +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_RemovesSandboxRootAfterwards(t *testing.T) { + var root string + Run(func() int { + root = Root() + if _, err := os.Stat(root); err != nil { + t.Errorf("sandbox root %q does not exist during run(): %v", root, err) + } + return 0 + }) + + if _, err := os.Stat(root); !os.IsNotExist(err) { + t.Errorf("sandbox root %q still exists after Run (stat err = %v)", root, err) + } + if Root() != "" { + t.Errorf("Root() = %q after Run, want empty", Root()) + } +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_HomeVarsPointAtARealDirectory(t *testing.T) { + Run(func() int { + home, err := os.UserHomeDir() + if err != nil { + t.Errorf("os.UserHomeDir() inside sandbox: %v", err) + return 1 + } + if !strings.HasPrefix(home, Root()) { + t.Errorf("os.UserHomeDir() = %q, want a path under sandbox root %q", home, Root()) + } + info, err := os.Stat(home) + if err != nil { + t.Errorf("sandbox home %q not created: %v", home, err) + return 1 + } + if !info.IsDir() { + t.Errorf("sandbox home %q is not a directory", home) + } + return 0 + }) +} + +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_PassesInsideSandbox(t *testing.T) { + Run(func() int { + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 0 { + t.Errorf("AssertSandboxed reported %d failures inside the sandbox: %v", len(rec.errs), rec.errs) + } + if rec.helperCalls == 0 { + t.Error("AssertSandboxed did not call Helper()") + } + return 0 + }) +} + +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOutsideSandbox(t *testing.T) { + if runtime.GOOS == "windows" { + // The resolvers read USERPROFILE (os.UserHomeDir) and HOME (the SDK + // profile loader) from different variables on Windows; forcing a + // deterministic "outside" state needs both, and t.Setenv of HOME on + // Windows has no effect on os.UserHomeDir. Covered on POSIX instead. + t.Skip("environment-driven resolvers diverge on Windows; covered on POSIX") + } + + outside := t.TempDir() + t.Setenv("HOME", outside) + t.Setenv("GRANT_CONFIG", filepath.Join(outside, "config.yaml")) + t.Setenv("IDSEC_PROFILES_FOLDER", filepath.Join(outside, "profiles")) + + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) == 0 { + t.Error("AssertSandboxed reported no failures while running outside any sandbox") + } +} + +// TestAssertSandboxed_FailsWhenAResolverEscapes is the case that actually +// matters: a sandbox IS active, but one resolver still points at real user +// state. Without this, the outside-the-sandbox test above would be satisfied +// solely by the Root()=="" short-circuit. +// +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsWhenAResolverEscapes(t *testing.T) { + escapee := t.TempDir() // deliberately NOT under the sandbox root + + Run(func() int { + // os.Setenv, not t.Setenv: t.Setenv's cleanup fires when the test + // ends, which is *after* Run has already restored the environment — + // it would put this bogus value back and leak it into later tests. + // Run's own restore covers us here. + //nolint:usetesting // see comment above + if err := os.Setenv("IDSEC_PROFILES_FOLDER", escapee); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 1 { + t.Errorf("AssertSandboxed reported %d failures, want exactly 1 (the escaped profiles folder): %v", + len(rec.errs), rec.errs) + } + return 0 + }) +} + +// Not parallel: reads no globals, but kept serial with the rest of the file. +func TestAssertUnder(t *testing.T) { + root := filepath.Join(string(filepath.Separator), "sandbox", "root") + + tests := []struct { + name string + got string + wantFail bool + }{ + {name: "root itself", got: root}, + {name: "direct child", got: filepath.Join(root, "home")}, + {name: "deep descendant", got: filepath.Join(root, "home", ".grant", "cache")}, + {name: "unclean but inside", got: filepath.Join(root, "home", "..", "home", "x")}, + { + // The classic prefix-matching bug: "/sandbox/rootless" shares a + // string prefix with "/sandbox/root" but is not under it. + name: "sibling sharing a string prefix", + got: root + "less", + wantFail: true, + }, + {name: "unrelated absolute path", got: filepath.Join(string(filepath.Separator), "home", "tim", ".grant"), wantFail: true}, + {name: "parent of root", got: filepath.Dir(root), wantFail: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec := &recordingTB{} + assertUnder(rec, "resolver()", tt.got, root) + if gotFail := len(rec.errs) > 0; gotFail != tt.wantFail { + t.Errorf("assertUnder(%q, %q) failed = %v, want %v", tt.got, root, gotFail, tt.wantFail) + } + }) + } +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_IsReentrant(t *testing.T) { + // Sequential calls must each get their own root. Nesting is covered + // separately by TestRun_NestsWithoutClobberingTheOuterRoot. + var first, second string + Run(func() int { + first = Root() + return 0 + }) + Run(func() int { + second = Root() + return 0 + }) + if first == "" || second == "" { + t.Fatal("Root() empty in one of the runs") + } + if first == second { + t.Errorf("both runs used the same sandbox root %q; each run must be isolated", first) + } +} + +// TestRun_NestsWithoutClobberingTheOuterRoot pins the save/restore of +// sandboxRoot. Before it, an inner Run cleared the global on the way out and +// the outer run continued with Root() == "", which makes AssertSandboxed report +// "no sandbox is active" even though one is. +// +// Not parallel: mutates process-wide environment variables. +func TestRun_NestsWithoutClobberingTheOuterRoot(t *testing.T) { + var outer, inner, afterInner string + + Run(func() int { + outer = Root() + Run(func() int { + inner = Root() + return 0 + }) + afterInner = Root() + return 0 + }) + + if outer == "" || inner == "" { + t.Fatalf("Root() empty: outer=%q inner=%q", outer, inner) + } + if outer == inner { + t.Errorf("nested Run reused the outer root %q; each run must get its own", outer) + } + if afterInner != outer { + t.Errorf("Root() = %q after the nested Run returned, want the outer root %q", afterInner, outer) + } + if Root() != "" { + t.Errorf("Root() = %q after both runs, want empty", Root()) + } +} + +// TestRun_RestoresEnvironmentAfterAPanic pins the deferred cleanup. Without it +// a panic inside run — a -race detection, or any stray panic — skipped both the +// environment restore and the sandbox removal, so the process kept running with +// a redirected HOME pointing at a directory that still existed. +// +// Not parallel: mutates process-wide environment variables. +func TestRun_RestoresEnvironmentAfterAPanic(t *testing.T) { + const sentinel = "/sentinel-home-value" + t.Setenv("HOME", sentinel) + + var root string + func() { + defer func() { + if recover() == nil { + t.Error("expected the panic to propagate out of Run") + } + }() + Run(func() int { + root = Root() + panic("boom") + }) + }() + + if got := os.Getenv("HOME"); got != sentinel { + t.Errorf("HOME = %q after a panicking Run, want the pre-existing value %q", got, sentinel) + } + if Root() != "" { + t.Errorf("Root() = %q after a panicking Run, want empty", Root()) + } + if _, err := os.Stat(root); !os.IsNotExist(err) { + t.Errorf("sandbox root %q survived a panicking Run (stat err = %v)", root, err) + } +} diff --git a/internal/workflows/main_test.go b/internal/workflows/main_test.go new file mode 100644 index 0000000..bb1913f --- /dev/null +++ b/internal/workflows/main_test.go @@ -0,0 +1,25 @@ +package workflows + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory. This package +// drives the *real* AccessRequestService constructor (see retry_policy_test.go), +// which builds an SDK client and can therefore reach the SDK's profile and +// keyring resolution; without the sandbox a new test here could read or write +// the developer's real ~/.idsec with no failing signal. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, this fails instead of the suite silently touching real user state. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +}