From 33c4dd4eb5f26172582c3f0b1cdd503fbae0399f Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 5 May 2026 23:54:42 -0700 Subject: [PATCH 01/10] auth: support headless token storage --- README.md | 27 +++ cmd/entire/cli/auth.go | 104 ++++++-- cmd/entire/cli/auth/env.go | 35 +++ cmd/entire/cli/auth/file_store.go | 188 +++++++++++++++ cmd/entire/cli/auth/store.go | 56 ++++- cmd/entire/cli/auth/store_test.go | 228 +++++++++++++++++- cmd/entire/cli/auth_test.go | 144 +++++++++++ cmd/entire/cli/integration_test/login_test.go | 92 ++++++- cmd/entire/cli/login.go | 18 +- cmd/entire/cli/login_test.go | 83 ++++++- cmd/entire/cli/logout.go | 13 +- cmd/entire/cli/logout_test.go | 51 ++++ 12 files changed, 1002 insertions(+), 37 deletions(-) create mode 100644 cmd/entire/cli/auth/env.go create mode 100644 cmd/entire/cli/auth/file_store.go diff --git a/README.md b/README.md index 2fa134a330..129722e327 100644 --- a/README.md +++ b/README.md @@ -346,6 +346,33 @@ Personal overrides, gitignored by default: | `strategy_options.summarize.enabled` | `true`, `false` | Auto-generate AI summaries at commit time | | `telemetry` | `true`, `false` | Send anonymous usage statistics to Posthog | +### Authentication Token Storage + +By default, `entire login` stores the CLI API token in your operating system keyring. In headless environments, containers, and CI jobs where a keyring is unavailable, use one of these explicit alternatives: + +| Environment Variable | Use Case | Behavior | +| -------------------- | -------- | -------- | +| `ENTIRE_AUTH_TOKEN` | CI and ephemeral shells | Highest-priority read-only CLI API token. Entire uses it for authenticated API requests and never writes, deletes, or rotates it. | +| `ENTIRE_SECRETS_PATH` | Headless machines that need persistent login | Absolute path to a JSON token file. `entire login` writes this file with `0600` permissions, and `entire logout` removes the token from this file. | + +For CI, set `ENTIRE_AUTH_TOKEN` directly: + +```bash +export ENTIRE_AUTH_TOKEN=entire_api_token +entire auth status +``` + +For a remote container or server where `entire login` cannot save to the OS keyring, set `ENTIRE_SECRETS_PATH` to an absolute path before logging in: + +```bash +export ENTIRE_SECRETS_PATH="$HOME/.config/entire/credentials.json" +entire login +``` + +The token file is plaintext JSON protected by local filesystem permissions. Keep it outside your repository, do not commit it, and choose a path only your user can read. `ENTIRE_AUTH_TOKEN` takes precedence over tokens stored in `ENTIRE_SECRETS_PATH`, and the file store takes precedence over the OS keyring. + +`ENTIRE_CHECKPOINT_TOKEN` is separate from CLI API authentication. It is only used for checkpoint Git fetch and push operations. + ### Agent Hook Configuration Each agent stores its hook configuration in its own directory. When you run `entire enable`, hooks are installed in the appropriate location for each selected agent: diff --git a/cmd/entire/cli/auth.go b/cmd/entire/cli/auth.go index 0a441aef07..f7228f0ed0 100644 --- a/cmd/entire/cli/auth.go +++ b/cmd/entire/cli/auth.go @@ -96,32 +96,54 @@ func defaultListTokens(ctx context.Context, token string) ([]api.Token, error) { } func runAuthStatus(ctx context.Context, w io.Writer, store tokenStore, list authTokenLister, baseURL string) error { - token, err := store.GetToken(baseURL) + info, err := store.GetTokenInfo(baseURL) if err != nil { - return fmt.Errorf("read keychain: %w", err) + return fmt.Errorf("read auth token: %w", err) } - if token == "" { + if info.Value == "" { fmt.Fprintf(w, "Not logged in to %s\n", baseURL) fmt.Fprintln(w, "Run 'entire login' to authenticate.") return nil } - tokens, err := list(ctx, token) + tokens, err := list(ctx, info.Value) if err != nil { if api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { - fmt.Fprintf(w, "Token in keychain for %s is no longer valid.\n", baseURL) - fmt.Fprintln(w, "Run 'entire login' to re-authenticate.") + fmt.Fprintf(w, "Token for %s is no longer valid.\n", baseURL) + if info.Source == auth.TokenSourceEnv { + fmt.Fprintf(w, "Update or unset %s to re-authenticate.\n", auth.AuthTokenEnvVar) + } else { + fmt.Fprintln(w, "Run 'entire login' to re-authenticate.") + } return nil } return fmt.Errorf("validate token: %w", err) } fmt.Fprintf(w, "Logged in to %s\n", baseURL) - fmt.Fprintln(w, " Token: stored in OS keychain") + fmt.Fprintf(w, " Token: %s\n", describeTokenSource(info)) fmt.Fprintf(w, " Active tokens on this account: %d\n", len(tokens)) return nil } +func describeTokenSource(info auth.TokenInfo) string { + switch info.Source { + case auth.TokenSourceNone: + return "not configured" + case auth.TokenSourceEnv: + return "supplied by " + auth.AuthTokenEnvVar + case auth.TokenSourceFile: + if info.Path != "" { + return "stored in file " + info.Path + } + return "stored in file" + case auth.TokenSourceKeyring: + return "stored in OS keychain" + default: + return "stored" + } +} + // --- list ------------------------------------------------------------------- func newAuthListCmd() *cobra.Command { @@ -146,7 +168,7 @@ func newAuthListCmd() *cobra.Command { func runAuthList(ctx context.Context, w io.Writer, store tokenStore, list authTokenLister, baseURL string, jsonOut bool) error { token, err := store.GetToken(baseURL) if err != nil { - return fmt.Errorf("read keychain: %w", err) + return fmt.Errorf("read auth token: %w", err) } if token == "" { return fmt.Errorf("not logged in to %s; run 'entire login' first", baseURL) @@ -391,7 +413,7 @@ func newAuthRevokeCmd() *cobra.Command { cmd := &cobra.Command{ Use: "revoke [id]", Short: "Revoke an API token by id", - Long: "Revoke a specific API token. Use --current to revoke the token used by this CLI (equivalent to 'entire logout').", + Long: "Revoke a specific API token. Use --current to revoke the token used by this CLI.", Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { id := "" @@ -412,7 +434,7 @@ func newAuthRevokeCmd() *cobra.Command { api.BaseURL(), id, revokeCurrent) }, } - cmd.Flags().BoolVar(&revokeCurrent, "current", false, "Revoke the token used by this CLI and remove the local copy") + cmd.Flags().BoolVar(&revokeCurrent, "current", false, "Revoke the token used by this CLI and remove the local copy when stored locally") addInsecureHTTPAuthFlag(cmd, &insecureHTTPAuth) return cmd } @@ -431,35 +453,79 @@ func runAuthRevoke( baseURL, id string, current bool, ) error { - token, err := store.GetToken(baseURL) + info, err := store.GetTokenInfo(baseURL) if err != nil { - return fmt.Errorf("read keychain: %w", err) + return fmt.Errorf("read auth token: %w", err) } - if token == "" { + if info.Value == "" { return fmt.Errorf("not logged in to %s; run 'entire login' first", baseURL) } if current { - // Revoking our own token is just logout — reuse that path so behavior - // stays identical (best-effort revoke + local delete). - return runLogout(ctx, outW, errW, store, revokeCurrent, baseURL) + return revokeCurrentToken(ctx, outW, errW, store, revokeCurrent, baseURL, info) } - if err := revokeByID(ctx, token, id); err != nil { + if err := revokeByID(ctx, info.Value, id); err != nil { return err } // The list endpoint requires bearer auth, so a 401 here means the id we // just revoked was the same one this CLI is using — the keychain entry is // now stale and would otherwise produce confusing 401s on every command. - if _, listErr := list(ctx, token); listErr != nil && api.IsHTTPErrorStatus(listErr, http.StatusUnauthorized) { + if _, listErr := list(ctx, info.Value); listErr != nil && api.IsHTTPErrorStatus(listErr, http.StatusUnauthorized) { + if info.Source == auth.TokenSourceEnv { + fmt.Fprintf(outW, "Revoked token %s (this was supplied by %s; unset it to stop using it locally).\n", id, auth.AuthTokenEnvVar) + return nil + } if delErr := store.DeleteToken(baseURL); delErr != nil { return fmt.Errorf("revoked token %s but failed to remove local copy: %w", id, delErr) } - fmt.Fprintf(outW, "Revoked token %s (this was your local token; removed from keychain).\n", id) + fmt.Fprintf(outW, "Revoked token %s (this was your local token; removed from %s).\n", id, localTokenSourceName(info)) return nil } fmt.Fprintf(outW, "Revoked token %s.\n", id) return nil } + +func revokeCurrentToken( + ctx context.Context, + outW, errW io.Writer, + store tokenStore, + revoke revokeCurrentFunc, + baseURL string, + info auth.TokenInfo, +) error { + if info.Source == auth.TokenSourceEnv { + if err := revoke(ctx, info.Value); err != nil { + return fmt.Errorf("revoke current token: %w", err) + } + fmt.Fprintf(outW, "Revoked current token supplied by %s. Unset it to stop using it locally.\n", auth.AuthTokenEnvVar) + return nil + } + + if err := revoke(ctx, info.Value); err != nil && !api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { + fmt.Fprintf(errW, "Warning: server-side token revocation failed: %v\n", err) + } + if err := store.DeleteToken(baseURL); err != nil { + return fmt.Errorf("remove auth token: %w", err) + } + + fmt.Fprintln(outW, "Logged out.") + return nil +} + +func localTokenSourceName(info auth.TokenInfo) string { + switch info.Source { + case auth.TokenSourceFile: + return "file" + case auth.TokenSourceKeyring: + return "keychain" + case auth.TokenSourceEnv: + return auth.AuthTokenEnvVar + case auth.TokenSourceNone: + return "local storage" + default: + return "local storage" + } +} diff --git a/cmd/entire/cli/auth/env.go b/cmd/entire/cli/auth/env.go new file mode 100644 index 0000000000..f06048920b --- /dev/null +++ b/cmd/entire/cli/auth/env.go @@ -0,0 +1,35 @@ +package auth + +import ( + "os" + "strings" +) + +const ( + // AuthTokenEnvVar provides a read-only bearer token for non-interactive CLI auth. + AuthTokenEnvVar = "ENTIRE_AUTH_TOKEN" // #nosec G101 -- this is an environment variable name, not a credential. + + // SecretsPathEnvVar points to an opt-in file-backed token store. + SecretsPathEnvVar = "ENTIRE_SECRETS_PATH" +) + +// TokenSource identifies where the active auth token came from. +type TokenSource string + +const ( + TokenSourceNone TokenSource = "" + TokenSourceEnv TokenSource = "env" + TokenSourceFile TokenSource = "file" + TokenSourceKeyring TokenSource = "keyring" +) + +// TokenInfo is a source-aware auth token lookup result. +type TokenInfo struct { + Value string + Source TokenSource + Path string +} + +func envAuthToken() string { + return strings.TrimSpace(os.Getenv(AuthTokenEnvVar)) +} diff --git a/cmd/entire/cli/auth/file_store.go b/cmd/entire/cli/auth/file_store.go new file mode 100644 index 0000000000..afeb75773b --- /dev/null +++ b/cmd/entire/cli/auth/file_store.go @@ -0,0 +1,188 @@ +package auth + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" +) + +const fileStoreVersion = 1 + +type fileStoreData struct { + Version int `json:"version"` + Tokens map[string]string `json:"tokens"` +} + +func configuredSecretsPath() (string, bool, error) { + raw := strings.TrimSpace(os.Getenv(SecretsPathEnvVar)) + if raw == "" { + return "", false, nil + } + + path, err := expandHome(raw) + if err != nil { + return "", false, err + } + path = filepath.Clean(path) + if !filepath.IsAbs(path) { + return "", false, fmt.Errorf("%s must be an absolute path after ~/ expansion", SecretsPathEnvVar) + } + + return path, true, nil +} + +func expandHome(path string) (string, error) { + if path == "~" { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("resolve home directory: %w", err) + } + return home, nil + } + if strings.HasPrefix(path, "~/") { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("resolve home directory: %w", err) + } + return filepath.Join(home, strings.TrimPrefix(path, "~/")), nil + } + return path, nil +} + +func readFileStore(path string) (fileStoreData, error) { + info, err := os.Stat(path) + if os.IsNotExist(err) { + return newFileStoreData(), nil + } + if err != nil { + return fileStoreData{}, fmt.Errorf("stat token file: %w", err) + } + if info.IsDir() { + return fileStoreData{}, fmt.Errorf("token file path is a directory: %s", path) + } + if runtime.GOOS != "windows" && info.Mode().Perm()&0o077 != 0 { + return fileStoreData{}, fmt.Errorf("token file %s has unsafe permissions %o; run chmod 600 %s", path, info.Mode().Perm(), path) + } + + data, err := os.ReadFile(path) //nolint:gosec // path is explicit user-provided credential path. + if err != nil { + return fileStoreData{}, fmt.Errorf("read token file: %w", err) + } + if len(strings.TrimSpace(string(data))) == 0 { + return newFileStoreData(), nil + } + + var store fileStoreData + if err := json.Unmarshal(data, &store); err != nil { + return fileStoreData{}, fmt.Errorf("parse token file: %w", err) + } + if store.Version == 0 { + store.Version = fileStoreVersion + } + if store.Version != fileStoreVersion { + return fileStoreData{}, fmt.Errorf("unsupported token file version %d", store.Version) + } + if store.Tokens == nil { + store.Tokens = make(map[string]string) + } + + return store, nil +} + +func writeFileStore(path string, store fileStoreData) error { + if store.Version == 0 { + store.Version = fileStoreVersion + } + if store.Tokens == nil { + store.Tokens = make(map[string]string) + } + + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { + return fmt.Errorf("create token file directory: %w", err) + } + + temp, err := os.CreateTemp(dir, "."+filepath.Base(path)+".tmp-*") + if err != nil { + return fmt.Errorf("create temporary token file: %w", err) + } + tempName := temp.Name() + defer func() { _ = os.Remove(tempName) }() + + if err := temp.Chmod(0o600); err != nil { + _ = temp.Close() + return fmt.Errorf("set temporary token file permissions: %w", err) + } + + enc := json.NewEncoder(temp) + enc.SetIndent("", " ") + if err := enc.Encode(store); err != nil { + _ = temp.Close() + return fmt.Errorf("write token file JSON: %w", err) + } + if err := temp.Close(); err != nil { + return fmt.Errorf("close temporary token file: %w", err) + } + + if err := os.Rename(tempName, path); err != nil { + return fmt.Errorf("replace token file: %w", err) + } + if runtime.GOOS != "windows" { + if err := os.Chmod(path, 0o600); err != nil { + return fmt.Errorf("set token file permissions: %w", err) + } + } + + return nil +} + +func newFileStoreData() fileStoreData { + return fileStoreData{ + Version: fileStoreVersion, + Tokens: make(map[string]string), + } +} + +func getFileToken(path, baseURL string) (string, error) { + store, err := readFileStore(path) + if err != nil { + return "", err + } + return store.Tokens[baseURL], nil +} + +func saveFileToken(path, baseURL, token string) error { + store, err := readFileStore(path) + if err != nil { + return err + } + store.Tokens[baseURL] = token + if err := writeFileStore(path, store); err != nil { + return err + } + return nil +} + +func deleteFileToken(path, baseURL string) error { + if _, err := os.Stat(path); os.IsNotExist(err) { + return nil + } else if err != nil { + return fmt.Errorf("stat token file: %w", err) + } + + store, err := readFileStore(path) + if err != nil { + return err + } + if _, ok := store.Tokens[baseURL]; !ok { + return nil + } + delete(store.Tokens, baseURL) + if err := writeFileStore(path, store); err != nil { + return err + } + return nil +} diff --git a/cmd/entire/cli/auth/store.go b/cmd/entire/cli/auth/store.go index ee39eef926..0bd03cab95 100644 --- a/cmd/entire/cli/auth/store.go +++ b/cmd/entire/cli/auth/store.go @@ -11,12 +11,12 @@ import ( const keyringService = "entire-cli" -// Store manages CLI authentication tokens in the OS keyring. +// Store manages CLI authentication tokens across the configured auth sources. type Store struct { service string } -// NewStore returns a Store backed by the system keyring. +// NewStore returns a Store using the default keyring service when no file store is configured. func NewStore() *Store { return &Store{service: keyringService} } @@ -33,6 +33,15 @@ func (s *Store) SaveToken(baseURL, token string) error { return errors.New("refusing to save empty token") } + if path, ok, err := configuredSecretsPath(); err != nil { + return err + } else if ok { + if err := saveFileToken(path, baseURL, token); err != nil { + return fmt.Errorf("save token to file: %w", err) + } + return nil + } + if err := keyring.Set(s.service, baseURL, token); err != nil { return fmt.Errorf("save token to keyring: %w", err) } @@ -43,19 +52,56 @@ func (s *Store) SaveToken(baseURL, token string) error { // GetToken retrieves a stored token for the given base URL. // Returns an empty string (and no error) if no token is stored. func (s *Store) GetToken(baseURL string) (string, error) { + info, err := s.GetTokenInfo(baseURL) + if err != nil { + return "", err + } + + return info.Value, nil +} + +// GetTokenInfo retrieves a stored token and reports where it came from. +// Returns an empty TokenInfo (and no error) if no token is stored. +func (s *Store) GetTokenInfo(baseURL string) (TokenInfo, error) { + if token := envAuthToken(); token != "" { + return TokenInfo{Value: token, Source: TokenSourceEnv}, nil + } + + if path, ok, err := configuredSecretsPath(); err != nil { + return TokenInfo{}, err + } else if ok { + token, err := getFileToken(path, baseURL) + if err != nil { + return TokenInfo{}, fmt.Errorf("get token from file: %w", err) + } + if token == "" { + return TokenInfo{}, nil + } + return TokenInfo{Value: token, Source: TokenSourceFile, Path: path}, nil + } + token, err := keyring.Get(s.service, baseURL) if errors.Is(err, keyring.ErrNotFound) { - return "", nil + return TokenInfo{}, nil } if err != nil { - return "", fmt.Errorf("get token from keyring: %w", err) + return TokenInfo{}, fmt.Errorf("get token from keyring: %w", err) } - return token, nil + return TokenInfo{Value: token, Source: TokenSourceKeyring}, nil } // DeleteToken removes a stored token for the given base URL. func (s *Store) DeleteToken(baseURL string) error { + if path, ok, err := configuredSecretsPath(); err != nil { + return err + } else if ok { + if err := deleteFileToken(path, baseURL); err != nil { + return fmt.Errorf("delete token from file: %w", err) + } + return nil + } + err := keyring.Delete(s.service, baseURL) if errors.Is(err, keyring.ErrNotFound) { return nil diff --git a/cmd/entire/cli/auth/store_test.go b/cmd/entire/cli/auth/store_test.go index 8ccf122372..bc3c33d95f 100644 --- a/cmd/entire/cli/auth/store_test.go +++ b/cmd/entire/cli/auth/store_test.go @@ -2,12 +2,20 @@ package auth import ( "os" + "path/filepath" + "runtime" + "strings" "testing" "github.com/entireio/cli/cmd/entire/cli/api" "github.com/zalando/go-keyring" ) +const ( + testLocalToken = "local-token" + testWindowsOS = "windows" +) + func TestMain(m *testing.M) { keyring.MockInit() os.Exit(m.Run()) @@ -43,6 +51,42 @@ func TestStoreGetToken_NotFound(t *testing.T) { } } +func TestStoreGetTokenInfo_EnvTokenTakesPrecedence(t *testing.T) { + // Not parallel: go-keyring's mock provider uses an unprotected map. + t.Setenv(AuthTokenEnvVar, " env-token ") + + store := NewStoreWithService("test-env-precedence") + if err := store.SaveToken("https://entire.io", "keyring-token"); err != nil { + t.Fatalf("SaveToken() error = %v", err) + } + + info, err := store.GetTokenInfo("https://entire.io") + if err != nil { + t.Fatalf("GetTokenInfo() error = %v", err) + } + if info.Value != "env-token" || info.Source != TokenSourceEnv { + t.Fatalf("GetTokenInfo() = %#v, want env token/source", info) + } +} + +func TestStoreGetTokenInfo_WhitespaceEnvTokenIgnored(t *testing.T) { + // Not parallel: go-keyring's mock provider uses an unprotected map. + t.Setenv(AuthTokenEnvVar, " ") + + store := NewStoreWithService("test-env-blank") + if err := store.SaveToken("https://entire.io", "keyring-token"); err != nil { + t.Fatalf("SaveToken() error = %v", err) + } + + info, err := store.GetTokenInfo("https://entire.io") + if err != nil { + t.Fatalf("GetTokenInfo() error = %v", err) + } + if info.Value != "keyring-token" || info.Source != TokenSourceKeyring { + t.Fatalf("GetTokenInfo() = %#v, want keyring token/source", info) + } +} + func TestStoreSaveToken_PreservesOtherBaseURLs(t *testing.T) { // Not parallel: go-keyring's mock provider uses an unprotected map. store := NewStoreWithService("test-preserve") @@ -51,7 +95,7 @@ func TestStoreSaveToken_PreservesOtherBaseURLs(t *testing.T) { t.Fatalf("SaveToken(prod) error = %v", err) } - if err := store.SaveToken("http://localhost:8787", "local-token"); err != nil { + if err := store.SaveToken("http://localhost:8787", testLocalToken); err != nil { t.Fatalf("SaveToken(local) error = %v", err) } @@ -67,8 +111,8 @@ func TestStoreSaveToken_PreservesOtherBaseURLs(t *testing.T) { if err != nil { t.Fatalf("GetToken(local) error = %v", err) } - if local != "local-token" { - t.Fatalf("local token = %q, want %q", local, "local-token") + if local != testLocalToken { + t.Fatalf("local token = %q, want %q", local, testLocalToken) } } @@ -132,11 +176,183 @@ func TestStoreDeleteToken_NotFoundIsNoop(t *testing.T) { } } +func TestStoreFileBackend_SaveGetDelete(t *testing.T) { + // Not parallel: auth env vars are process-global. + path := filepath.Join(t.TempDir(), "credentials.json") + t.Setenv(SecretsPathEnvVar, path) + + store := NewStoreWithService("test-file-backend") + if err := store.SaveToken("https://entire.io", "file-token"); err != nil { + t.Fatalf("SaveToken() error = %v", err) + } + + info, err := store.GetTokenInfo("https://entire.io") + if err != nil { + t.Fatalf("GetTokenInfo() error = %v", err) + } + if info.Value != "file-token" || info.Source != TokenSourceFile || info.Path != path { + t.Fatalf("GetTokenInfo() = %#v, want file token/source/path", info) + } + + if runtime.GOOS != testWindowsOS { + stat, statErr := os.Stat(path) + if statErr != nil { + t.Fatalf("Stat() error = %v", statErr) + } + if got := stat.Mode().Perm(); got != 0o600 { + t.Fatalf("credential file mode = %o, want 600", got) + } + } + + if err := store.DeleteToken("https://entire.io"); err != nil { + t.Fatalf("DeleteToken() error = %v", err) + } + got, err := store.GetToken("https://entire.io") + if err != nil { + t.Fatalf("GetToken() error = %v", err) + } + if got != "" { + t.Fatalf("GetToken() after delete = %q, want empty", got) + } +} + +func TestStoreFileBackend_PreservesOtherBaseURLs(t *testing.T) { + // Not parallel: auth env vars are process-global. + path := filepath.Join(t.TempDir(), "credentials.json") + t.Setenv(SecretsPathEnvVar, path) + + store := NewStoreWithService("test-file-preserve") + if err := store.SaveToken("https://entire.io", "prod-token"); err != nil { + t.Fatalf("SaveToken(prod) error = %v", err) + } + if err := store.SaveToken("http://localhost:8787", testLocalToken); err != nil { + t.Fatalf("SaveToken(local) error = %v", err) + } + if err := store.DeleteToken("https://entire.io"); err != nil { + t.Fatalf("DeleteToken(prod) error = %v", err) + } + + got, err := store.GetToken("http://localhost:8787") + if err != nil { + t.Fatalf("GetToken(local) error = %v", err) + } + if got != testLocalToken { + t.Fatalf("local token = %q, want %q", got, testLocalToken) + } +} + +func TestStoreFileBackend_EnvTokenTakesPrecedence(t *testing.T) { + // Not parallel: auth env vars are process-global. + path := filepath.Join(t.TempDir(), "credentials.json") + t.Setenv(SecretsPathEnvVar, path) + t.Setenv(AuthTokenEnvVar, "env-token") + + store := NewStoreWithService("test-file-env-precedence") + if err := store.SaveToken("https://entire.io", "file-token"); err != nil { + t.Fatalf("SaveToken() error = %v", err) + } + + info, err := store.GetTokenInfo("https://entire.io") + if err != nil { + t.Fatalf("GetTokenInfo() error = %v", err) + } + if info.Value != "env-token" || info.Source != TokenSourceEnv { + t.Fatalf("GetTokenInfo() = %#v, want env token/source", info) + } +} + +func TestStoreFileBackend_RejectsRelativePath(t *testing.T) { + // Not parallel: auth env vars are process-global. + t.Setenv(SecretsPathEnvVar, "credentials.json") + + store := NewStoreWithService("test-file-relative") + if _, err := store.GetTokenInfo("https://entire.io"); err == nil || !strings.Contains(err.Error(), "absolute") { + t.Fatalf("GetTokenInfo() err = %v, want absolute-path error", err) + } + if err := store.SaveToken("https://entire.io", "tok"); err == nil || !strings.Contains(err.Error(), "absolute") { + t.Fatalf("SaveToken() err = %v, want absolute-path error", err) + } + if err := store.DeleteToken("https://entire.io"); err == nil || !strings.Contains(err.Error(), "absolute") { + t.Fatalf("DeleteToken() err = %v, want absolute-path error", err) + } +} + +func TestStoreFileBackend_RejectsMalformedJSON(t *testing.T) { + // Not parallel: auth env vars are process-global. + path := filepath.Join(t.TempDir(), "credentials.json") + t.Setenv(SecretsPathEnvVar, path) + if err := os.WriteFile(path, []byte("{not-json"), 0o600); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + + store := NewStoreWithService("test-file-malformed") + if _, err := store.GetTokenInfo("https://entire.io"); err == nil || !strings.Contains(err.Error(), "parse") { + t.Fatalf("GetTokenInfo() err = %v, want parse error", err) + } +} + +func TestStoreFileBackend_RejectsGroupReadableFile(t *testing.T) { + if runtime.GOOS == testWindowsOS { + t.Skip("permission bit checks are Unix-specific") + } + // Not parallel: auth env vars are process-global. + path := filepath.Join(t.TempDir(), "credentials.json") + t.Setenv(SecretsPathEnvVar, path) + if err := os.WriteFile(path, []byte(`{"version":1,"tokens":{"https://entire.io":"tok"}}`), 0o640); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + + store := NewStoreWithService("test-file-perms") + if _, err := store.GetTokenInfo("https://entire.io"); err == nil || !strings.Contains(err.Error(), "chmod 600") { + t.Fatalf("GetTokenInfo() err = %v, want chmod 600 hint", err) + } +} + +func TestStoreFileBackend_DeleteMissingFileDoesNotCreateFile(t *testing.T) { + // Not parallel: auth env vars are process-global. + path := filepath.Join(t.TempDir(), "credentials.json") + t.Setenv(SecretsPathEnvVar, path) + + store := NewStoreWithService("test-file-delete-missing") + if err := store.DeleteToken("https://entire.io"); err != nil { + t.Fatalf("DeleteToken() error = %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("Stat() err = %v, want file to remain absent", err) + } +} + +func TestStoreFileBackend_DeleteMissingTokenDoesNotRewriteFile(t *testing.T) { + // Not parallel: auth env vars are process-global. + path := filepath.Join(t.TempDir(), "credentials.json") + t.Setenv(SecretsPathEnvVar, path) + + store := NewStoreWithService("test-file-delete-missing-token") + if err := store.SaveToken("https://entire.io", "file-token"); err != nil { + t.Fatalf("SaveToken() error = %v", err) + } + before, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile(before) error = %v", err) + } + + if err := store.DeleteToken("https://missing.example.com"); err != nil { + t.Fatalf("DeleteToken() error = %v", err) + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile(after) error = %v", err) + } + if string(after) != string(before) { + t.Fatalf("credentials file changed after deleting missing token\nbefore:\n%s\nafter:\n%s", string(before), string(after)) + } +} + func TestLookupCurrentToken(t *testing.T) { t.Setenv(api.BaseURLEnvVar, "http://localhost:8787") store := NewStore() - if err := store.SaveToken("http://localhost:8787", "local-token"); err != nil { + if err := store.SaveToken("http://localhost:8787", testLocalToken); err != nil { t.Fatalf("SaveToken() error = %v", err) } @@ -144,7 +360,7 @@ func TestLookupCurrentToken(t *testing.T) { if err != nil { t.Fatalf("LookupCurrentToken() error = %v", err) } - if got != "local-token" { - t.Fatalf("LookupCurrentToken() = %q, want %q", got, "local-token") + if got != testLocalToken { + t.Fatalf("LookupCurrentToken() = %q, want %q", got, testLocalToken) } } diff --git a/cmd/entire/cli/auth_test.go b/cmd/entire/cli/auth_test.go index 9acd3fe848..077ff9ee41 100644 --- a/cmd/entire/cli/auth_test.go +++ b/cmd/entire/cli/auth_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/entireio/cli/cmd/entire/cli/api" + "github.com/entireio/cli/cmd/entire/cli/auth" ) const ( @@ -70,6 +71,54 @@ func TestRunAuthStatus_LoggedIn(t *testing.T) { } } +func TestRunAuthStatus_SourceLabels(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + source auth.TokenSource + path string + want string + }{ + "env": { + source: auth.TokenSourceEnv, + want: "Token: supplied by " + auth.AuthTokenEnvVar, + }, + "file": { + source: auth.TokenSourceFile, + path: "/tmp/entire-credentials.json", + want: "Token: stored in file /tmp/entire-credentials.json", + }, + "keyring": { + source: auth.TokenSourceKeyring, + want: "Token: stored in OS keychain", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + store := newMockTokenStore() + store.tokens[testBaseURL] = testAuthTok + store.source = tt.source + store.path = tt.path + + list := func(context.Context, string) ([]api.Token, error) { + return []api.Token{{ID: "a", Name: "laptop"}}, nil + } + + var out bytes.Buffer + if err := runAuthStatus(context.Background(), &out, store, list, testBaseURL); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !strings.Contains(out.String(), tt.want) { + t.Fatalf("output = %q, want %q", out.String(), tt.want) + } + }) + } +} + func TestRunAuthStatus_TokenInvalid(t *testing.T) { t.Parallel() @@ -93,6 +142,30 @@ func TestRunAuthStatus_TokenInvalid(t *testing.T) { } } +func TestRunAuthStatus_EnvTokenInvalidSuggestsUpdatingEnv(t *testing.T) { + t.Parallel() + + store := newMockTokenStore() + store.tokens[testBaseURL] = testAuthTok + store.source = auth.TokenSourceEnv + + list := func(context.Context, string) ([]api.Token, error) { + return nil, &api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"} + } + + var out bytes.Buffer + if err := runAuthStatus(context.Background(), &out, store, list, testBaseURL); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !strings.Contains(out.String(), auth.AuthTokenEnvVar) { + t.Fatalf("output = %q, want env-token guidance", out.String()) + } + if strings.Contains(out.String(), "entire login") { + t.Fatalf("output = %q, should not suggest login while env token has precedence", out.String()) + } +} + func TestRunAuthStatus_ServerError(t *testing.T) { t.Parallel() @@ -408,6 +481,77 @@ func TestRunAuthRevoke_CurrentDelegatesToLogout(t *testing.T) { } } +func TestRunAuthRevoke_CurrentEnvTokenRevokesButDoesNotDelete(t *testing.T) { + t.Parallel() + + store := newMockTokenStore() + store.tokens[testBaseURL] = testAuthTok + store.source = auth.TokenSourceEnv + + revokeByIDCalled := false + revokeByID := func(context.Context, string, string) error { + revokeByIDCalled = true + return nil + } + + revokedToken := "" + revokeCurrent := func(_ context.Context, token string) error { + revokedToken = token + return nil + } + + list := func(context.Context, string) ([]api.Token, error) { return nil, nil } + + var out, errOut bytes.Buffer + err := runAuthRevoke(context.Background(), &out, &errOut, store, + list, revokeByID, revokeCurrent, testBaseURL, "", true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if revokeByIDCalled { + t.Fatal("revokeByID should not be called when --current is set") + } + if revokedToken != testAuthTok { + t.Errorf("revokeCurrent called with token %q, want %q", revokedToken, testAuthTok) + } + if store.deleted[testBaseURL] { + t.Fatal("env-token revoke should not delete lower-priority credentials") + } + if !strings.Contains(out.String(), auth.AuthTokenEnvVar) { + t.Fatalf("output = %q, want env-token guidance", out.String()) + } +} + +func TestRunAuthRevoke_ByIDEnvSelfRevokeDoesNotDeleteLowerPriority(t *testing.T) { + t.Parallel() + + store := newMockTokenStore() + store.tokens[testBaseURL] = testAuthTok + store.source = auth.TokenSourceEnv + + revokeByID := func(context.Context, string, string) error { return nil } + revokeCurrent := func(context.Context, string) error { return nil } + + list := func(context.Context, string) ([]api.Token, error) { + return nil, &api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"} + } + + var out, errOut bytes.Buffer + err := runAuthRevoke(context.Background(), &out, &errOut, store, + list, revokeByID, revokeCurrent, testBaseURL, testTokenID, false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if store.deleted[testBaseURL] { + t.Fatal("env-token self-revoke should not delete lower-priority credentials") + } + if !strings.Contains(out.String(), auth.AuthTokenEnvVar) { + t.Fatalf("output = %q, want env-token guidance", out.String()) + } +} + func TestRunAuthRevoke_NotLoggedInErrors(t *testing.T) { t.Parallel() diff --git a/cmd/entire/cli/integration_test/login_test.go b/cmd/entire/cli/integration_test/login_test.go index 23932049d9..dcfbdff93d 100644 --- a/cmd/entire/cli/integration_test/login_test.go +++ b/cmd/entire/cli/integration_test/login_test.go @@ -11,6 +11,9 @@ import ( "io" "net/http" "net/http/httptest" + "os" + "path/filepath" + "runtime" "strings" "sync" "testing" @@ -107,6 +110,92 @@ func TestLogin_SavesTokenAfterApproval(t *testing.T) { } } +func TestLogin_SavesTokenToSecretsPathAfterApproval(t *testing.T) { + t.Parallel() + + type state struct { + sync.Mutex + approved bool + } + + serverState := &state{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/oauth/device/code": + writeJSON(t, w, http.StatusOK, map[string]any{ + "device_code": "device-123", + "user_code": "ABCD-EFGH", + "verification_uri": serverURLWithPath(r, "/approve"), + "verification_uri_complete": serverURLWithPath(r, "/approve?code=ABCD-EFGH"), + "expires_in": 10, + "interval": 1, + }) + case r.Method == http.MethodPost && r.URL.Path == "/oauth/token": + serverState.Lock() + approved := serverState.approved + serverState.Unlock() + + if !approved { + writeJSON(t, w, http.StatusBadRequest, map[string]any{"error": "authorization_pending"}) + return + } + + writeJSON(t, w, http.StatusOK, map[string]any{"access_token": "local-token", "token_type": "Bearer", "expires_in": 3600, "scope": "cli"}) + case r.Method == http.MethodPost && r.URL.Path == "/approve": + serverState.Lock() + serverState.approved = true + serverState.Unlock() + writeJSON(t, w, http.StatusOK, map[string]any{"success": true}) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + credentialsPath := filepath.Join(t.TempDir(), "credentials.json") + proc := runLoginProcess(t, server.URL, "ENTIRE_SECRETS_PATH="+credentialsPath) + + approvalURL, _ := waitForLoginPrompt(t, proc.stdout) + approveResp, doErr := http.Post(approvalURL, "application/json", http.NoBody) + if doErr != nil { + t.Fatalf("approve request failed: %v", doErr) + } + _ = approveResp.Body.Close() + + output, waitErr := proc.wait() + if waitErr != nil { + t.Fatalf("login command failed: %v\nOutput:\n%s", waitErr, output) + } + if !strings.Contains(output, "Login complete.") { + t.Fatalf("output missing login complete message:\n%s", output) + } + + if runtime.GOOS != "windows" { + stat, statErr := os.Stat(credentialsPath) + if statErr != nil { + t.Fatalf("stat credentials file: %v", statErr) + } + if got := stat.Mode().Perm(); got != 0o600 { + t.Fatalf("credentials mode = %o, want 600", got) + } + } + + data, readErr := os.ReadFile(credentialsPath) + if readErr != nil { + t.Fatalf("read credentials file: %v", readErr) + } + var credentials struct { + Version int `json:"version"` + Tokens map[string]string `json:"tokens"` + } + if err := json.Unmarshal(data, &credentials); err != nil { + t.Fatalf("unmarshal credentials: %v", err) + } + if credentials.Tokens[server.URL] != "local-token" { + t.Fatalf("stored token for %s = %q, want local-token", server.URL, credentials.Tokens[server.URL]) + } +} + func TestLogin_ExpiredFlow(t *testing.T) { t.Parallel() @@ -190,7 +279,7 @@ type loginProcess struct { waitFn func() (string, error) } -func runLoginProcess(t *testing.T, apiBaseURL string) *loginProcess { +func runLoginProcess(t *testing.T, apiBaseURL string, extraEnv ...string) *loginProcess { t.Helper() env := NewTestEnv(t) @@ -203,6 +292,7 @@ func runLoginProcess(t *testing.T, apiBaseURL string) *loginProcess { "ENTIRE_TEST_OPENCODE_PROJECT_DIR="+env.OpenCodeProjectDir, "ENTIRE_API_BASE_URL="+apiBaseURL, ) + cmd.Env = append(cmd.Env, extraEnv...) stdoutPipe, err := cmd.StdoutPipe() if err != nil { diff --git a/cmd/entire/cli/login.go b/cmd/entire/cli/login.go index 88618c522e..4c6e64237d 100644 --- a/cmd/entire/cli/login.go +++ b/cmd/entire/cli/login.go @@ -10,6 +10,7 @@ import ( "os" "os/exec" "runtime" + "strings" "time" "github.com/entireio/cli/cmd/entire/cli/auth" @@ -33,6 +34,10 @@ type deviceAuthClient interface { BaseURL() string } +type loginTokenStore interface { + SaveToken(baseURL, token string) error +} + func newLoginCmd() *cobra.Command { var insecureHTTPAuth bool cmd := &cobra.Command{ @@ -50,6 +55,10 @@ func newLoginCmd() *cobra.Command { } func runLogin(ctx context.Context, outW, errW io.Writer, client deviceAuthClient, openURL browserOpenFunc) error { + return runLoginWithStore(ctx, outW, errW, client, openURL, auth.NewStore()) +} + +func runLoginWithStore(ctx context.Context, outW, errW io.Writer, client deviceAuthClient, openURL browserOpenFunc, store loginTokenStore) error { start, err := client.StartDeviceAuth(ctx) if err != nil { return fmt.Errorf("start login: %w", err) @@ -84,12 +93,17 @@ func runLogin(ctx context.Context, outW, errW io.Writer, client deviceAuthClient return fmt.Errorf("complete login: %w", err) } - store := auth.NewStore() - if err := store.SaveToken(client.BaseURL(), token); err != nil { + if strings.TrimSpace(os.Getenv(auth.SecretsPathEnvVar)) == "" { + fmt.Fprintf(errW, "For headless environments, set %s=/path/to/credentials.json and retry `entire login`.\n", auth.SecretsPathEnvVar) + } return fmt.Errorf("save auth token: %w", err) } + if strings.TrimSpace(os.Getenv(auth.AuthTokenEnvVar)) != "" { + fmt.Fprintf(errW, "Warning: %s is set and will take precedence over the newly saved login token until it is unset.\n", auth.AuthTokenEnvVar) + } + fmt.Fprintln(outW, "Login complete.") return nil } diff --git a/cmd/entire/cli/login_test.go b/cmd/entire/cli/login_test.go index 63834fec3a..8afbafe47c 100644 --- a/cmd/entire/cli/login_test.go +++ b/cmd/entire/cli/login_test.go @@ -1,6 +1,7 @@ package cli import ( + "bytes" "context" "errors" "strings" @@ -13,6 +14,8 @@ import ( // mockClient implements deviceAuthClient for unit tests. type mockClient struct { responses []pollResponse + start *auth.DeviceAuthStart + startErr error calls int } @@ -22,7 +25,13 @@ type pollResponse struct { } func (m *mockClient) StartDeviceAuth(_ context.Context) (*auth.DeviceAuthStart, error) { - return nil, errors.New("not implemented in mock") + if m.startErr != nil { + return nil, m.startErr + } + if m.start == nil { + return nil, errors.New("not implemented in mock") + } + return m.start, nil } func (m *mockClient) BaseURL() string { @@ -38,6 +47,78 @@ func (m *mockClient) PollDeviceAuth(_ context.Context, _ string) (*auth.DeviceAu return r.result, r.err } +type mockLoginTokenStore struct { + saveErr error + baseURL string + savedToken string +} + +func (m *mockLoginTokenStore) SaveToken(baseURL, token string) error { + m.baseURL = baseURL + m.savedToken = token + return m.saveErr +} + +func newSuccessfulLoginClient(token string) *mockClient { + return &mockClient{ + start: &auth.DeviceAuthStart{ + DeviceCode: "device-1", + UserCode: "ABCD-EFGH", + VerificationURI: "https://entire.io/cli/auth", + ExpiresIn: 60, + Interval: 1, + }, + responses: []pollResponse{ + {result: &auth.DeviceAuthPoll{AccessToken: token}}, + }, + } +} + +func TestRunLogin_SaveFailureIncludesHeadlessHint(t *testing.T) { + client := newSuccessfulLoginClient("login-token") + store := &mockLoginTokenStore{ + saveErr: errors.New("save token to keyring: failed to unlock correct collection"), + } + + var out, errOut bytes.Buffer + err := runLoginWithStore(context.Background(), &out, &errOut, client, + func(context.Context, string) error { return nil }, + store) + if err == nil { + t.Fatal("expected save failure") + } + if !strings.Contains(err.Error(), "save auth token") { + t.Fatalf("error = %v, want save auth token context", err) + } + if !strings.Contains(errOut.String(), auth.SecretsPathEnvVar) { + t.Fatalf("stderr = %q, want %s hint", errOut.String(), auth.SecretsPathEnvVar) + } +} + +func TestRunLogin_WarnsWhenEnvTokenShadowsSavedToken(t *testing.T) { + t.Setenv(auth.AuthTokenEnvVar, "env-token") + + client := newSuccessfulLoginClient("login-token") + store := &mockLoginTokenStore{} + + var out, errOut bytes.Buffer + err := runLoginWithStore(context.Background(), &out, &errOut, client, + func(context.Context, string) error { return nil }, + store) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if store.savedToken != "login-token" { + t.Fatalf("saved token = %q, want login-token", store.savedToken) + } + if !strings.Contains(errOut.String(), auth.AuthTokenEnvVar) { + t.Fatalf("stderr = %q, want %s warning", errOut.String(), auth.AuthTokenEnvVar) + } + if !strings.Contains(out.String(), "Login complete.") { + t.Fatalf("stdout = %q, want login complete", out.String()) + } +} + func TestWaitForApproval_ImmediateSuccess(t *testing.T) { t.Parallel() diff --git a/cmd/entire/cli/logout.go b/cmd/entire/cli/logout.go index 77c8533976..86c9ca4e28 100644 --- a/cmd/entire/cli/logout.go +++ b/cmd/entire/cli/logout.go @@ -16,6 +16,7 @@ import ( // Used by logout and the auth subcommands. type tokenStore interface { GetToken(baseURL string) (string, error) + GetTokenInfo(baseURL string) (auth.TokenInfo, error) DeleteToken(baseURL string) error } @@ -45,14 +46,20 @@ func defaultRevokeCurrentToken(ctx context.Context, token string) error { } func runLogout(ctx context.Context, outW, errW io.Writer, store tokenStore, revoke revokeCurrentFunc, baseURL string) error { - token, err := store.GetToken(baseURL) + info, err := store.GetTokenInfo(baseURL) if err != nil { // Fall through to the local delete: we still want the keyring entry // gone, even if we couldn't read it well enough to revoke server-side. fmt.Fprintf(errW, "Warning: failed to read token before revocation: %v\n", err) } - if token != "" { - if err := revoke(ctx, token); err != nil && !api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { + if info.Source == auth.TokenSourceEnv { + fmt.Fprintf(outW, "Token is supplied by %s. Unset that environment variable to log out locally.\n", auth.AuthTokenEnvVar) + fmt.Fprintln(outW, "Run 'entire auth revoke --current' to invalidate this token server-side.") + return nil + } + + if info.Value != "" { + if err := revoke(ctx, info.Value); err != nil && !api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { // Best-effort: a transient network error shouldn't block local // logout. A 401 means the token is already invalid server-side, // so the desired state is achieved — no warning needed. diff --git a/cmd/entire/cli/logout_test.go b/cmd/entire/cli/logout_test.go index 28b3b57466..6a995f90e3 100644 --- a/cmd/entire/cli/logout_test.go +++ b/cmd/entire/cli/logout_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/entireio/cli/cmd/entire/cli/api" + "github.com/entireio/cli/cmd/entire/cli/auth" ) const testLogoutToken = "tok123" @@ -18,6 +19,8 @@ type mockTokenStore struct { deleted map[string]bool getErr error deleteErr error + source auth.TokenSource + path string getCalls int deleteCall int } @@ -37,6 +40,22 @@ func (m *mockTokenStore) GetToken(baseURL string) (string, error) { return m.tokens[baseURL], nil } +func (m *mockTokenStore) GetTokenInfo(baseURL string) (auth.TokenInfo, error) { + m.getCalls++ + if m.getErr != nil { + return auth.TokenInfo{}, m.getErr + } + token := m.tokens[baseURL] + if token == "" { + return auth.TokenInfo{}, nil + } + source := m.source + if source == auth.TokenSourceNone { + source = auth.TokenSourceKeyring + } + return auth.TokenInfo{Value: token, Source: source, Path: m.path}, nil +} + func (m *mockTokenStore) DeleteToken(baseURL string) error { m.deleteCall++ if m.deleteErr != nil { @@ -46,6 +65,38 @@ func (m *mockTokenStore) DeleteToken(baseURL string) error { return nil } +func TestRunLogout_EnvTokenDoesNotRevokeOrDelete(t *testing.T) { + t.Parallel() + + store := newMockTokenStore() + store.tokens["https://entire.io"] = testLogoutToken + store.source = auth.TokenSourceEnv + + revokeCalled := false + revoke := func(context.Context, string) error { + revokeCalled = true + return nil + } + + var out, errOut bytes.Buffer + err := runLogout(context.Background(), &out, &errOut, store, revoke, "https://entire.io") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if revokeCalled { + t.Fatal("logout should not revoke env token server-side") + } + if store.deleted["https://entire.io"] { + t.Fatal("logout should not delete lower-priority credentials for env token") + } + if !strings.Contains(out.String(), auth.AuthTokenEnvVar) { + t.Fatalf("stdout = %q, want env-token guidance", out.String()) + } + if errOut.Len() != 0 { + t.Fatalf("stderr = %q, want empty", errOut.String()) + } +} + func TestRunLogout_RevokesServerSideThenDeletesLocally(t *testing.T) { t.Parallel() From 9518e5f7283788ed8af0bb6d66e8c66c278993f7 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 00:27:11 -0700 Subject: [PATCH 02/10] auth: tighten headless auth tests --- README.md | 25 ++------------ cmd/entire/cli/auth/store_test.go | 54 ++++++++++++------------------- 2 files changed, 23 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 129722e327..ba429ad05a 100644 --- a/README.md +++ b/README.md @@ -348,30 +348,9 @@ Personal overrides, gitignored by default: ### Authentication Token Storage -By default, `entire login` stores the CLI API token in your operating system keyring. In headless environments, containers, and CI jobs where a keyring is unavailable, use one of these explicit alternatives: +By default, `entire login` stores the CLI API token in your operating system keyring. For CI, set `ENTIRE_AUTH_TOKEN` to a CLI API token. For headless machines that need persistent login, set `ENTIRE_SECRETS_PATH` to an absolute path before running `entire login`; the file is plaintext JSON written with `0600` permissions. -| Environment Variable | Use Case | Behavior | -| -------------------- | -------- | -------- | -| `ENTIRE_AUTH_TOKEN` | CI and ephemeral shells | Highest-priority read-only CLI API token. Entire uses it for authenticated API requests and never writes, deletes, or rotates it. | -| `ENTIRE_SECRETS_PATH` | Headless machines that need persistent login | Absolute path to a JSON token file. `entire login` writes this file with `0600` permissions, and `entire logout` removes the token from this file. | - -For CI, set `ENTIRE_AUTH_TOKEN` directly: - -```bash -export ENTIRE_AUTH_TOKEN=entire_api_token -entire auth status -``` - -For a remote container or server where `entire login` cannot save to the OS keyring, set `ENTIRE_SECRETS_PATH` to an absolute path before logging in: - -```bash -export ENTIRE_SECRETS_PATH="$HOME/.config/entire/credentials.json" -entire login -``` - -The token file is plaintext JSON protected by local filesystem permissions. Keep it outside your repository, do not commit it, and choose a path only your user can read. `ENTIRE_AUTH_TOKEN` takes precedence over tokens stored in `ENTIRE_SECRETS_PATH`, and the file store takes precedence over the OS keyring. - -`ENTIRE_CHECKPOINT_TOKEN` is separate from CLI API authentication. It is only used for checkpoint Git fetch and push operations. +`ENTIRE_AUTH_TOKEN` takes precedence over tokens stored in `ENTIRE_SECRETS_PATH`, and the file store takes precedence over the OS keyring. `ENTIRE_CHECKPOINT_TOKEN` is separate and only applies to checkpoint Git fetch/push operations. ### Agent Hook Configuration diff --git a/cmd/entire/cli/auth/store_test.go b/cmd/entire/cli/auth/store_test.go index bc3c33d95f..8170fa69fc 100644 --- a/cmd/entire/cli/auth/store_test.go +++ b/cmd/entire/cli/auth/store_test.go @@ -176,12 +176,18 @@ func TestStoreDeleteToken_NotFoundIsNoop(t *testing.T) { } } -func TestStoreFileBackend_SaveGetDelete(t *testing.T) { - // Not parallel: auth env vars are process-global. +func newFileBackendTestStore(t *testing.T, service string) (*Store, string) { + t.Helper() + path := filepath.Join(t.TempDir(), "credentials.json") t.Setenv(SecretsPathEnvVar, path) - store := NewStoreWithService("test-file-backend") + return NewStoreWithService(service), path +} + +func TestStoreFileBackend_SaveGetDelete(t *testing.T) { + // Not parallel: auth env vars are process-global. + store, path := newFileBackendTestStore(t, "test-file-backend") if err := store.SaveToken("https://entire.io", "file-token"); err != nil { t.Fatalf("SaveToken() error = %v", err) } @@ -218,10 +224,7 @@ func TestStoreFileBackend_SaveGetDelete(t *testing.T) { func TestStoreFileBackend_PreservesOtherBaseURLs(t *testing.T) { // Not parallel: auth env vars are process-global. - path := filepath.Join(t.TempDir(), "credentials.json") - t.Setenv(SecretsPathEnvVar, path) - - store := NewStoreWithService("test-file-preserve") + store, _ := newFileBackendTestStore(t, "test-file-preserve") if err := store.SaveToken("https://entire.io", "prod-token"); err != nil { t.Fatalf("SaveToken(prod) error = %v", err) } @@ -243,11 +246,9 @@ func TestStoreFileBackend_PreservesOtherBaseURLs(t *testing.T) { func TestStoreFileBackend_EnvTokenTakesPrecedence(t *testing.T) { // Not parallel: auth env vars are process-global. - path := filepath.Join(t.TempDir(), "credentials.json") - t.Setenv(SecretsPathEnvVar, path) + store, _ := newFileBackendTestStore(t, "test-file-env-precedence") t.Setenv(AuthTokenEnvVar, "env-token") - store := NewStoreWithService("test-file-env-precedence") if err := store.SaveToken("https://entire.io", "file-token"); err != nil { t.Fatalf("SaveToken() error = %v", err) } @@ -279,13 +280,11 @@ func TestStoreFileBackend_RejectsRelativePath(t *testing.T) { func TestStoreFileBackend_RejectsMalformedJSON(t *testing.T) { // Not parallel: auth env vars are process-global. - path := filepath.Join(t.TempDir(), "credentials.json") - t.Setenv(SecretsPathEnvVar, path) + store, path := newFileBackendTestStore(t, "test-file-malformed") if err := os.WriteFile(path, []byte("{not-json"), 0o600); err != nil { t.Fatalf("WriteFile() error = %v", err) } - store := NewStoreWithService("test-file-malformed") if _, err := store.GetTokenInfo("https://entire.io"); err == nil || !strings.Contains(err.Error(), "parse") { t.Fatalf("GetTokenInfo() err = %v, want parse error", err) } @@ -296,13 +295,11 @@ func TestStoreFileBackend_RejectsGroupReadableFile(t *testing.T) { t.Skip("permission bit checks are Unix-specific") } // Not parallel: auth env vars are process-global. - path := filepath.Join(t.TempDir(), "credentials.json") - t.Setenv(SecretsPathEnvVar, path) + store, path := newFileBackendTestStore(t, "test-file-perms") if err := os.WriteFile(path, []byte(`{"version":1,"tokens":{"https://entire.io":"tok"}}`), 0o640); err != nil { t.Fatalf("WriteFile() error = %v", err) } - store := NewStoreWithService("test-file-perms") if _, err := store.GetTokenInfo("https://entire.io"); err == nil || !strings.Contains(err.Error(), "chmod 600") { t.Fatalf("GetTokenInfo() err = %v, want chmod 600 hint", err) } @@ -310,10 +307,7 @@ func TestStoreFileBackend_RejectsGroupReadableFile(t *testing.T) { func TestStoreFileBackend_DeleteMissingFileDoesNotCreateFile(t *testing.T) { // Not parallel: auth env vars are process-global. - path := filepath.Join(t.TempDir(), "credentials.json") - t.Setenv(SecretsPathEnvVar, path) - - store := NewStoreWithService("test-file-delete-missing") + store, path := newFileBackendTestStore(t, "test-file-delete-missing") if err := store.DeleteToken("https://entire.io"); err != nil { t.Fatalf("DeleteToken() error = %v", err) } @@ -322,29 +316,23 @@ func TestStoreFileBackend_DeleteMissingFileDoesNotCreateFile(t *testing.T) { } } -func TestStoreFileBackend_DeleteMissingTokenDoesNotRewriteFile(t *testing.T) { +func TestStoreFileBackend_DeleteMissingTokenPreservesExistingToken(t *testing.T) { // Not parallel: auth env vars are process-global. - path := filepath.Join(t.TempDir(), "credentials.json") - t.Setenv(SecretsPathEnvVar, path) - - store := NewStoreWithService("test-file-delete-missing-token") + store, _ := newFileBackendTestStore(t, "test-file-delete-missing-token") if err := store.SaveToken("https://entire.io", "file-token"); err != nil { t.Fatalf("SaveToken() error = %v", err) } - before, err := os.ReadFile(path) - if err != nil { - t.Fatalf("ReadFile(before) error = %v", err) - } if err := store.DeleteToken("https://missing.example.com"); err != nil { t.Fatalf("DeleteToken() error = %v", err) } - after, err := os.ReadFile(path) + + got, err := store.GetToken("https://entire.io") if err != nil { - t.Fatalf("ReadFile(after) error = %v", err) + t.Fatalf("GetToken() error = %v", err) } - if string(after) != string(before) { - t.Fatalf("credentials file changed after deleting missing token\nbefore:\n%s\nafter:\n%s", string(before), string(after)) + if got != "file-token" { + t.Fatalf("GetToken() = %q, want existing token preserved", got) } } From 890a09a4b4be7d79f1a1eb75c5afbfa7d8283c3e Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 10:31:27 -0700 Subject: [PATCH 03/10] auth: idempotent env-token revoke --current Mirror the non-env logout branch: 401 from the server means the token was already invalid, other errors warn-and-continue. Both still print the unset-guidance so a CI re-run isn't blocked by a transient failure. Per Copilot review on PR #1127. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 462f68820c0d --- cmd/entire/cli/auth.go | 7 +++-- cmd/entire/cli/auth_test.go | 62 +++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/cmd/entire/cli/auth.go b/cmd/entire/cli/auth.go index f7228f0ed0..e9f485fdbd 100644 --- a/cmd/entire/cli/auth.go +++ b/cmd/entire/cli/auth.go @@ -497,8 +497,11 @@ func revokeCurrentToken( info auth.TokenInfo, ) error { if info.Source == auth.TokenSourceEnv { - if err := revoke(ctx, info.Value); err != nil { - return fmt.Errorf("revoke current token: %w", err) + // Mirror the non-env branch: 401 means the token was already invalid + // server-side (idempotent re-run), other errors warn but still surface + // the unset-guidance so a CI re-run isn't blocked by a transient failure. + if err := revoke(ctx, info.Value); err != nil && !api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { + fmt.Fprintf(errW, "Warning: server-side token revocation failed: %v\n", err) } fmt.Fprintf(outW, "Revoked current token supplied by %s. Unset it to stop using it locally.\n", auth.AuthTokenEnvVar) return nil diff --git a/cmd/entire/cli/auth_test.go b/cmd/entire/cli/auth_test.go index 077ff9ee41..f8077cc533 100644 --- a/cmd/entire/cli/auth_test.go +++ b/cmd/entire/cli/auth_test.go @@ -523,6 +523,68 @@ func TestRunAuthRevoke_CurrentEnvTokenRevokesButDoesNotDelete(t *testing.T) { } } +// TestRunAuthRevoke_CurrentEnvTokenServerErrors locks in idempotent re-runs: +// 401 must be silent (already revoked), other errors must warn-and-continue, +// and both cases must still print the unset-guidance so a CI re-run isn't +// blocked by a transient failure. +func TestRunAuthRevoke_CurrentEnvTokenServerErrors(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + revokeErr error + wantWarning string // empty = stderr must be empty + }{ + "already invalid": { + revokeErr: &api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"}, + wantWarning: "", + }, + "transient error": { + revokeErr: errors.New("connection refused"), + wantWarning: "connection refused", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + store := newMockTokenStore() + store.tokens[testBaseURL] = testAuthTok + store.source = auth.TokenSourceEnv + + revokeCurrent := func(context.Context, string) error { return tt.revokeErr } + + var out, errOut bytes.Buffer + err := runAuthRevoke(context.Background(), &out, &errOut, store, + func(context.Context, string) ([]api.Token, error) { return nil, nil }, + func(context.Context, string, string) error { return nil }, + revokeCurrent, testBaseURL, "", true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if tt.wantWarning == "" { + if errOut.Len() != 0 { + t.Fatalf("stderr = %q, want empty", errOut.String()) + } + } else { + if !strings.Contains(errOut.String(), "server-side token revocation failed") { + t.Fatalf("stderr = %q, want warning prefix", errOut.String()) + } + if !strings.Contains(errOut.String(), tt.wantWarning) { + t.Fatalf("stderr = %q, want underlying error %q", errOut.String(), tt.wantWarning) + } + } + if !strings.Contains(out.String(), auth.AuthTokenEnvVar) { + t.Fatalf("output = %q, want env-token guidance", out.String()) + } + if store.deleted[testBaseURL] { + t.Fatal("env-token revoke should not delete lower-priority credentials") + } + }) + } +} + func TestRunAuthRevoke_ByIDEnvSelfRevokeDoesNotDeleteLowerPriority(t *testing.T) { t.Parallel() From 0a05665f7248652472a24fb2ba647335a2ecb735 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 11:42:51 -0700 Subject: [PATCH 04/10] auth: drop dead path guard, generalize tokenStore comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - describeTokenSource: TokenInfo.Path is always set when source is file (configuredSecretsPath only returns ok=true with a non-empty path), so the empty-path fallback was unreachable. - tokenStore comment: backend can now be keyring, file, or env — clarify the abstraction rather than scoping it to keyring + a caller list. - runLogout fall-through comment: "stored token" is backend-neutral. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 95b31c3a77c9 --- cmd/entire/cli/auth.go | 5 +---- cmd/entire/cli/logout.go | 8 ++++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/cmd/entire/cli/auth.go b/cmd/entire/cli/auth.go index e9f485fdbd..c1f5f083f4 100644 --- a/cmd/entire/cli/auth.go +++ b/cmd/entire/cli/auth.go @@ -133,10 +133,7 @@ func describeTokenSource(info auth.TokenInfo) string { case auth.TokenSourceEnv: return "supplied by " + auth.AuthTokenEnvVar case auth.TokenSourceFile: - if info.Path != "" { - return "stored in file " + info.Path - } - return "stored in file" + return "stored in file " + info.Path case auth.TokenSourceKeyring: return "stored in OS keychain" default: diff --git a/cmd/entire/cli/logout.go b/cmd/entire/cli/logout.go index 86c9ca4e28..585587a5a7 100644 --- a/cmd/entire/cli/logout.go +++ b/cmd/entire/cli/logout.go @@ -11,9 +11,9 @@ import ( "github.com/spf13/cobra" ) -// tokenStore abstracts keyring access so commands that read or delete the -// stored bearer token can be unit-tested without hitting the real OS keyring. -// Used by logout and the auth subcommands. +// tokenStore abstracts the auth backend (keyring, file, env) so commands that +// read or delete the stored bearer token can be unit-tested without hitting +// the real OS keyring or filesystem. type tokenStore interface { GetToken(baseURL string) (string, error) GetTokenInfo(baseURL string) (auth.TokenInfo, error) @@ -48,7 +48,7 @@ func defaultRevokeCurrentToken(ctx context.Context, token string) error { func runLogout(ctx context.Context, outW, errW io.Writer, store tokenStore, revoke revokeCurrentFunc, baseURL string) error { info, err := store.GetTokenInfo(baseURL) if err != nil { - // Fall through to the local delete: we still want the keyring entry + // Fall through to the local delete: we still want the stored token // gone, even if we couldn't read it well enough to revoke server-side. fmt.Fprintf(errW, "Warning: failed to read token before revocation: %v\n", err) } From b739cbe84a092cbb41b665fafd7a0a5195e89aa4 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 12:01:47 -0700 Subject: [PATCH 05/10] auth: fold revoke --current env-token success case into table test The standalone TestRunAuthRevoke_CurrentEnvTokenRevokesButDoesNotDelete duplicated 80% of the env-source revoke harness already used by the TestRunAuthRevoke_CurrentEnvTokenServerErrors table. Adding "success" as a third table case (revokeErr: nil) keeps every assertion the standalone made (revokeByID not called, revokeCurrent gets the right token, output mentions the env var, lower-priority store untouched) while sharing the setup. Net: -32 LOC, same coverage. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 89e4cbb6497a --- cmd/entire/cli/auth_test.go | 84 ++++++++++++------------------------- 1 file changed, 26 insertions(+), 58 deletions(-) diff --git a/cmd/entire/cli/auth_test.go b/cmd/entire/cli/auth_test.go index f8077cc533..c2b09571cb 100644 --- a/cmd/entire/cli/auth_test.go +++ b/cmd/entire/cli/auth_test.go @@ -481,67 +481,20 @@ func TestRunAuthRevoke_CurrentDelegatesToLogout(t *testing.T) { } } -func TestRunAuthRevoke_CurrentEnvTokenRevokesButDoesNotDelete(t *testing.T) { - t.Parallel() - - store := newMockTokenStore() - store.tokens[testBaseURL] = testAuthTok - store.source = auth.TokenSourceEnv - - revokeByIDCalled := false - revokeByID := func(context.Context, string, string) error { - revokeByIDCalled = true - return nil - } - - revokedToken := "" - revokeCurrent := func(_ context.Context, token string) error { - revokedToken = token - return nil - } - - list := func(context.Context, string) ([]api.Token, error) { return nil, nil } - - var out, errOut bytes.Buffer - err := runAuthRevoke(context.Background(), &out, &errOut, store, - list, revokeByID, revokeCurrent, testBaseURL, "", true) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - if revokeByIDCalled { - t.Fatal("revokeByID should not be called when --current is set") - } - if revokedToken != testAuthTok { - t.Errorf("revokeCurrent called with token %q, want %q", revokedToken, testAuthTok) - } - if store.deleted[testBaseURL] { - t.Fatal("env-token revoke should not delete lower-priority credentials") - } - if !strings.Contains(out.String(), auth.AuthTokenEnvVar) { - t.Fatalf("output = %q, want env-token guidance", out.String()) - } -} - -// TestRunAuthRevoke_CurrentEnvTokenServerErrors locks in idempotent re-runs: -// 401 must be silent (already revoked), other errors must warn-and-continue, -// and both cases must still print the unset-guidance so a CI re-run isn't -// blocked by a transient failure. -func TestRunAuthRevoke_CurrentEnvTokenServerErrors(t *testing.T) { +// TestRunAuthRevoke_CurrentEnvToken covers the env-source --current path: +// success, idempotent 401, and warn-and-continue on transient errors. All +// three must dispatch to revokeCurrent (not revokeByID), print unset-guidance, +// and leave any lower-priority stored credentials untouched. +func TestRunAuthRevoke_CurrentEnvToken(t *testing.T) { t.Parallel() tests := map[string]struct { revokeErr error wantWarning string // empty = stderr must be empty }{ - "already invalid": { - revokeErr: &api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"}, - wantWarning: "", - }, - "transient error": { - revokeErr: errors.New("connection refused"), - wantWarning: "connection refused", - }, + "success": {nil, ""}, + "already invalid": {&api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"}, ""}, + "transient error": {errors.New("connection refused"), "connection refused"}, } for name, tt := range tests { @@ -552,17 +505,32 @@ func TestRunAuthRevoke_CurrentEnvTokenServerErrors(t *testing.T) { store.tokens[testBaseURL] = testAuthTok store.source = auth.TokenSourceEnv - revokeCurrent := func(context.Context, string) error { return tt.revokeErr } + revokedToken := "" + revokeCurrent := func(_ context.Context, token string) error { + revokedToken = token + return tt.revokeErr + } + + revokeByIDCalled := false + revokeByID := func(context.Context, string, string) error { + revokeByIDCalled = true + return nil + } var out, errOut bytes.Buffer err := runAuthRevoke(context.Background(), &out, &errOut, store, func(context.Context, string) ([]api.Token, error) { return nil, nil }, - func(context.Context, string, string) error { return nil }, - revokeCurrent, testBaseURL, "", true) + revokeByID, revokeCurrent, testBaseURL, "", true) if err != nil { t.Fatalf("unexpected error: %v", err) } + if revokeByIDCalled { + t.Fatal("revokeByID should not be called when --current is set") + } + if revokedToken != testAuthTok { + t.Errorf("revokeCurrent token = %q, want %q", revokedToken, testAuthTok) + } if tt.wantWarning == "" { if errOut.Len() != 0 { t.Fatalf("stderr = %q, want empty", errOut.String()) From 42355148613598f1cb6658af0d7fce142f3e78c1 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 12:28:38 -0700 Subject: [PATCH 06/10] auth: env-token revoke must fail loud on non-401 server errors Previously a transient revoke failure was downgraded to a warning while stdout still claimed the token had been revoked and the command exited 0. Env-tokens have no local copy to fall back on, so a 5xx/timeout leaves the bearer token active server-side while the user (or CI) believes it was invalidated. Now: only 401 is treated as idempotent success. Any other error returns non-zero and suppresses the "Revoked" success line. Per Codex adversarial review of #1127 (high severity). Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: bcb0590b5df5 --- cmd/entire/cli/auth.go | 13 +++++---- cmd/entire/cli/auth_test.go | 55 +++++++++++++++++++++---------------- 2 files changed, 39 insertions(+), 29 deletions(-) diff --git a/cmd/entire/cli/auth.go b/cmd/entire/cli/auth.go index c1f5f083f4..951a7e0237 100644 --- a/cmd/entire/cli/auth.go +++ b/cmd/entire/cli/auth.go @@ -494,11 +494,14 @@ func revokeCurrentToken( info auth.TokenInfo, ) error { if info.Source == auth.TokenSourceEnv { - // Mirror the non-env branch: 401 means the token was already invalid - // server-side (idempotent re-run), other errors warn but still surface - // the unset-guidance so a CI re-run isn't blocked by a transient failure. - if err := revoke(ctx, info.Value); err != nil && !api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { - fmt.Fprintf(errW, "Warning: server-side token revocation failed: %v\n", err) + // Env tokens have no local copy to fall back on, so a non-401 server + // failure means the token is still active — we MUST NOT print + // "Revoked" and exit zero in that case. 401 is treated as idempotent + // success (the token was already invalid server-side). + if err := revoke(ctx, info.Value); err != nil { + if !api.IsHTTPErrorStatus(err, http.StatusUnauthorized) { + return fmt.Errorf("revoke current token: %w", err) + } } fmt.Fprintf(outW, "Revoked current token supplied by %s. Unset it to stop using it locally.\n", auth.AuthTokenEnvVar) return nil diff --git a/cmd/entire/cli/auth_test.go b/cmd/entire/cli/auth_test.go index c2b09571cb..0480185145 100644 --- a/cmd/entire/cli/auth_test.go +++ b/cmd/entire/cli/auth_test.go @@ -482,19 +482,24 @@ func TestRunAuthRevoke_CurrentDelegatesToLogout(t *testing.T) { } // TestRunAuthRevoke_CurrentEnvToken covers the env-source --current path: -// success, idempotent 401, and warn-and-continue on transient errors. All -// three must dispatch to revokeCurrent (not revokeByID), print unset-guidance, -// and leave any lower-priority stored credentials untouched. +// - success → "Revoked" message, exit 0 +// - 401 (already invalid) → idempotent success: same message, exit 0 +// - non-401 transient error → return error; do NOT print "Revoked" because +// the bearer token is still active server-side +// +// All cases must dispatch to revokeCurrent (not revokeByID) and leave any +// lower-priority stored credentials untouched. func TestRunAuthRevoke_CurrentEnvToken(t *testing.T) { t.Parallel() tests := map[string]struct { - revokeErr error - wantWarning string // empty = stderr must be empty + revokeErr error + wantSuccessMsg bool // true = expect "Revoked" stdout + nil error + wantErrSubstr string }{ - "success": {nil, ""}, - "already invalid": {&api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"}, ""}, - "transient error": {errors.New("connection refused"), "connection refused"}, + "success": {nil, true, ""}, + "already invalid": {&api.HTTPError{StatusCode: http.StatusUnauthorized, Message: "Not authenticated"}, true, ""}, + "transient error": {errors.New("connection refused"), false, "connection refused"}, } for name, tt := range tests { @@ -521,9 +526,6 @@ func TestRunAuthRevoke_CurrentEnvToken(t *testing.T) { err := runAuthRevoke(context.Background(), &out, &errOut, store, func(context.Context, string) ([]api.Token, error) { return nil, nil }, revokeByID, revokeCurrent, testBaseURL, "", true) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } if revokeByIDCalled { t.Fatal("revokeByID should not be called when --current is set") @@ -531,23 +533,28 @@ func TestRunAuthRevoke_CurrentEnvToken(t *testing.T) { if revokedToken != testAuthTok { t.Errorf("revokeCurrent token = %q, want %q", revokedToken, testAuthTok) } - if tt.wantWarning == "" { - if errOut.Len() != 0 { - t.Fatalf("stderr = %q, want empty", errOut.String()) - } - } else { - if !strings.Contains(errOut.String(), "server-side token revocation failed") { - t.Fatalf("stderr = %q, want warning prefix", errOut.String()) + if store.deleted[testBaseURL] { + t.Fatal("env-token revoke should not delete lower-priority credentials") + } + + if tt.wantSuccessMsg { + if err != nil { + t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(errOut.String(), tt.wantWarning) { - t.Fatalf("stderr = %q, want underlying error %q", errOut.String(), tt.wantWarning) + if !strings.Contains(out.String(), auth.AuthTokenEnvVar) { + t.Fatalf("output = %q, want env-token guidance", out.String()) } + return } - if !strings.Contains(out.String(), auth.AuthTokenEnvVar) { - t.Fatalf("output = %q, want env-token guidance", out.String()) + + if err == nil { + t.Fatal("expected error for non-401 server failure") } - if store.deleted[testBaseURL] { - t.Fatal("env-token revoke should not delete lower-priority credentials") + if !strings.Contains(err.Error(), tt.wantErrSubstr) { + t.Fatalf("error = %v, want substring %q", err, tt.wantErrSubstr) + } + if strings.Contains(out.String(), "Revoked") { + t.Fatalf("stdout = %q, must not claim revoke success when revoke failed", out.String()) } }) } From deab46b2e70f330e0c7f916ed3b37d9e1a769322 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 12:35:28 -0700 Subject: [PATCH 07/10] auth: scope ENTIRE_AUTH_TOKEN to the production API origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously the env token was returned for any baseURL, so a stray ENTIRE_API_BASE_URL=https://staging override (or accidental local-dev override left from another session) would silently send the production bearer to the wrong endpoint. The file/keyring stores are already keyed by baseURL — only the env path was unscoped. Now: ENTIRE_AUTH_TOKEN only applies when baseURL == api.DefaultBaseURL. Custom origins fall through to the per-origin stores. README updated. Per Codex adversarial review of #1127 (medium severity). Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 9bdf2cb85e3a --- README.md | 2 +- cmd/entire/cli/auth/store.go | 10 ++++++++-- cmd/entire/cli/auth/store_test.go | 30 ++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index ba429ad05a..92e1ff90a1 100644 --- a/README.md +++ b/README.md @@ -350,7 +350,7 @@ Personal overrides, gitignored by default: By default, `entire login` stores the CLI API token in your operating system keyring. For CI, set `ENTIRE_AUTH_TOKEN` to a CLI API token. For headless machines that need persistent login, set `ENTIRE_SECRETS_PATH` to an absolute path before running `entire login`; the file is plaintext JSON written with `0600` permissions. -`ENTIRE_AUTH_TOKEN` takes precedence over tokens stored in `ENTIRE_SECRETS_PATH`, and the file store takes precedence over the OS keyring. `ENTIRE_CHECKPOINT_TOKEN` is separate and only applies to checkpoint Git fetch/push operations. +`ENTIRE_AUTH_TOKEN` takes precedence over tokens stored in `ENTIRE_SECRETS_PATH`, and the file store takes precedence over the OS keyring. `ENTIRE_AUTH_TOKEN` only applies when the API origin is the production default (`https://entire.io`); custom origins set via `ENTIRE_API_BASE_URL` must use the per-origin file or keyring stores so a stray override can't leak a prod bearer to a staging endpoint. `ENTIRE_CHECKPOINT_TOKEN` is separate and only applies to checkpoint Git fetch/push operations. ### Agent Hook Configuration diff --git a/cmd/entire/cli/auth/store.go b/cmd/entire/cli/auth/store.go index 0bd03cab95..c5225e5b08 100644 --- a/cmd/entire/cli/auth/store.go +++ b/cmd/entire/cli/auth/store.go @@ -63,8 +63,14 @@ func (s *Store) GetToken(baseURL string) (string, error) { // GetTokenInfo retrieves a stored token and reports where it came from. // Returns an empty TokenInfo (and no error) if no token is stored. func (s *Store) GetTokenInfo(baseURL string) (TokenInfo, error) { - if token := envAuthToken(); token != "" { - return TokenInfo{Value: token, Source: TokenSourceEnv}, nil + // ENTIRE_AUTH_TOKEN is scoped to the production API origin so a stray + // ENTIRE_API_BASE_URL override can't leak a prod bearer to a staging + // or local endpoint. Custom origins must use the per-origin file or + // keyring stores, which are already keyed by baseURL. + if baseURL == api.DefaultBaseURL { + if token := envAuthToken(); token != "" { + return TokenInfo{Value: token, Source: TokenSourceEnv}, nil + } } if path, ok, err := configuredSecretsPath(); err != nil { diff --git a/cmd/entire/cli/auth/store_test.go b/cmd/entire/cli/auth/store_test.go index 8170fa69fc..2fef02eb3d 100644 --- a/cmd/entire/cli/auth/store_test.go +++ b/cmd/entire/cli/auth/store_test.go @@ -87,6 +87,36 @@ func TestStoreGetTokenInfo_WhitespaceEnvTokenIgnored(t *testing.T) { } } +func TestStoreGetTokenInfo_EnvTokenScopedToDefaultBaseURL(t *testing.T) { + // Not parallel: go-keyring's mock provider uses an unprotected map. + t.Setenv(AuthTokenEnvVar, "env-token") + + store := NewStoreWithService("test-env-scope") + if err := store.SaveToken("http://localhost:8787", "local-keyring-token"); err != nil { + t.Fatalf("SaveToken() error = %v", err) + } + + // Custom base URL must NOT receive ENTIRE_AUTH_TOKEN — falls through to + // the per-origin keyring/file store so a prod bearer can't leak to a + // staging/dev override. + info, err := store.GetTokenInfo("http://localhost:8787") + if err != nil { + t.Fatalf("GetTokenInfo(custom) error = %v", err) + } + if info.Value != "local-keyring-token" || info.Source != TokenSourceKeyring { + t.Fatalf("GetTokenInfo(custom) = %#v, want local-keyring-token/keyring", info) + } + + // Default base URL still gets ENTIRE_AUTH_TOKEN. + info, err = store.GetTokenInfo(api.DefaultBaseURL) + if err != nil { + t.Fatalf("GetTokenInfo(default) error = %v", err) + } + if info.Value != "env-token" || info.Source != TokenSourceEnv { + t.Fatalf("GetTokenInfo(default) = %#v, want env-token/env", info) + } +} + func TestStoreSaveToken_PreservesOtherBaseURLs(t *testing.T) { // Not parallel: go-keyring's mock provider uses an unprotected map. store := NewStoreWithService("test-preserve") From b203b2308a80421c1b9f56d1fc3a71d81330e971 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 12:37:34 -0700 Subject: [PATCH 08/10] auth: add --no-keyring fail-closed login flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restores the per-invocation opt-out the original issue asked for. Previously the only way to bypass keyring was the ambient env var ENTIRE_SECRETS_PATH; if that var was unset (or stripped from the environment), 'entire login' would silently fall back to the OS keyring even when the user had explicitly opted out of it. Now: --no-keyring preflights ENTIRE_SECRETS_PATH before kicking off device auth and refuses to proceed without it. Read paths are unchanged — the flag only constrains where login writes. Per Codex adversarial review of #1127 (medium severity), and addresses the third option from the original issue request. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: cc14ee7c7659 --- README.md | 2 +- cmd/entire/cli/login.go | 19 +++++++++++++++++++ cmd/entire/cli/login_test.go | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 92e1ff90a1..6b4f79cf6a 100644 --- a/README.md +++ b/README.md @@ -348,7 +348,7 @@ Personal overrides, gitignored by default: ### Authentication Token Storage -By default, `entire login` stores the CLI API token in your operating system keyring. For CI, set `ENTIRE_AUTH_TOKEN` to a CLI API token. For headless machines that need persistent login, set `ENTIRE_SECRETS_PATH` to an absolute path before running `entire login`; the file is plaintext JSON written with `0600` permissions. +By default, `entire login` stores the CLI API token in your operating system keyring. For CI, set `ENTIRE_AUTH_TOKEN` to a CLI API token. For headless machines that need persistent login, set `ENTIRE_SECRETS_PATH` to an absolute path before running `entire login`; the file is plaintext JSON written with `0600` permissions. Pass `--no-keyring` to `entire login` if you want the command to fail closed when `ENTIRE_SECRETS_PATH` is missing instead of silently falling back to the OS keyring. `ENTIRE_AUTH_TOKEN` takes precedence over tokens stored in `ENTIRE_SECRETS_PATH`, and the file store takes precedence over the OS keyring. `ENTIRE_AUTH_TOKEN` only applies when the API origin is the production default (`https://entire.io`); custom origins set via `ENTIRE_API_BASE_URL` must use the per-origin file or keyring stores so a stray override can't leak a prod bearer to a staging endpoint. `ENTIRE_CHECKPOINT_TOKEN` is separate and only applies to checkpoint Git fetch/push operations. diff --git a/cmd/entire/cli/login.go b/cmd/entire/cli/login.go index 4c6e64237d..d693018189 100644 --- a/cmd/entire/cli/login.go +++ b/cmd/entire/cli/login.go @@ -40,6 +40,7 @@ type loginTokenStore interface { func newLoginCmd() *cobra.Command { var insecureHTTPAuth bool + var noKeyring bool cmd := &cobra.Command{ Use: "login", Short: "Log in to Entire", @@ -47,13 +48,31 @@ func newLoginCmd() *cobra.Command { if err := requireSecureBaseURL(insecureHTTPAuth); err != nil { return err } + if err := requireNonKeyringStorageConfigured(noKeyring); err != nil { + return err + } return runLogin(cmd.Context(), cmd.OutOrStdout(), cmd.ErrOrStderr(), auth.NewClient(nil), openBrowser) }, } + cmd.Flags().BoolVar(&noKeyring, "no-keyring", false, "Refuse to save the token to the OS keyring; requires ENTIRE_SECRETS_PATH to be set") addInsecureHTTPAuthFlag(cmd, &insecureHTTPAuth) return cmd } +// requireNonKeyringStorageConfigured fails closed before device auth when +// --no-keyring is set without a non-keyring backend configured. The flag +// exists for headless/CI environments where falling back to the OS keyring +// silently is unsafe — the user has explicitly opted out of it. +func requireNonKeyringStorageConfigured(noKeyring bool) error { + if !noKeyring { + return nil + } + if strings.TrimSpace(os.Getenv(auth.SecretsPathEnvVar)) == "" { + return fmt.Errorf("--no-keyring requires %s to be set to an absolute file path", auth.SecretsPathEnvVar) + } + return nil +} + func runLogin(ctx context.Context, outW, errW io.Writer, client deviceAuthClient, openURL browserOpenFunc) error { return runLoginWithStore(ctx, outW, errW, client, openURL, auth.NewStore()) } diff --git a/cmd/entire/cli/login_test.go b/cmd/entire/cli/login_test.go index 8afbafe47c..a40f19174c 100644 --- a/cmd/entire/cli/login_test.go +++ b/cmd/entire/cli/login_test.go @@ -74,6 +74,40 @@ func newSuccessfulLoginClient(token string) *mockClient { } } +func TestRequireNonKeyringStorageConfigured(t *testing.T) { + t.Run("flag unset is a no-op", func(t *testing.T) { + t.Setenv(auth.SecretsPathEnvVar, "") + if err := requireNonKeyringStorageConfigured(false); err != nil { + t.Fatalf("err = %v, want nil when --no-keyring is unset", err) + } + }) + + t.Run("flag set without ENTIRE_SECRETS_PATH errors before device auth", func(t *testing.T) { + t.Setenv(auth.SecretsPathEnvVar, "") + err := requireNonKeyringStorageConfigured(true) + if err == nil { + t.Fatal("expected error when --no-keyring is set with no SECRETS_PATH") + } + if !strings.Contains(err.Error(), auth.SecretsPathEnvVar) { + t.Fatalf("err = %v, want mention of %s", err, auth.SecretsPathEnvVar) + } + }) + + t.Run("flag set with ENTIRE_SECRETS_PATH passes preflight", func(t *testing.T) { + t.Setenv(auth.SecretsPathEnvVar, "/tmp/credentials.json") + if err := requireNonKeyringStorageConfigured(true); err != nil { + t.Fatalf("err = %v, want nil when SECRETS_PATH is set", err) + } + }) + + t.Run("flag set with whitespace-only ENTIRE_SECRETS_PATH errors", func(t *testing.T) { + t.Setenv(auth.SecretsPathEnvVar, " ") + if err := requireNonKeyringStorageConfigured(true); err == nil { + t.Fatal("whitespace SECRETS_PATH must not satisfy --no-keyring") + } + }) +} + func TestRunLogin_SaveFailureIncludesHeadlessHint(t *testing.T) { client := newSuccessfulLoginClient("login-token") store := &mockLoginTokenStore{ From 19b26afc56e13ae2c7eb658123287f8d511ac97a Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 12:50:03 -0700 Subject: [PATCH 09/10] auth: serialize file-store writes with flock to prevent lost updates Two concurrent CLI invocations sharing ENTIRE_SECRETS_PATH could each read the pre-write JSON, mutate different baseURL entries, and the last temp+rename would clobber the first save. atomic-rename writes make individual writes torn-free for readers, but the read-modify-write cycle was unprotected. Wraps saveFileToken / deleteFileToken in a sidecar-flock-based critical section. The credentials file itself can't be flocked safely because writeFileStore replaces the inode via temp+rename, so the lock lives on credentials.json.lock alongside it. Reads stay lock-free since atomic rename guarantees reader sees a complete prior or new file. Unix uses syscall.Flock; Windows is currently a no-op (Credential Manager via the keyring backend is the recommended path on Windows; LockFileEx hook is documented for future work). Concurrency test: 16 goroutines saving distinct keys all land in the final file with no losses, exercised under -race. Per Codex adversarial review of #1127 (medium severity). Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: a111d61d40ac --- cmd/entire/cli/auth/file_lock_unix.go | 22 ++++++++ cmd/entire/cli/auth/file_lock_windows.go | 14 +++++ cmd/entire/cli/auth/file_store.go | 67 ++++++++++++++++-------- cmd/entire/cli/auth/store_test.go | 61 +++++++++++++++++++-- 4 files changed, 138 insertions(+), 26 deletions(-) create mode 100644 cmd/entire/cli/auth/file_lock_unix.go create mode 100644 cmd/entire/cli/auth/file_lock_windows.go diff --git a/cmd/entire/cli/auth/file_lock_unix.go b/cmd/entire/cli/auth/file_lock_unix.go new file mode 100644 index 0000000000..4dc5769460 --- /dev/null +++ b/cmd/entire/cli/auth/file_lock_unix.go @@ -0,0 +1,22 @@ +//go:build !windows + +package auth + +import ( + "fmt" + "os" + "syscall" +) + +// acquireExclusiveLock takes a blocking flock(2) exclusive lock on f. The +// returned release function unlocks the file; the kernel also releases the +// lock when the file is closed or the process exits. +func acquireExclusiveLock(f *os.File) (func(), error) { + fd := int(f.Fd()) //nolint:gosec // file descriptors fit in int on every supported platform. + if err := syscall.Flock(fd, syscall.LOCK_EX); err != nil { + return nil, fmt.Errorf("flock token file: %w", err) + } + return func() { + _ = syscall.Flock(fd, syscall.LOCK_UN) //nolint:errcheck // best-effort unlock; kernel releases on close/exit. + }, nil +} diff --git a/cmd/entire/cli/auth/file_lock_windows.go b/cmd/entire/cli/auth/file_lock_windows.go new file mode 100644 index 0000000000..9f114e5e04 --- /dev/null +++ b/cmd/entire/cli/auth/file_lock_windows.go @@ -0,0 +1,14 @@ +//go:build windows + +package auth + +import "os" + +// acquireExclusiveLock is a no-op on Windows. Concurrent CLI invocations +// against the same ENTIRE_SECRETS_PATH file are uncommon on Windows (users +// who need persistent CLI auth typically use Credential Manager via the +// keyring backend). If concurrent file access becomes a problem on Windows, +// switch this to LockFileEx via golang.org/x/sys/windows. +func acquireExclusiveLock(_ *os.File) (func(), error) { + return func() {}, nil +} diff --git a/cmd/entire/cli/auth/file_store.go b/cmd/entire/cli/auth/file_store.go index afeb75773b..d0a63630a1 100644 --- a/cmd/entire/cli/auth/file_store.go +++ b/cmd/entire/cli/auth/file_store.go @@ -147,6 +147,9 @@ func newFileStoreData() fileStoreData { } func getFileToken(path, baseURL string) (string, error) { + // Atomic-rename writes mean readers always see a complete prior or new + // file — no torn reads possible — so locking is unnecessary on the read + // path. store, err := readFileStore(path) if err != nil { return "", err @@ -155,34 +158,56 @@ func getFileToken(path, baseURL string) (string, error) { } func saveFileToken(path, baseURL, token string) error { - store, err := readFileStore(path) - if err != nil { - return err - } - store.Tokens[baseURL] = token - if err := writeFileStore(path, store); err != nil { - return err - } - return nil + return withFileStoreLock(path, func() error { + store, err := readFileStore(path) + if err != nil { + return err + } + store.Tokens[baseURL] = token + return writeFileStore(path, store) + }) } func deleteFileToken(path, baseURL string) error { - if _, err := os.Stat(path); os.IsNotExist(err) { - return nil - } else if err != nil { - return fmt.Errorf("stat token file: %w", err) + return withFileStoreLock(path, func() error { + if _, err := os.Stat(path); os.IsNotExist(err) { + return nil + } else if err != nil { + return fmt.Errorf("stat token file: %w", err) + } + + store, err := readFileStore(path) + if err != nil { + return err + } + if _, ok := store.Tokens[baseURL]; !ok { + return nil + } + delete(store.Tokens, baseURL) + return writeFileStore(path, store) + }) +} + +// withFileStoreLock serializes the read-modify-write cycle across processes +// using a sidecar lock file. The credentials file itself can't be flocked +// safely because writeFileStore replaces the inode via temp+rename. +func withFileStoreLock(path string, fn func() error) error { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return fmt.Errorf("create token file directory: %w", err) } - store, err := readFileStore(path) + lockPath := path + ".lock" + lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600) //nolint:gosec // sidecar lock for explicit user-provided credential path. if err != nil { - return err - } - if _, ok := store.Tokens[baseURL]; !ok { - return nil + return fmt.Errorf("open token lock file: %w", err) } - delete(store.Tokens, baseURL) - if err := writeFileStore(path, store); err != nil { + defer func() { _ = lockFile.Close() }() + + release, err := acquireExclusiveLock(lockFile) + if err != nil { return err } - return nil + defer release() + + return fn() } diff --git a/cmd/entire/cli/auth/store_test.go b/cmd/entire/cli/auth/store_test.go index 2fef02eb3d..12453f827a 100644 --- a/cmd/entire/cli/auth/store_test.go +++ b/cmd/entire/cli/auth/store_test.go @@ -1,6 +1,7 @@ package auth import ( + "fmt" "os" "path/filepath" "runtime" @@ -13,6 +14,7 @@ import ( const ( testLocalToken = "local-token" + testEnvToken = "env-token" testWindowsOS = "windows" ) @@ -64,7 +66,7 @@ func TestStoreGetTokenInfo_EnvTokenTakesPrecedence(t *testing.T) { if err != nil { t.Fatalf("GetTokenInfo() error = %v", err) } - if info.Value != "env-token" || info.Source != TokenSourceEnv { + if info.Value != testEnvToken || info.Source != TokenSourceEnv { t.Fatalf("GetTokenInfo() = %#v, want env token/source", info) } } @@ -89,7 +91,7 @@ func TestStoreGetTokenInfo_WhitespaceEnvTokenIgnored(t *testing.T) { func TestStoreGetTokenInfo_EnvTokenScopedToDefaultBaseURL(t *testing.T) { // Not parallel: go-keyring's mock provider uses an unprotected map. - t.Setenv(AuthTokenEnvVar, "env-token") + t.Setenv(AuthTokenEnvVar, testEnvToken) store := NewStoreWithService("test-env-scope") if err := store.SaveToken("http://localhost:8787", "local-keyring-token"); err != nil { @@ -112,7 +114,7 @@ func TestStoreGetTokenInfo_EnvTokenScopedToDefaultBaseURL(t *testing.T) { if err != nil { t.Fatalf("GetTokenInfo(default) error = %v", err) } - if info.Value != "env-token" || info.Source != TokenSourceEnv { + if info.Value != testEnvToken || info.Source != TokenSourceEnv { t.Fatalf("GetTokenInfo(default) = %#v, want env-token/env", info) } } @@ -277,7 +279,7 @@ func TestStoreFileBackend_PreservesOtherBaseURLs(t *testing.T) { func TestStoreFileBackend_EnvTokenTakesPrecedence(t *testing.T) { // Not parallel: auth env vars are process-global. store, _ := newFileBackendTestStore(t, "test-file-env-precedence") - t.Setenv(AuthTokenEnvVar, "env-token") + t.Setenv(AuthTokenEnvVar, testEnvToken) if err := store.SaveToken("https://entire.io", "file-token"); err != nil { t.Fatalf("SaveToken() error = %v", err) @@ -287,7 +289,7 @@ func TestStoreFileBackend_EnvTokenTakesPrecedence(t *testing.T) { if err != nil { t.Fatalf("GetTokenInfo() error = %v", err) } - if info.Value != "env-token" || info.Source != TokenSourceEnv { + if info.Value != testEnvToken || info.Source != TokenSourceEnv { t.Fatalf("GetTokenInfo() = %#v, want env token/source", info) } } @@ -346,6 +348,55 @@ func TestStoreFileBackend_DeleteMissingFileDoesNotCreateFile(t *testing.T) { } } +// TestStoreFileBackend_ConcurrentSavesNoLostUpdate verifies the cross-process +// lock serializes overlapping read-modify-write cycles. Without locking, two +// concurrent saves to different baseURLs would race: each would read the +// pre-write state, mutate its own entry, and the second rename would clobber +// the first save. The flock around the file_store cycle prevents that. +func TestStoreFileBackend_ConcurrentSavesNoLostUpdate(t *testing.T) { + if runtime.GOOS == testWindowsOS { + t.Skip("file locking is a no-op on Windows; concurrency guarantee not enforced") + } + // Not parallel: auth env vars are process-global. + store, path := newFileBackendTestStore(t, "test-file-concurrent") + + const writers = 16 + errs := make(chan error, writers) + start := make(chan struct{}) + for i := range writers { + baseURL := fmt.Sprintf("https://entire.io/test-%02d", i) + token := fmt.Sprintf("token-%02d", i) + go func() { + <-start + errs <- store.SaveToken(baseURL, token) + }() + } + close(start) + for range writers { + if err := <-errs; err != nil { + t.Fatalf("SaveToken() error = %v", err) + } + } + + // Reading via the package-level helper is fine: writes are atomic via + // temp+rename, so the post-write file always reflects all serialized + // updates. + final, err := readFileStore(path) + if err != nil { + t.Fatalf("readFileStore() error = %v", err) + } + if got := len(final.Tokens); got != writers { + t.Fatalf("token count = %d, want %d (lost update under concurrency)", got, writers) + } + for i := range writers { + baseURL := fmt.Sprintf("https://entire.io/test-%02d", i) + want := fmt.Sprintf("token-%02d", i) + if got := final.Tokens[baseURL]; got != want { + t.Errorf("Tokens[%s] = %q, want %q", baseURL, got, want) + } + } +} + func TestStoreFileBackend_DeleteMissingTokenPreservesExistingToken(t *testing.T) { // Not parallel: auth env vars are process-global. store, _ := newFileBackendTestStore(t, "test-file-delete-missing-token") From d63ed2685382edae03e5b60b6de31103a1de833c Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 12:56:12 -0700 Subject: [PATCH 10/10] Revert "auth: serialize file-store writes with flock to prevent lost updates" This reverts commit 19b26afc5. Scope reduction for #1036. The lost-update race the lock prevented is hypothetical: it requires two concurrent CLI invocations sharing one ENTIRE_SECRETS_PATH file, which is exotic in practice (CI jobs use distinct paths or ENTIRE_AUTH_TOKEN; dev machines run the CLI one at a time). 138 lines of cross-platform locking machinery + sidecar .lock convention + concurrency test is more than the threat model justifies in this PR. The atomic temp+rename writes still guarantee readers never see torn content. If concurrent-write collisions become a real reported issue, a follow-up can restore the flock without affecting the public API. Per Codex adversarial review (medium severity, deferred to follow-up). Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/entire/cli/auth/file_lock_unix.go | 22 -------- cmd/entire/cli/auth/file_lock_windows.go | 14 ----- cmd/entire/cli/auth/file_store.go | 67 ++++++++---------------- cmd/entire/cli/auth/store_test.go | 50 ------------------ 4 files changed, 21 insertions(+), 132 deletions(-) delete mode 100644 cmd/entire/cli/auth/file_lock_unix.go delete mode 100644 cmd/entire/cli/auth/file_lock_windows.go diff --git a/cmd/entire/cli/auth/file_lock_unix.go b/cmd/entire/cli/auth/file_lock_unix.go deleted file mode 100644 index 4dc5769460..0000000000 --- a/cmd/entire/cli/auth/file_lock_unix.go +++ /dev/null @@ -1,22 +0,0 @@ -//go:build !windows - -package auth - -import ( - "fmt" - "os" - "syscall" -) - -// acquireExclusiveLock takes a blocking flock(2) exclusive lock on f. The -// returned release function unlocks the file; the kernel also releases the -// lock when the file is closed or the process exits. -func acquireExclusiveLock(f *os.File) (func(), error) { - fd := int(f.Fd()) //nolint:gosec // file descriptors fit in int on every supported platform. - if err := syscall.Flock(fd, syscall.LOCK_EX); err != nil { - return nil, fmt.Errorf("flock token file: %w", err) - } - return func() { - _ = syscall.Flock(fd, syscall.LOCK_UN) //nolint:errcheck // best-effort unlock; kernel releases on close/exit. - }, nil -} diff --git a/cmd/entire/cli/auth/file_lock_windows.go b/cmd/entire/cli/auth/file_lock_windows.go deleted file mode 100644 index 9f114e5e04..0000000000 --- a/cmd/entire/cli/auth/file_lock_windows.go +++ /dev/null @@ -1,14 +0,0 @@ -//go:build windows - -package auth - -import "os" - -// acquireExclusiveLock is a no-op on Windows. Concurrent CLI invocations -// against the same ENTIRE_SECRETS_PATH file are uncommon on Windows (users -// who need persistent CLI auth typically use Credential Manager via the -// keyring backend). If concurrent file access becomes a problem on Windows, -// switch this to LockFileEx via golang.org/x/sys/windows. -func acquireExclusiveLock(_ *os.File) (func(), error) { - return func() {}, nil -} diff --git a/cmd/entire/cli/auth/file_store.go b/cmd/entire/cli/auth/file_store.go index d0a63630a1..afeb75773b 100644 --- a/cmd/entire/cli/auth/file_store.go +++ b/cmd/entire/cli/auth/file_store.go @@ -147,9 +147,6 @@ func newFileStoreData() fileStoreData { } func getFileToken(path, baseURL string) (string, error) { - // Atomic-rename writes mean readers always see a complete prior or new - // file — no torn reads possible — so locking is unnecessary on the read - // path. store, err := readFileStore(path) if err != nil { return "", err @@ -158,56 +155,34 @@ func getFileToken(path, baseURL string) (string, error) { } func saveFileToken(path, baseURL, token string) error { - return withFileStoreLock(path, func() error { - store, err := readFileStore(path) - if err != nil { - return err - } - store.Tokens[baseURL] = token - return writeFileStore(path, store) - }) + store, err := readFileStore(path) + if err != nil { + return err + } + store.Tokens[baseURL] = token + if err := writeFileStore(path, store); err != nil { + return err + } + return nil } func deleteFileToken(path, baseURL string) error { - return withFileStoreLock(path, func() error { - if _, err := os.Stat(path); os.IsNotExist(err) { - return nil - } else if err != nil { - return fmt.Errorf("stat token file: %w", err) - } - - store, err := readFileStore(path) - if err != nil { - return err - } - if _, ok := store.Tokens[baseURL]; !ok { - return nil - } - delete(store.Tokens, baseURL) - return writeFileStore(path, store) - }) -} - -// withFileStoreLock serializes the read-modify-write cycle across processes -// using a sidecar lock file. The credentials file itself can't be flocked -// safely because writeFileStore replaces the inode via temp+rename. -func withFileStoreLock(path string, fn func() error) error { - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { - return fmt.Errorf("create token file directory: %w", err) + if _, err := os.Stat(path); os.IsNotExist(err) { + return nil + } else if err != nil { + return fmt.Errorf("stat token file: %w", err) } - lockPath := path + ".lock" - lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600) //nolint:gosec // sidecar lock for explicit user-provided credential path. + store, err := readFileStore(path) if err != nil { - return fmt.Errorf("open token lock file: %w", err) + return err } - defer func() { _ = lockFile.Close() }() - - release, err := acquireExclusiveLock(lockFile) - if err != nil { + if _, ok := store.Tokens[baseURL]; !ok { + return nil + } + delete(store.Tokens, baseURL) + if err := writeFileStore(path, store); err != nil { return err } - defer release() - - return fn() + return nil } diff --git a/cmd/entire/cli/auth/store_test.go b/cmd/entire/cli/auth/store_test.go index 12453f827a..0b92d771d3 100644 --- a/cmd/entire/cli/auth/store_test.go +++ b/cmd/entire/cli/auth/store_test.go @@ -1,7 +1,6 @@ package auth import ( - "fmt" "os" "path/filepath" "runtime" @@ -348,55 +347,6 @@ func TestStoreFileBackend_DeleteMissingFileDoesNotCreateFile(t *testing.T) { } } -// TestStoreFileBackend_ConcurrentSavesNoLostUpdate verifies the cross-process -// lock serializes overlapping read-modify-write cycles. Without locking, two -// concurrent saves to different baseURLs would race: each would read the -// pre-write state, mutate its own entry, and the second rename would clobber -// the first save. The flock around the file_store cycle prevents that. -func TestStoreFileBackend_ConcurrentSavesNoLostUpdate(t *testing.T) { - if runtime.GOOS == testWindowsOS { - t.Skip("file locking is a no-op on Windows; concurrency guarantee not enforced") - } - // Not parallel: auth env vars are process-global. - store, path := newFileBackendTestStore(t, "test-file-concurrent") - - const writers = 16 - errs := make(chan error, writers) - start := make(chan struct{}) - for i := range writers { - baseURL := fmt.Sprintf("https://entire.io/test-%02d", i) - token := fmt.Sprintf("token-%02d", i) - go func() { - <-start - errs <- store.SaveToken(baseURL, token) - }() - } - close(start) - for range writers { - if err := <-errs; err != nil { - t.Fatalf("SaveToken() error = %v", err) - } - } - - // Reading via the package-level helper is fine: writes are atomic via - // temp+rename, so the post-write file always reflects all serialized - // updates. - final, err := readFileStore(path) - if err != nil { - t.Fatalf("readFileStore() error = %v", err) - } - if got := len(final.Tokens); got != writers { - t.Fatalf("token count = %d, want %d (lost update under concurrency)", got, writers) - } - for i := range writers { - baseURL := fmt.Sprintf("https://entire.io/test-%02d", i) - want := fmt.Sprintf("token-%02d", i) - if got := final.Tokens[baseURL]; got != want { - t.Errorf("Tokens[%s] = %q, want %q", baseURL, got, want) - } - } -} - func TestStoreFileBackend_DeleteMissingTokenPreservesExistingToken(t *testing.T) { // Not parallel: auth env vars are process-global. store, _ := newFileBackendTestStore(t, "test-file-delete-missing-token")