Keep credentials out of the MCP connection pool's cache key

.changeset/mcp-pool-key-credential-retention.md

@@ -0,0 +1,11 @@
+---
+"executor": patch
+---
+
+**The MCP connection pool no longer keeps credentials in its cache key**
+
+A pooled remote MCP session is looked up by a key describing the connection's identity, and that key included the connection's resolved credential values — plus the headers and query params those same secrets had already been rendered into. The key is retained as a `Map` key for the pool's lifetime, so the secret stayed readable in process memory long after the call that needed it had finished, with nothing left to read it.
+
+The key is now the SHA-256 digest of that identity rather than the identity itself. Reuse is unchanged, because equal identities still produce equal keys, and separation is unchanged too: a rotated access token, a different rendered auth header and a credential carried in a query param each still dial a fresh session instead of reusing one authenticated as somebody else. Hashing the whole identity rather than only the fields known to be sensitive means a field added later is covered without anyone having to remember it carries a secret.
+
+Nothing reads the key back — the pool only compares it, and it reaches no log, span or error message — so nothing observable changes.

packages/plugins/mcp/src/sdk/connection-pool-key.test.ts

@@ -0,0 +1,154 @@
+// ---------------------------------------------------------------------------
+// MCP connection-pool key
+//
+// The key decides which pooled session a call may reuse, and it is retained as
+// a `Map` key for the POOL's lifetime — much longer than the call that produced
+// it. Two things therefore have to hold at once, and they pull in opposite
+// directions:
+//
+//   * it must still SEPARATE identities — a different credential value, or a
+//     different rendered auth header, must never reuse somebody else's
+//     authenticated session;
+//   * it must not RETAIN the credential — the secret that distinguishes two
+//     identities must not survive in the key that distinguishes them.
+//
+// A digest satisfies both. These tests pin both halves, because a change that
+// satisfied only the second (say, dropping the credential from the key) would
+// look like a privacy improvement and be a session-hijack bug.
+//
+// The pool itself composes on top: it is a `Map` keyed by this string, so
+// "different key" ⇒ "different session" is the pool's own property, covered in
+// `connection-pool.test.ts`.
+// ---------------------------------------------------------------------------
+
+import { describe, expect, it } from "@effect/vitest";
+import { Effect } from "effect";
+
+import { connectionPoolKey } from "./plugin";
+import type { ConnectorInput } from "./connection";
+
+const SECRET = "sk-live-poolkey-Zq7!x-SECRET";
+const OTHER_SECRET = "sk-live-poolkey-Zq7!x-ROTATED";
+
+type RemoteInput = Extract<ConnectorInput, { readonly transport: "remote" }>;
+
+const remoteInput = (overrides: Partial<RemoteInput> = {}): RemoteInput => ({
+  transport: "remote",
+  endpoint: "https://mcp.example.com/sse",
+  remoteTransport: "streamable-http",
+  headers: { authorization: `Bearer ${SECRET}` },
+  ...overrides,
+});
+
+describe("MCP connection-pool key", () => {
+  it.effect("is a bare SHA-256 digest — no plaintext rides along", () =>
+    Effect.gen(function* () {
+      const key = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET });
+
+      // Asserted positively as well as negatively: "does not contain the
+      // secret" alone would still pass for a key that appended the digest to
+      // the plaintext identity.
+      expect(key).toMatch(/^[0-9a-f]{64}$/);
+      expect(key).not.toContain(SECRET);
+      expect(key).not.toContain(`Bearer ${SECRET}`);
+      expect(key).not.toContain("mcp.example.com");
+    }),
+  );
+
+  it.effect("the same identity keeps producing the same key, so reuse is unchanged", () =>
+    Effect.gen(function* () {
+      const first = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET });
+      const second = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET });
+
+      expect(first).toBe(second);
+    }),
+  );
+
+  it.effect("a rotated credential value produces a different key", () =>
+    Effect.gen(function* () {
+      // The case this field exists for: a refreshed access token must dial a
+      // fresh session rather than reuse one authenticated with the old token.
+      const before = yield* connectionPoolKey(remoteInput({ headers: {} }), "bearer", {
+        token: SECRET,
+      });
+      const after = yield* connectionPoolKey(remoteInput({ headers: {} }), "bearer", {
+        token: OTHER_SECRET,
+      });
+
+      expect(after).not.toBe(before);
+    }),
+  );
+
+  it.effect("a different rendered auth header produces a different key", () =>
+    Effect.gen(function* () {
+      // `buildConnectorInput` renders apikey placements onto `headers`, so the
+      // same secret reaches the key by a second route. Separation has to hold
+      // there too.
+      const mine = yield* connectionPoolKey(
+        remoteInput({ headers: { authorization: `Bearer ${SECRET}` } }),
+        "bearer",
+        {},
+      );
+      const theirs = yield* connectionPoolKey(
+        remoteInput({ headers: { authorization: `Bearer ${OTHER_SECRET}` } }),
+        "bearer",
+        {},
+      );
+
+      expect(theirs).not.toBe(mine);
+    }),
+  );
+
+  it.effect("a credential carried in a query param separates too", () =>
+    Effect.gen(function* () {
+      // Servers that authenticate via `?token=` put the secret here instead.
+      const mine = yield* connectionPoolKey(
+        remoteInput({ headers: {}, queryParams: { token: SECRET } }),
+        "query",
+        {},
+      );
+      const theirs = yield* connectionPoolKey(
+        remoteInput({ headers: {}, queryParams: { token: OTHER_SECRET } }),
+        "query",
+        {},
+      );
+
+      expect(theirs).not.toBe(mine);
+      expect(mine).not.toContain(SECRET);
+    }),
+  );
+
+  it.effect("insertion order does not split one identity into two", () =>
+    Effect.gen(function* () {
+      // `sortedRecord` exists for this: a key that changed with property order
+      // would silently dial a new session per call and never reuse anything.
+      const oneWay = yield* connectionPoolKey(
+        remoteInput({ headers: { authorization: `Bearer ${SECRET}`, "x-team": "acme" } }),
+        "bearer",
+        { token: SECRET, region: "eu" },
+      );
+      const otherWay = yield* connectionPoolKey(
+        remoteInput({ headers: { "x-team": "acme", authorization: `Bearer ${SECRET}` } }),
+        "bearer",
+        { region: "eu", token: SECRET },
+      );
+
+      expect(otherWay).toBe(oneWay);
+    }),
+  );
+
+  it.effect("a different endpoint or template separates identities", () =>
+    Effect.gen(function* () {
+      const base = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET });
+      const otherEndpoint = yield* connectionPoolKey(
+        remoteInput({ endpoint: "https://mcp.other.example.com/sse" }),
+        "bearer",
+        { token: SECRET },
+      );
+      const otherTemplate = yield* connectionPoolKey(remoteInput(), "apikey", { token: SECRET });
+
+      expect(otherEndpoint).not.toBe(base);
+      expect(otherTemplate).not.toBe(base);
+    }),
+  );
+});

