Wrap every handler to escape HTML, not just callback

Author: adamjmcgrath

src/auth0-session/handlers/callback.ts

@@ -11,17 +11,6 @@ function getRedirectUri(config: Config): string {
   return urlJoin(config.baseURL, config.routes.callback);
 }
 
-// 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;');
-}
-
 export type AfterCallback = (req: any, res: any, session: any, state: Record<string, any>) => Promise<any> | any;
 
 export type CallbackOptions = {
@@ -58,8 +47,7 @@ export default function callbackHandlerFactory(
         state: expectedState
       });
     } catch (err) {
-      // The error message can come from the route's query parameters, so do some basic escaping.
-      throw new BadRequest(htmlSafe(err.message));
+      throw new BadRequest(err.message);
     }
 
     const openidState: { returnTo?: string } = decodeState(expectedState as string);

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

@@ -239,14 +239,6 @@ describe('callback', () => {
     expect(session.claims).toEqual(expect.objectContaining(expected));
   });
 
-  it('should escape html in error qp', async () => {
-    const baseURL = await setup(defaultConfig);
-
-    await expect(get(baseURL, '/callback?error=<script>alert(1)</script>')).rejects.toThrowError(
-      '&lt;script&gt;alert(1)&lt;/script&gt;'
-    );
-  });
-
   it("should expose all tokens when id_token is valid and response_type is 'code id_token'", async () => {
     const baseURL = await setup({
       ...defaultConfig,

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

@@ -91,8 +91,8 @@ describe('callback handler', () => {
 
   it('should escape html in error qp', async () => {
     const baseUrl = await setup(withoutApi);
-    await expect(get(baseUrl, `/api/auth/callback?error=<script>alert(1)</script>`)).rejects.toThrow(
-      '&lt;script&gt;alert(1)&lt;/script&gt;'
+    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;'
     );
   });
 
@@ -329,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 &quot;foo&quot;, found &quot;bar&quot;'
+    );
   });
 
   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',