Repository: auth0/nextjs-auth0
Pull request #835 (2022-10-03 14:45:21)

[SDK-3553] Improved callback errors
### 📋 Changes

Improve errors from callback handler:

- better explanation when the login request's state cookie is not found in the callback request
- better explanation (and 404) when the callback request is called when not part of the login dance
- error types for RP and OP errors



Patch:
diff --git a/FAQ.md b/FAQ.md
index 2b07d546..9dac7abc 100644
--- a/FAQ.md
+++ b/FAQ.md
@@ -1,10 +1,10 @@
 # Frequently Asked Questions
 
-1. [Why do I get a `checks.state argument is missing` error when logging in from different tabs?](#1-why-do-i-get-a-checks.state-argument-is-missing-error-if-i-try-to-log-in-from-different-tabs)
+1. [Why do I get a `state mismatch` error when logging in from different tabs?](#1-why-do-i-get-a-state-mismatch-error-if-i-try-to-log-in-from-different-tabs)
 
-## 1. Why do I get a `checks.state argument is missing` error if I try to log in from different tabs?
+## 1. Why do I get a `state mismatch` error if I try to log in from different tabs?
 
-Every time you initiate login, the SDK stores in cookies some transient state (`nonce`, `state`, `code_verifier`) necessary to verify the callback request from Auth0. Initiating login concurrently from different tabs will result in that state being overwritten in each subsequent tab. Once the login is completed in some tab, the SDK will compare the state in the callback with the state stored in the cookies. As the cookies were overwritten, the values will not match (except for the tab that initiated login the last) and the SDK will return the `checks.state argument is missing` error.
+Every time you initiate login, the SDK stores in cookies some transient state (`nonce`, `state`, `code_verifier`) necessary to verify the callback request from Auth0. Initiating login concurrently from different tabs will result in that state being overwritten in each subsequent tab. Once the login is completed in some tab, the SDK will compare the state in the callback with the state stored in the cookies. As the cookies were overwritten, the values will not match (except for the tab that initiated login the last) and the SDK will return the `state mismatch` error.
 
 For example:
 
diff --git a/src/auth0-session/handlers/callback.ts b/src/auth0-session/handlers/callback.ts
index 606c933f..deeff61d 100644
--- a/src/auth0-session/handlers/callback.ts
+++ b/src/auth0-session/handlers/callback.ts
@@ -1,12 +1,19 @@
 import { IncomingMessage, ServerResponse } from 'http';
 import urlJoin from 'url-join';
 import createHttpError from 'http-errors';
+import { errors } from 'openid-client';
 import { AuthorizationParameters, Config } from '../config';
 import { ClientFactory } from '../client';
 import TransientStore from '../transient-store';
 import { decodeState } from '../hooks/get-login-state';
 import { SessionCache } from '../session-cache';
 import { htmlSafe } from '../../utils/errors';
+import {
+  ApplicationError,
+  IdentityProviderError,
+  MissingStateCookieError,
+  MissingStateParamError
+} from '../utils/errors';
 
 function getRedirectUri(config: Config): string {
   return urlJoin(config.baseURL, config.routes.callback);
@@ -36,15 +43,25 @@ export default function callbackHandlerFactory(
     const client = await getClient();
     const redirectUri = options?.redirectUri || getRedirectUri(config);
 
-    let expectedState;
     let tokenSet;
-    try {
-      const callbackParams = client.callbackParams(req);
-      expectedState = await transientCookieHandler.read('state', req, res);
-      const max_age = await transientCookieHandler.read('max_age', req, res);
-      const code_verifier = await transientCookieHandler.read('code_verifier', req, res);
-      const nonce = await transientCookieHandler.read('nonce', req, res);
 
+    const callbackParams = client.callbackParams(req);
+
+    if (!callbackParams.state) {
+      throw createHttpError(404, new MissingStateParamError());
+    }
+
+    const expectedState = await transientCookieHandler.read('state', req, res);
+
+    if (!expectedState) {
+      throw createHttpError(400, new MissingStateCookieError());
+    }
+
+    const max_age = await transientCookieHandler.read('max_age', req, res);
+    const code_verifier = await transientCookieHandler.read('code_verifier', req, res);
+    const nonce = await transientCookieHandler.read('nonce', req, res);
+
+    try {
       tokenSet = await client.callback(
         redirectUri,
         callbackParams,
@@ -57,11 +74,13 @@ export default function callbackHandlerFactory(
         { exchangeBody: options?.authorizationParams }
       );
     } catch (err) {
-      throw createHttpError(400, err.message, {
-        error: err.error,
-        error_description: err.error_description,
-        openidState: decodeState(expectedState)
-      });
+      if (err instanceof errors.OPError) {
+        err = new IdentityProviderError(err);
+      }
+      if (err instanceof errors.RPError) {
+        err = new ApplicationError(err);
+      }
+      throw createHttpError(400, err, { openIdState: decodeState(expectedState) });
     }
 
     const openidState: { returnTo?: string } = decodeState(expectedState as string) as ValidState;
diff --git a/src/auth0-session/index.ts b/src/auth0-session/index.ts
index e48db5e1..99342ece 100644
--- a/src/auth0-session/index.ts
+++ b/src/auth0-session/index.ts
@@ -1,4 +1,10 @@
 export { default as NodeCookies, Cookies } from './utils/cookies';
+export {
+  MissingStateParamError,
+  MissingStateCookieError,
+  IdentityProviderError,
+  ApplicationError
+} from './utils/errors';
 export { default as CookieStore } from './cookie-store';
 export { default as TransientStore } from './transient-store';
 export { Config, SessionConfig, CookieConfig, LoginOptions, LogoutOptions, AuthorizationParameters } from './config';
diff --git a/src/auth0-session/utils/errors.ts b/src/auth0-session/utils/errors.ts
new file mode 100644
index 00000000..8db6ae59
--- /dev/null
+++ b/src/auth0-session/utils/errors.ts
@@ -0,0 +1,56 @@
+import type { errors } from 'openid-client';
+
+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).';
+
+  constructor() {
+    /* c8 ignore next */
+    super(MissingStateParamError.message);
+    Object.setPrototypeOf(this, MissingStateParamError.prototype);
+  }
+}
+
+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.';
+
+  constructor() {
+    /* c8 ignore next */
+    super(MissingStateCookieError.message);
+    Object.setPrototypeOf(this, MissingStateCookieError.prototype);
+  }
+}
+
+export class ApplicationError extends Error {
+  constructor(rpError: errors.RPError) {
+    /* c8 ignore next */
+    super(rpError.message);
+    Object.setPrototypeOf(this, ApplicationError.prototype);
+  }
+}
+
+export class IdentityProviderError extends Error {
+  /**
+   * The 'error_description' parameter from the AS response.
+   * **WARNING** This can contain user input and is not escaped in any way.
+   */
+  errorDescription?: string;
+  /**
+   * The 'error' parameter from the AS response  (Warning: this can contain user input and is not escaped in any way).
+   */
+  error?: string;
+
+  /**
+   * **WARNING** The message can contain user input and is not escaped in any way.
+   */
+  constructor(rpError: errors.OPError) {
+    /* c8 ignore next */
+    super(rpError.message);
+    this.error = rpError.error;
+    this.errorDescription = rpError.error_description;
+    Object.setPrototypeOf(this, IdentityProviderError.prototype);
+  }
+}
diff --git a/src/index.ts b/src/index.ts
index f22cb545..c258db94 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -147,6 +147,13 @@ export {
   ProfileHandlerError
 } from './utils/errors';
 
+export {
+  MissingStateCookieError,
+  MissingStateParamError,
+  IdentityProviderError,
+  ApplicationError
+} from './auth0-session';
+
 export {
   ConfigParameters,
   HandleAuth,
diff --git a/src/utils/errors.ts b/src/utils/errors.ts
index ce4d0e8a..3edbd2dc 100644
--- a/src/utils/errors.ts
+++ b/src/utils/errors.ts
@@ -34,7 +34,7 @@ type AuthErrorOptions = {
  * Because part of the error message can come from the OpenID Connect `error` query parameter we
  * do some basic escaping which makes sure the default error handler is safe from XSS.
  *
- * **IMPORTANT** If you write your own error handler, you should **not** render the error message
+ * **IMPORTANT** If you write your own error handler, you should **not** render the error
  * without using a templating engine that will properly escape it for other HTML contexts first.
  *
  * Note that the error message of the {@link AuthError.cause | underlying error} is **not** escaped
@@ -59,8 +59,8 @@ export abstract class AuthError extends Error {
   /**
    * The underlying error, if any.
    *
-   * **IMPORTANT** The error message of this underlying error is **not** escaped in
-   * any way, so do **not** render it without escaping it first!
+   * **IMPORTANT** This underlying error is **not** escaped in any way,
+   * so do **not** render it without escaping it first!
    */
   public readonly cause?: Error;
 
@@ -136,7 +136,7 @@ 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** The error message of this underlying error is **not** escaped in any way, so do **not** render
+ * **IMPORTANT** This underlying error is **not** escaped in any way, so do **not** render
  * it without escaping it first!
  *
  * @see the {@link AuthError.status | status property} contains the HTTP status code of the error,
@@ -163,7 +163,7 @@ 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** The error message of this underlying error is **not** escaped in any way, so do **not** render
+ * **IMPORTANT** this underlying error is **not** escaped in any way, so do **not** render
  * it without escaping it first!
  *
  * @see the {@link AuthError.status | status property} contains the HTTP status code of the error,


Test patch:
diff --git a/tests/auth0-session/handlers/callback.test.ts b/tests/auth0-session/handlers/callback.test.ts
index 03929018..949ed931 100644
--- a/tests/auth0-session/handlers/callback.test.ts
+++ b/tests/auth0-session/handlers/callback.test.ts
@@ -23,8 +23,8 @@ describe('callback', () => {
       baseURL
     );
 
-    await expect(post(baseURL, '/callback', { body: {}, cookieJar })).rejects.toThrowError(
-      'state missing from the response'
+    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).'
     );
   });
 
@@ -39,7 +39,9 @@ describe('callback', () => {
         },
         cookieJar: new CookieJar()
       })
-    ).rejects.toThrowError('checks.state argument is missing');
+    ).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.'
+    );
   });
 
   it("should error when state doesn't match", async () => {
@@ -171,7 +173,9 @@ describe('callback', () => {
         },
         cookieJar
       })
-    ).rejects.toThrowError('checks.state argument is missing');
+    ).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.'
+    );
   });
 
   it('should error for expired ID token', async () => {
diff --git a/tests/fixtures/setup.ts b/tests/fixtures/setup.ts
index 0cda47b2..45592b52 100644
--- a/tests/fixtures/setup.ts
+++ b/tests/fixtures/setup.ts
@@ -36,7 +36,7 @@ export type SetupOptions = {
   asyncProps?: boolean;
 };
 
-const defaultOnError: OnError = (_req, res, error) => {
+export const defaultOnError: OnError = (_req, res, error) => {
   res.statusMessage = error.message;
   res.status(error.status || 500).end(error.message);
 };
diff --git a/tests/handlers/auth.test.ts b/tests/handlers/auth.test.ts
index 1be1d602..e74374d3 100644
--- a/tests/handlers/auth.test.ts
+++ b/tests/handlers/auth.test.ts
@@ -5,10 +5,10 @@ import { setup, teardown } from '../fixtures/setup';
 import { get } from '../auth0-session/fixtures/helpers';
 import { initAuth0, OnError } from '../../src';
 
-const handlerError = (status = 400, error = 'foo', error_description = 'bar') =>
+const handlerError = () =>
   expect.objectContaining({
-    status,
-    cause: expect.objectContaining({ error, error_description })
+    status: 400,
+    code: 'ERR_CALLBACK_HANDLER_FAILURE'
   });
 
 describe('auth handler', () => {
@@ -17,7 +17,7 @@ describe('auth handler', () => {
   test('accept custom error handler', async () => {
     const onError = jest.fn<void, ArgumentsOf<OnError>>((_req, res) => res.end());
     const baseUrl = await setup(withoutApi, { onError });
-    await get(baseUrl, '/api/auth/callback?error=foo&error_description=bar');
+    await get(baseUrl, '/api/auth/callback?error=foo&error_description=bar&state=foo');
     expect(onError).toHaveBeenCalledWith(expect.any(IncomingMessage), expect.any(ServerResponse), handlerError());
   });
 
@@ -26,8 +26,10 @@ describe('auth handler', () => {
     global.handleAuth = (await initAuth0(withoutApi)).handleAuth;
     delete global.onError;
     jest.spyOn(console, 'error').mockImplementation(() => {});
-    await expect(get(baseUrl, '/api/auth/callback?error=foo&error_description=bar')).rejects.toThrow('Bad Request');
-    expect(console.error).toHaveBeenCalledWith(new Error('Callback handler failed. CAUSE: foo (bar)'));
+    await expect(get(baseUrl, '/api/auth/callback?error=foo&error_description=bar&state=foo')).rejects.toThrow(
+      'Bad Request'
+    );
+    expect(console.error).toHaveBeenCalledWith(handlerError());
   });
 
   test('finish response if custom error does not', async () => {
@@ -35,7 +37,7 @@ describe('auth handler', () => {
     const baseUrl = await setup(withoutApi);
     global.handleAuth = (await initAuth0(withoutApi)).handleAuth.bind(null, { onError });
     await expect(
-      get(baseUrl, '/api/auth/callback?error=foo&error_description=bar', { fullResponse: true })
+      get(baseUrl, '/api/auth/callback?error=foo&error_description=bar&state=foo', { fullResponse: true })
     ).rejects.toThrow('Internal Server Error');
     expect(onError).toHaveBeenCalledWith(expect.any(IncomingMessage), expect.any(ServerResponse), handlerError());
   });
@@ -45,7 +47,7 @@ describe('auth handler', () => {
     const baseUrl = await setup(withoutApi);
     global.handleAuth = (await initAuth0(withoutApi)).handleAuth.bind(null, { onError });
     await expect(
-      get(baseUrl, '/api/auth/callback?error=foo&error_description=bar', { fullResponse: true })
+      get(baseUrl, '/api/auth/callback?error=foo&error_description=bar&state=foo', { fullResponse: true })
     ).rejects.toThrow("I'm a Teapot");
     expect(onError).toHaveBeenCalledWith(expect.any(IncomingMessage), expect.any(ServerResponse), handlerError());
   });
@@ -57,7 +59,7 @@ describe('auth handler', () => {
     jest.spyOn(console, 'error').mockImplementation((error) => {
       delete error.status;
     });
-    await expect(get(baseUrl, '/api/auth/callback?error=foo&error_description=bar')).rejects.toThrow(
+    await expect(get(baseUrl, '/api/auth/callback?error=foo&error_description=bar&state=foo')).rejects.toThrow(
       'Internal Server Error'
     );
   });
diff --git a/tests/handlers/callback.test.ts b/tests/handlers/callback.test.ts
index 37f48370..7359bf62 100644
--- a/tests/handlers/callback.test.ts
+++ b/tests/handlers/callback.test.ts
@@ -1,12 +1,14 @@
 import { CookieJar } from 'tough-cookie';
+import * as jose from 'jose';
 import timekeeper = require('timekeeper');
 import { withApi, withoutApi } from '../fixtures/default-settings';
 import { makeIdToken } from '../auth0-session/fixtures/cert';
-import { get, post, toSignedCookieJar } from '../auth0-session/fixtures/helpers';
+import { defaultConfig, get, post, toSignedCookieJar } from '../auth0-session/fixtures/helpers';
 import { encodeState } from '../../src/auth0-session/hooks/get-login-state';
-import { setup, teardown } from '../fixtures/setup';
-import { Session, AfterCallback } from '../../src';
+import { defaultOnError, setup, teardown } from '../fixtures/setup';
+import { Session, AfterCallback, MissingStateCookieError } from '../../src';
 import nock from 'nock';
+import { signing as deriveKey } from '../../src/auth0-session/utils/hkdf';
 
 const callback = (baseUrl: string, body: any, cookieJar?: CookieJar): Promise<any> =>
   post(baseUrl, `/api/auth/callback`, {
@@ -15,16 +17,32 @@ const callback = (baseUrl: string, body: any, cookieJar?: CookieJar): Promise<an
     fullResponse: true
   });
 
+const generateSignature = async (cookie: string, value: string): Promise<string> => {
+  const key = await deriveKey(defaultConfig.secret as string);
+  const { signature } = await new jose.FlattenedSign(new TextEncoder().encode(`${cookie}=${value}`))
+    .setProtectedHeader({ alg: 'HS256', b64: false, crit: ['b64'] })
+    .sign(key);
+  return signature;
+};
+
 describe('callback handler', () => {
   afterEach(teardown);
 
   test('should require a state', async () => {
-    const baseUrl = await setup(withoutApi);
+    expect.assertions(2);
+    const baseUrl = await setup(withoutApi, {
+      onError(req, res, err) {
+        expect(err.cause).toBeInstanceOf(MissingStateCookieError);
+        defaultOnError(req, res, err);
+      }
+    });
     await expect(
       callback(baseUrl, {
         state: '__test_state__'
       })
-    ).rejects.toThrow('checks.state argument is missing');
+    ).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.'
+    );
   });
 
   test('should validate the state', async () => {
@@ -92,9 +110,15 @@ describe('callback handler', () => {
 
   it('should escape html in error qp', async () => {
     const baseUrl = await setup(withoutApi);
-    await expect(get(baseUrl, `/api/auth/callback?error=%3Cscript%3Ealert(%27xss%27)%3C%2Fscript%3E`)).rejects.toThrow(
-      '&lt;script&gt;alert(&#39;xss&#39;)&lt;/script&gt;'
+    const cookieJar = await toSignedCookieJar(
+      {
+        state: `foo.${await generateSignature('state', 'foo')}`
+      },
+      baseUrl
     );
+    await expect(
+      get(baseUrl, `/api/auth/callback?error=%3Cscript%3Ealert(%27xss%27)%3C%2Fscript%3E&state=foo`, { cookieJar })
+    ).rejects.toThrow('&lt;script&gt;alert(&#39;xss&#39;)&lt;/script&gt;');
   });
 
   test('should create the session without OIDC claims', async () => {