packages/plugins/mcp/src/sdk/plugin.ts

@@ -14,6 +14,7 @@ import {
   IntegrationSlug,
   mergeAuthTemplates,
   OAuthClientSlug,
+  sha256Hex,
   tool,
   ToolResult,
   type AuthMethodDescriptor,
@@ -628,20 +629,45 @@ const sortedRecord = (
       .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)),
   );
 
-const connectionPoolKey = (
+/** The pooled remote connection's identity, as an opaque digest.
+ *
+ *  HASHED, not carried in the clear, because three of the fields below hold a
+ *  live credential: `values` is the connection's resolved secret inputs, and
+ *  `headers` / `queryParams` are those SAME secrets already rendered onto the
+ *  outbound request by `buildConnectorInput`. The result is retained as a `Map`
+ *  key for the POOL's lifetime (`connection-pool.ts`), which outlives by far the
+ *  call that needed the secret — so a plaintext key leaves credentials sitting
+ *  in process memory with no reader.
+ *
+ *  Hashing the WHOLE serialized identity rather than only the fields known to
+ *  be sensitive keeps equality exactly (same identity → same digest, so reuse is
+ *  unchanged) and keeps any field added later covered without anyone having to
+ *  remember it carries a secret. Nothing reads the key back: the pool only ever
+ *  compares it, and it reaches no log, span or error message.
+ *
+ *  SHA-256 rather than a cheap non-cryptographic hash on purpose. A collision
+ *  means reusing a connection authenticated as somebody else, so the hash has to
+ *  be one an attacker who controls their own credential values cannot aim.
+ *
+ *  Exported for tests (not re-exported from `sdk/index.ts`, so this widens no
+ *  public API): the retention property is a property of the KEY, and asserting
+ *  it through pool behaviour alone would not see it. */
+export const connectionPoolKey = (
   input: Extract<ConnectorInput, { readonly transport: "remote" }>,
   template: string,
   values: Record<string, string | null>,
-): string =>
-  JSON.stringify({
-    endpoint: input.endpoint,
-    transport: input.transport,
-    remoteTransport: input.remoteTransport,
-    headers: sortedRecord(input.headers),
-    queryParams: sortedRecord(input.queryParams),
-    template,
-    values: sortedRecord(values),
-  });
+): Effect.Effect<string> =>
+  sha256Hex(
+    JSON.stringify({
+      endpoint: input.endpoint,
+      transport: input.transport,
+      remoteTransport: input.remoteTransport,
+      headers: sortedRecord(input.headers),
+      queryParams: sortedRecord(input.queryParams),
+      template,
+      values: sortedRecord(values),
+    }),
+  );
 
 // ---------------------------------------------------------------------------
 // Declared auth methods — project the stored MCP config into the catalog's
@@ -1306,7 +1332,11 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
         const connector: McpConnector = createMcpConnector(connectorInput);
         const poolKey =
           connectorInput.transport === "remote"
-            ? connectionPoolKey(connectorInput, String(credential.template), credential.values)
+            ? yield* connectionPoolKey(
+                connectorInput,
+                String(credential.template),
+                credential.values,
+              )
             : undefined;
 
         const connectionRef = {