Why these thirteen PRs exist: one pass over credential handling

[GeiserX]

Why I opened thirteen pull requests

I've sent a run of PRs over the past weeks 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. Twelve of the
PRs are what I found. The thirteenth is a small capability change that came out of the same look.

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 twelve 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 twelve 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.

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. 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.

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 four 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.

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.

What I'm not asking for

I'm not asking for these to be taken as a batch. Each one is independent and mergeable on its own,
and if some are wrong or unwanted I'd rather they were closed than merged out of politeness. #1564 is
the only one that's really a discussion.

One practical note: none of these have had CI run on them, because workflow runs from a fork need a
maintainer's approval. I'd rather you saw green checks than took my word that the tests pass, so if
approving runs for the fork is easy on your side it would help — and if there's a reason not to,
that's fine, I'd just stop expecting checks to appear.

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.