fix: report a terminal OAuth token-endpoint refusal instead of a dead retry
Closes #2280 InsecureTokenEndpointError appeared nowhere in our source, so it fell through to the generic auth-failure path and rendered a "Re-authentication required" banner with a Re-authenticate button. That button can never work: the SDK's assertSecureTokenEndpoint runs inside executeTokenRequest, the error does not extend OAuthError, and auth() special-cases it to rethrow rather than start a fresh /authorize redirect. Clicking re-ran the same flow to the same refusal, under the raw SDK message, which names the three exempt host literals and nothing actionable. A terminal configuration error was being presented as a retryable auth error. Not specific to *.localhost: it fires for any endpoint outside the SDK's exemption — host.docker.internal (#1911), a LAN hostname, a reverse-proxy name, a mistyped scheme. It is now recognized (core/auth/insecureTokenEndpoint.ts, mirroring issuerBinding.ts's brand-plus-name classifier and walking cause / data.cause, since era negotiation and the transport wrappers bury the rejection) and surfaced as the configuration error it is, naming the endpoint and both ways out, with no action affordance. Six paths reach the refusal and all six classify it — the connect handshake, authenticate() during connect, the satisfied-challenge connect retry, the post-redirect callback, the command/background path, the deferred tab-visible resume (which re-armed on every focus, an unbounded loop on a terminal error), and the banner action. They share one wrapper so a seventh cannot omit the banner clear, and every clear is scoped to its own serverId: these paths are asynchronous, so a late continuation for one server must not erase a banner another raised in the meantime. Adds oauth-insecure-token-endpoint-http.json to reproduce it, whose issuer is http://localhost.:8091 — the root-anchored spelling every resolver sends to loopback but which is none of the SDK's exempt literals. transport.strictPort keeps that fixture honest, since its port is hard-coded inside the issuer. Making such an endpoint work has to land in the SDK (typescript-sdk#2591); this changes the reporting, not the outcome. Split out of #2282, where it was tangled with the *.localhost origin work for #1944 — a separate question the reporter is still clarifying. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
98911df74d
commit
b4b82493b1
5 changed files with 1400 additions and 9 deletions
27
test-servers/configs/oauth-insecure-token-endpoint-http.json
Normal file
27
test-servers/configs/oauth-insecure-token-endpoint-http.json
Normal file
|
|
@ -0,0 +1,27 @@
|
|||
{
|
||||
"serverInfo": {
|
||||
"name": "oauth-insecure-token-endpoint",
|
||||
"version": "1.0.0"
|
||||
},
|
||||
"tools": [
|
||||
{
|
||||
"preset": "echo"
|
||||
}
|
||||
],
|
||||
"oauth": {
|
||||
"enabled": true,
|
||||
"mode": "combined",
|
||||
"requireAuth": true,
|
||||
"scopesSupported": [
|
||||
"mcp"
|
||||
],
|
||||
"supportDCR": true,
|
||||
"supportRefreshTokens": true,
|
||||
"issuerUrl": "http://localhost.:8091"
|
||||
},
|
||||
"transport": {
|
||||
"type": "streamable-http",
|
||||
"port": 8091,
|
||||
"strictPort": true
|
||||
}
|
||||
}
|
||||
|
|
@ -456,6 +456,17 @@ export interface ServerConfig {
|
|||
| undefined; // Optional callback to customize resource handler during registration
|
||||
serverType?: "sse" | "streamable-http"; // Transport type (default: "streamable-http")
|
||||
port?: number; // Port to use (optional, will find available port if not specified)
|
||||
/**
|
||||
* Refuse to relocate: bind {@link port} exactly, or fail with EADDRINUSE.
|
||||
*
|
||||
* Off by default, because walking to the next free port is what lets several
|
||||
* fixtures run side by side. It exists for a fixture whose *advertised*
|
||||
* configuration hard-codes the port — `oauth-insecure-token-endpoint-http.json`
|
||||
* puts it in an OAuth issuer — where relocating leaves the server announcing
|
||||
* one port while its metadata still points at another process entirely, and
|
||||
* the fixture silently stops reproducing what it exists to reproduce.
|
||||
*/
|
||||
strictPort?: boolean;
|
||||
/**
|
||||
* Whether to advertise listChanged capability for each list type
|
||||
* If enabled, modification tools will send list_changed notifications
|
||||
|
|
|
|||
|
|
@ -119,6 +119,8 @@ export interface ConfigFile {
|
|||
transport: {
|
||||
type: "stdio" | "streamable-http" | "sse";
|
||||
port?: number;
|
||||
/** Bind `port` exactly, or fail — see `ServerConfig.strictPort`. */
|
||||
strictPort?: boolean;
|
||||
/**
|
||||
* Serve the modern (2026-07-28) protocol era via the SDK's
|
||||
* `createMcpHandler` (only valid with `type: "streamable-http"`). `true`
|
||||
|
|
@ -196,6 +198,48 @@ function validateConfig(
|
|||
`Invalid config in ${filePath}: transport.type must be stdio, streamable-http, or sse`,
|
||||
);
|
||||
}
|
||||
// `strictPort` is consumed as a plain truthiness check at bind time, so a
|
||||
// string `"false"` would read as *enabled* and silently disable the port walk
|
||||
// — the opposite of what the author wrote. Validate the type here, where the
|
||||
// file is being trusted, rather than letting it through as a `ConfigFile`.
|
||||
if (
|
||||
transport.strictPort !== undefined &&
|
||||
typeof transport.strictPort !== "boolean"
|
||||
) {
|
||||
throw new Error(
|
||||
`Invalid config in ${filePath}: transport.strictPort must be a boolean`,
|
||||
);
|
||||
}
|
||||
|
||||
// Beyond the type: reject every combination that cannot honor the flag's
|
||||
// contract, because each of them fails *silently* — the fixture looks strict
|
||||
// and relocates anyway, which is the failure the flag exists to prevent.
|
||||
//
|
||||
// - a string port (`"0"`, `"8091"`) is truthy, so it slips past the runtime
|
||||
// "no port to be strict about" guard, and Node then coerces `"0"` to the
|
||||
// dynamic port 0;
|
||||
// - a non-integer or out-of-range port cannot be bound as written;
|
||||
// - on `stdio` there is no listener at all, and `resolveConfig` drops the
|
||||
// flag, so it silently does nothing.
|
||||
if (transport.strictPort === true) {
|
||||
if (transportType === "stdio") {
|
||||
throw new Error(
|
||||
`Invalid config in ${filePath}: transport.strictPort requires an HTTP transport (streamable-http or sse)`,
|
||||
);
|
||||
}
|
||||
const port = transport.port;
|
||||
if (
|
||||
typeof port !== "number" ||
|
||||
!Number.isInteger(port) ||
|
||||
port < 1 ||
|
||||
port > 65535
|
||||
) {
|
||||
throw new Error(
|
||||
`Invalid config in ${filePath}: transport.strictPort requires transport.port to be an integer in 1-65535 (got ${JSON.stringify(port)})`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Only reject *enabling* modern on a non-HTTP transport; a falsy `modern`
|
||||
// (e.g. `false`) is a no-op that `resolveConfig` normalizes away.
|
||||
if (transport.modern && transportType !== "streamable-http") {
|
||||
|
|
|
|||
|
|
@ -102,6 +102,7 @@ export function resolveConfig(config: ConfigFile): ServerConfig {
|
|||
? (transport.type as "sse" | "streamable-http")
|
||||
: undefined,
|
||||
port: isHttp ? transport.port : undefined,
|
||||
strictPort: isHttp ? transport.strictPort : undefined,
|
||||
};
|
||||
|
||||
// Normalize the modern flag: `true` is shorthand for the default (dual-era
|
||||
|
|
|
|||
|
|
@ -400,9 +400,39 @@ export class TestServerHttp {
|
|||
const serverType = this.config.serverType ?? "streamable-http";
|
||||
const requestedPort = this.config.port;
|
||||
|
||||
// `strictPort` means "bind exactly this port, or fail". With no fixed port
|
||||
// to bind there is nothing to be strict about, and falling through to an
|
||||
// OS-assigned one would let a misconfigured fixture look strict while
|
||||
// relocating on every run — the exact failure the flag exists to prevent,
|
||||
// now silent. Reject the combination instead.
|
||||
if (
|
||||
this.config.strictPort &&
|
||||
(typeof requestedPort !== "number" ||
|
||||
!Number.isInteger(requestedPort) ||
|
||||
requestedPort < 1 ||
|
||||
requestedPort > 65535)
|
||||
) {
|
||||
// `loadConfig` rejects these for a config file; this covers a
|
||||
// programmatic caller, and specifically a value that is *truthy* but not
|
||||
// bindable as written — a string `"0"` slips past a bare falsiness check
|
||||
// and Node then coerces it to the dynamic port 0, so the fixture looks
|
||||
// strict and relocates anyway.
|
||||
throw new Error(
|
||||
`strictPort requires an explicit port as an integer in 1-65535 (got ${JSON.stringify(requestedPort)}): ` +
|
||||
"there is nothing to bind strictly otherwise.",
|
||||
);
|
||||
}
|
||||
|
||||
// If a port is explicitly requested, find an available port starting from that value
|
||||
// Otherwise, use 0 to let the OS assign an available port
|
||||
const port = requestedPort ? await findAvailablePort(requestedPort) : 0;
|
||||
// Otherwise, use 0 to let the OS assign an available port.
|
||||
// `strictPort` opts out of the walk: bind the requested port or fail loudly
|
||||
// (see the field's doc comment — a relocated server whose advertised config
|
||||
// hard-codes the port is worse than one that does not start).
|
||||
const port = requestedPort
|
||||
? this.config.strictPort
|
||||
? requestedPort
|
||||
: await findAvailablePort(requestedPort)
|
||||
: 0;
|
||||
|
||||
if (serverType === "streamable-http") {
|
||||
return this.startHttp(port);
|
||||
|
|
|
|||
Loading…
Reference in a new issue