Addressed all Claude review issues.
This commit is contained in:
parent
c04cd9958d
commit
446a9bc1cf
9 changed files with 228 additions and 58 deletions
|
|
@ -3,13 +3,21 @@
|
||||||
* Spawns the server and hits it like any other HTTP client would.
|
* Spawns the server and hits it like any other HTTP client would.
|
||||||
*/
|
*/
|
||||||
import { spawn, type ChildProcess } from "child_process";
|
import { spawn, type ChildProcess } from "child_process";
|
||||||
import { createServer, type Server } from "http";
|
import {
|
||||||
|
createServer,
|
||||||
|
type IncomingMessage,
|
||||||
|
type Server,
|
||||||
|
type ServerResponse,
|
||||||
|
} from "http";
|
||||||
import { resolve } from "path";
|
import { resolve } from "path";
|
||||||
|
|
||||||
const TEST_PORT = 16321;
|
const TEST_PORT = 16321;
|
||||||
const TEST_TOKEN = "test-proxy-token-12345";
|
const TEST_TOKEN = "test-proxy-token-12345";
|
||||||
const SERVER_PATH = resolve(__dirname, "../../../server/build/index.js");
|
const SERVER_PATH = resolve(__dirname, "../../../server/build/index.js");
|
||||||
|
|
||||||
|
/** Placeholder URL for tests where auth fails before the proxy fetches (no network). */
|
||||||
|
const UNUSED_UPSTREAM_URL = "http://127.0.0.1:1/unused";
|
||||||
|
|
||||||
async function waitForServer(baseUrl: string, maxWaitMs = 5000): Promise<void> {
|
async function waitForServer(baseUrl: string, maxWaitMs = 5000): Promise<void> {
|
||||||
const start = Date.now();
|
const start = Date.now();
|
||||||
while (Date.now() - start < maxWaitMs) {
|
while (Date.now() - start < maxWaitMs) {
|
||||||
|
|
@ -23,6 +31,36 @@ async function waitForServer(baseUrl: string, maxWaitMs = 5000): Promise<void> {
|
||||||
throw new Error("Server did not become ready");
|
throw new Error("Server did not become ready");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Runs `fn` with a local HTTP server on 127.0.0.1:ephemeral-port.
|
||||||
|
* `origin` is `http://127.0.0.1:<port>` (no trailing path).
|
||||||
|
*/
|
||||||
|
async function withLocalUpstream(
|
||||||
|
onRequest: (req: IncomingMessage, res: ServerResponse) => void,
|
||||||
|
fn: (origin: string) => Promise<void>,
|
||||||
|
): Promise<void> {
|
||||||
|
const upstream: Server = createServer(onRequest);
|
||||||
|
|
||||||
|
await new Promise<void>((resolve, reject) => {
|
||||||
|
upstream.once("error", reject);
|
||||||
|
upstream.listen(0, "127.0.0.1", () => resolve());
|
||||||
|
});
|
||||||
|
|
||||||
|
const addr = upstream.address();
|
||||||
|
if (!addr || typeof addr === "string") {
|
||||||
|
upstream.close();
|
||||||
|
throw new Error("Expected TCP listen address");
|
||||||
|
}
|
||||||
|
|
||||||
|
const origin = `http://127.0.0.1:${addr.port}`;
|
||||||
|
|
||||||
|
try {
|
||||||
|
await fn(origin);
|
||||||
|
} finally {
|
||||||
|
await new Promise<void>((r) => upstream.close(() => r()));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
describe("POST /fetch endpoint", () => {
|
describe("POST /fetch endpoint", () => {
|
||||||
let server: ChildProcess;
|
let server: ChildProcess;
|
||||||
const baseUrl = `http://localhost:${TEST_PORT}`;
|
const baseUrl = `http://localhost:${TEST_PORT}`;
|
||||||
|
|
@ -48,7 +86,7 @@ describe("POST /fetch endpoint", () => {
|
||||||
method: "POST",
|
method: "POST",
|
||||||
headers: { "Content-Type": "application/json" },
|
headers: { "Content-Type": "application/json" },
|
||||||
body: JSON.stringify({
|
body: JSON.stringify({
|
||||||
url: "http://example.com/",
|
url: UNUSED_UPSTREAM_URL,
|
||||||
init: { method: "GET" },
|
init: { method: "GET" },
|
||||||
}),
|
}),
|
||||||
});
|
});
|
||||||
|
|
@ -65,14 +103,14 @@ describe("POST /fetch endpoint", () => {
|
||||||
"X-MCP-Proxy-Auth": "Bearer wrong-token",
|
"X-MCP-Proxy-Auth": "Bearer wrong-token",
|
||||||
},
|
},
|
||||||
body: JSON.stringify({
|
body: JSON.stringify({
|
||||||
url: "http://example.com/",
|
url: UNUSED_UPSTREAM_URL,
|
||||||
init: { method: "GET" },
|
init: { method: "GET" },
|
||||||
}),
|
}),
|
||||||
});
|
});
|
||||||
expect(res.status).toBe(401);
|
expect(res.status).toBe(401);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("forwards request when auth token is valid", async () => {
|
it("returns 400 for non-http(s) URL when auth token is valid", async () => {
|
||||||
const res = await fetch(`${baseUrl}/fetch`, {
|
const res = await fetch(`${baseUrl}/fetch`, {
|
||||||
method: "POST",
|
method: "POST",
|
||||||
headers: {
|
headers: {
|
||||||
|
|
@ -80,59 +118,117 @@ describe("POST /fetch endpoint", () => {
|
||||||
"X-MCP-Proxy-Auth": `Bearer ${TEST_TOKEN}`,
|
"X-MCP-Proxy-Auth": `Bearer ${TEST_TOKEN}`,
|
||||||
},
|
},
|
||||||
body: JSON.stringify({
|
body: JSON.stringify({
|
||||||
url: "http://example.com/",
|
url: "file:///etc/passwd",
|
||||||
init: { method: "GET" },
|
init: { method: "GET" },
|
||||||
}),
|
}),
|
||||||
});
|
});
|
||||||
expect(res.status).toBe(200);
|
expect(res.status).toBe(400);
|
||||||
const body = await res.json();
|
const body = (await res.json()) as { error: string };
|
||||||
expect(body.ok).toBe(true);
|
expect(body.error).toBe("Only http/https URLs are allowed");
|
||||||
expect(body.status).toBe(200);
|
});
|
||||||
expect(body.body).toBeDefined();
|
|
||||||
|
it("returns 400 for invalid URL string when auth token is valid", async () => {
|
||||||
|
const res = await fetch(`${baseUrl}/fetch`, {
|
||||||
|
method: "POST",
|
||||||
|
headers: {
|
||||||
|
"Content-Type": "application/json",
|
||||||
|
"X-MCP-Proxy-Auth": `Bearer ${TEST_TOKEN}`,
|
||||||
|
},
|
||||||
|
body: JSON.stringify({
|
||||||
|
url: "not a valid url",
|
||||||
|
init: { method: "GET" },
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
const body = (await res.json()) as { error: string };
|
||||||
|
expect(body.error).toBe("Invalid URL");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns 400 when url is missing when auth token is valid", async () => {
|
||||||
|
const res = await fetch(`${baseUrl}/fetch`, {
|
||||||
|
method: "POST",
|
||||||
|
headers: {
|
||||||
|
"Content-Type": "application/json",
|
||||||
|
"X-MCP-Proxy-Auth": `Bearer ${TEST_TOKEN}`,
|
||||||
|
},
|
||||||
|
body: JSON.stringify({ init: { method: "GET" } }),
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
const body = (await res.json()) as { error: string };
|
||||||
|
expect(body.error).toBe("Missing or invalid url");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("forwards request when auth token is valid", async () => {
|
||||||
|
const upstreamPayload = JSON.stringify({ hello: "proxy-fetch-test" });
|
||||||
|
|
||||||
|
await withLocalUpstream(
|
||||||
|
(req, res) => {
|
||||||
|
res.writeHead(200, { "Content-Type": "application/json" });
|
||||||
|
res.end(upstreamPayload);
|
||||||
|
},
|
||||||
|
async (origin) => {
|
||||||
|
const upstreamUrl = `${origin}/ok`;
|
||||||
|
|
||||||
|
const res = await fetch(`${baseUrl}/fetch`, {
|
||||||
|
method: "POST",
|
||||||
|
headers: {
|
||||||
|
"Content-Type": "application/json",
|
||||||
|
"X-MCP-Proxy-Auth": `Bearer ${TEST_TOKEN}`,
|
||||||
|
},
|
||||||
|
body: JSON.stringify({
|
||||||
|
url: upstreamUrl,
|
||||||
|
init: { method: "GET" },
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
const body = (await res.json()) as {
|
||||||
|
ok: boolean;
|
||||||
|
status: number;
|
||||||
|
statusText: string;
|
||||||
|
body: string;
|
||||||
|
headers: Record<string, string>;
|
||||||
|
};
|
||||||
|
expect(body.ok).toBe(true);
|
||||||
|
expect(body.status).toBe(200);
|
||||||
|
expect(body.statusText).toBe("OK");
|
||||||
|
expect(body.body).toBe(upstreamPayload);
|
||||||
|
expect(body.headers["content-type"]).toMatch(/application\/json/i);
|
||||||
|
},
|
||||||
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("mirrors upstream 404 (non-2xx) when auth token is valid", async () => {
|
it("mirrors upstream 404 (non-2xx) when auth token is valid", async () => {
|
||||||
const upstream: Server = createServer((req, res) => {
|
await withLocalUpstream(
|
||||||
res.writeHead(404, { "Content-Type": "application/json" });
|
(req, res) => {
|
||||||
res.end('{"error":"not_found"}');
|
res.writeHead(404, { "Content-Type": "application/json" });
|
||||||
});
|
res.end('{"error":"not_found"}');
|
||||||
|
},
|
||||||
|
async (origin) => {
|
||||||
|
const upstreamUrl = `${origin}/missing`;
|
||||||
|
|
||||||
await new Promise<void>((resolve, reject) => {
|
const res = await fetch(`${baseUrl}/fetch`, {
|
||||||
upstream.once("error", reject);
|
method: "POST",
|
||||||
upstream.listen(0, "127.0.0.1", () => resolve());
|
headers: {
|
||||||
});
|
"Content-Type": "application/json",
|
||||||
|
"X-MCP-Proxy-Auth": `Bearer ${TEST_TOKEN}`,
|
||||||
|
},
|
||||||
|
body: JSON.stringify({
|
||||||
|
url: upstreamUrl,
|
||||||
|
init: { method: "GET" },
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
const addr = upstream.address();
|
expect(res.status).toBe(404);
|
||||||
if (!addr || typeof addr === "string") {
|
const body = (await res.json()) as {
|
||||||
upstream.close();
|
ok: boolean;
|
||||||
throw new Error("Expected TCP listen address");
|
status: number;
|
||||||
}
|
body: string;
|
||||||
const upstreamUrl = `http://127.0.0.1:${addr.port}/missing`;
|
};
|
||||||
|
expect(body.ok).toBe(false);
|
||||||
try {
|
expect(body.status).toBe(404);
|
||||||
const res = await fetch(`${baseUrl}/fetch`, {
|
expect(JSON.parse(body.body)).toEqual({ error: "not_found" });
|
||||||
method: "POST",
|
},
|
||||||
headers: {
|
);
|
||||||
"Content-Type": "application/json",
|
|
||||||
"X-MCP-Proxy-Auth": `Bearer ${TEST_TOKEN}`,
|
|
||||||
},
|
|
||||||
body: JSON.stringify({
|
|
||||||
url: upstreamUrl,
|
|
||||||
init: { method: "GET" },
|
|
||||||
}),
|
|
||||||
});
|
|
||||||
|
|
||||||
expect(res.status).toBe(404);
|
|
||||||
const body = (await res.json()) as {
|
|
||||||
ok: boolean;
|
|
||||||
status: number;
|
|
||||||
body: string;
|
|
||||||
};
|
|
||||||
expect(body.ok).toBe(false);
|
|
||||||
expect(body.status).toBe(404);
|
|
||||||
expect(JSON.parse(body.body)).toEqual({ error: "not_found" });
|
|
||||||
} finally {
|
|
||||||
await new Promise<void>((r) => upstream.close(() => r()));
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -108,8 +108,13 @@ const AuthDebugger = ({
|
||||||
});
|
});
|
||||||
}, [serverUrl, updateAuthState]);
|
}, [serverUrl, updateAuthState]);
|
||||||
|
|
||||||
const fetchFn =
|
const fetchFn = useMemo(
|
||||||
connectionType === "proxy" && config ? createProxyFetch(config) : undefined;
|
() =>
|
||||||
|
connectionType === "proxy" && config
|
||||||
|
? createProxyFetch(config)
|
||||||
|
: undefined,
|
||||||
|
[connectionType, config],
|
||||||
|
);
|
||||||
|
|
||||||
const stateMachine = useMemo(
|
const stateMachine = useMemo(
|
||||||
() => new OAuthStateMachine(serverUrl, updateAuthState, fetchFn),
|
() => new OAuthStateMachine(serverUrl, updateAuthState, fetchFn),
|
||||||
|
|
|
||||||
24
client/src/lib/__tests__/mcpProxyTransportErrorCode.test.ts
Normal file
24
client/src/lib/__tests__/mcpProxyTransportErrorCode.test.ts
Normal file
|
|
@ -0,0 +1,24 @@
|
||||||
|
/**
|
||||||
|
* Compare client vs server numeric literal without importing server/mcpProxy.ts:
|
||||||
|
* that module pulls in the SDK graph and breaks Jest (e.g. optional peer deps).
|
||||||
|
*/
|
||||||
|
import { readFileSync } from "fs";
|
||||||
|
import { resolve } from "path";
|
||||||
|
import { MCP_PROXY_TRANSPORT_ERROR_CODE as clientCode } from "../constants";
|
||||||
|
|
||||||
|
const EXPORT_RE =
|
||||||
|
/export const MCP_PROXY_TRANSPORT_ERROR_CODE\s*=\s*(-?\d+)\s*;/;
|
||||||
|
|
||||||
|
describe("MCP_PROXY_TRANSPORT_ERROR_CODE", () => {
|
||||||
|
it("matches server/src/mcpProxy.ts (avoid silent drift between client and server)", () => {
|
||||||
|
const serverSrcPath = resolve(
|
||||||
|
__dirname,
|
||||||
|
"../../../../server/src/mcpProxy.ts",
|
||||||
|
);
|
||||||
|
const serverSrc = readFileSync(serverSrcPath, "utf-8");
|
||||||
|
const match = serverSrc.match(EXPORT_RE);
|
||||||
|
expect(match).not.toBeNull();
|
||||||
|
const serverCode = Number(match![1]);
|
||||||
|
expect(clientCode).toBe(serverCode);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
@ -221,4 +221,24 @@ describe("createProxyFetch", () => {
|
||||||
const callBody = JSON.parse(mockFetch.mock.calls[0][1].body);
|
const callBody = JSON.parse(mockFetch.mock.calls[0][1].body);
|
||||||
expect(callBody.url).toBe("https://example.com/discovery");
|
expect(callBody.url).toBe("https://example.com/discovery");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("uses Request.url when input is a Request (not [object Request])", async () => {
|
||||||
|
mockFetch.mockResolvedValue({
|
||||||
|
ok: true,
|
||||||
|
json: () =>
|
||||||
|
Promise.resolve({
|
||||||
|
ok: true,
|
||||||
|
status: 200,
|
||||||
|
statusText: "OK",
|
||||||
|
headers: {},
|
||||||
|
body: "",
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
|
const fetchFn = createProxyFetch(configWithProxy);
|
||||||
|
await fetchFn(new Request("https://example.com/from-request"));
|
||||||
|
|
||||||
|
const callBody = JSON.parse(mockFetch.mock.calls[0][1].body);
|
||||||
|
expect(callBody.url).toBe("https://example.com/from-request");
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -41,7 +41,8 @@ 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.
|
* 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
|
* **-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
|
* (see comment on `server/src/mcpProxy.ts` `MCP_PROXY_TRANSPORT_ERROR_CODE`). Duplicated like
|
||||||
* `DEFAULT_MCP_PROXY_LISTEN_PORT`.
|
* `DEFAULT_MCP_PROXY_LISTEN_PORT`. Keep in sync with the server value;
|
||||||
|
* `src/lib/__tests__/mcpProxyTransportErrorCode.test.ts` fails if they drift.
|
||||||
*/
|
*/
|
||||||
export const MCP_PROXY_TRANSPORT_ERROR_CODE = -32099;
|
export const MCP_PROXY_TRANSPORT_ERROR_CODE = -32099;
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -379,8 +379,6 @@ export function useConnection({
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
const is401Error = isConnectionAuthError;
|
|
||||||
|
|
||||||
const isProxyAuthError = (error: unknown): boolean => {
|
const isProxyAuthError = (error: unknown): boolean => {
|
||||||
return (
|
return (
|
||||||
error instanceof Error &&
|
error instanceof Error &&
|
||||||
|
|
@ -389,7 +387,7 @@ export function useConnection({
|
||||||
};
|
};
|
||||||
|
|
||||||
const handleAuthError = async (error: unknown) => {
|
const handleAuthError = async (error: unknown) => {
|
||||||
if (is401Error(error)) {
|
if (isConnectionAuthError(error)) {
|
||||||
let scope = oauthScope?.trim();
|
let scope = oauthScope?.trim();
|
||||||
const fetchFn =
|
const fetchFn =
|
||||||
connectionType === "proxy" ? createProxyFetch(config) : undefined;
|
connectionType === "proxy" ? createProxyFetch(config) : undefined;
|
||||||
|
|
@ -820,7 +818,7 @@ export function useConnection({
|
||||||
if (shouldRetry) {
|
if (shouldRetry) {
|
||||||
return connect(undefined, retryCount + 1);
|
return connect(undefined, retryCount + 1);
|
||||||
}
|
}
|
||||||
if (is401Error(error)) {
|
if (isConnectionAuthError(error)) {
|
||||||
// Don't set error state if we're about to redirect for auth
|
// Don't set error state if we're about to redirect for auth
|
||||||
|
|
||||||
return;
|
return;
|
||||||
|
|
|
||||||
|
|
@ -92,7 +92,12 @@ export function createProxyFetch(config: InspectorConfig): typeof fetch {
|
||||||
input: RequestInfo | URL,
|
input: RequestInfo | URL,
|
||||||
init?: RequestInit,
|
init?: RequestInit,
|
||||||
): Promise<Response> => {
|
): Promise<Response> => {
|
||||||
const url = typeof input === "string" ? input : input.toString();
|
const url =
|
||||||
|
typeof input === "string"
|
||||||
|
? input
|
||||||
|
: input instanceof Request
|
||||||
|
? input.url
|
||||||
|
: input.toString();
|
||||||
|
|
||||||
// Serialize body for JSON transport. URLSearchParams and similar don't
|
// Serialize body for JSON transport. URLSearchParams and similar don't
|
||||||
// JSON-serialize (they become {}), so we must convert to string first.
|
// JSON-serialize (they become {}), so we must convert to string first.
|
||||||
|
|
|
||||||
|
|
@ -844,6 +844,24 @@ app.post(
|
||||||
try {
|
try {
|
||||||
const { url, init } = req.body as { url: string; init?: RequestInit };
|
const { url, init } = req.body as { url: string; init?: RequestInit };
|
||||||
|
|
||||||
|
if (typeof url !== "string" || url.length === 0) {
|
||||||
|
res.status(400).json({ error: "Missing or invalid url" });
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
let parsedUrl: URL;
|
||||||
|
try {
|
||||||
|
parsedUrl = new URL(url);
|
||||||
|
} catch {
|
||||||
|
res.status(400).json({ error: "Invalid URL" });
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!["http:", "https:"].includes(parsedUrl.protocol)) {
|
||||||
|
res.status(400).json({ error: "Only http/https URLs are allowed" });
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
const response = await fetch(url, {
|
const response = await fetch(url, {
|
||||||
method: init?.method ?? "GET",
|
method: init?.method ?? "GET",
|
||||||
headers: (init?.headers as Record<string, string>) ?? {},
|
headers: (init?.headers as Record<string, string>) ?? {},
|
||||||
|
|
|
||||||
|
|
@ -12,6 +12,9 @@ import { isJSONRPCRequest } from "@modelcontextprotocol/sdk/types.js";
|
||||||
* the band are assigned over time — e.g. **-32002** is used for **ResourceNotFound** in parts of
|
* the band are assigned over time — e.g. **-32002** is used for **ResourceNotFound** in parts of
|
||||||
* the MCP ecosystem). We use **-32099** so this inspector-only bridge error stays clear of those
|
* the MCP ecosystem). We use **-32099** so this inspector-only bridge error stays clear of those
|
||||||
* registered meanings; the client still keys off `error.data` shape, not the number alone.
|
* registered meanings; the client still keys off `error.data` shape, not the number alone.
|
||||||
|
*
|
||||||
|
* Keep in sync with `client/src/lib/constants.ts`; drift is caught by
|
||||||
|
* `client/src/lib/__tests__/mcpProxyTransportErrorCode.test.ts`.
|
||||||
*/
|
*/
|
||||||
export const MCP_PROXY_TRANSPORT_ERROR_CODE = -32099;
|
export const MCP_PROXY_TRANSPORT_ERROR_CODE = -32099;
|
||||||
|
|
||||||
|
|
@ -62,8 +65,8 @@ export function serializeProxyTransportError(
|
||||||
} else if (error instanceof SseError) {
|
} else if (error instanceof SseError) {
|
||||||
if (error.code !== undefined) attachHttpStatusIfValid(error.code);
|
if (error.code !== undefined) attachHttpStatusIfValid(error.code);
|
||||||
}
|
}
|
||||||
const u = headerHolder?.lastUpstream401;
|
if (headerHolder?.lastUpstream401) {
|
||||||
if (u && headerHolder) {
|
const u = headerHolder.lastUpstream401;
|
||||||
data.upstream401 = {
|
data.upstream401 = {
|
||||||
wwwAuthenticate: u.wwwAuthenticate,
|
wwwAuthenticate: u.wwwAuthenticate,
|
||||||
body: u.body,
|
body: u.body,
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue