Merge branch 'v2/main' into v2/fix/2096-stdio-env-cwd-revert
This commit is contained in:
commit
6af849e217
22 changed files with 1418 additions and 9 deletions
|
|
@ -692,6 +692,7 @@ import {
|
|||
useServers,
|
||||
} from "@inspector/core/react/useServers.js";
|
||||
import type {
|
||||
InspectorClientOptions,
|
||||
InspectorServerSettings,
|
||||
MessageEntry,
|
||||
ServerEntry,
|
||||
|
|
@ -743,6 +744,120 @@ const fetchLogInstances = (
|
|||
FetchLogModule as unknown as { __fetchLogInstances: EventTarget[] }
|
||||
).__fetchLogInstances;
|
||||
|
||||
// #2068 — the web connection seam for the refresh-token opt-out. The provider
|
||||
// and manager tests start from an already-built OAuth config and the runner test
|
||||
// covers only the CLI/TUI leg, so without this case deleting the
|
||||
// `requestRefreshToken` spread from App's `oauthFromServer` would leave the
|
||||
// checkbox persisted but inert with every other new test still green.
|
||||
describe("App wires the refresh-token opt-out into the client (#2068)", () => {
|
||||
// Fully-typed fixtures rather than a cast: `settings` is the whole
|
||||
// `InspectorServerSettings`, so a field added to that interface later is a
|
||||
// compile error here instead of a silently-absent value at runtime.
|
||||
const HTTP_SERVER: ServerEntry = {
|
||||
id: "A",
|
||||
name: "PlotRocket",
|
||||
config: { type: "streamable-http", url: "https://api.example.com/mcp" },
|
||||
connection: { status: "disconnected" },
|
||||
};
|
||||
|
||||
const BASE_SETTINGS: InspectorServerSettings = {
|
||||
headers: [],
|
||||
env: [],
|
||||
metadata: {},
|
||||
connectionTimeout: 0,
|
||||
requestTimeout: 0,
|
||||
taskTtl: 60000,
|
||||
maxFetchRequests: 1000,
|
||||
roots: [],
|
||||
};
|
||||
|
||||
function mockServersWith(overrides?: Partial<InspectorServerSettings>) {
|
||||
vi.mocked(useServers).mockReturnValue({
|
||||
servers: [
|
||||
{
|
||||
...HTTP_SERVER,
|
||||
...(overrides
|
||||
? { settings: { ...BASE_SETTINGS, ...overrides } }
|
||||
: {}),
|
||||
},
|
||||
],
|
||||
loading: false,
|
||||
error: undefined,
|
||||
refresh: vi.fn().mockResolvedValue(undefined),
|
||||
addServer: addServerSpy,
|
||||
updateServer: updateServerSpy,
|
||||
updateServerSettings: updateServerSettingsSpy,
|
||||
removeServer: vi.fn(),
|
||||
reorderServers: vi.fn(),
|
||||
importSource: vi.fn().mockResolvedValue({ servers: {} }),
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* The `oauth` option the mocked InspectorClient constructor was given. Read
|
||||
* through the constructor's own parameter type, so a rename of the option
|
||||
* fails to compile here rather than silently reading `undefined`.
|
||||
*/
|
||||
function constructedOAuth(): InspectorClientOptions["oauth"] {
|
||||
return vi.mocked(McpIndex.InspectorClient).mock.calls[0]?.[1]?.oauth;
|
||||
}
|
||||
|
||||
let previousUseServers: typeof useServers | undefined;
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
clientInstances.length = 0;
|
||||
previousUseServers = vi.mocked(useServers).getMockImplementation();
|
||||
vi.mocked(useInspectorClient).mockReturnValue(DEFAULT_USE_INSPECTOR_CLIENT);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
if (previousUseServers) {
|
||||
vi.mocked(useServers).mockImplementation(previousUseServers);
|
||||
}
|
||||
});
|
||||
|
||||
it("passes requestRefreshToken: false when the server opted out", async () => {
|
||||
const user = userEvent.setup();
|
||||
mockServersWith({ oauthRequestRefreshToken: false });
|
||||
renderWithMantine(<App />);
|
||||
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
expect(constructedOAuth()?.requestRefreshToken).toBe(false);
|
||||
});
|
||||
|
||||
// The default must not send the key at all — the provider's own default is
|
||||
// what declares the grant, and a stray `true` here would mask its removal.
|
||||
it("omits requestRefreshToken when the setting is on", async () => {
|
||||
const user = userEvent.setup();
|
||||
mockServersWith({ oauthScopes: "openid" });
|
||||
renderWithMantine(<App />);
|
||||
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
const oauth = constructedOAuth();
|
||||
expect(oauth).toBeDefined();
|
||||
expect(oauth).not.toHaveProperty("requestRefreshToken");
|
||||
});
|
||||
|
||||
// The opt-out is the only OAuth field set, so it must be enough on its own to
|
||||
// materialize the `oauth` option — otherwise it is dropped for any server
|
||||
// without credentials or scopes, which is the common case here.
|
||||
it("builds the oauth option from the opt-out alone", async () => {
|
||||
const user = userEvent.setup();
|
||||
mockServersWith({ oauthRequestRefreshToken: false });
|
||||
renderWithMantine(<App />);
|
||||
|
||||
await user.click(screen.getByText("connect"));
|
||||
await waitFor(() => expect(clientInstances).toHaveLength(1));
|
||||
|
||||
expect(constructedOAuth()).toEqual({ requestRefreshToken: false });
|
||||
});
|
||||
});
|
||||
|
||||
describe("App failed-connection card border (#1621)", () => {
|
||||
beforeEach(() => {
|
||||
clientInstances.length = 0;
|
||||
|
|
|
|||
|
|
@ -2624,7 +2624,8 @@ function App() {
|
|||
savedSettings.oauthScopes ||
|
||||
serverAuthorizationParams ||
|
||||
serverEndpointOverrides ||
|
||||
savedSettings.enterpriseManaged)
|
||||
savedSettings.enterpriseManaged ||
|
||||
savedSettings.oauthRequestRefreshToken === false)
|
||||
? {
|
||||
...(savedSettings.oauthClientId && {
|
||||
clientId: savedSettings.oauthClientId,
|
||||
|
|
@ -2642,6 +2643,11 @@ function App() {
|
|||
...(savedSettings.enterpriseManaged && {
|
||||
enterpriseManaged: true,
|
||||
}),
|
||||
// #2068: only the explicit opt-out is forwarded; omitting the key
|
||||
// leaves the provider's default (declare `refresh_token`) in place.
|
||||
...(savedSettings.oauthRequestRefreshToken === false && {
|
||||
requestRefreshToken: false,
|
||||
}),
|
||||
}
|
||||
: undefined;
|
||||
const oauth =
|
||||
|
|
|
|||
|
|
@ -706,9 +706,147 @@ describe("ServerSettingsForm", () => {
|
|||
authorizationUrl: "",
|
||||
tokenUrl: "",
|
||||
enterpriseManaged: false,
|
||||
requestRefreshToken: true,
|
||||
});
|
||||
});
|
||||
|
||||
// #2068 — the refresh-token opt-out. On by default; unchecking it is what
|
||||
// stops the SDK adding `offline_access` and then `prompt=consent`.
|
||||
it("renders Request refresh token checked by default", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={emptySettings}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByLabelText("Request refresh token")).toBeChecked();
|
||||
});
|
||||
|
||||
it("renders Request refresh token unchecked when the server opted out", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{ ...emptySettings, oauthRequestRefreshToken: false }}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByLabelText("Request refresh token")).not.toBeChecked();
|
||||
});
|
||||
|
||||
it("toggles Request refresh token through onOAuthChange", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onOAuthChange = vi.fn();
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
onOAuthChange={onOAuthChange}
|
||||
settings={emptySettings}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
await user.click(screen.getByLabelText("Request refresh token"));
|
||||
expect(onOAuthChange).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({ requestRefreshToken: false }),
|
||||
);
|
||||
});
|
||||
|
||||
it("warns when opting out while offline_access is still in Scopes", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{
|
||||
...emptySettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
oauthScopes: "openid offline_access",
|
||||
}}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.getByText("offline_access is still requested"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("does not warn when opted out and Scopes omits offline_access", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{
|
||||
...emptySettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
oauthScopes: "openid",
|
||||
}}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByText("offline_access is still requested"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// The scope alone is not a problem — with the grant declared, requesting
|
||||
// offline_access is exactly what the default configuration does.
|
||||
it("does not warn when offline_access is in Scopes but the grant is on", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{ ...emptySettings, oauthScopes: "openid offline_access" }}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByText("offline_access is still requested"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// A substring must not trip it — `offline_access_extra` is a different scope.
|
||||
it("matches offline_access as a whole scope token, not a substring", () => {
|
||||
renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{
|
||||
...emptySettings,
|
||||
oauthRequestRefreshToken: false,
|
||||
oauthScopes: "openid offline_access_extra",
|
||||
}}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByText("offline_access is still requested"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// #2068 — EMA wraps this provider and forwards its `clientMetadata`, so the
|
||||
// setting is not ignored outright; what it cannot change is the IdP
|
||||
// authorization leg's fixed `openid offline_access` scope. That leg is the
|
||||
// one the user signs in through, so the checkbox cannot affect EMA's consent
|
||||
// behavior and must say so.
|
||||
it("says the refresh-token setting is not applied under EMA", () => {
|
||||
const { rerender } = renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{ ...emptySettings, enterpriseManaged: true }}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.getByText(/which this setting cannot change/),
|
||||
).toBeInTheDocument();
|
||||
|
||||
rerender(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
settings={{ ...emptySettings, enterpriseManaged: false }}
|
||||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByText(/which this setting cannot change/),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// #2018 — custom authorization-request parameters.
|
||||
it("shows the empty hint for authorization parameters when none are set", () => {
|
||||
renderWithMantine(
|
||||
|
|
@ -1001,7 +1139,7 @@ describe("ServerSettingsForm", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("says the endpoint overrides are unused under enterprise-managed auth", () => {
|
||||
it("says the EMA-suppressed controls are unused under enterprise-managed auth", () => {
|
||||
const { rerender } = renderWithMantine(
|
||||
<ServerSettingsForm
|
||||
{...baseHandlers}
|
||||
|
|
@ -1009,11 +1147,15 @@ describe("ServerSettingsForm", () => {
|
|||
expandedSections={["oauth"]}
|
||||
/>,
|
||||
);
|
||||
// Three controls carry this notice: the two endpoint overrides (#1906) and
|
||||
// the refresh-token checkbox (#2068). The count is asserted rather than
|
||||
// merely "present" so a control that stops annotating itself — or a new one
|
||||
// that never starts — is caught here.
|
||||
expect(
|
||||
screen.getAllByText(
|
||||
/Not applied while Enterprise-managed authorization is on/,
|
||||
),
|
||||
).toHaveLength(2);
|
||||
).toHaveLength(3);
|
||||
|
||||
rerender(
|
||||
<ServerSettingsForm
|
||||
|
|
@ -1112,6 +1254,7 @@ describe("ServerSettingsForm", () => {
|
|||
authorizationUrl: "",
|
||||
tokenUrl: "",
|
||||
enterpriseManaged: true,
|
||||
requestRefreshToken: true,
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -1219,6 +1362,7 @@ describe("ServerSettingsForm", () => {
|
|||
authorizationUrl: "",
|
||||
tokenUrl: "",
|
||||
enterpriseManaged: false,
|
||||
requestRefreshToken: true,
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ import {
|
|||
} from "@mantine/core";
|
||||
import { ClearButton } from "../../elements/ClearButton/ClearButton";
|
||||
import { JsonObjectInput } from "../../elements/JsonObjectInput/JsonObjectInput";
|
||||
import type { ChangeEvent } from "react";
|
||||
import type { ProtocolEra } from "@modelcontextprotocol/client";
|
||||
import type {
|
||||
InspectorServerSettings,
|
||||
|
|
@ -518,6 +519,8 @@ export function ServerSettingsForm({
|
|||
tokenUrl: settings.oauthTokenUrl ?? "",
|
||||
enterpriseManaged: settings.enterpriseManaged ?? false,
|
||||
onInsufficientScope: settings.oauthOnInsufficientScope,
|
||||
// Unset means the default, on — only an explicit opt-out is stored.
|
||||
requestRefreshToken: settings.oauthRequestRefreshToken ?? true,
|
||||
};
|
||||
}
|
||||
|
||||
|
|
@ -528,6 +531,26 @@ export function ServerSettingsForm({
|
|||
? " Not applied while Enterprise-managed authorization is on: that flow authorizes against the enterprise IdP, a different authorization server."
|
||||
: "";
|
||||
|
||||
// #2068 — the opt-out only removes the SDK's *automatic* `offline_access`.
|
||||
// `prompt=consent` keys off the scope, not the grant, so an `offline_access`
|
||||
// the user typed here still forces the prompt the checkbox claims to avoid.
|
||||
// Flag that combination rather than letting it read as a fixed configuration.
|
||||
const scopesIncludeOfflineAccess = (settings.oauthScopes ?? "")
|
||||
.split(/\s+/)
|
||||
.includes("offline_access");
|
||||
const refreshTokenOptedOut = settings.oauthRequestRefreshToken === false;
|
||||
|
||||
// #2068 — annotated under EMA. Not because the provider is bypassed (EMA
|
||||
// wraps a BaseOAuthClientProvider and forwards its `clientMetadata`), but
|
||||
// because the IdP authorization leg — the one the user signs in through —
|
||||
// uses a fixed `openid offline_access` scope this setting cannot change. The
|
||||
// control therefore cannot affect EMA's consent behavior, which is the whole
|
||||
// reason to touch it. Say so rather than leave it looking effective, matching
|
||||
// how the endpoint overrides annotate themselves.
|
||||
const refreshTokenEmaNote = settings.enterpriseManaged
|
||||
? " Not applied while Enterprise-managed authorization is on: that flow signs in against the enterprise IdP using its own fixed scope, which this setting cannot change."
|
||||
: "";
|
||||
|
||||
const rejectedParamKeys = authorizationParams
|
||||
.map((p) => p.key.trim())
|
||||
.filter((key) => isReservedAuthorizationParam(key));
|
||||
|
|
@ -538,6 +561,15 @@ export function ServerSettingsForm({
|
|||
onOAuthChange({ ...currentOAuth(), authorizationParams: params });
|
||||
}
|
||||
|
||||
function handleRequestRefreshTokenChange(
|
||||
event: ChangeEvent<HTMLInputElement>,
|
||||
): void {
|
||||
onOAuthChange({
|
||||
...currentOAuth(),
|
||||
requestRefreshToken: event.currentTarget.checked,
|
||||
});
|
||||
}
|
||||
|
||||
function handleAddAuthorizationParam(): void {
|
||||
changeAuthorizationParams([...authorizationParams, { key: "", value: "" }]);
|
||||
}
|
||||
|
|
@ -879,6 +911,21 @@ export function ServerSettingsForm({
|
|||
) : null
|
||||
}
|
||||
/>
|
||||
<Checkbox
|
||||
label="Request refresh token"
|
||||
description={`Declares the refresh_token grant, so the authorization server may issue a refresh token and renew access tokens without a new sign-in. Uncheck it for authorization servers that reject the offline_access scope and forced consent prompt the grant brings with it. Applies on the next connect.${refreshTokenEmaNote}`}
|
||||
checked={settings.oauthRequestRefreshToken ?? true}
|
||||
onChange={handleRequestRefreshTokenChange}
|
||||
/>
|
||||
{refreshTokenOptedOut && scopesIncludeOfflineAccess ? (
|
||||
<Alert color="yellow" title="offline_access is still requested">
|
||||
The Scopes field above lists offline_access, and
|
||||
prompt=consent is added whenever that scope is requested —
|
||||
regardless of this setting. Remove it from Scopes as well, or
|
||||
the consent prompt this checkbox is meant to avoid still goes
|
||||
out.
|
||||
</Alert>
|
||||
) : null}
|
||||
<Stack gap="xs">
|
||||
<FieldLabel>Additional authorization parameters</FieldLabel>
|
||||
<FieldDescription>
|
||||
|
|
|
|||
|
|
@ -119,6 +119,43 @@ describe("ServerSettingsModal", () => {
|
|||
);
|
||||
});
|
||||
|
||||
// #2068 — only the opt-out is persisted; re-checking the box clears the field
|
||||
// rather than writing `true`, so a default-on server keeps a minimal entry.
|
||||
it("maps the refresh-token opt-out into settings, and back to unset", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onSettingsChange = vi.fn();
|
||||
const { rerender } = renderWithMantine(
|
||||
<ServerSettingsModal
|
||||
opened
|
||||
settings={initialSettings}
|
||||
serverType="streamable-http"
|
||||
isStdio={false}
|
||||
onClose={vi.fn()}
|
||||
onSettingsChange={onSettingsChange}
|
||||
/>,
|
||||
);
|
||||
await user.click(screen.getByRole("button", { name: /OAuth Settings/i }));
|
||||
await user.click(screen.getByLabelText("Request refresh token"));
|
||||
expect(onSettingsChange).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({ oauthRequestRefreshToken: false }),
|
||||
);
|
||||
|
||||
rerender(
|
||||
<ServerSettingsModal
|
||||
opened
|
||||
settings={{ ...initialSettings, oauthRequestRefreshToken: false }}
|
||||
serverType="streamable-http"
|
||||
isStdio={false}
|
||||
onClose={vi.fn()}
|
||||
onSettingsChange={onSettingsChange}
|
||||
/>,
|
||||
);
|
||||
await user.click(screen.getByLabelText("Request refresh token"));
|
||||
expect(onSettingsChange).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({ oauthRequestRefreshToken: undefined }),
|
||||
);
|
||||
});
|
||||
|
||||
it("maps the selected protocol era into settings (#1626)", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onSettingsChange = vi.fn();
|
||||
|
|
|
|||
|
|
@ -187,6 +187,10 @@ export function ServerSettingsModal({
|
|||
// the SDK's `reauthorize` behavior without writing a spurious field.
|
||||
oauthOnInsufficientScope:
|
||||
oauth.onInsufficientScope === "throw" ? "throw" : undefined,
|
||||
// #2068: persist only the non-default (off). `undefined` means on, so a
|
||||
// server that never touched the switch writes no field at all.
|
||||
oauthRequestRefreshToken:
|
||||
oauth.requestRefreshToken === false ? false : undefined,
|
||||
});
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -1140,6 +1140,165 @@ 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.
|
||||
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",
|
||||
);
|
||||
});
|
||||
|
||||
// With the grant declared there is nothing to heal, so the ordinary leg
|
||||
// keeps its previous behavior rather than persisting an unconfirmed
|
||||
// request.
|
||||
it("does not persist a request the AS never confirmed while the grant is on", async () => {
|
||||
const storage = await completeWith(undefined, undefined);
|
||||
expect(storage.saveScope).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
// #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({
|
||||
|
|
@ -1687,6 +1846,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();
|
||||
|
|
|
|||
|
|
@ -1199,6 +1199,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({
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ import type {
|
|||
import type { OAuthStorage, SaveClientInformationOptions } from "./storage.js";
|
||||
import { generateOAuthState } from "./utils.js";
|
||||
import { applyAuthorizationParams } from "./authorizationParams.js";
|
||||
import { scopeForDeclinedRefreshGrant } from "./scopes.js";
|
||||
|
||||
/**
|
||||
* Redirect URL provider. Returns the redirect URL for OAuth flows.
|
||||
|
|
@ -105,6 +106,36 @@ export type OAuthProviderConfig = {
|
|||
* unaffected.
|
||||
*/
|
||||
authorizationParams?: Record<string, string>;
|
||||
/**
|
||||
* Whether to declare the `refresh_token` grant in the registered client
|
||||
* metadata (#2068). Defaults to `true`; set `false` to register (and
|
||||
* authorize) as an authorization-code-only client.
|
||||
*
|
||||
* This is not merely cosmetic. The SDK's `determineScope()` appends
|
||||
* `offline_access` to the effective scope whenever the authorization server
|
||||
* advertises it **and** the client metadata declares `refresh_token`, and
|
||||
* `startAuthorization()` then appends `prompt=consent` whenever
|
||||
* `offline_access` is in scope. Against Microsoft Entra ID, that forced
|
||||
* consent prompt routes a non-admin user into the admin-consent workflow and
|
||||
* fails with `AADSTS90094` even after a tenant admin has consented.
|
||||
*
|
||||
* It removes that *automatic* augmentation only. `startAuthorization()` reads
|
||||
* the scope, never `grant_types`, so an `offline_access` the caller passed in
|
||||
* `scope` — or one the resource advertises when `scope` is unset, which
|
||||
* `determineScope` falls back to — still produces the prompt. This is not on
|
||||
* its own a guarantee that `prompt=consent` is absent.
|
||||
*/
|
||||
requestRefreshToken?: boolean;
|
||||
/**
|
||||
* The scope configured for this server (mcp.json / Server Settings), as
|
||||
* distinct from the scope carried in OAuth storage after a previous grant.
|
||||
*
|
||||
* Used only to decide whether an `offline_access` in the effective scope was
|
||||
* *asked for* or merely inherited — see {@link scopeForDeclinedRefreshGrant}.
|
||||
* An explicitly configured one is honored; an inherited one is dropped from
|
||||
* the request when {@link requestRefreshToken} is false. (#2068)
|
||||
*/
|
||||
configuredScope?: string;
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
@ -126,6 +157,10 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
|||
public clientMetadataUrl?: string;
|
||||
/** Custom authorization-request parameters (#2018). Authorize URL only. */
|
||||
protected authorizationParams?: Record<string, string>;
|
||||
/** Declare the `refresh_token` grant in {@link clientMetadata} (#2068). */
|
||||
protected requestRefreshToken: boolean;
|
||||
/** Server-configured scope, for the #2068 `offline_access` distinction. */
|
||||
protected configuredScope?: string;
|
||||
|
||||
constructor(serverUrl: string, oauthConfig: OAuthProviderConfig) {
|
||||
this.serverUrl = serverUrl;
|
||||
|
|
@ -134,6 +169,8 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
|||
this.navigation = oauthConfig.navigation;
|
||||
this.clientMetadataUrl = oauthConfig.clientMetadataUrl;
|
||||
this.authorizationParams = oauthConfig.authorizationParams;
|
||||
this.requestRefreshToken = oauthConfig.requestRefreshToken ?? true;
|
||||
this.configuredScope = oauthConfig.configuredScope;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -171,6 +208,21 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
|||
}
|
||||
|
||||
get scope(): string | undefined {
|
||||
// #2068: filter at the point of *request*, not in storage. Covers the two
|
||||
// readers that go through this getter — `clientMetadata.scope` below, and
|
||||
// the `scope:` argument `OAuthManager.authenticate` hands to the SDK.
|
||||
//
|
||||
// It is NOT the only request path: mid-session `insufficient_scope` step-up
|
||||
// builds its union from raw storage and passes it to `mcpAuth` directly,
|
||||
// never reading this getter. `OAuthManager.handleAuthChallenge` applies the
|
||||
// same filter there. Both call `scopeForDeclinedRefreshGrant`, so the rule
|
||||
// lives in one place even though it has to be applied twice.
|
||||
if (!this.requestRefreshToken) {
|
||||
return scopeForDeclinedRefreshGrant(
|
||||
this.cachedScope,
|
||||
this.configuredScope,
|
||||
);
|
||||
}
|
||||
return this.cachedScope;
|
||||
}
|
||||
|
||||
|
|
@ -186,7 +238,14 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
|||
const metadata: OAuthClientMetadata = {
|
||||
redirect_uris: this.redirect_uris,
|
||||
token_endpoint_auth_method: "none",
|
||||
grant_types: ["authorization_code", "refresh_token"],
|
||||
// #2068: `refresh_token` is declared by default, and dropped when the
|
||||
// server's "Request refresh token" setting is off — which stops the SDK
|
||||
// *adding* `offline_access` (and so the `prompt=consent` it forces). It
|
||||
// does not remove an `offline_access` that reaches the scope another way.
|
||||
// See `OAuthProviderConfig.requestRefreshToken`.
|
||||
grant_types: this.requestRefreshToken
|
||||
? ["authorization_code", "refresh_token"]
|
||||
: ["authorization_code"],
|
||||
response_types: ["code"],
|
||||
client_name: "MCP Inspector",
|
||||
client_uri: "https://github.com/modelcontextprotocol/inspector",
|
||||
|
|
|
|||
|
|
@ -18,6 +18,96 @@ export const computeScopeUnion = sdkComputeScopeUnion;
|
|||
*/
|
||||
export const isStrictScopeSuperset = sdkIsStrictScopeSuperset;
|
||||
|
||||
/** The scope token whose presence makes the SDK force `prompt=consent`. */
|
||||
export const OFFLINE_ACCESS_SCOPE = "offline_access";
|
||||
|
||||
/**
|
||||
* The scope to *request* when the `refresh_token` grant has been declined
|
||||
* (#2068).
|
||||
*
|
||||
* Declining the grant stops the SDK adding `offline_access`, but it cannot
|
||||
* remove one that is already in the scope — and the Inspector's own storage is
|
||||
* the way it gets there. A successful authorization persists the scope the AS
|
||||
* granted (`resolvePersistedScopeAfterGrant`), which for a default-on client
|
||||
* includes `offline_access`; the provider reloads that on the next connect and
|
||||
* hands it back to the SDK, which sees the token in scope and appends
|
||||
* `prompt=consent` again. So a server that authorized once before the opt-out
|
||||
* would keep hitting AADSTS90094 with the box unchecked — the exact failure the
|
||||
* setting exists to prevent.
|
||||
*
|
||||
* Dropping it from the *request* (not from storage) is what breaks that loop:
|
||||
* the next grant comes back without `offline_access` and the persisted scope
|
||||
* self-heals, while re-checking the box restores the old behavior from the
|
||||
* user's own configuration rather than from a value we destroyed.
|
||||
*
|
||||
* An `offline_access` the user explicitly configured is honored — they asked
|
||||
* for it, and the web form warns that it keeps the consent prompt alive. That
|
||||
* means *ensuring* it is requested, not merely declining to strip it: the
|
||||
* persisted scope can predate the configuration change (`createOAuthProvider`
|
||||
* preserves a stored scope rather than reseeding from current settings), and
|
||||
* silently omitting a scope the user typed would make both this promise and the
|
||||
* form's warning false.
|
||||
*
|
||||
* Only `offline_access` is added, never the rest of the configured scope.
|
||||
* Widening the request with unrelated configured tokens is outside what this
|
||||
* setting governs and could push an otherwise-satisfied connection into a
|
||||
* step-up authorization.
|
||||
*
|
||||
* `requiredScopes` carries scopes a `403 insufficient_scope` challenge demands
|
||||
* (SEP-2350). They are honored exactly like a configured one: the server is
|
||||
* asking for them, and stripping a scope the challenge requires would loop —
|
||||
* re-authorize, get the same challenge back, strip it again. The step-up path
|
||||
* needs this because it does not read the scope through the provider at all:
|
||||
* it unions the *raw* stored scope with the challenge and hands that to the
|
||||
* SDK directly, so an inherited `offline_access` would otherwise reappear
|
||||
* there with the setting off.
|
||||
*/
|
||||
export function scopeForDeclinedRefreshGrant(
|
||||
effectiveScope: string | undefined,
|
||||
configuredScope: string | undefined,
|
||||
requiredScopes?: readonly string[],
|
||||
): string | undefined {
|
||||
if (!effectiveScope) return effectiveScope;
|
||||
const configured = configuredScope?.trim().split(/\s+/) ?? [];
|
||||
const required = requiredScopes ?? [];
|
||||
if (
|
||||
configured.includes(OFFLINE_ACCESS_SCOPE) ||
|
||||
required.includes(OFFLINE_ACCESS_SCOPE)
|
||||
) {
|
||||
const tokens = effectiveScope
|
||||
.trim()
|
||||
.split(/\s+/)
|
||||
.filter((token) => token !== "");
|
||||
if (!tokens.includes(OFFLINE_ACCESS_SCOPE)) {
|
||||
tokens.push(OFFLINE_ACCESS_SCOPE);
|
||||
}
|
||||
return tokens.join(" ");
|
||||
}
|
||||
|
||||
const kept = effectiveScope
|
||||
.trim()
|
||||
.split(/\s+/)
|
||||
.filter((token) => token !== "" && token !== OFFLINE_ACCESS_SCOPE);
|
||||
if (kept.length > 0) return kept.join(" ");
|
||||
|
||||
// Filtering removed everything (the persisted scope was `offline_access`
|
||||
// alone). Returning `undefined` here would be read as "nothing stored" by
|
||||
// `OAuthManager.createOAuthProvider`, whose seeding branch then *writes* the
|
||||
// configured scope to storage — turning this request-only filter into a
|
||||
// silent overwrite of the persisted value, so re-enabling the grant could no
|
||||
// longer restore it. Hand back the configured scope instead: it is the right
|
||||
// thing to request, and it leaves storage alone.
|
||||
// Cannot itself contain `offline_access`: that case returned above.
|
||||
const fallback = configuredScope?.trim();
|
||||
if (fallback) return fallback;
|
||||
|
||||
// Nothing configured either, so there is no value to preserve and the seeding
|
||||
// branch is inert (it requires a configured scope). `undefined` rather than
|
||||
// `""` so the SDK falls back to its own resolution instead of requesting an
|
||||
// empty scope.
|
||||
return undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Scope to persist after a successful token grant (RFC 6749 §5.1).
|
||||
* When the AS returns `scope`, it is the authoritative full grant.
|
||||
|
|
|
|||
|
|
@ -87,7 +87,8 @@ export function buildRunnerClientAuthOptions(
|
|||
savedSettings.oauthScopes ||
|
||||
serverAuthorizationParams ||
|
||||
serverEndpointOverrides ||
|
||||
savedSettings.enterpriseManaged)
|
||||
savedSettings.enterpriseManaged ||
|
||||
savedSettings.oauthRequestRefreshToken === false)
|
||||
? {
|
||||
...(savedSettings.oauthClientId && {
|
||||
clientId: savedSettings.oauthClientId,
|
||||
|
|
@ -105,6 +106,11 @@ export function buildRunnerClientAuthOptions(
|
|||
...(savedSettings.enterpriseManaged && {
|
||||
enterpriseManaged: true,
|
||||
}),
|
||||
// #2068: only the explicit opt-out is forwarded; omitting the key
|
||||
// leaves the provider's default (declare `refresh_token`) in place.
|
||||
...(savedSettings.oauthRequestRefreshToken === false && {
|
||||
requestRefreshToken: false,
|
||||
}),
|
||||
}
|
||||
: undefined;
|
||||
|
||||
|
|
|
|||
|
|
@ -6340,6 +6340,8 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
/** Endpoint overrides applied to the discovered AS metadata (#1906). */
|
||||
authorizationUrl?: string;
|
||||
tokenUrl?: string;
|
||||
/** Declare the `refresh_token` grant in client metadata (#2068). */
|
||||
requestRefreshToken?: boolean;
|
||||
}): void {
|
||||
if (!this.oauthManager) {
|
||||
throw new Error(
|
||||
|
|
|
|||
|
|
@ -48,6 +48,7 @@ import {
|
|||
isStrictScopeSuperset,
|
||||
resolveEffectiveGrantedScope,
|
||||
resolvePersistedScopeAfterGrant,
|
||||
scopeForDeclinedRefreshGrant,
|
||||
} from "../auth/scopes.js";
|
||||
import { stepUpInsufficientScopeMessage } from "../auth/oauthUx.js";
|
||||
import type {
|
||||
|
|
@ -105,6 +106,8 @@ export class OAuthManager {
|
|||
authorizationParams?: Record<string, string>;
|
||||
authorizationUrl?: string;
|
||||
tokenUrl?: string;
|
||||
/** Declare the `refresh_token` grant in client metadata (#2068). */
|
||||
requestRefreshToken?: boolean;
|
||||
}): void {
|
||||
this.oauthConfig = {
|
||||
...this.oauthConfig,
|
||||
|
|
@ -176,6 +179,13 @@ export class OAuthManager {
|
|||
// read from mcp.json, and nothing in the flow needs to persist or union
|
||||
// them.
|
||||
authorizationParams: this.oauthConfig.authorizationParams,
|
||||
// #2068: per-server opt-out of the `refresh_token` grant. Like
|
||||
// `authorizationParams` this is pure config off mcp.json, so it lives on
|
||||
// the provider rather than in OAuth storage.
|
||||
requestRefreshToken: this.oauthConfig.requestRefreshToken,
|
||||
// #2068: lets the provider tell an `offline_access` the user asked for
|
||||
// from one inherited from a previous grant's persisted scope.
|
||||
configuredScope: this.oauthConfig.scope,
|
||||
});
|
||||
|
||||
provider.setEventTarget(this.params.getEventTarget());
|
||||
|
|
@ -305,9 +315,12 @@ export class OAuthManager {
|
|||
resourceMetadataUrl,
|
||||
});
|
||||
|
||||
// Read once: the getter filters (#2068), so this is what was actually
|
||||
// requested and what the callback leg has to persist.
|
||||
const requestedScope = provider.scope;
|
||||
const result = await mcpAuth(provider, {
|
||||
serverUrl,
|
||||
scope: provider.scope,
|
||||
scope: requestedScope,
|
||||
fetchFn: this.params.effectiveAuthFetch,
|
||||
resourceMetadataUrl,
|
||||
});
|
||||
|
|
@ -318,6 +331,21 @@ export class OAuthManager {
|
|||
);
|
||||
}
|
||||
|
||||
// #2068 — record the filtered request so `completeOAuthFlow` can persist
|
||||
// it. `resolvePersistedScopeAfterGrant` falls back to the requested scope
|
||||
// when the token response omits `scope` (RFC 6749 §5.1: omitted means the
|
||||
// grant matched the request), and without this that fallback is `undefined`
|
||||
// on the ordinary leg — only step-up ever set it. The old
|
||||
// `offline_access` would then stay in storage, so the scope never
|
||||
// self-heals and later satisfaction checks still read it as granted.
|
||||
//
|
||||
// Only when the filter is in effect. With the grant declared there is
|
||||
// nothing to heal, and persisting a request the AS did not confirm would be
|
||||
// a behavior change beyond this setting.
|
||||
if (this.oauthConfig.requestRefreshToken === false && requestedScope) {
|
||||
this.pendingAuthorizationScope = requestedScope;
|
||||
}
|
||||
|
||||
const capturedUrl = provider.getCapturedAuthUrl();
|
||||
if (!capturedUrl) {
|
||||
throw new Error("Failed to capture authorization URL");
|
||||
|
|
@ -774,13 +802,37 @@ export class OAuthManager {
|
|||
resourceMetadataUrl,
|
||||
});
|
||||
|
||||
const scopeForAuth =
|
||||
// #2068 — the step-up union is built from *raw* storage
|
||||
// (`enrichChallengeWithScopes`) and handed to the SDK directly, so it never
|
||||
// passes through the provider's filtering `scope` getter. Without this an
|
||||
// `offline_access` inherited from an earlier grant reappears here and the
|
||||
// SDK re-adds `prompt=consent` even with the setting off — the same failure
|
||||
// the option exists to prevent, on the one path that bypasses it. A scope
|
||||
// the challenge itself requires is preserved, since stripping that would
|
||||
// just re-earn the same challenge.
|
||||
const requestedScopeForAuth =
|
||||
enriched.reason === "insufficient_scope"
|
||||
? enriched.authorizationScopes?.join(" ")
|
||||
: this.oauthConfig.scope?.trim() ||
|
||||
(enriched.requiredScopes?.length
|
||||
? enriched.requiredScopes.join(" ")
|
||||
: undefined);
|
||||
const scopeForAuth =
|
||||
this.oauthConfig.requestRefreshToken === false
|
||||
? scopeForDeclinedRefreshGrant(
|
||||
requestedScopeForAuth,
|
||||
this.oauthConfig.scope,
|
||||
// The RAW challenge scopes, not `enriched.requiredScopes`:
|
||||
// `enrichChallengeWithScopes` narrows that to the *missing* subset,
|
||||
// so a challenge requiring `offline_access tools:write` against a
|
||||
// stored scope that already has `offline_access` arrives here as
|
||||
// just `["tools:write"]`. Passing the narrowed list would read the
|
||||
// server's explicit requirement as inherited and strip it — then
|
||||
// re-authorization earns the same challenge forever, which is the
|
||||
// loop this argument exists to prevent.
|
||||
challenge.requiredScopes,
|
||||
)
|
||||
: requestedScopeForAuth;
|
||||
|
||||
provider.setSuppressAuthorizationNavigation(true);
|
||||
let result: Awaited<ReturnType<typeof mcpAuth>>;
|
||||
|
|
|
|||
|
|
@ -1391,6 +1391,7 @@ export function createRemoteApp(
|
|||
authorizationUrl?: string;
|
||||
tokenUrl?: string;
|
||||
enterpriseManaged?: boolean;
|
||||
requestRefreshToken?: boolean;
|
||||
} => {
|
||||
if (v === null || typeof v !== "object" || Array.isArray(v)) return false;
|
||||
const o = v as Record<string, unknown>;
|
||||
|
|
@ -1418,6 +1419,14 @@ export function createRemoteApp(
|
|||
) {
|
||||
return false;
|
||||
}
|
||||
// #2068 — shape only; the read side keeps just an explicit `false`, so a
|
||||
// stray `true` from a hand-edited file reads back as the default anyway.
|
||||
if (
|
||||
o.requestRefreshToken !== undefined &&
|
||||
typeof o.requestRefreshToken !== "boolean"
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
};
|
||||
// A roots array: each entry must have a string `uri` and, when present, a
|
||||
|
|
@ -1531,7 +1540,7 @@ export function createRemoteApp(
|
|||
if ("oauth" in valObj && !isOauthObject(valObj.oauth)) {
|
||||
logWarn(
|
||||
{ route: "/api/servers", id, droppedKey: "oauth" },
|
||||
"Dropping malformed `oauth` field — expected `{ clientId?, clientSecret?, scopes?, authorizationParams?, authorizationUrl?, tokenUrl?, enterpriseManaged?, onInsufficientScope? }`.",
|
||||
"Dropping malformed `oauth` field — expected `{ clientId?, clientSecret?, scopes?, authorizationParams?, authorizationUrl?, tokenUrl?, enterpriseManaged?, onInsufficientScope?, requestRefreshToken? }`.",
|
||||
);
|
||||
delete valObj.oauth;
|
||||
}
|
||||
|
|
@ -1834,6 +1843,15 @@ export function createRemoteApp(
|
|||
error: "settings.enterpriseManaged must be a boolean",
|
||||
};
|
||||
}
|
||||
if (
|
||||
obj.oauthRequestRefreshToken !== undefined &&
|
||||
typeof obj.oauthRequestRefreshToken !== "boolean"
|
||||
) {
|
||||
return {
|
||||
ok: false,
|
||||
error: "settings.oauthRequestRefreshToken must be a boolean",
|
||||
};
|
||||
}
|
||||
if (
|
||||
obj.oauthOnInsufficientScope !== undefined &&
|
||||
obj.oauthOnInsufficientScope !== "reauthorize" &&
|
||||
|
|
@ -1961,6 +1979,11 @@ export function createRemoteApp(
|
|||
if (obj.enterpriseManaged === true) {
|
||||
value.enterpriseManaged = true;
|
||||
}
|
||||
// #2068: the default is on, so only the explicit opt-out is carried; a
|
||||
// `true` on the wire reads back as unset, which means the same thing.
|
||||
if (obj.oauthRequestRefreshToken === false) {
|
||||
value.oauthRequestRefreshToken = false;
|
||||
}
|
||||
if (
|
||||
obj.oauthOnInsufficientScope === "reauthorize" ||
|
||||
obj.oauthOnInsufficientScope === "throw"
|
||||
|
|
|
|||
|
|
@ -611,6 +611,12 @@ export function storedFieldsToInspectorSettings(
|
|||
if (stored.oauth?.enterpriseManaged === true) {
|
||||
settings.enterpriseManaged = true;
|
||||
}
|
||||
// Inverted from the flags above: the default is *on*, so only an explicit
|
||||
// `false` is carried through. Anything else on disk (absent, or a
|
||||
// hand-edited non-boolean) reads back as unset, i.e. the default. (#2068)
|
||||
if (stored.oauth?.requestRefreshToken === false) {
|
||||
settings.oauthRequestRefreshToken = false;
|
||||
}
|
||||
// Mirror the stdio working directory for the form. Like the OAuth fields, an
|
||||
// empty string coerces to absent so the form's "(inherit)" placeholder shows.
|
||||
if (stored.cwd) settings.cwd = stored.cwd;
|
||||
|
|
@ -738,6 +744,12 @@ export function inspectorSettingsToStoredFields(
|
|||
if (settings.enterpriseManaged === true) {
|
||||
oauthFields.enterpriseManaged = true;
|
||||
}
|
||||
// Only the non-default (off) is written; an absent field reads back as the
|
||||
// default (on), so writing `true` would inject the key into files that never
|
||||
// set it and break byte-stable round-trips. (#2068)
|
||||
if (settings.oauthRequestRefreshToken === false) {
|
||||
oauthFields.requestRefreshToken = false;
|
||||
}
|
||||
if (Object.keys(oauthFields).length > 0) {
|
||||
out.oauth = oauthFields;
|
||||
}
|
||||
|
|
|
|||
Some files were not shown because too many files have changed in this diff Show more
Loading…
Reference in a new issue