Merge pull request from GHSA-954c-jjx6-cxv7

Fix reflected XSS from the callback handler's error query parameter

Author: lzychowski

README.md

@@ -17,6 +17,7 @@ The Auth0 Next.js SDK is a library for implementing user authentication in Next.
   - [API Reference](#api-reference)
   - [v1 Migration Guide](./V1_MIGRATION_GUIDE.md)
   - [Cookies and Security](#cookies-and-security)
+  - [Error Handling and Security](#error-handling-and-security)
   - [Base Path and Internationalized Routing](#base-path-and-internationalized-routing)
   - [Architecture](./ARCHITECTURE.md)
   - [Comparison with auth0-react](#comparison-with-auth0-react)
@@ -188,6 +189,22 @@ The `HttpOnly` setting will make sure that client-side JavaScript is unable to a
 
 The `SameSite=Lax` setting will help mitigate CSRF attacks. Learn more about SameSite by reading the ["Upcoming Browser Behavior Changes: What Developers Need to Know"](https://auth0.com/blog/browser-behavior-changes-what-developers-need-to-know/) blog post.
 
+### Error Handling and Security
+
+The default server side error handler for the `/api/auth/*` routes prints the error message to screen, eg
+
+```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 without using a templating engine that will properly escape it for other HTML contexts first.
+
 ### Base Path and Internationalized Routing
 
 With Next.js you can deploy a Next.js application under a sub-path of a domain using [Base Path](https://nextjs.org/docs/api-reference/next.config.js/basepath) and serve internationalized (i18n) routes using [Internationalized Routing](https://nextjs.org/docs/advanced-features/i18n-routing).

src/handlers/callback.ts

@@ -4,6 +4,7 @@ import { HandleCallback as BaseHandleCallback } from '../auth0-session';
 import { Session } from '../session';
 import { assertReqRes } from '../utils/assert';
 import { NextConfig } from '../config';
+import { HandlerError } from '../utils/errors';
 
 /**
  * Use this function for validating additional claims on the user's ID Token or adding removing items from
@@ -122,10 +123,14 @@ const idTokenValidator = (afterCallback?: AfterCallback, organization?: string):
  */
 export default function handleCallbackFactory(handler: BaseHandleCallback, config: NextConfig): HandleCallback {
   return async (req, res, options = {}): Promise<void> => {
-    assertReqRes(req, res);
-    return handler(req, res, {
-      ...options,
-      afterCallback: idTokenValidator(options.afterCallback, options.organization || config.organization)
-    });
+    try {
+      assertReqRes(req, res);
+      return await handler(req, res, {
+        ...options,
+        afterCallback: idTokenValidator(options.afterCallback, options.organization || config.organization)
+      });
+    } catch (e) {
+      throw new HandlerError(e);
+    }
   };
 }

src/handlers/login.ts

@@ -3,6 +3,7 @@ import { AuthorizationParameters, HandleLogin as BaseHandleLogin } from '../auth
 import isSafeRedirect from '../utils/url-helpers';
 import { assertReqRes } from '../utils/assert';
 import { NextConfig } from '../config';
+import { HandlerError } from '../utils/errors';
 
 /**
  * Use this to store additional state for the user before they visit the Identity Provider to login.
@@ -109,23 +110,27 @@ export type HandleLogin = (req: NextApiRequest, res: NextApiResponse, options?:
  */
 export default function handleLoginFactory(handler: BaseHandleLogin, nextConfig: NextConfig): HandleLogin {
   return async (req, res, options = {}): Promise<void> => {
-    assertReqRes(req, res);
-    if (req.query.returnTo) {
-      const returnTo = Array.isArray(req.query.returnTo) ? req.query.returnTo[0] : req.query.returnTo;
+    try {
+      assertReqRes(req, res);
+      if (req.query.returnTo) {
+        const returnTo = Array.isArray(req.query.returnTo) ? req.query.returnTo[0] : req.query.returnTo;
 
-      if (!isSafeRedirect(returnTo)) {
-        throw new Error('Invalid value provided for returnTo, must be a relative url');
+        if (!isSafeRedirect(returnTo)) {
+          throw new Error('Invalid value provided for returnTo, must be a relative url');
+        }
+
+        options = { ...options, returnTo };
+      }
+      if (nextConfig.organization) {
+        options = {
+          ...options,
+          authorizationParams: { organization: nextConfig.organization, ...options.authorizationParams }
+        };
       }
 
-      options = { ...options, returnTo };
+      return await handler(req, res, options);
+    } catch (e) {
+      throw new HandlerError(e);
     }
-    if (nextConfig.organization) {
-      options = {
-        ...options,
-        authorizationParams: { organization: nextConfig.organization, ...options.authorizationParams }
-      };
-    }
-
-    return handler(req, res, options);
   };
 }

src/handlers/logout.ts

@@ -1,6 +1,7 @@
 import { NextApiResponse, NextApiRequest } from 'next';
 import { HandleLogout as BaseHandleLogout } from '../auth0-session';
 import { assertReqRes } from '../utils/assert';
+import { HandlerError } from '../utils/errors';
 
 /**
  * Custom options to pass to logout.
@@ -27,7 +28,11 @@ export type HandleLogout = (req: NextApiRequest, res: NextApiResponse, options?:
  */
 export default function handleLogoutFactory(handler: BaseHandleLogout): HandleLogout {
   return async (req, res, options): Promise<void> => {
-    assertReqRes(req, res);
-    return handler(req, res, options);
+    try {
+      assertReqRes(req, res);
+      return await handler(req, res, options);
+    } catch (e) {
+      throw new HandlerError(e);
+    }
   };
 }

src/handlers/profile.ts

@@ -2,6 +2,7 @@ import { NextApiResponse, NextApiRequest } from 'next';
 import { ClientFactory } from '../auth0-session';
 import { SessionCache, Session, fromJson, GetAccessToken } from '../session';
 import { assertReqRes } from '../utils/assert';
+import { HandlerError } from '../utils/errors';
 
 export type AfterRefetch = (req: NextApiRequest, res: NextApiResponse, session: Session) => Promise<Session> | Session;
 
@@ -39,46 +40,50 @@ export default function profileHandler(
   sessionCache: SessionCache
 ): HandleProfile {
   return async (req, res, options): Promise<void> => {
-    assertReqRes(req, res);
+    try {
+      assertReqRes(req, res);
 
-    if (!sessionCache.isAuthenticated(req, res)) {
-      res.status(401).json({
-        error: 'not_authenticated',
-        description: 'The user does not have an active session or is not authenticated'
-      });
-      return;
-    }
+      if (!sessionCache.isAuthenticated(req, res)) {
+        res.status(401).json({
+          error: 'not_authenticated',
+          description: 'The user does not have an active session or is not authenticated'
+        });
+        return;
+      }
 
-    const session = sessionCache.get(req, res) as Session;
-    res.setHeader('Cache-Control', 'no-store');
+      const session = sessionCache.get(req, res) as Session;
+      res.setHeader('Cache-Control', 'no-store');
 
-    if (options?.refetch) {
-      const { accessToken } = await getAccessToken(req, res);
-      if (!accessToken) {
-        throw new Error('No access token available to refetch the profile');
-      }
+      if (options?.refetch) {
+        const { accessToken } = await getAccessToken(req, res);
+        if (!accessToken) {
+          throw new Error('No access token available to refetch the profile');
+        }
 
-      const client = await getClient();
-      const userInfo = await client.userinfo(accessToken);
+        const client = await getClient();
+        const userInfo = await client.userinfo(accessToken);
 
-      let newSession = fromJson({
-        ...session,
-        user: {
-          ...session.user,
-          ...userInfo
+        let newSession = fromJson({
+          ...session,
+          user: {
+            ...session.user,
+            ...userInfo
+          }
+        }) as Session;
+
+        if (options.afterRefetch) {
+          newSession = await options.afterRefetch(req, res, newSession);
         }
-      }) as Session;
 
-      if (options.afterRefetch) {
-        newSession = await options.afterRefetch(req, res, newSession);
-      }
+        sessionCache.set(req, res, newSession);
 
-      sessionCache.set(req, res, newSession);
+        res.json(newSession.user);
+        return;
+      }
 
-      res.json(newSession.user);
-      return;
+      res.json(session.user);
+    } catch (e) {
+      throw new HandlerError(e);
     }
-
-    res.json(session.user);
   };
 }

src/utils/errors.ts

@@ -1,3 +1,5 @@
+import { HttpError } from 'http-errors';
+
 /**
  * The error thrown by {@link GetAccessToken}
  *
@@ -19,3 +21,44 @@ export class AccessTokenError extends Error {
     this.code = code;
   }
 }
+
+// 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
+function htmlSafe(input: string): string {
+  return input
+    .replace(/&/g, '&')
+    .replace(/</g, '&lt;')
+    .replace(/>/g, '&gt;')
+    .replace(/"/g, '&quot;')
+    .replace(/'/g, '&#39;');
+}
+
+/**
+ * The error thrown by API route handlers.
+ *
+ * Because 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.
+ *
+ * If you write your own error handler, you should **not** render the error message
+ * without using a templating engine that will properly escape it for other HTML contexts first.
+ *
+ * @category Server
+ */
+export class HandlerError extends Error {
+  public status: number | undefined;
+  public code: string | undefined;
+
+  constructor(error: Error | AccessTokenError | HttpError) {
+    super(htmlSafe(error.message));
+
+    this.name = error.name;
+
+    if ('code' in error) {
+      this.code = error.code;
+    }
+
+    if ('status' in error) {
+      this.status = error.status;
+    }
+  }
+}

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

@@ -377,7 +377,7 @@ describe('callback', () => {
     const redirectUri = 'http://messi:3000/api/auth/callback/runtime';
     const baseURL = await setup(defaultConfig, { callbackOptions: { redirectUri } });
     const state = encodeState({ foo: 'bar' });
-    const cookieJar = toSignedCookieJar( { state, nonce: '__test_nonce__' }, baseURL);
+    const cookieJar = toSignedCookieJar({ state, nonce: '__test_nonce__' }, baseURL);
     const { res } = await post(baseURL, '/callback', {
       body: {
         state: state,

tests/fixtures/oidc-nocks.ts

@@ -4,6 +4,16 @@ import { ConfigParameters } from '../../src';
 import { makeIdToken } from '../auth0-session/fixtures/cert';
 
 export function discovery(params: ConfigParameters, discoveryOptions?: any): nock.Scope {
+  const { error, ...metadata } = discoveryOptions || {};
+
+  if (error) {
+    return nock(params.issuerBaseURL as string)
+      .get('/.well-known/openid-configuration')
+      .reply(500, { error })
+      .get('/.well-known/oauth-authorization-server')
+      .reply(500, { error });
+  }
+
   return nock(params.issuerBaseURL as string)
     .get('/.well-known/openid-configuration')
     .reply(200, () => {
@@ -50,7 +60,7 @@ export function discovery(params: ConfigParameters, discoveryOptions?: any): noc
           'picture',
           'sub'
         ],
-        ...(discoveryOptions || {})
+        ...metadata
       };
     });
 }

tests/handlers/callback.test.ts

@@ -89,6 +89,13 @@ describe('callback handler', () => {
     ).rejects.toThrow('unexpected iss value, expected https://acme.auth0.local/, got: other-issuer');
   });
 
+  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(
+      '<script>alert('xss')</script>'
+    );
+  });
+
   test('should create the session without OIDC claims', async () => {
     const baseUrl = await setup(withoutApi);
     const state = encodeState({ returnTo: baseUrl });
@@ -322,7 +329,9 @@ describe('callback handler', () => {
         },
         cookieJar
       )
-    ).rejects.toThrow('Organization Id (org_id) claim value mismatch in the ID token; expected "foo", found "bar"');
+    ).rejects.toThrow(
+      'Organization Id (org_id) claim value mismatch in the ID token; expected "foo", found "bar"'
+    );
   });
 
   test('accepts a valid organization', async () => {

tests/handlers/login.test.ts

@@ -244,6 +244,14 @@ describe('login handler', () => {
     ).rejects.toThrow('Invalid value provided for returnTo, must be a relative url');
   });
 
+  test('should escape html in errors', 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',