feat(cli): add browser-based OAuth2 login with PKCE#1085
Open
jeremyeder wants to merge 3 commits intoambient-code:mainfrom
Open
feat(cli): add browser-based OAuth2 login with PKCE#1085jeremyeder wants to merge 3 commits intoambient-code:mainfrom
jeremyeder wants to merge 3 commits intoambient-code:mainfrom
Conversation
Add `acpctl login browser` subcommand that authenticates via OAuth2 Authorization Code + PKCE flow. Opens the user's browser to the identity provider, receives the callback on a local HTTP server, and exchanges the code for access and refresh tokens. - Add pkg/oauth/ package: OIDC discovery, PKCE generation, state generation, authorize URL construction, token exchange, local callback server, cross-platform browser opener - Add `login browser` subcommand with --issuer-url, --client-id, --scopes flags (env var and config file fallbacks) - Extend Config with RefreshToken, IssuerURL, ClientID fields - Support both automatic browser callback and manual redirect URL paste - 15 tests covering PKCE, state, OIDC discovery, token exchange, and callback server scenarios Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughNew browser-based OAuth2 Authorization Code + PKCE login for the CLI. Adds an oauth package (OIDC discovery, PKCE, state, token exchange), a local callback HTTP server, a browser subcommand orchestrating the flow, and config fields to persist issuer, client ID, and refresh token. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant IdP
participant LocalServer
participant Config
User->>CLI: run "acpctl login browser"
CLI->>IdP: OIDC discovery (/.well-known/openid-configuration)
IdP-->>CLI: authorization_endpoint, token_endpoint
CLI->>CLI: generate PKCE verifier/challenge and state
CLI->>LocalServer: start callback server on 127.0.0.1:0
LocalServer-->>CLI: return listen address
CLI->>CLI: build authorization URL (with code_challenge, state)
CLI->>User: open browser / print URL
User->>IdP: authenticate & authorize (browser)
IdP->>LocalServer: redirect to /callback?code=...&state=...
LocalServer->>LocalServer: validate state, extract code
LocalServer-->>CLI: send Code via channel
CLI->>IdP: POST token exchange (code, code_verifier)
IdP-->>CLI: access_token (+ refresh_token)
CLI->>Config: store access_token, refresh_token, issuer, client_id
Config-->>CLI: persisted
CLI-->>User: print success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
This works nicely. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/ambient-cli/cmd/acpctl/login/browser/cmd.go`:
- Around line 183-186: The current login flow stores tokenResp.RefreshToken into
cfg.RefreshToken and persists the full cfg even though no refresh logic consumes
it; remove writing the refresh token to the normal config to avoid persisting an
unused long‑lived credential. Update the assignment code where cfg.RefreshToken
is set (and any subsequent config persistence path that writes cfg) to skip or
null out cfg.RefreshToken before saving, continue to set
cfg.AccessToken/cfg.IssuerURL/cfg.ClientID as before, and ensure any
Save/WriteConfig call uses the trimmed cfg so refresh tokens are not written to
disk until a proper refresh implementation requires it.
- Around line 139-154: The manual URL paste path currently accepts an OAuth code
even if the "state" query parameter is missing, which bypasses the CSRF/session
check; update the logic in the paste handling (look for parsed, pastedState,
state, manualCh and oauth.CallbackResult in cmd.go) to require a non-empty
pastedState and to return an error (e.g., "URL missing 'state' parameter" or
"state mismatch in pasted URL") if pastedState is empty or does not equal the
expected state, otherwise continue to send the Code in oauth.CallbackResult.
In `@components/ambient-cli/pkg/oauth/oauth.go`:
- Around line 20-23: DiscoverEndpoints currently accepts any non-empty endpoints
from the well-known document; update DiscoverEndpoints to parse and validate the
discovery JSON's "issuer" equals the expected issuer passed into the function
and ensure AuthorizationEndpoint and TokenEndpoint
(OIDCConfig.AuthorizationEndpoint / OIDCConfig.TokenEndpoint) are valid absolute
URLs (scheme present and host non-empty) before returning; if the issuer
mismatches or endpoints are not absolute URLs, return an error and do not use
the discovery document.
- Around line 91-101: BuildAuthorizeURL currently concatenates params to
authEndpoint which breaks when authEndpoint already has query parameters; change
BuildAuthorizeURL to return (string, error), parse authEndpoint with url.Parse
inside BuildAuthorizeURL, merge the new params into the parsed URL's existing
query (use u.Query() and add each key/value from params), set u.RawQuery =
mergedQuery.Encode(), and return u.String(), nil; propagate the new error return
to callers so parse errors are handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90bfd56b-b56a-4df0-bfab-66b9a95fbd88
📒 Files selected for processing (8)
components/ambient-cli/cmd/acpctl/login/browser/cmd.gocomponents/ambient-cli/cmd/acpctl/login/cmd.gocomponents/ambient-cli/pkg/config/config.gocomponents/ambient-cli/pkg/oauth/browser.gocomponents/ambient-cli/pkg/oauth/callback.gocomponents/ambient-cli/pkg/oauth/callback_test.gocomponents/ambient-cli/pkg/oauth/oauth.gocomponents/ambient-cli/pkg/oauth/oauth_test.go
- Require state parameter in manual URL paste path to prevent CSRF bypass - Stop persisting unused refresh token to config file - Validate OIDC discovery issuer matches expected issuer (RFC 8414) - Validate discovery endpoints are absolute URLs - Use url.Parse for BuildAuthorizeURL to preserve existing query params - Add TestDiscoverEndpoints_IssuerMismatch test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/ambient-cli/cmd/acpctl/login/browser/cmd.go`:
- Around line 119-132: The stdin-copy goroutine reading from os.Stdin can block
forever if the browser callback finishes first; to fix, add a done channel and,
when the callback path completes, close done and if os.Stdin is a *os.File call
its Close() to unblock the reader; update the anonymous goroutine that writes to
pw to return when done is closed (check done in a non-blocking manner between
reads) and ensure pw.Close() is still deferred so the scanner stops cleanly —
reference the pw writer, the anonymous goroutine that reads os.Stdin, and the
callback/completion code path to wire up done and the conditional
os.Stdin.(*os.File).Close().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34b32c62-d9d1-49e8-8469-59ea564cea00
📒 Files selected for processing (3)
components/ambient-cli/cmd/acpctl/login/browser/cmd.gocomponents/ambient-cli/pkg/oauth/oauth.gocomponents/ambient-cli/pkg/oauth/oauth_test.go
Comment on lines
+119
to
+132
| // Copy stdin to pipe in background so we can close pr to stop the scanner. | ||
| go func() { | ||
| defer pw.Close() | ||
| buf := make([]byte, 4096) | ||
| for { | ||
| n, err := os.Stdin.Read(buf) | ||
| if n > 0 { | ||
| pw.Write(buf[:n]) //nolint:errcheck | ||
| } | ||
| if err != nil { | ||
| return | ||
| } | ||
| } | ||
| }() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stdin copy goroutine will outlive the function if callback succeeds first.
The goroutine reading from os.Stdin will remain blocked indefinitely if the browser callback completes before any stdin input. While this is a minor leak, it's acceptable for a CLI tool that exits after login. No action required, but worth noting for future refactoring if this code is reused in a long-running context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-cli/cmd/acpctl/login/browser/cmd.go` around lines 119 -
132, The stdin-copy goroutine reading from os.Stdin can block forever if the
browser callback finishes first; to fix, add a done channel and, when the
callback path completes, close done and if os.Stdin is a *os.File call its
Close() to unblock the reader; update the anonymous goroutine that writes to pw
to return when done is closed (check done in a non-blocking manner between
reads) and ensure pw.Close() is still deferred so the scanner stops cleanly —
reference the pw writer, the anonymous goroutine that reads os.Stdin, and the
callback/completion code path to wire up done and the conditional
os.Stdin.(*os.File).Close().
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
acpctl login browsersubcommand implementing OAuth2 Authorization Code + PKCE flowpkg/oauth/package with OIDC discovery, PKCE, callback server, and cross-platform browser openerrefresh_token,issuer_url,client_idfields (env var + config file fallbacks)Test plan
go build ./...— compiles cleanlygo vet ./...— no issuesgo test ./pkg/oauth/...— 15/15 passacpctl login browser --help— correct flags and description--issuer-url, missing--client-id, unreachable issuer🤖 Generated with Claude Code