[SDK-3553] Improved callback errors

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:
 

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;

src/auth0-session/utils/errors.ts

@@ -0,0 +1,52 @@
+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);
+  }
+}
+
+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);
+  }
+}
+
+export class ApplicationError extends Error {
+  constructor(rpError: errors.RPError) {
+    /* c8 ignore next */
+    super(rpError.message);
+  }
+}
+
+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;
+  }
+}

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,

tests/auth0-session/handlers/callback.test.ts

@@ -24,7 +24,7 @@ describe('callback', () => {
     );
 
     await expect(post(baseURL, '/callback', { body: {}, cookieJar })).rejects.toThrowError(
-      'state missing from the response'
+      '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 () => {

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,16 +26,18 @@ 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 () => {
     const onError = jest.fn();
     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'
     );
   });

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 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,6 +17,14 @@ 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);
 
@@ -24,7 +34,9 @@ describe('callback handler', () => {
       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 +104,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 () => {