diff --git a/.changeset/dcr-refusal-not-an-mcpjam-bug.md b/.changeset/dcr-refusal-not-an-mcpjam-bug.md new file mode 100644 index 0000000000..d1e3c320cb --- /dev/null +++ b/.changeset/dcr-refusal-not-an-mcpjam-bug.md @@ -0,0 +1,16 @@ +--- +"@mcpjam/sdk": patch +"@mcpjam/inspector": patch +--- + +A registration endpoint answering 4xx is the server under test declining, not an MCPJam defect + +The OAuth debugger reported every Dynamic Client Registration failure to Sentry. A 4xx from the registration endpoint is the authorization server declining to mint a client — DCR left unimplemented behind an advertised `registration_endpoint`, an allowlist, an initial access token the client was never given, or metadata the server will not accept. The debugger exists to show that; filing it as a bug against MCPJam is the same noise the client-side origin policy already removes everywhere else. + +`isClientRegistrationRefusal` classifies those 4xx messages. 5xx and transport failures are deliberately not refusals: those can be the debug proxy failing, which is ours. 4xx is read as the server's policy knowingly rather than because it always is — RFC 7591 also spends `invalid_client_metadata` on a body the client built wrong, but in practice that code comes back for a valid body the server declines, and the request builder is covered by its own unit tests rather than by this signal. + +One rejection also produced two reports. The machines write the bare message, then the same message with the fallback hint appended once no pre-registered client turns up, and the debugger's dedup guard keys on the string. `restatesWithFallbackHint` pairs them by matching the hint exactly rather than by prefix — a prefix rule would have swallowed `Token request failed: 400 Bad Request` followed by the same line carrying `invalid_grant`, which is a more informative failure and not a duplicate. + +Both messages still render in the debugger. Only the Sentry issue goes away. + +`REGISTRATION_FAILURE_PREFIX` is now exported and read by `trace.ts`, which previously repeated the literal, so a rewording cannot leave the recovered-fallback check matching stale text. diff --git a/mcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.ts b/mcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.ts index dfee280ff7..e9bfa5fece 100644 --- a/mcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.ts +++ b/mcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.ts @@ -15,7 +15,10 @@ vi.mock("@mcpjam/sdk/browser", async (importOriginal) => { return { ...actual, createOAuthStateMachine }; }); -import { AUTHORIZATION_SERVER_METADATA_MISSING_ISSUER } from "@mcpjam/sdk/browser"; +import { + AUTHORIZATION_SERVER_METADATA_MISSING_ISSUER, + executeDynamicClientRegistration, +} from "@mcpjam/sdk/browser"; import { createInspectorOAuthStateMachine } from "../debug-state-machine-adapter"; @@ -39,6 +42,41 @@ function wrappedUpdateState(updateState = vi.fn(), currentStep = "metadata") { return { wrapped: passed.updateState, updateState }; } +/** + * Replay the pair of messages one failed registration writes: the bare failure, + * then the same failure with the fallback hint appended once no pre-registered + * client turns up. Taken from the SDK outcome so a rewording there cannot leave + * this asserting stale text. + */ +async function replayRegistrationFailure(status: number) { + const dcr = await executeDynamicClientRegistration({ + request: { + method: "POST", + url: "https://auth.example.test/register", + headers: {}, + body: { client_name: "Test Client" }, + }, + requestExecutor: async () => ({ + ok: false, + status, + statusText: "Registration failed", + headers: {}, + body: { error: "registration_failed" }, + }), + }); + if (dcr.status === "registered") { + throw new Error(`expected a ${status} outcome`); + } + + const { wrapped, updateState } = wrappedUpdateState( + vi.fn(), + "request_client_registration", + ); + wrapped({ error: dcr.error }); + wrapped({ error: dcr.errorWithFallbackHint }); + return { updateState }; +} + describe("OAuth debugger step-failure reporting", () => { beforeEach(() => { reportCaught.mockReset(); @@ -163,6 +201,37 @@ describe("OAuth debugger step-failure reporting", () => { expect(reportCaught).toHaveBeenCalledTimes(2); }); + it("ignores a registration the authorization server refused", async () => { + // A 4xx from the registration endpoint is that server declining to mint a + // client, which is what the debugger exists to show — not a defect here. + const { updateState } = await replayRegistrationFailure(400); + + expect(reportCaught).not.toHaveBeenCalled(); + // Silenced for Sentry only — both messages still reach the screen. + expect(updateState).toHaveBeenCalledTimes(2); + }); + + it("reports a registration the server failed to answer exactly once", async () => { + // A 5xx can be ours (a broken debug proxy), so it still reports. One + // failure writes two strings though — the bare message, then the same + // message with the fallback hint appended — and both used to file an issue. + await replayRegistrationFailure(503); + + expect(reportCaught).toHaveBeenCalledTimes(1); + }); + + it("still reports a failure that merely extends the last one", () => { + // Only the fallback-hint pair collapses. A retry that comes back with the + // reason the first attempt lacked is a more informative failure, not a + // duplicate. + const { wrapped } = wrappedUpdateState(); + + wrapped({ error: "Token request failed: 400 Bad Request" }); + wrapped({ error: "Token request failed: 400 Bad Request: invalid_grant" }); + + expect(reportCaught).toHaveBeenCalledTimes(2); + }); + it("still forwards every update to the caller's updateState", () => { const { wrapped, updateState } = wrappedUpdateState(); diff --git a/mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts b/mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts index 653aaa8845..2d68e29e39 100644 --- a/mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts +++ b/mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts @@ -3,7 +3,9 @@ import { DEFAULT_MCPJAM_CLIENT_ID_METADATA_URL, createOAuthStateMachine, getBrowserDebugDynamicRegistrationMetadata, + isClientRegistrationRefusal, isLoopbackOAuthUrl, + restatesWithFallbackHint, type OAuthFlowState, type OAuthProtocolVersion, type OAuthRequestExecutor, @@ -272,6 +274,37 @@ const UNREPORTED_STEP_FAILURES = new Set([ AUTHORIZATION_SERVER_METADATA_MISSING_ISSUER, ]); +/** + * Failures the server under test owns, matched on shape rather than by exact + * string because the message carries the status code. + * + * A 4xx from the registration endpoint is that server declining to register a + * client — DCR unimplemented behind an advertised endpoint, an allowlist, an + * initial access token we were never given. The debugger exists to show that; + * reporting it files a bug against MCPJam for another project's policy. + */ +function isUnreportedStepFailure(error: string): boolean { + return ( + UNREPORTED_STEP_FAILURES.has(error) || isClientRegistrationRefusal(error) + ); +} + +/** + * Is this message the previous report restated rather than a new failure? + * + * Plain inequality treats a registration failure's bare message and its + * hint-appended twin as two failures, so one rejection filed two issues. + */ +function restatesLastReport( + error: string, + lastReported: string | undefined, +): boolean { + if (lastReported === undefined) return false; + return ( + error === lastReported || restatesWithFallbackHint(error, lastReported) + ); +} + /** * Wrap the caller's `updateState` so every NEW step failure is reported. * @@ -287,7 +320,7 @@ const UNREPORTED_STEP_FAILURES = new Set([ * * `Warning: `-prefixed messages are skipped entirely — those are advisories the * flow recovers from (an optional metadata field the server left out), not step - * failures. So are the messages in `UNREPORTED_STEP_FAILURES`. + * failures. So are the messages `isUnreportedStepFailure` claims. */ function withStepFailureReporting( updateState: InspectorOAuthStateMachineConfig["updateState"], @@ -299,7 +332,7 @@ function withStepFailureReporting( const error = updates.error; if ( typeof error === "string" && - (error.startsWith("Warning: ") || UNREPORTED_STEP_FAILURES.has(error)) + (error.startsWith("Warning: ") || isUnreportedStepFailure(error)) ) { // Not ours to act on: the message is already on screen, and reporting // these buries real step failures under server-under-test nits. @@ -309,7 +342,11 @@ function withStepFailureReporting( updateState(updates); return; } - if (typeof error === "string" && error !== "" && error !== lastReportedError) { + if ( + typeof error === "string" && + error !== "" && + !restatesLastReport(error, lastReportedError) + ) { lastReportedError = error; reportCaught(new Error(sanitizeStepError(error)), { source: "oauth_debugger_step", diff --git a/sdk/src/browser.ts b/sdk/src/browser.ts index d3edec932a..41c5b49df4 100644 --- a/sdk/src/browser.ts +++ b/sdk/src/browser.ts @@ -581,6 +581,8 @@ export { export { buildDynamicClientRegistrationRequest, executeDynamicClientRegistration, + isClientRegistrationRefusal, + restatesWithFallbackHint, } from "./oauth/state-machines/shared/dynamic-client-registration.js"; export type { DynamicClientRegistrationCredentials, diff --git a/sdk/src/index.ts b/sdk/src/index.ts index 3deed08bd0..3b946d9195 100644 --- a/sdk/src/index.ts +++ b/sdk/src/index.ts @@ -832,6 +832,8 @@ export type { export { buildDynamicClientRegistrationRequest, executeDynamicClientRegistration, + isClientRegistrationRefusal, + restatesWithFallbackHint, } from "./oauth/state-machines/shared/dynamic-client-registration.js"; export type { DynamicClientRegistrationCredentials, diff --git a/sdk/src/oauth/state-machines/shared/dynamic-client-registration.ts b/sdk/src/oauth/state-machines/shared/dynamic-client-registration.ts index 850dfa0c96..b64a4bc66d 100644 --- a/sdk/src/oauth/state-machines/shared/dynamic-client-registration.ts +++ b/sdk/src/oauth/state-machines/shared/dynamic-client-registration.ts @@ -212,6 +212,47 @@ function backfillHistory( const FALLBACK_HINT = "Configure a pre-registered client or enable DCR on the authorization server."; +export const REGISTRATION_FAILURE_PREFIX = "Dynamic Client Registration failed"; + +/** + * Did the authorization server REFUSE to register, rather than fail to answer? + * 5xx and transport failures can be ours, so they are deliberately not refusals. + * + * 4xx is read as the server's policy knowingly, not because it always is: RFC + * 7591 also spends `invalid_client_metadata` on a body the client built wrong. + * In practice that code comes back for a valid body the server declines — a + * loopback redirect, an auth method it will not issue — and the request builder + * is covered by its own unit tests rather than by this signal. + * + * Parses the messages built below — the machines expose only the string. + */ +export function isClientRegistrationRefusal(error: string): boolean { + const prefix = `${REGISTRATION_FAILURE_PREFIX} (`; + if (!error.startsWith(prefix)) return false; + const status = Number.parseInt(error.slice(prefix.length), 10); + return status >= 400 && status < 500; +} + +/** + * Is `error` the same failure as `previous`, restated with the fallback hint? + * + * One rejected registration writes both: the bare message, then the message + * plus this hint once no pre-registered client turns up. Matching the hint + * exactly keeps the pair together without swallowing an unrelated failure that + * merely extends the last one (`"…: 400 Bad Request"` → `"…: 400 Bad Request: + * invalid_grant"` is two reports, not one). + */ +export function restatesWithFallbackHint( + error: string, + previous: string +): boolean { + return ( + error !== previous && + error.startsWith(previous) && + error.endsWith(FALLBACK_HINT) + ); +} + /** * Executes an RFC 7591 registration POST via the caller's executor, back-fills * a cloned last history entry with the redacted response, and classifies the @@ -255,13 +296,13 @@ export async function executeDynamicClientRegistration< const httpHistory = backfillHistory(input.httpHistory, redacted); if (!response.ok) { - const registrationError = `Dynamic Client Registration failed (${response.status}).`; + const registrationError = `${REGISTRATION_FAILURE_PREFIX} (${response.status}).`; return { status: "http_error", response: redacted, httpHistory, error: registrationError, - fallbackNote: `Dynamic Client Registration failed (${response.status}); using pre-registered client credentials.`, + fallbackNote: `${REGISTRATION_FAILURE_PREFIX} (${response.status}); using pre-registered client credentials.`, errorWithFallbackHint: `${registrationError} ${FALLBACK_HINT}`, }; } diff --git a/sdk/src/oauth/state-machines/trace.ts b/sdk/src/oauth/state-machines/trace.ts index aa610bdd29..815179bfeb 100644 --- a/sdk/src/oauth/state-machines/trace.ts +++ b/sdk/src/oauth/state-machines/trace.ts @@ -1,3 +1,4 @@ +import { REGISTRATION_FAILURE_PREFIX } from "./shared/dynamic-client-registration.js"; import { extractResponseErrorReason, toSingleLine, @@ -255,7 +256,7 @@ function usesRecoveredDynamicClientRegistrationFallback( } return ( - state.error.startsWith("Dynamic Client Registration failed") || + state.error.startsWith(REGISTRATION_FAILURE_PREFIX) || state.error.startsWith("Client registration failed:") ); } diff --git a/sdk/tests/oauth/dynamic-client-registration.test.ts b/sdk/tests/oauth/dynamic-client-registration.test.ts index 18aa2b54ac..a079ba8a6b 100644 --- a/sdk/tests/oauth/dynamic-client-registration.test.ts +++ b/sdk/tests/oauth/dynamic-client-registration.test.ts @@ -1,7 +1,9 @@ import { buildDynamicClientRegistrationRequest, executeDynamicClientRegistration, + isClientRegistrationRefusal, redactDynamicClientRegistrationResponse, + restatesWithFallbackHint, } from "../../src/oauth/state-machines/shared/dynamic-client-registration.js"; import { createOAuthStateMachine } from "../../src/oauth/state-machines/factory.js"; import { EMPTY_OAUTH_FLOW_STATE } from "../../src/oauth/state-machines/types.js"; @@ -243,6 +245,100 @@ describe("executeDynamicClientRegistration", () => { ); }); + it.each([400, 401, 403, 404, 422, 499])( + "classifies a %i registration as a refusal", + async (status) => { + const outcome = await executeDynamicClientRegistration({ + request: buildRequest(), + requestExecutor: jest.fn().mockResolvedValue({ + ok: false, + status, + statusText: "Rejected", + headers: {}, + body: { error: "invalid_client_metadata" }, + }), + httpHistory: history(), + }); + expect(outcome.status).toBe("http_error"); + if (outcome.status === "registered") return; + // Both strings the machines can put in state, not just the bare one. + expect(isClientRegistrationRefusal(outcome.error)).toBe(true); + expect(isClientRegistrationRefusal(outcome.errorWithFallbackHint)).toBe( + true + ); + } + ); + + it.each([500, 502, 503])( + "does not classify a %i registration as a refusal", + async (status) => { + const outcome = await executeDynamicClientRegistration({ + request: buildRequest(), + requestExecutor: jest.fn().mockResolvedValue({ + ok: false, + status, + statusText: "Server Error", + headers: {}, + body: {}, + }), + httpHistory: history(), + }); + expect(outcome.status).toBe("http_error"); + if (outcome.status === "registered") return; + expect(isClientRegistrationRefusal(outcome.error)).toBe(false); + expect(isClientRegistrationRefusal(outcome.errorWithFallbackHint)).toBe( + false + ); + } + ); + + it("does not classify a transport failure as a refusal", async () => { + const outcome = await executeDynamicClientRegistration({ + request: buildRequest(), + requestExecutor: jest.fn().mockRejectedValue(new Error("boom")), + httpHistory: history(), + }); + expect(outcome.status).toBe("network_error"); + if (outcome.status === "registered") return; + expect(isClientRegistrationRefusal(outcome.error)).toBe(false); + }); + + it.each(["", "Some other step failed (400).", "Dynamic Client Registration"])( + "does not claim the unrelated message %p", + (message) => { + expect(isClientRegistrationRefusal(message)).toBe(false); + } + ); + + it("pairs a registration failure with its hint-appended twin", async () => { + const outcome = await executeDynamicClientRegistration({ + request: buildRequest(), + requestExecutor: jest.fn().mockResolvedValue({ + ok: false, + status: 503, + statusText: "Service Unavailable", + headers: {}, + body: {}, + }), + httpHistory: history(), + }); + expect(outcome.status).toBe("http_error"); + if (outcome.status === "registered") return; + expect( + restatesWithFallbackHint(outcome.errorWithFallbackHint, outcome.error) + ).toBe(true); + }); + + it.each([ + ["Token request failed: 400 Bad Request", "invalid_grant"], + ["Authenticated request failed: 401", "token expired"], + ])( + "does not pair %p with a message that merely extends it", + (bare, detail) => { + expect(restatesWithFallbackHint(`${bare}: ${detail}`, bare)).toBe(false); + } + ); + it("synthesizes a status-0 network_error on executor rejection", async () => { const outcome = await executeDynamicClientRegistration({ request: buildRequest(),