From 7a8458127752c4291349f57f037638b6e91d30bd Mon Sep 17 00:00:00 2001 From: adamjmcgrath Date: Wed, 16 Jun 2021 19:20:49 +0100 Subject: [PATCH 1/2] Fix reflected XSS from the callback handler's error query parameter --- README.md | 17 +++++++++++++++++ src/auth0-session/handlers/callback.ts | 14 +++++++++++++- tests/auth0-session/handlers/callback.test.ts | 10 +++++++++- tests/handlers/callback.test.ts | 7 +++++++ 4 files changed, 46 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 3e43a33ad..c73c2cb62 100644 --- a/README.md +++ b/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). diff --git a/src/auth0-session/handlers/callback.ts b/src/auth0-session/handlers/callback.ts index 8328afbd0..b58d88279 100644 --- a/src/auth0-session/handlers/callback.ts +++ b/src/auth0-session/handlers/callback.ts @@ -11,6 +11,17 @@ 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, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); +} + export type AfterCallback = (req: any, res: any, session: any, state: Record) => Promise | any; export type CallbackOptions = { @@ -47,7 +58,8 @@ export default function callbackHandlerFactory( state: expectedState }); } catch (err) { - throw new BadRequest(err.message); + // The error message can come from the route's query parameters, so do some basic escaping. + throw new BadRequest(htmlSafe(err.message)); } const openidState: { returnTo?: string } = decodeState(expectedState as string); diff --git a/tests/auth0-session/handlers/callback.test.ts b/tests/auth0-session/handlers/callback.test.ts index c0b5a4879..f12bca4c8 100644 --- a/tests/auth0-session/handlers/callback.test.ts +++ b/tests/auth0-session/handlers/callback.test.ts @@ -239,6 +239,14 @@ 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=')).rejects.toThrowError( + '<script>alert(1)</script>' + ); + }); + it("should expose all tokens when id_token is valid and response_type is 'code id_token'", async () => { const baseURL = await setup({ ...defaultConfig, @@ -377,7 +385,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, diff --git a/tests/handlers/callback.test.ts b/tests/handlers/callback.test.ts index 95390dae8..28a7be2d8 100644 --- a/tests/handlers/callback.test.ts +++ b/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=`)).rejects.toThrow( + '<script>alert(1)</script>' + ); + }); + test('should create the session without OIDC claims', async () => { const baseUrl = await setup(withoutApi); const state = encodeState({ returnTo: baseUrl }); From e051d6abd1738d1ac614c7722bc2d1ad967c01a2 Mon Sep 17 00:00:00 2001 From: adamjmcgrath Date: Thu, 17 Jun 2021 11:54:39 +0100 Subject: [PATCH 2/2] Wrap every handler to escape HTML, not just callback --- src/auth0-session/handlers/callback.ts | 14 +--- src/handlers/callback.ts | 15 +++-- src/handlers/login.ts | 33 +++++---- src/handlers/logout.ts | 9 ++- src/handlers/profile.ts | 67 ++++++++++--------- src/utils/errors.ts | 43 ++++++++++++ tests/auth0-session/handlers/callback.test.ts | 8 --- tests/fixtures/oidc-nocks.ts | 12 +++- tests/handlers/callback.test.ts | 8 ++- tests/handlers/login.test.ts | 8 +++ tests/handlers/logout.test.ts | 20 +++++- tests/handlers/profile.test.ts | 19 ++++++ 12 files changed, 178 insertions(+), 78 deletions(-) diff --git a/src/auth0-session/handlers/callback.ts b/src/auth0-session/handlers/callback.ts index b58d88279..8328afbd0 100644 --- a/src/auth0-session/handlers/callback.ts +++ b/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, '>') - .replace(/"/g, '"') - .replace(/'/g, '''); -} - export type AfterCallback = (req: any, res: any, session: any, state: Record) => Promise | 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); diff --git a/src/handlers/callback.ts b/src/handlers/callback.ts index f1a0ad7f5..95a7a0be3 100644 --- a/src/handlers/callback.ts +++ b/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 => { - 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); + } }; } diff --git a/src/handlers/login.ts b/src/handlers/login.ts index 3dafb152b..94e3631fe 100644 --- a/src/handlers/login.ts +++ b/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 => { - 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); }; } diff --git a/src/handlers/logout.ts b/src/handlers/logout.ts index 02c24cd6e..1940f0e0c 100644 --- a/src/handlers/logout.ts +++ b/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 => { - assertReqRes(req, res); - return handler(req, res, options); + try { + assertReqRes(req, res); + return await handler(req, res, options); + } catch (e) { + throw new HandlerError(e); + } }; } diff --git a/src/handlers/profile.ts b/src/handlers/profile.ts index 4c89c496d..02ccff441 100644 --- a/src/handlers/profile.ts +++ b/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; @@ -39,46 +40,50 @@ export default function profileHandler( sessionCache: SessionCache ): HandleProfile { return async (req, res, options): Promise => { - 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); }; } diff --git a/src/utils/errors.ts b/src/utils/errors.ts index 5f99f387f..ee1353666 100644 --- a/src/utils/errors.ts +++ b/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, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); +} + +/** + * 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; + } + } +} diff --git a/tests/auth0-session/handlers/callback.test.ts b/tests/auth0-session/handlers/callback.test.ts index f12bca4c8..894b10363 100644 --- a/tests/auth0-session/handlers/callback.test.ts +++ b/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=')).rejects.toThrowError( - '<script>alert(1)</script>' - ); - }); - it("should expose all tokens when id_token is valid and response_type is 'code id_token'", async () => { const baseURL = await setup({ ...defaultConfig, diff --git a/tests/fixtures/oidc-nocks.ts b/tests/fixtures/oidc-nocks.ts index cb5fb08fe..8271411eb 100644 --- a/tests/fixtures/oidc-nocks.ts +++ b/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 }; }); } diff --git a/tests/handlers/callback.test.ts b/tests/handlers/callback.test.ts index 28a7be2d8..235f66abe 100644 --- a/tests/handlers/callback.test.ts +++ b/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=`)).rejects.toThrow( - '<script>alert(1)</script>' + await expect(get(baseUrl, `/api/auth/callback?error=%3Cscript%3Ealert(%27xss%27)%3C%2Fscript%3E`)).rejects.toThrow( + '<script>alert('xss')</script>' ); }); @@ -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 "foo", found "bar"' + ); }); test('accepts a valid organization', async () => { diff --git a/tests/handlers/login.test.ts b/tests/handlers/login.test.ts index 52fdedc5d..88bc9e1d4 100644 --- a/tests/handlers/login.test.ts +++ b/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: '' } }); + + await expect(get(baseUrl, '/api/auth/login', { fullResponse: true })).rejects.toThrow( + '<script>alert("xss")</script>' + ); + }); + 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/handlers/logout.test.ts b/tests/handlers/logout.test.ts index e38f7dec0..111dfc63e 100644 --- a/tests/handlers/logout.test.ts +++ b/tests/handlers/logout.test.ts @@ -1,7 +1,17 @@ import { parse } from 'cookie'; -import { parse as parseUrl } from 'url'; +import { parse as parseUrl, URL } from 'url'; import { withoutApi } from '../fixtures/default-settings'; import { setup, teardown, login } from '../fixtures/setup'; +import { IncomingMessage } from 'http'; + +jest.mock('../../src/utils/assert', () => ({ + assertReqRes(req: IncomingMessage) { + if (req.url?.includes('error=')) { + const url = new URL(req.url, 'http://example.com'); + throw new Error(url.searchParams.get('error') as string); + } + } +})); describe('logout handler', () => { afterEach(teardown); @@ -88,4 +98,12 @@ describe('logout handler', () => { Path: '/' }); }); + + test('should escape html in errors', async () => { + const baseUrl = await setup(withoutApi); + + const res = await fetch(`${baseUrl}/api/auth/logout?error=%3Cscript%3Ealert(%27xss%27)%3C%2Fscript%3E`); + + expect(await res.text()).toEqual('<script>alert('xss')</script>'); + }); }); diff --git a/tests/handlers/profile.test.ts b/tests/handlers/profile.test.ts index 8cd97c192..5e83ecb8b 100644 --- a/tests/handlers/profile.test.ts +++ b/tests/handlers/profile.test.ts @@ -5,6 +5,17 @@ import { get } from '../auth0-session/fixtures/helpers'; import { setup, teardown, login } from '../fixtures/setup'; import { Session, AfterCallback } from '../../src'; import { makeIdToken } from '../auth0-session/fixtures/cert'; +import { IncomingMessage } from 'http'; +import { URL } from 'url'; + +jest.mock('../../src/utils/assert', () => ({ + assertReqRes(req: IncomingMessage) { + if (req.url?.includes('error=')) { + const url = new URL(req.url, 'http://example.com'); + throw new Error(url.searchParams.get('error') as string); + } + } +})); describe('profile handler', () => { afterEach(teardown); @@ -142,4 +153,12 @@ describe('profile handler', () => { await expect(get(baseUrl, '/api/auth/me', { cookieJar })).rejects.toThrowError('some validation error'); }); + + test('should escape html in errors', async () => { + const baseUrl = await setup(withoutApi); + + const res = await fetch(`${baseUrl}/api/auth/me?error=%3Cscript%3Ealert(%27xss%27)%3C%2Fscript%3E`); + + expect(await res.text()).toEqual('<script>alert('xss')</script>'); + }); });