google-gemini / google-gemini/gemini-cli

bug: concurrent MCP OAuth refreshes race, and any refresh failure deletes valid credentials (forced re-auth)

Open
#29,048 0 comments 0 reactions 0 assignees View on GitHub
area/security status/need-triage
Dominant language
TypeScript
Stars
107k
Forks
14.6k
Avg merge
2d 3h
Merged PRs (30d)
45

Description

## What happened?

Two related defects in `MCPOAuthTokenStorage`-backed refresh flow of `MCPOAuthProvider`:

1. **No concurrency control around token refresh.** Two MCP tool calls hitting an expired access token concurrently both read the same refresh token and both POST to the token endpoint. Servers that rotate refresh tokens (standard per RFC 6749 §10.4 / OAuth BCP) invalidate the old refresh token on first use — the loser gets `invalid_grant`, enters the catch, and calls `deleteCredentials(serverName)`, **deleting the fresh valid credentials just saved by the winner**. Result: forced full browser re-authentication despite a successful refresh.
2. **Any error deletes stored credentials.** The same catch wipes credentials for *every* failure — including transient network errors on the refresh POST. An offline blip at expiry time permanently logs the user out even though the refresh token was perfectly valid.

## Affected code

`packages/core/src/mcp/oauth-provider.ts:574-617` (`getValidToken`):

```ts
const newTokenResponse = await this.refreshAccessToken(
{ ...config, clientId }, token.refreshToken, ...);
// ...
await this.tokenStorage.saveToken(serverName, newToken, ...);
return newToken.accessToken;
} catch (error) {
coreEvents.emitFeedback('error', 'Failed to refresh auth token.', error);
// Remove invalid token
await this.tokenStorage.deleteCredentials(serverName); // ANY error -> wipe
}
```

There is no mutex / in-flight-promise dedup / re-read-after-refresh anywhere; the underlying storage is plain read-modify-write per operation.

## How can this be reproduced?

1. Connect an MCP server that rotates refresh tokens; let the access token expire.
2. Fire two tool calls concurrently.
3. Observe two refresh POSTs; the second fails `invalid_grant`; stored credentials are deleted; next call demands interactive login.

(Or simpler: kill network connectivity during a single refresh → credentials wiped.)

## What did you expect to happen?

- Single-flight refresh (share one in-flight promise across callers).
- On refresh failure, distinguish `invalid_grant` (credentials truly dead → delete) from transient errors (keep credentials, surface retryable error).

## Impact

Forced re-authentication loops for MCP servers; worst case repeated browser popups in automation contexts.

---

*Found by source audit on current `main` (commit `5411f113c`); platform-independent. No open issue/PR covering this was found (searched: MCP OAuth refresh token deleted).*

Contributor guide

Open the contributing guide

Research direction

Start in packages/core/src/mcp/oauth-provider.ts:574-617, especially getValidToken, then inspect refreshAccessToken and the MCPOAuthTokenStorage operations it uses. Reproduce the concurrent expired-token case and a network failure, then verify that refreshes are single-flight, invalid_grant removes dead credentials, and transient failures preserve valid credentials.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
authentication, cli
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.