Repository: auth0/nextjs-auth0
Pull request #864 (2022-10-17 08:04:49)

[ASEC-891] Escape idp error
### 📋 Changes

Escape the `cause.error` and `cause.error_description` properties in addition to the `message` per security review.



Patch:
diff --git a/README.md b/README.md
index cff7cb75..d6096ec8 100644
--- a/README.md
+++ b/README.md
@@ -217,19 +217,9 @@ Check your hosting provider's caching rules, but in general you should **never**
 
 ### Error Handling and Security
 
-The default server side error handler for the `/api/auth/*` routes prints the error message to screen, like this:
+Errors that come from Auth0 in the `redirect_uri` callback may contain reflected user input via the OpenID Connect `error` and `error_description` query parameter. Because of this, we do some [basic escaping](https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#rule-1-html-encode-before-inserting-untrusted-data-into-html-element-content) on the `message`, `error` and `error_description` properties of the `IdentityProviderError`.
 
-```js
-try {
-  await handler(req, res);
-} catch (error) {
-  res.status(error.status || 400).end(error.message);
-}
-```
-
-Because the error can come from the OpenID Connect `error` query parameter we do some [basic escaping](https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#rule-1-html-encode-before-inserting-untrusted-data-into-html-element-content) which makes sure the default error handler is safe from XSS.
-
-If you write your own error handler, you should **not** render the error `message`, or `error` and `error_description` properties without using a templating engine that will properly escape it for other HTML contexts first.
+But, if you write your own error handler, you should **not** render the error `message`, or `error` and `error_description` properties without using a templating engine that will properly escape them for other HTML contexts first.
 
 ### Base Path and Internationalized Routing
 
diff --git a/src/auth0-session/client.ts b/src/auth0-session/client.ts
index 824b5382..2353842a 100644
--- a/src/auth0-session/client.ts
+++ b/src/auth0-session/client.ts
@@ -2,6 +2,7 @@ import { Issuer, custom, HttpOptions, Client, EndSessionParameters } from 'openi
 import url, { UrlObject } from 'url';
 import urlJoin from 'url-join';
 import createDebug from './utils/debug';
+import { DiscoveryError } from './utils/errors';
 import { Config } from './config';
 
 const debug = createDebug('client');
@@ -19,17 +20,6 @@ function sortSpaceDelimitedString(str: string): string {
   return str.split(' ').sort().join(' ');
 }
 
-// Issuer.discover throws an `AggregateError` in some cases, this error includes the stack trace in the
-// message which causes the stack to be exposed when reporting the error in production. We're using the non standard
-// `_errors` property to identify the polyfilled `AggregateError`.
-// See https://github.com/sindresorhus/aggregate-error/issues/4#issuecomment-488356468
-function normalizeAggregateError(e: Error | (Error & { _errors: Error[] })): Error {
-  if ('_errors' in e) {
-    return e._errors[0];
-  }
-  return e;
-}
-
 export default function get(config: Config, { name, version }: Telemetry): ClientFactory {
   let client: Client | null = null;
 
@@ -69,7 +59,7 @@ export default function get(config: Config, { name, version }: Telemetry): Clien
     try {
       issuer = await Issuer.discover(config.issuerBaseURL);
     } catch (e) {
-      throw normalizeAggregateError(e);
+      throw new DiscoveryError(e, config.issuerBaseURL);
     }
     applyHttpOptionsCustom(issuer);
 
diff --git a/src/auth0-session/handlers/callback.ts b/src/auth0-session/handlers/callback.ts
index 7413b45f..907bd2c3 100644
--- a/src/auth0-session/handlers/callback.ts
+++ b/src/auth0-session/handlers/callback.ts
@@ -7,9 +7,10 @@ import { ClientFactory } from '../client';
 import TransientStore from '../transient-store';
 import { decodeState } from '../utils/encoding';
 import { SessionCache } from '../session-cache';
-import { htmlSafe } from '../../utils/errors';
 import {
   ApplicationError,
+  EscapedError,
+  htmlSafe,
   IdentityProviderError,
   MissingStateCookieError,
   MissingStateParamError
@@ -84,9 +85,11 @@ export default function callbackHandlerFactory(
     } catch (err) {
       if (err instanceof errors.OPError) {
         err = new IdentityProviderError(err);
-      }
-      if (err instanceof errors.RPError) {
+      } else if (err instanceof errors.RPError) {
         err = new ApplicationError(err);
+        /* c8 ignore next 3 */
+      } else {
+        err = new EscapedError(err.message);
       }
       throw createHttpError(400, err, { openIdState: decodeState(expectedState) });
     }
diff --git a/src/auth0-session/handlers/login.ts b/src/auth0-session/handlers/login.ts
index 3b896336..1a745471 100644
--- a/src/auth0-session/handlers/login.ts
+++ b/src/auth0-session/handlers/login.ts
@@ -6,7 +6,7 @@ import TransientStore, { StoreOptions } from '../transient-store';
 import { encodeState } from '../utils/encoding';
 import { ClientFactory } from '../client';
 import createDebug from '../utils/debug';
-import { htmlSafe } from '../../utils/errors';
+import { htmlSafe } from '../utils/errors';
 
 const debug = createDebug('handlers');
 
diff --git a/src/auth0-session/handlers/logout.ts b/src/auth0-session/handlers/logout.ts
index 41f142cc..9a8fda4c 100644
--- a/src/auth0-session/handlers/logout.ts
+++ b/src/auth0-session/handlers/logout.ts
@@ -5,7 +5,7 @@ import createDebug from '../utils/debug';
 import { Config, LogoutOptions } from '../config';
 import { ClientFactory } from '../client';
 import { SessionCache } from '../session-cache';
-import { htmlSafe } from '../../utils/errors';
+import { htmlSafe } from '../utils/errors';
 
 const debug = createDebug('logout');
 
diff --git a/src/auth0-session/utils/errors.ts b/src/auth0-session/utils/errors.ts
index 8db6ae59..621e0101 100644
--- a/src/auth0-session/utils/errors.ts
+++ b/src/auth0-session/utils/errors.ts
@@ -1,9 +1,19 @@
 import type { errors } from 'openid-client';
 
+export class EscapedError extends Error {
+  /**
+   * **WARNING** The message can contain user input and is only escaped using basic escaping for putting untrusted data
+   * directly into the HTML body
+   */
+  constructor(message: string) {
+    /* c8 ignore next */
+    super(htmlSafe(message));
+    Object.setPrototypeOf(this, EscapedError.prototype);
+  }
+}
+
 export class MissingStateParamError extends Error {
-  static message =
-    'This endpoint must be called as part of the login flow (with a state parameter from the initial' +
-    ' authorization request).';
+  static message = 'Missing state parameter in Authorization Response.';
 
   constructor() {
     /* c8 ignore next */
@@ -13,9 +23,7 @@ export class MissingStateParamError extends Error {
 }
 
 export class MissingStateCookieError extends Error {
-  static message =
-    'The cookie dropped by the login request cannot be found, check the url of the login request, the url of' +
-    ' this callback request and your cookie config.';
+  static message = 'Missing state cookie from login request (check login URL, callback URL and cookie config).';
 
   constructor() {
     /* c8 ignore next */
@@ -24,7 +32,11 @@ export class MissingStateCookieError extends Error {
   }
 }
 
-export class ApplicationError extends Error {
+export class ApplicationError extends EscapedError {
+  /**
+   * **WARNING** The message can contain user input and is only escaped using basic escaping for putting untrusted data
+   * directly into the HTML body
+   */
   constructor(rpError: errors.RPError) {
     /* c8 ignore next */
     super(rpError.message);
@@ -32,25 +44,63 @@ export class ApplicationError extends Error {
   }
 }
 
-export class IdentityProviderError extends Error {
+export class IdentityProviderError extends EscapedError {
   /**
    * The 'error_description' parameter from the AS response.
-   * **WARNING** This can contain user input and is not escaped in any way.
+   * **WARNING** This can contain user input and is only escaped using basic escaping for putting untrusted data
+   * directly into the HTML body
    */
   errorDescription?: string;
   /**
-   * The 'error' parameter from the AS response  (Warning: this can contain user input and is not escaped in any way).
+   * The 'error' parameter from the AS response
+   * **WARNING** This can contain user input and is only escaped using basic escaping for putting untrusted data
+   * directly into the HTML body
    */
   error?: string;
 
   /**
-   * **WARNING** The message can contain user input and is not escaped in any way.
+   * **WARNING** The message can contain user input and is only escaped using basic escaping for putting untrusted data
+   * directly into the HTML body
    */
   constructor(rpError: errors.OPError) {
     /* c8 ignore next */
     super(rpError.message);
-    this.error = rpError.error;
-    this.errorDescription = rpError.error_description;
+    this.error = htmlSafe(rpError.error);
+    this.errorDescription = htmlSafe(rpError.error_description);
     Object.setPrototypeOf(this, IdentityProviderError.prototype);
   }
 }
+
+export class DiscoveryError extends EscapedError {
+  constructor(error: Error | (Error & { _errors: Error[] }), issuerBaseUrl: string) {
+    const e = normalizeAggregateError(error);
+    /* c8 ignore next */
+    super(`Discovery requests failing for ${issuerBaseUrl}, ${e.message}`);
+    Object.setPrototypeOf(this, DiscoveryError.prototype);
+  }
+}
+
+// Issuer.discover throws an `AggregateError` in some cases, this error includes the stack trace in the
+// message which causes the stack to be exposed when reporting the error in production. We're using the non standard
+// `_errors` property to identify the polyfilled `AggregateError`.
+// See https://github.com/sindresorhus/aggregate-error/issues/4#issuecomment-488356468
+function normalizeAggregateError(e: Error | (Error & { _errors: Error[] })): Error {
+  if ('_errors' in e) {
+    return e._errors[0];
+  }
+  return e;
+}
+
+// eslint-disable-next-line max-len
+// Basic escaping for putting untrusted data directly into the HTML body, per: https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#rule-1-html-encode-before-inserting-untrusted-data-into-html-element-content.
+export function htmlSafe(input?: string): string | undefined {
+  return (
+    input &&
+    input
+      .replace(/&/g, '&amp;')
+      .replace(/</g, '&lt;')
+      .replace(/>/g, '&gt;')
+      .replace(/"/g, '&quot;')
+      .replace(/'/g, '&#39;')
+  );
+}
diff --git a/src/utils/errors.ts b/src/utils/errors.ts
index 3edbd2dc..32ff268f 100644
--- a/src/utils/errors.ts
+++ b/src/utils/errors.ts
@@ -1,23 +1,12 @@
 import { HttpError } from 'http-errors';
 
-// eslint-disable-next-line max-len
-// Basic escaping for putting untrusted data directly into the HTML body, per: https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#rule-1-html-encode-before-inserting-untrusted-data-into-html-element-content.
-export function htmlSafe(input: string): string {
-  return input
-    .replace(/&/g, '&amp;')
-    .replace(/</g, '&lt;')
-    .replace(/>/g, '&gt;')
-    .replace(/"/g, '&quot;')
-    .replace(/'/g, '&#39;');
-}
-
 /**
  * @ignore
  */
 export function appendCause(errorMessage: string, cause?: Error): string {
   if (!cause) return errorMessage;
   const separator = errorMessage.endsWith('.') ? '' : '.';
-  return `${errorMessage}${separator} CAUSE: ${htmlSafe(cause.message)}`;
+  return `${errorMessage}${separator} CAUSE: ${cause.message}`;
 }
 
 type AuthErrorOptions = {
@@ -59,8 +48,11 @@ export abstract class AuthError extends Error {
   /**
    * The underlying error, if any.
    *
-   * **IMPORTANT** This underlying error is **not** escaped in any way,
-   * so do **not** render it without escaping it first!
+   * **IMPORTANT** When this error is from the Identity Provider ({@Link IdentityProviderError}) it can contain user
+   * input and is only escaped using basic escaping for putting untrusted data directly into the HTML body.
+   *
+   * You should **not** render this error without using a templating engine that will properly escape it for other
+   * HTML contexts first.
    */
   public readonly cause?: Error;
 
@@ -136,8 +128,10 @@ type HandlerErrorOptions = {
  * without using a templating engine that will properly escape it for other HTML contexts first.
  *
  * @see the {@link AuthError.cause | cause property} contains the underlying error.
- * **IMPORTANT** This underlying error is **not** escaped in any way, so do **not** render
- * it without escaping it first!
+ * **IMPORTANT** When this error is from the Identity Provider ({@Link IdentityProviderError}) it can contain user
+ * input and is only escaped using basic escaping for putting untrusted data directly into the HTML body.
+ * You should **not** render this error without using a templating engine that will properly escape it for other
+ * HTML contexts first.
  *
  * @see the {@link AuthError.status | status property} contains the HTTP status code of the error,
  * if any.
@@ -163,8 +157,10 @@ export class HandlerError extends AuthError {
  * without using a templating engine that will properly escape it for other HTML contexts first.
  *
  * @see the {@link AuthError.cause | cause property} contains the underlying error.
- * **IMPORTANT** this underlying error is **not** escaped in any way, so do **not** render
- * it without escaping it first!
+ * **IMPORTANT** When this error is from the Identity Provider ({@Link IdentityProviderError}) it can contain user
+ * input and is only escaped using basic escaping for putting untrusted data directly into the HTML body.
+ * You should **not** render this error without using a templating engine that will properly escape it for other
+ * HTML contexts first.
  *
  * @see the {@link AuthError.status | status property} contains the HTTP status code of the error,
  * if any.


Test patch:
diff --git a/tests/auth0-session/client.test.ts b/tests/auth0-session/client.test.ts
index e5b73c4a..ea42ad1a 100644
--- a/tests/auth0-session/client.test.ts
+++ b/tests/auth0-session/client.test.ts
@@ -1,10 +1,11 @@
 import nock from 'nock';
-import { Client, Issuer } from 'openid-client';
+import { Client } from 'openid-client';
 import { getConfig, clientFactory, ConfigParameters } from '../../src/auth0-session';
 import { jwks } from './fixtures/cert';
 import pkg from '../../package.json';
 import wellKnown from './fixtures/well-known.json';
 import version from '../../src/version';
+import { DiscoveryError } from '../../src/auth0-session/utils/errors';
 
 const defaultConfig = {
   secret: '__test_session_secret__',
@@ -140,11 +141,6 @@ describe('clientFactory', function () {
     nock.cleanAll();
     nock('https://op.example.com').get('/.well-known/oauth-authorization-server').reply(500);
     nock('https://op.example.com').get('/.well-known/openid-configuration').reply(500);
-    await expect(getClient()).rejects.toThrowError(new Error('expected 200 OK, got: 500 Internal Server Error'));
-  });
-
-  it('should not normalize individual errors from discovery', async () => {
-    jest.spyOn(Issuer, 'discover').mockRejectedValue(new Error('foo'));
-    await expect(getClient()).rejects.toThrowError(new Error('foo'));
+    await expect(getClient()).rejects.toThrowError(DiscoveryError);
   });
 });
diff --git a/tests/auth0-session/handlers/callback.test.ts b/tests/auth0-session/handlers/callback.test.ts
index d33d6be5..eff4f9f1 100644
--- a/tests/auth0-session/handlers/callback.test.ts
+++ b/tests/auth0-session/handlers/callback.test.ts
@@ -25,7 +25,7 @@ describe('callback', () => {
     );
 
     await expect(post(baseURL, '/callback', { body: {}, cookieJar })).rejects.toThrow(
-      'This endpoint must be called as part of the login flow (with a state parameter from the initial authorization request).'
+      'Missing state parameter in Authorization Response.'
     );
   });
 
@@ -41,7 +41,7 @@ describe('callback', () => {
         cookieJar: new CookieJar()
       })
     ).rejects.toThrowError(
-      'The cookie dropped by the login request cannot be found, check the url of the login request, the url of this callback request and your cookie config.'
+      'Missing state cookie from login request (check login URL, callback URL and cookie config).'
     );
   });
 
@@ -175,7 +175,7 @@ describe('callback', () => {
         cookieJar
       })
     ).rejects.toThrowError(
-      'The cookie dropped by the login request cannot be found, check the url of the login request, the url of this callback request and your cookie config.'
+      'Missing state cookie from login request (check login URL, callback URL and cookie config).'
     );
   });
 
@@ -484,4 +484,82 @@ describe('callback', () => {
     ).rejects.toThrow('Unauthorized');
     expect(cookieJar.getCookieStringSync(baseURL)).toBeFalsy();
   });
+
+  it('should escape Identity Provider error', async () => {
+    const baseURL = await setup(defaultConfig);
+
+    const cookieJar = await toSignedCookieJar(
+      {
+        state: expectedDefaultState,
+        nonce: '__test_nonce__',
+        response_type: 'code id_token'
+      },
+      baseURL
+    );
+
+    await expect(
+      post(baseURL, '/callback', {
+        body: {
+          state: expectedDefaultState,
+          error: '<script>alert(1)</script>',
+          error_description: '<script>alert(2)</script>'
+        },
+        cookieJar,
+        fullResponse: true
+      })
+    ).rejects.toThrowError('&lt;script&gt;alert(1)&lt;/script&gt; (&lt;script&gt;alert(2)&lt;/script&gt;)');
+  });
+
+  it('should escape application error', async () => {
+    const baseURL = await setup(defaultConfig);
+
+    const cookieJar = await toSignedCookieJar(
+      {
+        state: expectedDefaultState,
+        nonce: '__test_nonce__',
+        response_type: 'code id_token'
+      },
+      baseURL
+    );
+
+    await expect(
+      post(baseURL, '/callback', {
+        body: {
+          state: '<script>alert(1)</script>',
+          id_token: await makeIdToken()
+        },
+        cookieJar,
+        fullResponse: true
+      })
+    ).rejects.toThrowError(
+      `state mismatch, expected ${expectedDefaultState}, got: &lt;script&gt;alert(1)&lt;/script&gt;`
+    );
+  });
+
+  it('should handle discovery error', async () => {
+    const baseURL = await setup({ ...defaultConfig, issuerBaseURL: 'https://op2.example.com' });
+    nock('https://op2.example.com').get('/.well-known/openid-configuration').reply(500);
+
+    const cookieJar = await toSignedCookieJar(
+      {
+        state: expectedDefaultState,
+        nonce: '__test_nonce__',
+        response_type: 'code id_token'
+      },
+      baseURL
+    );
+
+    await expect(
+      post(baseURL, '/callback', {
+        body: {
+          state: expectedDefaultState,
+          id_token: await makeIdToken()
+        },
+        cookieJar,
+        fullResponse: true
+      })
+    ).rejects.toThrowError(
+      'Discovery requests failing for https://op2.example.com, expected 200 OK, got: 500 Internal Server Error'
+    );
+  });
 });
diff --git a/tests/auth0-session/utils/errors.test.ts b/tests/auth0-session/utils/errors.test.ts
new file mode 100644
index 00000000..8981fc80
--- /dev/null
+++ b/tests/auth0-session/utils/errors.test.ts
@@ -0,0 +1,16 @@
+import { IdentityProviderError } from '../../../src';
+
+describe('IdentityProviderError', () => {
+  test('should escape error fields', () => {
+    const error = new IdentityProviderError({
+      name: 'RPError',
+      message: "<script>alert('foo')</script>",
+      error: "<script>alert('foo')</script>",
+      error_description: "<script>alert('foo')</script>"
+    });
+
+    expect(error.message).toEqual('&lt;script&gt;alert(&#39;foo&#39;)&lt;/script&gt;');
+    expect(error.error).toEqual('&lt;script&gt;alert(&#39;foo&#39;)&lt;/script&gt;');
+    expect(error.errorDescription).toEqual('&lt;script&gt;alert(&#39;foo&#39;)&lt;/script&gt;');
+  });
+});
diff --git a/tests/handlers/callback.test.ts b/tests/handlers/callback.test.ts
index 178e045d..bcdac5a6 100644
--- a/tests/handlers/callback.test.ts
+++ b/tests/handlers/callback.test.ts
@@ -41,8 +41,7 @@ describe('callback handler', () => {
         state: '__test_state__'
       })
     ).rejects.toThrow(
-      'Callback handler failed. CAUSE: The cookie dropped by the login request cannot be found, check the url of the ' +
-        'login request, the url of this callback request and your cookie config.'
+      'Callback handler failed. CAUSE: Missing state cookie from login request (check login URL, callback URL and cookie config).'
     );
   });
 
@@ -352,9 +351,7 @@ describe('callback handler', () => {
         },
         cookieJar
       )
-    ).rejects.toThrow(
-      'Organization Id (org_id) claim value mismatch in the ID token; expected &quot;foo&quot;, found &quot;bar&quot;'
-    );
+    ).rejects.toThrow('Organization Id (org_id) claim value mismatch in the ID token; expected "foo", found "bar"');
   });
 
   test('accepts a valid organization', async () => {
diff --git a/tests/handlers/login.test.ts b/tests/handlers/login.test.ts
index 5cbaae62..81ecfb3c 100644
--- a/tests/handlers/login.test.ts
+++ b/tests/handlers/login.test.ts
@@ -282,14 +282,6 @@ describe('login handler', () => {
     });
   });
 
