feat(auth): make browser OAuth user-controlled - #244
Open
Nowaker wants to merge 1 commit into
Open
Conversation
The first OAuth method opens whatever browser the OS has registered as default. On a machine already signed into a different ChatGPT account that is the wrong browser, and there was no way to choose another: the URL was never printed, so it could not be opened elsewhere. Manual URL Paste was the documented escape hatch, and it rejected a pasted raw authorization code, leaving no working path at all. Add `Codex OAuth (Open URL Manually)` as a first-class method. It binds the loopback listener first, prints the authorize URL instead of opening anything, and completes through localhost exactly like the default-browser method - the two now share one session primitive rather than each owning a private listener. Manual URL Paste required a state parameter unconditionally, which is why a raw code pasted on its own was rejected. What is required is now decided by what the input is. A full callback URL, a bare query or a fragment must carry a matching non-empty state, because that input reproduces the callback's own parameters and a missing one means the paste did not come from this attempt. A raw code is accepted without state, since it carries none to compare and PKCE already binds it to the attempt that produced it. The parser reports which of those forms it saw, so the two cases cannot be mistaken for one another. Two lifecycle defects are fixed alongside, because the shared primitive is where they live: - Callback observation now starts when the session is created rather than when the host invokes its callback, so an authorization the host abandons after taking the URL still releases port 1455 on the listener's existing five-minute deadline instead of pinning it for the lifetime of the process. The observation is normalized so it cannot reject, since an unobserved rejection in the window before the callback is awaited would be an unhandled rejection. - A default browser that fails to launch previously left the user waiting on a page that never opened. `openBrowserUrl` returning false or throwing now closes the listener and reports a typed failure naming the manual-browser method. Docs, docs-parity assertions and direct regressions cover all four methods. Implementation note: OpenCode discovers this plugin's auth methods at runtime and presents their labels; numeric positions are transient indexes into that returned list, not documented or persisted method identifiers. This implementation inserts `Codex OAuth (Open URL Manually)` after the default-browser entry, moving the later entries in the runtime array. The first method also stops interpreting the undocumented `noBrowser` and `no-browser` input keys; direct callers should resolve the new named method from the returned method list instead. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 3e1f523-dirty
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
|
Warning Review limit reachedNext included review available in 7 seconds. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (15)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR makes OAuth in the browser flow user-controlled by adding a manual “open URL yourself” method and unifying the loopback-listener lifecycle so the listener starts before any browser action, with consistent cleanup and improved manual input parsing.
Changes:
- Adds a shared loopback session primitive (
startLoopbackFlow) that owns listener-first startup, opener failure handling, and close-once cleanup. - Introduces a new auth label/method (“Open URL Manually”) and updates manual-paste behavior to accept raw codes as well as full callback URLs (with state validation for structured inputs).
- Expands unit/integration/docs parity tests and updates documentation to reflect the four-method OAuth contract.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/auth/loopback-flow.ts |
Adds listener-first loopback OAuth session primitive with typed lifecycle outcomes and close-once cleanup. |
index.ts |
Switches default browser OAuth to startLoopbackFlow, adds “Open URL Manually” method, and updates manual paste validation/callback handling. |
lib/constants.ts |
Adds new auth label + updated instructions for manual browser and manual paste flows. |
test/loopback-flow.test.ts |
New failing-first unit coverage for the loopback session lifecycle contract. |
test/oauth-server.integration.test.ts |
Adds integration coverage for real-port listener behavior and post-redirect cleanup. |
test/server.unit.test.ts |
Adds unit coverage for idempotent close and post-close cancellation behavior. |
test/index.test.ts |
Updates plugin auth-method contract to four methods and adds coverage for new paths and parsing rules. |
test/doc-parity.test.ts |
Adds doc parity assertions to keep auth labels/count in docs aligned with runtime constants. |
docs/* |
Updates getting started, troubleshooting, FAQ, and architecture docs to describe the new method and behavior. |
AGENTS.md / lib/AGENTS.md |
Updates agent maps to include the new loopback lifecycle module. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+671
to
+696
| const manualInputRejection = ( | ||
| parsed: AuthorizationInputParseResult, | ||
| expectedState: string, | ||
| ): string | undefined => { | ||
| if (!parsed.code) { | ||
| return "No authorization code found. Paste the raw code, or the full callback URL (e.g., http://localhost:1455/auth/callback?code=...). If browser callback keeps failing, retry with Device Code."; | ||
| } | ||
| switch (parsed.source) { | ||
| case "raw": | ||
| return undefined; | ||
| case "url": | ||
| case "query": | ||
| case "fragment": | ||
| if (!parsed.state) { | ||
| return "That callback URL carries no OAuth state, so it cannot be matched to this login attempt. Paste the complete callback URL including its state parameter, paste the raw code on its own, or retry with Device Code."; | ||
| } | ||
| if (parsed.state !== expectedState) { | ||
| return "OAuth state mismatch. Restart login and paste the code or callback URL generated for this login attempt, or retry with Device Code."; | ||
| } | ||
| return undefined; | ||
| default: { | ||
| const unreachable: never = parsed; | ||
| return unreachable; | ||
| } | ||
| } | ||
| }; |
Comment on lines
+119
to
+146
| it("captures a provider-shaped redirect on the real listener without the host awaiting first", async () => { | ||
| // Given a ready manual-browser session on the real callback listener, | ||
| // with only the network exchange stubbed | ||
| const exchange = { code: "", verifier: "", redirectUri: "" }; | ||
| const session = await startLoopbackFlow({ | ||
| openBrowser: false, | ||
| deps: { | ||
| exchangeAuthorizationCode: async (code, verifier, redirectUri) => { | ||
| exchange.code = code; | ||
| exchange.verifier = verifier; | ||
| exchange.redirectUri = redirectUri ?? ""; | ||
| return { type: "success", access: "a", refresh: "r", expires: 1 }; | ||
| }, | ||
| }, | ||
| }); | ||
| if (session.type !== "ready") { | ||
| throw new Error(`expected ready session, got ${session.type}`); | ||
| } | ||
| openSession = session; | ||
| const state = new URL(session.url).searchParams.get("state"); | ||
| expect(state).toBeTruthy(); | ||
|
|
||
| // When the browser follows the provider redirect back to loopback, in the | ||
| // parameter shape the real provider sends, before the host calls back | ||
| const providerCode = "ac_ZmFrZS1hdXRoLWNvZGU.ZmFrZS1zaWduYXR1cmU"; | ||
| const response = await fetch( | ||
| `http://127.0.0.1:1455/auth/callback?code=${providerCode}&scope=openid+profile+email+offline_access&state=${state}`, | ||
| ); |
Comment on lines
+184
to
+196
| await expect | ||
| .poll( | ||
| async () => { | ||
| try { | ||
| await fetch("http://127.0.0.1:1455/auth/callback?code=x&state=y"); | ||
| return "open"; | ||
| } catch { | ||
| return "closed"; | ||
| } | ||
| }, | ||
| { timeout: 5000 }, | ||
| ) | ||
| .toBe("closed"); |
Comment on lines
+633
to
+634
| expect(gettingStarted).toContain("four"); | ||
| expect(gettingStarted).not.toContain("**three**"); |
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.
Summary
Codex OAuth (Open URL Manually)as a first-class login method. It starts the localhost callback listener, prints the authorization URL, and lets the user open that URL in any browser without launching the OS default browser.code#stateinput must carry the matching state; a raw code can omit state because PKCE still binds it to the login attempt.Testing
npm run lintnpm run buildnpm testAdditional verification:
npm run typecheckpassed.test/paths.test.ts, which this change does not touch: it asserts that a path outside the home directory is rejected, and it fails only because the run used a redirectedHOME. Against the ordinary environment that file passes 28/28 on this commit.main.Compliance Confirmation
Notes
mainalready moved them once the same way when Device Code was added in 9968199.noBrowser/no-browserinput keys are no longer interpreted; the first-class Open URL Manually method supersedes them.main:test/oauth-server.integration.test.tsandtest/chaos/auth-faults.test.tsboth bind port 1455, and Vitest runs files in parallel, so under load whichever binds second can observeready=falseand fail for a reason unrelated to its subject. This change adds coverage to the first of those files and therefore widens an existing window without creating it. I kept a fix out of this PR because it is test infrastructure rather than OAuth behavior; happy to send it separately.HOMEbecausetest/chaos/auth-invalidated-401-stress.test.tsonmainwrites the developer's real account pool. That is fixed separately in fix(accounts): stop a disposed manager overwriting the account store #242 and is unrelated to this change.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr adds a user-controlled browser oauth method, unifies loopback listener lifecycle handling, and permits pkce-bound raw authorization codes in manual paste.
Confidence Score: 4/5
the pr appears safe to merge, with a non-blocking gap in host-level vitest coverage for abandoned manual-browser sessions and immediate retries.
the oauth state, pkce, token exchange, and listener cleanup paths remain protected; the only accepted concern is missing coverage for a concrete port-1455 concurrency scenario.
Files Needing Attention: test/index.test.ts
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[choose oauth method] --> B{method} B -->|default browser| C[start loopback listener] B -->|open url manually| C C --> D[create state and pkce session] D --> E[receive state-checked callback] B -->|manual paste| F[parse callback url or raw code] F --> G{structured input} G -->|yes| H[validate state] G -->|no| I[retain opaque raw code] E --> J[exchange with session verifier] H --> J I --> J J --> K[resolve and persist account]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(auth): make browser OAuth user-cont..." | Re-trigger Greptile
Context used (4)