aaif-goose / aaif-goose/goose

MCP OAuth re-authorizes on every new session after transient refresh failures or refresh rotation races

Ouverte
#12,016 0 commentaires 0 réactions 1 personne assignée Réclamée par @jbg Voir sur GitHub
Langage dominant
Rust
Étoiles
54.2k
Forks
6.2k
Merge moyen
3 j 4 h
PR mergées (30 j)
240

Description

**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.

Guide de contribution

Ouvrir le guide de contribution

Piste de recherche

Le bug se trouve dans crates/goose/src/oauth/mod.rs, plus précisément dans la fonction oauth_flow_with_challenge et dans la gestion du credential store. Commencez par examiner la gestion des erreurs de TokenRefreshRejected par rapport à TokenRefreshFailed dans le refresh error handler. Examinez la méthode GooseCredentialStore::load_persisted pour la journalisation des erreurs. Comprenez les types d’erreur de rmcp 3.x. Un test avec un serveur d’autorisation mock est nécessaire pour vérifier que le correctif couvre les échecs transitoires, les conditions de concurrence lors de la rotation des tokens et les cas invalid_grant.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

Évaluation

Domaine
authentication, backend
Type d'issue
Bug
Difficulté
4/5
Temps estimé
3-5 jours
Activité
Active
Clarté
Clairement spécifiée
Accessibilité débutants
45/100

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.