diff --git a/.changeset/dry-lamps-help.md b/.changeset/dry-lamps-help.md new file mode 100644 index 00000000000..20a5a960dc4 --- /dev/null +++ b/.changeset/dry-lamps-help.md @@ -0,0 +1,5 @@ +--- +"@atproto/oauth-provider": patch +--- + +Improve "invalid_client_metadata" error description diff --git a/.changeset/five-emus-retire.md b/.changeset/five-emus-retire.md new file mode 100644 index 00000000000..30a942188c7 --- /dev/null +++ b/.changeset/five-emus-retire.md @@ -0,0 +1,5 @@ +--- +"@atproto-labs/fetch": patch +--- + +Support parsing of more fetch() errors diff --git a/packages/internal/fetch/src/fetch-error.ts b/packages/internal/fetch/src/fetch-error.ts index d73e818ea94..afc63c3e9b0 100644 --- a/packages/internal/fetch/src/fetch-error.ts +++ b/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] } diff --git a/packages/internal/fetch/src/fetch-request.ts b/packages/internal/fetch/src/fetch-request.ts index eabe02851f7..18d87851880 100644 --- a/packages/internal/fetch/src/fetch-request.ts +++ b/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 diff --git a/packages/oauth/oauth-provider/src/client/client-manager.ts b/packages/oauth/oauth-provider/src/client/client-manager.ts index 549635d5864..772e0f67162 100644 --- a/packages/oauth/oauth-provider/src/client/client-manager.ts +++ b/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( diff --git a/packages/oauth/oauth-provider/src/errors/invalid-client-metadata-error.ts b/packages/oauth/oauth-provider/src/errors/invalid-client-metadata-error.ts index 2426106b5e6..68c046506f0 100644 --- a/packages/oauth/oauth-provider/src/errors/invalid-client-metadata-error.ts +++ b/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) } } diff --git a/packages/oauth/oauth-provider/src/lib/util/function.ts b/packages/oauth/oauth-provider/src/lib/util/function.ts new file mode 100644 index 00000000000..6f7c65e7e9a --- /dev/null +++ b/packages/oauth/oauth-provider/src/lib/util/function.ts @@ -0,0 +1,7 @@ +export async function callAsync unknown>( + this: ThisParameterType, + fn: F, + ...args: Parameters +): Promise>> { + return await (fn(...args) as ReturnType) +} diff --git a/packages/oauth/oauth-provider/src/token/token-manager.ts b/packages/oauth/oauth-provider/src/token/token-manager.ts index c16f32f3a3a..881770d7eca 100644 --- a/packages/oauth/oauth-provider/src/token/token-manager.ts +++ b/packages/oauth/oauth-provider/src/token/token-manager.ts @@ -2,8 +2,8 @@ import { isSignedJwt } from '@atproto/jwk' import { CLIENT_ASSERTION_TYPE_JWT_BEARER, OAuthAccessToken, - OAuthAuthorizationRequestParameters, OAuthAuthorizationCodeGrantTokenRequest, + OAuthAuthorizationRequestParameters, OAuthClientCredentialsGrantTokenRequest, OAuthPasswordGrantTokenRequest, OAuthRefreshTokenGrantTokenRequest, @@ -31,6 +31,7 @@ import { InvalidGrantError } from '../errors/invalid-grant-error.js' import { InvalidRequestError } from '../errors/invalid-request-error.js' import { InvalidTokenError } from '../errors/invalid-token-error.js' import { dateToEpoch, dateToRelativeSeconds } from '../lib/util/date.js' +import { callAsync } from '../lib/util/function.js' import { OAuthHooks } from '../oauth-hooks.js' import { Code, isCode } from '../request/code.js' import { Signer } from '../signer/signer.js' @@ -207,10 +208,13 @@ export class TokenManager { const now = new Date() const expiresAt = this.createTokenExpiry(now) - const authorizationDetails = await this.hooks.onAuthorizationDetails?.call( - null, - { client, parameters, account }, - ) + const authorizationDetails = this.hooks.onAuthorizationDetails + ? await callAsync(this.hooks.onAuthorizationDetails, { + client, + parameters, + account, + }) + : undefined const tokenData: TokenData = { createdAt: now, @@ -374,12 +378,13 @@ export class TokenManager { throw new InvalidGrantError(`Refresh token expired`) } - const authorization_details = - await this.hooks.onAuthorizationDetails?.call(null, { - client, - parameters, - account, - }) + const authorization_details = this.hooks.onAuthorizationDetails + ? await callAsync(this.hooks.onAuthorizationDetails, { + client, + parameters, + account, + }) + : undefined const nextTokenId = await generateTokenId() const nextRefreshToken = await generateRefreshToken()