chore: merge v2/main into main for the v2.7.0 release
This commit is contained in:
cliffhall
commit
8dedc5fea2
8 changed files with 1168 additions and 3 deletions
|
|
@ -0,0 +1,745 @@
|
|||
import { describe, it, expect, afterEach, beforeEach, vi } from "vitest";
|
||||
import path from "node:path";
|
||||
import type { Request, Response } from "express";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import {
|
||||
withOAuthRequestTimeout,
|
||||
OAuthRequestTimeoutError,
|
||||
} from "@inspector/core/auth/requestTimeout.js";
|
||||
import {
|
||||
createTestServerHttp,
|
||||
type TestServerHttp,
|
||||
createTestServerInfo,
|
||||
createEchoTool,
|
||||
loadConfig,
|
||||
resolveConfig,
|
||||
STALLABLE_OAUTH_ENDPOINTS,
|
||||
type StallableOAuthEndpoint,
|
||||
createOAuthStallMiddleware,
|
||||
createStallRegistry,
|
||||
MAX_STALL_MS,
|
||||
} from "@modelcontextprotocol/inspector-test-server";
|
||||
|
||||
/**
|
||||
* The OAuth-path request timeouts (#2319), driven against a **real, established,
|
||||
* idle socket** rather than a `fetch` stub (#2382).
|
||||
*
|
||||
* #2319 put an `AbortSignal.timeout` on five OAuth calls that previously went
|
||||
* out with no signal at all. Every test of that work injected a stubbed
|
||||
* `fetchFn`, which settles on the *client* side — so the state the issue
|
||||
* actually describes was never reproduced:
|
||||
*
|
||||
* > a stall is invisible in browser devtools […] while a Node-side socket sat
|
||||
* > established and idle.
|
||||
*
|
||||
* These tests use the `oauth.stallEndpoints` fixture capability to accept the
|
||||
* request and withhold the response, so the deadline is the only thing that can
|
||||
* end the call. A regression that dropped the wrapper would hang here until the
|
||||
* suite's own budget killed it, rather than passing against an obliging stub.
|
||||
*/
|
||||
const configsDir = path.resolve(
|
||||
path.dirname(fileURLToPath(import.meta.url)),
|
||||
"../../../../../../test-servers/configs",
|
||||
);
|
||||
|
||||
/** Comfortably under the suite budget, comfortably over a LAN round trip. */
|
||||
const BUDGET_MS = 1_500;
|
||||
|
||||
/** How each endpoint is requested, so every advertised name is really driven. */
|
||||
const ENDPOINTS: Record<
|
||||
StallableOAuthEndpoint,
|
||||
{ pathname: string; init: RequestInit; query?: Record<string, string> }
|
||||
> = {
|
||||
"protected-resource-metadata": {
|
||||
pathname: "/.well-known/oauth-protected-resource",
|
||||
init: { method: "GET" },
|
||||
},
|
||||
"as-metadata": {
|
||||
pathname: "/.well-known/oauth-authorization-server",
|
||||
init: { method: "GET" },
|
||||
},
|
||||
authorize: {
|
||||
pathname: "/oauth/authorize",
|
||||
init: { method: "GET" },
|
||||
// The one endpoint always called WITH a query string — which is what makes
|
||||
// the middleware's `req.path` match load-bearing. Matching `req.url`
|
||||
// instead passes every other case in this file and fails only here.
|
||||
query: {
|
||||
client_id: "test-client",
|
||||
response_type: "code",
|
||||
redirect_uri: "http://127.0.0.1:6274/oauth/callback",
|
||||
},
|
||||
},
|
||||
token: {
|
||||
pathname: "/oauth/token",
|
||||
init: {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/x-www-form-urlencoded" },
|
||||
body: "grant_type=authorization_code&code=irrelevant",
|
||||
},
|
||||
},
|
||||
revoke: {
|
||||
pathname: "/oauth/revoke",
|
||||
init: {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/x-www-form-urlencoded" },
|
||||
body: "token=irrelevant",
|
||||
},
|
||||
},
|
||||
register: {
|
||||
pathname: "/oauth/register",
|
||||
init: {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify({ redirect_uris: ["http://127.0.0.1:6274/cb"] }),
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
/** Wait for `predicate`, rather than sleeping and hoping. */
|
||||
async function until(
|
||||
predicate: () => boolean,
|
||||
what: string,
|
||||
timeoutMs = 5_000,
|
||||
): Promise<void> {
|
||||
const deadline = Date.now() + timeoutMs;
|
||||
while (Date.now() < deadline) {
|
||||
if (predicate()) return;
|
||||
await new Promise((resolve) => setTimeout(resolve, 10));
|
||||
}
|
||||
throw new Error(`timed out waiting for ${what}`);
|
||||
}
|
||||
|
||||
describe("OAuth request timeouts against a stalled endpoint (#2382)", () => {
|
||||
let server: TestServerHttp | null = null;
|
||||
|
||||
afterEach(async () => {
|
||||
if (server) {
|
||||
try {
|
||||
await server.stop();
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
server = null;
|
||||
}
|
||||
});
|
||||
|
||||
function urlFor(base: string, endpoint: StallableOAuthEndpoint): string {
|
||||
const { pathname, query } = ENDPOINTS[endpoint];
|
||||
const url = new URL(pathname, base);
|
||||
for (const [key, value] of Object.entries(query ?? {})) {
|
||||
url.searchParams.set(key, value);
|
||||
}
|
||||
return url.href;
|
||||
}
|
||||
|
||||
async function startStalling(
|
||||
endpoints: StallableOAuthEndpoint[],
|
||||
extra: { stallMs?: number } = {},
|
||||
): Promise<TestServerHttp> {
|
||||
const started = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo("oauth-stall", "1.0.0"),
|
||||
tools: [createEchoTool()],
|
||||
oauth: {
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
requireAuth: true,
|
||||
scopesSupported: ["mcp"],
|
||||
supportDCR: true,
|
||||
stallEndpoints: endpoints,
|
||||
...extra,
|
||||
},
|
||||
});
|
||||
await started.start();
|
||||
server = started;
|
||||
return started;
|
||||
}
|
||||
|
||||
// ⚠️ Every advertised endpoint, not a sample. `stallTargetsFor` hardcodes a
|
||||
// path and a method set per endpoint, so a typo in any entry produces a
|
||||
// fixture that quietly answers normally — the precise failure this option
|
||||
// exists to prevent, and one no other test would catch (Copilot).
|
||||
for (const endpoint of STALLABLE_OAUTH_ENDPOINTS) {
|
||||
it(`stalls the ${endpoint} endpoint until the deadline fires`, async () => {
|
||||
const started = await startStalling([endpoint]);
|
||||
const url = urlFor(started.url, endpoint);
|
||||
const timedFetch = withOAuthRequestTimeout(fetch, BUDGET_MS);
|
||||
const startedAt = Date.now();
|
||||
|
||||
await expect(timedFetch(url, ENDPOINTS[endpoint].init)).rejects.toThrow(
|
||||
OAuthRequestTimeoutError,
|
||||
);
|
||||
|
||||
// The deadline ended it, not an instant connection failure: a refused
|
||||
// port rejects in single-digit ms and would satisfy a bare
|
||||
// `rejects.toThrow` while proving nothing about the timer.
|
||||
expect(Date.now() - startedAt).toBeGreaterThanOrEqual(BUDGET_MS - 100);
|
||||
});
|
||||
|
||||
it(`names the ${endpoint} endpoint and the budget it gave up on`, async () => {
|
||||
const started = await startStalling([endpoint]);
|
||||
const url = urlFor(started.url, endpoint);
|
||||
const timedFetch = withOAuthRequestTimeout(fetch, BUDGET_MS);
|
||||
|
||||
await expect(
|
||||
timedFetch(url, ENDPOINTS[endpoint].init),
|
||||
).rejects.toMatchObject({
|
||||
timeoutMs: BUDGET_MS,
|
||||
url: expect.stringContaining(ENDPOINTS[endpoint].pathname),
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
it("stalls only the selected endpoint, leaving the others answering", async () => {
|
||||
// The control. A fixture that stalled everything would make every test
|
||||
// above pass while telling us nothing about which call was bounded.
|
||||
const started = await startStalling(["token"]);
|
||||
const metadata = await fetch(
|
||||
urlFor(started.url, "protected-resource-metadata"),
|
||||
);
|
||||
|
||||
expect(metadata.ok).toBe(true);
|
||||
await expect(metadata.json()).resolves.toMatchObject({
|
||||
authorization_servers: expect.any(Array),
|
||||
});
|
||||
});
|
||||
|
||||
it("distinguishes two calls that share a path by their method", async () => {
|
||||
// ⚠️ Both configurable document paths are caller-supplied, so a config may
|
||||
// point one at a path another endpoint already serves. Keyed on path alone,
|
||||
// stalling `token` would also stall this GET (Copilot).
|
||||
const started = await startStalling(["token"]);
|
||||
const collided = new URL("/oauth/token", started.url).href;
|
||||
|
||||
// The token POST is stalled…
|
||||
await expect(
|
||||
withOAuthRequestTimeout(fetch, BUDGET_MS)(collided, ENDPOINTS.token.init),
|
||||
).rejects.toThrow(OAuthRequestTimeoutError);
|
||||
|
||||
// …while a GET to the very same path is not held by the stall middleware.
|
||||
// Which status it gets does not matter; that a response arrives at all does.
|
||||
const samePathGet = await fetch(collided, { method: "GET" });
|
||||
expect(typeof samePathGet.status).toBe("number");
|
||||
});
|
||||
|
||||
it("answers late rather than never when stallMs is positive", async () => {
|
||||
// `stallMs` is advertised and was previously untested: every fixture used
|
||||
// 0, so the `setTimeout(() => next())` branch could regress unseen
|
||||
// (Copilot).
|
||||
const delayMs = 600;
|
||||
const started = await startStalling(["protected-resource-metadata"], {
|
||||
stallMs: delayMs,
|
||||
});
|
||||
const startedAt = Date.now();
|
||||
|
||||
const res = await fetch(urlFor(started.url, "protected-resource-metadata"));
|
||||
const elapsed = Date.now() - startedAt;
|
||||
|
||||
expect(res.ok).toBe(true);
|
||||
// Late, but it did arrive — both halves matter.
|
||||
expect(elapsed).toBeGreaterThanOrEqual(delayMs - 50);
|
||||
await expect(res.json()).resolves.toMatchObject({
|
||||
authorization_servers: expect.any(Array),
|
||||
});
|
||||
});
|
||||
|
||||
it("times out when the caller's budget is shorter than stallMs", async () => {
|
||||
const started = await startStalling(["token"], { stallMs: 5_000 });
|
||||
const timedFetch = withOAuthRequestTimeout(fetch, 400);
|
||||
|
||||
await expect(
|
||||
timedFetch(urlFor(started.url, "token"), ENDPOINTS.token.init),
|
||||
).rejects.toThrow(OAuthRequestTimeoutError);
|
||||
});
|
||||
|
||||
it("stops holding the request when the client aborts a delayed stall", async () => {
|
||||
// End-to-end: the fixture lets go. This observes the REGISTRY, which is
|
||||
// decremented by the `close` listener — so it does NOT prove the timer was
|
||||
// cleared. That claim belongs to the focused test below, which can actually
|
||||
// see the timer (Copilot).
|
||||
const started = await startStalling(["token"], { stallMs: 10_000 });
|
||||
const controller = new AbortController();
|
||||
|
||||
const pending = fetch(urlFor(started.url, "token"), {
|
||||
...ENDPOINTS.token.init,
|
||||
signal: controller.signal,
|
||||
}).catch(() => "aborted");
|
||||
|
||||
await until(
|
||||
() => started.stalledRequestCount() >= 1,
|
||||
"the request to be parked",
|
||||
);
|
||||
controller.abort();
|
||||
|
||||
await expect(pending).resolves.toBe("aborted");
|
||||
await until(
|
||||
() => started.stalledRequestCount() === 0,
|
||||
"the stall to be released",
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects an unknown stallEndpoints entry instead of answering normally", async () => {
|
||||
// A typo must fail loudly. Ignored, it would produce a fixture that answers
|
||||
// promptly, and a timeout test written against it would fail pointing at
|
||||
// the timeout rather than at the config.
|
||||
//
|
||||
// The throw lands on `start()`, not on the constructor: routes — and so the
|
||||
// middleware — are built when the server is started. Asserting on the
|
||||
// constructor would pass for the wrong reason, since it validates nothing.
|
||||
const typo = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo("oauth-stall-typo", "1.0.0"),
|
||||
tools: [createEchoTool()],
|
||||
oauth: {
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
// @ts-expect-error - deliberately not a StallableOAuthEndpoint
|
||||
stallEndpoints: ["tokens"],
|
||||
},
|
||||
});
|
||||
|
||||
await expect(typo.start()).rejects.toThrow(
|
||||
/Unknown oauth\.stallEndpoints entry.*"tokens"/s,
|
||||
);
|
||||
// It must also name what WAS valid, or the fixture author is left guessing.
|
||||
await expect(typo.start()).rejects.toThrow(/Expected one of:.*token/s);
|
||||
|
||||
try {
|
||||
await typo.stop();
|
||||
} catch {
|
||||
// never started; nothing to stop
|
||||
}
|
||||
});
|
||||
|
||||
it("stops cleanly with a withheld response still in flight", async () => {
|
||||
// ⚠️ The property that makes this fixture usable at all. A withheld
|
||||
// response holds an established socket, and `stop()` relies on
|
||||
// `httpServer.closeAllConnections?.()` to destroy it. Without that, every
|
||||
// suite touching this fixture would hang at teardown instead of failing —
|
||||
// and it would hang in `afterEach`, pointing at the wrong test.
|
||||
const started = await startStalling(["token"]);
|
||||
|
||||
// Deliberately unawaited and unbounded: the point is that a request with no
|
||||
// deadline of its own is in flight when the server goes down.
|
||||
const pending = fetch(urlFor(started.url, "token"), {
|
||||
...ENDPOINTS.token.init,
|
||||
}).catch(() => "socket destroyed");
|
||||
|
||||
// ⚠️ Wait for the request to be ACCEPTED AND PARKED, never a fixed sleep.
|
||||
// A sleep that lost the race would stop the server before the request
|
||||
// arrived; the fetch would then reject because the server closed, and this
|
||||
// test would pass without ever exercising `closeAllConnections()` on an
|
||||
// established request (Copilot).
|
||||
await until(
|
||||
() => started.stalledRequestCount() >= 1,
|
||||
"the request to be parked",
|
||||
);
|
||||
|
||||
await expect(started.stop()).resolves.toBeUndefined();
|
||||
server = null;
|
||||
|
||||
await expect(pending).resolves.toBe("socket destroyed");
|
||||
});
|
||||
|
||||
it("stalls the authorize POST, not only its GET", async () => {
|
||||
// ⚠️ `authorize` is the only endpoint mapped to two methods, and the table
|
||||
// above drives it as a GET. Without this, deleting `"POST"` from its method
|
||||
// list passes the whole suite while a consent submission stops stalling
|
||||
// (Copilot).
|
||||
const started = await startStalling(["authorize"]);
|
||||
const url = new URL("/oauth/authorize", started.url).href;
|
||||
const timedFetch = withOAuthRequestTimeout(fetch, BUDGET_MS);
|
||||
|
||||
await expect(
|
||||
timedFetch(url, {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/x-www-form-urlencoded" },
|
||||
body: "client_id=test-client&response_type=code&redirect_uri=http%3A%2F%2F127.0.0.1%3A6274%2Foauth%2Fcallback",
|
||||
}),
|
||||
).rejects.toThrow(OAuthRequestTimeoutError);
|
||||
});
|
||||
|
||||
it("follows resourceMetadataPath when the document is moved", async () => {
|
||||
// ⚠️ Both metadata paths are configurable, and the table above only ever
|
||||
// requests the DEFAULTS. A regression replacing either lookup in
|
||||
// `stallTargetsFor` with its default literal would leave every other test
|
||||
// green while `stallEndpoints` silently stopped working for any config that
|
||||
// moves the document (Copilot).
|
||||
const moved = "/custom/protected-resource.json";
|
||||
const started = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo("oauth-stall-moved-prm", "1.0.0"),
|
||||
tools: [createEchoTool()],
|
||||
oauth: {
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
requireAuth: true,
|
||||
scopesSupported: ["mcp"],
|
||||
resourceMetadataPath: moved,
|
||||
stallEndpoints: ["protected-resource-metadata"],
|
||||
},
|
||||
});
|
||||
await started.start();
|
||||
server = started;
|
||||
|
||||
await expect(
|
||||
withOAuthRequestTimeout(
|
||||
fetch,
|
||||
BUDGET_MS,
|
||||
)(new URL(moved, started.url).href),
|
||||
).rejects.toThrow(OAuthRequestTimeoutError);
|
||||
|
||||
// And the DEFAULT path is no longer the stalled one — it is not served at
|
||||
// all once the document moves, so it answers (404) instead of hanging.
|
||||
const atDefault = await fetch(
|
||||
new URL("/.well-known/oauth-protected-resource", started.url).href,
|
||||
);
|
||||
expect(typeof atDefault.status).toBe("number");
|
||||
});
|
||||
|
||||
it("follows asMetadataPath when the AS document is moved", async () => {
|
||||
const moved = "/custom/as-metadata.json";
|
||||
const started = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo("oauth-stall-moved-as", "1.0.0"),
|
||||
tools: [createEchoTool()],
|
||||
oauth: {
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
requireAuth: true,
|
||||
scopesSupported: ["mcp"],
|
||||
asMetadataPath: moved,
|
||||
stallEndpoints: ["as-metadata"],
|
||||
},
|
||||
});
|
||||
await started.start();
|
||||
server = started;
|
||||
|
||||
await expect(
|
||||
withOAuthRequestTimeout(
|
||||
fetch,
|
||||
BUDGET_MS,
|
||||
)(new URL(moved, started.url).href),
|
||||
).rejects.toThrow(OAuthRequestTimeoutError);
|
||||
});
|
||||
|
||||
it("drives the two checked-in showcase configs, not just inline ones", async () => {
|
||||
// The JSON-to-ServerConfig mapping is its own failure surface: a misspelled
|
||||
// key in either file, or a `resolveConfig` that stopped threading
|
||||
// `stallEndpoints`, would leave the documented manual fixtures quietly
|
||||
// permissive while every inline test above stayed green.
|
||||
for (const file of [
|
||||
"oauth-stalled-token-http.json",
|
||||
"oauth-stalled-discovery-http.json",
|
||||
]) {
|
||||
const resolved = resolveConfig(loadConfig(path.join(configsDir, file)));
|
||||
const started = createTestServerHttp(resolved);
|
||||
await started.start();
|
||||
server = started;
|
||||
|
||||
const endpoint: StallableOAuthEndpoint = file.includes("token")
|
||||
? "token"
|
||||
: "protected-resource-metadata";
|
||||
const timedFetch = withOAuthRequestTimeout(fetch, BUDGET_MS);
|
||||
|
||||
await expect(
|
||||
timedFetch(urlFor(started.url, endpoint), ENDPOINTS[endpoint].init),
|
||||
).rejects.toThrow(OAuthRequestTimeoutError);
|
||||
|
||||
await started.stop();
|
||||
server = null;
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* The middleware on its own, with no server, so the timer itself is observable.
|
||||
*
|
||||
* The end-to-end abort test can only watch the registry, which a separate
|
||||
* `close` listener decrements — so it passes with the timer still armed and
|
||||
* cannot support a claim about `clearTimeout` (Copilot). Here `next` is a spy
|
||||
* and time is fake, so "the delayed answer never fires after a close" is
|
||||
* directly checkable.
|
||||
*/
|
||||
describe("createOAuthStallMiddleware timer cleanup (#2382)", () => {
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
/**
|
||||
* Minimal Express doubles, bridged to the real types.
|
||||
*
|
||||
* The middleware touches exactly three things — `req.method`, `req.path` and
|
||||
* `res.on("close", …)` — so a full `Request`/`Response` is unnecessary and a
|
||||
* real server would defeat the purpose (the whole point here is to control
|
||||
* time and observe `next`).
|
||||
*
|
||||
* ⚠️ The bridge is a justified double cast, NOT an `any`: this repo forbids
|
||||
* `any` outright and forbids disabling the rule to satisfy the linter
|
||||
* (`AGENTS.md`, Typescript instructions). A single `as Request` cannot work —
|
||||
* the literal is missing ~50 properties, so TS rejects the conversion — and
|
||||
* these doubles are structurally correct for every field the code under test
|
||||
* reads. If the middleware ever reads more, the double will be wrong at
|
||||
* runtime rather than silently absorbing it, which is what an `any` would do
|
||||
* (Copilot).
|
||||
*/
|
||||
function fakeRes(): { res: Response; close: () => void } {
|
||||
const listeners: Array<() => void> = [];
|
||||
const double = {
|
||||
on(event: string, fn: () => void) {
|
||||
if (event === "close") listeners.push(fn);
|
||||
return double;
|
||||
},
|
||||
};
|
||||
return {
|
||||
res: double as unknown as Response,
|
||||
close: () => listeners.forEach((fn) => fn()),
|
||||
};
|
||||
}
|
||||
|
||||
function fakeReq(method: string, path: string): Request {
|
||||
return { method, path } as unknown as Request;
|
||||
}
|
||||
|
||||
const tokenReq = fakeReq("POST", "/oauth/token");
|
||||
|
||||
it("does not answer a delayed stall after the client closed", () => {
|
||||
const middleware = createOAuthStallMiddleware(
|
||||
{
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
stallMs: 10_000,
|
||||
},
|
||||
createStallRegistry(),
|
||||
);
|
||||
expect(middleware).not.toBeNull();
|
||||
|
||||
const next = vi.fn();
|
||||
const { res, close } = fakeRes();
|
||||
middleware!(tokenReq, res, next);
|
||||
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
close();
|
||||
|
||||
// Past the delay, with the timer cleared: nothing should fire. Without
|
||||
// `clearTimeout` this advances into `next()` and the spy is called.
|
||||
vi.advanceTimersByTime(20_000);
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("still answers a delayed stall that is not closed", () => {
|
||||
// The complement — otherwise a middleware that never calls `next` at all
|
||||
// would satisfy the test above.
|
||||
const middleware = createOAuthStallMiddleware(
|
||||
{
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
stallMs: 10_000,
|
||||
},
|
||||
createStallRegistry(),
|
||||
);
|
||||
const next = vi.fn();
|
||||
const { res } = fakeRes();
|
||||
middleware!(tokenReq, res, next);
|
||||
|
||||
vi.advanceTimersByTime(9_999);
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
vi.advanceTimersByTime(2);
|
||||
expect(next).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("rejects a malformed stallEndpoints container instead of disabling silently", () => {
|
||||
// ⚠️ The container, not just its entries. A config file is cast, so
|
||||
// `stallEndpoints` can arrive as a string or an object with a `length`. A
|
||||
// bare `.length === 0` check accepts `""` and `{ length: 0 }` and returns
|
||||
// "no stalling configured" — silently disabling the very capability this
|
||||
// validation exists to protect (Copilot).
|
||||
for (const stallEndpoints of ["", "token", null, { length: 0 }, 3]) {
|
||||
expect(() =>
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
// @ts-expect-error - a JSON config is cast, so this really can arrive
|
||||
stallEndpoints,
|
||||
}),
|
||||
).toThrow(/oauth\.stallEndpoints must be an array/);
|
||||
}
|
||||
|
||||
// An omitted value is not malformed — it means "no stalling".
|
||||
expect(
|
||||
createOAuthStallMiddleware({ enabled: true, mode: "combined" }),
|
||||
).toBeNull();
|
||||
// Nor is an explicitly empty array.
|
||||
expect(
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: [],
|
||||
}),
|
||||
).toBeNull();
|
||||
});
|
||||
|
||||
it("refuses to stall a route this config does not serve", () => {
|
||||
// ⚠️ The middleware runs BEFORE Express routing, so without this it will
|
||||
// hold a request for an endpoint that would otherwise 404 — turning a
|
||||
// contradictory fixture into a hang that reads as a timeout, and inviting a
|
||||
// test that passes for entirely the wrong reason (Copilot).
|
||||
const cases: Array<[Record<string, unknown>, StallableOAuthEndpoint]> = [
|
||||
// DCR off, but asked to stall the registration endpoint.
|
||||
[{ mode: "combined", supportDCR: false }, "register"],
|
||||
// Revocation explicitly disabled.
|
||||
[{ mode: "combined", supportRevocation: false }, "revoke"],
|
||||
// protected-resource mode serves no local AS routes at all.
|
||||
[
|
||||
{
|
||||
mode: "protected-resource",
|
||||
authorizationServers: ["https://as.example"],
|
||||
},
|
||||
"token",
|
||||
],
|
||||
[
|
||||
{
|
||||
mode: "protected-resource",
|
||||
authorizationServers: ["https://as.example"],
|
||||
},
|
||||
"authorize",
|
||||
],
|
||||
[
|
||||
{
|
||||
mode: "protected-resource",
|
||||
authorizationServers: ["https://as.example"],
|
||||
},
|
||||
"as-metadata",
|
||||
],
|
||||
];
|
||||
|
||||
for (const [oauth, endpoint] of cases) {
|
||||
expect(() =>
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
...oauth,
|
||||
stallEndpoints: [endpoint],
|
||||
}),
|
||||
).toThrow(/does not serve/);
|
||||
}
|
||||
|
||||
// The protected-resource document is served in every mode, so stalling it
|
||||
// is always legitimate — the check must not over-reject.
|
||||
expect(
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "protected-resource",
|
||||
authorizationServers: ["https://as.example"],
|
||||
stallEndpoints: ["protected-resource-metadata"],
|
||||
}),
|
||||
).not.toBeNull();
|
||||
// And the enabled forms are accepted.
|
||||
expect(
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
supportDCR: true,
|
||||
stallEndpoints: ["register", "revoke", "token"],
|
||||
}),
|
||||
).not.toBeNull();
|
||||
});
|
||||
|
||||
it("names the offending value honestly, including NaN and Infinity", () => {
|
||||
// ⚠️ `JSON.stringify(NaN)` is the string "null", so an error built with it
|
||||
// reports a value the author never wrote (Copilot).
|
||||
expect(() =>
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
stallMs: Number.NaN,
|
||||
}),
|
||||
).toThrow(/got NaN/);
|
||||
expect(() =>
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
stallMs: Infinity,
|
||||
}),
|
||||
).toThrow(/got Infinity/);
|
||||
// A string keeps its quotes, so "600" stays distinguishable from 600.
|
||||
expect(() =>
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
// @ts-expect-error - a JSON config is cast, so this really can arrive
|
||||
stallMs: "600",
|
||||
}),
|
||||
).toThrow(/got "600"/);
|
||||
});
|
||||
|
||||
it("rejects an explicit stallMs: null rather than defaulting it to 0", () => {
|
||||
// ⚠️ `?? 0` would turn `null` into a valid 0 and skip every check, silently
|
||||
// producing a permanent stall. Only an OMITTED value gets the default
|
||||
// (Copilot).
|
||||
expect(() =>
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
// @ts-expect-error - a JSON config is cast, so this really can arrive
|
||||
stallMs: null,
|
||||
}),
|
||||
).toThrow(/oauth\.stallMs must be a finite number/);
|
||||
|
||||
// Omitted still means 0 (stall forever), which is the documented default.
|
||||
expect(
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
}),
|
||||
).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects a stallMs that is negative, non-finite or past the timer range", () => {
|
||||
// ⚠️ A JSON/YAML config is only CAST to its interface, so anything can
|
||||
// arrive here. Unvalidated, a negative value makes `stallMs > 0` false and
|
||||
// silently becomes a PERMANENT stall, and a value past the 32-bit timer
|
||||
// range overflows to ~1ms and answers almost immediately — both read as
|
||||
// "the timeout behaved strangely" rather than "the config is wrong"
|
||||
// (Copilot).
|
||||
for (const stallMs of [-1, Number.NaN, Infinity, MAX_STALL_MS + 1]) {
|
||||
expect(() =>
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
stallMs,
|
||||
}),
|
||||
).toThrow(/oauth\.stallMs must be a finite number/);
|
||||
}
|
||||
// A non-numeric value from an unvalidated config file.
|
||||
expect(() =>
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
// @ts-expect-error - a JSON config is cast, so this really can arrive
|
||||
stallMs: "600",
|
||||
}),
|
||||
).toThrow(/oauth\.stallMs must be a finite number/);
|
||||
|
||||
// The boundaries themselves are valid.
|
||||
for (const stallMs of [0, MAX_STALL_MS]) {
|
||||
expect(() =>
|
||||
createOAuthStallMiddleware({
|
||||
enabled: true,
|
||||
mode: "combined",
|
||||
stallEndpoints: ["token"],
|
||||
stallMs,
|
||||
}),
|
||||
).not.toThrow();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
@ -56,6 +56,7 @@ as a missing capability rather than an error.
|
|||
| `oauth-rfc8414-at-oidc-path-http.json` **(legacy era)** | Plain OAuth 2.0 AS metadata served at the OIDC well-known path | [#2172](https://github.com/modelcontextprotocol/inspector/issues/2172) |
|
||||
| `oauth-insecure-token-endpoint-http.json` **(legacy era)** | A token endpoint the SDK refuses to post credentials to | [#2280](https://github.com/modelcontextprotocol/inspector/issues/2280) |
|
||||
| `oauth-cimd-http.json` **(legacy era)** | URL-based client IDs (CIMD / SEP-991), DCR deliberately off | [#2242](https://github.com/modelcontextprotocol/inspector/issues/2242) |
|
||||
| `oauth-stalled-{token,discovery}-http.json` **(legacy era)** | An OAuth endpoint that accepts the request and never answers, so the OAuth-path request timeouts fire against a real idle socket | [#2382](https://github.com/modelcontextprotocol/inspector/issues/2382) |
|
||||
| `logging-{legacy,modern}-http.json` **(era per file)** | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) |
|
||||
| `subscriptions-{legacy,modern}-http.json` **(era per file)** | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) |
|
||||
| `subscriptions-never-acknowledged-http.json` **(modern era)** | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) |
|
||||
|
|
@ -648,6 +649,21 @@ string included — so a server that walked to another port on `EADDRINUSE` stil
|
|||
`client_id` equal to the URL you fetched, and the integration test drives it on a harness-chosen port
|
||||
for exactly that reason. The fixed-port dependency that *does* bite is `redirect_uris`, above.
|
||||
|
||||
## An OAuth endpoint that never answers
|
||||
|
||||
Two OAuth-protected servers (combined AS + resource, DCR, refresh tokens) that are ordinary in every respect but one: a single endpoint **accepts the request and withholds the response forever**. Plain streamable-HTTP — connect with the **default (legacy)** protocol era.
|
||||
|
||||
- `oauth-stalled-token-http.json` stalls `POST /oauth/token`, so the token exchange and the refresh — which share that endpoint — both hang.
|
||||
- `oauth-stalled-discovery-http.json` stalls `GET /.well-known/oauth-protected-resource`, so the stall happens before a client has any authorization server to talk to.
|
||||
|
||||
**What to do.** Connect to either from the Inspector and start an OAuth flow. Each should fail with an `OAuthRequestTimeoutError` naming the endpoint it gave up on and the budget it used, after **30 seconds** — `DEFAULT_OAUTH_REQUEST_TIMEOUT_MS`. That wait is not configurable from the UI or from a config file: `InspectorClient` passes the constant straight into its transport chain and passes no budget at all on the auth chain, so both take the default. Budget it for a full 30s of waiting per attempt. Every *other* endpoint on the same server answers normally, so the failure points at one call rather than at the server.
|
||||
|
||||
**What the broken build did.** Before [#2319](https://github.com/modelcontextprotocol/inspector/issues/2319), every OAuth-path call went out with no `AbortSignal` at all and these fixtures produced an unbounded spinner rather than an error — and a silent one, because a connect-time auth error deliberately holds the connection status at `connecting` on the theory that a redirect is about to end the attempt. The stall is server-side, in the Inspector's own Node process, so browser devtools showed no pending request while a socket sat established and idle.
|
||||
|
||||
**Why the fixture exists at all.** #2319's own tests inject a `fetch` stub, which settles on the client side and cannot produce an established idle socket — the precise state the issue is about. Until [#2382](https://github.com/modelcontextprotocol/inspector/issues/2382) added `oauth.stallEndpoints`, nothing in the repo could make those five timeouts fire.
|
||||
|
||||
**Configuring your own.** Any config's `oauth` block takes `stallEndpoints` — any of `protected-resource-metadata`, `as-metadata`, `authorize`, `token`, `revoke`, `register` — plus an optional `stallMs` to answer *late* instead of never. Endpoints are named by the **call**, not the path, because the token exchange and the refresh share `/oauth/token` and two of the documents sit at configurable paths. An unrecognized name throws when the server starts rather than being ignored, so a typo cannot quietly produce a fixture that answers normally.
|
||||
|
||||
## Revoking tokens on clear (RFC 7009)
|
||||
|
||||
`oauth-revocation-http.json` and `oauth-no-revocation-http.json` are the same OAuth-protected server (combined AS + resource, DCR, refresh tokens) differing in one thing: the first advertises a `revocation_endpoint`, the second advertises none. Plain streamable-HTTP — connect with the **default (legacy)** protocol era.
|
||||
|
|
|
|||
25
test-servers/configs/oauth-stalled-discovery-http.json
Normal file
25
test-servers/configs/oauth-stalled-discovery-http.json
Normal file
|
|
@ -0,0 +1,25 @@
|
|||
{
|
||||
"serverInfo": {
|
||||
"name": "oauth-stalled-discovery",
|
||||
"version": "1.0.0"
|
||||
},
|
||||
"tools": [
|
||||
{
|
||||
"preset": "echo"
|
||||
}
|
||||
],
|
||||
"resources": [],
|
||||
"oauth": {
|
||||
"enabled": true,
|
||||
"mode": "combined",
|
||||
"requireAuth": true,
|
||||
"scopesSupported": ["mcp"],
|
||||
"supportDCR": true,
|
||||
"stallEndpoints": ["protected-resource-metadata"],
|
||||
"stallMs": 0
|
||||
},
|
||||
"transport": {
|
||||
"type": "streamable-http",
|
||||
"port": 8094
|
||||
}
|
||||
}
|
||||
26
test-servers/configs/oauth-stalled-token-http.json
Normal file
26
test-servers/configs/oauth-stalled-token-http.json
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
{
|
||||
"serverInfo": {
|
||||
"name": "oauth-stalled-token",
|
||||
"version": "1.0.0"
|
||||
},
|
||||
"tools": [
|
||||
{
|
||||
"preset": "echo"
|
||||
}
|
||||
],
|
||||
"resources": [],
|
||||
"oauth": {
|
||||
"enabled": true,
|
||||
"mode": "combined",
|
||||
"requireAuth": true,
|
||||
"scopesSupported": ["mcp"],
|
||||
"supportDCR": true,
|
||||
"supportRefreshTokens": true,
|
||||
"stallEndpoints": ["token"],
|
||||
"stallMs": 0
|
||||
},
|
||||
"transport": {
|
||||
"type": "streamable-http",
|
||||
"port": 8093
|
||||
}
|
||||
}
|
||||
|
|
@ -50,6 +50,7 @@ import {
|
|||
wireModernTaskHandlers,
|
||||
} from "./modern-tasks.js";
|
||||
import { SKILLS_EXTENSION_KEY, wireSkillsHandlers } from "./skills.js";
|
||||
import type { StallableOAuthEndpoint } from "./test-server-oauth.js";
|
||||
|
||||
/**
|
||||
* MCP Apps extension id. Hardcoded for the same reason the Inspector's
|
||||
|
|
@ -792,6 +793,29 @@ export interface ServerConfig {
|
|||
*/
|
||||
supportRefreshTokens?: boolean;
|
||||
|
||||
/**
|
||||
* Endpoints that accept the request and then withhold the response, so the
|
||||
* `AbortSignal.timeout` #2319 put on the OAuth path can be driven against a
|
||||
* real, established, idle socket (#2382).
|
||||
*
|
||||
* A `fetch` stub cannot reproduce that state — it settles on the client
|
||||
* side — which is why the five timeouts shipped covered only by unit tests.
|
||||
* Name the *call*, not the path: exchange and refresh share `/oauth/token`,
|
||||
* and two of the documents sit at configurable paths.
|
||||
*
|
||||
* Per endpoint rather than global, so a test can tell "discovery timed out"
|
||||
* from "token exchange timed out" — the distinction the five separate
|
||||
* timeouts exist to make.
|
||||
*/
|
||||
stallEndpoints?: StallableOAuthEndpoint[];
|
||||
|
||||
/**
|
||||
* Answer a stalled endpoint after this many ms instead of never
|
||||
* (default: 0, never). Lets one fixture cover both "slower than the
|
||||
* client's budget" and "no answer at all".
|
||||
*/
|
||||
stallMs?: number;
|
||||
|
||||
/**
|
||||
* Whether to advertise and serve the RFC 7009 `revocation_endpoint`
|
||||
* (default: true). Set to `false` to reproduce an authorization server that
|
||||
|
|
|
|||
|
|
@ -6,7 +6,10 @@
|
|||
import { readFileSync } from "fs";
|
||||
import path from "path";
|
||||
import YAML from "yaml";
|
||||
import { isOriginRelativePath } from "./test-server-oauth.js";
|
||||
import {
|
||||
isOriginRelativePath,
|
||||
type StallableOAuthEndpoint,
|
||||
} from "./test-server-oauth.js";
|
||||
|
||||
export interface PresetRef {
|
||||
preset: string;
|
||||
|
|
@ -58,6 +61,18 @@ export interface ConfigFileOAuth {
|
|||
supportRefreshTokens?: boolean;
|
||||
/** RFC 7009 revocation endpoint; default true (#2144). */
|
||||
supportRevocation?: boolean;
|
||||
/**
|
||||
* Endpoints that accept the request and withhold the response, to drive the
|
||||
* OAuth-path request timeouts against a real socket (#2382). One of
|
||||
* `protected-resource-metadata`, `as-metadata`, `authorize`, `token`,
|
||||
* `revoke`, `register`. An unrecognized name throws at setup.
|
||||
*/
|
||||
stallEndpoints?: StallableOAuthEndpoint[];
|
||||
/**
|
||||
* Answer a stalled endpoint after this many ms instead of never (default 0 =
|
||||
* never). Use it for "slower than the budget"; leave it out for "no answer".
|
||||
*/
|
||||
stallMs?: number;
|
||||
}
|
||||
|
||||
export interface ConfigFile {
|
||||
|
|
|
|||
|
|
@ -22,6 +22,8 @@ import * as crypto from "node:crypto";
|
|||
import type { ServerConfig } from "./test-server-fixtures.js";
|
||||
import {
|
||||
setupOAuthRoutes,
|
||||
createStallRegistry,
|
||||
type StallRegistry,
|
||||
createBearerTokenMiddleware,
|
||||
buildScopeRequirementRegistry,
|
||||
scopeRequirementRegistryHasEntries,
|
||||
|
|
@ -307,6 +309,13 @@ export class TestServerHttp {
|
|||
private readonly configWithCallback: ServerConfig;
|
||||
private readonly serverControl: ServerControl;
|
||||
private _closing = false;
|
||||
/**
|
||||
* Requests currently parked by the OAuth stall middleware (#2382).
|
||||
*
|
||||
* Read through `stalledRequestCount()` so a test can wait for a request to be
|
||||
* accepted and parked rather than sleeping — see `StallRegistry`.
|
||||
*/
|
||||
private readonly stallRegistry: StallRegistry = createStallRegistry();
|
||||
private recordedRequests: RecordedRequest[] = [];
|
||||
private httpServer?: HttpServer;
|
||||
private transport?:
|
||||
|
|
@ -519,7 +528,7 @@ export class TestServerHttp {
|
|||
// Set up OAuth if enabled (BEFORE MCP routes)
|
||||
if (this.config.oauth?.enabled) {
|
||||
// We need baseUrl, but it's not set yet - we'll set it after server starts
|
||||
setupOAuthRoutes(app, this.config.oauth);
|
||||
setupOAuthRoutes(app, this.config.oauth, this.stallRegistry);
|
||||
}
|
||||
|
||||
// Bearer token middleware for MCP routes if requireAuth
|
||||
|
|
@ -668,7 +677,7 @@ export class TestServerHttp {
|
|||
// But the routes use relative paths, so they should work regardless
|
||||
if (this.config.oauth?.enabled) {
|
||||
// Use placeholder URL - actual baseUrl will be set after server starts
|
||||
setupOAuthRoutes(app, this.config.oauth);
|
||||
setupOAuthRoutes(app, this.config.oauth, this.stallRegistry);
|
||||
}
|
||||
|
||||
// Bearer token middleware for SSE routes if requireAuth
|
||||
|
|
@ -740,6 +749,17 @@ export class TestServerHttp {
|
|||
return this.listen(port);
|
||||
}
|
||||
|
||||
/**
|
||||
* How many requests this server is currently holding in an OAuth stall.
|
||||
*
|
||||
* On the instance rather than a module export: Vitest can load the fixture
|
||||
* module more than once, and a module-level counter left the middleware
|
||||
* incrementing one copy while the test polled another (#2382).
|
||||
*/
|
||||
stalledRequestCount(): number {
|
||||
return this.stallRegistry.parked;
|
||||
}
|
||||
|
||||
/**
|
||||
* Stop the server. Set closing before closing transport so in-flight tools can skip sending.
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -139,6 +139,293 @@ function bearerChallenge(config: OAuthConfig, req: Request): string {
|
|||
return `Bearer resource_metadata="${new URL(path, requestBaseUrl).href}"`;
|
||||
}
|
||||
|
||||
/**
|
||||
* The OAuth endpoints a fixture can be told to stall.
|
||||
*
|
||||
* One name per call that #2319 put an `AbortSignal.timeout` on, plus
|
||||
* `authorize` for completeness. They are named for the *call* rather than the
|
||||
* path because two of them share `/oauth/token` (exchange and refresh are the
|
||||
* same endpoint with a different `grant_type`), and because the path of the
|
||||
* protected-resource document is itself configurable.
|
||||
*/
|
||||
export const STALLABLE_OAUTH_ENDPOINTS = [
|
||||
"protected-resource-metadata",
|
||||
"as-metadata",
|
||||
"authorize",
|
||||
"token",
|
||||
"revoke",
|
||||
"register",
|
||||
] as const;
|
||||
|
||||
export type StallableOAuthEndpoint = (typeof STALLABLE_OAUTH_ENDPOINTS)[number];
|
||||
|
||||
/**
|
||||
* The largest delay `setTimeout` can actually schedule. Past `2 ** 31 - 1` the
|
||||
* delay overflows a 32-bit signed integer and Node falls back to **1ms**, so an
|
||||
* over-large `stallMs` would produce an *immediate* answer — the opposite of
|
||||
* what the fixture author asked for. Mirrors `MAX_TIMER_DELAY_MS` in
|
||||
* `core/auth/requestTimeout.ts`, which exists for the same reason.
|
||||
*/
|
||||
export const MAX_STALL_MS = 2_147_483_647;
|
||||
|
||||
/**
|
||||
* Render a rejected config value for an error message.
|
||||
*
|
||||
* ⚠️ `JSON.stringify` returns the string `"null"` for `NaN` and `Infinity` — the
|
||||
* two values most likely to reach the `stallMs` check — so an error built with
|
||||
* it names the wrong offending value and sends the reader looking for a `null`
|
||||
* they did not write (Copilot). Strings keep their quotes, which is what makes
|
||||
* `"600"` distinguishable from `600` in the message.
|
||||
*/
|
||||
function describeValue(value: unknown): string {
|
||||
if (typeof value === "number" && !Number.isFinite(value))
|
||||
return String(value);
|
||||
return JSON.stringify(value) ?? String(value);
|
||||
}
|
||||
|
||||
export function isStallableOAuthEndpoint(
|
||||
value: unknown,
|
||||
): value is StallableOAuthEndpoint {
|
||||
return (STALLABLE_OAUTH_ENDPOINTS as readonly unknown[]).includes(value);
|
||||
}
|
||||
|
||||
/** How a stallable endpoint is addressed: its path, and the methods it serves. */
|
||||
export interface StallTarget {
|
||||
path: string;
|
||||
methods: readonly string[];
|
||||
}
|
||||
|
||||
/**
|
||||
* Where each stallable endpoint lives, for this config.
|
||||
*
|
||||
* Two of these are configurable, so the map is built per config rather than
|
||||
* hardcoded: the protected-resource document moves with `resourceMetadataPath`
|
||||
* and the AS metadata with `asMetadataPath`. Getting either wrong would make
|
||||
* the stall silently never match, which is the one failure this fixture must
|
||||
* not have — a test would then read as "the timeout did not fire". Every entry
|
||||
* is covered by a real-request test for exactly that reason.
|
||||
*
|
||||
* ⚠️ **The method is part of the identity, not decoration.** Both configurable
|
||||
* paths are caller-supplied, so a config may legitimately point one of them at
|
||||
* a path another endpoint already uses — `asMetadataPath: "/oauth/token"` is
|
||||
* valid. Keyed on path alone, selecting `token` would then also stall the
|
||||
* metadata GET and selecting `as-metadata` would stall the token POST, which
|
||||
* breaks the per-call contract this option exists to provide (Copilot).
|
||||
*
|
||||
* `authorize` serves both GET (the consent page) and POST (the submission), so
|
||||
* it carries both: stalling "the authorize call" means either direction.
|
||||
*/
|
||||
/**
|
||||
* Does this config actually serve `endpoint`?
|
||||
*
|
||||
* Mirrors the route registration in `setupOAuthRoutes` / `setupMetadataEndpoints`
|
||||
* exactly — the protected-resource document is always served, everything else
|
||||
* on the local AS exists only in `combined` mode, and two routes carry their own
|
||||
* feature flags. Kept adjacent to `stallTargetsFor` so the two stay in step: a
|
||||
* new OAuth route needs an entry in both.
|
||||
*/
|
||||
export function servesEndpoint(
|
||||
config: OAuthConfig,
|
||||
endpoint: StallableOAuthEndpoint,
|
||||
): boolean {
|
||||
const combined = getOAuthMode(config) === "combined";
|
||||
switch (endpoint) {
|
||||
case "protected-resource-metadata":
|
||||
return true;
|
||||
case "as-metadata":
|
||||
case "authorize":
|
||||
case "token":
|
||||
return combined;
|
||||
case "revoke":
|
||||
return combined && config.supportRevocation !== false;
|
||||
case "register":
|
||||
return combined && config.supportDCR === true;
|
||||
}
|
||||
}
|
||||
|
||||
export function stallTargetsFor(
|
||||
config: OAuthConfig,
|
||||
): Record<StallableOAuthEndpoint, StallTarget> {
|
||||
return {
|
||||
"protected-resource-metadata": {
|
||||
path:
|
||||
resourceMetadataPath(config) ?? "/.well-known/oauth-protected-resource",
|
||||
methods: ["GET"],
|
||||
},
|
||||
"as-metadata": { path: asMetadataPath(config), methods: ["GET"] },
|
||||
authorize: { path: "/oauth/authorize", methods: ["GET", "POST"] },
|
||||
token: { path: "/oauth/token", methods: ["POST"] },
|
||||
revoke: { path: "/oauth/revoke", methods: ["POST"] },
|
||||
register: { path: "/oauth/register", methods: ["POST"] },
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* How many requests are currently parked in a stall on one server.
|
||||
*
|
||||
* A test waits on this to know a request was actually **accepted and parked**,
|
||||
* instead of sleeping and hoping. That distinction is the whole point of the
|
||||
* teardown test: a fixed sleep that lost the race would stop the server before
|
||||
* the request arrived, the fetch would then reject because the server closed,
|
||||
* and the test would pass without ever exercising `closeAllConnections()` on an
|
||||
* established request (Copilot).
|
||||
*
|
||||
* ⚠️ **Per server, deliberately not a module-level counter.** A module global
|
||||
* was tried first and failed under Vitest, which can load this module more than
|
||||
* once: the middleware incremented one copy while the test polled another, and
|
||||
* the wait timed out with the fixture working perfectly. Hanging the state off
|
||||
* the server instance the test already holds makes module identity irrelevant —
|
||||
* and scopes the count to one fixture, which is what a caller means anyway.
|
||||
*/
|
||||
export interface StallRegistry {
|
||||
parked: number;
|
||||
}
|
||||
|
||||
export function createStallRegistry(): StallRegistry {
|
||||
return { parked: 0 };
|
||||
}
|
||||
|
||||
/**
|
||||
* Accept a request on a configured endpoint and withhold its response.
|
||||
*
|
||||
* ⚠️ **This deliberately calls neither `next()` nor any `res` method.** That is
|
||||
* the whole point: the socket is accepted and established, the client's fetch
|
||||
* is pending, and nothing ever answers — which is the state #2319 describes and
|
||||
* the one a `fetch` stub cannot reproduce. A stub rejects or resolves on the
|
||||
* client side; only a real server holding a real socket exercises the
|
||||
* `AbortSignal.timeout` that #2319 added.
|
||||
*
|
||||
* With `stallMs > 0` it answers late instead of never, by handing control back
|
||||
* to the real route after the delay — so one fixture covers both "slower than
|
||||
* the budget" and "never".
|
||||
*
|
||||
* **Teardown is already safe and this relies on it rather than re-implementing
|
||||
* it.** `TestServerHttp.stop()` calls `httpServer.closeAllConnections?.()`,
|
||||
* which destroys an established socket whether or not a response was ever
|
||||
* written. A withheld response therefore cannot hang a suite at teardown —
|
||||
* `stalls the token endpoint and still stops cleanly` pins that, because it is
|
||||
* the property most likely to be broken by a future change to the stop path.
|
||||
*/
|
||||
export function createOAuthStallMiddleware(
|
||||
config: OAuthConfig,
|
||||
registry?: StallRegistry,
|
||||
): express.RequestHandler | null {
|
||||
// ⚠️ Validate the CONTAINER before its contents. A JSON/YAML config is only
|
||||
// cast, so `stallEndpoints` can arrive as a string, `null`, or an object with
|
||||
// a `length`. A bare `.length === 0` check accepts `""` and `{ length: 0 }`
|
||||
// and silently returns "no stalling configured" — the precise misconfiguration
|
||||
// this startup validation exists to catch — while a non-empty string fails
|
||||
// later with an incidental `.filter is not a function` (Copilot).
|
||||
if (
|
||||
config.stallEndpoints !== undefined &&
|
||||
!Array.isArray(config.stallEndpoints)
|
||||
) {
|
||||
throw new Error(
|
||||
`oauth.stallEndpoints must be an array (got ${JSON.stringify(config.stallEndpoints)}).`,
|
||||
);
|
||||
}
|
||||
const requested = config.stallEndpoints ?? [];
|
||||
if (requested.length === 0) return null;
|
||||
|
||||
const unknown = requested.filter((e) => !isStallableOAuthEndpoint(e));
|
||||
if (unknown.length > 0) {
|
||||
// Loud, not ignored: a typo would otherwise produce a fixture that answers
|
||||
// normally, and a test asserting a timeout would fail pointing at the
|
||||
// timeout rather than at the config.
|
||||
throw new Error(
|
||||
`Unknown oauth.stallEndpoints entry: ${unknown.map((e) => JSON.stringify(e)).join(", ")}. ` +
|
||||
`Expected one of: ${STALLABLE_OAUTH_ENDPOINTS.join(", ")}.`,
|
||||
);
|
||||
}
|
||||
|
||||
// ⚠️ `stallMs` needs validating for the same reason `stallEndpoints` does, and
|
||||
// more urgently: a JSON/YAML config is only *cast* to its interface, so
|
||||
// anything can arrive here. Unchecked, a negative or non-numeric value makes
|
||||
// `stallMs > 0` false and silently becomes a PERMANENT stall, and a value past
|
||||
// the 32-bit timer range overflows and fires almost immediately — both of
|
||||
// which read as "the timeout behaved strangely" rather than "the config is
|
||||
// wrong" (Copilot).
|
||||
// ⚠️ `?? 0` would turn an explicit `stallMs: null` from a config file into a
|
||||
// valid 0 and skip every check below, silently producing a permanent stall.
|
||||
// Only an OMITTED value gets the default; `null` falls through to the
|
||||
// finite-number check and is rejected (Copilot).
|
||||
const rawStallMs = config.stallMs === undefined ? 0 : config.stallMs;
|
||||
if (
|
||||
typeof rawStallMs !== "number" ||
|
||||
!Number.isFinite(rawStallMs) ||
|
||||
rawStallMs < 0 ||
|
||||
rawStallMs > MAX_STALL_MS
|
||||
) {
|
||||
throw new Error(
|
||||
`oauth.stallMs must be a finite number between 0 and ${MAX_STALL_MS} (got ${describeValue(config.stallMs)}).`,
|
||||
);
|
||||
}
|
||||
|
||||
// ⚠️ Refuse to stall a route this config does not actually serve. The stall
|
||||
// middleware runs BEFORE Express routing, so it will happily hold a request
|
||||
// for an endpoint that would otherwise 404 — turning a contradictory fixture
|
||||
// (`supportDCR: false` with `stallEndpoints: ["register"]`) into a hanging
|
||||
// registration endpoint rather than a configuration error, and inviting a
|
||||
// timeout test that passes for entirely the wrong reason (Copilot).
|
||||
const unavailable = requested.filter(
|
||||
(endpoint) => !servesEndpoint(config, endpoint),
|
||||
);
|
||||
if (unavailable.length > 0) {
|
||||
throw new Error(
|
||||
`oauth.stallEndpoints names ${unavailable.map((e) => JSON.stringify(e)).join(", ")}, ` +
|
||||
`which this config does not serve (mode=${getOAuthMode(config)}, ` +
|
||||
`supportDCR=${String(config.supportDCR ?? false)}, ` +
|
||||
`supportRevocation=${String(config.supportRevocation !== false)}). ` +
|
||||
"Stalling a route that would otherwise 404 produces a hang that reads as a timeout.",
|
||||
);
|
||||
}
|
||||
|
||||
const targets = stallTargetsFor(config);
|
||||
// `${METHOD} ${path}` rather than a path set — see `stallTargetsFor`.
|
||||
const stalled = new Set(
|
||||
requested.flatMap((endpoint) => {
|
||||
const { path, methods } = targets[endpoint];
|
||||
return methods.map((method) => `${method} ${path}`);
|
||||
}),
|
||||
);
|
||||
const stallMs = rawStallMs;
|
||||
|
||||
return (req: Request, res: Response, next: express.NextFunction) => {
|
||||
// `req.path`, never `req.url`: the latter carries the query string, which
|
||||
// every authorize request has, so matching on it would silently stop
|
||||
// hitting `authorize` while every bare-path endpoint kept working.
|
||||
if (!stalled.has(`${req.method} ${req.path}`)) {
|
||||
next();
|
||||
return;
|
||||
}
|
||||
|
||||
if (registry) registry.parked += 1;
|
||||
// One decrement per request, whichever way it ends: answered late, or the
|
||||
// socket destroyed under it. Without this the counter only ever rises and
|
||||
// a later "wait until parked" would pass instantly on a stale count.
|
||||
let released = false;
|
||||
const release = () => {
|
||||
if (released) return;
|
||||
released = true;
|
||||
if (registry) registry.parked -= 1;
|
||||
};
|
||||
res.on("close", release);
|
||||
|
||||
if (stallMs > 0) {
|
||||
const timer = setTimeout(() => {
|
||||
release();
|
||||
next();
|
||||
}, stallMs);
|
||||
// Release the timer if the client gives up first, so a stalled fixture
|
||||
// cannot keep the event loop alive past the test that used it.
|
||||
res.on("close", () => clearTimeout(timer));
|
||||
return;
|
||||
}
|
||||
// Answer never. The socket stays established and idle until the client's
|
||||
// own timeout fires or the server destroys it on stop.
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Set up OAuth routes on an Express application
|
||||
* This adds all OAuth endpoints (authorization, token, metadata, etc.)
|
||||
|
|
@ -149,7 +436,14 @@ function bearerChallenge(config: OAuthConfig, req: Request): string {
|
|||
export function setupOAuthRoutes(
|
||||
app: express.Application,
|
||||
config: OAuthConfig,
|
||||
stallRegistry?: StallRegistry,
|
||||
): void {
|
||||
// Ahead of every OAuth route, so a stalled endpoint is withheld before any
|
||||
// handler can answer it — including the metadata documents, which are
|
||||
// registered first.
|
||||
const stall = createOAuthStallMiddleware(config, stallRegistry);
|
||||
if (stall) app.use(stall);
|
||||
|
||||
setupMetadataEndpoints(app, config);
|
||||
|
||||
if (getOAuthMode(config) === "combined") {
|
||||
|
|
|
|||
Loading…
Reference in a new issue