Merge branch 'v2/main' into v2/fix/2117-persist-requested-scope
#2114 landed on v2/main, which is the PR this one follows up. Two conflicts, one of them silent. The marked conflict was cosmetic: keep v2/main's comment on the `provider.scope` read, which now notes the getter filters (#2068). The real one git merged cleanly and would have left dead: #2114's if (this.oauthConfig.requestRefreshToken === false && requestedScope) { this.pendingAuthorizationScope = requestedScope; } sat directly above this branch's unconditional `recordAuthorizationRequestScope`, which overwrites the same field a few lines later. Dropping that gate is what #2117 is for, so it is removed rather than kept as a no-op. The two changes compose: the SDK adds `offline_access` only when `clientMetadata.grant_types` includes `refresh_token`, which the provider drops while the opt-out is on -- so reading the authorize URL yields the filtered scope there, exactly what #2114's gated branch persisted. #2114's third case pinned the gate and inverts here, as its issue predicted. It now asserts the request is persisted even with the grant on; that records no new claim, since the request equals what storage already held. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
commit
71b960e078
7 changed files with 1424 additions and 8 deletions
|
|
@ -464,6 +464,125 @@ describe("OAuthNavigation", () => {
|
|||
expect(provider.clientMetadata.application_type).toBe("native");
|
||||
});
|
||||
|
||||
describe("requestRefreshToken (#2068)", () => {
|
||||
function makeProviderWithRefresh(
|
||||
requestRefreshToken: boolean | undefined,
|
||||
): BaseOAuthClientProvider {
|
||||
return new BaseOAuthClientProvider(SERVER, {
|
||||
storage: makeStorage(),
|
||||
redirectUrlProvider: new MutableRedirectUrlProvider(),
|
||||
navigation: new CallbackNavigation(vi.fn()),
|
||||
requestRefreshToken,
|
||||
});
|
||||
}
|
||||
|
||||
it("declares the refresh_token grant by default", () => {
|
||||
expect(makeProvider(makeStorage()).clientMetadata.grant_types).toEqual([
|
||||
"authorization_code",
|
||||
"refresh_token",
|
||||
]);
|
||||
});
|
||||
|
||||
it("declares the refresh_token grant when explicitly enabled", () => {
|
||||
expect(
|
||||
makeProviderWithRefresh(true).clientMetadata.grant_types,
|
||||
).toEqual(["authorization_code", "refresh_token"]);
|
||||
});
|
||||
|
||||
it("treats an omitted setting as enabled", () => {
|
||||
expect(
|
||||
makeProviderWithRefresh(undefined).clientMetadata.grant_types,
|
||||
).toEqual(["authorization_code", "refresh_token"]);
|
||||
});
|
||||
|
||||
// The point of the opt-out: the SDK's determineScope() only appends
|
||||
// `offline_access` when the client metadata declares `refresh_token`.
|
||||
// This asserts our half — the grant list. That the authorize URL then
|
||||
// carries neither `offline_access` nor `prompt=consent` is proved against
|
||||
// a real AS in the e2e suite (inspectorClient-oauth-e2e.test.ts), since
|
||||
// the SDK, not this array, is what ultimately writes that URL.
|
||||
it("drops the refresh_token grant when disabled", () => {
|
||||
expect(
|
||||
makeProviderWithRefresh(false).clientMetadata.grant_types,
|
||||
).toEqual(["authorization_code"]);
|
||||
});
|
||||
|
||||
// The regression that makes the opt-out actually work for a server that
|
||||
// already authorized once. A successful default-on grant persists the
|
||||
// AS-granted scope — `offline_access` included — and the provider reloads
|
||||
// it here. Handing that straight back to the SDK puts the token in scope
|
||||
// again, and `startAuthorization` appends `prompt=consent` off the scope
|
||||
// alone, so the box would be unchecked and AADSTS90094 would persist.
|
||||
describe("scope inherited from an earlier default-on grant", () => {
|
||||
function makeProviderWithScopes(
|
||||
storedScope: string,
|
||||
configuredScope: string | undefined,
|
||||
requestRefreshToken: boolean,
|
||||
): { provider: BaseOAuthClientProvider; storage: OAuthStorage } {
|
||||
const storage = makeStorage();
|
||||
vi.mocked(storage.getScope).mockResolvedValue(storedScope);
|
||||
const provider = new BaseOAuthClientProvider(SERVER, {
|
||||
storage,
|
||||
redirectUrlProvider: new MutableRedirectUrlProvider(),
|
||||
navigation: new CallbackNavigation(vi.fn()),
|
||||
requestRefreshToken,
|
||||
configuredScope,
|
||||
});
|
||||
return { provider, storage };
|
||||
}
|
||||
|
||||
it("drops the inherited offline_access from the requested scope", async () => {
|
||||
const { provider } = makeProviderWithScopes(
|
||||
"mcp offline_access",
|
||||
"mcp",
|
||||
false,
|
||||
);
|
||||
await provider.prepareForAuth();
|
||||
|
||||
expect(provider.scope).toBe("mcp");
|
||||
expect(provider.clientMetadata.scope).toBe("mcp");
|
||||
});
|
||||
|
||||
it("honors an offline_access the user configured", async () => {
|
||||
const { provider } = makeProviderWithScopes(
|
||||
"mcp offline_access",
|
||||
"mcp offline_access",
|
||||
false,
|
||||
);
|
||||
await provider.prepareForAuth();
|
||||
|
||||
expect(provider.scope).toBe("mcp offline_access");
|
||||
});
|
||||
|
||||
it("leaves the scope alone while the grant is declared", async () => {
|
||||
const { provider } = makeProviderWithScopes(
|
||||
"mcp offline_access",
|
||||
"mcp",
|
||||
true,
|
||||
);
|
||||
await provider.prepareForAuth();
|
||||
|
||||
expect(provider.scope).toBe("mcp offline_access");
|
||||
});
|
||||
|
||||
// Filtered at the point of request, never in storage: re-checking the
|
||||
// box must restore the previous behavior rather than find the value
|
||||
// destroyed, and the persisted scope is also what scope-satisfaction
|
||||
// checks read.
|
||||
it("does not rewrite the persisted scope", async () => {
|
||||
const { provider, storage } = makeProviderWithScopes(
|
||||
"mcp offline_access",
|
||||
"mcp",
|
||||
false,
|
||||
);
|
||||
await provider.prepareForAuth();
|
||||
void provider.scope;
|
||||
|
||||
expect(storage.saveScope).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("SEP-2352 issuer threading", () => {
|
||||
it("forwards ctx.issuer to storage on clientInformation/tokens reads", async () => {
|
||||
const storage = makeStorage();
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import {
|
|||
isStrictScopeSuperset,
|
||||
resolveEffectiveGrantedScope,
|
||||
resolvePersistedScopeAfterGrant,
|
||||
scopeForDeclinedRefreshGrant,
|
||||
} from "@inspector/core/auth/scopes.js";
|
||||
|
||||
describe("scopes", () => {
|
||||
|
|
@ -65,3 +66,93 @@ describe("scopes", () => {
|
|||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("scopeForDeclinedRefreshGrant (#2068)", () => {
|
||||
it("drops an inherited offline_access from the requested scope", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("mcp offline_access", "mcp")).toBe(
|
||||
"mcp",
|
||||
);
|
||||
});
|
||||
|
||||
// The persisted scope is what a previous default-on grant left behind, and
|
||||
// the configured scope may be unset entirely (DCR + resource-advertised
|
||||
// scopes). An inherited token is still inherited.
|
||||
it("drops it when no scope is configured at all", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("mcp offline_access", undefined)).toBe(
|
||||
"mcp",
|
||||
);
|
||||
});
|
||||
|
||||
it("keeps an offline_access the user explicitly configured", () => {
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant("mcp offline_access", "mcp offline_access"),
|
||||
).toBe("mcp offline_access");
|
||||
});
|
||||
|
||||
// Storage can predate the settings change — `createOAuthProvider` preserves a
|
||||
// stored scope rather than reseeding from current settings. Merely declining
|
||||
// to strip would silently omit a scope the user just typed, and the form's
|
||||
// "still requested" warning would be lying.
|
||||
it("adds a newly configured offline_access that stale storage lacks", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("mcp", "mcp offline_access")).toBe(
|
||||
"mcp offline_access",
|
||||
);
|
||||
});
|
||||
|
||||
// Only the token itself is added; unrelated configured scopes are not unioned
|
||||
// in, since widening the request could force a step-up authorization.
|
||||
it("does not union unrelated configured scopes into the request", () => {
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant("mcp", "tools:read offline_access"),
|
||||
).toBe("mcp offline_access");
|
||||
});
|
||||
|
||||
it("does not duplicate an offline_access already present", () => {
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant(" mcp offline_access ", "offline_access"),
|
||||
).toBe("mcp offline_access");
|
||||
});
|
||||
|
||||
it("leaves a scope without offline_access untouched", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("mcp tools:read", "mcp")).toBe(
|
||||
"mcp tools:read",
|
||||
);
|
||||
});
|
||||
|
||||
// Requesting `scope=` empty is not the same as omitting it; collapse to
|
||||
// undefined so the SDK falls back to its own resolution.
|
||||
it("collapses to undefined when nothing is left and nothing is configured", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("offline_access", "")).toBeUndefined();
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant("offline_access", undefined),
|
||||
).toBeUndefined();
|
||||
});
|
||||
|
||||
// Returning undefined here would read as "nothing stored" to
|
||||
// OAuthManager.createOAuthProvider, whose seeding branch writes the
|
||||
// configured scope to storage — turning a request-only filter into a silent
|
||||
// overwrite. The configured scope is both the right request and inert to that
|
||||
// branch.
|
||||
it("falls back to the configured scope rather than emptying out", () => {
|
||||
expect(scopeForDeclinedRefreshGrant("offline_access", "mcp")).toBe("mcp");
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant(" offline_access ", " mcp tools:read "),
|
||||
).toBe("mcp tools:read");
|
||||
});
|
||||
|
||||
it("passes an absent scope through", () => {
|
||||
expect(scopeForDeclinedRefreshGrant(undefined, "mcp")).toBeUndefined();
|
||||
});
|
||||
|
||||
it("matches whole tokens, not substrings", () => {
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant("mcp offline_access_extra", "mcp"),
|
||||
).toBe("mcp offline_access_extra");
|
||||
});
|
||||
|
||||
it("tolerates irregular whitespace", () => {
|
||||
expect(
|
||||
scopeForDeclinedRefreshGrant(" mcp offline_access ", "mcp"),
|
||||
).toBe("mcp");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -86,6 +86,34 @@ describe("runner client auth options", () => {
|
|||
expect(opts.oauth?.clientId).toBe("resource-client");
|
||||
});
|
||||
|
||||
// #2068 — the refresh-token opt-out is a per-server setting, so the CLI/TUI
|
||||
// leg honors it exactly as the web leg does.
|
||||
it("buildRunnerClientAuthOptions forwards the refresh-token opt-out", () => {
|
||||
const base: InspectorServerSettings = {
|
||||
requestTimeout: 0,
|
||||
connectionTimeout: 0,
|
||||
taskTtl: 60000,
|
||||
maxFetchRequests: 10,
|
||||
metadata: {},
|
||||
headers: [],
|
||||
env: [],
|
||||
roots: [],
|
||||
};
|
||||
expect(
|
||||
buildRunnerClientAuthOptions(
|
||||
{},
|
||||
{
|
||||
...base,
|
||||
oauthRequestRefreshToken: false,
|
||||
},
|
||||
).oauth?.requestRefreshToken,
|
||||
).toBe(false);
|
||||
// On (the default) forwards nothing, so the provider keeps its own default.
|
||||
expect(
|
||||
buildRunnerClientAuthOptions({}, base).oauth?.requestRefreshToken,
|
||||
).toBeUndefined();
|
||||
});
|
||||
|
||||
// #2018 — the CLI/TUI leg picks the authorization parameters up from the same
|
||||
// per-server settings the web leg reads, so all three clients agree.
|
||||
it("buildRunnerClientAuthOptions forwards custom authorization params", () => {
|
||||
|
|
|
|||
|
|
@ -1268,6 +1268,179 @@ describe("OAuthManager", () => {
|
|||
).toBe(false);
|
||||
});
|
||||
|
||||
// #2068 round 11 — the callback leg of an *ordinary* authorization.
|
||||
// `resolvePersistedScopeAfterGrant` falls back to the requested scope when
|
||||
// the token response omits `scope` (RFC 6749 §5.1), but only step-up ever
|
||||
// populated that fallback. So a filtered request whose AS omitted `scope`
|
||||
// persisted nothing, the stale `offline_access` survived in storage, and
|
||||
// the "self-healing" this feature documents never happened.
|
||||
//
|
||||
// #2068 fixed that only while the opt-out was on, which was the case where
|
||||
// a stale scope caused a user-visible failure. #2117 dropped the gate, so
|
||||
// the recording is now unconditional and reads the authorize URL rather
|
||||
// than the provider — see `recordAuthorizationRequestScope`. The filter
|
||||
// still applies with the opt-out on, because the SDK's `offline_access`
|
||||
// augmentation is gated on the `refresh_token` grant type the provider
|
||||
// drops there.
|
||||
describe("persisting the filtered scope after an ordinary grant (#2068)", () => {
|
||||
async function completeWith(
|
||||
tokenScope: string | undefined,
|
||||
requestRefreshToken: boolean | undefined,
|
||||
): Promise<ReturnType<typeof storageOf>> {
|
||||
const params = createMockParams();
|
||||
const storage = storageOf(params);
|
||||
storage.getScope.mockResolvedValue("mcp offline_access");
|
||||
storage.getClientInformation.mockResolvedValue({ client_id: "cid" });
|
||||
const manager = new OAuthManager(params);
|
||||
manager.setOAuthConfig({
|
||||
scope: "mcp",
|
||||
...(requestRefreshToken !== undefined && { requestRefreshToken }),
|
||||
});
|
||||
mockedMcpAuth.mockResolvedValue("REDIRECT");
|
||||
|
||||
const captureSpy = vi
|
||||
.spyOn(
|
||||
(await import("@inspector/core/auth/providers.js"))
|
||||
.BaseOAuthClientProvider.prototype,
|
||||
"getCapturedAuthUrl",
|
||||
)
|
||||
.mockReturnValue(new URL("https://as.example.com/authorize?state=s"));
|
||||
await manager.authenticate();
|
||||
captureSpy.mockRestore();
|
||||
|
||||
// The callback leg calls `mcpAuth` again and requires AUTHORIZED.
|
||||
mockedMcpAuth.mockResolvedValue("AUTHORIZED");
|
||||
storage.getTokens.mockResolvedValue({
|
||||
access_token: "access",
|
||||
token_type: "Bearer",
|
||||
...(tokenScope !== undefined && { scope: tokenScope }),
|
||||
});
|
||||
await manager.completeOAuthFlow("code");
|
||||
return storage;
|
||||
}
|
||||
|
||||
it("persists the filtered request when the token response omits scope", async () => {
|
||||
const storage = await completeWith(undefined, false);
|
||||
expect(storage.saveScope).toHaveBeenLastCalledWith(SERVER_URL, "mcp");
|
||||
});
|
||||
|
||||
// An explicit grant is authoritative and wins over what we requested.
|
||||
it("persists the granted scope when the token response carries one", async () => {
|
||||
const storage = await completeWith("mcp tools:read", false);
|
||||
expect(storage.saveScope).toHaveBeenLastCalledWith(
|
||||
SERVER_URL,
|
||||
"mcp tools:read",
|
||||
);
|
||||
});
|
||||
|
||||
// #2117 inverted this. #2068 gated the recording on the opt-out being
|
||||
// on, so with the grant declared the ordinary leg persisted nothing;
|
||||
// that gate is gone, and one wire case now has one behavior rather than
|
||||
// two selected by a checkbox. Persisting here records no new claim: the
|
||||
// request is what storage already held, so this rewrites the same value
|
||||
// rather than asserting a scope the AS never mentioned.
|
||||
it("persists the request even with the grant on, matching what storage held", async () => {
|
||||
const storage = await completeWith(undefined, undefined);
|
||||
expect(storage.saveScope).toHaveBeenLastCalledWith(
|
||||
SERVER_URL,
|
||||
"mcp offline_access",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// #2068 round 8 — the step-up path never reads the provider's filtering
|
||||
// `scope` getter: `enrichChallengeWithScopes` unions the *raw* stored scope
|
||||
// with the challenge and `handleAuthChallenge` hands that straight to
|
||||
// `mcpAuth`. So an inherited `offline_access` reappeared here with the
|
||||
// setting off, and the SDK re-added `prompt=consent` — the exact failure
|
||||
// the option exists to prevent, on the one path that bypassed it.
|
||||
describe("step-up scope with the refresh grant declined (#2068)", () => {
|
||||
/** The `scope` this challenge run handed to the SDK. */
|
||||
async function scopeSentForChallenge(
|
||||
storedScope: string,
|
||||
configuredScope: string | undefined,
|
||||
requiredScopes: string[],
|
||||
requestRefreshToken: boolean | undefined,
|
||||
): Promise<string | undefined> {
|
||||
const params = createMockParams();
|
||||
storageOf(params).getScope.mockResolvedValue(storedScope);
|
||||
storageOf(params).getTokens.mockResolvedValue({
|
||||
access_token: "tok",
|
||||
token_type: "Bearer",
|
||||
scope: storedScope,
|
||||
});
|
||||
storageOf(params).getClientInformation.mockResolvedValue({
|
||||
client_id: "cid",
|
||||
});
|
||||
const manager = new OAuthManager(params);
|
||||
manager.setOAuthConfig({
|
||||
...(configuredScope !== undefined && { scope: configuredScope }),
|
||||
...(requestRefreshToken !== undefined && { requestRefreshToken }),
|
||||
});
|
||||
mockedMcpAuth.mockResolvedValue("REDIRECT");
|
||||
|
||||
await manager.handleAuthChallenge({
|
||||
reason: "insufficient_scope",
|
||||
requiredScopes,
|
||||
});
|
||||
|
||||
const call = mockedMcpAuth.mock.calls.at(-1);
|
||||
return (call?.[1] as { scope?: string } | undefined)?.scope;
|
||||
}
|
||||
|
||||
it("drops an inherited offline_access from the step-up union", async () => {
|
||||
const sent = await scopeSentForChallenge(
|
||||
"mcp offline_access",
|
||||
"mcp",
|
||||
["tools:write"],
|
||||
false,
|
||||
);
|
||||
expect(sent?.split(/\s+/)).not.toContain("offline_access");
|
||||
// The step-up still asks for what the challenge demanded.
|
||||
expect(sent?.split(/\s+/)).toContain("tools:write");
|
||||
});
|
||||
|
||||
it("keeps offline_access in the step-up union while the grant is on", async () => {
|
||||
const sent = await scopeSentForChallenge(
|
||||
"mcp offline_access",
|
||||
"mcp",
|
||||
["tools:write"],
|
||||
undefined,
|
||||
);
|
||||
expect(sent?.split(/\s+/)).toContain("offline_access");
|
||||
});
|
||||
|
||||
// Stripping a scope the challenge itself requires would loop:
|
||||
// re-authorize, earn the same challenge, strip it again.
|
||||
it("preserves an offline_access the challenge requires", async () => {
|
||||
const sent = await scopeSentForChallenge(
|
||||
"mcp",
|
||||
"mcp",
|
||||
["offline_access"],
|
||||
false,
|
||||
);
|
||||
expect(sent?.split(/\s+/)).toContain("offline_access");
|
||||
});
|
||||
|
||||
// The mixed case, and the one that made the guard above insufficient:
|
||||
// `enrichChallengeWithScopes` narrows `requiredScopes` to the *missing*
|
||||
// subset, so a challenge requiring both — against a stored scope that
|
||||
// already carries `offline_access` — arrives as just `["tools:write"]`.
|
||||
// Reading the narrowed list treats the server's explicit requirement as
|
||||
// inherited and strips it, and re-authorization then earns the same
|
||||
// challenge forever.
|
||||
it("preserves a required offline_access when the challenge also names a missing scope", async () => {
|
||||
const sent = await scopeSentForChallenge(
|
||||
"mcp offline_access",
|
||||
"mcp",
|
||||
["offline_access", "tools:write"],
|
||||
false,
|
||||
);
|
||||
expect(sent?.split(/\s+/)).toContain("offline_access");
|
||||
expect(sent?.split(/\s+/)).toContain("tools:write");
|
||||
});
|
||||
});
|
||||
|
||||
it("short-circuits handleAuthChallenge when scope already satisfied", async () => {
|
||||
const params = createMockParams();
|
||||
storageOf(params).getTokens.mockResolvedValue({
|
||||
|
|
@ -1817,6 +1990,54 @@ describe("OAuthManager", () => {
|
|||
expect(navigated.searchParams.get("state")).toBe("xyz");
|
||||
});
|
||||
|
||||
// #2068 — the manager→provider bridge for the refresh-token opt-out. Same
|
||||
// gap as the authorizationParams case above: the provider test constructs
|
||||
// `BaseOAuthClientProvider` directly and the runner test stops at the
|
||||
// options object, so deleting `requestRefreshToken:` from
|
||||
// `createOAuthProvider` would leave the feature dead with every other test
|
||||
// still green.
|
||||
it("forwards the refresh-token opt-out to the provider it builds", async () => {
|
||||
const manager = new OAuthManager(createMockParams());
|
||||
manager.setOAuthConfig({ requestRefreshToken: false });
|
||||
|
||||
const provider = await manager.createOAuthProviderForTransport();
|
||||
expect(provider.clientMetadata.grant_types).toEqual([
|
||||
"authorization_code",
|
||||
]);
|
||||
});
|
||||
|
||||
// #2068 round 5 — the provider-level test for "storage is never rewritten"
|
||||
// could not see this: `createOAuthProvider` reads `provider.scope` (the
|
||||
// filtered getter) and seeds storage when it comes back undefined. A
|
||||
// persisted scope of `offline_access` alone therefore used to be silently
|
||||
// overwritten by the configured scope, which is the one thing the filter
|
||||
// promises not to do. Exercised through the manager, where the seeding
|
||||
// branch actually lives.
|
||||
it("does not overwrite a persisted scope that filters down to nothing", async () => {
|
||||
const params = createMockParams();
|
||||
const storage = params.initialConfig.storage;
|
||||
if (!storage) throw new Error("expected mock storage");
|
||||
vi.mocked(storage.getScope).mockResolvedValue("offline_access");
|
||||
|
||||
const manager = new OAuthManager(params);
|
||||
manager.setOAuthConfig({ requestRefreshToken: false, scope: "mcp" });
|
||||
const provider = await manager.createOAuthProviderForTransport();
|
||||
|
||||
expect(storage.saveScope).not.toHaveBeenCalled();
|
||||
// Still requests the configured scope, without the declined token.
|
||||
expect(provider.clientMetadata.scope).toBe("mcp");
|
||||
});
|
||||
|
||||
it("declares the refresh_token grant when the opt-out is not configured", async () => {
|
||||
const manager = new OAuthManager(createMockParams());
|
||||
|
||||
const provider = await manager.createOAuthProviderForTransport();
|
||||
expect(provider.clientMetadata.grant_types).toEqual([
|
||||
"authorization_code",
|
||||
"refresh_token",
|
||||
]);
|
||||
});
|
||||
|
||||
it("drops a reserved key configured on the manager", async () => {
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const params = createMockParams();
|
||||
|
|
|
|||
|
|
@ -1193,6 +1193,68 @@ describe("enterpriseManaged oauth settings", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("oauth.requestRefreshToken (#2068)", () => {
|
||||
const baseSettings = {
|
||||
headers: [],
|
||||
env: [],
|
||||
metadata: {},
|
||||
connectionTimeout: 0,
|
||||
requestTimeout: 0,
|
||||
taskTtl: 60000,
|
||||
maxFetchRequests: 1000,
|
||||
roots: [],
|
||||
};
|
||||
|
||||
it("lifts an explicit false to settings.oauthRequestRefreshToken", () => {
|
||||
const settings = storedFieldsToInspectorSettings({
|
||||
oauth: { clientId: "cid", requestRefreshToken: false },
|
||||
});
|
||||
expect(settings?.oauthRequestRefreshToken).toBe(false);
|
||||
});
|
||||
|
||||
it("leaves the setting unset when the field is absent (default: on)", () => {
|
||||
const settings = storedFieldsToInspectorSettings({
|
||||
oauth: { clientId: "cid" },
|
||||
});
|
||||
expect(settings?.oauthRequestRefreshToken).toBeUndefined();
|
||||
});
|
||||
|
||||
it("leaves the setting unset for an explicit true, which means the default", () => {
|
||||
const settings = storedFieldsToInspectorSettings({
|
||||
oauth: { clientId: "cid", requestRefreshToken: true },
|
||||
});
|
||||
expect(settings?.oauthRequestRefreshToken).toBeUndefined();
|
||||
});
|
||||
|
||||
it("persists the opt-out under oauth on disk", () => {
|
||||
const stored = inspectorSettingsToStoredFields({
|
||||
...baseSettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
});
|
||||
expect(stored.oauth?.requestRefreshToken).toBe(false);
|
||||
});
|
||||
|
||||
it("writes nothing when the setting is on", () => {
|
||||
expect(
|
||||
inspectorSettingsToStoredFields({
|
||||
...baseSettings,
|
||||
oauthRequestRefreshToken: true,
|
||||
}).oauth,
|
||||
).toBeUndefined();
|
||||
expect(inspectorSettingsToStoredFields(baseSettings).oauth).toBeUndefined();
|
||||
});
|
||||
|
||||
it("round-trips the opt-out", () => {
|
||||
const stored = inspectorSettingsToStoredFields({
|
||||
...baseSettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
});
|
||||
expect(
|
||||
storedFieldsToInspectorSettings(stored)?.oauthRequestRefreshToken,
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("oauthOnInsufficientScope (SEP-2350)", () => {
|
||||
const baseSettings = {
|
||||
headers: [],
|
||||
|
|
|
|||
|
|
@ -1383,6 +1383,158 @@ describe("InspectorClient OAuth E2E", () => {
|
|||
if (!authUrl) throw new Error("Expected authorization URL");
|
||||
const scope = authUrl.searchParams.get("scope") ?? "";
|
||||
expect(scope.split(/\s+/)).toContain("offline_access");
|
||||
// The consequence of that scope, and the actual failure in #2068: the SDK
|
||||
// forces an interactive consent prompt. Asserted here so the opt-out case
|
||||
// below is a real before/after pair rather than a scope check alone.
|
||||
expect(authUrl.searchParams.get("prompt")).toBe("consent");
|
||||
});
|
||||
|
||||
// #2068 — the same server, the same AS advertising `offline_access`, with
|
||||
// the grant declined. This is the end-to-end proof the unit tests cannot
|
||||
// give: they assert our `grant_types` array, while the behavior that
|
||||
// actually unblocks an Entra tenant is what the SDK puts in the authorize
|
||||
// URL. If the SDK ever stopped keying its augmentation off `grant_types`,
|
||||
// every other test here would still pass and AADSTS90094 would remain.
|
||||
it("omits offline_access and prompt=consent when the refresh-token grant is declined", async () => {
|
||||
const transport = transports[1]!;
|
||||
const staticClientId = "test-no-offline-access-client";
|
||||
const staticClientSecret = "test-no-offline-access-secret";
|
||||
const serverConfig = {
|
||||
...getDefaultServerConfig(),
|
||||
serverType: transport.serverType,
|
||||
...createOAuthTestServerConfig({
|
||||
requireAuth: true,
|
||||
supportRefreshTokens: true,
|
||||
scopesSupported: ["mcp", "offline_access"],
|
||||
staticClients: [
|
||||
{
|
||||
clientId: staticClientId,
|
||||
clientSecret: staticClientSecret,
|
||||
redirectUris: [testRedirectUrl],
|
||||
},
|
||||
],
|
||||
}),
|
||||
};
|
||||
server = new TestServerHttp(serverConfig);
|
||||
const port = await server.start();
|
||||
const serverUrl = `http://localhost:${port}`;
|
||||
await waitForOAuthWellKnown(serverUrl);
|
||||
|
||||
const oauthConfig = createTestOAuthConfig({
|
||||
mode: "static",
|
||||
clientId: staticClientId,
|
||||
clientSecret: staticClientSecret,
|
||||
redirectUrl: testRedirectUrl,
|
||||
scope: "mcp",
|
||||
});
|
||||
client = new InspectorClient(
|
||||
{
|
||||
type: transport.clientType,
|
||||
url: `${serverUrl}${transport.endpoint}`,
|
||||
} as MCPServerConfig,
|
||||
{
|
||||
environment: {
|
||||
transport: createTransportNode,
|
||||
oauth: {
|
||||
storage: oauthConfig.storage,
|
||||
navigation: oauthConfig.navigation,
|
||||
redirectUrlProvider: oauthConfig.redirectUrlProvider,
|
||||
},
|
||||
},
|
||||
oauth: {
|
||||
clientId: oauthConfig.clientId,
|
||||
clientSecret: oauthConfig.clientSecret,
|
||||
scope: oauthConfig.scope,
|
||||
requestRefreshToken: false,
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
const authUrl = await client.authenticate();
|
||||
if (!authUrl) throw new Error("Expected authorization URL");
|
||||
const scope = authUrl.searchParams.get("scope") ?? "";
|
||||
expect(scope.split(/\s+/)).not.toContain("offline_access");
|
||||
expect(authUrl.searchParams.get("prompt")).toBeNull();
|
||||
// The configured scope still goes out — the opt-out drops the SDK's
|
||||
// addition, not what the user asked for.
|
||||
expect(scope.split(/\s+/)).toContain("mcp");
|
||||
});
|
||||
|
||||
// #2068 — the case that makes the opt-out worth anything to the user who
|
||||
// reported it: a server that already authorized successfully. That grant
|
||||
// persists the AS-granted scope, `offline_access` included, and the
|
||||
// provider reloads it on the next connect. Handing it back unfiltered puts
|
||||
// the token in scope again and `startAuthorization` re-adds
|
||||
// `prompt=consent` off the scope alone — box unchecked, AADSTS90094 intact.
|
||||
it("drops an offline_access inherited from an earlier grant's stored scope", async () => {
|
||||
const transport = transports[1]!;
|
||||
const staticClientId = "test-inherited-offline-client";
|
||||
const staticClientSecret = "test-inherited-offline-secret";
|
||||
const serverConfig = {
|
||||
...getDefaultServerConfig(),
|
||||
serverType: transport.serverType,
|
||||
...createOAuthTestServerConfig({
|
||||
requireAuth: true,
|
||||
supportRefreshTokens: true,
|
||||
scopesSupported: ["mcp", "offline_access"],
|
||||
staticClients: [
|
||||
{
|
||||
clientId: staticClientId,
|
||||
clientSecret: staticClientSecret,
|
||||
redirectUris: [testRedirectUrl],
|
||||
},
|
||||
],
|
||||
}),
|
||||
};
|
||||
server = new TestServerHttp(serverConfig);
|
||||
const port = await server.start();
|
||||
const serverUrl = `http://localhost:${port}`;
|
||||
await waitForOAuthWellKnown(serverUrl);
|
||||
|
||||
const oauthConfig = createTestOAuthConfig({
|
||||
mode: "static",
|
||||
clientId: staticClientId,
|
||||
clientSecret: staticClientSecret,
|
||||
redirectUrl: testRedirectUrl,
|
||||
scope: "mcp",
|
||||
});
|
||||
// Exactly what a completed default-on authorization leaves behind (see
|
||||
// `resolvePersistedScopeAfterGrant`): the scope the AS granted.
|
||||
const mcpUrl = `${serverUrl}${transport.endpoint}`;
|
||||
await oauthConfig.storage.saveScope(mcpUrl, "mcp offline_access");
|
||||
|
||||
client = new InspectorClient(
|
||||
{ type: transport.clientType, url: mcpUrl } as MCPServerConfig,
|
||||
{
|
||||
environment: {
|
||||
transport: createTransportNode,
|
||||
oauth: {
|
||||
storage: oauthConfig.storage,
|
||||
navigation: oauthConfig.navigation,
|
||||
redirectUrlProvider: oauthConfig.redirectUrlProvider,
|
||||
},
|
||||
},
|
||||
oauth: {
|
||||
clientId: oauthConfig.clientId,
|
||||
clientSecret: oauthConfig.clientSecret,
|
||||
scope: oauthConfig.scope,
|
||||
requestRefreshToken: false,
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
const authUrl = await client.authenticate();
|
||||
if (!authUrl) throw new Error("Expected authorization URL");
|
||||
const scope = authUrl.searchParams.get("scope") ?? "";
|
||||
expect(scope.split(/\s+/)).not.toContain("offline_access");
|
||||
expect(authUrl.searchParams.get("prompt")).toBeNull();
|
||||
expect(scope.split(/\s+/)).toContain("mcp");
|
||||
|
||||
// Filtered on the way out, not in storage — re-checking the box has to
|
||||
// restore the old behavior rather than find the scope destroyed.
|
||||
expect(await oauthConfig.storage.getScope(mcpUrl)).toBe(
|
||||
"mcp offline_access",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -631,6 +631,14 @@ describe("server.ts supplemental coverage", () => {
|
|||
expect((await res.json()).error).toMatch(/enterpriseManaged/);
|
||||
});
|
||||
|
||||
it("rejects a non-boolean oauthRequestRefreshToken (#2068)", async () => {
|
||||
const res = await postSettings({
|
||||
...base,
|
||||
oauthRequestRefreshToken: "no",
|
||||
});
|
||||
expect((await res.json()).error).toMatch(/oauthRequestRefreshToken/);
|
||||
});
|
||||
|
||||
it("rejects malformed roots", async () => {
|
||||
const res = await postSettings({ ...base, roots: [{ uri: 1 }] });
|
||||
expect((await res.json()).error).toMatch(/roots/);
|
||||
|
|
@ -667,6 +675,39 @@ describe("server.ts supplemental coverage", () => {
|
|||
expect(res.status).toBe(200);
|
||||
});
|
||||
|
||||
// #2068 — a 200 only proves the payload validated. Without reading the
|
||||
// saved entry back, deleting the `oauthRequestRefreshToken` line from
|
||||
// `normalizeSettings` leaves every other test green while saves through
|
||||
// this route silently revert to the default.
|
||||
it("persists the refresh-token opt-out through a save", async () => {
|
||||
expect(
|
||||
(await postSettings({ ...base, oauthRequestRefreshToken: false }))
|
||||
.status,
|
||||
).toBe(200);
|
||||
|
||||
const res = await fetch(`${h.baseUrl}/api/servers`);
|
||||
const body = (await res.json()) as {
|
||||
mcpServers: Record<
|
||||
string,
|
||||
{ oauth?: { requestRefreshToken?: boolean } }
|
||||
>;
|
||||
};
|
||||
expect(body.mcpServers.srv?.oauth?.requestRefreshToken).toBe(false);
|
||||
});
|
||||
|
||||
it("writes no refresh-token field when the setting is on", async () => {
|
||||
expect(
|
||||
(await postSettings({ ...base, oauthRequestRefreshToken: true }))
|
||||
.status,
|
||||
).toBe(200);
|
||||
|
||||
const res = await fetch(`${h.baseUrl}/api/servers`);
|
||||
const body = (await res.json()) as {
|
||||
mcpServers: Record<string, { oauth?: Record<string, unknown> }>;
|
||||
};
|
||||
expect(body.mcpServers.srv?.oauth?.requestRefreshToken).toBeUndefined();
|
||||
});
|
||||
|
||||
it("accepts a fully-populated valid settings payload", async () => {
|
||||
const res = await postSettings({
|
||||
...base,
|
||||
|
|
@ -682,6 +723,7 @@ describe("server.ts supplemental coverage", () => {
|
|||
oauthClientId: "cid",
|
||||
oauthScopes: "a b",
|
||||
enterpriseManaged: true,
|
||||
oauthRequestRefreshToken: false,
|
||||
roots: [{ uri: "file:///x", name: "x" }],
|
||||
});
|
||||
expect(res.status).toBe(200);
|
||||
|
|
@ -824,6 +866,57 @@ describe("server.ts supplemental coverage", () => {
|
|||
}
|
||||
});
|
||||
|
||||
// #2068 — same all-or-nothing rule for the refresh-token opt-out: a
|
||||
// non-boolean drops the whole `oauth` node rather than reaching the
|
||||
// provider, where only an explicit `false` means anything.
|
||||
it("drops oauth whose requestRefreshToken is not a boolean", async () => {
|
||||
const h = await start({
|
||||
seedConfig: JSON.stringify({
|
||||
mcpServers: {
|
||||
srv: {
|
||||
type: "streamable-http",
|
||||
url: "https://x.test/mcp",
|
||||
oauth: { requestRefreshToken: "no" },
|
||||
},
|
||||
},
|
||||
}),
|
||||
});
|
||||
try {
|
||||
const res = await fetch(`${h.baseUrl}/api/servers`);
|
||||
const body = (await res.json()) as {
|
||||
mcpServers: Record<string, Record<string, unknown>>;
|
||||
};
|
||||
expect(body.mcpServers.srv).not.toHaveProperty("oauth");
|
||||
} finally {
|
||||
await stop(h);
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps a well-formed requestRefreshToken opt-out on read (#2068)", async () => {
|
||||
const h = await start({
|
||||
seedConfig: JSON.stringify({
|
||||
mcpServers: {
|
||||
srv: {
|
||||
type: "streamable-http",
|
||||
url: "https://x.test/mcp",
|
||||
oauth: { requestRefreshToken: false },
|
||||
},
|
||||
},
|
||||
}),
|
||||
});
|
||||
try {
|
||||
const res = await fetch(`${h.baseUrl}/api/servers`);
|
||||
const body = (await res.json()) as {
|
||||
mcpServers: Record<string, Record<string, unknown>>;
|
||||
};
|
||||
expect(body.mcpServers.srv?.oauth).toEqual({
|
||||
requestRefreshToken: false,
|
||||
});
|
||||
} finally {
|
||||
await stop(h);
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps a well-formed authorizationParams record on read", async () => {
|
||||
const h = await start({
|
||||
seedConfig: JSON.stringify({
|
||||
|
|
|
|||
Loading…
Reference in a new issue