From db30e21b5a2f22640e2dbdd8cbc7758d3ea1a3fb Mon Sep 17 00:00:00 2001 From: Steve Hobbs Date: Tue, 16 Nov 2021 11:48:09 +0000 Subject: [PATCH 1/3] Exclude nowProvider from authorize URL params --- src/Auth0Client.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Auth0Client.ts b/src/Auth0Client.ts index 95570b942..9f75360da 100644 --- a/src/Auth0Client.ts +++ b/src/Auth0Client.ts @@ -319,6 +319,7 @@ export default class Auth0Client { cacheLocation, advancedOptions, detailedResponse, + nowProvider, ...withoutClientOptions } = this.options; From 92d3ae6d03d3741d73d4dcc6304063a88ebc4481 Mon Sep 17 00:00:00 2001 From: Steve Hobbs Date: Tue, 16 Nov 2021 14:30:16 +0000 Subject: [PATCH 2/3] Added test to ensure client options not passed in authorize URL --- __tests__/Auth0Client/constructor.test.ts | 11 +-- __tests__/Auth0Client/helpers.ts | 35 ++++++++-- .../Auth0Client/loginWithRedirect.test.ts | 70 ++++++++++++++++--- src/Auth0Client.ts | 15 ++-- tsconfig.test.json | 3 +- 5 files changed, 103 insertions(+), 31 deletions(-) diff --git a/__tests__/Auth0Client/constructor.test.ts b/__tests__/Auth0Client/constructor.test.ts index b29d4fd94..169c07c39 100644 --- a/__tests__/Auth0Client/constructor.test.ts +++ b/__tests__/Auth0Client/constructor.test.ts @@ -7,7 +7,7 @@ import * as scope from '../../src/scope'; // @ts-ignore -import { loginWithRedirectFn, setupFn } from './helpers'; +import { assertUrlEquals, loginWithRedirectFn, setupFn } from './helpers'; import { TEST_CLIENT_ID, TEST_CODE_CHALLENGE, TEST_DOMAIN } from '../constants'; import { ICache } from '../../src/cache'; @@ -33,15 +33,6 @@ jest jest.spyOn(utils, 'runPopup'); -const assertUrlEquals = (actualUrl, host, path, queryParams) => { - const url = new URL(actualUrl); - expect(url.host).toEqual(host); - expect(url.pathname).toEqual(path); - for (let [key, value] of Object.entries(queryParams)) { - expect(url.searchParams.get(key)).toEqual(value); - } -}; - const setup = setupFn(mockVerify); describe('Auth0Client', () => { diff --git a/__tests__/Auth0Client/helpers.ts b/__tests__/Auth0Client/helpers.ts index f85ac772c..c3d0f9e81 100644 --- a/__tests__/Auth0Client/helpers.ts +++ b/__tests__/Auth0Client/helpers.ts @@ -27,7 +27,7 @@ const authorizationResponse: AuthenticationResult = { state: TEST_STATE }; -export const assertPostFn = mockFetch => { +export const assertPostFn = (mockFetch: jest.Mock) => { return ( url: string, body: any, @@ -51,12 +51,37 @@ export const assertPostFn = mockFetch => { }; }; -export const assertUrlEquals = (actualUrl, host, path, queryParams) => { +/** + * Extracts the keys and values from an IterableIterator and applies + * them to an object. + * @param itor The iterable + * @returns An object with the keys and values from the iterable + */ +const itorToObject = ( + itor: IterableIterator<[K, V]> +): Record => + [...itor].reduce((m, [key, value]) => { + m[key] = value; + return m; + }, {} as Record); + +export const assertUrlEquals = ( + actualUrl: URL | string, + host: string, + path: string, + queryParams: Record, + strict: boolean = false +) => { const url = new URL(actualUrl); + const searchParamsObj = itorToObject(url.searchParams.entries()); + expect(url.host).toEqual(host); expect(url.pathname).toEqual(path); - for (let [key, value] of Object.entries(queryParams)) { - expect(url.searchParams.get(key)).toEqual(value); + + if (strict) { + expect(searchParamsObj).toStrictEqual(queryParams); + } else { + expect(searchParamsObj).toMatchObject(queryParams); } }; @@ -66,7 +91,7 @@ export const fetchResponse = (ok, json) => json: () => Promise.resolve(json) }); -export const setupFn = mockVerify => { +export const setupFn = (mockVerify: jest.Mock) => { return (config?: Partial, claims?: Partial) => { const auth0 = new Auth0Client( Object.assign( diff --git a/__tests__/Auth0Client/loginWithRedirect.test.ts b/__tests__/Auth0Client/loginWithRedirect.test.ts index a74b40ca0..17b8fa2ec 100644 --- a/__tests__/Auth0Client/loginWithRedirect.test.ts +++ b/__tests__/Auth0Client/loginWithRedirect.test.ts @@ -109,17 +109,24 @@ describe('Auth0Client', () => { const url = new URL(mockWindow.location.assign.mock.calls[0][0]); - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - client_id: TEST_CLIENT_ID, - redirect_uri: TEST_REDIRECT_URI, - scope: TEST_SCOPES, - response_type: 'code', - response_mode: 'query', - state: TEST_STATE, - nonce: TEST_NONCE, - code_challenge: TEST_CODE_CHALLENGE, - code_challenge_method: 'S256' - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + auth0Client: expect.any(String), + client_id: TEST_CLIENT_ID, + redirect_uri: TEST_REDIRECT_URI, + scope: TEST_SCOPES, + response_type: 'code', + response_mode: 'query', + state: TEST_STATE, + nonce: TEST_NONCE, + code_challenge: TEST_CODE_CHALLENGE, + code_challenge_method: 'S256' + }, + true // strict mode + ); assertPost( 'https://auth0_domain/oauth/token', @@ -453,5 +460,46 @@ describe('Auth0Client', () => { } ); }); + + it('should not include client options on the URL', async () => { + // ** IMPORTANT **: if adding a new client option, ensure it is added to the destructure + // list in Auth0Client._getParams so that it is not sent to the IdP + const auth0 = setup({ + useRefreshTokens: true, + advancedOptions: { + defaultScope: 'openid profile email offline_access' + }, + useCookiesForTransactions: true, + authorizeTimeoutInSeconds: 10, + cacheLocation: 'localstorage', + legacySameSiteCookie: true, + nowProvider: () => Date.now(), + sessionCheckExpiryDays: 1, + useFormData: true + }); + + await loginWithRedirect(auth0); + + const url = new URL(mockWindow.location.assign.mock.calls[0][0]); + + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + auth0Client: expect.any(String), + client_id: TEST_CLIENT_ID, + redirect_uri: TEST_REDIRECT_URI, + scope: 'openid profile email offline_access', + response_type: 'code', + response_mode: 'query', + state: TEST_STATE, + nonce: TEST_NONCE, + code_challenge: TEST_CODE_CHALLENGE, + code_challenge_method: 'S256' + }, + true // strict mode + ); + }); }); }); diff --git a/src/Auth0Client.ts b/src/Auth0Client.ts index 9f75360da..19b8f01b5 100644 --- a/src/Auth0Client.ts +++ b/src/Auth0Client.ts @@ -309,9 +309,10 @@ export default class Auth0Client { code_challenge: string, redirect_uri: string ): AuthorizeOptions { + // These options should be excluded from the authorize URL, + // as they're options for the client and not for the IdP. + // ** IMPORTANT ** If adding a new client option, include it in this destructure list. const { - domain, - leeway, useRefreshTokens, useCookiesForTransactions, useFormData, @@ -320,11 +321,16 @@ export default class Auth0Client { advancedOptions, detailedResponse, nowProvider, - ...withoutClientOptions + authorizeTimeoutInSeconds, + legacySameSiteCookie, + sessionCheckExpiryDays, + domain, + leeway, + ...loginOptions } = this.options; return { - ...withoutClientOptions, + ...loginOptions, ...authorizeOptions, scope: getUniqueScopes( this.defaultScope, @@ -340,6 +346,7 @@ export default class Auth0Client { code_challenge_method: 'S256' }; } + private _authorizeUrl(authorizeOptions: AuthorizeOptions) { return this._url(`/authorize?${createQueryParams(authorizeOptions)}`); } diff --git a/tsconfig.test.json b/tsconfig.test.json index 483e75cd0..462614d96 100644 --- a/tsconfig.test.json +++ b/tsconfig.test.json @@ -1,6 +1,7 @@ { "extends": "./tsconfig.json", "compilerOptions": { - "noImplicitAny": false + "noImplicitAny": false, + "target": "es6" } } From 71bba1699512af5fc31b4b925566813c7cb2c277 Mon Sep 17 00:00:00 2001 From: Steve Hobbs Date: Tue, 16 Nov 2021 15:04:01 +0000 Subject: [PATCH 3/3] Default strict URL mode to true, clean up tests --- .../Auth0Client/getTokenSilently.test.ts | 25 +++- __tests__/Auth0Client/helpers.ts | 16 ++- __tests__/Auth0Client/loginWithPopup.test.ts | 64 ++++++--- .../Auth0Client/loginWithRedirect.test.ts | 130 ++++++++++-------- 4 files changed, 156 insertions(+), 79 deletions(-) diff --git a/__tests__/Auth0Client/getTokenSilently.test.ts b/__tests__/Auth0Client/getTokenSilently.test.ts index 709704c91..673428bc9 100644 --- a/__tests__/Auth0Client/getTokenSilently.test.ts +++ b/__tests__/Auth0Client/getTokenSilently.test.ts @@ -146,6 +146,7 @@ describe('Auth0Client', () => { const [[url]] = (utils.runIframe).mock.calls; assertUrlEquals(url, 'auth0_domain', '/authorize', { + scope: TEST_SCOPES, client_id: TEST_CLIENT_ID, response_type: 'code', response_mode: 'web_message', @@ -174,9 +175,15 @@ describe('Auth0Client', () => { const [[url]] = (utils.runIframe).mock.calls; - assertUrlEquals(url, 'auth0_domain', '/authorize', { - redirect_uri - }); + assertUrlEquals( + url, + 'auth0_domain', + '/authorize', + { + redirect_uri + }, + false + ); }); it('calls the token endpoint with the correct params', async () => { @@ -385,9 +392,15 @@ describe('Auth0Client', () => { await getTokenSilently(auth0); const [[url]] = (utils.runIframe).mock.calls; - assertUrlEquals(url, 'auth0_domain', '/authorize', { - scope: 'openid email' - }); + assertUrlEquals( + url, + 'auth0_domain', + '/authorize', + { + scope: 'openid email' + }, + false + ); }); it('refreshes the token using custom default scope when using refresh tokens', async () => { diff --git a/__tests__/Auth0Client/helpers.ts b/__tests__/Auth0Client/helpers.ts index c3d0f9e81..0efc0ff75 100644 --- a/__tests__/Auth0Client/helpers.ts +++ b/__tests__/Auth0Client/helpers.ts @@ -65,12 +65,21 @@ const itorToObject = ( return m; }, {} as Record); +/** + * Asserts that the supplied URL matches various criteria, including host, path, and query params. + * @param actualUrl The URL + * @param host The host + * @param path The URL path + * @param queryParams The query parameters to check + * @param strict If true, the query parameters must match the URL parameters exactly. Otherwise, a loose match is performed to check that + * the parameters passed in at least appear in the URL (but the URL can have extra ones). Default is true. + */ export const assertUrlEquals = ( actualUrl: URL | string, host: string, path: string, queryParams: Record, - strict: boolean = false + strict: boolean = true ) => { const url = new URL(actualUrl); const searchParamsObj = itorToObject(url.searchParams.entries()); @@ -79,7 +88,10 @@ export const assertUrlEquals = ( expect(url.pathname).toEqual(path); if (strict) { - expect(searchParamsObj).toStrictEqual(queryParams); + expect(searchParamsObj).toStrictEqual({ + auth0Client: expect.any(String), + ...queryParams + }); } else { expect(searchParamsObj).toMatchObject(queryParams); } diff --git a/__tests__/Auth0Client/loginWithPopup.test.ts b/__tests__/Auth0Client/loginWithPopup.test.ts index 40144650d..192fe7b79 100644 --- a/__tests__/Auth0Client/loginWithPopup.test.ts +++ b/__tests__/Auth0Client/loginWithPopup.test.ts @@ -149,10 +149,16 @@ describe('Auth0Client', () => { // prettier-ignore const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href; - assertUrlEquals(url, 'auth0_domain', '/authorize', { - state: TEST_STATE, - nonce: TEST_NONCE - }); + assertUrlEquals( + url, + 'auth0_domain', + '/authorize', + { + state: TEST_STATE, + nonce: TEST_NONCE + }, + false + ); }); it('creates `code_challenge` by using `utils.sha256` with the result of `utils.createRandomString`', async () => { @@ -163,10 +169,16 @@ describe('Auth0Client', () => { // prettier-ignore const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href; - assertUrlEquals(url, 'auth0_domain', '/authorize', { - code_challenge: TEST_CODE_CHALLENGE, - code_challenge_method: 'S256' - }); + assertUrlEquals( + url, + 'auth0_domain', + '/authorize', + { + code_challenge: TEST_CODE_CHALLENGE, + code_challenge_method: 'S256' + }, + false + ); }); it('should log the user in with a popup and redirect using a default redirect URI', async () => { @@ -235,9 +247,15 @@ describe('Auth0Client', () => { // prettier-ignore const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href; - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - scope: `${TEST_SCOPES} offline_access` - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + scope: `${TEST_SCOPES} offline_access` + }, + false + ); }); it('should log the user and redirect when using different default redirect_uri', async () => { @@ -250,9 +268,15 @@ describe('Auth0Client', () => { // prettier-ignore const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href; - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - redirect_uri - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + redirect_uri + }, + false + ); }); it('should log the user in with a popup and get the token', async () => { @@ -398,9 +422,15 @@ describe('Auth0Client', () => { // prettier-ignore const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href; - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - auth0Client: btoa(JSON.stringify(auth0Client)) - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + auth0Client: btoa(JSON.stringify(auth0Client)) + }, + false + ); }); it('throws error if state from popup response is different from the provided state', async () => { diff --git a/__tests__/Auth0Client/loginWithRedirect.test.ts b/__tests__/Auth0Client/loginWithRedirect.test.ts index 17b8fa2ec..28eae44f7 100644 --- a/__tests__/Auth0Client/loginWithRedirect.test.ts +++ b/__tests__/Auth0Client/loginWithRedirect.test.ts @@ -109,24 +109,17 @@ describe('Auth0Client', () => { const url = new URL(mockWindow.location.assign.mock.calls[0][0]); - assertUrlEquals( - url, - TEST_DOMAIN, - '/authorize', - { - auth0Client: expect.any(String), - client_id: TEST_CLIENT_ID, - redirect_uri: TEST_REDIRECT_URI, - scope: TEST_SCOPES, - response_type: 'code', - response_mode: 'query', - state: TEST_STATE, - nonce: TEST_NONCE, - code_challenge: TEST_CODE_CHALLENGE, - code_challenge_method: 'S256' - }, - true // strict mode - ); + assertUrlEquals(url, TEST_DOMAIN, '/authorize', { + client_id: TEST_CLIENT_ID, + redirect_uri: TEST_REDIRECT_URI, + scope: TEST_SCOPES, + response_type: 'code', + response_mode: 'query', + state: TEST_STATE, + nonce: TEST_NONCE, + code_challenge: TEST_CODE_CHALLENGE, + code_challenge_method: 'S256' + }); assertPost( 'https://auth0_domain/oauth/token', @@ -159,9 +152,15 @@ describe('Auth0Client', () => { const url = new URL(mockWindow.location.assign.mock.calls[0][0]); - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - scope: 'openid email' - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + scope: 'openid email' + }, + false + ); }); it('should log the user in using different default redirect_uri', async () => { @@ -175,9 +174,15 @@ describe('Auth0Client', () => { const url = new URL(mockWindow.location.assign.mock.calls[0][0]); - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - redirect_uri - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + redirect_uri + }, + false + ); }); it('should log the user in when overriding default redirect_uri', async () => { @@ -193,9 +198,15 @@ describe('Auth0Client', () => { const url = new URL(mockWindow.location.assign.mock.calls[0][0]); - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - redirect_uri: 'https://my-redirect-uri/callback' - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + redirect_uri: 'https://my-redirect-uri/callback' + }, + false + ); }); it('should log the user in by calling window.location.replace when redirectMethod=replace param is passed', async () => { @@ -208,9 +219,15 @@ describe('Auth0Client', () => { const url = new URL(mockWindow.location.replace.mock.calls[0][0]); - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - audience: 'test_audience' - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + audience: 'test_audience' + }, + false + ); }); it('should log the user in with custom params', async () => { @@ -222,9 +239,15 @@ describe('Auth0Client', () => { const url = new URL(mockWindow.location.assign.mock.calls[0][0]); - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - audience: 'test_audience' - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + audience: 'test_audience' + }, + false + ); }); it('should log the user in using offline_access when using refresh tokens', async () => { @@ -236,9 +259,15 @@ describe('Auth0Client', () => { const url = new URL(mockWindow.location.assign.mock.calls[0][0]); - assertUrlEquals(url, TEST_DOMAIN, '/authorize', { - scope: `${TEST_SCOPES} offline_access` - }); + assertUrlEquals( + url, + TEST_DOMAIN, + '/authorize', + { + scope: `${TEST_SCOPES} offline_access` + }, + false + ); }); it('should log the user in and get the user', async () => { @@ -482,24 +511,17 @@ describe('Auth0Client', () => { const url = new URL(mockWindow.location.assign.mock.calls[0][0]); - assertUrlEquals( - url, - TEST_DOMAIN, - '/authorize', - { - auth0Client: expect.any(String), - client_id: TEST_CLIENT_ID, - redirect_uri: TEST_REDIRECT_URI, - scope: 'openid profile email offline_access', - response_type: 'code', - response_mode: 'query', - state: TEST_STATE, - nonce: TEST_NONCE, - code_challenge: TEST_CODE_CHALLENGE, - code_challenge_method: 'S256' - }, - true // strict mode - ); + assertUrlEquals(url, TEST_DOMAIN, '/authorize', { + client_id: TEST_CLIENT_ID, + redirect_uri: TEST_REDIRECT_URI, + scope: 'openid profile email offline_access', + response_type: 'code', + response_mode: 'query', + state: TEST_STATE, + nonce: TEST_NONCE, + code_challenge: TEST_CODE_CHALLENGE, + code_challenge_method: 'S256' + }); }); }); });