From b83b6a22100236b346b6dbebfa9918164cd18210 Mon Sep 17 00:00:00 2001 From: galz10 Date: Tue, 24 Feb 2026 11:24:18 -0800 Subject: [PATCH] fix(core): resolve race conditions in KeychainTokenStorage loading Ensures that multiple concurrent calls to getKeytar or checkKeychainAvailability wait for a single operation to complete using promise-based guards. This prevents scenarios where a slow module load or availability check could result in a permanently cached 'unavailable' state. --- package-lock.json | 28 +++- .../token-storage/keychain-token-storage.ts | 120 ++++++++++-------- 2 files changed, 94 insertions(+), 54 deletions(-) diff --git a/package-lock.json b/package-lock.json index 2b027e0246..6b3acb975e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2129,6 +2129,7 @@ "resolved": "https://registry.npmjs.org/@modelcontextprotocol/sdk/-/sdk-1.26.0.tgz", "integrity": "sha512-Y5RmPncpiDtTXDbLKswIJzTqu2hyBKxTNsgKqKclDbhIgg1wgtf1fRuvxgTnRfcnxtvvgbIEcqUOzZrJ6iSReg==", "license": "MIT", + "peer": true, "dependencies": { "@hono/node-server": "^1.19.9", "ajv": "^8.17.1", @@ -2271,6 +2272,7 @@ "integrity": "sha512-t54CUOsFMappY1Jbzb7fetWeO0n6K0k/4+/ZpkS+3Joz8I4VcvY9OiEBFRYISqaI2fq5sCiPtAjRDOzVYG8m+Q==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@octokit/auth-token": "^6.0.0", "@octokit/graphql": "^9.0.2", @@ -2451,6 +2453,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/api/-/api-1.9.0.tgz", "integrity": "sha512-3giAOQvZiH5F9bMlMiv8+GSPMeqg0dbaeo58/0SlA9sxSqZhnUtxzX9/2FzyhS9sWQf5S0GJE0AKBrFqjpeYcg==", "license": "Apache-2.0", + "peer": true, "engines": { "node": ">=8.0.0" } @@ -2500,6 +2503,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/core/-/core-2.5.0.tgz", "integrity": "sha512-ka4H8OM6+DlUhSAZpONu0cPBtPPTQKxbxVzC4CzVx5+K4JnroJVBtDzLAMx4/3CDTJXRvVFhpFjtl4SaiTNoyQ==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@opentelemetry/semantic-conventions": "^1.29.0" }, @@ -2874,6 +2878,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/resources/-/resources-2.5.0.tgz", "integrity": "sha512-F8W52ApePshpoSrfsSk1H2yJn9aKjCrbpQF1M9Qii0GHzbfVeFUB+rc3X4aggyZD8x9Gu3Slua+s6krmq6Dt8g==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@opentelemetry/core": "2.5.0", "@opentelemetry/semantic-conventions": "^1.29.0" @@ -2907,6 +2912,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/sdk-metrics/-/sdk-metrics-2.5.0.tgz", "integrity": "sha512-BeJLtU+f5Gf905cJX9vXFQorAr6TAfK3SPvTFqP+scfIpDQEJfRaGJWta7sJgP+m4dNtBf9y3yvBKVAZZtJQVA==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@opentelemetry/core": "2.5.0", "@opentelemetry/resources": "2.5.0" @@ -2961,6 +2967,7 @@ "resolved": "https://registry.npmjs.org/@opentelemetry/sdk-trace-base/-/sdk-trace-base-2.5.0.tgz", "integrity": "sha512-VzRf8LzotASEyNDUxTdaJ9IRJ1/h692WyArDBInf5puLCjxbICD6XkHgpuudis56EndyS7LYFmtTMny6UABNdQ==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@opentelemetry/core": "2.5.0", "@opentelemetry/resources": "2.5.0", @@ -4124,6 +4131,7 @@ "integrity": "sha512-6mDvHUFSjyT2B2yeNx2nUgMxh9LtOWvkhIU3uePn2I2oyNymUAX1NIsdgviM4CH+JSrp2D2hsMvJOkxY+0wNRA==", "devOptional": true, "license": "MIT", + "peer": true, "dependencies": { "csstype": "^3.0.2" } @@ -4398,6 +4406,7 @@ "integrity": "sha512-6sMvZePQrnZH2/cJkwRpkT7DxoAWh+g6+GFRK6bV3YQo7ogi3SX5rgF6099r5Q53Ma5qeT7LGmOmuIutF4t3lA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.35.0", "@typescript-eslint/types": "8.35.0", @@ -5323,6 +5332,7 @@ "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.15.0.tgz", "integrity": "sha512-NZyJarBfL7nWwIq+FDL6Zp/yHEhePMNnnJ0y3qfieCrmNvYct8uvtiV41UvlSe6apAfk0fY1FbWx+NwfmpvtTg==", "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -7890,6 +7900,7 @@ "integrity": "sha512-GsGizj2Y1rCWDu6XoEekL3RLilp0voSePurjZIkxL3wlm5o5EC9VpgaP7lrCvjnkuLvzFBQWB3vWB3K5KQTveQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@eslint-community/eslint-utils": "^4.2.0", "@eslint-community/regexpp": "^4.12.1", @@ -8410,6 +8421,7 @@ "resolved": "https://registry.npmjs.org/express/-/express-5.2.1.tgz", "integrity": "sha512-hIS4idWWai69NezIdRt2xFVofaF4j+6INOpJlVOLDO8zXGpUVEVzIYk12UUi2JzjEzWL3IOAxcTubgz9Po0yXw==", "license": "MIT", + "peer": true, "dependencies": { "accepts": "^2.0.0", "body-parser": "^2.2.1", @@ -9706,6 +9718,7 @@ "resolved": "https://registry.npmjs.org/hono/-/hono-4.11.9.tgz", "integrity": "sha512-Eaw2YTGM6WOxA6CXbckaEvslr2Ne4NFsKrvc0v97JD5awbmeBLO5w9Ho9L9kmKonrwF9RJlW6BxT1PVv/agBHQ==", "license": "MIT", + "peer": true, "engines": { "node": ">=16.9.0" } @@ -10006,6 +10019,7 @@ "resolved": "https://registry.npmjs.org/@jrichman/ink/-/ink-6.4.11.tgz", "integrity": "sha512-93LQlzT7vvZ1XJcmOMwN4s+6W334QegendeHOMnEJBlhnpIzr8bws6/aOEHG8ZCuVD/vNeeea5m1msHIdAY6ig==", "license": "MIT", + "peer": true, "dependencies": { "@alcalzone/ansi-tokenize": "^0.2.1", "ansi-escapes": "^7.0.0", @@ -13694,6 +13708,7 @@ "resolved": "https://registry.npmjs.org/react/-/react-19.2.4.tgz", "integrity": "sha512-9nfp2hYpCwOjAN+8TZFGhtWEwgvWHXqESH8qT89AT/lWklpLON22Lc8pEtnpsZz7VmawabSU0gCjnj8aC0euHQ==", "license": "MIT", + "peer": true, "engines": { "node": ">=0.10.0" } @@ -13704,6 +13719,7 @@ "integrity": "sha512-ePrwPfxAnB+7hgnEr8vpKxL9cmnp7F322t8oqcPshbIQQhDKgFDW4tjhF2wjVbdXF9O/nyuy3sQWd9JGpiLPvA==", "devOptional": true, "license": "MIT", + "peer": true, "dependencies": { "shell-quote": "^1.6.1", "ws": "^7" @@ -15757,6 +15773,7 @@ "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz", "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -15980,7 +15997,8 @@ "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.8.1.tgz", "integrity": "sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==", "dev": true, - "license": "0BSD" + "license": "0BSD", + "peer": true }, "node_modules/tsx": { "version": "4.20.3", @@ -15988,6 +16006,7 @@ "integrity": "sha512-qjbnuR9Tr+FJOMBqJCW5ehvIo/buZq7vH7qD7JziU98h6l3qGy0a/yPFjwO+y0/T7GFpNgNAvEcPPVfyT8rrPQ==", "devOptional": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "~0.25.0", "get-tsconfig": "^4.7.5" @@ -16148,6 +16167,7 @@ "integrity": "sha512-p1diW6TqL9L07nNxvRMM7hMMw4c5XOo/1ibL4aAIGmSAt9slTE1Xgw5KWuof2uTOvCg9BY7ZRi+GaF+7sfgPeQ==", "devOptional": true, "license": "Apache-2.0", + "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -16355,6 +16375,7 @@ "resolved": "https://registry.npmjs.org/vite/-/vite-7.2.2.tgz", "integrity": "sha512-BxAKBWmIbrDgrokdGZH1IgkIk/5mMHDreLDmCJ0qpyJaAteP8NvMhkwr/ZCQNqNH97bw/dANTE9PDzqwJghfMQ==", "license": "MIT", + "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.5.0", @@ -16468,6 +16489,7 @@ "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz", "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -16480,6 +16502,7 @@ "resolved": "https://registry.npmjs.org/vitest/-/vitest-3.2.4.tgz", "integrity": "sha512-LUCP5ev3GURDysTWiP47wRRUpLKMOfPh+yKTx3kVIEiu5KOMeqzpnYNsKyOoVrULivR8tLcks4+lga33Whn90A==", "license": "MIT", + "peer": true, "dependencies": { "@types/chai": "^5.2.2", "@vitest/expect": "3.2.4", @@ -17111,6 +17134,7 @@ "resolved": "https://registry.npmjs.org/zod/-/zod-3.25.76.tgz", "integrity": "sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==", "license": "MIT", + "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } @@ -17448,6 +17472,7 @@ "shell-quote": "^1.8.3", "simple-git": "^3.28.0", "strip-ansi": "^7.1.0", + "strip-json-comments": "^3.1.1", "systeminformation": "^5.25.11", "tree-sitter-bash": "^0.25.0", "undici": "^7.10.0", @@ -17648,6 +17673,7 @@ "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.3.tgz", "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, diff --git a/packages/core/src/mcp/token-storage/keychain-token-storage.ts b/packages/core/src/mcp/token-storage/keychain-token-storage.ts index 4be0d082e5..74c6aa15dc 100644 --- a/packages/core/src/mcp/token-storage/keychain-token-storage.ts +++ b/packages/core/src/mcp/token-storage/keychain-token-storage.ts @@ -32,27 +32,30 @@ export class KeychainTokenStorage { private keychainAvailable: boolean | null = null; private keytarModule: Keytar | null = null; - private keytarLoadAttempted = false; + private keytarPromise: Promise | null = null; + private availabilityPromise: Promise | null = null; async getKeytar(): Promise { - // If we've already tried loading (successfully or not), return the result - if (this.keytarLoadAttempted) { - return this.keytarModule; + if (this.keytarPromise) { + return this.keytarPromise; } - this.keytarLoadAttempted = true; + this.keytarPromise = (async () => { + try { + // Try to import keytar without any timeout - let the OS handle it + const moduleName = 'keytar'; + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + const module = await import(moduleName); + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + this.keytarModule = module.default || module; + return this.keytarModule; + } catch (_) { + //Keytar is optional so we shouldn't raise an error of log anything. + return null; + } + })(); - try { - // Try to import keytar without any timeout - let the OS handle it - const moduleName = 'keytar'; - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const module = await import(moduleName); - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - this.keytarModule = module.default || module; - } catch (_) { - //Keytar is optional so we shouldn't raise an error of log anything. - } - return this.keytarModule; + return this.keytarPromise; } async getCredentials(serverName: string): Promise { @@ -240,49 +243,60 @@ export class KeychainTokenStorage } } - // Checks whether or not a set-get-delete cycle with the keychain works. - // Returns false if any operation fails. async checkKeychainAvailability(): Promise { if (this.keychainAvailable !== null) { return this.keychainAvailable; } - try { - const keytar = await this.getKeytar(); - if (!keytar) { - this.keychainAvailable = false; - return false; - } - - const testAccount = `${KEYCHAIN_TEST_PREFIX}${crypto.randomBytes(8).toString('hex')}`; - const testPassword = 'test'; - - await keytar.setPassword(this.serviceName, testAccount, testPassword); - const retrieved = await keytar.getPassword(this.serviceName, testAccount); - const deleted = await keytar.deletePassword( - this.serviceName, - testAccount, - ); - - const success = deleted && retrieved === testPassword; - this.keychainAvailable = success; - - coreEvents.emitTelemetryKeychainAvailability( - new KeychainAvailabilityEvent(success), - ); - - return success; - } catch (_error) { - this.keychainAvailable = false; - - // Do not log the raw error message to avoid potential PII leaks - // (e.g. from OS-level error messages containing file paths) - coreEvents.emitTelemetryKeychainAvailability( - new KeychainAvailabilityEvent(false), - ); - - return false; + if (this.availabilityPromise) { + return this.availabilityPromise; } + + this.availabilityPromise = (async () => { + try { + const keytar = await this.getKeytar(); + if (!keytar) { + this.keychainAvailable = false; + return false; + } + + const testAccount = `${KEYCHAIN_TEST_PREFIX}${crypto.randomBytes(8).toString('hex')}`; + const testPassword = 'test'; + + await keytar.setPassword(this.serviceName, testAccount, testPassword); + const retrieved = await keytar.getPassword( + this.serviceName, + testAccount, + ); + const deleted = await keytar.deletePassword( + this.serviceName, + testAccount, + ); + + const success = deleted && retrieved === testPassword; + this.keychainAvailable = success; + + coreEvents.emitTelemetryKeychainAvailability( + new KeychainAvailabilityEvent(success), + ); + + return success; + } catch (_error) { + this.keychainAvailable = false; + + // Do not log the raw error message to avoid potential PII leaks + // (e.g. from OS-level error messages containing file paths) + coreEvents.emitTelemetryKeychainAvailability( + new KeychainAvailabilityEvent(false), + ); + + return false; + } finally { + this.availabilityPromise = null; + } + })(); + + return this.availabilityPromise; } async isAvailable(): Promise {