From fad84441594542edcebf0be9f0baea4fa53b0cfd Mon Sep 17 00:00:00 2001 From: adamjmcgrath Date: Fri, 9 Apr 2021 12:11:41 +0100 Subject: [PATCH 1/2] returnTo should be encoded as it contains url unsafe chars --- src/frontend/with-page-auth-required.tsx | 2 +- src/helpers/with-page-auth-required.ts | 7 ++++++- tests/frontend/with-page-auth-required.test.tsx | 13 +++++++++++++ tests/helpers/with-page-auth-required.test.ts | 11 +++++++++++ 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/frontend/with-page-auth-required.tsx b/src/frontend/with-page-auth-required.tsx index 9ac110377..c3d768844 100644 --- 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 index 66e347c59..617ccccd3 100644 --- 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 index 8aace4ff5..ba23d7297 100644 --- a/tests/frontend/with-page-auth-required.test.tsx +++ b/tests/frontend/with-page-auth-required.test.tsx @@ -122,4 +122,17 @@ describe('with-page-auth-required csr', () => { 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(, { 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 index 5765e8e62..3726eae06 100644 --- 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'; @@ -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'); + }); }); From 8382c3d972eb51bbc3cae8896c78952ac7e47f35 Mon Sep 17 00:00:00 2001 From: adamjmcgrath Date: Fri, 9 Apr 2021 12:18:17 +0100 Subject: [PATCH 2/2] fix tests --- tests/frontend/with-page-auth-required.test.tsx | 10 ++++++++-- tests/helpers/with-page-auth-required.test.ts | 6 +++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/frontend/with-page-auth-required.test.tsx b/tests/frontend/with-page-auth-required.test.tsx index ba23d7297..2c6d396e0 100644 --- 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(, { 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,7 +121,9 @@ describe('with-page-auth-required csr', () => { render(, { 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; diff --git a/tests/helpers/with-page-auth-required.test.ts b/tests/helpers/with-page-auth-required.test.ts index 3726eae06..1e6c9900a 100644 --- a/tests/helpers/with-page-auth-required.test.ts +++ b/tests/helpers/with-page-auth-required.test.ts @@ -12,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 () => { @@ -32,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 () => { @@ -57,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; });