diff --git a/src/identity/configuration/IdentityProviderFactory.ts b/src/identity/configuration/IdentityProviderFactory.ts index ea442ab21..9395f94ea 100644 --- a/src/identity/configuration/IdentityProviderFactory.ts +++ b/src/identity/configuration/IdentityProviderFactory.ts @@ -20,7 +20,9 @@ import { BasicRepresentation } from '../../http/representation/BasicRepresentati import { getLoggerFor } from '../../logging/LogUtil'; import type { KeyValueStorage } from '../../storage/keyvalue/KeyValueStorage'; import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError'; +import type { HttpError } from '../../util/errors/HttpError'; import { InternalServerError } from '../../util/errors/InternalServerError'; +import { OAuthHttpError } from '../../util/errors/OAuthHttpError'; import { RedirectHttpError } from '../../util/errors/RedirectHttpError'; import { guardStream } from '../../util/GuardedStream'; import { joinUrl } from '../../util/PathUtil'; @@ -379,7 +381,8 @@ export class IdentityProviderFactory implements ProviderFactory { // Doesn't really matter which type it is since all relevant fields are optional const oidcError = error as errors.OIDCProviderError; - let detailedError = error.message; + // Create a more detailed error message for logging and to show is `showStackTrace` is enabled. + let detailedError = oidcError.message; if (oidcError.error_description) { detailedError += ` - ${oidcError.error_description}`; } @@ -389,13 +392,21 @@ export class IdentityProviderFactory implements ProviderFactory { this.logger.warn(`OIDC request failed: ${detailedError}`); - // OIDC library hides extra details in these fields + // Convert to our own error object. + // This ensures serializing the error object will generate the correct output later on. + // We specifically copy the fields instead of passing the object to contain the `oidc-provider` dependency + // to the current file. + let resultingError: HttpError = new OAuthHttpError(out, oidcError.name, oidcError.statusCode, oidcError.message); + // Keep the original stack to make debugging easier + resultingError.stack = oidcError.stack; + if (this.showStackTrace) { - error.message = detailedError; + // Expose more information if `showStackTrace` is enabled + resultingError.message = detailedError; // Also change the error message in the stack trace - if (error.stack) { - error.stack = error.stack.replace(/.*/u, `${error.name}: ${error.message}`); + if (resultingError.stack) { + resultingError.stack = resultingError.stack.replace(/.*/u, `${oidcError.name}: ${oidcError.message}`); } } @@ -411,11 +422,11 @@ export class IdentityProviderFactory implements ProviderFactory { }, }, ); - unknownClientError.stack = error.stack; - error = unknownClientError; + unknownClientError.stack = oidcError.stack; + resultingError = unknownClientError; } - const result = await this.errorHandler.handleSafe({ error, request: guardStream(ctx.req) }); + const result = await this.errorHandler.handleSafe({ error: resultingError, request: guardStream(ctx.req) }); await this.responseWriter.handleSafe({ response: ctx.res, result }); }; } diff --git a/src/index.ts b/src/index.ts index d38f5c906..2cc35642d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -406,6 +406,7 @@ export * from './util/errors/MethodNotAllowedHttpError'; export * from './util/errors/MovedPermanentlyHttpError'; export * from './util/errors/NotFoundHttpError'; export * from './util/errors/NotImplementedHttpError'; +export * from './util/errors/OAuthHttpError'; export * from './util/errors/PreconditionFailedHttpError'; export * from './util/errors/RedirectHttpError'; export * from './util/errors/SystemError'; diff --git a/src/storage/conversion/ErrorToJsonConverter.ts b/src/storage/conversion/ErrorToJsonConverter.ts index 2f33c44f8..986a13888 100644 --- a/src/storage/conversion/ErrorToJsonConverter.ts +++ b/src/storage/conversion/ErrorToJsonConverter.ts @@ -2,6 +2,7 @@ import { BasicRepresentation } from '../../http/representation/BasicRepresentati import type { Representation } from '../../http/representation/Representation'; import { APPLICATION_JSON, INTERNAL_ERROR } from '../../util/ContentTypes'; import { HttpError } from '../../util/errors/HttpError'; +import { OAuthHttpError } from '../../util/errors/OAuthHttpError'; import { getSingleItem } from '../../util/StreamUtil'; import { BaseTypedRepresentationConverter } from './BaseTypedRepresentationConverter'; import type { RepresentationConverterArgs } from './RepresentationConverter'; @@ -22,6 +23,11 @@ export class ErrorToJsonConverter extends BaseTypedRepresentationConverter { message: error.message, }; + // OAuth errors responses require additional fields + if (OAuthHttpError.isInstance(error)) { + Object.assign(result, error.mandatoryFields); + } + if (HttpError.isInstance(error)) { result.statusCode = error.statusCode; result.errorCode = error.errorCode; diff --git a/src/util/errors/OAuthHttpError.ts b/src/util/errors/OAuthHttpError.ts new file mode 100644 index 000000000..715f1af3e --- /dev/null +++ b/src/util/errors/OAuthHttpError.ts @@ -0,0 +1,36 @@ +import type { HttpErrorOptions } from './HttpError'; +import { HttpError } from './HttpError'; + +/** + * These are the fields that can occur in an OAuth error response as described in RFC 6749, §4.1.2.1. + * https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + * + * This interface is identical to the ErrorOut interface of the `oidc-provider` library, + * but having our own version reduces the part of the codebase that is dependent on that library. + */ +export interface OAuthErrorFields { + error: string; + // eslint-disable-next-line @typescript-eslint/naming-convention + error_description?: string | undefined; + scope?: string | undefined; + state?: string | undefined; +} + +/** + * Represents on OAuth error that is being thrown. + * OAuth error responses have additional fields that need to be present in the JSON response, + * as described in RFC 6749, §4.1.2.1. + */ +export class OAuthHttpError extends HttpError { + public readonly mandatoryFields: OAuthErrorFields; + + public constructor(mandatoryFields: OAuthErrorFields, name?: string, statusCode?: number, message?: string, + options?: HttpErrorOptions) { + super(statusCode ?? 500, name ?? 'OAuthHttpError', message, options); + this.mandatoryFields = mandatoryFields; + } + + public static isInstance(error: unknown): error is OAuthHttpError { + return HttpError.isInstance(error) && Boolean((error as OAuthHttpError).mandatoryFields); + } +} diff --git a/test/integration/Identity.test.ts b/test/integration/Identity.test.ts index d1c3ee3c8..c8302de76 100644 --- a/test/integration/Identity.test.ts +++ b/test/integration/Identity.test.ts @@ -628,6 +628,7 @@ describe('A Solid server with IDP', (): void => { expect(json.message).toBe(`invalid_request - unrecognized route or not allowed method (GET on /.oidc/foo)`); expect(json.statusCode).toBe(404); expect(json.stack).toBeDefined(); + expect(json.error).toBe('invalid_request'); }); }); }); diff --git a/test/unit/identity/configuration/IdentityProviderFactory.test.ts b/test/unit/identity/configuration/IdentityProviderFactory.test.ts index 7adf696e7..9e0292a9b 100644 --- a/test/unit/identity/configuration/IdentityProviderFactory.test.ts +++ b/test/unit/identity/configuration/IdentityProviderFactory.test.ts @@ -12,6 +12,7 @@ import type { Interaction, InteractionHandler } from '../../../../src/identity/i import type { AdapterFactory } from '../../../../src/identity/storage/AdapterFactory'; import type { KeyValueStorage } from '../../../../src/storage/keyvalue/KeyValueStorage'; import { FoundHttpError } from '../../../../src/util/errors/FoundHttpError'; +import { OAuthHttpError } from '../../../../src/util/errors/OAuthHttpError'; /* eslint-disable @typescript-eslint/naming-convention */ jest.mock('oidc-provider', (): any => ({ @@ -229,14 +230,16 @@ describe('An IdentityProviderFactory', (): void => { error.error_description = 'more info'; error.error_detail = 'more details'; + const oAuthError = new OAuthHttpError(error, error.name, 500, 'bad data - more info - more details'); + await expect((config.renderError as any)(ctx, {}, error)).resolves.toBeUndefined(); expect(errorHandler.handleSafe).toHaveBeenCalledTimes(1); expect(errorHandler.handleSafe) - .toHaveBeenLastCalledWith({ error, request: ctx.req }); + .toHaveBeenLastCalledWith({ error: oAuthError, request: ctx.req }); expect(responseWriter.handleSafe).toHaveBeenCalledTimes(1); expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response: ctx.res, result: { statusCode: 500 }}); - expect(error.message).toBe('bad data - more info - more details'); - expect(error.stack).toContain('Error: bad data - more info - more details'); + expect(oAuthError.message).toBe('bad data - more info - more details'); + expect(oAuthError.stack).toContain('Error: bad data - more info - more details'); }); it('throws a specific error for unknown clients.', async(): Promise => { diff --git a/test/unit/storage/conversion/ErrorToJsonConverter.test.ts b/test/unit/storage/conversion/ErrorToJsonConverter.test.ts index 1b9ca1f01..018d0398d 100644 --- a/test/unit/storage/conversion/ErrorToJsonConverter.test.ts +++ b/test/unit/storage/conversion/ErrorToJsonConverter.test.ts @@ -1,6 +1,8 @@ import { BasicRepresentation } from '../../../../src/http/representation/BasicRepresentation'; import { ErrorToJsonConverter } from '../../../../src/storage/conversion/ErrorToJsonConverter'; import { BadRequestHttpError } from '../../../../src/util/errors/BadRequestHttpError'; +import type { OAuthErrorFields } from '../../../../src/util/errors/OAuthHttpError'; +import { OAuthHttpError } from '../../../../src/util/errors/OAuthHttpError'; import { readJsonStream } from '../../../../src/util/StreamUtil'; describe('An ErrorToJsonConverter', (): void => { @@ -47,6 +49,35 @@ describe('An ErrorToJsonConverter', (): void => { }); }); + it('adds OAuth fields if present.', async(): Promise => { + const out: OAuthErrorFields = { + error: 'error', + // eslint-disable-next-line @typescript-eslint/naming-convention + error_description: 'error_description', + scope: 'scope', + state: 'state', + }; + const error = new OAuthHttpError(out, 'InvalidRequest', 400, 'error text'); + const representation = new BasicRepresentation([ error ], 'internal/error', false); + const prom = converter.handle({ identifier, representation, preferences }); + await expect(prom).resolves.toBeDefined(); + const result = await prom; + expect(result.binary).toBe(true); + expect(result.metadata.contentType).toBe('application/json'); + await expect(readJsonStream(result.data)).resolves.toEqual({ + name: 'InvalidRequest', + message: 'error text', + statusCode: 400, + errorCode: 'H400', + stack: error.stack, + error: 'error', + // eslint-disable-next-line @typescript-eslint/naming-convention + error_description: 'error_description', + scope: 'scope', + state: 'state', + }); + }); + it('does not copy the details if they are not serializable.', async(): Promise => { const error = new BadRequestHttpError('error text', { details: { object: BigInt(1) }}); const representation = new BasicRepresentation([ error ], 'internal/error', false); diff --git a/test/unit/util/errors/OAuthHttpError.test.ts b/test/unit/util/errors/OAuthHttpError.test.ts new file mode 100644 index 000000000..629cdb0e1 --- /dev/null +++ b/test/unit/util/errors/OAuthHttpError.test.ts @@ -0,0 +1,24 @@ +import { OAuthHttpError } from '../../../../src/util/errors/OAuthHttpError'; + +describe('An OAuthHttpError', (): void => { + it('contains relevant information.', async(): Promise => { + const error = new OAuthHttpError({ error: 'error!' }, 'InvalidRequest', 400, 'message!'); + expect(error.mandatoryFields.error).toBe('error!'); + expect(error.name).toBe('InvalidRequest'); + expect(error.statusCode).toBe(400); + expect(error.message).toBe('message!'); + }); + + it('has optional fields.', async(): Promise => { + const error = new OAuthHttpError({ error: 'error!' }); + expect(error.mandatoryFields.error).toBe('error!'); + expect(error.name).toBe('OAuthHttpError'); + expect(error.statusCode).toBe(500); + }); + + it('can identify OAuth errors.', async(): Promise => { + const error = new OAuthHttpError({ error: 'error!' }); + expect(OAuthHttpError.isInstance('apple')).toBe(false); + expect(OAuthHttpError.isInstance(error)).toBe(true); + }); +});