Improve messaging of client metadata loading errors (bluesky-social#3135

)

* Improve messaging of client metadata loading errors

Fixes bluesky-social#3096

* Support parsing of more fetch() errors

.changeset/dry-lamps-help.md

@@ -0,0 +1,5 @@
+---
+"@atproto/oauth-provider": patch
+---
+
+Improve "invalid_client_metadata" error description

.changeset/five-emus-retire.md

@@ -0,0 +1,5 @@
+---
+"@atproto-labs/fetch": patch
+---
+
+Support parsing of more fetch() errors

packages/internal/fetch/src/fetch-error.ts

@@ -1,59 +1,13 @@
-export class FetchError extends Error {
-  public readonly statusCode: number
-
-  constructor(statusCode?: number, message?: string, options?: ErrorOptions) {
-    if (statusCode == null || !message) {
-      const info = extractInfo(extractRootCause(options?.cause))
-      statusCode = statusCode ?? info[0]
-      message = message || info[1]
-    }
-
-    super(message, options)
-
-    this.statusCode = statusCode
-  }
-}
-
-function extractRootCause(err: unknown): unknown {
-  // Unwrap the Network error from undici (i.e. Node's internal fetch() implementation)
-  // https://github.com/nodejs/undici/blob/3274c975947ce11a08508743df026f73598bfead/lib/web/fetch/index.js#L223-L228
-  if (
-    err instanceof TypeError &&
-    err.message === 'fetch failed' &&
-    err.cause !== undefined
+export abstract class FetchError extends Error {
+  constructor(
+    public readonly statusCode: number,
+    message?: string,
+    options?: ErrorOptions,
   ) {
-    return err.cause
-  }
-
-  return err
-}
-
-function extractInfo(err: unknown): [statusCode: number, message: string] {
-  if (typeof err === 'string' && err.length > 0) {
-    return [500, err]
-  }
-
-  if (!(err instanceof Error)) {
-    return [500, 'Failed to fetch']
+    super(message, options)
   }
 
-  const code = err['code']
-  if (typeof code === 'string') {
-    switch (true) {
-      case code === 'ENOTFOUND':
-        return [400, 'Invalid hostname']
-      case code === 'ECONNREFUSED':
-        return [502, 'Connection refused']
-      case code === 'DEPTH_ZERO_SELF_SIGNED_CERT':
-        return [502, 'Self-signed certificate']
-      case code.startsWith('ERR_TLS'):
-        return [502, 'TLS error']
-      case code.startsWith('ECONN'):
-        return [502, 'Connection error']
-      default:
-        return [500, `${code} error`]
-    }
+  get expose() {
+    return true
   }
-
-  return [500, err.message]
 }

packages/internal/fetch/src/fetch-request.ts

@@ -9,15 +9,87 @@ export class FetchRequestError extends FetchError {
     message?: string,
     options?: ErrorOptions,
   ) {
+    if (statusCode == null || !message) {
+      const info = extractInfo(extractRootCause(options?.cause))
+      statusCode ??= info[0]
+      message ||= info[1]
+    }
+
     super(statusCode, message, options)
   }
 
+  get expose() {
+    // A 500 request error means that the request was not made due to an infra,
+    // programming or server side issue. The message should no be exposed to
+    // downstream clients.
+    return this.statusCode !== 500
+  }
+
   static from(request: Request, cause: unknown): FetchRequestError {
     if (cause instanceof FetchRequestError) return cause
     return new FetchRequestError(request, undefined, undefined, { cause })
   }
 }
 
+function extractRootCause(err: unknown): unknown {
+  // Unwrap the Network error from undici (i.e. Node's internal fetch() implementation)
+  // https://github.com/nodejs/undici/blob/3274c975947ce11a08508743df026f73598bfead/lib/web/fetch/index.js#L223-L228
+  if (
+    err instanceof TypeError &&
+    err.message === 'fetch failed' &&
+    err.cause !== undefined
+  ) {
+    return err.cause
+  }
+
+  return err
+}
+
+function extractInfo(err: unknown): [statusCode: number, message: string] {
+  if (typeof err === 'string' && err.length > 0) {
+    return [500, err]
+  }
+
+  if (!(err instanceof Error)) {
+    return [500, 'Failed to fetch']
+  }
+
+  // Undici fetch() "network" errors
+  switch (err.message) {
+    case 'failed to fetch the data URL':
+      return [400, err.message]
+    case 'unexpected redirect':
+    case 'cors failure':
+    case 'blocked':
+    case 'proxy authentication required':
+      // These cases could be represented either as a 4xx user error (invalid
+      // URL provided), or as a 5xx server error (server didn't behave as
+      // expected).
+      return [502, err.message]
+  }
+
+  // NodeJS errors
+  const code = err['code']
+  if (typeof code === 'string') {
+    switch (true) {
+      case code === 'ENOTFOUND':
+        return [400, 'Invalid hostname']
+      case code === 'ECONNREFUSED':
+        return [502, 'Connection refused']
+      case code === 'DEPTH_ZERO_SELF_SIGNED_CERT':
+        return [502, 'Self-signed certificate']
+      case code.startsWith('ERR_TLS'):
+        return [502, 'TLS error']
+      case code.startsWith('ECONN'):
+        return [502, 'Connection error']
+      default:
+        return [500, `${code} error`]
+    }
+  }
+
+  return [500, err.message]
+}
+
 export function protocolCheckRequestTransform(protocols: {
   'about:'?: boolean
   'blob:'?: boolean

packages/oauth/oauth-provider/src/client/client-manager.ts

@@ -1,11 +1,9 @@
 import {
   bindFetch,
   Fetch,
-  FetchError,
   fetchJsonProcessor,
   fetchJsonZodProcessor,
   fetchOkProcessor,
-  FetchResponseError,
 } from '@atproto-labs/fetch'
 import { pipe } from '@atproto-labs/pipe'
 import {
@@ -25,11 +23,10 @@ import {
   OAuthClientMetadataInput,
   oauthClientMetadataSchema,
 } from '@atproto/oauth-types'
-import { ZodError } from 'zod'
 
 import { InvalidClientMetadataError } from '../errors/invalid-client-metadata-error.js'
 import { InvalidRedirectUriError } from '../errors/invalid-redirect-uri-error.js'
-import { OAuthError } from '../errors/oauth-error.js'
+import { callAsync } from '../lib/util/function.js'
 import {
   isInternetHost,
   isInternetUrl,
@@ -98,52 +95,38 @@ export class ClientManager {
    * @see {@link https://openid.net/specs/openid-connect-registration-1_0.html#rfc.section.2 OIDC Client Registration}
    */
   public async getClient(clientId: string) {
-    try {
-      const metadata = await this.getClientMetadata(clientId)
-
-      const jwks = metadata.jwks_uri
-        ? await this.jwks.get(metadata.jwks_uri)
-        : undefined
-
-      const partialInfo = await this.hooks.onClientInfo?.(clientId, {
-        metadata,
-        jwks,
-      })
-
-      const isFirstParty = partialInfo?.isFirstParty ?? false
-      const isTrusted = partialInfo?.isTrusted ?? isFirstParty
-
-      return new Client(clientId, metadata, jwks, { isFirstParty, isTrusted })
-    } catch (err) {
-      if (err instanceof OAuthError) {
-        throw err
-      }
-      if (err instanceof FetchError) {
-        const message =
-          err instanceof FetchResponseError || err.statusCode !== 500
-            ? // Only expose 500 message if it was generated on another server
-              `Failed to fetch client information: ${err.message}`
-            : `Failed to fetch client information due to an internal error`
-        throw new InvalidClientMetadataError(message, err)
-      }
-      if (err instanceof ZodError) {
-        const issues = err.issues
-          .map(
-            ({ path, message }) =>
-              `Validation${path.length ? ` of "${path.join('.')}"` : ''} failed with error: ${message}`,
-          )
-          .join(' ')
-        throw new InvalidClientMetadataError(issues || err.message, err)
-      }
-      if (err?.['code'] === 'DEPTH_ZERO_SELF_SIGNED_CERT') {
-        throw new InvalidClientMetadataError('Self-signed certificate', err)
-      }
-
+    const metadata = await this.getClientMetadata(clientId).catch((err) => {
       throw InvalidClientMetadataError.from(
         err,
-        `Unable to load client information for "${clientId}"`,
+        `Unable to obtain client metadata for "${clientId}"`,
       )
-    }
+    })
+
+    const jwks = metadata.jwks_uri
+      ? await this.jwks.get(metadata.jwks_uri).catch((err) => {
+          throw InvalidClientMetadataError.from(
+            err,
+            `Unable to obtain jwks from "${metadata.jwks_uri}" for "${clientId}"`,
+          )
+        })
+      : undefined
+
+    const partialInfo = this.hooks.onClientInfo
+      ? await callAsync(this.hooks.onClientInfo, clientId, {
+          metadata,
+          jwks,
+        }).catch((err) => {
+          throw InvalidClientMetadataError.from(
+            err,
+            `Rejected client information for "${clientId}"`,
+          )
+        })
+      : undefined
+
+    const isFirstParty = partialInfo?.isFirstParty ?? false
+    const isTrusted = partialInfo?.isTrusted ?? isFirstParty
+
+    return new Client(clientId, metadata, jwks, { isFirstParty, isTrusted })
   }
 
   protected async getClientMetadata(

packages/oauth/oauth-provider/src/errors/invalid-client-metadata-error.ts

@@ -1,3 +1,5 @@
+import { FetchError } from '@atproto-labs/fetch'
+import { ZodError } from 'zod'
 import { OAuthError } from './oauth-error.js'
 
 /**
@@ -12,20 +14,55 @@ export class InvalidClientMetadataError extends OAuthError {
     super('invalid_client_metadata', error_description, 400, cause)
   }
 
-  static from(
-    cause: unknown,
-    fallbackMessage = 'Invalid client metadata document',
-  ): InvalidClientMetadataError {
-    if (cause instanceof InvalidClientMetadataError) {
+  static from(cause: unknown, message = 'Invalid client metadata'): OAuthError {
+    if (cause instanceof OAuthError) {
       return cause
     }
+
+    if (cause instanceof FetchError) {
+      throw new InvalidClientMetadataError(
+        cause.expose ? `${message}: ${cause.message}` : message,
+        cause,
+      )
+    }
+
+    if (cause instanceof ZodError) {
+      const causeMessage =
+        cause.issues
+          .map(
+            ({ path, message }) =>
+              `Validation${path.length ? ` of "${path.join('.')}"` : ''} failed with error: ${message}`,
+          )
+          .join(' ') || cause.message
+
+      throw new InvalidClientMetadataError(
+        causeMessage ? `${message}: ${causeMessage}` : message,
+        cause,
+      )
+    }
+
+    if (
+      cause instanceof Error &&
+      'code' in cause &&
+      cause.code === 'DEPTH_ZERO_SELF_SIGNED_CERT'
+    ) {
+      throw new InvalidClientMetadataError(
+        `${message}: Self-signed certificate`,
+        cause,
+      )
+    }
+
     if (cause instanceof TypeError) {
       // This method is meant to be used in the context of parsing & validating
       // a client client metadata. In that context, a TypeError would more
       // likely represent a problem with the data (e.g. invalid URL constructor
       // arg) and not a programming error.
-      return new InvalidClientMetadataError(cause.message, cause)
+      return new InvalidClientMetadataError(
+        `${message}: ${cause.message}`,
+        cause,
+      )
     }
-    return new InvalidClientMetadataError(fallbackMessage, cause)
+
+    return new InvalidClientMetadataError(message, cause)
   }
 }

packages/oauth/oauth-provider/src/lib/util/function.ts

@@ -0,0 +1,7 @@
+export async function callAsync<F extends (...args: any[]) => unknown>(
+  this: ThisParameterType<F>,
+  fn: F,
+  ...args: Parameters<F>
+): Promise<Awaited<ReturnType<F>>> {
+  return await (fn(...args) as ReturnType<F>)
+}