fix: Always render OIDC errors correctly

src/identity/configuration/IdentityProviderFactory.ts

@@ -17,6 +17,7 @@ import type { Operation } from '../../http/Operation';
 import type { ErrorHandler } from '../../http/output/error/ErrorHandler';
 import type { ResponseWriter } from '../../http/output/ResponseWriter';
 import { BasicRepresentation } from '../../http/representation/BasicRepresentation';
+import { getLoggerFor } from '../../logging/LogUtil';
 import type { KeyValueStorage } from '../../storage/keyvalue/KeyValueStorage';
 import { InternalServerError } from '../../util/errors/InternalServerError';
 import { RedirectHttpError } from '../../util/errors/RedirectHttpError';
@@ -77,6 +78,8 @@ const COOKIES_KEY = 'cookie-secret';
  * Routes will be updated based on the `baseUrl` and `oidcPath`.
  */
 export class IdentityProviderFactory implements ProviderFactory {
+  protected readonly logger = getLoggerFor(this);
+
   private readonly config: Configuration;
   private readonly adapterFactory: AdapterFactory;
   private readonly baseUrl: string;
@@ -136,9 +139,44 @@ export class IdentityProviderFactory implements ProviderFactory {
     const provider = new Provider(this.baseUrl, config);
     provider.proxy = true;
 
+    this.captureErrorResponses(provider);
+
     return provider;
   }
 
+  /**
+   * In the `configureErrors` function below, we configure the `renderError` function of the provider configuration.
+   * This function is called by the OIDC provider library to render errors,
+   * but only does this if the accept header is HTML.
+   * Otherwise, it just returns the error object iself as a JSON object.
+   * See https://github.com/panva/node-oidc-provider/blob/0fcc112e0a95b3b2dae4eba6da812253277567c9/lib/shared/error_handler.js#L48-L52.
+   *
+   * In this function we override the `ctx.accepts` function
+   * to make the above code think HTML is always requested there.
+   * This way we have full control over error representation as configured in `configureErrors`.
+   * We still check the accept headers ourselves so there still is content negotiation on the output,
+   * the client will not simply always receive HTML.
+   *
+   * Should this part of the OIDC library code ever change, our function will break,
+   * at which point behaviour will simply revert to what it was before.
+   */
+  private captureErrorResponses(provider: Provider): void {
+    provider.use(async(ctx, next): Promise<void> => {
+      const accepts = ctx.accepts.bind(ctx);
+
+      // Using `any` typings to make sure we support all different versions of `ctx.accepts`
+      ctx.accepts = (...types: any[]): any => {
+        // Make sure we only override our specific case
+        if (types.length === 2 && types[0] === 'json' && types[1] === 'html') {
+          return 'html';
+        }
+        return accepts(...types);
+      };
+
+      return next();
+    });
+  }
+
   /**
    * Creates a configuration by copying the internal configuration
    * and adding the adapter, default audience and jwks/cookie keys.
@@ -340,14 +378,19 @@ 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;
+      if (oidcError.error_description) {
+        detailedError += ` - ${oidcError.error_description}`;
+      }
+      if (oidcError.error_detail) {
+        detailedError += ` - ${oidcError.error_detail}`;
+      }
+
+      this.logger.warn(`OIDC request failed: ${detailedError}`);
+
       // OIDC library hides extra details in these fields
       if (this.showStackTrace) {
-        if (oidcError.error_description) {
-          error.message += ` - ${oidcError.error_description}`;
-        }
-        if (oidcError.error_detail) {
-          oidcError.message += ` - ${oidcError.error_detail}`;
-        }
+        error.message = detailedError;
 
         // Also change the error message in the stack trace
         if (error.stack) {

test/integration/Identity.test.ts

@@ -621,9 +621,13 @@ describe('A Solid server with IDP', (): void => {
     });
 
     it('should return correct error output.', async(): Promise<void> => {
-      const res = await fetch(`${baseUrl}.oidc/auth`);
-      expect(res.status).toBe(400);
-      await expect(res.text()).resolves.toContain('InvalidRequest: invalid_request');
+      const res = await fetch(`${baseUrl}.oidc/foo`, { headers: { accept: 'application/json' }});
+      expect(res.status).toBe(404);
+      const json = await res.json();
+      expect(json.name).toBe(`InvalidRequest`);
+      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();
     });
   });
 });

test/unit/identity/configuration/IdentityProviderFactory.test.ts

@@ -1,4 +1,5 @@
 import { Readable } from 'stream';
+import type * as Koa from 'koa';
 import type { errors, Configuration, KoaContextWithOIDC } from 'oidc-provider';
 import type { ErrorHandler } from '../../../../src/http/output/error/ErrorHandler';
 import type { ResponseWriter } from '../../../../src/http/output/ResponseWriter';
@@ -14,7 +15,8 @@ import { FoundHttpError } from '../../../../src/util/errors/FoundHttpError';
 
 /* eslint-disable @typescript-eslint/naming-convention */
 jest.mock('oidc-provider', (): any => ({
-  Provider: jest.fn().mockImplementation((issuer: string, config: Configuration): any => ({ issuer, config })),
+  Provider: jest.fn().mockImplementation((issuer: string, config: Configuration): any =>
+    ({ issuer, config, use: jest.fn() })),
 }));
 
 const routes = {
@@ -58,6 +60,7 @@ describe('An IdentityProviderFactory', (): void => {
       request: {
         href: 'http://example.com/idp/',
       },
+      accepts: jest.fn().mockReturnValue('type'),
     } as any;
 
     interactionHandler = {
@@ -146,10 +149,11 @@ describe('An IdentityProviderFactory', (): void => {
     });
 
     // Test the renderError function
-    await expect((config.renderError as any)(ctx, {}, 'error!')).resolves.toBeUndefined();
+    const error = new Error('error!');
+    await expect((config.renderError as any)(ctx, {}, error)).resolves.toBeUndefined();
     expect(errorHandler.handleSafe).toHaveBeenCalledTimes(1);
     expect(errorHandler.handleSafe)
-      .toHaveBeenLastCalledWith({ error: 'error!', request: ctx.req });
+      .toHaveBeenLastCalledWith({ error, request: ctx.req });
     expect(responseWriter.handleSafe).toHaveBeenCalledTimes(1);
     expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response: ctx.res, result: { statusCode: 500 }});
   });
