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.
2026-05-23 18:22:37 -07:00 · 2026-02-24 11:24:18 -08:00
parent dae67983a8
commit b83b6a2210
2 changed files with 94 additions and 54 deletions
@@ -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"
      },
@@ -32,27 +32,30 @@ export class KeychainTokenStorage
 {
  private keychainAvailable: boolean | null = null;
  private keytarModule: Keytar | null = null;
-  private keytarLoadAttempted = false;
+  private keytarPromise: Promise<Keytar | null> | null = null;
+  private availabilityPromise: Promise<boolean> | null = null;

  async getKeytar(): Promise<Keytar | null> {
-    // 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<OAuthCredentials | null> {
@@ -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<boolean> {
    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<boolean> {