You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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. Fifteen of the PRs are what I found. The sixteenth (#1564) 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.
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 fifteen 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:
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:
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.
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.
Why I opened sixteen 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. Fifteen of the PRs are what I found. The sixteenth (#1564) 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:
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 fifteen 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:
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 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:
A malformed
200from a token endpoint put an access token inside an error object, where anything logging that error would write the token out.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.
The 1Password backend left its service-account token on a module-level global after each call. One of the smallest of the set.
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.
The OAuth popup wrote its result to
localStorageas a fallback and never cleared it. Not a credential: the parked payload is the identity label (an email) and, on failure, an error preview.Credentials outliving what they were for:
Removing a connection left the credential it had minted in the store, unreferenced and invisible, including the long-lived refresh token.
Abandoned authorization sessions were never swept, so their PKCE verifiers stayed in the database indefinitely.
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.
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:
~/.executor/server-connections.jsonheld 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.The same problem in the desktop settings store.
Telling the truth about state:
An MCP connection with a missing credential reported healthy, which is the kind of wrong answer that hides the other kind.
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.
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.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.