[RFC] feat(oauth): proactive token refresh to avoid re-auth on every session - #8386
Conversation
DOsinga
left a comment
There was a problem hiding this comment.
Hey @vincenzopalazzo — nice idea here! The proactive token refresh to skip the unnecessary 401 → browser cycle is a real UX improvement. The connect_with_auth extraction is a clean DRY win too. A few things to tighten up before this is ready:
Logging is a bit heavy. There are 8 new log lines and several just narrate what the code is about to do (e.g. "Stored credentials found, attempting proactive token refresh", "No stored credentials found, starting browser OAuth flow"). The debug! block that logs has_refresh_token, expires_in, and scopes after browser auth is diagnostic info that's useful during development but not something we want in production. I'd keep the warn! on refresh failure (that's genuinely useful) and drop most of the rest.
has_stored_credentials duplicates existing logic. It reaches directly into Config::global() to do the same thing GooseCredentialStore::load() already does. Could you reuse the existing CredentialStore trait instead? Something like instantiating a GooseCredentialStore and calling .load().await — that way if the storage mechanism ever changes, there's only one place to update.
Looking forward to the next iteration!
…ate credential check - Remove 6 info!/debug! log lines that narrate control flow; keep only the warn! on refresh failure (genuinely useful for production). - Remove has_stored_credentials() which duplicated Config::global() logic; reuse GooseCredentialStore via the CredentialStore trait instead. Addresses review feedback from @DOsinga on aaif-goose#8386. Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Re: review feedback from @DOsingaFixed in f44795c. Two changes:
|
|
This pull request has been automatically marked as stale because it has not had recent activity for 23 days. What happens next?
Thank you for your contribution! 🚀 |
When connecting to OAuth-protected MCP servers, goose now checks for stored credentials before attempting an unauthenticated connection. If credentials exist, it silently refreshes the token and connects directly, avoiding the unnecessary 401 → browser re-auth cycle that previously happened on every new chat session. Also extracts a connect_with_auth() helper to eliminate duplicated HTTP client + transport construction between the proactive and fallback OAuth paths, and adds diagnostic logging to surface whether servers issue refresh tokens. Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
…ate credential check - Remove 6 info!/debug! log lines that narrate control flow; keep only the warn! on refresh failure (genuinely useful for production). - Remove has_stored_credentials() which duplicated Config::global() logic; reuse GooseCredentialStore via the CredentialStore trait instead. Addresses review feedback from @DOsinga on aaif-goose#8386. Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
f44795c to
7c096ed
Compare
The non-exhaustive StreamableHttpClientTransportConfig struct can't be constructed via struct expression. Use the existing with_uri builder pattern, matching the other call sites in this file. Signed-off-by: Douwe Osinga <douwe@squareup.com>
* main: (102 commits) Dynamically refresh skill instructions each turn (#9217) Build non-vulkan linux variants using ubuntu 22.04 (#9211) fix(ui): show tool name in approval prompt (#9216) feat: add Atomic Chat as declarative OpenAI-compatible provider (#9210) chore: bump package.json versions from 0.19.1 to 0.20.0 (#9218) feat: support GOOSE_OAUTH_CALLBACK_PORT for stable OAuth redirect_uri (#9209) [RFC] feat(oauth): proactive token refresh to avoid re-auth on every session (#8386) fix: resolve Azure CLI on Windows by using az.cmd (#9215) fix: handle non-interactive terminal in goose configure on Windows (#9214) Better parsing of pasted html as markdown so agents understand (#9190) fix: persist accumulated cost in session DB to survive reload (#9191) fix(publish-npm): build binary from current SHA + add compat check (#9212) feat(desktop): add goose://new-session deep link to open fresh chat (#9196) Add PR previews using cloudflare pages (#9208) fix: prevent tool-use marker leakage in toolshim output (#8310) Prompt injection mitigation: update pattern-based detection (#9198) remove goose2 related skills (#9189) Switch GH pages deploy to actions/artifact workflow (#9025) fix(summon): re-apply canonical limits when delegate overrides model (#9183) Split code signing from build (#8587) ...
…path connect_with_auth() only set User-Agent, silently dropping any headers configured on a StreamableHttp extension when the OAuth path was taken. This affected two call sites in create_streamable_http_client: - The proactive credential refresh path (added in aaif-goose#8386), which fires on every new session when OAuth credentials are stored for an extension. - The reactive 401 fallback path. Fix: add a headers parameter to connect_with_auth and insert them into the reqwest client's default headers before building the AuthClient, mirroring what the non-OAuth path already does. Signed-off-by: Cameron Yick <cameron.yick@datadoghq.com>
Bug traces to aaif-goose#8148 (01a3d14), not aaif-goose#8386. Added header forwarding test scenarios 8-10 to test-scenarios-oauth-fallback.md. Signed-off-by: Cameron Yick <cameron.yick@datadoghq.com>
…session (aaif-goose#8386) Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com> Signed-off-by: Douwe Osinga <douwe@squareup.com> Co-authored-by: Douwe Osinga <douwe@squareup.com>
Problem
Every time a new chat session starts, goose creates a fresh
ExtensionManagerthat reconnects to all configured MCP extensions. For OAuth-protected servers (e.g. RUbe), the connection logic always attempts an unauthenticated request first, receives a 401, and then triggers the full browser-based OAuth flow — even when valid refresh tokens are already stored in the keyring.This means users must re-authenticate via the browser on every single new chat, making OAuth MCP servers impractical to use.
Root Cause
The connection flow was:
The
oauth_flow()function already had the logic to load stored credentials and silently refresh tokens, but it was only ever called after getting a 401 rejection — never proactively.Fix
Added a credential check before the unauthenticated connection attempt:
If the proactive refresh fails (e.g., network issue), it gracefully falls back to the existing unauthenticated → 401 → browser flow. No new failure modes.
Changes
oauth/persist.rs— Addedhas_stored_credentials()to check if OAuth credentials exist for an extensionextension_manager.rs— Before connecting, check for stored credentials and attempt silent refreshextension_manager.rs— Extractedconnect_with_auth()helper to eliminate duplicated auth client construction (was in 2 places)oauth/mod.rs— Added diagnostic logging to surface refresh outcomes and token metadataDiagnostic Logging
The new logging will tell you exactly what is happening:
Token refresh succeeded - has_refresh_token: trueToken refresh succeeded - has_refresh_token: falseToken refresh failed: No refresh token availableNo stored credentials foundImportant Caveat
This fix is necessary but may not be sufficient for all OAuth MCP servers. The silent refresh only works if the server issues refresh tokens (long-lived tokens that can be exchanged for new access tokens). Some servers may only issue short-lived access tokens without refresh tokens — in that case, the diagnostic logging will show
has_refresh_token: falseand we will know a different approach is needed (e.g., persisting the full authorization session state, or background token refresh).Testing
cargo build -p goose✅cargo clippy --all-targets -- -D warnings✅ zero warningscargo fmt✅cargo test -p goose -- extension_manager✅