-  test('should escape html in error message', async () => {
-    const baseUrl = await setup(withoutApi, { discoveryOptions: { error: '<script>alert("xss")</script>' } });
-
-    await expect(get(baseUrl, '/api/auth/login', { fullResponse: true })).rejects.toThrow(
-      '&lt;script&gt;alert(&quot;xss&quot;)&lt;/script&gt;'
-    );
-  });
-
   test('should allow the returnTo to be be overwritten by getState() when provided in the querystring', async () => {
     const loginOptions = {
       returnTo: '/profile',
diff --git a/tests/utils/errors.test.ts b/tests/utils/errors.test.ts
index 880320eb..da76adbb 100644
--- a/tests/utils/errors.test.ts
+++ b/tests/utils/errors.test.ts
@@ -18,13 +18,6 @@ describe('appendCause', () => {
     expect(appendCause(message, cause)).toEqual(`${message}. CAUSE: ${cause.message}`);
   });
 
-  test('should escape the cause error message', () => {
-    const message = 'foo';
-    const cause = new Error('&<>"\'');
-
-    expect(appendCause(message, cause)).toEqual(`${message}. CAUSE: &amp;&lt;&gt;&quot;&#39;`);
-  });
-
   test('should not add a period if there is one already', () => {
     const message = 'foo.';
     const cause = new Error('bar');