Skip to content

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

Open
GeiserX wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
GeiserX:fix/mcp-pool-key-plaintext
Open

Keep credentials out of the MCP connection pool's cache key#1573
GeiserX wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
GeiserX:fix/mcp-pool-key-plaintext

Conversation

@GeiserX

@GeiserX GeiserX commented Aug 13, 2026

Copy link
Copy Markdown

TL;DR

The MCP connection pool's cache key contains the connection's credentials, and that key outlives the call by a long way. The key is a Map key held for the pool's lifetime, so a bearer token sits readable in process memory long after the call that needed it finished — with nothing left to read it.

This hashes the key. Equal identities still produce equal keys, so pooled reuse is unchanged; a rotated token or a different auth header still dials a fresh session, so separation is unchanged. Nothing reads the key back, so nothing observable changes.

Not a leak — the key never reaches a log, span or error message. It is unnecessary retention, which is a smaller problem, but a free one to remove.


What is wrong

connectionPoolKey builds the identity a pooled remote MCP session is looked up by:

JSON.stringify({
  endpoint, transport, remoteTransport,
  headers:     sortedRecord(input.headers),      // ← rendered credential
  queryParams: sortedRecord(input.queryParams),  // ← rendered credential
  template,
  values:      sortedRecord(values),             // ← the raw secret
})

The secret arrives by two routes, not one. values is the connection's resolved credential inputs — for an OAuth connection that is the access token itself. And buildConnectorInput has already rendered those same secrets onto headers / queryParams (Authorization: Bearer …, or ?token=… for servers that authenticate that way), so they are in the key a second time.

That string is then stored as a key in the pool's Map and kept until the pool drops the entry. The call that needed the credential is long finished; the credential is still there.

What changes

The key becomes the SHA-256 digest of the same identity. One function, one call site.

The whole serialised identity is hashed rather than only the fields known to hold secrets. That is deliberate: the two credential-bearing fields today are values and headers/queryParams, but a field added later would have to be remembered to be sensitive. Hashing everything covers it by construction. Nothing is lost by making the key opaque, because nothing ever reads it back — the pool only compares it.

SHA-256 rather than a cheap non-cryptographic hash is also deliberate. A collision here means reusing a connection authenticated as somebody else, so it has to be one that a user who controls their own credential values cannot aim at another key.

Why reuse and separation both still hold

These pull in opposite directions, and a change that satisfied only one of them would be worse than doing nothing — dropping the credential from the key entirely would look like a privacy improvement and be a session-hijack bug.

  • Reuse: same identity → same JSON → same digest. Sequential calls on one connection still share a session.
  • Separation: a refreshed access token, a different rendered auth header, or a different query-param credential each change the JSON, so each still forces a fresh dial rather than reusing a session authenticated with the old value.

Tests

packages/plugins/mcp/src/sdk/connection-pool-key.test.ts — 7 tests pinning both halves: the key is a bare 64-char hex digest carrying no plaintext, the same identity is stable, and a rotated value / a different auth header / a query-param credential / a different endpoint / a different template each separate. Insertion order still does not split one identity in two.

Every test was mutation-checked, with each mutation verified to have landed in the file and an unmutated control run before and after:

mutation result
return the identity unhashed (i.e. revert this PR) killed
drop values from the key killed
drop headers from the key killed
drop queryParams from the key killed
drop endpoint from the key killed
drop template from the key killed
stop sorting (insertion order leaks in) killed

Full package: 133 passed, 29 skipped. tsgo --noEmit, oxlint --deny-warnings and oxfmt --check all clean on the changed files.

The pool key describing a remote MCP session's identity embedded the
connection's resolved credential values, and the headers and query params
those same secrets had already been rendered into. That 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, with nothing left to read it.

Hash the whole serialised identity instead. Equal identities still produce
equal keys, so reuse is unchanged, and a rotated token, a different rendered
auth header or a query-param credential each still dial a fresh session.
Hashing everything rather than the fields known to be sensitive means a field
added later is covered without anyone having to remember it carries a secret.

SHA-256 rather than a cheap hash on purpose: a collision would mean reusing a
connection authenticated as somebody else.
@GeiserX

GeiserX commented Aug 13, 2026

Copy link
Copy Markdown
Author

Context for this one: #1585 explains why this PR and twelve others exist — they came out of a single pass over credential handling, asking for each credential where it ends up, how long it stays, and who can read it once it's there.

This PR stands alone and doesn't depend on any of the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant