From c3c60b7bab71ebe783b4043243639699d16cdc2b Mon Sep 17 00:00:00 2001 From: Yakout <154746121+MarawanYakout@users.noreply.github.com> Date: Sat, 4 Apr 2026 08:26:56 +0200 Subject: [PATCH] fix: OAuth tokens secure storage for Windows & Linux (#215) * fix: OAuth tokens secure storage for Windows & Linux * fix: OAuth tokens secure storage for Windows & Linux #215 * fix: OAuth tokens secure storage for Windows & Linux #215 * fix: OAuth tokens secure storage for Windows & Linux #215 --- src/cli/handlers/mcp.tsx | 2 +- src/services/mcp/auth.ts | 11 +- src/utils/secureStorage/fallbackStorage.ts | 2 +- src/utils/secureStorage/index.ts | 46 ++++- src/utils/secureStorage/linuxSecretStorage.ts | 86 ++++++++++ .../secureStorage/macOsKeychainHelpers.ts | 14 +- .../secureStorage/macOsKeychainStorage.ts | 2 +- src/utils/secureStorage/plainTextStorage.ts | 2 +- .../secureStorage/platformStorage.test.ts | 158 ++++++++++++++++++ .../secureStorage/windowsCredentialStorage.ts | 97 +++++++++++ 10 files changed, 406 insertions(+), 14 deletions(-) create mode 100644 src/utils/secureStorage/linuxSecretStorage.ts create mode 100644 src/utils/secureStorage/platformStorage.test.ts create mode 100644 src/utils/secureStorage/windowsCredentialStorage.ts diff --git a/src/cli/handlers/mcp.tsx b/src/cli/handlers/mcp.tsx index ac6edaef..cd1f7ec7 100644 --- a/src/cli/handlers/mcp.tsx +++ b/src/cli/handlers/mcp.tsx @@ -175,7 +175,7 @@ export async function mcpRemoveHandler(name: string, options: { const serverBeforeRemoval = getMcpConfigByName(name); const cleanupSecureStorage = () => { if (serverBeforeRemoval && (serverBeforeRemoval.type === 'sse' || serverBeforeRemoval.type === 'http')) { - clearServerTokensFromLocalStorage(name, serverBeforeRemoval); + clearServerTokensFromSecureStorage(name, serverBeforeRemoval); clearMcpClientConfig(name, serverBeforeRemoval); } }; diff --git a/src/services/mcp/auth.ts b/src/services/mcp/auth.ts index 61c94636..939454cc 100644 --- a/src/services/mcp/auth.ts +++ b/src/services/mcp/auth.ts @@ -40,7 +40,7 @@ import { logMCPDebug } from '../../utils/log.js' import { getPlatform } from '../../utils/platform.js' import { getSecureStorage } from '../../utils/secureStorage/index.js' import { clearKeychainCache } from '../../utils/secureStorage/macOsKeychainHelpers.js' -import type { SecureStorageData } from '../../utils/secureStorage/types.js' +import type { SecureStorageData } from '../../utils/secureStorage/index.js' import { sleep } from '../../utils/sleep.js' import { jsonParse, jsonStringify } from '../../utils/slowOperations.js' import { logEvent } from '../analytics/index.js' @@ -573,7 +573,7 @@ export async function revokeServerTokens( } // Always clear local tokens, regardless of server-side revocation result. - clearServerTokensFromLocalStorage(serverName, serverConfig) + clearServerTokensFromSecureStorage(serverName, serverConfig) // When re-authenticating, preserve step-up auth state (scope + discovery) // so the next performMCPOAuthFlow can use cached scope instead of @@ -617,7 +617,8 @@ export async function revokeServerTokens( } } -export function clearServerTokensFromLocalStorage( +// Utilizing platform-specific secure storage to protect sensitive tokens +export function clearServerTokensFromSecureStorage( serverName: string, serverConfig: McpSSEServerConfig | McpHTTPServerConfig, ): void { @@ -629,7 +630,7 @@ export function clearServerTokensFromLocalStorage( if (existingData.mcpOAuth[serverKey]) { delete existingData.mcpOAuth[serverKey] storage.update(existingData) - logMCPDebug(serverName, 'Cleared stored tokens') + logMCPDebug(serverName, 'Cleared stored tokens from secure storage') } } @@ -913,7 +914,7 @@ export async function performMCPOAuthFlow( // Clear any existing stored credentials to ensure fresh client registration. // Note: this deletes the entire entry (including discoveryState/stepUpScope), // but we already read the cached values above. - clearServerTokensFromLocalStorage(serverName, serverConfig) + clearServerTokensFromSecureStorage(serverName, serverConfig) // Use cached step-up scope and resource metadata URL if available. // The transport-attached auth provider caches these when it receives a diff --git a/src/utils/secureStorage/fallbackStorage.ts b/src/utils/secureStorage/fallbackStorage.ts index 8667badd..e6a6b93e 100644 --- a/src/utils/secureStorage/fallbackStorage.ts +++ b/src/utils/secureStorage/fallbackStorage.ts @@ -1,4 +1,4 @@ -import type { SecureStorage, SecureStorageData } from './types.js' +import type { SecureStorage, SecureStorageData } from './index.js' /** * Creates a fallback storage that tries to use the primary storage first, diff --git a/src/utils/secureStorage/index.ts b/src/utils/secureStorage/index.ts index d84f07a9..817d2320 100644 --- a/src/utils/secureStorage/index.ts +++ b/src/utils/secureStorage/index.ts @@ -1,17 +1,57 @@ import { createFallbackStorage } from './fallbackStorage.js' import { macOsKeychainStorage } from './macOsKeychainStorage.js' +import { linuxSecretStorage } from './linuxSecretStorage.js' +import { windowsCredentialStorage } from './windowsCredentialStorage.js' import { plainTextStorage } from './plainTextStorage.js' -import type { SecureStorage } from './types.js' + +export interface SecureStorageData { + mcpOAuth?: Record< + string, + { + serverName: string + serverUrl: string + accessToken: string + refreshToken?: string + expiresAt: number + scope?: string + clientId?: string + clientSecret?: string + discoveryState?: { + authorizationServerUrl: string + resourceMetadataUrl?: string + } + stepUpScope?: string + } + > + mcpOAuthClientConfig?: Record + trustedDeviceToken?: string + pluginSecrets?: Record> +} + +export interface SecureStorage { + name: string + read(): SecureStorageData | null + readAsync(): Promise + update(data: SecureStorageData): { success: boolean; warning?: string } + delete(): boolean +} /** - * Get the appropriate secure storage implementation for the current platform + * Get the appropriate secure storage implementation for the current platform. + * Prefers native OS vaults (Keychain, libsecret, Credential Locker) with a plaintext fallback. */ export function getSecureStorage(): SecureStorage { if (process.platform === 'darwin') { return createFallbackStorage(macOsKeychainStorage, plainTextStorage) } - // TODO: add libsecret support for Linux + if (process.platform === 'linux') { + return createFallbackStorage(linuxSecretStorage, plainTextStorage) + } + + if (process.platform === 'win32') { + return createFallbackStorage(windowsCredentialStorage, plainTextStorage) + } return plainTextStorage } diff --git a/src/utils/secureStorage/linuxSecretStorage.ts b/src/utils/secureStorage/linuxSecretStorage.ts new file mode 100644 index 00000000..525bbc39 --- /dev/null +++ b/src/utils/secureStorage/linuxSecretStorage.ts @@ -0,0 +1,86 @@ +import { execaSync } from 'execa' +import { jsonParse, jsonStringify } from '../slowOperations.js' +import { + CREDENTIALS_SERVICE_SUFFIX, + getSecureStorageServiceName, + getUsername, +} from './macOsKeychainHelpers.js' +import type { SecureStorage, SecureStorageData } from './index.js' + +/** + * Linux-specific secure storage implementation using the secret-tool CLI. + * secret-tool interacts with the Secret Service API (GNOME Keyring, KWallet, etc.). + */ +export const linuxSecretStorage: SecureStorage = { + name: 'libsecret', + read(): SecureStorageData | null { + try { + const username = getUsername() + const serviceName = getSecureStorageServiceName( + CREDENTIALS_SERVICE_SUFFIX, + ) + // secret-tool lookup service [service] account [account] + const result = execaSync( + 'secret-tool', + ['lookup', 'service', serviceName, 'account', username], + { reject: false }, + ) + + if (result.exitCode === 0 && result.stdout) { + return jsonParse(result.stdout) + } + } catch { + // fall through + } + return null + }, + async readAsync(): Promise { + // Reusing sync implementation for simplicity as it wraps a CLI call + return this.read() + }, + update(data: SecureStorageData): { success: boolean; warning?: string } { + try { + const username = getUsername() + const serviceName = getSecureStorageServiceName( + CREDENTIALS_SERVICE_SUFFIX, + ) + const payload = jsonStringify(data) + // secret-tool store --label=[label] service [service] account [account] + // The payload is passed via stdin + const result = execaSync( + 'secret-tool', + [ + 'store', + '--label', + serviceName, + 'service', + serviceName, + 'account', + username, + ], + { input: payload, reject: false }, + ) + + return { success: result.exitCode === 0 } + } catch { + return { success: false } + } + }, + delete(): boolean { + try { + const username = getUsername() + const serviceName = getSecureStorageServiceName( + CREDENTIALS_SERVICE_SUFFIX, + ) + // secret-tool clear service [service] account [account] + const result = execaSync( + 'secret-tool', + ['clear', 'service', serviceName, 'account', username], + { reject: false }, + ) + return result.exitCode === 0 + } catch { + return false + } + }, +} diff --git a/src/utils/secureStorage/macOsKeychainHelpers.ts b/src/utils/secureStorage/macOsKeychainHelpers.ts index 9da8bb2d..f03547c1 100644 --- a/src/utils/secureStorage/macOsKeychainHelpers.ts +++ b/src/utils/secureStorage/macOsKeychainHelpers.ts @@ -18,7 +18,7 @@ import { createHash } from 'crypto' import { userInfo } from 'os' import { getOauthConfig } from 'src/constants/oauth.js' import { getClaudeConfigHomeDir } from '../envUtils.js' -import type { SecureStorageData } from './types.js' +import type { SecureStorageData } from './index.js' // Suffix distinguishing the OAuth credentials keychain entry from the legacy // API key entry (which uses no suffix). Both share the service name base. @@ -26,7 +26,11 @@ import type { SecureStorageData } from './types.js' // orphan existing stored credentials. export const CREDENTIALS_SERVICE_SUFFIX = '-credentials' -export function getMacOsKeychainStorageServiceName( +/** + * Get the service/resource name for secure storage, scoped by CLAUDE_CONFIG_DIR + * if it's set to a non-default location. + */ +export function getSecureStorageServiceName( serviceSuffix: string = '', ): string { const configDir = getClaudeConfigHomeDir() @@ -40,6 +44,12 @@ export function getMacOsKeychainStorageServiceName( return `Claude Code${getOauthConfig().OAUTH_FILE_SUFFIX}${serviceSuffix}${dirHash}` } +export function getMacOsKeychainStorageServiceName( + serviceSuffix: string = '', +): string { + return getSecureStorageServiceName(serviceSuffix) +} + export function getUsername(): string { try { return process.env.USER || userInfo().username diff --git a/src/utils/secureStorage/macOsKeychainStorage.ts b/src/utils/secureStorage/macOsKeychainStorage.ts index 344cfba2..1e4ec37c 100644 --- a/src/utils/secureStorage/macOsKeychainStorage.ts +++ b/src/utils/secureStorage/macOsKeychainStorage.ts @@ -11,7 +11,7 @@ import { KEYCHAIN_CACHE_TTL_MS, keychainCacheState, } from './macOsKeychainHelpers.js' -import type { SecureStorage, SecureStorageData } from './types.js' +import type { SecureStorage, SecureStorageData } from './index.js' // `security -i` reads stdin with a 4096-byte fgets() buffer (BUFSIZ on darwin). // A command line longer than this is truncated mid-argument: the first 4096 diff --git a/src/utils/secureStorage/plainTextStorage.ts b/src/utils/secureStorage/plainTextStorage.ts index e5383fb5..77e6ff59 100644 --- a/src/utils/secureStorage/plainTextStorage.ts +++ b/src/utils/secureStorage/plainTextStorage.ts @@ -8,7 +8,7 @@ import { jsonStringify, writeFileSync_DEPRECATED, } from '../slowOperations.js' -import type { SecureStorage, SecureStorageData } from './types.js' +import type { SecureStorage, SecureStorageData } from './index.js' function getStoragePath(): { storageDir: string; storagePath: string } { const storageDir = getClaudeConfigHomeDir() diff --git a/src/utils/secureStorage/platformStorage.test.ts b/src/utils/secureStorage/platformStorage.test.ts new file mode 100644 index 00000000..eb68edd1 --- /dev/null +++ b/src/utils/secureStorage/platformStorage.test.ts @@ -0,0 +1,158 @@ + +import { expect, test, mock, describe, beforeEach, afterEach } from "bun:test"; +import { getSecureStorage } from "./index.js"; +import { linuxSecretStorage } from "./linuxSecretStorage.js"; +import { windowsCredentialStorage } from "./windowsCredentialStorage.js"; +import { getSecureStorageServiceName, CREDENTIALS_SERVICE_SUFFIX } from "./macOsKeychainHelpers.js"; + +// Mock execaSync +const mockExecaSync = mock(() => ({ exitCode: 0, stdout: "" })); +mock.module("execa", () => ({ + execaSync: mockExecaSync, +})); + +describe("Secure Storage Platform Implementations", () => { + const originalEnv = process.env; + + beforeEach(() => { + process.env = { ...originalEnv }; + mockExecaSync.mockClear(); + // Default mock behavior + mockExecaSync.mockImplementation(() => ({ exitCode: 0, stdout: "" })); + }); + + afterEach(() => { + process.env = originalEnv; + }); + + const testData = { + mcpOAuth: { + "test-server": { + accessToken: "secret-token", + expiresAt: 123456789, + serverName: "test", + serverUrl: "http://test" + } + } + }; + + describe("Config-Dir Isolation", () => { + test("service name changes with CLAUDE_CONFIG_DIR", () => { + const defaultName = getSecureStorageServiceName(CREDENTIALS_SERVICE_SUFFIX); + + process.env.CLAUDE_CONFIG_DIR = "/tmp/other-config"; + const otherName = getSecureStorageServiceName(CREDENTIALS_SERVICE_SUFFIX); + + expect(otherName).not.toBe(defaultName); + expect(otherName).toContain("Claude Code"); + expect(otherName).toContain(CREDENTIALS_SERVICE_SUFFIX); + }); + + test("Linux storage uses scoped service name", () => { + process.env.CLAUDE_CONFIG_DIR = "/tmp/linux-scoped"; + const expectedName = getSecureStorageServiceName(CREDENTIALS_SERVICE_SUFFIX); + + linuxSecretStorage.update(testData); + + const args = mockExecaSync.mock.calls[0]; + expect(args[1]).toContain(expectedName); + }); + + test("Windows storage uses scoped resource name", () => { + process.env.CLAUDE_CONFIG_DIR = "/tmp/win-scoped"; + const expectedName = getSecureStorageServiceName(CREDENTIALS_SERVICE_SUFFIX); + + windowsCredentialStorage.update(testData); + + const script = mockExecaSync.mock.calls[0][1][1]; + expect(script).toContain(expectedName); + expect(script).toContain("Add-Type -AssemblyName System.Runtime.WindowsRuntime"); + }); + }); + + describe("Windows PowerShell Escaping", () => { + test("escapes single quotes and prevents $ expansion", () => { + const dataWithDollar = { + mcpOAuth: { + "server": { + accessToken: "token-with-$env:USERNAME", + expiresAt: 123, + serverName: "s", + serverUrl: "u" + } + } + }; + + windowsCredentialStorage.update(dataWithDollar); + + const script = mockExecaSync.mock.calls[0][1][1]; + // Should use single quotes for the payload + expect(script).toMatch(/'\{.*\}'/); + // Should escape ' by doubling it + expect(script).not.toContain("'token-with-$env:USERNAME'"); + // But since it's JSON, the value will be "token-with-$env:USERNAME" inside the single-quoted string + // The JSON itself shouldn't have single quotes unless the data has them. + + const dataWithQuote = { mcpOAuth: { "s": { accessToken: "token'quote", expiresAt: 1, serverName: "s", serverUrl: "u" } } }; + windowsCredentialStorage.update(dataWithQuote); + const script2 = mockExecaSync.mock.calls[1][1][1]; + expect(script2).toContain("token''quote"); + }); + + test("delete() includes assembly load", () => { + windowsCredentialStorage.delete(); + const script = mockExecaSync.mock.calls[0][1][1]; + expect(script).toContain("Add-Type -AssemblyName System.Runtime.WindowsRuntime"); + }); + + test("escapes double quotes in username", () => { + process.env.USER = 'user"name'; + windowsCredentialStorage.read(); + const script = mockExecaSync.mock.calls[0][1][1]; + expect(script).toContain('user`"name'); + expect(script).not.toContain('user"name'); + }); + }); + + describe("Linux secret-tool Interaction", () => { + test("update passes payload via stdin", () => { + linuxSecretStorage.update(testData); + + const options = mockExecaSync.mock.calls[0][2]; + expect(options.input).toContain("secret-token"); + }); + + test("read parses stdout", () => { + mockExecaSync.mockReturnValue({ exitCode: 0, stdout: JSON.stringify(testData) }); + const result = linuxSecretStorage.read(); + + expect(result).toEqual(testData); + }); + }); + + describe("Platform Selection", () => { + const originalPlatform = process.platform; + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: originalPlatform }); + }); + + test("darwin returns keychain with fallback", () => { + Object.defineProperty(process, 'platform', { value: 'darwin' }); + const storage = getSecureStorage(); + expect(storage.name).toContain("keychain"); + }); + + test("linux returns libsecret with fallback", () => { + Object.defineProperty(process, 'platform', { value: 'linux' }); + const storage = getSecureStorage(); + expect(storage.name).toContain("libsecret"); + }); + + test("win32 returns credential-locker with fallback", () => { + Object.defineProperty(process, 'platform', { value: 'win32' }); + const storage = getSecureStorage(); + expect(storage.name).toContain("credential-locker"); + }); + }); +}); diff --git a/src/utils/secureStorage/windowsCredentialStorage.ts b/src/utils/secureStorage/windowsCredentialStorage.ts new file mode 100644 index 00000000..050fe781 --- /dev/null +++ b/src/utils/secureStorage/windowsCredentialStorage.ts @@ -0,0 +1,97 @@ +import { execaSync } from 'execa' +import { jsonParse, jsonStringify } from '../slowOperations.js' +import { + CREDENTIALS_SERVICE_SUFFIX, + getSecureStorageServiceName, + getUsername, +} from './macOsKeychainHelpers.js' +import type { SecureStorage, SecureStorageData } from './index.js' + +/** + * Windows-specific secure storage implementation using the Windows Credential Locker. + * Accessed via PowerShell's [Windows.Security.Credentials.PasswordVault]. + */ +export const windowsCredentialStorage: SecureStorage = { + name: 'credential-locker', + read(): SecureStorageData | null { + const resourceName = getSecureStorageServiceName( + CREDENTIALS_SERVICE_SUFFIX, + ).replace(/"/g, '`"') + const username = getUsername().replace(/"/g, '`"') + // PowerShell script to retrieve password from vault + const script = ` + Add-Type -AssemblyName System.Runtime.WindowsRuntime + $vault = New-Object Windows.Security.Credentials.PasswordVault + try { + $cred = $vault.Retrieve("${resourceName}", "${username}") + $cred.FillPassword() + $cred.Password + } catch { + exit 1 + } + ` + try { + const result = execaSync('powershell.exe', ['-Command', script], { + reject: false, + }) + if (result.exitCode === 0 && result.stdout) { + return jsonParse(result.stdout) + } + } catch { + // fall through + } + return null + }, + async readAsync(): Promise { + return this.read() + }, + update(data: SecureStorageData): { success: boolean; warning?: string } { + const resourceName = getSecureStorageServiceName( + CREDENTIALS_SERVICE_SUFFIX, + ).replace(/"/g, '`"') + const username = getUsername().replace(/"/g, '`"') + // Use single quotes for the payload and escape ' by doubling it (''). + // This prevents PowerShell from expanding $... inside the string. + const payload = jsonStringify(data).replace(/'/g, "''") + // PowerShell script to add/update credential in vault + const script = ` + Add-Type -AssemblyName System.Runtime.WindowsRuntime + $vault = New-Object Windows.Security.Credentials.PasswordVault + $cred = New-Object Windows.Security.Credentials.PasswordCredential("${resourceName}", "${username}", '${payload}') + $vault.Add($cred) + ` + try { + const result = execaSync('powershell.exe', ['-Command', script], { + reject: false, + }) + return { success: result.exitCode === 0 } + } catch { + return { success: false } + } + }, + delete(): boolean { + const resourceName = getSecureStorageServiceName( + CREDENTIALS_SERVICE_SUFFIX, + ).replace(/"/g, '`"') + const username = getUsername().replace(/"/g, '`"') + // PowerShell script to remove credential from vault + const script = ` + Add-Type -AssemblyName System.Runtime.WindowsRuntime + $vault = New-Object Windows.Security.Credentials.PasswordVault + try { + $cred = $vault.Retrieve("${resourceName}", "${username}") + $vault.Remove($cred) + } catch { + exit 0 + } + ` + try { + const result = execaSync('powershell.exe', ['-Command', script], { + reject: false, + }) + return result.exitCode === 0 + } catch { + return false + } + }, +}