Uh oh!
There was an error while loading. Please reload this page.
fix(auth): redirect browser and surface OAuth errors on PKCE callback failure - #258
Conversation
… failure
When `extract_pkce_code` returned an error (most commonly because the
user denied authorization, producing a callback with
`?error=...&error_description=...&state=...` and no `code`),
`await_pkce_callback` dropped the TCP stream without writing any HTTP
response. The browser saw a blank "connection reset" tab and the CLI
printed the generic "No code in callback URL".
Two fixes:
- `extract_pkce_code` now checks for OAuth 2.0 `error` /
`error_description` parameters (RFC 6749 §4.1.2.1) after state
validation, surfacing the provider-reported reason instead of the
misleading "no code" message.
- `await_pkce_callback` takes `app_url` and, on extraction failure,
redirects the browser to `{app_url}/cli-auth/error` before returning
the error — mirroring the token-exchange error path already in place.
Fixes#225.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>| match extract_pkce_code(request, expected_state) { | ||
| Ok(code) => Ok((code, stream)), | ||
| Err(e) => { | ||
| // Redirect the browser to the error page before surfacing the | ||
| // error, otherwise the user sees a blank "connection reset" tab. | ||
| // Mirrors the token-exchange error path in the caller. | ||
| redirect_browser(&mut stream, &format!("{app_url}/cli-auth/error")).await; | ||
| Err(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: No double-redirect on extract_pkce_code failure
The new error path in await_pkce_callback (line 184-190) redirects the browser and then returns Err. Since pkce_login uses ? at line 152 (let (code, mut stream) = await_pkce_callback(...).await?), the error propagates immediately and the match pkce_token_exchange(...) block at line 154 is never reached. The stream is consumed inside await_pkce_callback on the error path and dropped when the function returns, so there's no possibility of a second redirect. This is correct but worth noting since the redirect logic now exists in two places.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // RFC 6749 §4.1.2.1: the provider may redirect to the callback with | ||
| // `error=...&error_description=...` instead of `code=...` (e.g. when the | ||
| // user denies authorization). Surface that directly. | ||
| if let Some(err) = params.get("error") { | ||
| match params.get("error_description") { | ||
| Some(desc) => bail!("OAuth authorization failed: {err} — {desc}"), | ||
| None => bail!("OAuth authorization failed: {err}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: State check correctly prioritized over OAuth error check
The state mismatch check at line 212 runs before the new OAuth error parameter check at line 219. This is security-critical: a malicious callback cannot bypass the CSRF state validation by including error= in the query string. The test at line 311 (extract_code_still_checks_state_before_oauth_error) explicitly guards against regression here.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:be75405583
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match params.get("error_description") { | ||
| Some(desc) => bail!("OAuth authorization failed: {err} — {desc}"), |
There was a problem hiding this comment.
extract_pkce_code now surfaces error_description, but this value is consumed as-is from parse_query_string, which only percent-decodes and does not translate + into spaces. OAuth authorization responses are form-encoded, so providers commonly send descriptions like The+user+denied+the+request; with the current code this reaches users with literal + characters, making the new callback-failure messaging less readable. Please normalize + to spaces before decoding (for error_description or all query values).
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a> <!-- devin-review-badge-end --> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
When the PKCE callback came back without a
code(most commonly because the user denied authorization — the provider then redirects to?error=access_denied&error_description=...&state=...), the CLI:No code in callback URLinstead of the provider-reported reason.Two fixes, both in
src/commands/auth.rs:extract_pkce_codenow inspectserror/error_descriptionquery params (RFC 6749 §4.1.2.1) after state validation and returns them directly. State-mismatch still takes priority so a malicious callback can't skip the CSRF check by addingerror=.await_pkce_callbacktakesapp_urland, on extraction failure, sends a302redirect to{app_url}/cli-auth/errorbefore returning the error. This mirrors the existing token-exchange error path at the call site.Test plan
cargo test --lib commands::auth— 9 pass, including 3 new cases for the OAuth error shape and the state-before-error orderingcargo clippy -- -D warningscleancargo fmt --checkclean/cli-auth/errorwith a helpful CLI messageFixes#225.
🤖 Generated with Claude Code