fix(auth): key grant dedup by issuer, and isolate per-slot read failures (#2144)
Copilot review round 5. - Deduplicating grants by token string alone could conflate two grants: a token is only meaningful to the authorization server that minted it, so two issuers minting the same opaque value are two grants. The second was dropped before the issuer-mismatch check could report it, and `clear` then deleted it. The key is now issuer+token; the ctx-less read stays suppressed only when its token already came from a slot, which is exactly the active-issuer duplicate and never the legacy one. - A read failure in one issuer slot aborted `collectGrants`, so one corrupt slot left every other revocable grant untouched while the clear deleted them all. Each slot is now read independently and its failure carried as an outcome alongside the grants that could still be revoked; the no-metadata and no-endpoint returns carry them too, so a failure is never swallowed by a skip. - The CLI no longer deduplicates its two key spellings. That could only be done by pre-reading a single token, and `revokeStoredOAuthTokens` now enumerates every issuer slot under a key — so a shared active token skipped the second key entirely, taking any other issuer-bound grant under it with the local delete. A duplicate RFC 7009 request is harmless (§2.2 makes an unknown token a success); a missed one is the leak. 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
6c499f3ea1
commit
e9cd678cd0
4 changed files with 230 additions and 84 deletions
|
|
@ -287,10 +287,11 @@ describe("clearStoredAuthForRelogin", () => {
|
|||
}
|
||||
});
|
||||
|
||||
// A token the AS never accepted is not spent, so the other key — which may
|
||||
// carry the credentials or metadata that would have worked — must still be
|
||||
// tried rather than skipped as a duplicate.
|
||||
it("does not treat a failed attempt as having ended the grant", async () => {
|
||||
// Keys are revoked from independently. Deduplicating them could only be
|
||||
// done by pre-reading one token, which would skip the second key's OTHER
|
||||
// issuer-bound grants along with it; a duplicate RFC 7009 request is
|
||||
// harmless (§2.2 makes an unknown token a success), a missed one is not.
|
||||
it("revokes from both keys even when they hold the same token", async () => {
|
||||
seedBothSpellings("same-r", "same-r");
|
||||
const fetchSpy = vi
|
||||
.spyOn(globalThis, "fetch")
|
||||
|
|
|
|||
|
|
@ -4,7 +4,6 @@ import {
|
|||
} from "@inspector/core/auth/node/storage-node.js";
|
||||
import {
|
||||
revokeStoredOAuthTokens,
|
||||
selectRevocableToken,
|
||||
type TokenRevocationOutcome,
|
||||
} from "@inspector/core/auth/revocation.js";
|
||||
import { createProxyFetch } from "@inspector/core/mcp/node/proxyFetch.js";
|
||||
|
|
@ -44,10 +43,10 @@ export async function clearStoredAuthForRelogin(
|
|||
// Both spellings are cleared below, so both are revoked from — a stale entry
|
||||
// under the other key is a live grant at the authorization server, and
|
||||
// deleting it locally without revoking is exactly the leak this closes. The
|
||||
// normalised key goes first because that is the precedence `findStoredServerState`
|
||||
// reads with, so the grant actually in use is the one whose outcome is
|
||||
// reported; a second key holding the *same* token is skipped rather than
|
||||
// revoked twice.
|
||||
// normalised key goes first because that is the precedence
|
||||
// `findStoredServerState` reads with, so the grant actually in use is the one
|
||||
// whose outcome is reported. They are deliberately not deduplicated; see
|
||||
// `revokeStoredKeys`.
|
||||
const revocation =
|
||||
options?.revoke === false
|
||||
? undefined
|
||||
|
|
@ -63,22 +62,26 @@ export async function clearStoredAuthForRelogin(
|
|||
}
|
||||
|
||||
/**
|
||||
* Revoke every distinct grant held under `keys`, in order, and report the first
|
||||
* key that had something to revoke.
|
||||
* Revoke every grant held under `keys`, in order, and report the outcome that
|
||||
* matters most.
|
||||
*
|
||||
* Both keys are about to be deleted, so both are revoked from: a stale entry
|
||||
* under the other spelling is still a live grant at the authorization server,
|
||||
* and deleting it locally without revoking is the leak this whole change
|
||||
* closes. Two keys holding the *same* token are one grant, so the duplicate is
|
||||
* skipped rather than producing a second request for something already ended —
|
||||
* but only once one has actually been *revoked*, since a failed or unsupported
|
||||
* attempt has ended nothing and the other key may hold the credentials that
|
||||
* would have worked.
|
||||
* closes.
|
||||
*
|
||||
* Reporting prefers a **failure** over any earlier success — a grant still live
|
||||
* at the authorization server is what the user needs to hear about, and an
|
||||
* earlier success would otherwise silence the warning. Failing that, it is the
|
||||
* first key's outcome that was not "nothing to do", which with `keys` in
|
||||
* There is deliberately **no** cross-key deduplication. It looked cheap — the
|
||||
* two keys are usually two spellings of one server — but it can only be done
|
||||
* by pre-reading a single token, and `revokeStoredOAuthTokens` enumerates every
|
||||
* issuer slot under a key. So a shared *active* token would have skipped the
|
||||
* second key entirely, taking any additional issuer-bound grant under it with
|
||||
* the local delete. A duplicate RFC 7009 request is harmless (§2.2 makes an
|
||||
* unknown token a success), which is a much better trade than a missed one.
|
||||
*
|
||||
* Reporting prefers a **failure** over any success — a grant still live at the
|
||||
* authorization server is what the user needs to hear about, and a success
|
||||
* would otherwise silence the warning. Failing that it is the first key's
|
||||
* outcome that was not "nothing to do", which with `keys` in
|
||||
* `findStoredServerState` precedence means the grant the CLI would actually
|
||||
* have connected with. When no key holds a token, the last "nothing to do"
|
||||
* answer is returned so the caller can still tell "no tokens" from "this
|
||||
|
|
@ -89,26 +92,9 @@ async function revokeStoredKeys(
|
|||
keys: string[],
|
||||
): Promise<TokenRevocationOutcome | undefined> {
|
||||
const fetchFn = createProxyFetch() ?? fetch;
|
||||
const revokedTokens = new Set<string>();
|
||||
let reported: TokenRevocationOutcome | undefined;
|
||||
let lastSkip: TokenRevocationOutcome | undefined;
|
||||
for (const key of new Set(keys)) {
|
||||
// This read is OUTSIDE `revokeStoredOAuthTokens`'s own best-effort catch,
|
||||
// so it needs its own: `getTokens` parses through `OAuthTokensSchema` and
|
||||
// rejects on a persisted token that no longer validates. Letting that
|
||||
// escape would abandon the *local* delete `--relogin` promises, over a
|
||||
// grant we could not have revoked anyway.
|
||||
let token: string | undefined;
|
||||
try {
|
||||
token = selectRevocableToken(await storage.getTokens(key))?.token;
|
||||
} catch (err) {
|
||||
reported ??= {
|
||||
status: "failed",
|
||||
detail: err instanceof Error ? err.message : String(err),
|
||||
};
|
||||
continue;
|
||||
}
|
||||
if (token !== undefined && revokedTokens.has(token)) continue;
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl: key,
|
||||
storage,
|
||||
|
|
@ -118,16 +104,6 @@ async function revokeStoredKeys(
|
|||
lastSkip = outcome;
|
||||
continue;
|
||||
}
|
||||
// Only a token the authorization server actually accepted counts as spent.
|
||||
// Marking one revoked on a `failed` or `no_endpoint` outcome would skip a
|
||||
// duplicate entry under the other key that may carry usable metadata or
|
||||
// credentials — the one chance left to end that grant.
|
||||
if (token !== undefined && outcome.status === "revoked") {
|
||||
revokedTokens.add(token);
|
||||
}
|
||||
// A failure outranks an earlier success for reporting: a grant that is
|
||||
// still live is what the user needs to hear about, and reporting the
|
||||
// success would print no warning while a stale grant survives.
|
||||
if (outcome.status === "failed") {
|
||||
if (reported?.status !== "failed") reported = outcome;
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -617,13 +617,15 @@ describe("revokeStoredOAuthTokens", () => {
|
|||
it("reports a non-Error store failure as failed", async () => {
|
||||
vi.spyOn(storage, "listIssuers").mockRejectedValue("store exploded");
|
||||
|
||||
await expect(
|
||||
revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn: vi.fn<typeof fetch>(),
|
||||
}),
|
||||
).resolves.toEqual({ status: "failed", detail: "store exploded" });
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn: vi.fn<typeof fetch>(),
|
||||
});
|
||||
expect(outcome).toMatchObject({ status: "failed" });
|
||||
expect(outcome.status === "failed" ? outcome.detail : "").toContain(
|
||||
"store exploded",
|
||||
);
|
||||
});
|
||||
|
||||
it("reports a store read failure as failed", async () => {
|
||||
|
|
@ -631,12 +633,104 @@ describe("revokeStoredOAuthTokens", () => {
|
|||
new Error("store unreadable"),
|
||||
);
|
||||
|
||||
await expect(
|
||||
revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn: vi.fn<typeof fetch>(),
|
||||
}),
|
||||
).resolves.toEqual({ status: "failed", detail: "store unreadable" });
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn: vi.fn<typeof fetch>(),
|
||||
});
|
||||
expect(outcome).toMatchObject({ status: "failed" });
|
||||
expect(outcome.status === "failed" ? outcome.detail : "").toContain(
|
||||
"store unreadable",
|
||||
);
|
||||
});
|
||||
|
||||
// A corrupt slot must not abandon the grants that are still revocable — the
|
||||
// clear deletes them all either way, so the failure has to be reported
|
||||
// BESIDE the successes rather than instead of them.
|
||||
it("still revokes the readable grants when one issuer slot cannot be read", async () => {
|
||||
await storage.saveServerMetadata(
|
||||
SERVER_URL,
|
||||
metadata({ issuer: "https://as.example.com" }),
|
||||
);
|
||||
await storage.saveTokens(
|
||||
SERVER_URL,
|
||||
{ access_token: "a", token_type: "Bearer", refresh_token: "r-good" },
|
||||
{ issuer: "https://as.example.com" },
|
||||
);
|
||||
// A second issuer whose exact read throws.
|
||||
vi.spyOn(storage, "listIssuers").mockResolvedValue([
|
||||
"https://as.example.com",
|
||||
"https://broken.example.com",
|
||||
]);
|
||||
vi.spyOn(storage, "getIssuerTokens").mockImplementation(
|
||||
async (_url: string, issuer: string) => {
|
||||
if (issuer === "https://broken.example.com") {
|
||||
throw new Error("corrupt slot");
|
||||
}
|
||||
return {
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "r-good",
|
||||
};
|
||||
},
|
||||
);
|
||||
const fetchFn = vi.fn<typeof fetch>(
|
||||
async () => new Response(null, { status: 200 }),
|
||||
);
|
||||
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn,
|
||||
});
|
||||
|
||||
// The good grant was still revoked...
|
||||
expect(fetchFn).toHaveBeenCalledTimes(1);
|
||||
expect(
|
||||
new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"),
|
||||
).toBe("r-good");
|
||||
// ...and the unreadable slot is reported rather than swallowed.
|
||||
expect(outcome).toMatchObject({ status: "failed" });
|
||||
expect(outcome.status === "failed" ? outcome.detail : "").toContain(
|
||||
"corrupt slot",
|
||||
);
|
||||
});
|
||||
|
||||
// A token is only meaningful to the AS that minted it, so two issuers minting
|
||||
// the same opaque string are two grants. Collapsing them would drop the
|
||||
// second before the issuer-mismatch check could even report it.
|
||||
it("does not collapse two issuers that minted the same token value", async () => {
|
||||
await storage.saveServerMetadata(
|
||||
SERVER_URL,
|
||||
metadata({ issuer: "https://as-a.example.com" }),
|
||||
);
|
||||
for (const issuer of [
|
||||
"https://as-a.example.com",
|
||||
"https://as-b.example.com",
|
||||
]) {
|
||||
await storage.saveTokens(
|
||||
SERVER_URL,
|
||||
{ access_token: "same", token_type: "Bearer", refresh_token: "same-r" },
|
||||
{ issuer },
|
||||
);
|
||||
}
|
||||
const fetchFn = vi.fn<typeof fetch>(
|
||||
async () => new Response(null, { status: 200 }),
|
||||
);
|
||||
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn,
|
||||
});
|
||||
|
||||
// Only as-a matches the cached metadata, so only it is revocable — but
|
||||
// as-b is REPORTED rather than silently dropped, which is what collapsing
|
||||
// by token alone would have done.
|
||||
expect(fetchFn).toHaveBeenCalledTimes(1);
|
||||
expect(outcome).toMatchObject({ status: "failed" });
|
||||
expect(outcome.status === "failed" ? outcome.detail : "").toContain(
|
||||
"as-b.example.com",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -346,8 +346,20 @@ interface StoredGrant {
|
|||
clientInformation?: OAuthClientInformation;
|
||||
}
|
||||
|
||||
/** What {@link collectGrants} found, including the slots it could not read. */
|
||||
interface CollectedGrants {
|
||||
grants: StoredGrant[];
|
||||
/**
|
||||
* One `failed` outcome per slot whose read threw. Kept apart from the grants
|
||||
* so a single corrupt slot cannot abandon the ones that are still revocable
|
||||
* — the clear deletes them all either way, so the failure has to be reported
|
||||
* *beside* the successes rather than instead of them.
|
||||
*/
|
||||
failures: TokenRevocationOutcome[];
|
||||
}
|
||||
|
||||
/**
|
||||
* Every grant `clear(serverUrl)` is about to delete, deduplicated by token.
|
||||
* Every grant `clear(serverUrl)` is about to delete.
|
||||
*
|
||||
* `clear` drops **every** `byIssuer` slot, so reading only the context-free
|
||||
* (active-issuer) token would leave an earlier authorization server's grant
|
||||
|
|
@ -355,31 +367,46 @@ interface StoredGrant {
|
|||
* exists to close, just moved one level down. A server that authorized against
|
||||
* issuers A and B has two grants here, not one.
|
||||
*
|
||||
* The ctx-less read is included last and deduped: on an issuer-bound entry it
|
||||
* returns the active issuer's token, which the loop above has already seen; on
|
||||
* a legacy entry it is the only thing that returns anything at all. It is also
|
||||
* the only read here allowed to fall back — an *enumerated* issuer is read
|
||||
* exactly, so a legacy token is never mislabelled as belonging to it.
|
||||
* Deduplication is by **issuer *and* token**, not by token alone. A token is
|
||||
* only meaningful to the authorization server that minted it, so two issuers
|
||||
* that happen to mint the same opaque string are two grants; collapsing them
|
||||
* would drop the second before the issuer-mismatch check could even report it.
|
||||
*
|
||||
* The ctx-less read is included last and is the one exception: it resolves to
|
||||
* the *active* issuer's slot (already collected above) or, on a pre-SEP-2352
|
||||
* entry, to the legacy unkeyed token — which nothing else returns. So it is
|
||||
* suppressed when its token was already taken from any slot, which is exactly
|
||||
* the first case and never the second. It is also the only read here allowed
|
||||
* to fall back; an enumerated issuer is read exactly, so a legacy token is
|
||||
* never mislabelled as belonging to one.
|
||||
*/
|
||||
async function collectGrants(
|
||||
storage: OAuthStorage,
|
||||
serverUrl: string,
|
||||
): Promise<StoredGrant[]> {
|
||||
): Promise<CollectedGrants> {
|
||||
const grants: StoredGrant[] = [];
|
||||
const seen = new Set<string>();
|
||||
const failures: TokenRevocationOutcome[] = [];
|
||||
const seenKeys = new Set<string>();
|
||||
const slotTokens = new Set<string>();
|
||||
|
||||
const add = async (issuer?: string): Promise<void> => {
|
||||
// Exact read for an enumerated issuer. `getTokens(serverUrl, issuer)` falls
|
||||
// back to the legacy unkeyed slot when that issuer holds none, and treating
|
||||
// the fallback as the issuer's would label an old, unbound token with a
|
||||
// newly discovered authorization server and send it there.
|
||||
const tokens =
|
||||
issuer === undefined
|
||||
? await storage.getTokens(serverUrl)
|
||||
: await storage.getIssuerTokens(serverUrl, issuer);
|
||||
const revocable = selectRevocableToken(tokens);
|
||||
if (!revocable || seen.has(revocable.token)) return;
|
||||
seen.add(revocable.token);
|
||||
if (!revocable) return;
|
||||
|
||||
if (issuer === undefined) {
|
||||
// Same grant as the active issuer's slot, already collected.
|
||||
if (slotTokens.has(revocable.token)) return;
|
||||
} else {
|
||||
const key = `${issuer}\u0000${revocable.token}`;
|
||||
if (seenKeys.has(key)) return;
|
||||
seenKeys.add(key);
|
||||
slotTokens.add(revocable.token);
|
||||
}
|
||||
|
||||
grants.push({
|
||||
issuer,
|
||||
...revocable,
|
||||
|
|
@ -391,11 +418,41 @@ async function collectGrants(
|
|||
});
|
||||
};
|
||||
|
||||
for (const issuer of await storage.listIssuers(serverUrl)) {
|
||||
await add(issuer);
|
||||
/** Read one slot; a slot that throws is reported, not fatal to the rest. */
|
||||
const addSafely = async (issuer?: string): Promise<void> => {
|
||||
try {
|
||||
await add(issuer);
|
||||
} catch (err) {
|
||||
const where =
|
||||
issuer === undefined ? "the unkeyed slot" : `issuer ${issuer}`;
|
||||
failures.push({
|
||||
status: "failed",
|
||||
detail: `could not read the stored grant for ${where}: ${
|
||||
err instanceof Error ? err.message : String(err)
|
||||
}`,
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
// A `listIssuers` failure is fatal on its own — there is nothing to
|
||||
// enumerate — but the ctx-less read below can still find a legacy grant, so
|
||||
// it is recorded rather than thrown.
|
||||
let issuers: string[] = [];
|
||||
try {
|
||||
issuers = await storage.listIssuers(serverUrl);
|
||||
} catch (err) {
|
||||
failures.push({
|
||||
status: "failed",
|
||||
detail: `could not list the stored authorization servers: ${
|
||||
err instanceof Error ? err.message : String(err)
|
||||
}`,
|
||||
});
|
||||
}
|
||||
await add();
|
||||
return grants;
|
||||
for (const issuer of issuers) {
|
||||
await addSafely(issuer);
|
||||
}
|
||||
await addSafely();
|
||||
return { grants, failures };
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -430,16 +487,34 @@ async function computeOutcome(
|
|||
try {
|
||||
// Grants first. They are the cheapest disqualifier, and a server with no
|
||||
// stored grant should not provoke a metadata read at all.
|
||||
const grants = await collectGrants(storage, serverUrl);
|
||||
if (grants.length === 0) return { status: "skipped", reason: "no_tokens" };
|
||||
//
|
||||
// `failures` seeds `outcomes` rather than short-circuiting: a slot that
|
||||
// could not be read is still about to be deleted, so it has to be reported
|
||||
// *alongside* whatever the readable grants do, not instead of them.
|
||||
const { grants, failures } = await collectGrants(storage, serverUrl);
|
||||
if (grants.length === 0) {
|
||||
return failures.length > 0
|
||||
? aggregateOutcomes(failures)
|
||||
: { status: "skipped", reason: "no_tokens" };
|
||||
}
|
||||
|
||||
const metadata = await storage.getServerMetadata(serverUrl);
|
||||
if (!metadata) return { status: "skipped", reason: "no_metadata" };
|
||||
if (!metadata) {
|
||||
return aggregateOutcomes([
|
||||
...failures,
|
||||
{ status: "skipped", reason: "no_metadata" },
|
||||
]);
|
||||
}
|
||||
const endpoint = metadata.revocation_endpoint;
|
||||
if (!endpoint) return { status: "skipped", reason: "no_endpoint" };
|
||||
if (!endpoint) {
|
||||
return aggregateOutcomes([
|
||||
...failures,
|
||||
{ status: "skipped", reason: "no_endpoint" },
|
||||
]);
|
||||
}
|
||||
|
||||
const supportedAuthMethods = revocationAuthMethods(metadata);
|
||||
const outcomes: TokenRevocationOutcome[] = [];
|
||||
const outcomes: TokenRevocationOutcome[] = [...failures];
|
||||
for (const grant of grants) {
|
||||
// Metadata is cached once per server, not per issuer, so it describes
|
||||
// whichever authorization server was discovered last. Sending another
|
||||
|
|
|
|||
Loading…
Reference in a new issue