Skip to content

Commit

Permalink
Fix reflected XSS from the callback handler's error query parameter
Browse files Browse the repository at this point in the history
  • Loading branch information
adamjmcgrath committed Jun 16, 2021
1 parent 36655df commit 7a84581
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 2 deletions.
17 changes: 17 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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).
Expand Down
14 changes: 13 additions & 1 deletion src/auth0-session/handlers/callback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, '&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 = {
Expand Down Expand Up @@ -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);
Expand Down
10 changes: 9 additions & 1 deletion tests/auth0-session/handlers/callback.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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=<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,
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions tests/handlers/callback.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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=<script>alert(1)</script>`)).rejects.toThrow(
'&lt;script&gt;alert(1)&lt;/script&gt;'
);
});

test('should create the session without OIDC claims', async () => {
const baseUrl = await setup(withoutApi);
const state = encodeState({ returnTo: baseUrl });
Expand Down

0 comments on commit 7a84581

Please sign in to comment.