Repository: auth0/auth0-spa-js (ts)

Issue:
If providing a nowProvider during client creation, the value/function is being encoded in calls to the /authorize endpoint
### Describe the problem

The recently added nowProvider parameter to Auth0ClientOptions is wrongly being encoded and sent as a url query parameter to the /authorize endpoint.  If an Auth0 tenant is fronted by Akamai, then Akamai rejects/403s the request (possible attack detection).

The culprit seems to be the utils.ts::createQueryParams function receiving the nowProvider parameter from upstream and not filtering it out while building the /authorize url parameters.

### What was the expected behavior?

The value passed to nowProvider should not be sent as a url query parameter to the /authorize endpoint.

- **Version of `auth0-spa-js` used:**
- 1.19.2
- 
- **Which browsers have you tested in?**
- Edge, Chrome
- 
- **Which framework are you using, if applicable (Angular, React, etc):**
- SPA SDK
- 




Pull request:
Prevent nowProvider from being passed to authorize endpoint
<!-- By submitting a PR to this repository, you agree to the terms within the [Auth0 Code of Conduct](https://github.com/auth0/open-source-template/blob/master/CODE-OF-CONDUCT.md). Please see the [contributing guidelines](https://github.com/auth0/.github/blob/master/CONTRIBUTING.md) for how to create and submit a high-quality PR for this repo. -->

### Changes

<!--
Please describe both what is changing and why this is important. Include:
- Classes and methods added, deleted, deprecated, or changed
- Screenshots of new or changed UI, if applicable
- A summary of usage if this is a new feature or change to a public API (this should also be added to relevant documentation once released)
-->

This PR prevents `nowProvider` from being passed through the URL when logging in. The fundamental change is that `nowProvider` is added to the destructure list in `Auth0Client._getParams`.

In addition, I've attemped to help prevent this in the future by adding a test that includes all the client options and verifying they're not in the URL. Adding this test flagged up a couple other properties that should have been excluded, so these have also been added.

Also did some refactoring in `_getParams` to tidy up the code a bit.

### References

<!--
Please include relevant links supporting this change such as a:

- support ticket
- community post
- StackOverflow post
- support forum thread

Please note any links that are not publicly accessible.
-->

Fixes #836

### Testing

<!--
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.
-->

- [x] This change adds unit test coverage
- [ ] This change adds integration test coverage
- [x] This change has been tested on the latest version of the platform/language

### Checklist

- [x] I have read the [Auth0 general contribution guidelines](https://github.com/auth0/open-source-template/blob/master/GENERAL-CONTRIBUTING.md)
- [x] I have read the [Auth0 Code of Conduct](https://github.com/auth0/open-source-template/blob/master/CODE-OF-CONDUCT.md)
- [x] All code quality tools/guidelines have been run/followed


Patch:
diff --git a/src/Auth0Client.ts b/src/Auth0Client.ts
index 95570b9..19b8f01 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,
@@ -319,11 +320,17 @@ export default class Auth0Client {
       cacheLocation,
       advancedOptions,
       detailedResponse,
-      ...withoutClientOptions
+      nowProvider,
+      authorizeTimeoutInSeconds,
+      legacySameSiteCookie,
+      sessionCheckExpiryDays,
+      domain,
+      leeway,
+      ...loginOptions
     } = this.options;
 
     return {
-      ...withoutClientOptions,
+      ...loginOptions,
       ...authorizeOptions,
       scope: getUniqueScopes(
         this.defaultScope,
@@ -339,6 +346,7 @@ export default class Auth0Client {
       code_challenge_method: 'S256'
     };
   }
+
   private _authorizeUrl(authorizeOptions: AuthorizeOptions) {
     return this._url(`/authorize?${createQueryParams(authorizeOptions)}`);
   }


Test patch:
diff --git a/__tests__/Auth0Client/constructor.test.ts b/__tests__/Auth0Client/constructor.test.ts
index b29d4fd..169c07c 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/getTokenSilently.test.ts b/__tests__/Auth0Client/getTokenSilently.test.ts
index 709704c..673428b 100644
--- a/__tests__/Auth0Client/getTokenSilently.test.ts
+++ b/__tests__/Auth0Client/getTokenSilently.test.ts
@@ -146,6 +146,7 @@ describe('Auth0Client', () => {
       const [[url]] = (<jest.Mock>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]] = (<jest.Mock>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]] = (<jest.Mock>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 f85ac77..0efc0ff 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,49 @@ 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 = <K extends string | number | symbol, V>(
+  itor: IterableIterator<[K, V]>
+): Record<K, V> =>
+  [...itor].reduce((m, [key, value]) => {
+    m[key] = value;
+    return m;
+  }, {} as Record<K, V>);
+
+/**
+ * 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<string, string>,
+  strict: boolean = true
+) => {
   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({
+      auth0Client: expect.any(String),
+      ...queryParams
+    });
+  } else {
+    expect(searchParamsObj).toMatchObject(queryParams);
   }
 };
 
@@ -66,7 +103,7 @@ export const fetchResponse = (ok, json) =>
     json: () => Promise.resolve(json)
   });
 
-export const setupFn = mockVerify => {
+export const setupFn = (mockVerify: jest.Mock) => {
   return (config?: Partial<Auth0ClientOptions>, claims?: Partial<IdToken>) => {
     const auth0 = new Auth0Client(
       Object.assign(
diff --git a/__tests__/Auth0Client/loginWithPopup.test.ts b/__tests__/Auth0Client/loginWithPopup.test.ts
index 4014465..192fe7b 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 a74b40c..28eae44 100644
--- a/__tests__/Auth0Client/loginWithRedirect.test.ts
+++ b/__tests__/Auth0Client/loginWithRedirect.test.ts
@@ -152,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 () => {
@@ -168,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 () => {
@@ -186,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 () => {
@@ -201,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 () => {
@@ -215,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 () => {
@@ -229,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 () => {
@@ -453,5 +489,39 @@ 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', {
+        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'
+      });
+    });
   });
 });
diff --git a/tsconfig.test.json b/tsconfig.test.json
index 483e75c..462614d 100644
--- a/tsconfig.test.json
+++ b/tsconfig.test.json
@@ -1,6 +1,7 @@
 {
   "extends": "./tsconfig.json",
   "compilerOptions": {
-    "noImplicitAny": false
+    "noImplicitAny": false,
+    "target": "es6"
   }
 }