feat(services): generic OAuth 2.0 + PKCE service#277
Conversation
Phase 8.12 service-layer fill-in. Adds a provider-agnostic Authorization-Code-with-PKCE flow under crates/lib/src/services/oauth.rs. - OAuthProviderConfig holds per-provider URLs, client id, scopes, redirect uri; nothing is hardcoded. - OAuthService::login spawns a loopback HTTP server on 127.0.0.1:0, opens the auth URL via the platform default opener, captures the redirect, exchanges the code (PKCE S256) at the token endpoint, and persists the token set. - current_token transparently refreshes when within 60s of expiry; 4xx refresh failures clear the cached creds and surface ReauthRequired so callers can drive a fresh browser flow. - logout is idempotent. - Loopback hard-times-out at 5 min so an abandoned tab cannot hang. - TokenSet has a custom Debug that prints *** for token fields; no OAuthError variant carries a token. Regression test asserts Display+Debug do not leak. - Pluggable CredentialStore trait. Default backend is a 0o600 JSON file under the agent config directory; misses (no HOME, locked store) return KeychainError instead of panicking. Trait lets a follow-up plug an OS keychain backend without touching the flow. Tests (25 new, all passing): - PKCE verifier alphabet + length conformance (RFC 7636 §4.1) - PKCE challenge = base64url(SHA256(verifier)) (RFC 7636 §4.2 + Appendix B test vector) - base64url no-padding url-safe alphabet - Loopback captures code, rejects state mismatch, accepts correct state - Authorization URL building (PKCE params, scope encoding, query merge) - Refresh against a hand-rolled fake token server (200 + 4xx paths) - current_token refresh-and-persist, 4xx-clears-creds-and-reauth, no-creds-stored, logout idempotency - Full login flow with stub authz callback + stub token server - File credential store round-trip, 0o600 perms (unix), key sanitization - Token-leak regression: errors and TokenSet Debug never print tokens No new workspace dependencies.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7d06d9ce7
ℹ️ 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".
| if let Some(state) = params.get("state") | ||
| && state != expected_state | ||
| { |
There was a problem hiding this comment.
The state validation in capture_redirect only rejects mismatches when a state param is present, but it still accepts any request containing code when state is missing. That means a local process can inject a callback without the CSRF binding and have its authorization code exchanged and persisted. The callback should require state to exist and exactly match before accepting code.
Useful? React with 👍 / 👎.
| fn is_terminal_refresh(status: u16) -> bool { | ||
| (400..500).contains(&status) |
There was a problem hiding this comment.
is_terminal_refresh classifies all 4xx responses as terminal, and current_token deletes stored credentials on that path. This will force re-authentication and discard cached credentials for transient or policy-driven 4xx responses (for example throttling-style 429), even when retrying later would succeed. Only truly invalid/expired refresh-token conditions should trigger credential deletion.
Useful? React with 👍 / 👎.
| let res = std::process::Command::new(bin) | ||
| .args(args_for(bin)) | ||
| .stdout(std::process::Stdio::null()) | ||
| .stderr(std::process::Stdio::null()) | ||
| .spawn(); | ||
| match res { | ||
| Ok(_) => return Ok(()), | ||
| Err(e) => last_err = e.to_string(), |
There was a problem hiding this comment.
The browser launcher reports success as soon as spawn() succeeds, without checking the child exit status. If the opener binary exists but immediately fails (e.g., no handler/display), login still waits on loopback until timeout and returns a misleading loopback error. Waiting for process completion and treating non-zero exit as BrowserLaunchFailed avoids this false-success path.
Useful? React with 👍 / 👎.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…nt/terminal, await opener exit, validate https endpoints Addresses four PR review findings on the generic OAuth service. 1. P1 CSRF: capture_redirect now requires the `state` query param to be both present and exactly equal to the per-login expected value. A callback that delivers a `code` without `state` is rejected with HTTP 400 and never reaches the token endpoint — closes a path where a local process could smuggle a stolen authorization code into the loopback. Mismatched state is also a hard error rather than a silent "keep waiting". 2. P2 refresh classification: 4xx is no longer a blanket "delete the cached creds" signal. RefreshFailed gains a `transient: bool` field. The new is_terminal_refresh parses the RFC 6749 §5.2 error code from the body and returns terminal only for `invalid_grant` / `invalid_client`, plus HTTP 401 with no parsable body. 429 / 503 / 408 / network blips / unparseable non-401 bodies are transient — creds are preserved and the caller can retry. current_token only deletes credentials when transient is false. 3. P2 browser launcher: spawn() no longer counts as success. The launcher now waits for the opener and checks its exit status. Non-zero (or signal) returns BrowserLaunchFailed with the actual exit code in the message instead of letting the user sit through a 5-minute LoopbackTimeout. macOS, Linux, and Windows openers all exit promptly after handing off, so the wait is short. 4. CodeQL #59 (HTTPS): OAuthProviderConfig::validate rejects any authorization_url / token_url that is not https://. `redirect_uri` is allowed http://127.0.0.1 / localhost (RFC 8252 §7.3 loopback). A new `allow_insecure_local` flag opens a narrow door for loopback-only test fixtures; off by default. OAuthService::new and with_store now return Result and propagate InvalidConfig. Tests cover every documented case: no-state and mismatched- state callbacks, 429/503 transient + invalid_grant terminal refresh, opener-exits-non-zero surfacing as BrowserLaunchFailed with the exit code, http authorization_url rejected, https + loopback redirect accepted.
|
Addressed review findings: required state on every loopback callback (P1 CSRF), classified refresh failures as transient vs terminal so 429/503/network blips no longer drop creds (P2), made the browser launcher wait on the opener and surface non-zero exits as BrowserLaunchFailed (P2), and added HTTPS validation on OAuthProviderConfig with a narrow allow_insecure_local door for loopback test fixtures (CodeQL #59). |
2 participants
Summary
crates/lib/src/services/oauth.rs— a provider-agnostic OAuth 2.0 Authorization-Code-with-PKCE flow that powers Phase 8.12 of the service-layer fill-in.OAuthProviderConfigcarries per-provider URLs / client id / scopes / redirect uri; nothing is hardcoded.OAuthService::new,login,current_token,logout, plus a pluggableCredentialStoretrait with a default 0o600 JSON-file backend under the agent config directory. The trait shape lets a follow-up plug an OS keychain without touching the flow.OAuthErrorvariant or inTokenSet'sDebugoutput (custom impl prints***); loopback server hard-times-out at 5 min; refresh that returns 4xx clears the cached creds and surfacesReauthRequired; no panics on a missing config dir.Test Plan
cargo check --all-targetscargo test --all-targets(25 new OAuth tests pass; pre-existingbwrap_*sandbox failures on this machine are environmental and reproduce onmain)cargo clippy --all-targets -- -D warningscargo fmt --all -- --checkcode, rejects state mismatch, accepts correct state, returns the close-this-tab HTML?tokio::net::TcpListenerfake (200 happy path + 4xx terminal path)current_token: refresh-and-persist when stale, 4xx clears creds and demands reauth, no-creds returnsReauthRequiredlogoutidempotencyloginflow with stub authz callback + stub token server, asserting credential write and request body shape..//OAuthErrorvariant'sDisplay+Debug, andTokenSet'sDebug, never print injected token strings