cloudflare / cloudflare/cloudflare-os
Disconnecting an MCP account can leave its refresh token live at the authorization server
- Dominant language
- TypeScript
- Stars
- 9.9k
- Forks
- 1.2k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 107
Description
`McpAccountBase.revoke()` revokes the access token and the refresh token inside a single `try`, so
the refresh token is never presented once the first call fails — and `revokeToken()` throws on any
non-2xx response.
https://github.com/cloudflare/cloudflare-os/blob/aedcda8/packages/mcp-shared/src/account.ts#L829-L848
https://github.com/cloudflare/cloudflare-os/blob/aedcda8/packages/mcp-shared/src/oauth.ts#L63-L64
`revoke()` then calls `deleteAll()`, so the refresh token is gone locally too and can never be
presented for revocation again. `revoke()` is what `GatekeeperUser.revoke()` calls when a user
disconnects the account (`mcp-shared/src/user.ts:62`), so the account disappears from the user's
side while the longer-lived credential has never been revoked — and, having been deleted locally,
cannot be revoked afterwards either.
Nothing malicious is needed to reach it. RFC 7009 §2.2.1 defines `unsupported_token_type` for "the
authorization server does not support the revocation of the presented token type", so an AS that
revokes refresh tokens but not access tokens answers an error to the *first* call here — and the
token that matters more is the one that survives. A 503 does the same thing, and §2.2.1 is explicit
that "the client must assume the token still exists" in that case; so does a dropped connection,
where `fetch` itself throws.
### Reproduction
Driving the unmodified `revoke()` under vitest, with `fetch` stubbed and storage seeded the way a
connected OAuth account holds it. The only thing that varies between the two runs is what the
revocation endpoint answers.
```js
vi.stubGlobal("fetch", async (_url, init) => {
const body = String(init.body);
posted.push(body);
// RFC 7009 §2.2.1: an AS that will revoke refresh tokens but not access tokens.
if (body.includes("token_type_hint=access_token")) {
return new Response(JSON.stringify({ error: "unsupported_token_type" }), { status: 400 });
}
return new Response("", { status: 200 });
});
await account(values).revoke();
```
Server answers 200 to both — the intended behaviour:
```
token=ACCESS-TOKEN-VALUE&token_type_hint=access_token&client_id=client-abc
token=REFRESH-TOKEN-VALUE&token_type_hint=refresh_token&client_id=client-abc
```
Server refuses the access token with 400 `unsupported_token_type`:
```
token=ACCESS-TOKEN-VALUE&token_type_hint=access_token&client_id=client-abc
storage after revoke(): []
```
One request. The refresh token was never sent to the revocation endpoint, `revoke()` returned
normally after logging `failed to revoke MCP tokens`, and the token is no longer in storage to
retry with. A 503 on the first call produces the same single request.
### What this reproduction does and does not establish
Worth separating, since the two halves of the title are not evidenced equally.
**Directly observed.** With the access-token revocation refused, exactly one request reaches the
revocation endpoint, it carries `token_type_hint=access_token`, and storage is empty by the time
`revoke()` returns. The refresh token is never sent, and after `deleteAll()` there is nothing left
to send it with.
**Inferred, not measured.** That the refresh token is consequently still *usable* at the
authorization server. `fetch` is stubbed here, so nothing in this run reaches a real AS. The
inference is that a token which is never presented for revocation stays valid until it expires on
its own — an AS has no reason to invalidate it, and RFC 7009 §2.1's cascade runs the other way
(revoking the refresh token invalidates the access tokens, not the reverse). Reasonable, but it is
a property of the code path, not something this probe demonstrates. If you would rather see that
leg closed against a live authorization server before anyone acts on it, say so and I will run it
and post the result.
### Suggestion
The intent reads correctly — the comment already says "Best effort: a server that does not implement
RFC 7009 must not block the disconnect". It is the granularity: one failure ends the whole
best-effort block rather than that one token. Revoking each independently, refresh token first,
replaces the body of the existing `if`:
```ts
const fetchFn = sdkFetch(this.fetchOptions());
// Best effort per token rather than per disconnect: a server that refuses one token type --
// RFC 7009 §2.2.1's `unsupported_token_type` -- must not cost the revocation of the other.
const revokeOne = async (token: string, hint: "access_token" | "refresh_token") => {
try {
await revokeToken(discovery, client, token, hint, fetchFn);
} catch (err) {
this.log().warn("failed to revoke MCP token",
{ event: "oauth.token.revoke.failed", error: err });
}
};
// Refresh token first: where the server revokes access tokens at all, RFC 7009 §2.1 says
// revoking the refresh token SHOULD also invalidate the access tokens issued under the same
// grant, so this is the one revocation that must not be skipped.
if (tokens.refresh_token) await revokeOne(tokens.refresh_token, "refresh_token");
await revokeOne(tokens.access_token, "access_token");
```
(No `tokenTypeHint` log field: `McpLogFields` is a closed vocabulary and does not have one, so
adding it to the call would not type-check. It would be a reasonable field to add if you want the
two failures distinguishable in logs.)
To be straight about what that does and does not buy: it stops one token's failure from taking the
other with it, but it does not make a failed revocation recoverable. `deleteAll()` still runs, so a
refresh token the server refused to revoke is still unreachable afterwards. Making that recoverable
would mean keeping the credential and retrying, which is a much bigger change and probably not one
you want for a disconnect.
---
Not offering a PR for this one — per CONTRIBUTING.md and your note on #39, the sketch above is here
as material for your own agents to use or discard, not as a patch awaiting review.
AI tools assisted this investigation. I ran the reproduction myself, and the transcripts above are
its actual output rather than a description of what it should print.
Contributor guide
Research direction
Start in packages/mcp-shared/src/account.ts at McpAccountBase.revoke(), then read revokeToken() in packages/mcp-shared/src/oauth.ts and its caller in mcp-shared/src/user.ts. Run the described vitest reproduction with fetch stubbed; done means both token revocation requests are attempted when the first fails, while disconnect still completes and storage is cleared.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- authentication, security
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100