fix(auth): refuse a grant whose authorization server cannot be established (#2144)
Copilot review round 15. The issuer check still failed open for the legacy unkeyed grant. "Unkeyed" means its authorization server is UNKNOWN, not that the cached endpoint is proven to own it: server metadata is a single slot a later discovery overwrites, while a pre-SEP-2352 token survives until the first issuer-stamped save. So a clear could post an old bearer credential to an authorization server that never minted it. Such a grant is now cleared without being revoked, and reported as such with a message saying re-authorizing makes future clears revocable. The cost is that a pre-issuer-binding entry gets no revocation — which is exactly the behavior it had before this feature existed, so nothing regresses — and it earns revocation back on the next authorization. Not sending is the only answer that cannot be wrong. Every seed in the suite is now issuer-bound, which is what a real flow writes; the two tests that were about the legacy grant now assert the refusal, since that is the behavior worth pinning. Also renamed the CLI test that still described the pre-round-13 ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
4429c0c0bf
commit
a4ee263365
8 changed files with 190 additions and 77 deletions
|
|
@ -8,6 +8,8 @@ import {
|
|||
} from "@inspector/core/auth/node/storage-node.js";
|
||||
import { clearStoredAuthForRelogin } from "../src/clear-stored-auth-for-relogin.js";
|
||||
|
||||
const AS_ISSUER = "https://as.example.com";
|
||||
|
||||
describe("clearStoredAuthForRelogin", () => {
|
||||
let dir: string | undefined;
|
||||
let prevPath: string | undefined;
|
||||
|
|
@ -77,17 +79,25 @@ describe("clearStoredAuthForRelogin", () => {
|
|||
file,
|
||||
JSON.stringify({
|
||||
servers: {
|
||||
// Issuer-bound (SEP-2352), matching the cached metadata below: an
|
||||
// unkeyed grant records no authorization server and is refused, so
|
||||
// seeding one would make these cases assert that instead.
|
||||
"https://example.com/mcp": {
|
||||
tokens: {
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "r",
|
||||
activeIssuer: AS_ISSUER,
|
||||
byIssuer: {
|
||||
[AS_ISSUER]: {
|
||||
tokens: {
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "r",
|
||||
},
|
||||
},
|
||||
},
|
||||
serverMetadata: {
|
||||
issuer: "https://as.example.com",
|
||||
authorization_endpoint: "https://as.example.com/authorize",
|
||||
token_endpoint: "https://as.example.com/token",
|
||||
revocation_endpoint: "https://as.example.com/revoke",
|
||||
issuer: AS_ISSUER,
|
||||
authorization_endpoint: `${AS_ISSUER}/authorize`,
|
||||
token_endpoint: `${AS_ISSUER}/token`,
|
||||
revocation_endpoint: `${AS_ISSUER}/revoke`,
|
||||
response_types_supported: ["code"],
|
||||
},
|
||||
...over,
|
||||
|
|
@ -111,17 +121,22 @@ describe("clearStoredAuthForRelogin", () => {
|
|||
dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-two-"));
|
||||
const file = path.join(dir, "oauth.json");
|
||||
const metadata = {
|
||||
issuer: "https://as.example.com",
|
||||
authorization_endpoint: "https://as.example.com/authorize",
|
||||
token_endpoint: "https://as.example.com/token",
|
||||
revocation_endpoint: "https://as.example.com/revoke",
|
||||
issuer: AS_ISSUER,
|
||||
authorization_endpoint: `${AS_ISSUER}/authorize`,
|
||||
token_endpoint: `${AS_ISSUER}/token`,
|
||||
revocation_endpoint: `${AS_ISSUER}/revoke`,
|
||||
response_types_supported: ["code"],
|
||||
};
|
||||
const entry = (refresh: string) => ({
|
||||
tokens: {
|
||||
access_token: `a-${refresh}`,
|
||||
token_type: "Bearer",
|
||||
refresh_token: refresh,
|
||||
activeIssuer: AS_ISSUER,
|
||||
byIssuer: {
|
||||
[AS_ISSUER]: {
|
||||
tokens: {
|
||||
access_token: `a-${refresh}`,
|
||||
token_type: "Bearer",
|
||||
refresh_token: refresh,
|
||||
},
|
||||
},
|
||||
},
|
||||
serverMetadata: metadata,
|
||||
});
|
||||
|
|
@ -141,7 +156,7 @@ describe("clearStoredAuthForRelogin", () => {
|
|||
resetNodeOAuthStorageCache();
|
||||
}
|
||||
|
||||
it("revokes the stored grant before deleting it", async () => {
|
||||
it("revokes the stored grant, having snapshotted it before the delete", async () => {
|
||||
const file = seed();
|
||||
const fetchSpy = vi
|
||||
.spyOn(globalThis, "fetch")
|
||||
|
|
@ -194,10 +209,10 @@ describe("clearStoredAuthForRelogin", () => {
|
|||
dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-both-"));
|
||||
const file = path.join(dir, "oauth.json");
|
||||
const metadata = {
|
||||
issuer: "https://as.example.com",
|
||||
authorization_endpoint: "https://as.example.com/authorize",
|
||||
token_endpoint: "https://as.example.com/token",
|
||||
revocation_endpoint: "https://as.example.com/revoke",
|
||||
issuer: AS_ISSUER,
|
||||
authorization_endpoint: `${AS_ISSUER}/authorize`,
|
||||
token_endpoint: `${AS_ISSUER}/token`,
|
||||
revocation_endpoint: `${AS_ISSUER}/revoke`,
|
||||
response_types_supported: ["code"],
|
||||
};
|
||||
fs.writeFileSync(
|
||||
|
|
@ -206,19 +221,29 @@ describe("clearStoredAuthForRelogin", () => {
|
|||
servers: {
|
||||
// The raw spelling the transport keyed by, holding a STALE grant.
|
||||
"https://example.com": {
|
||||
tokens: {
|
||||
access_token: "stale-a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "stale-r",
|
||||
activeIssuer: AS_ISSUER,
|
||||
byIssuer: {
|
||||
[AS_ISSUER]: {
|
||||
tokens: {
|
||||
access_token: "stale-a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "stale-r",
|
||||
},
|
||||
},
|
||||
},
|
||||
serverMetadata: metadata,
|
||||
},
|
||||
// The normalised spelling, holding the CURRENT grant.
|
||||
"https://example.com/": {
|
||||
tokens: {
|
||||
access_token: "live-a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "live-r",
|
||||
activeIssuer: AS_ISSUER,
|
||||
byIssuer: {
|
||||
[AS_ISSUER]: {
|
||||
tokens: {
|
||||
access_token: "live-a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "live-r",
|
||||
},
|
||||
},
|
||||
},
|
||||
serverMetadata: metadata,
|
||||
},
|
||||
|
|
@ -334,8 +359,11 @@ describe("clearStoredAuthForRelogin", () => {
|
|||
JSON.stringify({
|
||||
servers: {
|
||||
"https://example.com/mcp": {
|
||||
// No `token_type` — fails OAuthTokensSchema.
|
||||
tokens: { access_token: 42 },
|
||||
activeIssuer: AS_ISSUER,
|
||||
byIssuer: {
|
||||
// No `token_type` — fails OAuthTokensSchema.
|
||||
[AS_ISSUER]: { tokens: { access_token: 42 } },
|
||||
},
|
||||
},
|
||||
},
|
||||
idpSessions: {},
|
||||
|
|
|
|||
|
|
@ -21,7 +21,8 @@ import { resetNodeOAuthStorageCache } from "@inspector/core/auth/node/storage-no
|
|||
import { runCli } from "./helpers/cli-runner.js";
|
||||
|
||||
const SERVER_URL = "https://example.com/mcp";
|
||||
const REVOKE_URL = "https://as.example.com/revoke";
|
||||
const AS_ISSUER = "https://as.example.com";
|
||||
const REVOKE_URL = `${AS_ISSUER}/revoke`;
|
||||
|
||||
let dir: string | undefined;
|
||||
let prevPath: string | undefined;
|
||||
|
|
@ -46,16 +47,23 @@ function seedStore(): void {
|
|||
file,
|
||||
JSON.stringify({
|
||||
servers: {
|
||||
// Issuer-bound (SEP-2352) and matching the cached metadata: an unkeyed
|
||||
// grant records no authorization server and is deliberately refused.
|
||||
[SERVER_URL]: {
|
||||
tokens: {
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "r",
|
||||
activeIssuer: AS_ISSUER,
|
||||
byIssuer: {
|
||||
[AS_ISSUER]: {
|
||||
tokens: {
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "r",
|
||||
},
|
||||
},
|
||||
},
|
||||
serverMetadata: {
|
||||
issuer: "https://as.example.com",
|
||||
authorization_endpoint: "https://as.example.com/authorize",
|
||||
token_endpoint: "https://as.example.com/token",
|
||||
issuer: AS_ISSUER,
|
||||
authorization_endpoint: `${AS_ISSUER}/authorize`,
|
||||
token_endpoint: `${AS_ISSUER}/token`,
|
||||
revocation_endpoint: REVOKE_URL,
|
||||
response_types_supported: ["code"],
|
||||
},
|
||||
|
|
|
|||
|
|
@ -85,11 +85,17 @@ describe("clearServerOAuthState", () => {
|
|||
// metadata to build a request from), but the clear itself runs before the
|
||||
// network — so by the time the request goes out the store is already empty.
|
||||
it("clears before sending, using the snapshot it took first", async () => {
|
||||
await storage.saveTokens(SERVER_URL, {
|
||||
access_token: "tok",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "refresh-tok",
|
||||
});
|
||||
await storage.saveTokens(
|
||||
SERVER_URL,
|
||||
{
|
||||
access_token: "tok",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "refresh-tok",
|
||||
},
|
||||
// Issuer-bound and matching the metadata below: an unkeyed grant records
|
||||
// no authorization server and is deliberately refused.
|
||||
{ issuer: "https://as.example.com" },
|
||||
);
|
||||
await storage.saveServerMetadata(SERVER_URL, {
|
||||
issuer: "https://as.example.com",
|
||||
authorization_endpoint: "https://as.example.com/authorize",
|
||||
|
|
|
|||
|
|
@ -395,12 +395,20 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => {
|
|||
await storage.clear(SERVER_URL);
|
||||
});
|
||||
|
||||
const ISSUER = "https://as.example.com";
|
||||
|
||||
/**
|
||||
* A revocable grant: issuer-bound, and matching the cached metadata. Unkeyed
|
||||
* grants are deliberately NOT revocable (their authorization server is
|
||||
* unknown), so seeding one here would make every case below assert the
|
||||
* refusal instead of the behavior it is about.
|
||||
*/
|
||||
async function seed(over: Partial<OAuthMetadata> = {}): Promise<void> {
|
||||
await storage.saveTokens(SERVER_URL, {
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "r",
|
||||
});
|
||||
await storage.saveTokens(
|
||||
SERVER_URL,
|
||||
{ access_token: "a", token_type: "Bearer", refresh_token: "r" },
|
||||
{ issuer: ISSUER },
|
||||
);
|
||||
await storage.saveServerMetadata(SERVER_URL, metadata(over));
|
||||
}
|
||||
|
||||
|
|
@ -508,14 +516,21 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => {
|
|||
const fetchFn = vi.fn<typeof fetch>(
|
||||
async () => new Response(null, { status: 200 }),
|
||||
);
|
||||
await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn });
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn,
|
||||
});
|
||||
|
||||
// Exactly one request — the legacy grant, read ctx-lessly and unlabelled.
|
||||
// Never two, and never the legacy token presented as that issuer's.
|
||||
expect(fetchFn).toHaveBeenCalledTimes(1);
|
||||
expect(
|
||||
new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"),
|
||||
).toBe("legacy-r");
|
||||
// Nothing is sent: the only grant here is the legacy one, whose
|
||||
// authorization server is unknown. The point is that it is never presented
|
||||
// as the enumerated issuer's — one request with `legacy-r` would be exactly
|
||||
// that mistake.
|
||||
expect(fetchFn).not.toHaveBeenCalled();
|
||||
expect(outcome).toMatchObject({ status: "failed" });
|
||||
expect(outcome.status === "failed" ? outcome.detail : "").toContain(
|
||||
"predates issuer binding",
|
||||
);
|
||||
});
|
||||
|
||||
// The active issuer slot can hold client information without a token, in
|
||||
|
|
@ -551,10 +566,21 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => {
|
|||
async () => new Response(null, { status: 200 }),
|
||||
);
|
||||
|
||||
await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn });
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn,
|
||||
});
|
||||
|
||||
// Two grants, two requests — the issuer-bound one and the legacy one.
|
||||
expect(fetchFn).toHaveBeenCalledTimes(2);
|
||||
// Still two GRANTS — that is what this is about. The issuer-bound one is
|
||||
// revoked; the legacy one is refused (unknown authorization server) and
|
||||
// reported, rather than being silently collapsed into the other by a
|
||||
// token-value dedup.
|
||||
expect(fetchFn).toHaveBeenCalledTimes(1);
|
||||
expect(outcome).toMatchObject({ status: "failed" });
|
||||
expect(outcome.status === "failed" ? outcome.detail : "").toContain(
|
||||
"predates issuer binding",
|
||||
);
|
||||
});
|
||||
|
||||
// Absence establishes nothing: a metadata document with no `issuer` cannot
|
||||
|
|
@ -586,26 +612,35 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => {
|
|||
);
|
||||
});
|
||||
|
||||
// A legacy unkeyed grant is not bound to any authorization server, so there
|
||||
// is nothing to contradict — it proceeds without the comparison.
|
||||
it("still revokes a legacy grant when the metadata names no issuer", async () => {
|
||||
// "Unkeyed" means the authorization server is UNKNOWN, not that the cached
|
||||
// endpoint is proven to own the grant — server metadata is a single slot a
|
||||
// later discovery overwrites, while a legacy token survives until the first
|
||||
// issuer-stamped save. Refusing costs such an entry its revocation, which is
|
||||
// the behavior it had before this feature existed, and it earns it back on
|
||||
// the next authorization.
|
||||
it("refuses a legacy grant, whose authorization server is unknown", async () => {
|
||||
await storage.saveServerMetadata(
|
||||
SERVER_URL,
|
||||
metadata({ issuer: undefined }),
|
||||
metadata({ issuer: "https://as.example.com" }),
|
||||
);
|
||||
await storage.saveTokens(SERVER_URL, {
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "r",
|
||||
});
|
||||
const fetchFn = vi.fn<typeof fetch>(
|
||||
async () => new Response(null, { status: 200 }),
|
||||
);
|
||||
const fetchFn = vi.fn<typeof fetch>();
|
||||
|
||||
await expect(
|
||||
revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }),
|
||||
).resolves.toMatchObject({ status: "revoked" });
|
||||
expect(fetchFn).toHaveBeenCalledTimes(1);
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn,
|
||||
});
|
||||
|
||||
expect(fetchFn).not.toHaveBeenCalled();
|
||||
expect(outcome).toMatchObject({ status: "failed" });
|
||||
expect(outcome.status === "failed" ? outcome.detail : "").toContain(
|
||||
"re-authorize",
|
||||
);
|
||||
});
|
||||
|
||||
// If `listIssuers` fails there is no enumerated slot behind the stamp, so
|
||||
|
|
|
|||
|
|
@ -207,7 +207,12 @@ describe("OAuthManager", () => {
|
|||
it("clears local state before waiting on the revocation request", async () => {
|
||||
const params = createMockParams();
|
||||
const storage = params.initialConfig.storage!;
|
||||
vi.mocked(storage.getTokens).mockResolvedValue({
|
||||
// Issuer-bound and matching the metadata: an unkeyed grant records no
|
||||
// authorization server and is deliberately refused.
|
||||
vi.mocked(storage.listIssuers).mockResolvedValue([
|
||||
"https://as.example.com",
|
||||
]);
|
||||
vi.mocked(storage.getIssuerTokens).mockResolvedValue({
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "r",
|
||||
|
|
@ -242,7 +247,10 @@ describe("OAuthManager", () => {
|
|||
it("clears local state even when the revocation request fails", async () => {
|
||||
const params = createMockParams();
|
||||
const storage = params.initialConfig.storage!;
|
||||
vi.mocked(storage.getTokens).mockResolvedValue({
|
||||
vi.mocked(storage.listIssuers).mockResolvedValue([
|
||||
"https://as.example.com",
|
||||
]);
|
||||
vi.mocked(storage.getIssuerTokens).mockResolvedValue({
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
});
|
||||
|
|
|
|||
|
|
@ -138,7 +138,13 @@ describe("OAuth token revocation (RFC 7009)", () => {
|
|||
}): Promise<BrowserOAuthStorage> {
|
||||
const storage = new BrowserOAuthStorage();
|
||||
await storage.clear(serverUrl);
|
||||
await storage.saveTokens(serverUrl, { token_type: "Bearer", ...tokens });
|
||||
// Issuer-bound and matching the discovered metadata: an unkeyed grant
|
||||
// records no authorization server and is deliberately refused.
|
||||
await storage.saveTokens(
|
||||
serverUrl,
|
||||
{ token_type: "Bearer", ...tokens },
|
||||
{ issuer: metadata.issuer },
|
||||
);
|
||||
await storage.saveServerMetadata(serverUrl, metadata);
|
||||
// Preconfigured, so it goes in the preregistered slot — the same one a
|
||||
// server with `oauth.clientId` uses. That is the slot the revocation path
|
||||
|
|
|
|||
|
|
@ -631,10 +631,30 @@ async function runPlan(
|
|||
// server that never minted it — worse than not revoking. So an issuer-bound
|
||||
// grant must be able to PROVE the endpoint is its own, which means a
|
||||
// metadata document carrying no `issuer` is a mismatch rather than a free
|
||||
// pass: absence establishes nothing. Only a legacy unkeyed grant proceeds
|
||||
// without the comparison, since nothing binds it to a different
|
||||
// authorization server in the first place.
|
||||
if (grant.issuer !== undefined && plan.metadataIssuer !== grant.issuer) {
|
||||
// pass: absence establishes nothing. The same reasoning rules out the
|
||||
// unkeyed legacy grant just below — see there.
|
||||
if (grant.issuer === undefined) {
|
||||
// An unkeyed (pre-SEP-2352) grant records no authorization server at all,
|
||||
// and "unknown" is not "whatever the cache currently holds": server-level
|
||||
// metadata is a single slot that a later discovery overwrites, while the
|
||||
// legacy token survives until the first issuer-stamped save. So the
|
||||
// endpoint on hand may belong to an authorization server that never
|
||||
// minted this token, and sending it there would disclose a bearer
|
||||
// credential to a stranger.
|
||||
//
|
||||
// Refusing costs such an entry its revocation — which is exactly the
|
||||
// behavior it had before this feature existed, so nothing regresses — and
|
||||
// it earns revocation back the moment it is re-authorized, since that
|
||||
// save is issuer-stamped. Not sending is the only answer that cannot be
|
||||
// wrong.
|
||||
outcomes.push({
|
||||
status: "failed",
|
||||
detail:
|
||||
"the stored grant is not bound to an authorization server (it predates issuer binding), so it could not be matched to this revocation endpoint and was cleared without revocation — re-authorize to make future clears revocable",
|
||||
});
|
||||
continue;
|
||||
}
|
||||
if (plan.metadataIssuer !== grant.issuer) {
|
||||
outcomes.push({
|
||||
status: "failed",
|
||||
detail:
|
||||
|
|
|
|||
|
|
@ -219,6 +219,8 @@ The request names the **refresh token** when there is one. RFC 7009 §2.1 asks a
|
|||
|
||||
The order is **snapshot → clear → revoke**. Everything the request needs — the token, the client credentials, the discovered `revocation_endpoint` — is read out of the store first, because the clear empties it; but the clear then runs immediately rather than behind the network. Waiting would leave a window in which a *fresh* authorization could complete and then be deleted by a clear still reasoning about the grant it replaced.
|
||||
|
||||
> **A grant is only revoked where the endpoint can be shown to belong to it.** The Inspector caches authorization-server metadata once per server, not per issuer, so a token bound to a *different* issuer than the cached document names is cleared without being revoked, and reported as such — sending it would disclose a bearer credential to a server that never minted it. The same applies to a grant stored before the Inspector began binding credentials to an issuer: "unbound" means its authorization server is unknown, not that the current endpoint owns it. Such an entry is left unrevoked (exactly what it did before this feature existed) and becomes revocable again the first time it is re-authorized.
|
||||
|
||||
> **It is best-effort, and the local clear always finishes.** An authorization server that advertises no `revocation_endpoint` is left behaving exactly as it did before this existed — nothing is sent. A network error, a non-2xx, or a slow server that trips the short timeout is reported (a toast in the web client, a status line in the TUI, a stderr warning from the CLI) and nothing more. Forgetting the tokens is what you asked for, so no failure on this leg stops it.
|
||||
|
||||
> **Turning it off is a testing affordance, not only an escape hatch.** A client that walks away still holding live tokens is a case a server author may want to reproduce deliberately, to watch how the server under test copes with it.
|
||||
|
|
|
|||
Loading…
Reference in a new issue