Let a credential provider own the OAuth refresh grant - #1564
Conversation
A provider that serves an indirection instead of a raw value can protect an
access token: that token's only use is to be sent to a bound host, and the
reply is not itself a credential. The refresh grant breaks that. The exchange
needs the real refresh token, and the authorization server's reply carries a
brand-new real access token, so serving an indirection here moves the exposure
one step later while appearing to remove it. Providers backed by a sealed store
have to refuse the refresh item outright today, which costs them refresh
entirely.
Add an optional `refreshGrant` to CredentialProvider so such a provider can own
the exchange instead: it spends the refresh token, seals the new access token
(and a rotated refresh token) under the same item ids, and returns only
`{ expiresAt, scope }`. The caller then reads the access token back through
`get`, the same hop every other credential already takes.
Absence is not a downgrade: when the method is missing the existing host-side
exchange runs unchanged. client_credentials is excluded deliberately - it has
no refresh token to spend. Secrets are named by item id, never passed as
values, since passing them would reintroduce the exposure this removes.
The test pins the custody property directly - that the host never resolves the
refresh token through the provider - rather than asserting the refresh
succeeded, because a provider that quietly served the token would also go
green.
Address review of the delegated refresh path: the fast path returned before the machinery that classifies an authorization-server refusal, so a provider that owned the grant lost re-auth entirely. - Report a refusal with a typed `RefreshGrantRejected` carrying the RFC 6749 §5.2 code. Both grant paths now share one classifier and one known-dead gate, so a delegated refresh surfaces `invalid_grant` to the caller and stops re-sending a doomed grant on every proactive cycle. - Resolve the client secret BELOW the delegated branch. It was read in plaintext and then never used, which both defeated the point of passing `clientSecretItemId` and meant a store that seals that item failed the refresh before `refreshGrant` was ever reached. - Read the new access token back before recording success, and fail when it cannot be resolved, instead of stamping a fresh expiry over a token nobody can read. - Enforce the host's OAuth endpoint URL policy on the delegated path too. - Report `expiresInSeconds` rather than an absolute instant, so the caller converts on the clock that later decides whether the token is due. - Carry `clientAuth` so a provider never has to guess the client authentication method. - Export `RefreshGrantInput`, `RefreshGrantResult` and `RefreshGrantRejected` from the package entry point; an external provider could not name them. - Add a changeset, and cover the refusal, read-back, scope, expiry and client_credentials paths.
A credential provider is an external plugin boundary, so nothing it authors may reach host error channels, telemetry, or persisted connection state — those values can carry token responses and other secret material. - Close the rejection classification to the standards-defined set (RFC 6749 §5.2 plus RFC 8707 `invalid_target`) and validate it at runtime, so an unrecognised value cannot reach `oauthErrorCode`, span attributes or persisted health. Drop `message`/`cause` from `RefreshGrantRejected` entirely; Executor now emits fixed host-facing text carrying only the validated code. - Contain provider storage failures, synchronous throws, Effect defects, and throwing or stateful property getters — including on the capability itself, on the success object's fields, and on the post-grant read-back. Cancellation is still propagated as cancellation; only the provider-authored reasons are dropped. - Rebuild the persisted scope from the host's own recorded grant set rather than the provider's string. `oauth_scope` is replayed to the authorization server on the next refresh, so accepting it verbatim was a persisted provider-controlled channel. A scope outside the granted set fails the refresh (RFC 6749 §6: a refresh may narrow scope, never widen it). - Bound the reported lifetime to finite, non-negative and at most ten years, instead of stamping NaN/Infinity/negative straight into `expires_at`. - Treat an unresolvable read-back as a retryable provider-invariant failure rather than demanding re-authentication: the authorization server ACCEPTED the grant, so re-auth is the one remedy that cannot be required, and a rotated refresh token may already be sealed. - Re-export the contract from the promise and shared surfaces too, and generalise the security note from `tokenUrl` to the whole caller-authored input tuple.
The scope validation compared the provider's reported scope against the connection's recorded grant set. When that set is empty the comparison had no subset to accept, so every reported scope failed the whole refresh — and the failure is a retryable StorageError, so the connection would retry a grant that could never succeed, indefinitely. An empty grant set is a legitimate state, not a corrupt one: RFC 6749 §5.1 lets an authorization server omit the scope it granted, and the refresh request then omits the scope parameter entirely. With nothing recorded there is also nothing to widen away from, so the safe action is to keep the refresh and record no scope. The provider's string is still never persisted, which is the property the validation exists to hold.
|
Self-review while building against this: the host resolves the client secret even when it is about to delegate, which contradicts the contract this interface states for itself.
But in // The secret is stored in the provider (a vault item id), not inline.
const clientSecret = clientRow.client_secret_item_id
? ((yield* provider.get(ProviderItemId.make(String(clientRow.client_secret_item_id)))) ?? "")
: "";The delegation branch is ~50 lines later and passes the id: if (provider.refreshGrant && String(clientRow.grant) !== "client_credentials") {
const granted = yield* provider.refreshGrant({
clientSecretItemId: clientRow.client_secret_item_id ? ... : undefined,So on the delegated path the host fetches the plaintext secret, holds it, then names the same secret by id and throws the value away. The exposure the interface removes for the refresh token stays open for the client secret, one line before it is closed. Two consequences worth separating:
Fix is a move rather than a change: hoist the delegation branch above the secret resolution, or make the resolution lazy. Happy to push it here if you'd like it in this PR, or leave it for whatever shape the seam ends up taking — it is a small change either way, and it does not affect providers without |
TL;DR
Problem. To refresh an OAuth token today, Executor asks the credential provider to hand over the refresh token, then performs the exchange itself. A provider that keeps secrets sealed — in a TEE, an HSM, a hardware-backed store — cannot do that without unsealing. Its only options are to unseal (defeating the point) or to refuse refresh entirely (breaking every long-lived connection).
Change. This adds an optional
refreshGranttoCredentialProvider. When a provider implements it, Executor asks it to perform the exchange instead of to reveal the token. The provider spends the refresh token, seals the new tokens under the same item ids, and returns only two pieces of metadata: how long the new token lasts, and what scope it was granted.Blast radius. None for anyone who doesn't implement it. Providers without
refreshGranttake the existing host-side path, unchanged and untouched. All five in-tree providers are in that group.Why the seam belongs here and not in a wrapper. A deployer can reach the refresh exchange today via
config.fetch. That is not equivalent, for three reasons, spelled out under Why notconfig.fetch? below — briefly: it puts the decision with whoever deploys rather than with whoever owns the secret, it sees a URL and a request body instead of item ids, and it fails open — one shim intercepts every outbound call in the process, so a bug in it degrades unrelated traffic rather than just this exchange.Where this goes, and where it stops
This is the narrow half of a larger idea, and deliberately the half that stands alone. The same seam extends to two more exchanges:
client_credentials, where the long-lived secret is the client secret rather than a refresh token, and login, where the provider mints and keeps the PKCE verifier so the authorization code Executor receives is one it cannot spend. Both are written and tested. Neither is opened, because both build on the interface introduced here and it seems wrong to argue three versions of one seam at once — if the direction here is wrong, they should change with it rather than lobby for it.Worth being precise about the limit, because an interface like this invites overclaiming. What these remove from Executor's memory are the secrets that sit at rest: the refresh token, the client secret, the PKCE verifier. What they do not remove is the access token at the moment it is spent — something has to hold a token to put it on the wire, and no provider interface changes that. So the accurate claim is "the credentials that live for months never materialise in this process", not "Executor never sees a secret". Closing the second one is a different mechanism at the network layer, and not something this repo can do alone.
The PRs listed at the bottom are independent of all of this — plain defects I hit while working here, each mergeable on its own.
Read this in one minute
What this adds
An optional
refreshGrantonCredentialProvider, so a provider can perform the OAuth refreshexchange itself and return only
{ expiresInSeconds, scope }. If a provider doesn't implement it,nothing changes — the existing host-side exchange runs unmodified.
Why
CredentialProviderlets a backend serve an indirection instead of a raw value. That works for anaccess token: it is spent against a bound host, and the reply is not itself a credential.
The refresh grant is the one exchange where a sealed, long-lived secret must be spent and the reply
is itself a fresh credential. Today a provider whose store the host cannot read has two options,
and neither is good: refuse the refresh item and lose refresh entirely, or have the deployer install
a
config.fetchshim (see below).I know
vision.md:42and:137-138put the boundary at the agent/sandbox and treat the host astrusted space — for the backends it names (1Password, keychain, env) that is exactly right, and all
five in-tree providers return raw values from
getaccordingly. The case here is narrower: when thecustody boundary has to sit below executor rather than around it.
The reason it can't sit around executor: a hardware-isolated boundary protects the VM, not the
process. Two of executor's four sandbox runtimes (
runtime-deno-subprocess,runtime-workerd-subprocess) run model-authored code as co-resident subprocesses inside that sameboundary, so enclosing executor reduces the secrets' safety to the soundness of executor's own JS
sandboxes — a hardware guarantee traded for a software one. That leaves the provider as the only
place the secret stays sealed, and the refresh grant as the only operation that seam can't express.
performTokenRefreshis where it bites:Shape
The provider spends the refresh token, seals the new access token (and a rotated refresh token —
RFC 9700 §2.2.2 makes rotation the normal case) under the same item ids, and reports only what the
connectionrow needs. Secrets are named by item id, never passed as values.The shape is well-worn: MS-OAPXBC standardizes "a broker client obtains access tokens on behalf of
calling clients" against a TPM-bound refresh token; Azure Key Vault splits the same way, where keys
are used via operations and never leave while secrets are returned.
getis the secret,refreshGrantis the key operation.The provider is treated as an untrusted boundary
A provider is a plugin, and on the delegated path it is also the thing performing a network exchange
against a third party. So nothing it authors reaches host error channels, telemetry, or persisted
connection state — those values can carry token responses and other secret material:
RefreshGrantRejectedcarries only astandards-defined code (RFC 6749 §5.2 plus RFC 8707
invalid_target); it deliberately has nomessageorcause. Executor generates fixed host-facing text and interpolates only thevalidated code, so nothing provider-authored reaches
oauthErrorCode, span attributes, orpersisted health.
invalid_grantstill means re-auth and still arms the known-dead gate, exactlyas the host-side path does.
and throwing or stateful property getters — on the capability itself, on the success object's
fields, and on the post-grant read-back. Cancellation is still propagated as cancellation; only
the provider-authored reasons are dropped.
oauth_scopeisreplayed to the authorization server on the next refresh, so accepting the provider's string
would have been a persisted provider-controlled channel. A scope outside the granted set fails
the refresh (RFC 6749 §6: a refresh may narrow scope, never widen it).
NaN/Infinity/negative into
expires_at. It is also relative (expiresInSeconds), because theprovider may run on another machine; the host converts on the clock
shouldRefreshTokenreads.oauthEndpointUrlPolicyis enforced before delegating —delegating the exchange must not delegate the guard.
grant, so re-auth is the one remedy that cannot be required, and a rotated refresh token may
already be sealed.
One open question for you
RefreshGrantInputis entirely the caller's view: a compromised host can rewrite not justtokenUrlbut every item id, the client id, scopes, and resource. The doc comment now says aprovider MUST authenticate the whole tuple against its own enrollment metadata — but a comment
cannot make caller-authored data trustworthy.
The structurally correct fix is a provider-owned opaque descriptor minted at enrollment, with
later grants carrying only that handle. I did not do that here because it is a larger contract
change and I would rather you choose the shape. Happy to follow up with it, or to keep this as a
deliberately narrow seam where the provider does its own binding out of band. Your call.
Why not
config.fetch?Worth asking first, and the honest answer is that it does work:
config.fetchreaches the refreshgrant (
executor.ts→ oauth4webapi'scustomFetch), so a deployer-installed shim can swapsentinels on the wire and close the same custody gap with no upstream change. So this PR isn't the
only route. I think it's the right one for three reasons:
config.fetchis a deployer seam sitting eleven lines fromproviders?:,which is the provider seam. Sealed custody stops being a self-contained provider capability and
becomes a two-part assembly the deployer must keep in sync — and the drift failure is silent,
because the provider still looks sealed while the host quietly holds real tokens.
reverse-map
(tokenUrl, client_id)back to the right item id — exactly the coupling thisinterface removes by passing ids by name.
custody shim there intercepts everything and must discriminate the token POST by URL matching. A
matching bug fails open.
If you'd rather keep all RFC 6749 logic in
oauth-helpers.ts, I'm happy to reshape this as "hostbuilds the token request, provider substitutes-and-sends and returns the
Response". That keeps URLpolicy, refusal classification and telemetry in one place and still closes custody.
Notes
is a seam for out-of-tree sealed backends. If you'd rather not carry an interface member with no
in-tree consumer, the
Response-substitution shape above is easier to justify.client_credentialsis excluded — no refresh token to spend, so it stays on the existing path.refreshAccessToken(oauth-helpers.ts) is untouched; the change isprovider.ts, one callsite in
executor.ts, and the package entry points.token_endpoint_auth_methodandparseClientAuthMethod()at the same anchor. If it lands first I'll rebase and pass the parsedmethod instead of the current hardcoded
clientAuth: "body"(the types already line up — thefield is
"body" | "basic")."body"is the existing default, so today's behaviour is unchangedeither way.
index,promiseandshared. Please confirm that's thepublic surface you want provider authors to use.
Redacted).refreshGrantnever carries token material ineither direction, so it needs no change if
getis retyped. The only touch point is theread-back's
typeof access === "string"check, which becomes aRedactedunwrap at the sameboundary. Happy to land it either order — use effect redact #1492 is still a draft, so this targets
mainas it is.Testing
packages/core/sdk: 620 tests / 45 files passing; typecheck, lint and format clean.26 tests cover the delegated path. Beyond the happy path and the fallback, they pin the custody
claims directly: a token canary is asserted absent from the returned failure, from
Cause.pretty,and from the persisted row, across coded / code-less / storage-failure / synchronous-throw /
defect / malformed-getter / stateful-getter paths; scope outside the granted set is refused and
never persisted; lifetime bounds are pinned at and above the maximum; cancellation survives while a
concurrent secret-bearing defect is dropped. Each guard was mutation-checked — reverting any one of
them fails a test.
One of those tests exists because the scope validation first got this wrong: a connection whose
recorded grant set is empty had no subset to accept, so every reported scope failed the refresh as a
retryable error — an unbreakable retry loop on a perfectly live connection. An empty grant set is
legitimate (RFC 6749 §5.1 lets the server omit the granted scope), so that case now keeps the
refresh and records no scope.
Related: the rest of the series
Index and reasoning: #1585 — why these exist, what I was actually looking for, and how the
pieces fit. Worth reading first if this run of PRs looked disconnected.
While building this I found twelve defects in the surrounding credential code. Each is independent of this PR and opened separately, so none of them has to wait on this design discussion:
200from a token endpoint put an access token inside an error object'scause, where anything that logged the error would write it out.localStorageas a fallback channel and never cleared it, parking the user's email in the browser profile.op-js's module-level global after each call. Smallest of the set, and its description says so.HttpClientErrormessage and went straight to the log on any transport failure.~/.executor/server-connections.jsonheld 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 running.Two further changes extend the same seam introduced here — a provider-owned
client_credentialsgrant, and a provider-owned login where the provider holds the PKCE verifier. Both build directly on this PR's interface, so they are held until this one has a direction. GitHub cannot base a pull request on a fork branch, so they cannot be stacked behind it; happy to open them against whatever base you prefer.