auth: review follow-ups (provider routing, URL normalization, expiry preflight, HTTP timeouts)#1156
Open
Soph wants to merge 1 commit intoalex/cli-auth-consolidationfrom
Open
auth: review follow-ups (provider routing, URL normalization, expiry preflight, HTTP timeouts)#1156Soph wants to merge 1 commit intoalex/cli-auth-consolidationfrom
Soph wants to merge 1 commit intoalex/cli-auth-consolidationfrom
Conversation
…preflight, timeouts)
Six fixes called out across the security/architecture/correctness reviews
of #cli-auth-consolidation. Each fix lands with focused tests; no
behaviour changes outside the auth path.
1. Eliminate duplicate ENTIRE_AUTH_PROVIDER_VERSION read in
cmd/entire/cli/api/auth_tokens.go. The provider table in
cmd/entire/cli/auth.Provider now owns AuthTokensPath; api.Client
takes it via WithAuthTokensPath. The api package no longer reads
the env var.
2. Read provider version once at startup. CurrentProvider() resolves
via sync.Once and freezes; tests inject via SetProviderForTest.
resolveProvider is a pure function so the routing table is
exercisable without env-var gymnastics.
3. Normalize URLs in tokenmanager same-host / aud / cache-key compares.
normalizeOriginURL handles trailing slash, scheme/host case, and
default ports (RFC 3986 §6.2.2.1 / §6.2.3); non-URL audiences pass
through unchanged for byte-exact compare.
4. Preflight core-token expiry and clear the exchange cache on
SaveCoreToken. Long-expired tokens surface as ErrNotLoggedIn (so
"run login" UX kicks in) instead of confusing STS / 401 errors;
a re-login can't return the previous user's exchanged tokens.
5. Tighten tokenstore malformed-JSON detection. Well-formed JSON
without an access_token now surfaces as ErrMalformed. The shim's
bare-string fallback rejects JSON-shaped content via
looksLikeBareToken so "Authorization: Bearer {}" can't ship.
6. Add per-request timeouts (DefaultRequestTimeout = 30s) to
deviceflow.Client and sts.Client via context.WithTimeout. The wrap
lives at the method level so the deadline covers the body read,
not just the dial. Tests pin both the firing path and the
default/override resolution.
Entire-Checkpoint: c79c0ff7d6c1
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a set of follow-up fixes to the CLI authentication path (provider routing, token-store hardening, tokenmanager correctness, and network robustness) intended to address review feedback from #1153 without impacting non-auth behavior.
Changes:
- Centralizes provider-version routing (including auth-tokens endpoint base path) in
cmd/entire/cli/authand threads it intoapi.ClientviaWithAuthTokensPath, removing env-var reads fromapi/. - Hardens token resolution/storage behavior: URL normalization for equality/cache keys, core-token expiry preflight, exchange-cache invalidation on re-login, and stricter malformed-token detection (including legacy bare-string fallback filtering).
- Adds per-request HTTP timeouts (default 30s, configurable) to RFC 8628 device-flow and RFC 8693 STS clients with focused tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/logout.go | Routes logout token revocation through WithAuthTokensPath based on the active provider. |
| cmd/entire/cli/auth.go | Updates auth status/list/revoke flows to use provider-supplied auth-tokens base path. |
| cmd/entire/cli/auth/store.go | Adds JSON-shaped filtering to legacy bare-token fallback reads. |
| cmd/entire/cli/auth/store_test.go | Adds tests ensuring JSON-shaped values aren’t shipped as bearer tokens. |
| cmd/entire/cli/auth/provider.go | Introduces exported Provider, pure resolveProvider, and a cached CurrentProvider with test override. |
| cmd/entire/cli/auth/provider_test.go | Tests provider resolution, trimming, and test override seam. |
| cmd/entire/cli/auth/exchange.go | Switches manager construction to CurrentProvider() (single env read per process). |
| cmd/entire/cli/auth/client.go | Switches device-flow client wiring to CurrentProvider() fields. |
| cmd/entire/cli/api/client.go | Adds authTokensPath field and WithAuthTokensPath configuration hook. |
| cmd/entire/cli/api/auth_tokens.go | Removes provider env-var routing; errors clearly when auth-tokens path is unset. |
| cmd/entire/cli/api/auth_tokens_test.go | Refactors tests to configure auth-tokens path explicitly and asserts unset-path errors. |
| auth/tokenstore/keyring.go | Treats well-formed JSON without access_token as ErrMalformed. |
| auth/tokenstore/keyring_test.go | Adds coverage for empty/missing access_token malformed cases. |
| auth/tokenmanager/tokenmanager.go | Adds core-token expiry preflight, URL normalization for comparisons/cache keys, and clears exchange cache on core-token save. |
| auth/tokenmanager/tokenmanager_test.go | Adds tests for expiry preflight, URL normalization behavior, and cache invalidation on SaveCoreToken. |
| auth/sts/sts.go | Adds per-request timeout policy to STS exchanges (default 30s, configurable/disableable). |
| auth/sts/sts_test.go | Adds slow-loris/timeout tests and timeout-resolution unit tests. |
| auth/deviceflow/deviceflow.go | Adds per-request timeout policy to device-flow requests (default 30s, configurable/disableable). |
| auth/deviceflow/deviceflow_test.go | Adds slow-loris/timeout tests and timeout-resolution unit tests. |
Comment on lines
+195
to
+203
| got, err := NewStoreWithService(service).LoadTokens(profile) | ||
| // We expect ErrNotFound — JSON-shaped malformed entries must | ||
| // not be routed through the bare-string fallback. | ||
| if err == nil { | ||
| t.Fatalf("LoadTokens(%q) returned %+v; want ErrNotFound", body, got) | ||
| } | ||
| if got.AccessToken != "" { | ||
| t.Fatalf("LoadTokens(%q) AccessToken = %q, want empty", body, got.AccessToken) | ||
| } |
Comment on lines
+315
to
+323
| // normalizeOriginURL canonicalises an origin URL for equality | ||
| // comparisons. RFC 3986 §6.2.2.1 makes scheme and host case-insensitive | ||
| // and §6.2.3 makes the empty path equivalent to "/" — we collapse to | ||
| // no-trailing-slash. Default ports (80/http, 443/https) are stripped. | ||
| // | ||
| // On parse failure (or when the input lacks a scheme or host — common | ||
| // for non-URL audiences) the input is returned unchanged so callers | ||
| // fall back to byte-exact comparison. | ||
| func normalizeOriginURL(raw string) string { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/330
Summary
Six fixes called out across the security / architecture / correctness reviews of #1153. Each lands with focused tests; no behaviour change outside the auth path. Targets
alex/cli-auth-consolidationso it can ride along with that PR (rebase before merging the parent, or merge this first into the parent branch).Fixes
ENTIRE_AUTH_PROVIDER_VERSIONread incmd/entire/cli/api/auth_tokens.go. Provider table now ownsAuthTokensPath;api.Clienttakes it viaWithAuthTokensPath. The api package no longer reads the env var.CurrentProvider()resolves viasync.Onceand freezes; tests inject viaSetProviderForTest.resolveProvideris a pure function so the routing table can be exercised without env-var gymnastics.normalizeOriginURLhandles trailing slash, scheme/host case, and default ports (RFC 3986 §6.2.2.1 / §6.2.3). Non-URL audiences pass through unchanged for byte-exact compare.SaveCoreToken. Long-expired tokens surface asErrNotLoggedIn(so the "run login" UX kicks in) instead of confusing STS / 401 errors. A re-login can't return the previous user's exchanged tokens — defence-in-depth against future cache-key refactors.access_token(e.g.{}or an unrelated CLI's blob) now surfaces asErrMalformed. The shim's bare-string fallback rejects JSON-shaped content vialooksLikeBareTokensoAuthorization: Bearer {}can't ship.DefaultRequestTimeout = 30s) todeviceflow.Clientandsts.Client. Wrap lives at the method level so the deadline covers the body read, not just the dial. Tests pin both the firing path and the default/override resolution.Test plan
mise run fmt && mise run lintcleanmise run test:ciall green (unit + integration + canary E2E)TestSetProviderForTest_OverridesCurrentProvider,TestResolveProvider_*(Move "clean" root command to "session cleanup", some refactoring #2)TestClient_AuthTokens_RoutesV2Path,TestClient_AuthTokens_UnsetPathErrors(use github runners #1)TestNormalizeOriginURL,TestToken_SameHostShortcut_NormalisesURLs,TestToken_CacheCollapsesURLEquivalents(add .entire/logs to .entire/.gitignore, small refactor #3)TestToken_ExpiredCoreReturnsNotLoggedIn,TestToken_OpaqueCorePassesPreflight,TestSaveCoreToken_ClearsExchangeCache(Remove start command #4)TestKeyring_LoadTokens_EmptyAccessTokenReturnsErrMalformed,TestStoreLoadTokens_RejectsJSONShapedFallback,TestStoreGetToken_RejectsJSONShapedFallback(use strings.Contains #5)TestExchange_RequestTimeoutFires,TestPollDeviceAuth_RequestTimeoutFires,TestStartDeviceAuth_RequestTimeoutFires,TestRequestTimeout_DefaultAndOverride(Improved entire enable messaging #6)🤖 Generated with Claude Code
Note
Medium Risk
Touches authentication/token acquisition and storage paths (provider routing, keyring decoding, token caching/expiry) which can affect login/logout and bearer usage, though changes are defensive and well-covered by new tests.
Overview
Improves auth robustness by centralizing provider-version routing in
auth.CurrentProvider()(single env read viasync.Once) and havingapi.Clienttake an explicitAuthTokensPathviaWithAuthTokensPath, with clear errors when the path isn’t configured.Hardens token resolution by normalizing origin URLs for same-host/audience/cache-key comparisons, preflighting core-token JWT expiry to return
ErrNotLoggedInearlier, and clearing the exchange cache onSaveCoreToken.Strengthens credential safety by treating keyring JSON missing
access_tokenasErrMalformedand preventing the legacy bare-string fallback from shipping JSON-shaped blobs as a bearer token.Adds per-request timeouts (
DefaultRequestTimeoutwith per-client override/disable) to device-flow and STS HTTP calls to avoid slow-loris hangs, with targeted tests for timeout behavior and policy resolution.Reviewed by Cursor Bugbot for commit 6a9e601. Configure here.