cloudflare / cloudflare/cloudflare-os

Disconnecting an MCP account can leave its refresh token live at the authorization server

Open Beginner friendly
#41 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.