Address @claude review: issuer-stamp source gating + clear/activeIssuer + phase over-match
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
- SEP-2352: stamp the issuer only when the credential actually came from the byIssuer slot (`fromSlot`), not merely when a slot exists. A partially-populated slot alongside a surviving legacy field would otherwise stamp a legacy (possibly cross-AS) credential with the current issuer, defeating discardIfIssuerMismatch. - Issuer-scoped clears no longer promote the cleared issuer to `activeIssuer` (updateIssuerSlot gains a `setActive` flag; save paths keep the promotion). - oauthNetworkPhase matches register/token/authorize as the final path segment (trailing slash tolerated) so `/token/refresh` or `/api/register/foo` no longer over-match. - Tests for all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
5feebf25c2
commit
111d50b325
4 changed files with 102 additions and 9 deletions
|
|
@ -533,6 +533,63 @@ describe("BrowserOAuthStorage", () => {
|
|||
});
|
||||
});
|
||||
|
||||
it("does not stamp a legacy fallback credential when the issuer slot lacks it", async () => {
|
||||
// Legacy unkeyed client info survives, but a byIssuer slot exists for
|
||||
// issuerA holding only tokens (no clientInformation). Reading client info
|
||||
// for issuerA must fall back to the legacy value WITHOUT stamping it with
|
||||
// issuerA — the legacy credential may have been minted by a different AS.
|
||||
await storage.saveClientInformation(
|
||||
testServerUrl,
|
||||
{ client_id: "legacy-client" },
|
||||
{ registrationKind: "dcr" },
|
||||
);
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "tok-a", token_type: "Bearer" },
|
||||
{ issuer: issuerA },
|
||||
);
|
||||
|
||||
const clientInfo = await storage.getClientInformation(
|
||||
testServerUrl,
|
||||
false,
|
||||
issuerA,
|
||||
);
|
||||
// No issuer stamp — so the SDK's discardIfIssuerMismatch stays engaged.
|
||||
expect(clientInfo).toEqual({ client_id: "legacy-client" });
|
||||
|
||||
// Symmetric case: legacy tokens survive, slot has only clientInformation.
|
||||
await storage.clear(testServerUrl);
|
||||
await storage.saveTokens(testServerUrl, {
|
||||
access_token: "legacy-tok",
|
||||
token_type: "Bearer",
|
||||
});
|
||||
await storage.saveClientInformation(
|
||||
testServerUrl,
|
||||
{ client_id: "client-a" },
|
||||
{ registrationKind: "dcr", issuer: issuerA },
|
||||
);
|
||||
const tokens = await storage.getTokens(testServerUrl, issuerA);
|
||||
expect(tokens).toEqual({
|
||||
access_token: "legacy-tok",
|
||||
token_type: "Bearer",
|
||||
});
|
||||
});
|
||||
|
||||
it("does not promote a cleared issuer to the active (ctx-less) slot", async () => {
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "tok-a", token_type: "Bearer" },
|
||||
{ issuer: issuerA },
|
||||
);
|
||||
// Clearing a *different* issuer must not make it the active issuer.
|
||||
await storage.clearTokens(testServerUrl, issuerB);
|
||||
|
||||
// The ctx-less (per-request bearer) read still resolves issuerA's token.
|
||||
expect(await storage.getTokens(testServerUrl)).toMatchObject({
|
||||
access_token: "tok-a",
|
||||
});
|
||||
});
|
||||
|
||||
it("clears every issuer's tokens/registration when no issuer is given", async () => {
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
|
|
|
|||
|
|
@ -45,6 +45,20 @@ describe("oauthNetworkPhase", () => {
|
|||
expect(oauthNetworkPhase("https://mcp.example.com/mcp")).toBeUndefined();
|
||||
});
|
||||
|
||||
it("matches the endpoint as the final path segment, not a nested one", () => {
|
||||
// A nested segment must not classify as the OAuth endpoint...
|
||||
expect(
|
||||
oauthNetworkPhase("https://as.example.com/oauth/token/refresh"),
|
||||
).toBeUndefined();
|
||||
expect(
|
||||
oauthNetworkPhase("https://gw.example.com/api/register/foo"),
|
||||
).toBeUndefined();
|
||||
// ...but a trailing slash on the real endpoint is tolerated.
|
||||
expect(oauthNetworkPhase("https://as.example.com/oauth/token/")).toBe(
|
||||
"token",
|
||||
);
|
||||
});
|
||||
|
||||
it("tolerates a non-absolute URL by stripping the query", () => {
|
||||
expect(oauthNetworkPhase("/register?foo=bar")).toBe("registration");
|
||||
expect(oauthNetworkPhase("not a url")).toBeUndefined();
|
||||
|
|
|
|||
|
|
@ -41,13 +41,16 @@ export function oauthNetworkPhase(
|
|||
) {
|
||||
return "discovery";
|
||||
}
|
||||
if (path.endsWith("/register") || path.includes("/register/")) {
|
||||
// Match the endpoint as the final path segment (trailing slash tolerated) so a
|
||||
// nested path like `/token/refresh` or `/api/register/foo` is not misclassified.
|
||||
const endpoint = path.replace(/\/+$/, "");
|
||||
if (endpoint.endsWith("/register")) {
|
||||
return "registration";
|
||||
}
|
||||
if (path.endsWith("/token") || path.includes("/token/")) {
|
||||
if (endpoint.endsWith("/token")) {
|
||||
return "token";
|
||||
}
|
||||
if (path.endsWith("/authorize") || path.includes("/authorize/")) {
|
||||
if (endpoint.endsWith("/authorize")) {
|
||||
return "authorize";
|
||||
}
|
||||
return undefined;
|
||||
|
|
|
|||
|
|
@ -112,6 +112,10 @@ export class OAuthStorageBase implements OAuthStorage {
|
|||
issuer: string,
|
||||
updates: Partial<IssuerBoundOAuthState>,
|
||||
clearLegacy: Partial<ServerOAuthState>,
|
||||
// Saves promote the issuer to `activeIssuer` (it answers ctx-less reads);
|
||||
// clears must not — clearing one AS's credentials shouldn't make it the
|
||||
// active one.
|
||||
setActive = true,
|
||||
): void {
|
||||
const state = this.memory.getState().getServerState(serverUrl);
|
||||
const byIssuer = {
|
||||
|
|
@ -120,7 +124,7 @@ export class OAuthStorageBase implements OAuthStorage {
|
|||
};
|
||||
this.memory.getState().setServerState(serverUrl, {
|
||||
byIssuer,
|
||||
activeIssuer: issuer,
|
||||
...(setActive && { activeIssuer: issuer }),
|
||||
...clearLegacy,
|
||||
});
|
||||
}
|
||||
|
|
@ -154,14 +158,19 @@ export class OAuthStorageBase implements OAuthStorage {
|
|||
|
||||
// Per-issuer registration (SEP-2352), falling back to the legacy unkeyed slot.
|
||||
const slot = this.issuerSlot(state, issuer);
|
||||
const clientInfo = slot?.clientInformation ?? state.clientInformation;
|
||||
const fromSlot = slot?.clientInformation;
|
||||
const clientInfo = fromSlot ?? state.clientInformation;
|
||||
if (!clientInfo) {
|
||||
return undefined;
|
||||
}
|
||||
const parsed = await OAuthClientInformationSchema.parseAsync(clientInfo);
|
||||
// Stamp only when the value actually came from the issuer-keyed slot. Gating
|
||||
// on `fromSlot` (not merely on the slot existing) keeps a legacy unkeyed
|
||||
// credential — potentially minted by a *different* AS — from being stamped
|
||||
// with this issuer, which would defeat the SDK's discardIfIssuerMismatch.
|
||||
return withIssuer(
|
||||
parsed,
|
||||
slot ? this.resolveReadIssuer(state, issuer) : undefined,
|
||||
fromSlot ? this.resolveReadIssuer(state, issuer) : undefined,
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -237,6 +246,7 @@ export class OAuthStorageBase implements OAuthStorage {
|
|||
issuer,
|
||||
{ clientInformation: undefined, clientRegistrationKind: undefined },
|
||||
{},
|
||||
false,
|
||||
);
|
||||
} else {
|
||||
// Clear every issuer's registration plus the legacy unkeyed entry.
|
||||
|
|
@ -261,14 +271,17 @@ export class OAuthStorageBase implements OAuthStorage {
|
|||
await this.ensureLoaded();
|
||||
const state = this.memory.getState().getServerState(serverUrl);
|
||||
const slot = this.issuerSlot(state, issuer);
|
||||
const tokens = slot?.tokens ?? state.tokens;
|
||||
const fromSlot = slot?.tokens;
|
||||
const tokens = fromSlot ?? state.tokens;
|
||||
if (!tokens) {
|
||||
return undefined;
|
||||
}
|
||||
const parsed = await OAuthTokensSchema.parseAsync(tokens);
|
||||
// Stamp only when the value came from the issuer-keyed slot (see
|
||||
// getClientInformation) — never stamp a legacy unkeyed token with this issuer.
|
||||
return withIssuer(
|
||||
parsed,
|
||||
slot ? this.resolveReadIssuer(state, issuer) : undefined,
|
||||
fromSlot ? this.resolveReadIssuer(state, issuer) : undefined,
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -302,7 +315,13 @@ export class OAuthStorageBase implements OAuthStorage {
|
|||
async clearTokens(serverUrl: string, issuer?: string): Promise<void> {
|
||||
await this.ensureLoaded();
|
||||
if (issuer !== undefined) {
|
||||
this.updateIssuerSlot(serverUrl, issuer, { tokens: undefined }, {});
|
||||
this.updateIssuerSlot(
|
||||
serverUrl,
|
||||
issuer,
|
||||
{ tokens: undefined },
|
||||
{},
|
||||
false,
|
||||
);
|
||||
} else {
|
||||
const state = this.memory.getState().getServerState(serverUrl);
|
||||
const byIssuer = this.mapIssuerSlots(state, () => ({
|
||||
|
|
|
|||
Loading…
Reference in a new issue