MCP OAuth re-authorizes on every new session after transient refresh failures or refresh rotation races
- Vorherrschende Sprache
- Rust
- Sterne
- 54.2k
- Forks
- 6.2k
- Ø Merge
- 3 T. 2 Std.
- Gemergte PRs (30 T.)
- 262
Beschreibung
**Describe the bug**
MCP servers that authenticate via OAuth demand a fresh interactive browser authorization on (nearly) every new goose session, even though valid credentials were previously stored. Once it starts happening, the stored credentials are repeatedly destroyed, so every subsequent session starts from zero.
Root cause (in `crates/goose/src/oauth/mod.rs`, `oauth_flow_with_challenge`):
1. **Any token-refresh failure clears the stored credentials.** The refresh error handler treats every failure identically — `credential_store.clear()` wipes the access *and* refresh token, then falls back to browser auth. A transient failure (authorization-server 5xx, network timeout, DNS hiccup) therefore permanently discards a refresh token that was still perfectly valid. The next session finds nothing stored → unauthenticated connect → 401 → full browser re-auth. rmcp 3.x already distinguishes `TokenRefreshRejected` (definitive `invalid_grant`) from `TokenRefreshFailed` (transient/retryable, doc comment explicitly says "Callers may retry this error"), but the handler does not use that distinction.
2. **Cross-process refresh rotation races clobber the winner's credentials.** Multiple goose processes share one credential store (the desktop spawns one `goose serve` per working directory, plus the CLI). With authorization servers that rotate refresh tokens on every use (e.g. Atlassian/Trello, Composio), the process that loses the refresh race receives `invalid_grant` and clears the *fresh* credentials the other process just persisted.
3. **Credential-store read errors are silently swallowed.** `GooseCredentialStore::load_persisted` maps every error (not just `NotFound`) to `Ok(None)`, so a storage problem is indistinguishable from "never authorized" and also forces a fresh browser auth, with no log line to diagnose it.
---
**To Reproduce**
Code-level (deterministic):
1. Configure a `streamable_http` MCP extension whose authorization server issues access tokens with a short `expires_in` (e.g. 3600) and supports the refresh grant.
2. Authorize once; credentials are persisted under `oauth_creds_{name}`.
3. Make the server's token endpoint return a 503 (or block it) before starting the next session.
4. Start a new goose session: the proactive refresh fails transiently → stored credentials are cleared → browser authorization is demanded.
5. Restore the token endpoint: the refresh token is gone, so re-authentication is required again even though it would have succeeded.
Rotation-race variant:
1. Run two goose processes against the same config (desktop `goose serve` + CLI) with a server that rotates refresh tokens on use.
2. Both sessions start ~simultaneously; both attempt refresh; the loser's `invalid_grant` clears the winner's just-stored credentials.
3. Subsequent sessions must re-authorize interactively.
Observed in the wild: `secrets.yaml` entries stuck with `token_response: null` (forces full browser auth on every session until one completes) and expired-token entries untouched for days despite daily use.
---
**Expected behavior**
- A transient refresh failure must keep the stored credentials; only this session falls back to browser auth, and the next session retries the refresh.
- Only a definitive `invalid_grant` rejection should clear credentials — and only if another process has not already replaced them with a rotated refresh token.
- Credential-store read errors should be logged, not silently reported as "missing".
N/A (behavioral; see log messages `[OAuth:...] Token refresh failed ... clearing stored credentials and falling back to browser auth`).
---
**Please provide the following information**
- **OS & Arch:** macOS 15 (arm64) — not platform-specific
- **Interface:** CLI and UI (both go through `oauth_flow_with_challenge`)
- **Version:** observed on v1.48+; behavior present on `main` as of `846cbeaf5`
- **Extensions enabled:** streamable_http MCP servers with OAuth (e.g. Trello/Atlassian, Composio-backed, self-hosted)
- **Provider & Model:** N/A
---
**Additional context**
Provenance: the clear-on-failure policy dates to #4007 (reactive-only flow, low impact). #8386 added the proactive per-session refresh and kept the clear, which amplified it into per-session re-auth; at the time rmcp 1.6 exposed only `TokenRefreshFailed`, so transient vs definitive could not be distinguished. #10789 upgraded to rmcp 3.0 which introduced exactly that distinction, but the handler was never revisited.
Suggested fix scope: in the refresh error handler, clear only on `AuthError::TokenRefreshRejected` and only when the stored refresh token was not replaced by another process since the attempt began; otherwise keep credentials and fall back to browser auth for the current session only. Log read errors in `load_persisted` instead of swallowing them. Non-goals: no change to challenge/scope step-up behavior (SEP-2350 rationale for skipping refresh when challenged), no rmcp API changes.
Verification plan: regression test driving `oauth_flow` against a mock authorization server covering (a) transient 5xx/503 failure preserves stored credentials, (b) successful refresh persists rotated tokens without touching the authorization endpoint, (c) definitive `invalid_grant` without rotation still clears, (d) a lost rotation race reuses the replacement credentials from the other process.
Beitragsleitfaden
Bewertung
Dieses Issue wurde noch nicht bewertet.