Merge pull request #2022 from modelcontextprotocol/v2/feat/2018-custom-authorization-params
feat: per-server custom OAuth authorization-request parameters
This commit is contained in:
commit
66fbf98cb8
25 changed files with 1405 additions and 18 deletions
|
|
@ -38,7 +38,14 @@ v2/main/
|
||||||
│ │ # issuerBinding.ts SEP-2352 callback-leg failure
|
│ │ # issuerBinding.ts SEP-2352 callback-leg failure
|
||||||
│ │ # classification — separates a recoverable
|
│ │ # classification — separates a recoverable
|
||||||
│ │ # "lost authorization state" from a genuine
|
│ │ # "lost authorization state" from a genuine
|
||||||
│ │ # cross-AS issuer mismatch — #1808)
|
│ │ # cross-AS issuer mismatch — #1808;
|
||||||
|
│ │ # authorizationParams.ts per-server custom
|
||||||
|
│ │ # authorization-request parameters — the
|
||||||
|
│ │ # reserved-key list plus the merge applied in
|
||||||
|
│ │ # providers.redirectToAuthorization, the only
|
||||||
|
│ │ # seam that sees the SDK-built authorize URL;
|
||||||
|
│ │ # authorization request only, never the token
|
||||||
|
│ │ # request — #2018)
|
||||||
│ │ ├── browser/ # Browser-side OAuth (sessionStorage, BrowserNavigation)
|
│ │ ├── browser/ # Browser-side OAuth (sessionStorage, BrowserNavigation)
|
||||||
│ │ ├── node/ # Node-side OAuth (NodeOAuthStorage, OAuthCallbackServer,
|
│ │ ├── node/ # Node-side OAuth (NodeOAuthStorage, OAuthCallbackServer,
|
||||||
│ │ │ # runner-interactive-oauth loopback callback flow)
|
│ │ │ # runner-interactive-oauth loopback callback flow)
|
||||||
|
|
|
||||||
|
|
@ -63,6 +63,7 @@ import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedReq
|
||||||
import { ResourceSubscriptionsState } from "@inspector/core/mcp/state/resourceSubscriptionsState.js";
|
import { ResourceSubscriptionsState } from "@inspector/core/mcp/state/resourceSubscriptionsState.js";
|
||||||
import {
|
import {
|
||||||
cleanRoots,
|
cleanRoots,
|
||||||
|
oauthAuthorizationParamsFromSettings,
|
||||||
serializeMcpConfig,
|
serializeMcpConfig,
|
||||||
} from "@inspector/core/mcp/serverList.js";
|
} from "@inspector/core/mcp/serverList.js";
|
||||||
import type { ClientConfig } from "@inspector/core/client/types.js";
|
import type { ClientConfig } from "@inspector/core/client/types.js";
|
||||||
|
|
@ -2301,11 +2302,15 @@ function App() {
|
||||||
.map((m) => [m.key, m.value]),
|
.map((m) => [m.key, m.value]),
|
||||||
)
|
)
|
||||||
: undefined;
|
: undefined;
|
||||||
|
const serverAuthorizationParams = savedSettings
|
||||||
|
? oauthAuthorizationParamsFromSettings(savedSettings)
|
||||||
|
: undefined;
|
||||||
const oauthFromServer =
|
const oauthFromServer =
|
||||||
savedSettings &&
|
savedSettings &&
|
||||||
(savedSettings.oauthClientId ||
|
(savedSettings.oauthClientId ||
|
||||||
savedSettings.oauthClientSecret ||
|
savedSettings.oauthClientSecret ||
|
||||||
savedSettings.oauthScopes ||
|
savedSettings.oauthScopes ||
|
||||||
|
serverAuthorizationParams ||
|
||||||
savedSettings.enterpriseManaged)
|
savedSettings.enterpriseManaged)
|
||||||
? {
|
? {
|
||||||
...(savedSettings.oauthClientId && {
|
...(savedSettings.oauthClientId && {
|
||||||
|
|
@ -2317,6 +2322,9 @@ function App() {
|
||||||
...(savedSettings.oauthScopes && {
|
...(savedSettings.oauthScopes && {
|
||||||
scope: savedSettings.oauthScopes,
|
scope: savedSettings.oauthScopes,
|
||||||
}),
|
}),
|
||||||
|
...(serverAuthorizationParams && {
|
||||||
|
authorizationParams: serverAuthorizationParams,
|
||||||
|
}),
|
||||||
...(savedSettings.enterpriseManaged && {
|
...(savedSettings.enterpriseManaged && {
|
||||||
enterpriseManaged: true,
|
enterpriseManaged: true,
|
||||||
}),
|
}),
|
||||||
|
|
|
||||||
|
|
@ -700,10 +700,272 @@ describe("ServerSettingsForm", () => {
|
||||||
clientId: "a",
|
clientId: "a",
|
||||||
clientSecret: "",
|
clientSecret: "",
|
||||||
scopes: "",
|
scopes: "",
|
||||||
|
authorizationParams: [],
|
||||||
enterpriseManaged: false,
|
enterpriseManaged: false,
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2018 — custom authorization-request parameters.
|
||||||
|
it("shows the empty hint for authorization parameters when none are set", () => {
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
settings={emptySettings}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
screen.getByText("Additional authorization parameters"),
|
||||||
|
).toBeInTheDocument();
|
||||||
|
expect(
|
||||||
|
screen.getByText("No additional parameters configured"),
|
||||||
|
).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("adds an authorization-parameter row through onOAuthChange", async () => {
|
||||||
|
const user = userEvent.setup();
|
||||||
|
const onOAuthChange = vi.fn();
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
onOAuthChange={onOAuthChange}
|
||||||
|
settings={emptySettings}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
await user.click(screen.getByRole("button", { name: "+ Add Parameter" }));
|
||||||
|
expect(onOAuthChange).toHaveBeenCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
authorizationParams: [{ key: "", value: "" }],
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("edits and removes an authorization-parameter row", async () => {
|
||||||
|
const user = userEvent.setup();
|
||||||
|
const onOAuthChange = vi.fn();
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
onOAuthChange={onOAuthChange}
|
||||||
|
settings={{
|
||||||
|
...emptySettings,
|
||||||
|
oauthAuthorizationParams: [{ key: "kc_idp_hint", value: "corp" }],
|
||||||
|
}}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
const valueInput = screen.getByDisplayValue("corp");
|
||||||
|
await user.type(valueInput, "x");
|
||||||
|
expect(onOAuthChange).toHaveBeenLastCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
authorizationParams: [{ key: "kc_idp_hint", value: "corpx" }],
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
|
||||||
|
onOAuthChange.mockClear();
|
||||||
|
const removeButton = screen
|
||||||
|
.getAllByRole("button")
|
||||||
|
.find((b) => b.textContent === "X");
|
||||||
|
await user.click(removeButton as HTMLElement);
|
||||||
|
expect(onOAuthChange).toHaveBeenLastCalledWith(
|
||||||
|
expect.objectContaining({ authorizationParams: [] }),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("clears an authorization-parameter key via its Clear button", async () => {
|
||||||
|
const user = userEvent.setup();
|
||||||
|
const onOAuthChange = vi.fn();
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
onOAuthChange={onOAuthChange}
|
||||||
|
settings={{
|
||||||
|
...emptySettings,
|
||||||
|
oauthAuthorizationParams: [{ key: "kc_idp_hint", value: "corp" }],
|
||||||
|
}}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
await user.click(clearButtonFor(screen.getByDisplayValue("kc_idp_hint")));
|
||||||
|
expect(onOAuthChange).toHaveBeenLastCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
authorizationParams: [{ key: "", value: "corp" }],
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
|
||||||
|
onOAuthChange.mockClear();
|
||||||
|
await user.click(clearButtonFor(screen.getByDisplayValue("corp")));
|
||||||
|
expect(onOAuthChange).toHaveBeenLastCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
authorizationParams: [{ key: "kc_idp_hint", value: "" }],
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("rejects a reserved authorization-parameter key inline and with a warning", () => {
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
settings={{
|
||||||
|
...emptySettings,
|
||||||
|
oauthAuthorizationParams: [{ key: "state", value: "spoofed" }],
|
||||||
|
}}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
screen.getByText(
|
||||||
|
'"state" is set by the authorization flow and cannot be overridden.',
|
||||||
|
),
|
||||||
|
).toBeInTheDocument();
|
||||||
|
expect(screen.getByText("Reserved parameters ignored")).toBeInTheDocument();
|
||||||
|
expect(screen.getByDisplayValue("state")).toHaveAttribute(
|
||||||
|
"aria-invalid",
|
||||||
|
"true",
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// #2018 — the section heading and the placeholders are not programmatically
|
||||||
|
// associated with these inputs, so each control carries its own aria-label and
|
||||||
|
// the reserved-key reason is the input's `error` string (which Mantine wires
|
||||||
|
// to the input via aria-describedby) rather than free-standing text.
|
||||||
|
it("gives each authorization-parameter control an accessible name", () => {
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
settings={{
|
||||||
|
...emptySettings,
|
||||||
|
oauthAuthorizationParams: [{ key: "kc_idp_hint", value: "corp" }],
|
||||||
|
}}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
screen.getByRole("textbox", {
|
||||||
|
name: "Authorization parameter name, kc_idp_hint",
|
||||||
|
}),
|
||||||
|
).toHaveValue("kc_idp_hint");
|
||||||
|
expect(
|
||||||
|
screen.getByRole("textbox", {
|
||||||
|
name: "Authorization parameter value, kc_idp_hint",
|
||||||
|
}),
|
||||||
|
).toHaveValue("corp");
|
||||||
|
expect(
|
||||||
|
screen.getByRole("button", {
|
||||||
|
name: "Remove authorization parameter kc_idp_hint",
|
||||||
|
}),
|
||||||
|
).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("falls back to a row number in the accessible name when the key is blank", () => {
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
settings={{
|
||||||
|
...emptySettings,
|
||||||
|
oauthAuthorizationParams: [{ key: "", value: "" }],
|
||||||
|
}}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
screen.getByRole("textbox", {
|
||||||
|
name: "Authorization parameter name, row 1",
|
||||||
|
}),
|
||||||
|
).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
// The reason must reach assistive tech through the input, not just as text on
|
||||||
|
// the page: Mantine emits the `error` string with an id the input references.
|
||||||
|
it("associates the reserved-key reason with the key input", () => {
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
settings={{
|
||||||
|
...emptySettings,
|
||||||
|
oauthAuthorizationParams: [{ key: "state", value: "spoofed" }],
|
||||||
|
}}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
const keyInput = screen.getByRole("textbox", {
|
||||||
|
name: "Authorization parameter name, state",
|
||||||
|
});
|
||||||
|
const describedBy = keyInput.getAttribute("aria-describedby");
|
||||||
|
expect(describedBy).toBeTruthy();
|
||||||
|
const reason = describedBy
|
||||||
|
?.split(" ")
|
||||||
|
.map((id) => document.getElementById(id)?.textContent ?? "")
|
||||||
|
.join(" ");
|
||||||
|
expect(reason).toContain(
|
||||||
|
'"state" is set by the authorization flow and cannot be overridden.',
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// #2018 — the EMA leg authorizes against the enterprise IdP, a different
|
||||||
|
// authorization server, and deliberately sends none of these parameters. The
|
||||||
|
// description has to say so, or the form shows a configured value as active
|
||||||
|
// when it will not be sent.
|
||||||
|
it("warns that authorization parameters are unused under enterprise-managed auth", () => {
|
||||||
|
const { rerender } = renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
settings={{ ...emptySettings, enterpriseManaged: true }}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
screen.getByText(/Not sent while Enterprise-managed authorization is on/),
|
||||||
|
).toBeInTheDocument();
|
||||||
|
|
||||||
|
rerender(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
settings={{ ...emptySettings, enterpriseManaged: false }}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
screen.queryByText(
|
||||||
|
/Not sent while Enterprise-managed authorization is on/,
|
||||||
|
),
|
||||||
|
).not.toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not flag a blank authorization-parameter row as reserved", () => {
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
settings={{
|
||||||
|
...emptySettings,
|
||||||
|
oauthAuthorizationParams: [{ key: "", value: "" }],
|
||||||
|
}}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
screen.queryByText("Reserved parameters ignored"),
|
||||||
|
).not.toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("pluralizes the reserved-parameter warning for multiple keys", () => {
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsForm
|
||||||
|
{...baseHandlers}
|
||||||
|
settings={{
|
||||||
|
...emptySettings,
|
||||||
|
oauthAuthorizationParams: [
|
||||||
|
{ key: "state", value: "x" },
|
||||||
|
{ key: "scope", value: "y" },
|
||||||
|
],
|
||||||
|
}}
|
||||||
|
expandedSections={["oauth"]}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(screen.getByText(/state, scope are set/)).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
it("invokes onOAuthChange with the chosen insufficient-scope policy (SEP-2350)", async () => {
|
it("invokes onOAuthChange with the chosen insufficient-scope policy (SEP-2350)", async () => {
|
||||||
const user = userEvent.setup();
|
const user = userEvent.setup();
|
||||||
const onOAuthChange = vi.fn();
|
const onOAuthChange = vi.fn();
|
||||||
|
|
@ -744,6 +1006,7 @@ describe("ServerSettingsForm", () => {
|
||||||
clientId: "",
|
clientId: "",
|
||||||
clientSecret: "",
|
clientSecret: "",
|
||||||
scopes: "",
|
scopes: "",
|
||||||
|
authorizationParams: [],
|
||||||
enterpriseManaged: true,
|
enterpriseManaged: true,
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
@ -848,6 +1111,7 @@ describe("ServerSettingsForm", () => {
|
||||||
clientId: "",
|
clientId: "",
|
||||||
clientSecret: "z",
|
clientSecret: "z",
|
||||||
scopes: "",
|
scopes: "",
|
||||||
|
authorizationParams: [],
|
||||||
enterpriseManaged: false,
|
enterpriseManaged: false,
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,7 @@
|
||||||
import {
|
import {
|
||||||
Accordion,
|
Accordion,
|
||||||
ActionIcon,
|
ActionIcon,
|
||||||
|
Alert,
|
||||||
Button,
|
Button,
|
||||||
Checkbox,
|
Checkbox,
|
||||||
Flex,
|
Flex,
|
||||||
|
|
@ -26,6 +27,11 @@ import {
|
||||||
MODERN_LOG_LEVELS,
|
MODERN_LOG_LEVELS,
|
||||||
} from "@inspector/core/mcp/types.js";
|
} from "@inspector/core/mcp/types.js";
|
||||||
import { isOAuthCapableServerType } from "@inspector/core/mcp/config.js";
|
import { isOAuthCapableServerType } from "@inspector/core/mcp/config.js";
|
||||||
|
import {
|
||||||
|
RESERVED_AUTHORIZATION_PARAMS,
|
||||||
|
authorizationParamKeyError,
|
||||||
|
isReservedAuthorizationParam,
|
||||||
|
} from "@inspector/core/auth/authorizationParams.js";
|
||||||
import { ADVERTISABLE_EXTENSIONS } from "@inspector/core/mcp/extensions.js";
|
import { ADVERTISABLE_EXTENSIONS } from "@inspector/core/mcp/extensions.js";
|
||||||
import type { Root } from "@modelcontextprotocol/client";
|
import type { Root } from "@modelcontextprotocol/client";
|
||||||
|
|
||||||
|
|
@ -184,6 +190,25 @@ const EmptyHint = Text.withProps({
|
||||||
fs: "italic",
|
fs: "italic",
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Matches the label/description pair Mantine renders for a single input, so the
|
||||||
|
// authorization-parameter row list reads as one labelled field next to Scopes.
|
||||||
|
const FieldLabel = Text.withProps({
|
||||||
|
size: "sm",
|
||||||
|
fw: 500,
|
||||||
|
});
|
||||||
|
|
||||||
|
const FieldDescription = Text.withProps({
|
||||||
|
size: "xs",
|
||||||
|
c: "dimmed",
|
||||||
|
});
|
||||||
|
|
||||||
|
// `align="flex-start"` keeps the key/value/remove controls top-aligned when the
|
||||||
|
// key input grows an error message, so the row doesn't jump as it appears.
|
||||||
|
const AuthorizationParamRow = Group.withProps({
|
||||||
|
grow: true,
|
||||||
|
align: "flex-start",
|
||||||
|
});
|
||||||
|
|
||||||
const ClearStoredOAuthButton = Button.withProps({
|
const ClearStoredOAuthButton = Button.withProps({
|
||||||
variant: "light",
|
variant: "light",
|
||||||
color: "red",
|
color: "red",
|
||||||
|
|
@ -245,6 +270,74 @@ function KeyValueRows({
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Reserved-key rejection is inline per row (#2018): the reason is passed as the
|
||||||
|
// key input's `error` **string**, so Mantine renders it in the input's own error
|
||||||
|
// slot and wires the `aria-describedby` association — a bare boolean would mark
|
||||||
|
// the field invalid without telling a screen reader why. The section-level Alert
|
||||||
|
// below repeats it so the rejection stays visible when the offending row is
|
||||||
|
// scrolled out of view.
|
||||||
|
//
|
||||||
|
// Each control carries a per-row `aria-label`: the section heading and the
|
||||||
|
// placeholders are not programmatically associated with the inputs, so without
|
||||||
|
// one an assistive technology cannot tell the key box from the value box, and
|
||||||
|
// the remove button announces only "X".
|
||||||
|
function AuthorizationParamRows({
|
||||||
|
params,
|
||||||
|
onChange,
|
||||||
|
onRemove,
|
||||||
|
}: {
|
||||||
|
params: { key: string; value: string }[];
|
||||||
|
onChange: (index: number, key: string, value: string) => void;
|
||||||
|
onRemove: (index: number) => void;
|
||||||
|
}) {
|
||||||
|
return (
|
||||||
|
<>
|
||||||
|
{params.map((param, index) => {
|
||||||
|
const rowLabel = param.key.trim() || `row ${index + 1}`;
|
||||||
|
return (
|
||||||
|
<AuthorizationParamRow key={index}>
|
||||||
|
<ClearableTextInput
|
||||||
|
aria-label={`Authorization parameter name, ${rowLabel}`}
|
||||||
|
placeholder="Parameter (e.g. kc_idp_hint)"
|
||||||
|
value={param.key}
|
||||||
|
error={authorizationParamKeyError(param.key)}
|
||||||
|
onChange={(e) =>
|
||||||
|
onChange(index, e.currentTarget.value, param.value)
|
||||||
|
}
|
||||||
|
rightSection={
|
||||||
|
param.key ? (
|
||||||
|
<ClearButton
|
||||||
|
onClick={() => onChange(index, "", param.value)}
|
||||||
|
/>
|
||||||
|
) : null
|
||||||
|
}
|
||||||
|
/>
|
||||||
|
<ClearableTextInput
|
||||||
|
aria-label={`Authorization parameter value, ${rowLabel}`}
|
||||||
|
placeholder="Value"
|
||||||
|
value={param.value}
|
||||||
|
onChange={(e) =>
|
||||||
|
onChange(index, param.key, e.currentTarget.value)
|
||||||
|
}
|
||||||
|
rightSection={
|
||||||
|
param.value ? (
|
||||||
|
<ClearButton onClick={() => onChange(index, param.key, "")} />
|
||||||
|
) : null
|
||||||
|
}
|
||||||
|
/>
|
||||||
|
<RemoveIcon
|
||||||
|
aria-label={`Remove authorization parameter ${rowLabel}`}
|
||||||
|
onClick={() => onRemove(index)}
|
||||||
|
>
|
||||||
|
X
|
||||||
|
</RemoveIcon>
|
||||||
|
</AuthorizationParamRow>
|
||||||
|
);
|
||||||
|
})}
|
||||||
|
</>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
function RootRows({
|
function RootRows({
|
||||||
roots,
|
roots,
|
||||||
onChange,
|
onChange,
|
||||||
|
|
@ -374,16 +467,52 @@ export function ServerSettingsForm({
|
||||||
onTimeoutChange(field, numValue);
|
onTimeoutChange(field, numValue);
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// #2018 — custom authorization-request parameters. The rows ride the single
|
||||||
|
// `onOAuthChange` callback with the rest of the Authorization section rather
|
||||||
|
// than growing three more props on this form.
|
||||||
|
const authorizationParams = settings.oauthAuthorizationParams ?? [];
|
||||||
|
|
||||||
function currentOAuth(): OAuthSettings {
|
function currentOAuth(): OAuthSettings {
|
||||||
return {
|
return {
|
||||||
clientId: settings.oauthClientId ?? "",
|
clientId: settings.oauthClientId ?? "",
|
||||||
clientSecret: settings.oauthClientSecret ?? "",
|
clientSecret: settings.oauthClientSecret ?? "",
|
||||||
scopes: settings.oauthScopes ?? "",
|
scopes: settings.oauthScopes ?? "",
|
||||||
|
authorizationParams,
|
||||||
enterpriseManaged: settings.enterpriseManaged ?? false,
|
enterpriseManaged: settings.enterpriseManaged ?? false,
|
||||||
onInsufficientScope: settings.oauthOnInsufficientScope,
|
onInsufficientScope: settings.oauthOnInsufficientScope,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const rejectedParamKeys = authorizationParams
|
||||||
|
.map((p) => p.key.trim())
|
||||||
|
.filter((key) => isReservedAuthorizationParam(key));
|
||||||
|
|
||||||
|
function changeAuthorizationParams(
|
||||||
|
params: { key: string; value: string }[],
|
||||||
|
): void {
|
||||||
|
onOAuthChange({ ...currentOAuth(), authorizationParams: params });
|
||||||
|
}
|
||||||
|
|
||||||
|
function handleAddAuthorizationParam(): void {
|
||||||
|
changeAuthorizationParams([...authorizationParams, { key: "", value: "" }]);
|
||||||
|
}
|
||||||
|
|
||||||
|
function handleRemoveAuthorizationParam(index: number): void {
|
||||||
|
changeAuthorizationParams(
|
||||||
|
authorizationParams.filter((_, i) => i !== index),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
function handleAuthorizationParamChange(
|
||||||
|
index: number,
|
||||||
|
key: string,
|
||||||
|
value: string,
|
||||||
|
): void {
|
||||||
|
changeAuthorizationParams(
|
||||||
|
authorizationParams.map((p, i) => (i === index ? { key, value } : p)),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
return (
|
return (
|
||||||
// Stays inline: Accordion is a compound, `multiple`-discriminated generic,
|
// Stays inline: Accordion is a compound, `multiple`-discriminated generic,
|
||||||
// so `.withProps({ multiple: true, ... })` loses its JSX call signature
|
// so `.withProps({ multiple: true, ... })` loses its JSX call signature
|
||||||
|
|
@ -699,6 +828,41 @@ export function ServerSettingsForm({
|
||||||
) : null
|
) : null
|
||||||
}
|
}
|
||||||
/>
|
/>
|
||||||
|
<Stack gap="xs">
|
||||||
|
<FieldLabel>Additional authorization parameters</FieldLabel>
|
||||||
|
<FieldDescription>
|
||||||
|
Extra query parameters appended to the authorization request
|
||||||
|
only — never the token request. Use them for provider-specific
|
||||||
|
hints such as kc_idp_hint, login_hint, prompt, acr_values, or
|
||||||
|
audience.
|
||||||
|
{settings.enterpriseManaged
|
||||||
|
? " Not sent while Enterprise-managed authorization is on: that flow authorizes against the enterprise IdP, a different authorization server."
|
||||||
|
: ""}
|
||||||
|
</FieldDescription>
|
||||||
|
{authorizationParams.length === 0 ? (
|
||||||
|
<EmptyHint>No additional parameters configured</EmptyHint>
|
||||||
|
) : (
|
||||||
|
<AuthorizationParamRows
|
||||||
|
params={authorizationParams}
|
||||||
|
onChange={handleAuthorizationParamChange}
|
||||||
|
onRemove={handleRemoveAuthorizationParam}
|
||||||
|
/>
|
||||||
|
)}
|
||||||
|
{rejectedParamKeys.length > 0 ? (
|
||||||
|
<Alert color="red" title="Reserved parameters ignored">
|
||||||
|
{`${rejectedParamKeys.join(", ")} ${
|
||||||
|
rejectedParamKeys.length === 1 ? "is" : "are"
|
||||||
|
} set by the authorization flow and will not be sent. Overriding ${
|
||||||
|
rejectedParamKeys.length === 1 ? "it" : "them"
|
||||||
|
} would break PKCE, the CSRF state binding, or the resource indicator. Reserved: ${RESERVED_AUTHORIZATION_PARAMS.join(
|
||||||
|
", ",
|
||||||
|
)}.`}
|
||||||
|
</Alert>
|
||||||
|
) : null}
|
||||||
|
<AddButton onClick={handleAddAuthorizationParam}>
|
||||||
|
+ Add Parameter
|
||||||
|
</AddButton>
|
||||||
|
</Stack>
|
||||||
<Select
|
<Select
|
||||||
label="Insufficient-scope response"
|
label="Insufficient-scope response"
|
||||||
description="On a 403 insufficient_scope challenge (SEP-2350): re-authorize with the accumulated scope union, or surface the error to you."
|
description="On a 403 insufficient_scope challenge (SEP-2350): re-authorize with the accumulated scope union, or surface the error to you."
|
||||||
|
|
|
||||||
|
|
@ -440,10 +440,35 @@ describe("ServerSettingsModal", () => {
|
||||||
oauthClientId: "",
|
oauthClientId: "",
|
||||||
oauthClientSecret: "",
|
oauthClientSecret: "",
|
||||||
oauthScopes: "",
|
oauthScopes: "",
|
||||||
|
oauthAuthorizationParams: [],
|
||||||
enterpriseManaged: true,
|
enterpriseManaged: true,
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2018 — the authorization-parameter rows ride the same onOAuthChange
|
||||||
|
// callback, so the modal folds them onto the settings object.
|
||||||
|
it("persists an added authorization-parameter row onto the settings", async () => {
|
||||||
|
const user = userEvent.setup();
|
||||||
|
const onSettingsChange = vi.fn();
|
||||||
|
renderWithMantine(
|
||||||
|
<ServerSettingsModal
|
||||||
|
opened
|
||||||
|
settings={emptySettings}
|
||||||
|
serverType="streamable-http"
|
||||||
|
isStdio={false}
|
||||||
|
onClose={vi.fn()}
|
||||||
|
onSettingsChange={onSettingsChange}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
await user.click(screen.getByRole("button", { name: "OAuth Settings" }));
|
||||||
|
await user.click(screen.getByRole("button", { name: "+ Add Parameter" }));
|
||||||
|
expect(onSettingsChange).toHaveBeenCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
oauthAuthorizationParams: [{ key: "", value: "" }],
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
it("calls onSettingsChange with a blank row when adding a root", async () => {
|
it("calls onSettingsChange with a blank row when adding a root", async () => {
|
||||||
const user = userEvent.setup();
|
const user = userEvent.setup();
|
||||||
const onSettingsChange = vi.fn();
|
const onSettingsChange = vi.fn();
|
||||||
|
|
|
||||||
|
|
@ -179,6 +179,10 @@ export function ServerSettingsModal({
|
||||||
oauthClientId: oauth.clientId,
|
oauthClientId: oauth.clientId,
|
||||||
oauthClientSecret: oauth.clientSecret,
|
oauthClientSecret: oauth.clientSecret,
|
||||||
oauthScopes: oauth.scopes,
|
oauthScopes: oauth.scopes,
|
||||||
|
// #2018: kept as rows (blank ones included) so a half-typed parameter
|
||||||
|
// survives a re-render; the drop-blank/omit-empty filtering happens on the
|
||||||
|
// way to disk. An emptied list persists as `[]`, which writes nothing.
|
||||||
|
oauthAuthorizationParams: oauth.authorizationParams,
|
||||||
enterpriseManaged: oauth.enterpriseManaged ? true : undefined,
|
enterpriseManaged: oauth.enterpriseManaged ? true : undefined,
|
||||||
// SEP-2350: persist only the non-default ('throw') so unset servers keep
|
// SEP-2350: persist only the non-default ('throw') so unset servers keep
|
||||||
// the SDK's `reauthorize` behavior without writing a spurious field.
|
// the SDK's `reauthorize` behavior without writing a spurious field.
|
||||||
|
|
|
||||||
111
clients/web/src/test/core/auth/authorizationParams.test.ts
Normal file
111
clients/web/src/test/core/auth/authorizationParams.test.ts
Normal file
|
|
@ -0,0 +1,111 @@
|
||||||
|
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||||
|
import {
|
||||||
|
RESERVED_AUTHORIZATION_PARAMS,
|
||||||
|
applyAuthorizationParams,
|
||||||
|
authorizationParamKeyError,
|
||||||
|
isReservedAuthorizationParam,
|
||||||
|
} from "@inspector/core/auth/authorizationParams.js";
|
||||||
|
|
||||||
|
const AUTHORIZE_URL =
|
||||||
|
"https://as.example.com/authorize?client_id=abc&response_type=code&state=xyz";
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
vi.restoreAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("isReservedAuthorizationParam", () => {
|
||||||
|
it.each(RESERVED_AUTHORIZATION_PARAMS)("reserves %s", (key) => {
|
||||||
|
expect(isReservedAuthorizationParam(key)).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("ignores case and surrounding whitespace", () => {
|
||||||
|
expect(isReservedAuthorizationParam(" Client_ID ")).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("allows a provider-specific parameter", () => {
|
||||||
|
expect(isReservedAuthorizationParam("kc_idp_hint")).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("authorizationParamKeyError", () => {
|
||||||
|
it("returns no error for a blank key (a half-edited row)", () => {
|
||||||
|
expect(authorizationParamKeyError("")).toBeUndefined();
|
||||||
|
expect(authorizationParamKeyError(" ")).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns no error for an allowed key", () => {
|
||||||
|
expect(authorizationParamKeyError("login_hint")).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("names the offending key for a reserved one", () => {
|
||||||
|
expect(authorizationParamKeyError(" state ")).toBe(
|
||||||
|
'"state" is set by the authorization flow and cannot be overridden.',
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("applyAuthorizationParams", () => {
|
||||||
|
it("returns the same URL instance when no params are configured", () => {
|
||||||
|
const url = new URL(AUTHORIZE_URL);
|
||||||
|
expect(applyAuthorizationParams(url, undefined)).toBe(url);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns the same URL instance when the params object is empty", () => {
|
||||||
|
const url = new URL(AUTHORIZE_URL);
|
||||||
|
expect(applyAuthorizationParams(url, {})).toBe(url);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("appends custom params without mutating the input URL", () => {
|
||||||
|
const url = new URL(AUTHORIZE_URL);
|
||||||
|
const merged = applyAuthorizationParams(url, {
|
||||||
|
kc_idp_hint: "corp-idp",
|
||||||
|
login_hint: "user@example.com",
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(merged).not.toBe(url);
|
||||||
|
expect(url.searchParams.get("kc_idp_hint")).toBeNull();
|
||||||
|
expect(merged.searchParams.get("kc_idp_hint")).toBe("corp-idp");
|
||||||
|
expect(merged.searchParams.get("login_hint")).toBe("user@example.com");
|
||||||
|
// The SDK-built parameters survive untouched.
|
||||||
|
expect(merged.searchParams.get("client_id")).toBe("abc");
|
||||||
|
expect(merged.searchParams.get("state")).toBe("xyz");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("trims the key and preserves the value verbatim", () => {
|
||||||
|
const merged = applyAuthorizationParams(new URL(AUTHORIZE_URL), {
|
||||||
|
" prompt ": " login ",
|
||||||
|
});
|
||||||
|
expect(merged.searchParams.get("prompt")).toBe(" login ");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("skips blank keys", () => {
|
||||||
|
const url = new URL(AUTHORIZE_URL);
|
||||||
|
const merged = applyAuthorizationParams(url, { " ": "nope" });
|
||||||
|
expect(merged).toBe(url);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("drops a reserved key with a warning rather than overriding it", () => {
|
||||||
|
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||||
|
const merged = applyAuthorizationParams(new URL(AUTHORIZE_URL), {
|
||||||
|
state: "attacker-controlled",
|
||||||
|
kc_idp_hint: "corp-idp",
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(merged.searchParams.get("state")).toBe("xyz");
|
||||||
|
expect(merged.searchParams.get("kc_idp_hint")).toBe("corp-idp");
|
||||||
|
expect(warn).toHaveBeenCalledTimes(1);
|
||||||
|
expect(warn.mock.calls[0]?.[0]).toContain("state");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns the original URL when every configured key is reserved", () => {
|
||||||
|
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||||
|
const url = new URL(AUTHORIZE_URL);
|
||||||
|
expect(
|
||||||
|
applyAuthorizationParams(url, {
|
||||||
|
Client_Id: "spoofed",
|
||||||
|
code_challenge: "spoofed",
|
||||||
|
}),
|
||||||
|
).toBe(url);
|
||||||
|
expect(warn).toHaveBeenCalledTimes(2);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
@ -1,6 +1,14 @@
|
||||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||||
import type { OAuthTokens } from "@modelcontextprotocol/client";
|
import type { OAuthTokens } from "@modelcontextprotocol/client";
|
||||||
import type { BaseOAuthClientProvider } from "@inspector/core/auth/providers.js";
|
import {
|
||||||
|
BaseOAuthClientProvider,
|
||||||
|
CallbackNavigation,
|
||||||
|
MutableRedirectUrlProvider,
|
||||||
|
} from "@inspector/core/auth/providers.js";
|
||||||
|
import type { OAuthStorage } from "@inspector/core/auth/storage.js";
|
||||||
|
import { OAuthStorageBase } from "@inspector/core/auth/oauth-storage.js";
|
||||||
|
import { OAuthMemoryStore } from "@inspector/core/auth/store.js";
|
||||||
|
import type { OAuthPersistBackend } from "@inspector/core/auth/oauth-persist.js";
|
||||||
import type { EmaFlowConfig } from "@inspector/core/auth/ema/emaFlow.js";
|
import type { EmaFlowConfig } from "@inspector/core/auth/ema/emaFlow.js";
|
||||||
import { EmaTransportOAuthProvider } from "@inspector/core/auth/ema/transportProvider.js";
|
import { EmaTransportOAuthProvider } from "@inspector/core/auth/ema/transportProvider.js";
|
||||||
import {
|
import {
|
||||||
|
|
@ -18,6 +26,20 @@ const startIdpMock = vi.mocked(startEmaIdpAuthorization);
|
||||||
|
|
||||||
const SERVER_URL = "http://127.0.0.1:9999/mcp";
|
const SERVER_URL = "http://127.0.0.1:9999/mcp";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A real `OAuthStorage` — in-memory state over a no-op persist backend — for the
|
||||||
|
* one test below that drives an actual `BaseOAuthClientProvider`. Building the
|
||||||
|
* genuine article costs two lines and avoids an `as unknown as` stub that would
|
||||||
|
* stop type-checking against the interface the provider is handed.
|
||||||
|
*/
|
||||||
|
function makeRealStorage(): OAuthStorage {
|
||||||
|
const backend: OAuthPersistBackend = {
|
||||||
|
read: async () => null,
|
||||||
|
write: async () => {},
|
||||||
|
};
|
||||||
|
return new OAuthStorageBase(new OAuthMemoryStore(), backend);
|
||||||
|
}
|
||||||
|
|
||||||
function jwtWithExp(expSec: number): string {
|
function jwtWithExp(expSec: number): string {
|
||||||
const payload = btoa(JSON.stringify({ exp: expSec }))
|
const payload = btoa(JSON.stringify({ exp: expSec }))
|
||||||
.replace(/\+/g, "-")
|
.replace(/\+/g, "-")
|
||||||
|
|
@ -39,6 +61,7 @@ interface FakeInner {
|
||||||
tokens: ReturnType<typeof vi.fn>;
|
tokens: ReturnType<typeof vi.fn>;
|
||||||
saveTokens: ReturnType<typeof vi.fn>;
|
saveTokens: ReturnType<typeof vi.fn>;
|
||||||
redirectToAuthorization: ReturnType<typeof vi.fn>;
|
redirectToAuthorization: ReturnType<typeof vi.fn>;
|
||||||
|
redirectToExternalAuthorization: ReturnType<typeof vi.fn>;
|
||||||
clearCapturedAuthUrl: ReturnType<typeof vi.fn>;
|
clearCapturedAuthUrl: ReturnType<typeof vi.fn>;
|
||||||
saveCodeVerifier: ReturnType<typeof vi.fn>;
|
saveCodeVerifier: ReturnType<typeof vi.fn>;
|
||||||
codeVerifier: ReturnType<typeof vi.fn>;
|
codeVerifier: ReturnType<typeof vi.fn>;
|
||||||
|
|
@ -55,6 +78,7 @@ function createInner(): FakeInner {
|
||||||
tokens: vi.fn(),
|
tokens: vi.fn(),
|
||||||
saveTokens: vi.fn(),
|
saveTokens: vi.fn(),
|
||||||
redirectToAuthorization: vi.fn(),
|
redirectToAuthorization: vi.fn(),
|
||||||
|
redirectToExternalAuthorization: vi.fn(),
|
||||||
clearCapturedAuthUrl: vi.fn(),
|
clearCapturedAuthUrl: vi.fn(),
|
||||||
saveCodeVerifier: vi.fn(),
|
saveCodeVerifier: vi.fn(),
|
||||||
codeVerifier: vi.fn(() => "verifier-xyz"),
|
codeVerifier: vi.fn(() => "verifier-xyz"),
|
||||||
|
|
@ -178,6 +202,43 @@ describe("EmaTransportOAuthProvider", () => {
|
||||||
|
|
||||||
expect(startIdpMock).toHaveBeenCalledWith(emaConfig);
|
expect(startIdpMock).toHaveBeenCalledWith(emaConfig);
|
||||||
expect(inner.clearCapturedAuthUrl).toHaveBeenCalledTimes(1);
|
expect(inner.clearCapturedAuthUrl).toHaveBeenCalledTimes(1);
|
||||||
expect(inner.redirectToAuthorization).toHaveBeenCalledWith(idpUrl);
|
expect(inner.redirectToExternalAuthorization).toHaveBeenCalledWith(idpUrl);
|
||||||
|
// The custom-parameter merge lives on `redirectToAuthorization`, so the EMA
|
||||||
|
// leg must never take that path.
|
||||||
|
expect(inner.redirectToAuthorization).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
// #2018 — the inner provider carries the per-server custom authorization
|
||||||
|
// parameters, which belong to the MCP server's authorization server. The EMA
|
||||||
|
// leg sends the user to a *different* authorization server (the enterprise
|
||||||
|
// IdP), so those parameters must not reach it. Driven against a real
|
||||||
|
// BaseOAuthClientProvider rather than the fake inner above, since the defect
|
||||||
|
// is precisely in what the real provider does with the URL it is handed.
|
||||||
|
it("does not leak custom authorization params onto the IdP authorize URL", async () => {
|
||||||
|
const navCallback = vi.fn();
|
||||||
|
const realInner = new BaseOAuthClientProvider(SERVER_URL, {
|
||||||
|
// A real OAuthStorage rather than a cast stub: the redirect path never
|
||||||
|
// reads it, and a genuine implementation keeps this test honest if the
|
||||||
|
// storage contract changes.
|
||||||
|
storage: makeRealStorage(),
|
||||||
|
redirectUrlProvider: new MutableRedirectUrlProvider(),
|
||||||
|
navigation: new CallbackNavigation(navCallback),
|
||||||
|
authorizationParams: { kc_idp_hint: "corp-idp", audience: "api://mcp" },
|
||||||
|
});
|
||||||
|
const emaProvider = new EmaTransportOAuthProvider(realInner, emaConfig);
|
||||||
|
|
||||||
|
const idpUrl = new URL("https://idp.test/authorize?state=abc");
|
||||||
|
startIdpMock.mockResolvedValue(idpUrl);
|
||||||
|
|
||||||
|
await emaProvider.redirectToAuthorization(
|
||||||
|
new URL("https://resource-as.test/authorize"),
|
||||||
|
);
|
||||||
|
|
||||||
|
const navigated = navCallback.mock.calls[0]?.[0] as URL;
|
||||||
|
expect(navigated.searchParams.get("kc_idp_hint")).toBeNull();
|
||||||
|
expect(navigated.searchParams.get("audience")).toBeNull();
|
||||||
|
expect(navigated.href).toBe(idpUrl.href);
|
||||||
|
// The captured URL (what the step-up UI shows) must agree.
|
||||||
|
expect(realInner.getCapturedAuthUrl()?.href).toBe(idpUrl.href);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -352,6 +352,68 @@ describe("OAuthNavigation", () => {
|
||||||
expect(navCallback).toHaveBeenCalledWith(url);
|
expect(navCallback).toHaveBeenCalledWith(url);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2018 — the provider is the seam where per-server custom parameters reach
|
||||||
|
// the authorize URL, since the SDK builds that URL with no hook of its own.
|
||||||
|
function makeProviderWithParams(
|
||||||
|
storage: OAuthStorage,
|
||||||
|
authorizationParams: Record<string, string>,
|
||||||
|
navCallback = vi.fn(),
|
||||||
|
): BaseOAuthClientProvider {
|
||||||
|
return new BaseOAuthClientProvider(SERVER, {
|
||||||
|
storage,
|
||||||
|
redirectUrlProvider: new MutableRedirectUrlProvider(),
|
||||||
|
navigation: new CallbackNavigation(navCallback),
|
||||||
|
authorizationParams,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
it("redirectToAuthorization merges configured authorization params", () => {
|
||||||
|
const storage = makeStorage();
|
||||||
|
const navCallback = vi.fn();
|
||||||
|
const provider = makeProviderWithParams(
|
||||||
|
storage,
|
||||||
|
{ kc_idp_hint: "corp-idp" },
|
||||||
|
navCallback,
|
||||||
|
);
|
||||||
|
const target = new EventTarget();
|
||||||
|
const handler = vi.fn();
|
||||||
|
target.addEventListener("oauthAuthorizationRequired", handler);
|
||||||
|
provider.setEventTarget(target);
|
||||||
|
|
||||||
|
const url = new URL("https://mcp.example.com/authorize?client_id=abc");
|
||||||
|
provider.redirectToAuthorization(url);
|
||||||
|
|
||||||
|
const navigated = navCallback.mock.calls[0]?.[0] as URL;
|
||||||
|
expect(navigated.searchParams.get("kc_idp_hint")).toBe("corp-idp");
|
||||||
|
expect(navigated.searchParams.get("client_id")).toBe("abc");
|
||||||
|
// The captured URL, the event detail, and the navigation all agree.
|
||||||
|
expect(provider.getCapturedAuthUrl()?.href).toBe(navigated.href);
|
||||||
|
const detail = (handler.mock.calls[0]?.[0] as CustomEvent<{ url: URL }>)
|
||||||
|
.detail;
|
||||||
|
expect(detail.url.href).toBe(navigated.href);
|
||||||
|
// The URL the SDK handed in is left untouched.
|
||||||
|
expect(url.searchParams.get("kc_idp_hint")).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("redirectToAuthorization refuses a reserved authorization param", () => {
|
||||||
|
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||||
|
const storage = makeStorage();
|
||||||
|
const navCallback = vi.fn();
|
||||||
|
const provider = makeProviderWithParams(
|
||||||
|
storage,
|
||||||
|
{ state: "spoofed" },
|
||||||
|
navCallback,
|
||||||
|
);
|
||||||
|
|
||||||
|
const url = new URL("https://mcp.example.com/authorize?state=real");
|
||||||
|
provider.redirectToAuthorization(url);
|
||||||
|
|
||||||
|
const navigated = navCallback.mock.calls[0]?.[0] as URL;
|
||||||
|
expect(navigated.searchParams.get("state")).toBe("real");
|
||||||
|
expect(warn).toHaveBeenCalled();
|
||||||
|
warn.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
it("delegates token and scope persistence to storage", async () => {
|
it("delegates token and scope persistence to storage", async () => {
|
||||||
const storage = makeStorage();
|
const storage = makeStorage();
|
||||||
const provider = makeProvider(storage);
|
const provider = makeProvider(storage);
|
||||||
|
|
|
||||||
|
|
@ -86,6 +86,44 @@ describe("runner client auth options", () => {
|
||||||
expect(opts.oauth?.clientId).toBe("resource-client");
|
expect(opts.oauth?.clientId).toBe("resource-client");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #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", () => {
|
||||||
|
const settings: InspectorServerSettings = {
|
||||||
|
oauthAuthorizationParams: [
|
||||||
|
{ key: "kc_idp_hint", value: "corp" },
|
||||||
|
{ key: "", value: "dropped" },
|
||||||
|
],
|
||||||
|
requestTimeout: 0,
|
||||||
|
connectionTimeout: 0,
|
||||||
|
taskTtl: 60000,
|
||||||
|
maxFetchRequests: 10,
|
||||||
|
autoRefreshOnListChanged: false,
|
||||||
|
metadata: [],
|
||||||
|
headers: [],
|
||||||
|
env: [],
|
||||||
|
roots: [],
|
||||||
|
};
|
||||||
|
const opts = buildRunnerClientAuthOptions({}, settings);
|
||||||
|
expect(opts.oauth?.authorizationParams).toEqual({ kc_idp_hint: "corp" });
|
||||||
|
});
|
||||||
|
|
||||||
|
it("buildRunnerClientAuthOptions ignores all-blank authorization param rows", () => {
|
||||||
|
const settings: InspectorServerSettings = {
|
||||||
|
oauthAuthorizationParams: [{ key: " ", value: "x" }],
|
||||||
|
requestTimeout: 0,
|
||||||
|
connectionTimeout: 0,
|
||||||
|
taskTtl: 60000,
|
||||||
|
maxFetchRequests: 10,
|
||||||
|
autoRefreshOnListChanged: false,
|
||||||
|
metadata: [],
|
||||||
|
headers: [],
|
||||||
|
env: [],
|
||||||
|
roots: [],
|
||||||
|
};
|
||||||
|
expect(buildRunnerClientAuthOptions({}, settings)).toEqual({});
|
||||||
|
});
|
||||||
|
|
||||||
it("buildRunnerClientAuthOptions returns no oauth when nothing supplies it", () => {
|
it("buildRunnerClientAuthOptions returns no oauth when nothing supplies it", () => {
|
||||||
expect(buildRunnerClientAuthOptions({})).toEqual({});
|
expect(buildRunnerClientAuthOptions({})).toEqual({});
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -1574,6 +1574,53 @@ describe("OAuthManager", () => {
|
||||||
const provider = await manager.createOAuthProviderForTransport();
|
const provider = await manager.createOAuthProviderForTransport();
|
||||||
expect(provider.constructor.name).toBe("EmaTransportOAuthProvider");
|
expect(provider.constructor.name).toBe("EmaTransportOAuthProvider");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2018 — the manager→provider bridge for the custom authorization
|
||||||
|
// parameters. The provider test proves the merge and the runner test proves
|
||||||
|
// the options object, but neither crosses this seam: without this case,
|
||||||
|
// deleting `authorizationParams:` from `createOAuthProvider` would leave the
|
||||||
|
// feature dead with every other test still green.
|
||||||
|
it("forwards configured authorizationParams to the provider it builds", async () => {
|
||||||
|
const params = createMockParams();
|
||||||
|
const manager = new OAuthManager(params);
|
||||||
|
manager.setOAuthConfig({
|
||||||
|
authorizationParams: { kc_idp_hint: "corp-idp", prompt: "login" },
|
||||||
|
});
|
||||||
|
|
||||||
|
const provider = await manager.createOAuthProviderForTransport();
|
||||||
|
provider.redirectToAuthorization(
|
||||||
|
new URL("https://as.example.com/authorize?client_id=abc&state=xyz"),
|
||||||
|
);
|
||||||
|
|
||||||
|
const navigate = params.initialConfig.navigation
|
||||||
|
?.navigateToAuthorization as ReturnType<typeof vi.fn>;
|
||||||
|
const navigated = navigate.mock.calls[0]?.[0] as URL;
|
||||||
|
expect(navigated.searchParams.get("kc_idp_hint")).toBe("corp-idp");
|
||||||
|
expect(navigated.searchParams.get("prompt")).toBe("login");
|
||||||
|
// The flow's own parameters are untouched.
|
||||||
|
expect(navigated.searchParams.get("state")).toBe("xyz");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("drops a reserved key configured on the manager", async () => {
|
||||||
|
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||||
|
const params = createMockParams();
|
||||||
|
const manager = new OAuthManager(params);
|
||||||
|
manager.setOAuthConfig({
|
||||||
|
authorizationParams: { state: "spoofed", kc_idp_hint: "corp-idp" },
|
||||||
|
});
|
||||||
|
|
||||||
|
const provider = await manager.createOAuthProviderForTransport();
|
||||||
|
provider.redirectToAuthorization(
|
||||||
|
new URL("https://as.example.com/authorize?client_id=abc&state=xyz"),
|
||||||
|
);
|
||||||
|
|
||||||
|
const navigate = params.initialConfig.navigation
|
||||||
|
?.navigateToAuthorization as ReturnType<typeof vi.fn>;
|
||||||
|
const navigated = navigate.mock.calls[0]?.[0] as URL;
|
||||||
|
expect(navigated.searchParams.get("state")).toBe("xyz");
|
||||||
|
expect(navigated.searchParams.get("kc_idp_hint")).toBe("corp-idp");
|
||||||
|
expect(warn).toHaveBeenCalled();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("getOAuthState (enterprise managed / scope)", () => {
|
describe("getOAuthState (enterprise managed / scope)", () => {
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,6 @@
|
||||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||||
import {
|
import {
|
||||||
|
cleanAuthorizationParams,
|
||||||
cleanRoots,
|
cleanRoots,
|
||||||
DEFAULT_SEED_CONFIG,
|
DEFAULT_SEED_CONFIG,
|
||||||
envPairsToRecord,
|
envPairsToRecord,
|
||||||
|
|
@ -10,6 +11,7 @@ import {
|
||||||
mcpConfigToServerEntries,
|
mcpConfigToServerEntries,
|
||||||
mergeSecretsIntoStored,
|
mergeSecretsIntoStored,
|
||||||
normalizeServerType,
|
normalizeServerType,
|
||||||
|
oauthAuthorizationParamsFromSettings,
|
||||||
serverEntriesToMcpConfig,
|
serverEntriesToMcpConfig,
|
||||||
serializeMcpConfig,
|
serializeMcpConfig,
|
||||||
storedFieldsToInspectorSettings,
|
storedFieldsToInspectorSettings,
|
||||||
|
|
@ -924,6 +926,42 @@ describe("extractSecretsFromStored", () => {
|
||||||
expect(stripped.oauth).toEqual({ enterpriseManaged: true });
|
expect(stripped.oauth).toEqual({ enterpriseManaged: true });
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// The stripped entry now subtracts the secret instead of enumerating the
|
||||||
|
// fields to keep, so a field added to `oauth` later survives by construction.
|
||||||
|
// `onInsufficientScope` is the case the old allow-list had already lost.
|
||||||
|
it("preserves oauth.onInsufficientScope when lifting clientSecret to keychain", () => {
|
||||||
|
const stored: StoredMCPServer = {
|
||||||
|
type: "streamable-http",
|
||||||
|
url: "https://x.test",
|
||||||
|
oauth: {
|
||||||
|
clientId: "cid",
|
||||||
|
clientSecret: "shh",
|
||||||
|
onInsufficientScope: "throw",
|
||||||
|
},
|
||||||
|
};
|
||||||
|
const { stripped, secrets } = extractSecretsFromStored(stored);
|
||||||
|
expect(secrets).toEqual({ [SECRET_FIELD_OAUTH_CLIENT_SECRET]: "shh" });
|
||||||
|
expect(stripped.oauth).toEqual({
|
||||||
|
clientId: "cid",
|
||||||
|
onInsufficientScope: "throw",
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("preserves oauth.authorizationParams when lifting clientSecret to keychain", () => {
|
||||||
|
const stored: StoredMCPServer = {
|
||||||
|
type: "streamable-http",
|
||||||
|
url: "https://x.test",
|
||||||
|
oauth: {
|
||||||
|
clientSecret: "shh",
|
||||||
|
authorizationParams: { kc_idp_hint: "corp" },
|
||||||
|
},
|
||||||
|
};
|
||||||
|
const { stripped } = extractSecretsFromStored(stored);
|
||||||
|
expect(stripped.oauth).toEqual({
|
||||||
|
authorizationParams: { kc_idp_hint: "corp" },
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it("removes the oauth block entirely when clientSecret was its only property", () => {
|
it("removes the oauth block entirely when clientSecret was its only property", () => {
|
||||||
const stored: StoredMCPServer = {
|
const stored: StoredMCPServer = {
|
||||||
type: "streamable-http",
|
type: "streamable-http",
|
||||||
|
|
@ -1188,6 +1226,68 @@ describe("oauthOnInsufficientScope (SEP-2350)", () => {
|
||||||
});
|
});
|
||||||
expect(stored.oauth?.onInsufficientScope).toBeUndefined();
|
expect(stored.oauth?.onInsufficientScope).toBeUndefined();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2018 — custom authorization-request parameters.
|
||||||
|
it("lifts oauth.authorizationParams into settings rows", () => {
|
||||||
|
const settings = storedFieldsToInspectorSettings({
|
||||||
|
oauth: { authorizationParams: { kc_idp_hint: "corp", prompt: "login" } },
|
||||||
|
});
|
||||||
|
expect(settings?.oauthAuthorizationParams).toEqual([
|
||||||
|
{ key: "kc_idp_hint", value: "corp" },
|
||||||
|
{ key: "prompt", value: "login" },
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("reads an empty authorizationParams object back as unset", () => {
|
||||||
|
const settings = storedFieldsToInspectorSettings({
|
||||||
|
oauth: { clientId: "cid", authorizationParams: {} },
|
||||||
|
});
|
||||||
|
expect(settings?.oauthAuthorizationParams).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("persists authorizationParams under oauth, dropping blank-key rows", () => {
|
||||||
|
const stored = inspectorSettingsToStoredFields({
|
||||||
|
...baseSettings,
|
||||||
|
oauthAuthorizationParams: [
|
||||||
|
{ key: "kc_idp_hint", value: "corp" },
|
||||||
|
{ key: " ", value: "orphan" },
|
||||||
|
],
|
||||||
|
});
|
||||||
|
expect(stored.oauth?.authorizationParams).toEqual({ kc_idp_hint: "corp" });
|
||||||
|
});
|
||||||
|
|
||||||
|
it("omits the oauth block when every authorization-param row is blank", () => {
|
||||||
|
const stored = inspectorSettingsToStoredFields({
|
||||||
|
...baseSettings,
|
||||||
|
oauthAuthorizationParams: [{ key: "", value: "" }],
|
||||||
|
});
|
||||||
|
expect(stored).not.toHaveProperty("oauth");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("omits authorizationParams when the rows are absent", () => {
|
||||||
|
const stored = inspectorSettingsToStoredFields({
|
||||||
|
...baseSettings,
|
||||||
|
oauthClientId: "cid",
|
||||||
|
});
|
||||||
|
expect(stored.oauth?.authorizationParams).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("derives the client option record, or undefined when nothing survives", () => {
|
||||||
|
expect(
|
||||||
|
oauthAuthorizationParamsFromSettings({
|
||||||
|
oauthAuthorizationParams: [
|
||||||
|
{ key: "kc_idp_hint", value: "corp" },
|
||||||
|
{ key: "", value: "x" },
|
||||||
|
],
|
||||||
|
}),
|
||||||
|
).toEqual({ kc_idp_hint: "corp" });
|
||||||
|
expect(
|
||||||
|
oauthAuthorizationParamsFromSettings({
|
||||||
|
oauthAuthorizationParams: [{ key: " ", value: "x" }],
|
||||||
|
}),
|
||||||
|
).toBeUndefined();
|
||||||
|
expect(oauthAuthorizationParamsFromSettings({})).toBeUndefined();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("envRecordToPairs / envPairsToRecord", () => {
|
describe("envRecordToPairs / envPairsToRecord", () => {
|
||||||
|
|
@ -1316,3 +1416,94 @@ describe("stdio env / cwd mirroring", () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2018 — `StoredMCPServer.oauth.authorizationParams` is typed
|
||||||
|
// `Record<string, string>`, but only the web client's `/api/servers` route
|
||||||
|
// validates it; the CLI and TUI read `mcp.json` off disk. These cases drive the
|
||||||
|
// shapes a hand-edited file can hold, which the type says are impossible.
|
||||||
|
describe("cleanAuthorizationParams", () => {
|
||||||
|
let warnSpy: ReturnType<typeof vi.spyOn>;
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
warnSpy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("passes a well-formed record through", () => {
|
||||||
|
expect(
|
||||||
|
cleanAuthorizationParams({ kc_idp_hint: "corp", prompt: "login" }),
|
||||||
|
).toEqual({ kc_idp_hint: "corp", prompt: "login" });
|
||||||
|
expect(warnSpy).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns undefined for an absent or empty record", () => {
|
||||||
|
expect(cleanAuthorizationParams(undefined)).toBeUndefined();
|
||||||
|
expect(cleanAuthorizationParams({})).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
// A bare string enumerates as character-indexed pairs (0=o, 1=o, …), so
|
||||||
|
// without this guard the Inspector would send four invented parameters.
|
||||||
|
it("rejects a string with a warning instead of enumerating its characters", () => {
|
||||||
|
expect(
|
||||||
|
cleanAuthorizationParams("oops" as unknown as Record<string, string>),
|
||||||
|
).toBeUndefined();
|
||||||
|
expect(warnSpy).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("rejects an array with a warning", () => {
|
||||||
|
expect(
|
||||||
|
cleanAuthorizationParams(["a"] as unknown as Record<string, string>),
|
||||||
|
).toBeUndefined();
|
||||||
|
expect(warnSpy.mock.calls[0]?.[1]).toBe("array");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("rejects null with a warning", () => {
|
||||||
|
expect(
|
||||||
|
cleanAuthorizationParams(null as unknown as Record<string, string>),
|
||||||
|
).toBeUndefined();
|
||||||
|
expect(warnSpy).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
// `URLSearchParams.set` stringifies whatever it is given, so a number would
|
||||||
|
// otherwise reach the authorize URL as "5".
|
||||||
|
it("drops a non-string value with a warning and keeps the rest", () => {
|
||||||
|
expect(
|
||||||
|
cleanAuthorizationParams({
|
||||||
|
audience: 5,
|
||||||
|
kc_idp_hint: "corp",
|
||||||
|
} as unknown as Record<string, string>),
|
||||||
|
).toEqual({ kc_idp_hint: "corp" });
|
||||||
|
expect(warnSpy).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns undefined when every value was dropped", () => {
|
||||||
|
expect(
|
||||||
|
cleanAuthorizationParams({ audience: 5 } as unknown as Record<
|
||||||
|
string,
|
||||||
|
string
|
||||||
|
>),
|
||||||
|
).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("skips a blank key", () => {
|
||||||
|
expect(cleanAuthorizationParams({ " ": "x", ok: "y" })).toEqual({
|
||||||
|
ok: "y",
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("is applied when lifting stored fields into settings", () => {
|
||||||
|
const settings = storedFieldsToInspectorSettings({
|
||||||
|
type: "streamable-http",
|
||||||
|
url: "https://x.test",
|
||||||
|
oauth: {
|
||||||
|
authorizationParams: { audience: 5, kc_idp_hint: "corp" },
|
||||||
|
},
|
||||||
|
} as unknown as StoredMCPServer);
|
||||||
|
expect(settings?.oauthAuthorizationParams).toEqual([
|
||||||
|
{ key: "kc_idp_hint", value: "corp" },
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -701,6 +701,58 @@ describe("server.ts supplemental coverage", () => {
|
||||||
await stop(h);
|
await stop(h);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2018 — authorizationParams must be a string record; a hand-edited file
|
||||||
|
// with anything else drops the whole `oauth` node rather than feeding
|
||||||
|
// non-string values to the authorize-URL merge.
|
||||||
|
it("drops oauth whose authorizationParams is not a string record", async () => {
|
||||||
|
const h = await start({
|
||||||
|
seedConfig: JSON.stringify({
|
||||||
|
mcpServers: {
|
||||||
|
srv: {
|
||||||
|
type: "streamable-http",
|
||||||
|
url: "https://x.test/mcp",
|
||||||
|
oauth: { authorizationParams: { kc_idp_hint: 5 } },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
try {
|
||||||
|
const res = await fetch(`${h.baseUrl}/api/servers`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
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 authorizationParams record on read", async () => {
|
||||||
|
const h = await start({
|
||||||
|
seedConfig: JSON.stringify({
|
||||||
|
mcpServers: {
|
||||||
|
srv: {
|
||||||
|
type: "streamable-http",
|
||||||
|
url: "https://x.test/mcp",
|
||||||
|
oauth: { authorizationParams: { kc_idp_hint: "corp" } },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
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({
|
||||||
|
authorizationParams: { kc_idp_hint: "corp" },
|
||||||
|
});
|
||||||
|
} finally {
|
||||||
|
await stop(h);
|
||||||
|
}
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("plaintext-secret migration over a mixed config", () => {
|
describe("plaintext-secret migration over a mixed config", () => {
|
||||||
|
|
|
||||||
|
|
@ -449,6 +449,53 @@ describe("/api/servers routes", () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2018 — the authorization-parameter rows travel as a pair array on the
|
||||||
|
// wire and land as a record under `oauth` on disk, mirroring `headers`.
|
||||||
|
it("persists custom authorization params under oauth on POST", async () => {
|
||||||
|
const res = await fetch(`${h.baseUrl}/api/servers`, {
|
||||||
|
method: "POST",
|
||||||
|
headers: { "Content-Type": "application/json" },
|
||||||
|
body: JSON.stringify({
|
||||||
|
id: "authparams",
|
||||||
|
config: { type: "streamable-http", url: "https://x.test/mcp" },
|
||||||
|
settings: {
|
||||||
|
headers: [],
|
||||||
|
metadata: [],
|
||||||
|
connectionTimeout: 0,
|
||||||
|
requestTimeout: 0,
|
||||||
|
oauthAuthorizationParams: [
|
||||||
|
{ key: "kc_idp_hint", value: "corp" },
|
||||||
|
{ key: "", value: "dropped" },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
const stored = readConfig(h.configPath).mcpServers.authparams;
|
||||||
|
expect(stored?.oauth).toEqual({
|
||||||
|
authorizationParams: { kc_idp_hint: "corp" },
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("rejects a malformed oauthAuthorizationParams payload", async () => {
|
||||||
|
const res = await fetch(`${h.baseUrl}/api/servers`, {
|
||||||
|
method: "POST",
|
||||||
|
headers: { "Content-Type": "application/json" },
|
||||||
|
body: JSON.stringify({
|
||||||
|
id: "badauthparams",
|
||||||
|
config: { type: "streamable-http", url: "https://x.test/mcp" },
|
||||||
|
settings: {
|
||||||
|
headers: [],
|
||||||
|
metadata: [],
|
||||||
|
connectionTimeout: 0,
|
||||||
|
requestTimeout: 0,
|
||||||
|
oauthAuthorizationParams: { kc_idp_hint: "corp" },
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
});
|
||||||
|
|
||||||
it("updates Inspector-extension fields at the top level on PUT", async () => {
|
it("updates Inspector-extension fields at the top level on PUT", async () => {
|
||||||
writeFileSync(
|
writeFileSync(
|
||||||
h.configPath,
|
h.configPath,
|
||||||
|
|
|
||||||
105
core/auth/authorizationParams.ts
Normal file
105
core/auth/authorizationParams.ts
Normal file
|
|
@ -0,0 +1,105 @@
|
||||||
|
/**
|
||||||
|
* Per-server custom authorization-request parameters (#2018).
|
||||||
|
*
|
||||||
|
* Real deployments gate IdP selection, prompt behavior, or audience on a query
|
||||||
|
* parameter the OAuth/OIDC core specs never standardized — Keycloak's
|
||||||
|
* `kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's
|
||||||
|
* `audience`. The Inspector never builds the authorize URL itself (the SDK's
|
||||||
|
* `auth()` does, and exposes no hook for extra parameters), so the merge
|
||||||
|
* happens at the one seam that sees the finished URL:
|
||||||
|
* `BaseOAuthClientProvider.redirectToAuthorization`.
|
||||||
|
*
|
||||||
|
* The parameters apply to the **authorization request only** — never the token
|
||||||
|
* request, which the SDK builds separately and which this module is not wired
|
||||||
|
* into. They also apply only to the server's **own** authorization server:
|
||||||
|
* the enterprise-managed (EMA) leg redirects to a different one (the enterprise
|
||||||
|
* IdP) and goes through `BaseOAuthClientProvider.redirectToExternalAuthorization`,
|
||||||
|
* which deliberately skips the merge.
|
||||||
|
*/
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Parameters the OAuth flow owns and a user may not override.
|
||||||
|
*
|
||||||
|
* Overriding any of these does not produce a useful debugging scenario — it
|
||||||
|
* breaks PKCE (`code_challenge`, `code_challenge_method`), CSRF binding
|
||||||
|
* (`state`), the callback leg (`redirect_uri`, `response_type`, `client_id`),
|
||||||
|
* or RFC 8707 resource indicators (`resource`), and the failure surfaces as an
|
||||||
|
* opaque authorization-server error rather than as an Inspector problem.
|
||||||
|
* `scope` is excluded too: it has its own field, and the SEP-2350 step-up union
|
||||||
|
* is computed from that field rather than from these parameters.
|
||||||
|
*/
|
||||||
|
export const RESERVED_AUTHORIZATION_PARAMS = [
|
||||||
|
"client_id",
|
||||||
|
"code_challenge",
|
||||||
|
"code_challenge_method",
|
||||||
|
"redirect_uri",
|
||||||
|
"resource",
|
||||||
|
"response_type",
|
||||||
|
"scope",
|
||||||
|
"state",
|
||||||
|
] as const;
|
||||||
|
|
||||||
|
export type ReservedAuthorizationParam =
|
||||||
|
(typeof RESERVED_AUTHORIZATION_PARAMS)[number];
|
||||||
|
|
||||||
|
const RESERVED: ReadonlySet<string> = new Set(RESERVED_AUTHORIZATION_PARAMS);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whether `key` names a parameter the OAuth flow owns. Compared case- and
|
||||||
|
* whitespace-insensitively: query parameter names are case-sensitive on the
|
||||||
|
* wire, but a `Client_Id` typed into the form is plainly an attempt to set the
|
||||||
|
* reserved one, and letting it through would be the silent override this rule
|
||||||
|
* exists to prevent.
|
||||||
|
*/
|
||||||
|
export function isReservedAuthorizationParam(key: string): boolean {
|
||||||
|
return RESERVED.has(key.trim().toLowerCase());
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Validation message for one authorization-parameter key, or `undefined` when
|
||||||
|
* the key is acceptable. A blank key is not an error — the form lets a user
|
||||||
|
* leave a freshly-added row empty mid-edit, and blank rows are dropped rather
|
||||||
|
* than rejected.
|
||||||
|
*/
|
||||||
|
export function authorizationParamKeyError(key: string): string | undefined {
|
||||||
|
const trimmed = key.trim();
|
||||||
|
if (trimmed === "") return undefined;
|
||||||
|
if (isReservedAuthorizationParam(trimmed)) {
|
||||||
|
return `"${trimmed}" is set by the authorization flow and cannot be overridden.`;
|
||||||
|
}
|
||||||
|
return undefined;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Merge custom parameters into a finished authorization URL.
|
||||||
|
*
|
||||||
|
* Blank keys are skipped (a half-edited form row) and reserved keys are dropped
|
||||||
|
* with a warning — never silently, and never overriding what the SDK put there.
|
||||||
|
* Returns the original URL instance when there is nothing to apply, and
|
||||||
|
* otherwise a copy, so the caller's URL is never mutated.
|
||||||
|
*/
|
||||||
|
export function applyAuthorizationParams(
|
||||||
|
authorizationUrl: URL,
|
||||||
|
params: Record<string, string> | undefined,
|
||||||
|
): URL {
|
||||||
|
if (!params) return authorizationUrl;
|
||||||
|
|
||||||
|
const applicable = Object.entries(params).filter(([key]) => {
|
||||||
|
if (key.trim() === "") return false;
|
||||||
|
if (isReservedAuthorizationParam(key)) {
|
||||||
|
console.warn(
|
||||||
|
`[oauth] Ignoring reserved authorization parameter "${key.trim()}" — it is set by the authorization flow.`,
|
||||||
|
);
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
return true;
|
||||||
|
});
|
||||||
|
|
||||||
|
if (applicable.length === 0) return authorizationUrl;
|
||||||
|
|
||||||
|
const merged = new URL(authorizationUrl.href);
|
||||||
|
for (const [key, value] of applicable) {
|
||||||
|
merged.searchParams.set(key.trim(), value);
|
||||||
|
}
|
||||||
|
return merged;
|
||||||
|
}
|
||||||
|
|
@ -84,7 +84,11 @@ export class EmaTransportOAuthProvider implements OAuthClientProvider {
|
||||||
async redirectToAuthorization(_authorizationUrl: URL): Promise<void> {
|
async redirectToAuthorization(_authorizationUrl: URL): Promise<void> {
|
||||||
const idpAuthorizationUrl = await startEmaIdpAuthorization(this.emaConfig);
|
const idpAuthorizationUrl = await startEmaIdpAuthorization(this.emaConfig);
|
||||||
this.inner.clearCapturedAuthUrl();
|
this.inner.clearCapturedAuthUrl();
|
||||||
this.inner.redirectToAuthorization(idpAuthorizationUrl);
|
// The IdP is a different authorization server than the one the inner
|
||||||
|
// provider was configured for, so the per-server custom authorization
|
||||||
|
// parameters (#2018) must not be appended here — hence the "external"
|
||||||
|
// entry point rather than `redirectToAuthorization`.
|
||||||
|
this.inner.redirectToExternalAuthorization(idpAuthorizationUrl);
|
||||||
}
|
}
|
||||||
|
|
||||||
saveCodeVerifier(codeVerifier: string): void | Promise<void> {
|
saveCodeVerifier(codeVerifier: string): void | Promise<void> {
|
||||||
|
|
|
||||||
|
|
@ -27,6 +27,15 @@ export { ensureCimdClientRegistration } from "./cimd.js";
|
||||||
export { mcpAuth, type McpAuthOptions, type McpAuthResult } from "./mcpAuth.js";
|
export { mcpAuth, type McpAuthOptions, type McpAuthResult } from "./mcpAuth.js";
|
||||||
export { computeScopeUnion, isStrictScopeSuperset } from "./scopes.js";
|
export { computeScopeUnion, isStrictScopeSuperset } from "./scopes.js";
|
||||||
|
|
||||||
|
// Custom authorization-request parameters (#2018)
|
||||||
|
export {
|
||||||
|
RESERVED_AUTHORIZATION_PARAMS,
|
||||||
|
isReservedAuthorizationParam,
|
||||||
|
authorizationParamKeyError,
|
||||||
|
applyAuthorizationParams,
|
||||||
|
} from "./authorizationParams.js";
|
||||||
|
export type { ReservedAuthorizationParam } from "./authorizationParams.js";
|
||||||
|
|
||||||
// Storage
|
// Storage
|
||||||
export type {
|
export type {
|
||||||
OAuthStorage,
|
OAuthStorage,
|
||||||
|
|
|
||||||
|
|
@ -11,6 +11,7 @@ import type {
|
||||||
} from "@modelcontextprotocol/client";
|
} from "@modelcontextprotocol/client";
|
||||||
import type { OAuthStorage, SaveClientInformationOptions } from "./storage.js";
|
import type { OAuthStorage, SaveClientInformationOptions } from "./storage.js";
|
||||||
import { generateOAuthState } from "./utils.js";
|
import { generateOAuthState } from "./utils.js";
|
||||||
|
import { applyAuthorizationParams } from "./authorizationParams.js";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Redirect URL provider. Returns the redirect URL for OAuth flows.
|
* Redirect URL provider. Returns the redirect URL for OAuth flows.
|
||||||
|
|
@ -96,6 +97,14 @@ export type OAuthProviderConfig = {
|
||||||
redirectUrlProvider: RedirectUrlProvider;
|
redirectUrlProvider: RedirectUrlProvider;
|
||||||
navigation: OAuthNavigation;
|
navigation: OAuthNavigation;
|
||||||
clientMetadataUrl?: string;
|
clientMetadataUrl?: string;
|
||||||
|
/**
|
||||||
|
* Per-server custom authorization-request parameters (#2018), merged into the
|
||||||
|
* finished authorize URL in {@link BaseOAuthClientProvider.redirectToAuthorization}.
|
||||||
|
* Reserved (protocol-critical) keys are dropped with a warning — see
|
||||||
|
* `authorizationParams.ts`. Authorization request only; the token request is
|
||||||
|
* unaffected.
|
||||||
|
*/
|
||||||
|
authorizationParams?: Record<string, string>;
|
||||||
};
|
};
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
@ -115,6 +124,8 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
||||||
protected redirectUrlProvider: RedirectUrlProvider;
|
protected redirectUrlProvider: RedirectUrlProvider;
|
||||||
protected navigation: OAuthNavigation;
|
protected navigation: OAuthNavigation;
|
||||||
public clientMetadataUrl?: string;
|
public clientMetadataUrl?: string;
|
||||||
|
/** Custom authorization-request parameters (#2018). Authorize URL only. */
|
||||||
|
protected authorizationParams?: Record<string, string>;
|
||||||
|
|
||||||
constructor(serverUrl: string, oauthConfig: OAuthProviderConfig) {
|
constructor(serverUrl: string, oauthConfig: OAuthProviderConfig) {
|
||||||
this.serverUrl = serverUrl;
|
this.serverUrl = serverUrl;
|
||||||
|
|
@ -122,6 +133,7 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
||||||
this.redirectUrlProvider = oauthConfig.redirectUrlProvider;
|
this.redirectUrlProvider = oauthConfig.redirectUrlProvider;
|
||||||
this.navigation = oauthConfig.navigation;
|
this.navigation = oauthConfig.navigation;
|
||||||
this.clientMetadataUrl = oauthConfig.clientMetadataUrl;
|
this.clientMetadataUrl = oauthConfig.clientMetadataUrl;
|
||||||
|
this.authorizationParams = oauthConfig.authorizationParams;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
@ -272,6 +284,34 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
||||||
}
|
}
|
||||||
|
|
||||||
redirectToAuthorization(authorizationUrl: URL): void {
|
redirectToAuthorization(authorizationUrl: URL): void {
|
||||||
|
// #2018: the SDK builds the authorize URL and offers no hook for extra
|
||||||
|
// parameters, so the per-server ones are merged here — the one seam that
|
||||||
|
// sees the finished URL before navigation. Everything downstream (the
|
||||||
|
// captured URL used by the step-up modal, the event, the navigation) uses
|
||||||
|
// the merged URL so all three agree on what was actually requested.
|
||||||
|
this.performAuthorizationRedirect(
|
||||||
|
applyAuthorizationParams(authorizationUrl, this.authorizationParams),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Redirect to an authorize URL belonging to a **different** authorization
|
||||||
|
* server than the one this provider was configured for, so the per-server
|
||||||
|
* custom parameters (#2018) MUST NOT be applied.
|
||||||
|
*
|
||||||
|
* The only caller is the enterprise-managed (EMA) transport provider, which
|
||||||
|
* discards the resource AS URL and sends the user to the enterprise IdP
|
||||||
|
* instead. Parameters a user configured for their MCP server's authorization
|
||||||
|
* server — a `kc_idp_hint`, an `audience` — are meaningless and potentially
|
||||||
|
* flow-breaking on the IdP's, and appending them there would leak per-server
|
||||||
|
* config across an authorization-server boundary.
|
||||||
|
*/
|
||||||
|
redirectToExternalAuthorization(authorizationUrl: URL): void {
|
||||||
|
this.performAuthorizationRedirect(authorizationUrl);
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Capture, announce, and navigate to a finished authorize URL. */
|
||||||
|
private performAuthorizationRedirect(authorizationUrl: URL): void {
|
||||||
// Capture URL for return value
|
// Capture URL for return value
|
||||||
this.capturedAuthUrl = authorizationUrl;
|
this.capturedAuthUrl = authorizationUrl;
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -11,6 +11,7 @@ import type {
|
||||||
InspectorClientOptions,
|
InspectorClientOptions,
|
||||||
InspectorServerSettings,
|
InspectorServerSettings,
|
||||||
} from "../mcp/types.js";
|
} from "../mcp/types.js";
|
||||||
|
import { oauthAuthorizationParamsFromSettings } from "../mcp/serverList.js";
|
||||||
import { loadClientConfig } from "./config.js";
|
import { loadClientConfig } from "./config.js";
|
||||||
import type { ClientConfig } from "./types.js";
|
import type { ClientConfig } from "./types.js";
|
||||||
import {
|
import {
|
||||||
|
|
@ -70,11 +71,16 @@ export function buildRunnerClientAuthOptions(
|
||||||
const activeIdp = getActiveEnterpriseManagedAuthIdp(clientConfig);
|
const activeIdp = getActiveEnterpriseManagedAuthIdp(clientConfig);
|
||||||
const activeCimdUrl = getActiveCimdClientMetadataUrl(clientConfig);
|
const activeCimdUrl = getActiveCimdClientMetadataUrl(clientConfig);
|
||||||
|
|
||||||
|
const serverAuthorizationParams = savedSettings
|
||||||
|
? oauthAuthorizationParamsFromSettings(savedSettings)
|
||||||
|
: undefined;
|
||||||
|
|
||||||
const oauthFromServer =
|
const oauthFromServer =
|
||||||
savedSettings &&
|
savedSettings &&
|
||||||
(savedSettings.oauthClientId ||
|
(savedSettings.oauthClientId ||
|
||||||
savedSettings.oauthClientSecret ||
|
savedSettings.oauthClientSecret ||
|
||||||
savedSettings.oauthScopes ||
|
savedSettings.oauthScopes ||
|
||||||
|
serverAuthorizationParams ||
|
||||||
savedSettings.enterpriseManaged)
|
savedSettings.enterpriseManaged)
|
||||||
? {
|
? {
|
||||||
...(savedSettings.oauthClientId && {
|
...(savedSettings.oauthClientId && {
|
||||||
|
|
@ -86,6 +92,9 @@ export function buildRunnerClientAuthOptions(
|
||||||
...(savedSettings.oauthScopes && {
|
...(savedSettings.oauthScopes && {
|
||||||
scope: savedSettings.oauthScopes,
|
scope: savedSettings.oauthScopes,
|
||||||
}),
|
}),
|
||||||
|
...(serverAuthorizationParams && {
|
||||||
|
authorizationParams: serverAuthorizationParams,
|
||||||
|
}),
|
||||||
...(savedSettings.enterpriseManaged && {
|
...(savedSettings.enterpriseManaged && {
|
||||||
enterpriseManaged: true,
|
enterpriseManaged: true,
|
||||||
}),
|
}),
|
||||||
|
|
|
||||||
|
|
@ -5436,6 +5436,8 @@ export class InspectorClient extends InspectorClientEventTarget {
|
||||||
clientSecret?: string;
|
clientSecret?: string;
|
||||||
clientMetadataUrl?: string;
|
clientMetadataUrl?: string;
|
||||||
scope?: string;
|
scope?: string;
|
||||||
|
/** Custom authorization-request parameters (#2018). Authorize URL only. */
|
||||||
|
authorizationParams?: Record<string, string>;
|
||||||
}): void {
|
}): void {
|
||||||
if (!this.oauthManager) {
|
if (!this.oauthManager) {
|
||||||
throw new Error(
|
throw new Error(
|
||||||
|
|
|
||||||
Some files were not shown because too many files have changed in this diff Show more
Loading…
Reference in a new issue