Fix for 401 errors not being sent back through the proxy (and relying on error string text matching to detect an OAuth 401)
This commit is contained in:
parent
c344ffb87c
commit
6e796ac744
4 changed files with 290 additions and 38 deletions
41
client/src/lib/connectionAuthErrors.ts
Normal file
41
client/src/lib/connectionAuthErrors.ts
Normal file
|
|
@ -0,0 +1,41 @@
|
|||
import { SseError } from "@modelcontextprotocol/sdk/client/sse.js";
|
||||
import { StreamableHTTPError } from "@modelcontextprotocol/sdk/client/streamableHttp.js";
|
||||
import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js";
|
||||
import { McpError } from "@modelcontextprotocol/sdk/types.js";
|
||||
import { MCP_PROXY_TRANSPORT_ERROR_CODE } from "./constants";
|
||||
|
||||
function isPlainObject(value: unknown): value is Record<string, unknown> {
|
||||
return typeof value === "object" && value !== null && !Array.isArray(value);
|
||||
}
|
||||
|
||||
/** `McpError.data` from server `mcpProxy` `serializeProxyTransportError`. */
|
||||
export function mcpProxyTransportErrorDataIndicatesUnauthorized(
|
||||
data: Record<string, unknown>,
|
||||
): boolean {
|
||||
if ("upstream401" in data) {
|
||||
const snapshot = data.upstream401;
|
||||
if (snapshot != null) return true;
|
||||
}
|
||||
const status = data.httpStatus;
|
||||
return typeof status === "number" && status === 401;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether `handleAuthError` / OAuth recovery should run for this failure.
|
||||
*/
|
||||
export function isConnectionAuthError(error: unknown): boolean {
|
||||
if (error instanceof SseError && error.code === 401) return true;
|
||||
if (error instanceof StreamableHTTPError && error.code === 401) return true;
|
||||
if (error instanceof UnauthorizedError) return true;
|
||||
|
||||
if (
|
||||
error instanceof McpError &&
|
||||
error.code === MCP_PROXY_TRANSPORT_ERROR_CODE &&
|
||||
isPlainObject(error.data) &&
|
||||
mcpProxyTransportErrorDataIndicatesUnauthorized(error.data)
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
|
@ -37,6 +37,14 @@ export type ConnectionStatus =
|
|||
|
||||
export const DEFAULT_MCP_PROXY_LISTEN_PORT = "6277";
|
||||
|
||||
/**
|
||||
* JSON-RPC error code when the inspector proxy cannot complete a forward to the upstream MCP server.
|
||||
* **-32099** — high end of JSON-RPC’s -32000..-32099 server-error band to avoid MCP-assigned codes
|
||||
* (see comment on `server/src/mcpProxy.ts` `MCP_PROXY_TRANSPORT_ERROR_CODE`). Duplicated like
|
||||
* `DEFAULT_MCP_PROXY_LISTEN_PORT`.
|
||||
*/
|
||||
export const MCP_PROXY_TRANSPORT_ERROR_CODE = -32099;
|
||||
|
||||
/**
|
||||
* Default configuration for the MCP Inspector, Currently persisted in local_storage in the Browser.
|
||||
* Future plans: Provide json config file + Browser local_storage to override default values
|
||||
|
|
|
|||
|
|
@ -5,12 +5,17 @@ import {
|
|||
ClientRequest,
|
||||
CreateTaskResultSchema,
|
||||
JSONRPCMessage,
|
||||
McpError,
|
||||
} from "@modelcontextprotocol/sdk/types.js";
|
||||
import type {
|
||||
AnySchema,
|
||||
SchemaOutput,
|
||||
} from "@modelcontextprotocol/sdk/server/zod-compat.js";
|
||||
import { DEFAULT_INSPECTOR_CONFIG, CLIENT_IDENTITY } from "../../constants";
|
||||
import {
|
||||
DEFAULT_INSPECTOR_CONFIG,
|
||||
CLIENT_IDENTITY,
|
||||
MCP_PROXY_TRANSPORT_ERROR_CODE,
|
||||
} from "../../constants";
|
||||
import {
|
||||
SSEClientTransportOptions,
|
||||
SseError,
|
||||
|
|
@ -94,17 +99,38 @@ jest.mock("@modelcontextprotocol/sdk/client/sse.js", () => {
|
|||
};
|
||||
});
|
||||
|
||||
jest.mock("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({
|
||||
StreamableHTTPClientTransport: jest.fn((url, options) => {
|
||||
mockStreamableHTTPTransport.url = url;
|
||||
mockStreamableHTTPTransport.options = options;
|
||||
return mockStreamableHTTPTransport;
|
||||
}),
|
||||
}));
|
||||
jest.mock("@modelcontextprotocol/sdk/client/streamableHttp.js", () => {
|
||||
class StreamableHTTPError extends Error {
|
||||
code: number;
|
||||
constructor(code: number, message: string) {
|
||||
super(`Streamable HTTP error: ${message}`);
|
||||
this.code = code;
|
||||
}
|
||||
}
|
||||
|
||||
jest.mock("@modelcontextprotocol/sdk/client/auth.js", () => ({
|
||||
auth: jest.fn().mockResolvedValue("AUTHORIZED"),
|
||||
}));
|
||||
return {
|
||||
StreamableHTTPError,
|
||||
StreamableHTTPClientTransport: jest.fn((url, options) => {
|
||||
mockStreamableHTTPTransport.url = url;
|
||||
mockStreamableHTTPTransport.options = options;
|
||||
return mockStreamableHTTPTransport;
|
||||
}),
|
||||
};
|
||||
});
|
||||
|
||||
jest.mock("@modelcontextprotocol/sdk/client/auth.js", () => {
|
||||
class UnauthorizedError extends Error {
|
||||
constructor(message?: string) {
|
||||
super(message ?? "Unauthorized");
|
||||
this.name = "UnauthorizedError";
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
UnauthorizedError,
|
||||
auth: jest.fn().mockResolvedValue("AUTHORIZED"),
|
||||
};
|
||||
});
|
||||
|
||||
// Mock the toast hook
|
||||
const mockToast = jest.fn();
|
||||
|
|
@ -1576,6 +1602,71 @@ describe("useConnection", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("Inspector proxy McpError auth recovery", () => {
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
mockAuth.mockResolvedValue("AUTHORIZED");
|
||||
mockDiscoverScopes.mockResolvedValue(undefined);
|
||||
mockClient.connect.mockResolvedValue(undefined);
|
||||
});
|
||||
|
||||
const attemptConnect = async (
|
||||
props: Parameters<typeof useConnection>[0] = defaultProps,
|
||||
) => {
|
||||
const { result } = renderHook(() => useConnection(props));
|
||||
await act(async () => {
|
||||
try {
|
||||
await result.current.connect();
|
||||
} catch {
|
||||
// connect may throw when auth recovery does not retry
|
||||
}
|
||||
});
|
||||
};
|
||||
|
||||
it("invokes auth when connect fails with inspector proxy transport McpError and upstream401 data", async () => {
|
||||
mockClient.connect.mockRejectedValueOnce(
|
||||
new McpError(MCP_PROXY_TRANSPORT_ERROR_CODE, "proxy transport", {
|
||||
upstream401: { body: "{}", contentType: "application/json" },
|
||||
}),
|
||||
);
|
||||
await attemptConnect();
|
||||
expect(mockAuth).toHaveBeenCalledWith(
|
||||
expect.any(Object),
|
||||
expect.objectContaining({
|
||||
serverUrl: defaultProps.sseUrl,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("invokes auth when connect fails with inspector proxy transport McpError and httpStatus 401", async () => {
|
||||
mockClient.connect.mockRejectedValueOnce(
|
||||
new McpError(MCP_PROXY_TRANSPORT_ERROR_CODE, "proxy transport", {
|
||||
httpStatus: 401,
|
||||
}),
|
||||
);
|
||||
await attemptConnect();
|
||||
expect(mockAuth).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not invoke auth for inspector proxy McpError without auth payload", async () => {
|
||||
mockClient.connect.mockRejectedValueOnce(
|
||||
new McpError(MCP_PROXY_TRANSPORT_ERROR_CODE, "proxy transport", {
|
||||
message: "upstream failure",
|
||||
}),
|
||||
);
|
||||
await attemptConnect();
|
||||
expect(mockAuth).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not invoke auth when httpStatus is 401 but JSON-RPC code is not inspector proxy", async () => {
|
||||
mockClient.connect.mockRejectedValueOnce(
|
||||
new McpError(-32603, "Internal error", { httpStatus: 401 }),
|
||||
);
|
||||
await attemptConnect();
|
||||
expect(mockAuth).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("MCP_PROXY_FULL_ADDRESS Configuration", () => {
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
|
|
|
|||
|
|
@ -1,13 +1,11 @@
|
|||
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
|
||||
import {
|
||||
SSEClientTransport,
|
||||
SseError,
|
||||
SSEClientTransportOptions,
|
||||
} from "@modelcontextprotocol/sdk/client/sse.js";
|
||||
import {
|
||||
StreamableHTTPClientTransport,
|
||||
StreamableHTTPClientTransportOptions,
|
||||
StreamableHTTPError,
|
||||
} from "@modelcontextprotocol/sdk/client/streamableHttp.js";
|
||||
import {
|
||||
ClientNotification,
|
||||
|
|
@ -51,6 +49,7 @@ import { RequestOptions } from "@modelcontextprotocol/sdk/shared/protocol.js";
|
|||
import { useEffect, useRef, useState } from "react";
|
||||
import { useToast } from "@/lib/hooks/useToast";
|
||||
import { ConnectionStatus, CLIENT_IDENTITY } from "../constants";
|
||||
import { isConnectionAuthError } from "../connectionAuthErrors";
|
||||
import { Notification } from "../notificationTypes";
|
||||
import {
|
||||
auth,
|
||||
|
|
@ -380,16 +379,7 @@ export function useConnection({
|
|||
}
|
||||
};
|
||||
|
||||
const is401Error = (error: unknown): boolean => {
|
||||
return (
|
||||
(error instanceof SseError && error.code === 401) ||
|
||||
(error instanceof StreamableHTTPError && error.code === 401) ||
|
||||
(error instanceof Error && error.message.includes("401")) ||
|
||||
(error instanceof Error && error.message.includes("Unauthorized")) ||
|
||||
(error instanceof Error &&
|
||||
error.message.includes("Missing Authorization header"))
|
||||
);
|
||||
};
|
||||
const is401Error = isConnectionAuthError;
|
||||
|
||||
const isProxyAuthError = (error: unknown): boolean => {
|
||||
return (
|
||||
|
|
|
|||
Loading…
Reference in a new issue