Why these nineteen PRs exist: two passes over credential handling #1585

Why I opened nineteen pull requests

I've sent a run of PRs over the past couple of days and they probably look disconnected from the outside — a file permission here, a cache key there, a log message somewhere else. They aren't. They came out of one exercise, and I think the whole reads better than the parts, so this issue is the index and the reasoning behind them.

Short version: we run Executor with real credentials, in a place where it matters to us if one leaks. So I went looking, systematically, for every spot where a credential lives longer than it needs to, travels further than it needs to, or ends up somewhere nobody meant it to go. Eighteen of the PRs are what I found. The other one (#1564) is a small capability change that came out of the same look. Sixteen came from the first pass; a second pass over the same question afterwards turned up three more, which are marked below.

None of this is a criticism of the project. Most of what I found is the ordinary kind of thing that accumulates in any codebase handling secrets — a value that was fine where it was written and became a problem two hops away. I found them because I was specifically hunting for them, with a fairly paranoid threat model, not because they were lying around obviously.

What I'd like from you

Putting this first, because the rest is context and you may not want all of it:

• Review them individually, not as a batch. Each is independent and mergeable alone. If some are wrong or unwanted, I'd genuinely rather they were closed than merged out of politeness.
• Let a credential provider own the OAuth refresh grant #1564 is the only one that's a design discussion. The other eighteen are ordinary fixes.
• If approving CI runs for the fork is easy on your side, that would help. None of these have had checks run, because workflow runs from a fork need a maintainer's approval — so you're currently being asked to take my word that the tests pass, which isn't a reasonable thing to ask. If there's a reason not to, that's fine; I'd just stop expecting checks to appear.

Nothing here is urgent, and none of it is a report of something broken in production.

The direction behind it, briefly

Worth saying, because it explains the shape of the whole set. Where we're going is running Executor with credentials held in a TEE — a hardware-isolated environment the host process can't read into. What that buys is a credential the software can use without ever holding: if the machine is compromised, what an attacker finds is ciphertext and a handle, not the key. "We can't read it" is a much stronger thing to be able to say than "we promise not to look".

#1564 is the only PR that moves toward that directly — it lets a provider perform the OAuth refresh itself, so the host never has to be handed the refresh token in order to spend it.

The other eighteen are the groundwork, and they matter more than they look. A sealed store is worth very little while the same credential is also sitting in a cache key, an error message, a world-readable file, or a browser's localStorage — a secret that never leaves the enclave through the front door is no safer if it left through a log line an hour earlier. Most of what I found is exactly that: not the credential store failing, but copies of the credential accumulating around it.

That's also why I'd rather these were judged as ordinary fixes than as a strategy. Each one is worth doing whether or not anyone ever puts a TEE behind it.

One thing worth adding, since it changes how much weight to put on the above: the TEE half is not hypothetical. Executor's credential paths have been exercised against a real hardware-attested enclave — GCP Confidential Space, production image, attestation verified against Google's live JWKS rather than a fixture. The findings themselves came from driving Executor directly rather than out of that run — the enclave is what they are groundwork for, not where they were caught. It is still why several of them are about what an operator can observe: in that setting the interesting failure is not a credential leaking, it is a control that quietly did not engage while everything still looked fine.

The question I was asking

For each credential Executor touches, I asked three things:

1. Where does it end up? Not where it's meant to go — where it actually ends up. Error messages, cache keys, log lines, browser storage, module-level globals, files on disk. 2. How long does it stay? A secret that's correct to hold for one call is a different thing when something keeps it for the lifetime of the process. 3. Who can read it once it's there? File modes, and what a person with access to the machine or the logs would see.

That framing is worth stating because it explains why the PRs look scattered. They're scattered because credentials are scattered; the question was the same every time.

Credit where it's due

A good part of the thinking behind this — particularly the idea that a credential should be usable without ever being held, and that "we simply cannot read it" is a stronger promise than "we promise not to look" — came out of conversations with @alexboone29. The framing is his. The bugs are mine to have found and, where I got something wrong in a PR, mine to have got wrong.

The pull requests

A capability, and the only one that's a design question rather than a fix:

• Let a credential provider own the OAuth refresh grant #1564
  Let a credential provider perform the OAuth refresh exchange itself, instead of handing the refresh token to Executor to spend. Optional; providers that don't implement it are completely unaffected. This is the one that needs a direction from you, and the rest don't depend on it.

Credentials ending up somewhere they weren't meant to:

• Keep token material out of OAuth token-endpoint errors #1567
  A malformed 200 from a token endpoint put an access token inside an error object, where anything logging that error would write the token out.
• Keep credentials out of the MCP connection pool's cache key #1573
  The MCP connection pool's cache key contained the connection's credentials, and that key is kept for the pool's lifetime rather than the call's.
• Clear the 1Password service-account token from the op-js global after each call #1574
  The 1Password backend left its service-account token on a module-level global after each call. One of the smallest of the set.
• Keep a query-carried credential out of GraphQL introspection's failure log #1575
  GraphQL introspection built its request from a URL string, so a credential carried in the query string landed in the error message and went to the log on any transport failure.
• Clear the OAuth popup result from localStorage after handover #1570
  The OAuth popup wrote its result to localStorage as a fallback and never cleared it. Not a credential: the parked payload is the identity label (an email) and, on failure, an error preview.
• fix(mcp): stop handing stdio servers every secret in executor's environment #1595 (second pass)
  Stdio MCP servers were handed every environment variable Executor holds, including EXECUTOR_SECRET_KEY. The MCP SDK ships a sudo-style safe-list for exactly this and merges what we pass on top of it — so spreading process.env did not extend that list, it overwrote it.
• fix(health): keep credentials out of the response sample we persist #1596 (second pass)
  A health check writes its response sample to connection.last_health, unredacted, while the field next to it is scrubbed. The probed operation is user-chosen, so it can be a key-listing endpoint as easily as a /me.

Credentials outliving what they were for:

• Delete the credential a connection minted when the connection is removed #1568
  Removing a connection left the credential it had minted in the store, unreferenced and invisible, including the long-lived refresh token.
• Sweep expired authorization sessions so abandoned PKCE verifiers do not persist #1569
  Abandoned authorization sessions were never swept, so their PKCE verifiers stayed in the database indefinitely.
• Sweep every expired MCP connection, not just the one being asked for #1577
  The connection pool's idle window was only checked for the key being asked for, so a session nobody dialled again stayed open and authenticated.
• Defer irreversible cleanup until the removal commits #1572
  Cleanup that reaches outside the database ran inside the transaction that authorised it, so a rollback could leave the row alive with its credential already deleted.

Files anyone on the machine could read:

• Write the CLI server-connection store owner-only, and make its tests run #1576
  ~/.executor/server-connections.json held a bearer token, or an OAuth access and refresh token, and was created world-readable. Also revives two tests in that file that were never actually running.
• Write the desktop settings store owner-only #1579
  The same problem in the desktop settings store.
• fix(local): stop the database being readable by other accounts on the machine #1594 (second pass)
  ~/.executor/data.db and its WAL sidecars are created world-readable, while auth.json beside them is 0600. The database holds live PKCE verifiers, health-check response samples, and — for pre-revamp stdio MCP integrations — plaintext secret env. Same fix as Write the CLI server-connection store owner-only, and make its tests run #1576, on the file that pass missed.

Telling the truth about state:

• Report expired, not healthy, when an MCP connection's credential is missing #1582
  An MCP connection with a missing credential reported healthy, which is the kind of wrong answer that hides the other kind.
• Stop reporting a rejected credential as a missing tool #1586
  When a connection's credential is rejected upstream it discovers no tools, and asking for one reported that the tool did not exist. The reader goes looking for a renamed or removed tool, which is the one thing that is not wrong.
• Refuse an empty credential value instead of dialing with it #1587
  A credential input supplied as "" read as present, so the request went out with an empty value and the upstream's 401 became the error the operator saw. The OpenAPI backing already treated "" as missing, so this was an inconsistency rather than a choice.
• Fail a credential lookup when the store stops answering, instead of hanging #1588
  A credential provider is often remote, and nothing bounded the call. A store that stopped answering hung the invocation rather than failing it. Executor already bounds its other remote calls, so this was the one that got missed.

Finally

Happy to split, rebase, or rework any of it. Thanks for building this — it's genuinely good software, which is why we're using it somewhere that matters.