[COR-174] auth: shareable auth library + split-host config + RFC 8693 token exchange#1153
[COR-174] auth: shareable auth library + split-host config + RFC 8693 token exchange#1153khaong wants to merge 23 commits into
Conversation
Introduces github.com/entireio/cli/auth as a shared OAuth client library for the Entire CLI. Three subpackages ship in this commit: * auth/tokens — TokenSet bundle plus unverified JWT claim parsing * auth/tokenstore — Store interface plus an OS-keyring reference impl * auth/deviceflow — RFC 8628 OAuth Device Authorization Grant client The packages are deliberately provider-agnostic: every server-specific value (endpoint paths, client_id, scope) is supplied at construction. The library has no global state, no implicit URLs, and no provider detection. It is intended to be importable by any RFC 8628 / RFC 8693 caller. No existing callers are wired up in this commit; the cmd/entire/cli shim swap follows separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the bespoke device-flow client and keyring store in cmd/entire/cli/auth with thin wrappers over auth/deviceflow and auth/tokenstore. The package's exported API (NewClient, NewStore, DeviceAuthStart, DeviceAuthPoll, LookupCurrentToken, etc.) is preserved field-for-field so login.go / logout.go / auth.go don't need to change. Two wrapper concerns worth noting: 1. PollDeviceAuth maps the shared library's RFC 8628 §3.5 sentinel errors back to the wire-side error code in DeviceAuthPoll.Error. This keeps the existing polling loop in login.go (which switches on result.Error) working unchanged. 2. Store.GetToken keeps a backward-compatibility fallback for keyring entries written before this commit, which stored bare access-token strings rather than JSON-encoded TokenSets. SaveToken always writes the new shape; GetToken transparently handles both. The legacy decodeJSON / decodeJSONStrict tests are removed; equivalent coverage now lives in auth/deviceflow tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a transition-period env-var switch that picks between two device-flow configurations: v1 (default): /oauth/device/code + /oauth/token, client_id="entire-cli" v2 : /api/auth/oauth/device/code + /api/auth/token, client_id="cli" Both surfaces speak the same RFC 8628 protocol; only the paths and client_id differ. Default behaviour is unchanged. Setting ENTIRE_AUTH_PROVIDER_VERSION=v2 (alongside an appropriate ENTIRE_API_BASE_URL) opts a user into the next-generation surface early. Unrecognised values fall back to v1 so old binaries stay safe if a future v3 ever ships. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the fourth subpackage of the auth/ library: a small, provider- agnostic client for RFC 8693 token exchange. Caller supplies BaseURL, Path, and per-call ExchangeRequest fields (SubjectToken, SubjectTokenType, RequestedTokenType, plus optional Audience/Resource/Scope and an Extra url.Values for any non-standard form fields the server expects). The package defines constants only for RFC 8693's standard token-type URIs and the token-exchange grant_type — the requested-token-type URI is always caller-supplied. Returns *tokens.TokenSet on success with absolute ExpiresAt; wraps RFC 6749 / 8693 error responses with both code and description. Tests cover happy path, optional-field omission, Extra forwarding, standard-fields-override-Extra precedence, missing required fields, JSON and non-JSON server errors, missing access_token, and the no- expiry case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captive portals, corporate proxies, and VPN firewalls (Cloudflare WARP,
etc.) commonly intercept the OAuth endpoint and return a 200 OK with an
HTML error page. Today the JSON decoder produces an opaque error like:
start login: decode device auth start response: decode JSON response:
invalid character '<' looking for beginning of value
That tells the user nothing actionable. Now both auth/deviceflow and
auth/sts surface:
could not reach authentication server: server returned non-JSON
response (check VPN, proxy, or firewall — e.g. Cloudflare WARP)
Implementation lives in a new internal package auth/internal/oauthhttp.
Both deviceflow and sts now run their successful-response bodies
through oauthhttp.ReadAndDecodeJSON, which sniffs for a leading '<'
(after trimming whitespace) and returns a typed ErrNonJSONResponse
sentinel — callers can errors.Is when they want to branch, or just let
the message bubble up.
Tests cover the helper in isolation plus end-to-end paths through both
StartDeviceAuth, PollDeviceAuth, and Exchange.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The token endpoint's error response carries an optional human-readable
error_description alongside the standard error code (RFC 6749 §5.2,
inherited by RFC 8628 §3.5). The lib was decoding only the code, which
collapsed several distinct invalid_grant flavours — "device_code unknown"
vs "client_id does not match grant" vs already-consumed replay — into a
single opaque "device authorization failed: invalid_grant" at the CLI.
Pull through:
* auth/deviceflow now decodes both fields on a non-2xx response and
wraps the sentinel error as fmt.Errorf("%w: %s", sentinel, desc) so
errors.Is(err, ErrInvalidGrant) keeps matching while the message
retains the description.
* cmd/entire/cli/auth.DeviceAuthPoll grows an ErrorDescription field;
the shim extracts it from the wrapped sentinel.
* cmd/entire/cli/login.go appends ": <description>" to the user-facing
failure message when the server provided one.
Two new deviceflow tests cover the description-present and
description-absent paths (no trailing colon-space when absent).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…version The auth-tokens endpoint family lives at different paths on the two backends — historical /api/v1/auth/tokens vs the consolidated /api/auth/tokens. ENTIRE_AUTH_PROVIDER_VERSION already gates the device-flow path split; auth_tokens.go now reads the same env var to pick its base path. ListTokens, RevokeToken, and RevokeCurrentToken all flow through one authTokensBasePath() helper so future paths land in one place. The env-var name is duplicated as a constant rather than imported from cmd/entire/cli/auth: api/ is a leaf package and shouldn't take a dependency on auth/ for routing. Both reads must stay in sync; flagged in a comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Across the auth/ library and customer-CLI shim, golangci-lint flagged a
fistful of routine findings that the existing files inherited or that
my recent commits introduced. None are correctness bugs; just noise
that the repo's strict configuration wants explicit suppression for.
* auth/deviceflow/deviceflow_test.go and auth/sts/sts_test.go grow a
shared writeBody(t, w, body) helper, replacing every `_, _ = io.WriteString`
in test fixtures. errcheck-clean without per-callsite nolints.
newTestClient drops its unused *httptest.Server return (unparam).
* auth/sts/sts.go suppresses gosec G101 on the three RFC 8693 standard
URI constants (GrantTypeTokenExchange, SubjectTokenType*) and
errcheck on the best-effort body read in readAPIError.
* auth/tokenstore/keyring.go suppresses gosec G117 on the json.Marshal
call that intentionally serialises the access token into the
OS-keyring entry (encrypted at rest by the OS).
* cmd/entire/cli/api/auth_tokens.go suppresses G101 on
authTokensProviderVersionEnvVar — env-var name, not a credential.
* cmd/entire/cli/auth/provider.go suppresses G101 on the v1/v2 entries
in the providers map (OAuth client_id and endpoint paths, not
credentials).
* cmd/entire/cli/auth/provider_test.go extracts wantClientIDV1 /
wantClientIDV2 test-local constants to satisfy goconst, then uses
them in every comparison.
cmd/entire/cli/auth/{client,store}.go also need nolint:wrapcheck
comments on the four shim returns — those changes sit in the working
tree alongside the in-progress AuthBaseURL refactor and will go in
together with that commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lets the CLI talk to deployments where the auth issuer and data API
live on different origins (e.g. us.console.partial.to mints tokens that
are then exchanged for partial.to-scoped tokens before each data-API
call).
Split-host plumbing:
- New ENTIRE_AUTH_BASE_URL env var; AuthBaseURL() falls back to
BaseURL() so single-host deployments are unchanged.
- Tokens are keyed in the keyring by the auth issuer (the host that
minted them), not by the data API URL.
- Auth-management commands (auth list/revoke/status/logout) hit the
auth host via NewClientWithBaseURL since their endpoints live there.
- Align v2 client_id to "entire-cli" to match v1.
New shareable library auth/tokenmanager:
- Provider-agnostic orchestration over auth/sts: cache, JWT-aud
shortcut, exchange dispatch.
- Config struct takes Issuer, ClientID, STSPath, Store, plus defaults
and test hooks. No globals, no env-var reads, no implicit URLs —
ready to share with other internal CLIs.
- TokenForResource/Token resolve to:
1) ErrNotLoggedIn when the store is empty,
2) core token verbatim when issuer == resource,
3) core token verbatim when its aud claim already includes the
resource (multi-audience tokens skip exchange),
4) RFC 8693 exchange otherwise, cached per (core, resource,
audience, requested-token-type, scope) until expiry.
CLI wiring:
- NewAuthenticatedAPIClient now takes ctx and routes through
tokenmanager so data-API calls carry the right-audience bearer.
All 7 callers updated to pass ctx.
- cmd/entire/cli/auth/exchange.go is a thin shim that builds a
package-level Manager from the active provider + NewStore() and
exposes TokenForResource / Token / ErrNotLoggedIn.
- *Store now implements tokenstore.Store so it can be passed to the
Manager, preserving the legacy bare-string keyring fallback.
Fix discovered along the way:
- search defaulted to a hardcoded entire.io serviceURL; now defaults
to api.BaseURL() when ENTIRE_SEARCH_URL is unset.
Misc gofmt/lint autofixes in auth/deviceflow, auth/sts, auth/tokens
that the linter applied while iterating.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nager Two fixes that came out of getting `entire trail list` working against partial.to's split-host deployment: - Provider config now carries an stsPath alongside the OAuth token endpoint. v2's STS lives at /api/authz/sts/token, distinct from the /api/auth/token OAuth endpoint that rejects token-exchange grants with unsupported_grant_type. cmd/entire/cli/auth/exchange.go now passes provider.stsPath (rather than provider.tokenPath) into the tokenmanager. - v1 is the legacy single-host surface (entire.io for both auth and data API), so the same-host shortcut in tokenmanager.Token always wins and STS is never invoked. v1.stsPath is left empty. - tokenmanager.Config.STSPath is now optional. New() no longer rejects empty STSPath; runExchange() returns the new ErrNoSTSPath sentinel if an exchange is actually attempted with no path configured. Single-host setups (incl. v1) need no STS endpoint; split-host misconfigurations fail loudly at the right layer. Tests updated to cover empty-STSPath construction, the ErrNoSTSPath path, and v1's empty stsPath contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`entire search` was sending the raw core token to the search service, which on split-host deployments has the wrong audience (auth host issuer, not the data API). Switch to auth.TokenForResource(ctx, serviceURL) so the bearer is exchange-resolved against the search service URL: same-host shortcut keeps single-host setups unchanged, split-host setups now get an exchanged token with aud=entire-api. Also moves the auth lookup after the git/repo plumbing so the resource URL (which can come from ENTIRE_SEARCH_URL or api.BaseURL()) is known at the time we resolve the bearer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review surfaced a real correctness bug plus a handful of clarity/coverage gaps. This commit fixes them in one pass. Critical fix — Store.LoadTokens legacy bare-string fallback was dead code: - tokenstore.Keyring.LoadTokens returned "unmarshal TokenSet: ..." for pre-shim bare-string entries, not ErrNotFound. The cmd-side shim's fallback only fired on ErrNotFound, so users with pre-shim keyring entries appeared logged out after upgrading to the manager-backed code path (entire trail/search/etc.). The legacy GetToken path was separately over-permissive: it fell back on any error, masking real keyring errors. - Add tokenstore.ErrMalformed sentinel returned (wrapped) by decodeTokenSet on JSON unmarshal or expires_at parse failures. - Update Store.LoadTokens / Store.GetToken to fall back precisely on ErrMalformed (legacy path) and surface ErrNotFound + real keyring errors verbatim. Regression tests pre-seed bare-string keyring entries and assert the round-trip. api.bearerTransport: reject empty bearer at first request rather than sending Authorization: Bearer<space> on the wire (which produces a confusing 401). New errEmptyBearerToken sentinel. api/auth_tokens: add table-driven test that pins the ENTIRE_AUTH_PROVIDER_VERSION → path mapping (v1/v2/unrecognised/ whitespace) plus an end-to-end ListTokens routing check. The path switch is the whole point of the version env var; it had no test. Doc fixes (review found these stale or misleading): - auth/doc.go: list tokenmanager subpackage (was missing). - auth/tokenstore/tokenstore.go: drop the "File impl" claim — only Keyring ships today. - auth/tokenstore/keyring.go: collapse the duplicated keyringTokenSet comment paragraph, drop the dangling G117 reference. - cmd/entire/cli/auth/exchange.go: defaultManager rationale corrected (sync.Once means later env-var changes are ignored, not honoured); TokenForResource doc points at Manager.Token (the rules live there, not on TokenForResource). - cmd/entire/cli/api_client.go: cache-key undercount — list all wire-affecting fields rather than just (core-token, resource). - cmd/entire/cli/search_cmd.go: rewrite the misleading "fall back to search.DefaultServiceURL" comment (the fallback is api.BaseURL()). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ions) Behaviour: - tokenmanager.DeleteCoreToken now deletes the keyring entry first and only clears the in-memory exchange cache on success. Pre-emptively clearing would leave a window where the CLI thinks it's logged out but the keyring still hands out the core token to the next process. Surfaces the store error wrapped as "delete core token: ...". Coverage: - tokenmanager: regression tests for the cache-clear (and its inverse — cache survives a failed delete), cache-key independence for RequestedTokenType and Scope (matching the existing Audience test), malformed-JWT fallthrough on the audience shortcut (security contract — corrupt cores must not be returned verbatim), and surface-don't-collapse for non-ErrNotFound store errors. Adds an erroringStore test helper for failure-path tests. - tokenstore.Keyring: pin the ErrMalformed contract — malformed JSON, legacy bare-string entries, and bad expires_at all surface as ErrMalformed (wrapped), not ErrNotFound. cmd-side legacy fallback depends on this distinction. Deprecations / docs: - Mark cmd/entire/cli/auth.Store.SaveToken/GetToken/DeleteToken as // Deprecated so godoc and IDE hover steer new callers to the tokenstore.Store interface methods. Legacy direct-bearer call sites (login, logout, auth status/list/revoke) keep using them; login.go carries a //nolint:staticcheck with a pointer to the doc. - Document keyringService = "entire-cli" as immutable — renaming would orphan every existing user's stored credentials. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a provider-agnostic, shareable auth library (auth/…) and migrates the CLI to use it, enabling split-host deployments (separate auth issuer and data API origins) via RFC 8693 token exchange.
Changes:
- Added a new root-level auth library implementing RFC 8628 device flow, RFC 8693 token exchange, token persistence, JWT claim parsing, and a token manager with caching/exchange orchestration.
- Added split-host configuration (
ENTIRE_AUTH_BASE_URL) and updated CLI auth/token storage and auth-management endpoints to route to the auth origin. - Updated CLI API client creation and
entire searchto resolve resource-scoped tokens (including URL-origin normalization) and threadcontext.Contextthrough authenticated client creation.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/trail_cmd.go | Passes ctx into authenticated API client creation. |
| cmd/entire/cli/search/search.go | Clarifies GitHubToken field meaning for backwards compatibility. |
| cmd/entire/cli/search_cmd.go | Routes search via token manager; defaults service URL to api.BaseURL(); normalizes resource origin. |
| cmd/entire/cli/search_cmd_test.go | Adds coverage for search service URL → origin normalization. |
| cmd/entire/cli/recap.go | Passes ctx into authenticated API client creation. |
| cmd/entire/cli/logout.go | Routes logout revocation calls to auth base URL. |
| cmd/entire/cli/login.go | Uses legacy token save shim; surfaces RFC 8628 error_description on failures. |
| cmd/entire/cli/integration_test/login_test.go | Sets auth base URL + provider version for login integration tests. |
| cmd/entire/cli/dispatch_wizard.go | Passes ctx into authenticated API client creation. |
| cmd/entire/cli/auth/store.go | Wraps shared tokenstore.Keyring while preserving legacy bare-token fallback reads; keys tokens by auth issuer. |
| cmd/entire/cli/auth/store_test.go | Adds tests for legacy bare-string fallback behavior. |
| cmd/entire/cli/auth/provider.go | Introduces provider version switch (v1/v2) including per-provider STS path. |
| cmd/entire/cli/auth/provider_test.go | Tests provider selection and client wiring. |
| cmd/entire/cli/auth/exchange.go | Adds CLI shim over tokenmanager.Manager (singleton + test injection). |
| cmd/entire/cli/auth/exchange_test.go | Tests shim delegation and ErrNotLoggedIn aliasing. |
| cmd/entire/cli/auth/client.go | Replaces bespoke device-flow client logic with shared auth/deviceflow client + compatibility shim types. |
| cmd/entire/cli/auth/client_test.go | Removes tests for old JSON decoding helpers (now in shared oauthhttp helpers). |
| cmd/entire/cli/auth.go | Validates both API and auth origins for HTTPS; routes auth-management commands to auth base URL. |
| cmd/entire/cli/api/client.go | Adds NewClientWithBaseURL; fails early on empty bearer tokens at first request. |
| cmd/entire/cli/api/client_test.go | Adds test ensuring empty bearer tokens are rejected. |
| cmd/entire/cli/api/base_url.go | Adds ENTIRE_AUTH_BASE_URL and AuthBaseURL() fallback to BaseURL(). |
| cmd/entire/cli/api/base_url_test.go | Adds tests for AuthBaseURL() fallback/override behavior. |
| cmd/entire/cli/api/auth_tokens.go | Routes auth-token endpoints by provider version; documents v1/v2 differences. |
| cmd/entire/cli/api/auth_tokens_test.go | Adds tests for provider-version routing and v2 path selection. |
| cmd/entire/cli/api_client.go | Reworks authenticated API client creation to use token manager + RFC 8693 exchange when needed. |
| cmd/entire/cli/activity_cmd.go | Passes ctx into authenticated API client creation. |
| auth/tokenstore/tokenstore.go | Defines persistence interface and sentinel errors (ErrNotFound, ErrMalformed). |
| auth/tokenstore/keyring.go | Implements keyring-backed tokenstore.Store with JSON TokenSet encoding and malformed-entry signaling. |
| auth/tokenstore/keyring_test.go | Adds tests for keyring round-trips and malformed-entry behavior. |
| auth/tokens/tokens.go | Introduces TokenSet + unverified JWT claims parsing helpers. |
| auth/tokens/tokens_test.go | Adds tests for expiry helpers and JWT claim parsing. |
| auth/tokenmanager/tokenmanager.go | Adds manager orchestration: core token lookup, same-host / aud shortcuts, RFC 8693 exchange, caching. |
| auth/tokenmanager/tokenmanager_test.go | Adds comprehensive tests for exchange dispatch, caching, shortcuts, and failure modes. |
| auth/sts/sts.go | Adds RFC 8693 token-exchange client. |
| auth/sts/sts_test.go | Adds tests for form construction, error surfacing, and expiry handling. |
| auth/internal/oauthhttp/jsonresp.go | Adds shared JSON response decoding with HTML/non-JSON detection. |
| auth/internal/oauthhttp/jsonresp_test.go | Tests strict/tolerant decoding and HTML detection behavior. |
| auth/doc.go | Documents the new auth library package structure and goals. |
| auth/deviceflow/deviceflow.go | Adds RFC 8628 device-flow client with sentinel error mapping and strict/tolerant decoding rules. |
| auth/deviceflow/deviceflow_test.go | Adds tests for device-flow success/error behaviors, including error_description handling. |
`entire dispatch` was sending the raw core token (audience = auth host) to the data API and getting back a 401 that cloud.go mapped to "dispatch requires login — run \`entire login\`" — misleading on split-host deployments where the user IS logged in but with the wrong-audience bearer. Same trap search hit before; same fix. - mode_cloud.go now resolves the bearer via auth.TokenForResource so the tokenmanager's same-host shortcut / JWT-aud shortcut / RFC 8693 exchange all apply. ErrNotLoggedIn is mapped to the friendly "dispatch requires login" message; other errors surface verbatim. - mode_local.go grows a lookupResourceToken seam (defaulted to auth.TokenForResource) for test injection; the existing lookupCurrentToken seam is retained for back-compat with tests that haven't migrated. - Test stubs (stubCloudDispatchAuth + per-test cleanups) updated to swap both seams so the assertions still cover what they used to. Sweep confirmed no other data-API caller bypasses the manager: search, trail, recap, dispatch_wizard, and activity all flow through NewAuthenticatedAPIClient → tokenmanager. Auth-host commands (auth list/revoke/status, logout) correctly retain LookupCurrentToken since they need the auth-audience bearer. Docs: - New CLAUDE.md "Auth and token resolution" section flags the two blessed entry points (NewAuthenticatedAPIClient, TokenForResource), the resolution rules, and that LookupCurrentToken is for auth-host callers only. - New auth/README.md positions the library as shareable across internal CLIs: subpackage map, embedding checklist, design principles (no globals, no env-var reads, provider-agnostic), non-goals (OIDC discovery, server-side, code-flow PKCE), quick- start snippets for login / data-API call / logout. - search.Config.GitHubToken doc now points at TokenForResource (was LookupCurrentToken). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- PollDeviceAuth: unknown OAuth error codes (invalid_request, invalid_client, server_error, unsupported_grant_type, etc.) used to fall through to login.go's transient-retry path, burning ~25-150s on permanent server failures before producing a confusing "after N consecutive failures" message. Replace oauthErrorCode + descriptionFromSentinel with a single oauthErrorParts that also matches deviceflow's generic "oauth error: <code>" wrapper. Unknown codes now land in DeviceAuthPoll.Error so the polling loop's default switch arm fails fast with "device authorization failed: <code>". Tests cover known sentinels, sentinel-with-description, unknown- passthrough, unknown-with-description, and non-OAuth (transient) errors. - Store.GetToken: doc said "only ErrNotFound and ErrMalformed trigger the fallback" but ErrNotFound short-circuits to the empty-string return without a keyring read; only ErrMalformed actually triggers the bare-string fallback. Fixed the doc to match. - api.RevokeCurrentToken: dropped the stale comment about v2 not exposing /current — server-side fix is incoming. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
bugbot run |
Two cursor bug-bot findings, both Low severity but worth closing. - auth/deviceflow + auth/sts: TestPollDeviceAuth_Success and TestExchange_Success call freezeClock, which mutates the package- level nowFunc. Both tests were marked t.Parallel(), creating a latent race against any future parallel test that reads nowFunc through a real Exchange/PollDeviceAuth call. Drop the t.Parallel() on those two tests with a comment explaining why; the rest of the package keeps parallelism. -race confirms no race remains. - auth/tokenmanager: cacheKey was a delimiter-joined string, structurally vulnerable to collisions if any field embedded the "|" separator (none do today, but no guarantee for future callers). Replace with a struct map key — Go's map can use comparable structs directly, so there's no string encoding to misbehave. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9c2b070. Configure here.
…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
auth: review follow-ups (provider routing, URL normalization, expiry preflight, HTTP timeouts)
…dation # Conflicts: # cmd/entire/cli/auth/store.go # cmd/entire/cli/integration_test/login_test.go # cmd/entire/cli/recap.go
Four review-surfaced findings, all defense-in-depth on top of server-side validation: 1. Reject JWTs with alg:none header (RFC 7515 / RFC 7518 §3.6 known attack vector). tokens.ParseClaims now decodes the JWT header and returns tokens.ErrUnsignedJWT for any case-insensitive "none" variant. The CLI's use of ParseClaims is documented as unverified and only feeds routing decisions, but a future caller could be tempted to rely on the values — rejecting the unsigned shape at the source keeps that door closed. New makeJWTWithHeader test helper lets us produce well-formed JWTs with specific issues (alg variants, expired exp, etc.) without a real JOSE library. Five regression tests cover lowercase / capitalised / uppercase / whitespace-padded "none" + a sanity check that standard algs (HS256, RS256, ES256, EdDSA, PS512) still parse. 2. Enforce HTTPS on STS exchange and device-flow endpoints. Both sts.Client and deviceflow.Client now reject http:// BaseURLs unless AllowInsecureHTTP is set; new ErrInsecureBaseURL sentinel on each. The cmd-side wires this to auto-permit only loopback http:// (localhost, 127.0.0.1, ::1) via new isLoopbackHTTP helper in cmd/entire/cli/auth/exchange.go and the deviceflow client constructor — production never qualifies, local dev does. tokenmanager.Config.AllowInsecureHTTP plumbs the flag through to the sts.Client at exchange time. 3. Validate verification_uri before showing it to the user. The device-code response field is what we echo and open in the user's browser — a malicious AS pointing it at a phishing page is a direct credential-harvesting vector. New ErrUnsafeVerificationURI sentinel rejects: missing/non-URL, non-https (loopback http only), embedded userinfo (user:pass@host eye-trick), and control characters in the URI string. Tests cover both the safe shapes (https with port / path / query, loopback http) and the unsafe ones (ftp/javascript/data schemes, plain http on non-loopback, newline injection, control chars). 4. Validate received token in login.go before persisting. New validateReceivedToken runs minimum-trust checks on the access token: rejects alg:none (via ParseClaims), iss-mismatch against the issuer we sent the device-code request to, and already- expired exp. Opaque (non-JWT) tokens are allowed — the AS may not issue JWTs at all. Omitted iss is allowed (some servers skip it) but a non-empty mismatch is hard-rejected. Seven unit tests cover the matrix. Also merges in 164 commits from origin/main with conflict resolution in auth/store.go (preserved both the tokenBackend abstraction for the authfilestore test build and the tokenstore.Store interface for the tokenmanager), recap.go (newRecapClient now goes through auth.TokenForResource so split-host setups work), trail_watch_cmd.go (ctx threading), and integration_test/login_test.go (both ENTIRE_AUTH_BASE_URL/ENTIRE_AUTH_PROVIDER_VERSION and ENTIRE_TEST_AUTH_STORE_FILE env vars now set). Soph's earlier defense — rejecting JSON-shaped values in the keyring fallback so corruption never ships as a Bearer header — is also preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pulls the entire auth library out of this repo and into its own module at https://github.com/entireio/auth-go (tagged v0.1.0). The CLI's local auth/ directory is gone; cmd/entire/cli/auth and friends now import the library from github.com/entireio/auth-go. Extraction used `git subtree split --prefix=auth` against this branch, so the new repo's history is the 14 auth/-touching commits from the PR plus an extraction commit (import-path rewrite + go.mod) and an MIT license commit. CLI side: - All "github.com/entireio/cli/auth/<pkg>" imports rewritten to "github.com/entireio/auth-go/<pkg>". - go.mod gains the auth-go dep at v0.1.0; deviceflow/sts/tokenmanager/ tokenstore/tokens drop out of the indirect set. - Local auth/ directory deleted. Follow-ups (separate PRs): - Once auth-go gets its own CI green check, tighten the tagged version here to a more precise constraint. - Constructor parity for auth-go's deviceflow.Client / sts.Client (deferred from earlier review) can now land in the auth-go repo as a v0.2.0 API tightening, and the CLI updates when ready. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8821a55. Configure here.
| if errors.Is(err, auth.ErrNotLoggedIn) { | ||
| token = "" | ||
| err = nil | ||
| } |
There was a problem hiding this comment.
Recap wraps STS exchange errors as keyring errors
Medium Severity
newRecapClient wraps all non-ErrNotLoggedIn errors from auth.TokenForResource as &keyringReadError{Cause: err}. Previously this was correct because auth.LookupCurrentToken only touched the keyring. Now auth.TokenForResource can also fail during RFC 8693 token exchange (STS network errors, protocol errors) or manager construction. The caller in runRecap matches errors.As(err, &keyringErr) and shows "Could not read your auth token from the system keyring … Check your OS keychain settings" — completely misleading for STS failures in split-host deployments.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8821a55. Configure here.
| u.RawQuery = "" | ||
| u.Fragment = "" | ||
| return u.String(), nil | ||
| } |
There was a problem hiding this comment.
Issuer normalization strips full path, not just trailing slash
Low Severity
normalizeIssuer is documented as "trim trailing slashes" in issMatches, but it actually strips the entire Path, RawQuery, and Fragment from URLs. A JWT with iss: "https://issuer.example/different-tenant" would pass validation against expected issuer "https://issuer.example" because both normalize to the same origin. This weakens the defense-in-depth issuer mismatch check beyond its stated intent.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8821a55. Configure here.
| prev := managerForTest | ||
| managerForTest = mgr | ||
| return func() { managerForTest = prev } | ||
| } |
There was a problem hiding this comment.
SetManagerForTest lacks mutex unlike SetProviderForTest
Low Severity
managerForTest is read and written without synchronization in both SetManagerForTest and defaultManager. The analogous providerForTest in provider.go properly uses providerTestMu for all accesses. This inconsistency means concurrent test goroutines calling defaultManager() while SetManagerForTest runs could hit a data race, even though current tests avoid it by being non-parallel.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8821a55. Configure here.
| // cloud-mode runner now resolves its bearer via lookupResourceToken | ||
| // so it picks up the RFC 8693 exchange in split-host deployments; | ||
| // existing tests that swap lookupCurrentToken keep working because | ||
| // the default lookupResourceToken delegates to it. |
There was a problem hiding this comment.
Dead lookupCurrentToken variable with misleading comment
Low Severity
lookupCurrentToken is declared and initialized but never called in production code — mode_cloud.go now uses lookupResourceToken exclusively. The comment claims "existing tests that swap lookupCurrentToken keep working because the default lookupResourceToken delegates to it," which is incorrect: auth.TokenForResource and auth.LookupCurrentToken are independent functions. Tests actually work because they set both variables. The misleading comment could cause future test authors to only set lookupCurrentToken, which would have no effect.
Reviewed by Cursor Bugbot for commit 8821a55. Configure here.


