Merge pull request #365 from auth0/encode-return-to

[SDK-2452] returnTo should be encoded as it contains url unsafe chars
auth0 · Apr 9, 2021 · 85018c7 · 85018c7
2 parents 2b5aeeb + 8382c3d
commit 85018c7
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 7 deletions.
diff --git a/src/frontend/with-page-auth-required.tsx b/src/frontend/with-page-auth-required.tsx
@@ -84,7 +84,7 @@ const withPageAuthRequired: WithPageAuthRequired = (Component, options = {}) =>
 
     useEffect(() => {
       if ((user && !error) || isLoading) return;
-      window.location.assign(`${loginUrl}?returnTo=${returnTo}`);
+      window.location.assign(`${loginUrl}?returnTo=${encodeURIComponent(returnTo)}`);
     }, [user, error, isLoading]);
 
     if (error) return onError(error);

diff --git a/src/helpers/with-page-auth-required.ts b/src/helpers/with-page-auth-required.ts
@@ -106,7 +106,12 @@ export default function withPageAuthRequiredFactory(loginUrl: string, getSession
         // 10 - redirect
         // 9.5.4 - unstable_redirect
         // 9.4 - res.setHeaders
-        return { redirect: { destination: `${loginUrl}?returnTo=${returnTo || ctx.resolvedUrl}`, permanent: false } };
+        return {
+          redirect: {
+            destination: `${loginUrl}?returnTo=${encodeURIComponent(returnTo || ctx.resolvedUrl)}`,
+            permanent: false
+          }
+        };
       }
       let ret: any = { props: {} };
       if (getServerSideProps) {

diff --git a/tests/frontend/with-page-auth-required.test.tsx b/tests/frontend/with-page-auth-required.test.tsx
@@ -92,7 +92,11 @@ describe('with-page-auth-required csr', () => {
     const ProtectedPage = withPageAuthRequired(MyPage, { returnTo: '/foo' });
 
     render(<ProtectedPage />, { wrapper: withUserProvider() });
-    await waitFor(() => expect(window.location.assign).toHaveBeenCalledWith(expect.stringContaining('?returnTo=/foo')));
+    await waitFor(() =>
+      expect(window.location.assign).toHaveBeenCalledWith(
+        expect.stringContaining(`?returnTo=${encodeURIComponent('/foo')}`)
+      )
+    );
   });
 
   it('should use a custom login url', async () => {
@@ -117,9 +121,24 @@ describe('with-page-auth-required csr', () => {
 
     render(<ProtectedPage />, { wrapper: withUserProvider() });
     await waitFor(() =>
-      expect(window.location.assign).toHaveBeenCalledWith(expect.stringContaining('?returnTo=/foo/bar'))
+      expect(window.location.assign).toHaveBeenCalledWith(
+        expect.stringContaining(`?returnTo=${encodeURIComponent('/foo/bar')}`)
+      )
     );
     routerMock.basePath = basePath;
     routerMock.asPath = asPath;
   });
+
+  it('should preserve multiple query params in the returnTo URL', async () => {
+    (global as any).fetch = fetchUserUnsuccessfulMock;
+    const MyPage = (): JSX.Element => <>Private</>;
+    const ProtectedPage = withPageAuthRequired(MyPage, { returnTo: '/foo?bar=baz&qux=quux' });
+
+    render(<ProtectedPage />, { wrapper: withUserProvider() });
+    await waitFor(() => {
+      expect(window.location.assign).toHaveBeenCalled();
+    });
+    const url = new URL((window.location.assign as jest.Mock).mock.calls[0][0], 'https://example.com');
+    expect(url.searchParams.get('returnTo')).toEqual('/foo?bar=baz&qux=quux');
+  });
 });
diff --git a/tests/helpers/with-page-auth-required.test.ts b/tests/helpers/with-page-auth-required.test.ts
@@ -1,3 +1,4 @@
+import { URL } from 'url';
 import { login, setup, teardown } from '../fixtures/setup';
 import { withoutApi } from '../fixtures/default-settings';
 import { get } from '../auth0-session/fixtures/helpers';
@@ -11,7 +12,7 @@ describe('with-page-auth-required ssr', () => {
       res: { statusCode, headers }
     } = await get(baseUrl, '/protected', { fullResponse: true });
     expect(statusCode).toBe(307);
-    expect(headers.location).toBe('/api/auth/login?returnTo=/protected');
+    expect(decodeURIComponent(headers.location)).toBe('/api/auth/login?returnTo=/protected');
   });
 
   test('allow access to a page with a valid session', async () => {
@@ -31,7 +32,7 @@ describe('with-page-auth-required ssr', () => {
       res: { statusCode, headers }
     } = await get(baseUrl, '/protected', { fullResponse: true });
     expect(statusCode).toBe(307);
-    expect(headers.location).toBe('/api/auth/login?returnTo=/foo');
+    expect(decodeURIComponent(headers.location)).toBe('/api/auth/login?returnTo=/foo');
   });
 
   test('accept custom server-side props', async () => {
@@ -56,7 +57,7 @@ describe('with-page-auth-required ssr', () => {
       res: { statusCode, headers }
     } = await get(baseUrl, '/protected', { fullResponse: true });
     expect(statusCode).toBe(307);
-    expect(headers.location).toBe('/api/foo?returnTo=/protected');
+    expect(decodeURIComponent(headers.location)).toBe('/api/foo?returnTo=/protected');
     delete process.env.NEXT_PUBLIC_AUTH0_LOGIN;
   });
 
@@ -68,4 +69,14 @@ describe('with-page-auth-required ssr', () => {
     } = await get(baseUrl, '/csr-protected', { cookieJar, fullResponse: true });
     expect(statusCode).toBe(200);
   });
+
+  test('should preserve multiple query params in the returnTo URL', async () => {
+    const baseUrl = await setup(withoutApi, { withPageAuthRequiredOptions: { returnTo: '/foo?bar=baz&qux=quux' } });
+    const {
+      res: { statusCode, headers }
+    } = await get(baseUrl, '/protected', { fullResponse: true });
+    expect(statusCode).toBe(307);
+    const url = new URL(headers.location, baseUrl);
+    expect(url.searchParams.get('returnTo')).toEqual('/foo?bar=baz&qux=quux');
+  });
 });