@@ -230,4 +234,35 @@ describe('An IdentityProviderFactory', (): void => {
     expect(error.message).toBe('bad data - more info - more details');
     expect(error.stack).toContain('Error: bad data - more info - more details');
   });
+
+  it('adds middleware to make the OIDC provider think the request wants HTML.', async(): Promise<void> => {
+    const provider = await factory.getProvider() as any;
+    const use = provider.use as jest.Mock<ReturnType<Koa['use']>, Parameters<Koa['use']>>;
+    expect(use).toHaveBeenCalledTimes(1);
+    const middleware = use.mock.calls[0][0];
+
+    const oldAccept = ctx.accepts;
+    const next = jest.fn();
+    await expect(middleware(ctx, next)).resolves.toBeUndefined();
+    expect(next).toHaveBeenCalledTimes(1);
+
+    expect(ctx.accepts('json', 'html')).toBe('html');
+    expect(oldAccept).toHaveBeenCalledTimes(0);
+  });
+
+  it('does not modify the context accepts function in other cases.', async(): Promise<void> => {
+    const provider = await factory.getProvider() as any;
+    const use = provider.use as jest.Mock<ReturnType<Koa['use']>, Parameters<Koa['use']>>;
+    expect(use).toHaveBeenCalledTimes(1);
+    const middleware = use.mock.calls[0][0];
+
+    const oldAccept = ctx.accepts;
+    const next = jest.fn();
+    await expect(middleware(ctx, next)).resolves.toBeUndefined();
+    expect(next).toHaveBeenCalledTimes(1);
+
+    expect(ctx.accepts('something')).toBe('type');
+    expect(oldAccept).toHaveBeenCalledTimes(1);
+    expect(oldAccept).toHaveBeenLastCalledWith('something');
+  });
 });