From 0bbd75655f194ea88aaed076a505c82c33f797a9 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Tue, 5 May 2026 17:04:03 -0700 Subject: [PATCH] Clean up core OAuth discovery errors --- packages/core/sdk/src/oauth-discovery.test.ts | 23 ++- packages/core/sdk/src/oauth-discovery.ts | 154 +++++++----------- packages/core/sdk/src/oauth-helpers.test.ts | 14 +- packages/core/sdk/src/oauth-helpers.ts | 35 ++-- 4 files changed, 103 insertions(+), 123 deletions(-) diff --git a/packages/core/sdk/src/oauth-discovery.test.ts b/packages/core/sdk/src/oauth-discovery.test.ts index 2d0cd9462..88d17b13c 100644 --- a/packages/core/sdk/src/oauth-discovery.test.ts +++ b/packages/core/sdk/src/oauth-discovery.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "@effect/vitest"; -import { Cause, Effect, Exit } from "effect"; +import { Cause, Effect, Exit, Schema } from "effect"; import { OAuthDiscoveryError, @@ -11,6 +11,14 @@ import { type Handler = (url: string, init: RequestInit) => Response | Promise; +const DcrRequestBody = Schema.Struct({ + redirect_uris: Schema.Array(Schema.String), + token_endpoint_auth_method: Schema.String, +}); +const decodeDcrRequestBody = Schema.decodeUnknownSync( + Schema.fromJsonString(DcrRequestBody), +); + const installFetchRouter = ( handlers: readonly { match: (url: string) => boolean; handle: Handler }[], ): { calls: Array<{ url: string; init: RequestInit }> } => { @@ -196,7 +204,7 @@ describe("registerDynamicClient", () => { const call = calls[0]!; expect(call.init.method).toBe("POST"); - const body = JSON.parse(call.init.body as string); + const body = decodeDcrRequestBody(call.init.body); expect(body.redirect_uris).toEqual(["https://app.example.com/cb"]); expect(body.token_endpoint_auth_method).toBe("none"); }); @@ -251,11 +259,12 @@ describe("registerDynamicClient", () => { expect(Exit.isFailure(exit)).toBe(true); if (!Exit.isFailure(exit)) return; const reason = exit.cause.reasons.find(Cause.isFailReason); - if (!(reason?.error instanceof OAuthDiscoveryError)) { - throw new Error("expected OAuthDiscoveryError"); - } - expect(reason.error.status).toBe(400); - expect(reason.error.message).toMatch(/invalid_client_metadata/); + const error = reason?.error; + expect(error).toEqual(expect.objectContaining({ + _tag: "OAuthDiscoveryError", + status: 400, + message: expect.stringMatching(/invalid_client_metadata/), + })); }); }); diff --git a/packages/core/sdk/src/oauth-discovery.ts b/packages/core/sdk/src/oauth-discovery.ts index cd33e9b87..682eecea1 100644 --- a/packages/core/sdk/src/oauth-discovery.ts +++ b/packages/core/sdk/src/oauth-discovery.ts @@ -18,7 +18,7 @@ // callers actually need. // --------------------------------------------------------------------------- -import { Data, Effect, Result, Schema } from "effect"; +import { Data, Effect, Option, Result, Schema } from "effect"; import * as oauth from "oauth4webapi"; import { @@ -45,22 +45,15 @@ export class OAuthDiscoveryError extends Data.TaggedError( readonly cause?: unknown; }> {} -const discoveryError = ( - message: string, - options: { status?: number; cause?: unknown } = {}, -): OAuthDiscoveryError => - new OAuthDiscoveryError({ - message, - status: options.status, - cause: options.cause, - }); - // --------------------------------------------------------------------------- // Schemas (narrow structural parsing — the RFCs leave many fields // optional; we validate only the subset consumers read) // --------------------------------------------------------------------------- const StringArray = Schema.Array(Schema.String); +const JsonUnknownFromString = Schema.fromJsonString(Schema.Unknown); +const decodeJsonUnknownSync = Schema.decodeUnknownSync(JsonUnknownFromString); +const decodeJsonUnknownOption = Schema.decodeUnknownOption(JsonUnknownFromString); export const OAuthProtectedResourceMetadataSchema = Schema.Struct({ resource: Schema.optional(Schema.String), @@ -155,20 +148,17 @@ export interface DiscoveryRequestOptions { const MCP_PROTOCOL_VERSION_HEADER = "mcp-protocol-version"; const isLoopbackHttpUrl = (value: string): boolean => { - try { - const url = new URL(value); - if (url.protocol !== "http:") return false; - const hostname = url.hostname.toLowerCase(); - return ( - hostname === "localhost" || - hostname === "0.0.0.0" || - hostname === "::1" || - hostname === "[::1]" || - hostname.startsWith("127.") - ); - } catch { - return false; - } + if (!URL.canParse(value)) return false; + const url = new URL(value); + if (url.protocol !== "http:") return false; + const hostname = url.hostname.toLowerCase(); + return ( + hostname === "localhost" || + hostname === "0.0.0.0" || + hostname === "::1" || + hostname === "[::1]" || + hostname.startsWith("127.") + ); }; const oauth4webapiOptions = ( @@ -260,30 +250,26 @@ export const discoverProtectedResourceMetadata = ( } const text = await response.text(); if (text.length === 0) return "skip" as const; - return { status: response.status, body: JSON.parse(text) } as const; + return { status: response.status, body: decodeJsonUnknownSync(text) } as const; }, catch: (cause) => - discoveryError( - `Failed to fetch ${url}: ${cause instanceof Error ? cause.message : String(cause)}`, - { cause }, - ), + new OAuthDiscoveryError({ + message: `Failed to fetch protected resource metadata at ${url}`, + cause, + }), }); if (result === "skip") continue; if (!("body" in result)) { - return yield* Effect.fail( - discoveryError( - `Protected resource metadata returned status ${result.status}`, - { status: result.status }, - ), - ); + return yield* new OAuthDiscoveryError({ + message: `Protected resource metadata returned status ${result.status}`, + status: result.status, + }); } const metadata = yield* decodeResourceMetadata(result.body).pipe( Effect.mapError( (err) => new OAuthDiscoveryError({ - message: `Protected resource metadata is malformed: ${ - Schema.isSchemaError(err) ? err.message : String(err) - }`, + message: "Protected resource metadata is malformed", cause: err, }), ), @@ -348,15 +334,11 @@ export const discoverAuthorizationServerMetadata = ( raw: as, }; }, - catch: (cause) => { - if (cause instanceof OAuthDiscoveryError) return cause; - return discoveryError( - `Discovery (${algorithm}) failed for ${issuer}: ${ - cause instanceof Error ? cause.message : String(cause) - }`, - { cause }, - ); - }, + catch: (cause) => + new OAuthDiscoveryError({ + message: `Discovery (${algorithm}) failed for ${issuer}`, + cause, + }), }).pipe( // If one algorithm fails mid-roundtrip (network, parse, issuer // mismatch) we still want to try the other before giving up. @@ -370,9 +352,7 @@ export const discoverAuthorizationServerMetadata = ( Effect.mapError( (err) => new OAuthDiscoveryError({ - message: `Authorization server metadata is malformed: ${ - Schema.isSchemaError(err) ? err.message : String(err) - }`, + message: "Authorization server metadata is malformed", cause: err, }), ), @@ -437,11 +417,7 @@ const interpretDcrFailure = ( ): DcrErrorBody | DcrTransport => { // RFC 6749 error envelope: `{error, error_description?}` with 4xx. if (status >= 400 && status < 500) { - const parsed = Result.try({ - try: () => (text ? (JSON.parse(text) as unknown) : null), - catch: () => null, - }); - const body = Result.isSuccess(parsed) ? parsed.success : null; + const body = text ? Option.getOrNull(decodeJsonUnknownOption(text)) : null; if ( body && typeof body === "object" && @@ -497,7 +473,7 @@ export const registerDynamicClient = ( }), catch: (cause) => new DcrTransport({ - message: `Dynamic Client Registration request failed: ${cause instanceof Error ? cause.message : String(cause)}`, + message: "Dynamic Client Registration request failed", cause, }), }); @@ -505,9 +481,10 @@ export const registerDynamicClient = ( // Accept both 200 and 201 as success — RFC 7591 mandates 201, but // Todoist (and others) return 200 OK with the client information body. if (response.status !== 200 && response.status !== 201) { - const text = yield* Effect.promise(() => - response.text().catch(() => ""), - ); + const text = yield* Effect.tryPromise({ + try: () => response.text(), + catch: () => "", + }); return yield* interpretDcrFailure(response.status, text); } @@ -520,22 +497,20 @@ export const registerDynamicClient = ( cause, }), }); - const json = yield* Effect.try({ - try: () => JSON.parse(text) as unknown, - catch: (cause) => + const json = yield* Schema.decodeUnknownEffect(JsonUnknownFromString)(text).pipe( + Effect.mapError((cause) => new DcrTransport({ message: "Dynamic Client Registration response was not valid JSON", status: response.status, cause, }), - }); + ), + ); return yield* decodeClientInformation(json).pipe( Effect.mapError( (err) => new OAuthDiscoveryError({ - message: `Dynamic Client Registration response is malformed: ${ - Schema.isSchemaError(err) ? err.message : String(err) - }`, + message: "Dynamic Client Registration response is malformed", cause: err, }), ), @@ -544,16 +519,18 @@ export const registerDynamicClient = ( Effect.catchTags({ DcrErrorBody: (err) => Effect.fail( - discoveryError( - `Dynamic Client Registration failed: ${err.error}${ - err.error_description ? ` — ${err.error_description}` : "" + new OAuthDiscoveryError({ + message: `Dynamic Client Registration failed: ${err.error}${ + err.error_description ? ` - ${err.error_description}` : "" }`, - { status: err.status, cause: err }, - ), + status: err.status, + cause: err, + }), ), DcrTransport: (err) => Effect.fail( - discoveryError(`Dynamic Client Registration failed: ${err.message}`, { + new OAuthDiscoveryError({ + message: "Dynamic Client Registration failed", status: err.status, cause: err.cause ?? err, }), @@ -649,29 +626,23 @@ export const beginDynamicAuthorization = ( ); if (!authServer) { - return yield* Effect.fail( - discoveryError( - `No OAuth authorization server metadata at ${authorizationServerUrl}`, - ), - ); + return yield* new OAuthDiscoveryError({ + message: `No OAuth authorization server metadata at ${authorizationServerUrl}`, + }); } const pkceMethods = authServer.metadata.code_challenge_methods_supported ?? []; if (pkceMethods.length > 0 && !pkceMethods.includes("S256")) { - return yield* Effect.fail( - discoveryError( - `Authorization server does not support PKCE S256 (advertised: ${pkceMethods.join(", ")})`, - ), - ); + return yield* new OAuthDiscoveryError({ + message: `Authorization server does not support PKCE S256 (advertised: ${pkceMethods.join(", ")})`, + }); } const responseTypes = authServer.metadata.response_types_supported ?? []; if (responseTypes.length > 0 && !responseTypes.includes("code")) { - return yield* Effect.fail( - discoveryError( - `Authorization server does not support response_type=code (advertised: ${responseTypes.join(", ")})`, - ), - ); + return yield* new OAuthDiscoveryError({ + message: `Authorization server does not support response_type=code (advertised: ${responseTypes.join(", ")})`, + }); } const baseClientMetadata: DynamicClientMetadata = { @@ -689,9 +660,10 @@ export const beginDynamicAuthorization = ( const reg = authServer.metadata.registration_endpoint; if (!reg) { return Effect.fail( - discoveryError( - "Authorization server does not advertise registration_endpoint — cannot auto-register a client", - ), + new OAuthDiscoveryError({ + message: + "Authorization server does not advertise registration_endpoint - cannot auto-register a client", + }), ); } return registerDynamicClient( diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index 93d9fd7f9..7700d42a6 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -365,7 +365,7 @@ describe("exchangeAuthorizationCode", () => { }); it("returns a typed OAuth2Error on transport failure", async () => { - globalThis.fetch = vi.fn().mockRejectedValue(new Error("boom")) as typeof fetch; + globalThis.fetch = vi.fn().mockRejectedValue({ message: "boom" }) as typeof fetch; const exit = await Effect.runPromiseExit( exchangeAuthorizationCode({ tokenUrl: "https://example.com/token", @@ -380,7 +380,7 @@ describe("exchangeAuthorizationCode", () => { const err = exit.cause; const failure = JSON.stringify(err); expect(failure).toContain("OAuth2Error"); - expect(failure).toContain("boom"); + expect(failure).toContain("OAuth token exchange failed"); }); it("propagates RFC 6749 error_description text in the OAuth2Error", async () => { @@ -563,7 +563,7 @@ describe("shouldRefreshToken", () => { describe("OAuth2Error tagging", () => { beforeEach(() => { - globalThis.fetch = vi.fn().mockRejectedValue(new Error("network down")) as typeof fetch; + globalThis.fetch = vi.fn().mockRejectedValue({ message: "network down" }) as typeof fetch; }); afterEach(() => { globalThis.fetch = originalFetch; @@ -585,8 +585,10 @@ describe("OAuth2Error tagging", () => { it("OAuth2Error is constructable directly with message and cause", () => { const err = new OAuth2Error({ message: "test", cause: { foo: 1 } }); - expect(err._tag).toBe("OAuth2Error"); - expect(err.message).toBe("test"); - expect(err.cause).toEqual({ foo: 1 }); + expect(err).toMatchObject({ + _tag: "OAuth2Error", + message: "test", + cause: { foo: 1 }, + }); }); }); diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index ec616973b..c62abc03b 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -134,7 +134,7 @@ const toOAuth2Error = (cause: unknown): OAuth2Error => { }); } return new OAuth2Error({ - message: `OAuth token exchange failed: ${String(cause)}`, + message: "OAuth token exchange failed", cause, }); }; @@ -173,20 +173,17 @@ const asFromTokenUrlAndIssuer = ( }; const isLoopbackHttpUrl = (value: string): boolean => { - try { - const url = new URL(value); - if (url.protocol !== "http:") return false; - const hostname = url.hostname.toLowerCase(); - return ( - hostname === "localhost" || - hostname === "0.0.0.0" || - hostname === "::1" || - hostname === "[::1]" || - hostname.startsWith("127.") - ); - } catch { - return false; - } + if (!URL.canParse(value)) return false; + const url = new URL(value); + if (url.protocol !== "http:") return false; + const hostname = url.hostname.toLowerCase(); + return ( + hostname === "localhost" || + hostname === "0.0.0.0" || + hostname === "::1" || + hostname === "[::1]" || + hostname.startsWith("127.") + ); }; const oauth4webapiRequestOptions = ( @@ -230,10 +227,10 @@ const tokenResponseFrom = ( // its claims against the AS metadata and rejects mismatches we don't care // about. Strip the field before delegation. const stripIdToken = async (response: Response): Promise => { - const body = await response - .clone() - .json() - .catch(() => null); + const body = await response.clone().json().then( + (value: unknown) => value, + () => null, + ); if ( !body || typeof body !== "object" ||