github / github/copilot-cli

Bug: Stale keytar (OS keychain) entries cause repeated browser OAuth popups for HTTP MCP servers on every launch

Ouverte
#2,112 1 commentaire 1 réaction 0 personnes assignées Voir sur GitHub
area:authentication area:mcp
Langage dominant
Shell
Étoiles
11.2k
Forks
1.9k
Merge moyen
14 h 16 min
PR mergées (30 j)
6

Description

## Summary

When HTTP MCP servers are configured, Copilot CLI opens browser windows for OAuth authentication **on every single launch**, even though valid tokens with refresh tokens exist in the file-based cache. The root cause is stale, expired tokens persisted in the OS keychain (via keytar) that shadow newer file-based tokens.

## Environment

- **Copilot CLI version:** 1.0.7
- **OS:** Windows 11 (x64)
- **Node:** v24.11.1

## Root Cause Analysis

The OAuth token store (`mcp-oauth-config`) has two backends with a priority order:

1. **Keytar** (OS keychain / Windows Credential Manager) — tried **first** for reads and writes
2. **File system** (`~/.copilot/mcp-oauth-config/{hash}.tokens.json`) — fallback when keytar fails

### The Bug

The following sequence creates a permanent re-auth loop:

1. **First authentication:** User authenticates via browser. Token is **small** (no `refresh_token`, ~1.8KB). Keytar `setPassword` **succeeds** → token stored in OS keychain. File is **not written** (keytar succeeded, no fallback needed).

2. **Subsequent launches:** Access token expires after ~1 hour. CLI reads keytar → finds stale token → no `refresh_token` available → opens browser. User re-authenticates → new token is **larger** (~3.8KB, now includes `refresh_token`). Keytar `setPassword` **silently fails** (payload exceeds credential blob size limit). Falls back to writing `.tokens.json` file.

3. **Next launch:** CLI reads keytar **first** → finds the **original stale entry** (still there because `setPassword` failed but `deletePassword` was never called) → expired, no refresh token → opens browser again. The valid file-based token with a working refresh token is **never read**.

4. **This repeats forever** — the stale keytar entry is never cleaned up.

### Why It Only Affects Some Servers

Servers whose initial tokens are **too large for keytar** (e.g., servers returning 30+ scopes, ~9.6KB tokens) fail the initial keytar write and go straight to file storage. These servers work perfectly — their file-based tokens have refresh tokens and silently refresh on every launch.

Only servers with **small initial tokens** (that fit in keytar) are affected, because the subsequent larger tokens (with refresh tokens) fail the keytar write while the old small entry persists.

## Reproduction Steps

1. Configure multiple HTTP MCP servers in `~/.copilot/mcp-config.json`:
```json
{
"mcpServers": {
"server-a": { "type": "http", "url": "https://some-oauth-mcp-server.example.com/" },
"server-b": { "type": "http", "url": "https://another-oauth-mcp-server.example.com/" }
}
}
```

2. Launch Copilot CLI — browser windows open for each server (expected on first auth).

3. Complete authentication in the browser for all servers.

4. Close the CLI session, wait for the access tokens to expire (~1 hour), then restart.

5. **Observe:** Browser windows open again for servers whose initial tokens were small enough to be saved to keytar.

6. This repeats on every launch indefinitely.

## Evidence from Source Code (app.js, v1.0.7)

### Token read prioritizes keytar over files (stale data wins):
```javascript
// getTokens (function 's' in the store factory)
async getTokens(url) {
let hash = SHA256(url);
if (memoryCache.has(hash)) return memoryCache.get(hash);
try {
let keytarValue = await keytar.getPassword('copilot-mcp-oauth', hash);
if (keytarValue) {
let parsed = JSON.parse(keytarValue);
memoryCache.set(hash, parsed);
return parsed; // ← Returns stale keytar data, never reaches file
}
} catch { /* swallowed */ }
// File fallback — only reached if keytar has NO entry
let filePath = path.join(configDir, `${hash}.tokens.json`);
if (existsSync(filePath)) { /* read and return */ }
}
```

### Token write fails silently, leaving stale keytar entry:
```javascript
// saveTokens (function 'a' in the store factory)
async saveTokens(url, tokens) {
let hash = SHA256(url);
memoryCache.set(hash, tokens);
try {
await keytar.setPassword('copilot-mcp-oauth', hash, JSON.stringify(tokens));
return true; // ← If this succeeds, file is NOT written
} catch {
// Falls through to file — but does NOT delete the stale keytar entry!
}
// File fallback
await writeFile(path.join(configDir, `${hash}.tokens.json`), ...);
}
```

### Refresh failure swallowed silently:
```javascript
async tryRefreshTokens(serverUrl, staticConfig) {
try {
let tokens = await this.store.getTokens(serverUrl);
if (!tokens?.refreshToken) return false; // ← Stale keytar token has no refreshToken!
// ... refresh logic ...
} catch {
return false; // ← Error silently swallowed, falls through to browser auth
}
}
```

## Expected Behavior

- Tokens should silently refresh using cached `refresh_token` — **no browser popups** after initial auth
- If keytar `setPassword` fails, the stale keytar entry should be **deleted** so the file fallback is used on next read
- Alternatively, reads should compare timestamps between keytar and file sources and prefer the **newest**

## Actual Behavior

- Browser opens for affected MCP servers on **every single launch**
- Valid file-based tokens with working refresh tokens are permanently shadowed by stale keytar entries

## Workaround

**Option 1: Delete stale keytar entries manually**
```javascript
// Run with: node -e "..."
const keytar = require('keytar');
const creds = await keytar.findCredentials('copilot-mcp-oauth');
for (const { account } of creds) {
await keytar.deletePassword('copilot-mcp-oauth', account);
console.log(`Deleted: ${account}`);
}
```

**Option 2: Disable keytar entirely**
Set environment variable `COPILOT_DISABLE_KEYTAR=1` to force file-only token storage.

## Proposed Fix

In `saveTokens`, when keytar `setPassword` fails, **delete the existing keytar entry** before falling back to file:

```javascript
async saveTokens(url, tokens) {
let hash = SHA256(url);
memoryCache.set(hash, tokens);
try {
await keytar.setPassword('copilot-mcp-oauth', hash, JSON.stringify(tokens));
return true;
} catch {
// FIX: Delete stale keytar entry so file fallback is used on next read
try { await keytar.deletePassword('copilot-mcp-oauth', hash); } catch {}
}
await writeFile(path.join(configDir, `${hash}.tokens.json`), ...);
}
```

Additionally, consider logging the actual error in `tryRefreshTokens` instead of silently swallowing it — this would have made this issue diagnosable from logs alone.

Guide de contribution

Ouvrir le guide de contribution

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

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