Repository navigation
feat: use oauth to authenticate (COR-14002) - #31
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
OAuth refresh, callback validation, concurrent rotation, credential cleanup, and documented override behavior have unresolved correctness and security issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds browser-based OAuth 2.0 authentication with PKCE, secure credential storage, and automatic token refresh.
Changes:
- Implements OAuth discovery, registration, callback, token, refresh, and storage flows.
- Integrates OAuth into authentication commands and API client credential resolution.
- Adds comprehensive tests and user documentation.
File summaries
| File | Description |
|---|---|
| README.md | Documents browser sign-in and credential precedence. |
| internal/oauth/token.go | Implements token exchange and refresh. |
| internal/oauth/token_test.go | Tests token endpoint behavior. |
| internal/oauth/testsupport_test.go | Adds shared OAuth test helpers. |
| internal/oauth/store.go | Persists sessions and client registrations. |
| internal/oauth/store_test.go | Tests keychain and file storage. |
| internal/oauth/register.go | Implements dynamic client registration. |
| internal/oauth/pkce.go | Generates PKCE verifiers and challenges. |
| internal/oauth/pkce_test.go | Tests PKCE generation. |
| internal/oauth/oauth.go | Coordinates login, refresh, and logout flows. |
| internal/oauth/metadata.go | Implements authorization-server discovery. |
| internal/oauth/metadata_test.go | Tests discovery and authorization URLs. |
| internal/oauth/login_test.go | Tests end-to-end login and refresh. |
| internal/oauth/command.go | Provides OAuth command integration. |
| internal/oauth/command_test.go | Tests command flags and status output. |
| internal/oauth/callback.go | Implements the loopback callback server. |
| internal/oauth/callback_test.go | Tests callback validation and responses. |
| internal/oauth/browser.go | Opens authorization URLs in browsers. |
| internal/client/client.go | Adds OAuth credential resolution. |
| internal/cli/whoami.go | Displays OAuth session status. |
| internal/cli/auth.go | Makes browser OAuth the default login flow. |
| docs/vf_auth.md | Updates authentication command documentation. |
| docs/vf_auth_login.md | Documents OAuth login flags and behavior. |
Review details
Files not reviewed (3)
- internal/cli/auth.go: Generated file
- internal/cli/whoami.go: Generated file
- internal/client/client.go: Generated file
- Files reviewed: 20/23 changed files
- Comments generated: 9
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
effervescentia
force-pushed
the
ben/cor-14002
branch
from
September 21, 2026 20:48
ebbd75b to
dd85573
Compare
|
Prerelease Binaries for all six platforms are attached to the release. Not published to npm. gh release download v0.255.0-pr.31.1.1 --repo voiceflow/cli --pattern 'vf_Darwin_arm64.tar.gz'
tar xzf vf_Darwin_arm64.tar.gz && ./vf versionRe-add the |
Nine findings from the PR review, all verified against the code: - metadata: require the discovered issuer to be present and to match the one discovery was run against (RFC 8414 §3.3). - callback: validate state before handling a success *or* error response, and never deliver a result for a mismatch, so an unsolicited request to a predictable loopback port cannot abort a live login. - store: report keychain deletion failures from ClearSession instead of swallowing them, and keep the session file so a retry can finish. - store: report the persisted storage state back to the caller so login prints the keychain rather than always the session file. - oauth: honour VF_OAUTH_REDIRECT_URI in the dynamic-registration path — validate cached registrations against it and register that list. - oauth: serialise refresh across processes with a file lock, so parallel commands cannot have one delete the session the other just rotated. - oauth: refresh a token whose response carried no expires_in after an assumed lifetime rather than reusing it forever. - client: propagate non-ErrNoSession OAuth failures out of NewClient instead of sending a request that fails with a misleading API error. - README: document that SSH sign-in needs loopback port forwarding, not just --no-browser. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
No description provided.