Add extend login / logout via OAuth browser sign-in - #31
Merged
Conversation
Implements the CLI side of the first-party OAuth login: RFC 8252
loopback redirect with PKCE S256 against {apiBase}/oauth2/*, RFC 8414
discovery with hardcoded fallback, and the required RFC 8707 resource
parameter set to the configured API base URL.
Tokens live in the OS keychain (file fallback under the config dir for
headless hosts), keyed per API base URL. Access tokens refresh silently
on expiry or a 401 (once, with retry), always persisting the rotated
refresh token. Auth precedence: env API key > config-file key > stored
login. Logout revokes the grant family and clears local state.
Replace the bare callback page with an Extend-styled card: inline logomark SVG (derived from the terminal logo geometry), embedded CSS, system fonts, no network fetches. Distinct copy for success, a user-denied consent (access_denied), and error variants, with the server-supplied error description HTML-escaped.
After the token exchange, call GET /me with the fresh access token and print "Signed in to <workspace> (<Production|Test>) as <email>." built from the resolved workspace, its granted environment, and the user email. Best-effort: any /me failure keeps the generic "Logged in to <base>." line and never fails the login (tokens are already stored).
Contributor
Author
|
D94 polish: branded landing pages + personalized success
|
The OAuth client id is migration-seeded as "extend-cli" identically in every environment, so a user-facing override has no legitimate use. Drop the env var, its help/usage text, and its env-var doc entry; the client id is always oauth.DefaultClientID.
Contributor
Author
|
fb165d5: removed the EXTEND_OAUTH_CLIENT_ID override entirely per D94 amendment; the OAuth client id is always the hardcoded "extend-cli". |
Any local process could previously GET /callback with a wrong or missing state and consume the one-shot result channel, aborting (or error-spoofing) a pending real login. The state check now gates every branch including error redirects: non-matching callbacks get a 404 page and never touch the channel, so only the real redirect resolves the flow. Also: respond 405 to non-GET methods, set ReadHeaderTimeout on the loopback server, and mark all landing pages Cache-Control: no-store.
RFC 8414 metadata was trusted wholesale: a redirected or poisoned well-known document could advertise a foreign token endpoint and receive the authorization code plus PKCE verifier. Every endpoint in a successfully fetched document must now be on the API base URL's exact host, over https (or the base's own scheme, which permits http only for local dev bases that are themselves http); anything else fails the login with an explicit rejection instead of falling back. Fetch failures still fall back to the /oauth2/* defaults as before.
…ections Two refresh-path fixes. First, the rotated token pair is now persisted before the new access token is handed out, with one retry; if both attempts fail, the source keeps working in memory (the rotation already happened server-side, so failing would strand the user harder) but warns loudly that a re-login may be needed instead of silently continuing or erroring with the pair trapped in RAM. Second, only a 400/401 carrying invalid_grant or invalid_client maps to ReauthError. Previously any 4xx did, so a 429 or 408 from the token endpoint told the user their login had expired; those (and 5xx, timeouts, network errors) now surface as plain retryable refresh failures that never clear or condemn the stored session.
The refresh single-flight was in-process only: two concurrent extend invocations each redeem the same rotate-on-use refresh token, and the loser trips the server's reuse detection, revoking the whole grant family. Refresh now takes a cross-process lock (an O_CREATE|O_EXCL lock file beside the fallback token store, with a stale-lock takeover for crashed holders) and, once acquired, re-reads the store: if another process already rotated the pair, its result is adopted without a second grant. Lock-machinery failures other than cancellation degrade to the in-process mutex rather than blocking refresh.
The workspace name and email from GET /me were printed to the terminal raw. Both are attacker-influenced (workspace names are user-set), so embedded ANSI CSI or OSC sequences could restyle the terminal or spoof output. Escape sequences are now stripped whole (including payloads), along with C0 controls, DEL, and the raw C1 range, before the values reach the success line.
- Token file writes fsync before the atomic rename so a crash cannot install a truncated file over the previous tokens. - Token file reads refuse to follow a symlinked oauth_tokens.json. - Token/revoke error bodies are capped at 64KB instead of 1MB. - The SDK token func now runs under the command's signal-aware context (plumbed via extendx.Config.TokenContext) instead of context.Background(), so Ctrl-C aborts an in-flight refresh; the SDK's WithTokenFunc signature takes no per-request context, so the command context is the closest available scope.
The default http.Client follows 307/308 redirects by replaying the request body, which on these calls carries the authorization code and PKCE verifier, a live refresh token, or the token being revoked. Give the discovery, exchange, refresh, and revoke clients a CheckRedirect that rejects any hop whose origin (scheme + host + port) differs from the configured API base, re-validating every hop of a chain.
Refreshing without the lock lets two CLI processes redeem the same rotate-on-use refresh token, tripping the server's reuse detection and revoking the whole family. Lock path or acquisition failures now fail the refresh (with a bounded wait for a held lock) instead of degrading to the in-process mutex alone. The lock also moves to a stable per-user path keyed by the normalized API base, so processes launched with different XDG_CONFIG_HOME still contend on the same lock while logins against different bases no longer serialize each other.
The general SDK client had no redirect policy: Go's client copies the Authorization header to same-registrable-host redirect targets, and the OAuth bearer transport re-attaches a fresh access token on every hop, so a 302 off the API origin (open redirect, compromised endpoint, misconfigured CDN) would hand the live bearer to whatever host the Location header names. Reuse the OAuth clients' CheckRedirect (now exported) to pin every hop to the configured base URL, falling back to the SDK's production default when none is set. The upload client swapped in by UploadOption carries the same header but has no view of the configured base, so it refuses redirects outright; the eval runner's judge client gets the same origin pin for its Anthropic key, which Go does not strip on cross-host redirects.
Nothing forced https for a remote base URL: pointing EXTEND_BASE_URL (or the config file) at http://some.host ran the whole OAuth handshake — PKCE verifier, authorization code, access and refresh tokens — and every bearer-authenticated API call in cleartext. ValidateBaseURL now refuses http bases unless the host is loopback (localhost, 127.0.0.0/8, ::1), where local dev servers live and traffic never leaves the machine. Enforced where the base is resolved — effectiveBaseURL for login/logout/refresh, the client closure and extendx.NewClient for API commands — so every consumer inherits the check.
The /me sanitizer stripped escape sequences from the login success line, but every other server-derived string on an error path was printed verbatim: token endpoint error/error_description fields (also raw proxy error bodies), the authorize redirect's error parameters relayed by the loopback callback, and API error code/message/request-id fields. A malicious or compromised endpoint could smuggle ANSI CSI/OSC sequences through any of them to rewrite the terminal, hide text, or set the window title. Move the sanitizer to iostreams.SanitizeForTerminal and apply it at construction — parseTokenError, the loopback callback handler, and AsAPIError — so every formatting path (the CLI error printer as well as %v/%w chains through Error()) inherits it. Discovery errors already render endpoint values with %q, which escapes control bytes.
UX:
- Re-login now best-effort revokes the grant it replaces, so repeated
logins no longer accumulate live orphaned grant families server-side.
- The success line always says "Signed in" and names non-region bases
("... on https://api.staging.extend.ai"): the environment
parenthetical alone read as the production deployment when signed in
to staging or a rig.
- New `extend whoami` prints the workspace, environment, user, auth
method, and base URL for whichever credential is in effect (API key
or stored login) via GET /me, which accepts both.
- `extend setup` ends with a tip pointing at `extend login`.
- Ctrl-C now renders "Canceled." instead of a raw "context canceled".
- README and the auth topic note that macOS may re-prompt for keychain
access after a binary upgrade.
Hardening/polish:
- A token response without expires_in gets a floor lifetime instead of
an already-expired token that would refresh on every command.
- Falling back from the keychain to the token file warns once per
process; a keychain delete that provably left a record behind now
fails logout instead of letting the login resurrect.
- A corrupted token store surfaces its read error in the
"not logged in" message instead of masquerading as missing auth.
- TokenSource's persist-failure warning is routed through the command's
IO streams (exported Warn) rather than a hardcoded stderr.
- A success callback landing after a failed one renders a failure page
instead of "Already signed in".
The skill now installs as extend-cli (~/.agents/skills/extend-cli/, name: extend-cli) instead of claiming the bare "extend" name — Extend has other agent surfaces (the MCP server) that would contend for it. Eval harnesses and tests follow the new path. The skill's Authentication section catches up with the OAuth login: it leads with `extend whoami`, presents both credential sources (API key for scripts/CI/agents, browser login for interactive use), states the precedence, and tells agents to ask the user rather than invent a key. The pagination example's --max claim is now test-guarded too. The README's Authenticate section is restructured by use case — `extend login` for your own machine, API keys for scripts/CI/agents — with the precedence rules in their own subsection, and the skill section documents the extend-cli scoping plus a project-local install target.
Setup is now a two-line choice (browser login or the API-key wizard); Authentication contrasts the two credential types and lists precedence as a numbered list. The OAuth flow mechanics, keychain re-prompt note, and skill-naming rationale move out of the README — token storage details stay available via 'extend help auth', which the README now points at.
The wizard's first step now asks how to sign in. The browser path picks a region, offers the skill install, then runs the same flow as 'extend login' after the TUI exits — persisting the region and dropping any saved API key first, since a resolved key always shadows a stored login (an environment key gets a warning for the same reason). The API-key path is unchanged, and the post-setup login tip is gone now that the wizard itself offers the choice. Esc on the region step goes back to the method chooser.
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
1 participant
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.
Summary
Implements the CLI side of D93: first-party OAuth login for the CLI, built strictly to the fixed wire contract so the API-side lane can land in parallel.
extend login: RFC 8252 native-app authorization code flow with PKCE S256. Binds an ephemeral 127.0.0.1 port (redirect_uri = http://127.0.0.1:{port}/callback), opens the browser (URL always printed;--no-browserprints and waits), verifiesstate, exchanges the code at{apiBase}/oauth2/token. Endpoint discovery via RFC 8414/.well-known/oauth-authorization-serverwith hardcoded/oauth2/*fallback. The RFC 8707resourceparameter is always sent and equals the configured API base URL.extend logout: POST/oauth2/revokewith the refresh token, then clears local state. Idempotent; clears locally even when revocation fails.zalando/go-keyring(serviceextend-cli, account = API base URL), with a 0600~/.config/extend/oauth_tokens.jsonfallback for headless hosts without a secret service. Stored per API base URL so regions and rigs coexist.EXTEND_OAUTH_NO_KEYRING=1forces the file store (tests, rigs).eort_token. Refresh failure tells the user to runextend loginagain.EXTEND_API_KEY(orEXTEND_<LABEL>_API_KEYunder--env) > config-file API key > stored OAuth login.--env <label>never falls back to the stored login.extend-cli, overridable viaEXTEND_OAUTH_CLIENT_IDfor rig testing.EXTEND_BASE_URL(and the config-filebaseUrl/ region) steer login, discovery,resource, and storage.extend help auth, root help,extend confignow reports "OAuth login" as the auth method.The
extend setupTUI wizard is unchanged (adding a login branch to the bubbletea model was not cheap);extend loginis documented alongside it.Test plan
go build ./...,go vet ./...,gofmt -lcleantest/integration/login_test.go: black-box login -> authenticatedworkflows listwith the stored bearer ->extend configshows OAuth -> logout revokes and clears, against a fake OAuth server (no real credentials needed)Security-review batch (D96)
Implements every fix mandated by the login security review, in six commits on top of the original flow. No API-side contract changes: the wire protocol (authorize/token/revoke parameters, discovery document shape, /me) is untouched; all changes are client-side behavior.
Mediums
1e2c960): the loopback handler previously let any local process abort a pending login by hitting/callbackwith a wrong or missingstate(includingerror=callbacks). The state check now gates every branch; non-matching callbacks get a 404 page and never touch the one-shot result channel.6b918e9): RFC 8414 metadata endpoints must now be on the API base URL's exact host, over https (or the base's own scheme for local http dev bases). A poisoned well-known document pointing elsewhere fails the login with an explicit error instead of receiving the code + PKCE verifier. Fetch failures still soft-fall-back to/oauth2/*.Discovernow returns(Endpoints, error).cf43a4c): the rotated token pair is persisted before the new access token is handed out, with one retry; if both attempts fail the source keeps working in memory (rotation already happened server-side) but warns loudly that a re-login may be needed.cf43a4c): only 400/401 withinvalid_grant/invalid_clientmaps toReauthError. 429/408/5xx/timeouts/proxy-shaped 4xx now surface as plain retryable refresh failures instead of "your login has expired".adde736): refresh takes anO_CREATE|O_EXCLlock file beside the fallback token store (60s stale-lock takeover) and re-reads the store after acquiring, adopting a rotation another process already performed. Prevents double-spending oneeort_from two concurrent CLI invocations (server-side family revocation).15f26db): workspace name and email are stripped of ANSI CSI/OSC sequences (whole, including payloads), C0 controls, DEL, and raw C1 before the success line is printed.Lows (
1e2c960,2a00531)ReadHeaderTimeout, and marks landing pagesCache-Control: no-store.oauth_tokens.json.extendx.Config.TokenContextinstead ofcontext.Background(). (The SDK'sWithTokenFunctakes no per-request context, so the command context is the closest available scope; the bearer transport still uses the true per-request context.)Tests: new/updated coverage for every medium — forged-callback ignore + real login still completing (loopback and full
runLoginlevel), foreign-host and scheme-downgrade discovery rejection, persist-failure retry/warn paths, transient-vs-reauth taxonomy table (429/408/400-no-code/401-no-code/5xx/network error), lock contention + stale-lock takeover + a two-source shared-file-store race with server-side reuse detection (-raceclean), ANSI/OSC/C0 sanitization table + end-to-end escape-injection via /me.go build,go vet,gofmt -l, unit and integration suites all pass locally.