https://entire.io/gh/entireio/cli/trails/327
Summary
Linear: COR-174
Note
STS endpoint will be moving soon. v2 currently exposes the RFC 8693 token-exchange endpoint at
/api/authz/sts/token(encoded inprovider.go's v2 surface). Server-side work is in flight to relocate it; once that lands, the new path moves into the auth-go repo's provider config in this CLI and the CLI side is a one-line bump.Enables the partial.to staging deployment where the auth issuer (
us.auth.partial.to) and the data API (partial.to) live on different origins by introducing per-resource RFC 8693 token exchange, then extracts the resulting library out of this repo into a standalone module so other internal CLIs can adopt the same pattern.Library: now lives at github.com/entireio/auth-go (v0.1.0)
The
auth/directory in this repo is gone — its contents have been split out viagit subtree split --prefix=authinto a standalone MIT-licensed Go module with preserved history. The CLI now importsgithub.com/entireio/auth-go/{deviceflow,sts,tokens,tokenstore,tokenmanager}instead of carrying them inline.What that buys us:
go get github.com/entireio/auth-goand reuse the same OAuth plumbing instead of reimplementing it. No vendoring, no copy-paste drift.github.com/entireio/cli— every endpoint, identifier, and default value is supplied byConfig.Subpackages, all provider-agnostic:
deviceflowststokensalg:none)tokenstoretokenmanagerCLI-side wiring
ENTIRE_AUTH_BASE_URLenv var;AuthBaseURL()falls back toBaseURL()so single-host deployments are unchanged. Tokens are now keyed in the keyring by the auth issuer (the host that minted them). Auth-management commands (auth list/revoke/status/logout) routed to the auth host since their endpoints live there.NewAuthenticatedAPIClientroutes through the manager. Data-API calls obtain a resource-audience bearer via RFC 8693 exchange when the core token's audience doesn't already match. All 8 callers threadctx: activity, dispatch_wizard, recap, search, trail (×4 plus watch). Search additionally defaultsserviceURLtoapi.BaseURL()and normalizes path-bearing URLs to scheme+host before token resolution.entire dispatchwas the last command sending the raw core token to the data API; mode_cloud now resolves throughauth.TokenForResourcelike search/trail/recap.entire-cli(matched v1).stsPathper surface; v2's STS lives at/api/authz/sts/token. v1 is single-host, so itsstsPathis empty andtokenmanager.Config.STSPathis optional — the same-host shortcut wins. Misconfigured split-host setups fail loudly viaErrNoSTSPath.Security hardening (defense in depth)
Four findings landed in addition to the core split-host work, all on top of server-side validation:
alg:none—tokens.ParseClaimsdecodes the JWT header and refuses the RFC 7515 unsigned shape. Case-insensitive, trims whitespace. Even though the CLI's claim use is documented as routing-only, the unsigned shape would let a hostile AS bypass shape checks. Newtokens.ErrUnsignedJWTsentinel + 5 regression test cases.http://BaseURLs unlessAllowInsecureHTTPis set, which the CLI auto-permits only for loopback hosts (localhost, 127.0.0.1, ::1). Production never qualifies; local dev does. NewErrInsecureBaseURLsentinel on each.verification_uribefore showing it to the user or opening it in a browser. Rejects: non-URL, non-HTTPS (loopback http only), embedded userinfo (user:pass@hosteye-trick), and control characters. NewErrUnsafeVerificationURIsentinel + comprehensive tests on safe/unsafe shapes.login.gobefore persisting.validateReceivedTokenparses the JWT (if it is one), rejectsalg:none, hard-rejectsissmismatch vs. the configured issuer, and rejects already-expiredexp. Opaque (non-JWT) tokens are permitted — the AS may not issue JWTs at all.Plus a defense soph added earlier in PR #1156: the keyring fallback rejects JSON-shaped values so corruption never ships as a Bearer header.
Other behaviour fixes surfaced during review
Store.LoadTokenslegacy bare-string fallback was unreachable — it only fired onErrNotFound, but a pre-shim raw-token entry produced an unmarshal error. Addedtokenstore.ErrMalformedso callers can distinguish "no entry" from "entry exists but malformed" and route the legacy path correctly.tokenmanager.runExchangewas silently droppingreq.Resource—sts.ExchangeRequest.Resource(RFC 8693 §2.1) was never sent to the AS.tokenmanager.Tokennow gated on emptyAudienceso an explicit per-callAudiencealways forces an exchange (was silently downgraded if the core token'saudhappened to include the resource).tokenmanager.DeleteCoreTokenorder swapped: keyring delete first, in-memory cache clear only on success — pre-emptive clear created a window where the CLI thought it was logged out but the keyring still held the token.api.bearerTransportrejects empty bearer at first request rather than puttingAuthorization: Bearer<space>on the wire (which produced confusing 401s).PollDeviceAuthshim now surfaces unknown OAuth error codes throughDeviceAuthPoll.Errorso login's polling loop fails fast on terminal rejections instead of retrying ~5×.tokenmanagercache key is now a struct map key instead of a delimiter-joined string — structurally collision-free.freezeClockindeviceflow/ststests is no longert.Parallel()(latent race against any future parallel test readingnowFunc).Test plan
mise run fmt && mise run lintclean.go test ./...— only the two pre-existing main failures (TestGroupCommitsByDay_SortsNewestFirst,TestExplainCmd_PositionalArgConflictsWithFlags, plusTestExplainCmd_SummaryTimeoutSecondsValidationfrom a recent main commit) remain.go test -race ./...— no new races.entireio/auth-gorepo builds and tests pass standalone at the v0.1.0 tag.entire login,entire trail list,entire search,entire dispatchall working through the device flow → STS exchange → resource-scoped bearer chain.Notes for reviewers
main. Easiest review path is by commit. The meaningful new work starts atc492a54b1. Most-substantial commits:c492a54b1— split-host + tokenmanager (largest single change)a9aeb9e6f— dispatch routing fix + the CLAUDE.md "Auth and token resolution" section + the auth-go READMEdc7c003e4— soph's review follow-ups (PR auth: review follow-ups (provider routing, URL normalization, expiry preflight, HTTP timeouts) #1156, already merged via merge commit)706a4a28e— defense-in-depth security hardening (the four findings above)8821a55ae— subtree extraction intoentireio/auth-go; CLI imports rewritten, localauth/deletedauth-go'sdeviceflow.Client/sts.Client, movingtokenmanager.Config.Now/Exchangeoff the public Config, renamingTokenRequest.Resource→ResourceURL, typed-error replacement ofdescriptionFromSentinel) now ship asauth-gov0.2.0 work, independent of this CLI PR.🤖 Generated with Claude Code
Note
High Risk
High risk because it changes how the CLI resolves, stores, and uses bearer tokens (including new split-host routing and token exchange), affecting authentication and every authenticated API call path.
Overview
Enables split-host deployments by adding
ENTIRE_AUTH_BASE_URL(auth issuer) alongsideENTIRE_API_BASE_URL(data API), updating token storage/lookups to key off the auth origin, and validating both origins for HTTPS.Moves token resolution for data-API callers to
auth.TokenForResource/cli.NewAuthenticatedAPIClient(ctx, ...), which can perform RFC 8693 token exchange and cache results; command flows likeactivity,search,dispatch,trail,recap, and completion paths are updated to threadcontext.Contextand use resource-scoped bearers.Refactors auth management endpoints to be provider-version routed via
auth.CurrentProvider()andapi.Client.WithAuthTokensPath, adds provider table + tests, replaces the in-repo device-flow client with anauth-go/deviceflowshim (including improved surfacing of OAuth error codes/descriptions), and adds defense-in-depth checks like rejecting empty bearer requests and validating login tokens (issuer/expiry/unsigned JWT).Reviewed by Cursor Bugbot for commit 8821a55. Configure here.