From 690c627f813f74a76d628d2def7bd8159df4d875 Mon Sep 17 00:00:00 2001 From: Manuel <30572287+manuel-rw@users.noreply.github.com> Date: Sun, 24 Sep 2023 16:04:07 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Add=20login=20redirection?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ✨ Add login redirection * 🚑 Fix cross site scripting using server side regex validation * ✅ Add unit test --- .github/workflows/docker_dev.yml | 7 +- package.json | 1 + public/locales/en/authentication/login.json | 3 +- src/pages/auth/login.tsx | 28 ++++- src/pages/board/[slug].tsx | 68 +++++------ src/pages/manage/boards/index.tsx | 9 +- src/pages/manage/users/create.tsx | 8 +- src/tools/server/loginBuilder.ts | 34 ++++++ tests/pages/auth/login.spec.ts | 125 ++++++++++++++++++++ tests/pages/board/[slug].spec.ts | 9 +- vitest.config.ts | 2 +- yarn.lock | 46 ++++++- 12 files changed, 275 insertions(+), 65 deletions(-) create mode 100644 src/tools/server/loginBuilder.ts create mode 100644 tests/pages/auth/login.spec.ts diff --git a/.github/workflows/docker_dev.yml b/.github/workflows/docker_dev.yml index a61132e63..221fe189e 100644 --- a/.github/workflows/docker_dev.yml +++ b/.github/workflows/docker_dev.yml @@ -41,6 +41,7 @@ jobs: permissions: packages: write contents: read + pull-requests: write steps: - name: Setup @@ -74,7 +75,11 @@ jobs: - run: yarn turbo build - - run: yarn test:run + - run: yarn test:coverage + + - name: Report coverage + if: always() + uses: davelosert/vitest-coverage-report-action@v2 - name: Docker meta if: github.event_name != 'pull_request' diff --git a/package.json b/package.json index aeea86ea4..02ba07e21 100644 --- a/package.json +++ b/package.json @@ -120,6 +120,7 @@ "@typescript-eslint/eslint-plugin": "^6.0.0", "@typescript-eslint/parser": "^6.0.0", "@vitest/coverage-c8": "^0.33.0", + "@vitest/coverage-v8": "^0.34.5", "@vitest/ui": "^0.34.4", "eslint": "^8.0.1", "eslint-config-next": "^13.4.5", diff --git a/public/locales/en/authentication/login.json b/public/locales/en/authentication/login.json index 434127630..33fcdd9d7 100644 --- a/public/locales/en/authentication/login.json +++ b/public/locales/en/authentication/login.json @@ -13,7 +13,8 @@ }, "buttons": { "submit": "Sign in" - } + }, + "afterLoginRedirection": "After login, you'll be redirected to {{url}}" }, "alert": "Your credentials are incorrect or this account doesn't exist. Please try again." } \ No newline at end of file diff --git a/src/pages/auth/login.tsx b/src/pages/auth/login.tsx index 5ce5bbd24..cc1b2a81a 100644 --- a/src/pages/auth/login.tsx +++ b/src/pages/auth/login.tsx @@ -12,7 +12,7 @@ import { } from '@mantine/core'; import { useForm } from '@mantine/form'; import { IconAlertTriangle } from '@tabler/icons-react'; -import { GetServerSideProps } from 'next'; +import { GetServerSideProps, InferGetServerSidePropsType } from 'next'; import { signIn } from 'next-auth/react'; import { useTranslation } from 'next-i18next'; import Head from 'next/head'; @@ -20,14 +20,16 @@ import Image from 'next/image'; import { useRouter } from 'next/router'; import { useState } from 'react'; import { z } from 'zod'; +import { ThemeSchemeToggle } from '~/components/ThemeSchemeToggle/ThemeSchemeToggle'; import { FloatingBackground } from '~/components/layout/Background/FloatingBackground'; import { getServerAuthSession } from '~/server/auth'; import { getServerSideTranslations } from '~/tools/server/getServerSideTranslations'; import { useI18nZodResolver } from '~/utils/i18n-zod-resolver'; import { signInSchema } from '~/validations/user'; -import { ThemeSchemeToggle } from '~/components/ThemeSchemeToggle/ThemeSchemeToggle'; -export default function LoginPage() { +export default function LoginPage({ + redirectAfterLogin, +}: InferGetServerSidePropsType) { const { t } = useTranslation('authentication/login'); const { i18nZodResolver } = useI18nZodResolver(); const router = useRouter(); @@ -54,7 +56,7 @@ export default function LoginPage() { setIsError(true); return; } - router.push('/manage'); + router.push(redirectAfterLogin ?? '/manage'); }); }; @@ -68,7 +70,7 @@ export default function LoginPage() { - + @@ -120,6 +122,12 @@ export default function LoginPage() { + + {redirectAfterLogin && ( + + {t('form.afterLoginRedirection', { url: redirectAfterLogin })} + + )} @@ -129,9 +137,16 @@ export default function LoginPage() { ); } -export const getServerSideProps: GetServerSideProps = async ({ locale, req, res }) => { +const regexExp = /^\/{1}[A-z\/]*$/; + +export const getServerSideProps: GetServerSideProps = async ({ locale, req, res, query }) => { const session = await getServerAuthSession({ req, res }); + const zodResult = await z + .object({ redirectAfterLogin: z.string().regex(regexExp) }) + .safeParseAsync(query); + const redirectAfterLogin = zodResult.success ? zodResult.data.redirectAfterLogin : null; + if (session) { return { redirect: { @@ -144,6 +159,7 @@ export const getServerSideProps: GetServerSideProps = async ({ locale, req, res return { props: { ...(await getServerSideTranslations(['authentication/login'], locale, req, res)), + redirectAfterLogin, }, }; }; diff --git a/src/pages/board/[slug].tsx b/src/pages/board/[slug].tsx index f473a3b36..567abd141 100644 --- a/src/pages/board/[slug].tsx +++ b/src/pages/board/[slug].tsx @@ -5,12 +5,13 @@ import { Dashboard } from '~/components/Dashboard/Dashboard'; import { BoardLayout } from '~/components/layout/Templates/BoardLayout'; import { useInitConfig } from '~/config/init'; import { env } from '~/env'; +import { getServerAuthSession } from '~/server/auth'; import { configExists } from '~/tools/config/configExists'; import { getFrontendConfig } from '~/tools/config/getFrontendConfig'; import { getServerSideTranslations } from '~/tools/server/getServerSideTranslations'; +import { checkForSessionOrAskForLogin } from '~/tools/server/loginBuilder'; import { boardNamespaces } from '~/tools/server/translation-namespaces'; import { ConfigType } from '~/types/config'; -import { getServerAuthSession } from '~/server/auth'; export default function BoardPage({ config: initialConfig, @@ -35,13 +36,8 @@ const routeParamsSchema = z.object({ slug: z.string(), }); -export const getServerSideProps: GetServerSideProps = async ({ - params, - locale, - req, - res, -}) => { - const routeParams = routeParamsSchema.safeParse(params); +export const getServerSideProps: GetServerSideProps = async (ctx) => { + const routeParams = routeParamsSchema.safeParse(ctx.params); if (!routeParams.success) { return { notFound: true, @@ -56,38 +52,32 @@ export const getServerSideProps: GetServerSideProps = a } const config = await getFrontendConfig(routeParams.data.slug); - const translations = await getServerSideTranslations(boardNamespaces, locale, req, res); + const translations = await getServerSideTranslations( + boardNamespaces, + ctx.locale, + ctx.req, + ctx.res + ); - const getSuccessResponse = () => { - return { - props: { - config, - primaryColor: config.settings.customization.colors.primary, - secondaryColor: config.settings.customization.colors.secondary, - primaryShade: config.settings.customization.colors.shade, - dockerEnabled: !!env.DOCKER_HOST && !!env.DOCKER_PORT, - ...translations, - }, - }; + const session = await getServerAuthSession({ req: ctx.req, res: ctx.res }); + + const result = checkForSessionOrAskForLogin( + ctx, + session, + () => config.settings.access.allowGuests || !session?.user + ); + if (result) { + return result; } - - if (!config.settings.access.allowGuests) { - const session = await getServerAuthSession({ req, res }); - - if (session?.user) { - return getSuccessResponse(); - } - - return { - notFound: true, - props: { - primaryColor: config.settings.customization.colors.primary, - secondaryColor: config.settings.customization.colors.secondary, - primaryShade: config.settings.customization.colors.shade, - } - }; - } - - return getSuccessResponse(); + return { + props: { + config, + primaryColor: config.settings.customization.colors.primary, + secondaryColor: config.settings.customization.colors.secondary, + primaryShade: config.settings.customization.colors.shade, + dockerEnabled: !!env.DOCKER_HOST && !!env.DOCKER_PORT, + ...translations, + }, + }; }; diff --git a/src/pages/manage/boards/index.tsx b/src/pages/manage/boards/index.tsx index 84dcb9b73..d79e73947 100644 --- a/src/pages/manage/boards/index.tsx +++ b/src/pages/manage/boards/index.tsx @@ -12,7 +12,6 @@ import { Title, } from '@mantine/core'; import { useListState } from '@mantine/hooks'; -import { modals } from '@mantine/modals'; import { IconBox, IconCategory, @@ -35,6 +34,7 @@ import { ManageLayout } from '~/components/layout/Templates/ManageLayout'; import { getServerAuthSession } from '~/server/auth'; import { sleep } from '~/tools/client/time'; import { getServerSideTranslations } from '~/tools/server/getServerSideTranslations'; +import { checkForSessionOrAskForLogin } from '~/tools/server/loginBuilder'; import { manageNamespaces } from '~/tools/server/translation-namespaces'; import { api } from '~/utils/api'; @@ -204,10 +204,9 @@ const BoardsPage = () => { export const getServerSideProps: GetServerSideProps = async (ctx) => { const session = await getServerAuthSession(ctx); - if (!session?.user) { - return { - notFound: true, - }; + const result = checkForSessionOrAskForLogin(ctx, session, () => true); + if (result) { + return result; } const translations = await getServerSideTranslations( diff --git a/src/pages/manage/users/create.tsx b/src/pages/manage/users/create.tsx index 421290cc8..f1c894c7f 100644 --- a/src/pages/manage/users/create.tsx +++ b/src/pages/manage/users/create.tsx @@ -19,6 +19,7 @@ import { import { ManageLayout } from '~/components/layout/Templates/ManageLayout'; import { getServerAuthSession } from '~/server/auth'; import { getServerSideTranslations } from '~/tools/server/getServerSideTranslations'; +import { checkForSessionOrAskForLogin } from '~/tools/server/loginBuilder'; import { manageNamespaces } from '~/tools/server/translation-namespaces'; import { useI18nZodResolver } from '~/utils/i18n-zod-resolver'; @@ -132,10 +133,9 @@ export type CreateAccountSchema = z.infer; export const getServerSideProps: GetServerSideProps = async (ctx) => { const session = await getServerAuthSession(ctx); - if (!session?.user.isAdmin) { - return { - notFound: true, - }; + const result = checkForSessionOrAskForLogin(ctx, session, () => session?.user.isAdmin == true); + if (result) { + return result; } const translations = await getServerSideTranslations( diff --git a/src/tools/server/loginBuilder.ts b/src/tools/server/loginBuilder.ts new file mode 100644 index 000000000..5120a6dc9 --- /dev/null +++ b/src/tools/server/loginBuilder.ts @@ -0,0 +1,34 @@ +import { + GetServerSideProps, + GetServerSidePropsContext, + GetServerSidePropsResult, + PreviewData, +} from 'next'; +import { Session } from 'next-auth'; +import { ParsedUrlQuery } from 'querystring'; + +export const checkForSessionOrAskForLogin = ( + context: GetServerSidePropsContext, + session: Session | null, + accessCallback: () => boolean, +): GetServerSidePropsResult | undefined => { + if (!session?.user) { + console.log('detected logged out user!'); + return { + props: {}, + redirect: { + destination: `/auth/login?redirectAfterLogin=${context.resolvedUrl}`, + permanent: false, + }, + }; + } + + if (!accessCallback()) { + return { + props: {}, + notFound: true + } + } + + return undefined; +}; diff --git a/tests/pages/auth/login.spec.ts b/tests/pages/auth/login.spec.ts new file mode 100644 index 000000000..8fb9c2be9 --- /dev/null +++ b/tests/pages/auth/login.spec.ts @@ -0,0 +1,125 @@ +import { IncomingMessage } from 'http'; +import { ServerResponse } from 'http'; +import { GetServerSidePropsContext } from 'next'; +import { SSRConfig } from 'next-i18next'; +import { ParsedUrlQuery } from 'querystring'; +import { describe, expect, it, vitest } from 'vitest'; +import { getServerSideProps } from '~/pages/auth/login'; +import * as serverAuthModule from '~/server/auth'; + +import * as getServerSideTranslationsModule from '../../../src/tools/server/getServerSideTranslations'; + +vitest.mock('./../../server/auth.ts', () => ({ + getServerAuthSession: () => null, +})); + +vitest.mock('./../../tools/server/getServerSideTranslations.ts', () => ({ + getServerSideTranslations: () => null, +})); + +describe('login page', () => { + it('getServerSideProps should return null redirectAfterLogin when no query value', async () => { + // arrange + vitest.spyOn(serverAuthModule, 'getServerAuthSession').mockReturnValue(Promise.resolve(null)); + vitest + .spyOn(getServerSideTranslationsModule, 'getServerSideTranslations') + .mockReturnValue(Promise.resolve({ _i18Next: 'hello' } as unknown as SSRConfig)); + + // act + const response = await getServerSideProps({ + query: {}, + locale: 'de-DE', + req: {}, + res: {}, + } as GetServerSidePropsContext); + + // assert + expect(response).toStrictEqual({ + props: { + redirectAfterLogin: null, + _i18Next: 'hello', + }, + }); + + expect(serverAuthModule.getServerAuthSession).toHaveBeenCalledOnce(); + expect(getServerSideTranslationsModule.getServerSideTranslations).toHaveBeenCalledOnce(); + expect(getServerSideTranslationsModule.getServerSideTranslations).toHaveBeenCalledWith( + ['authentication/login'], + 'de-DE', + {}, + {} + ); + }); + + it('getServerSideProps should return url when redirectAfterLogin is local and valid', async () => { + // arrange + vitest.spyOn(serverAuthModule, 'getServerAuthSession').mockReturnValue(Promise.resolve(null)); + vitest + .spyOn(getServerSideTranslationsModule, 'getServerSideTranslations') + .mockReturnValue(Promise.resolve({ _i18Next: 'hello' } as unknown as SSRConfig)); + + // act + const response = await getServerSideProps({ + query: { + redirectAfterLogin: '/manage/users/create', + }, + locale: 'de-DE', + req: {} as IncomingMessage & { cookies: Partial<{ [key: string]: string }> }, + res: {} as ServerResponse, + resolvedUrl: '/auth/login', + } as GetServerSidePropsContext); + + // assert + expect(response).toStrictEqual({ + props: { + redirectAfterLogin: '/manage/users/create', + _i18Next: 'hello', + }, + }); + + expect(serverAuthModule.getServerAuthSession).toHaveBeenCalledOnce(); + expect(getServerSideTranslationsModule.getServerSideTranslations).toHaveBeenCalledOnce(); + expect(getServerSideTranslationsModule.getServerSideTranslations).toHaveBeenCalledWith( + ['authentication/login'], + 'de-DE', + {}, + {} + ); + }); + + it('getServerSideProps should return null when url does not match regex', async () => { + // arrange + vitest.spyOn(serverAuthModule, 'getServerAuthSession').mockReturnValue(Promise.resolve(null)); + vitest + .spyOn(getServerSideTranslationsModule, 'getServerSideTranslations') + .mockReturnValue(Promise.resolve({ _i18Next: 'hello' } as unknown as SSRConfig)); + + // act + const response = await getServerSideProps({ + query: { + redirectAfterLogin: "data:text/html,", + }, + locale: 'de-DE', + req: {} as IncomingMessage & { cookies: Partial<{ [key: string]: string }> }, + res: {} as ServerResponse, + resolvedUrl: '/auth/login', + } as GetServerSidePropsContext); + + // assert + expect(response).toStrictEqual({ + props: { + redirectAfterLogin: null, + _i18Next: 'hello', + }, + }); + + expect(serverAuthModule.getServerAuthSession).toHaveBeenCalledOnce(); + expect(getServerSideTranslationsModule.getServerSideTranslations).toHaveBeenCalledOnce(); + expect(getServerSideTranslationsModule.getServerSideTranslations).toHaveBeenCalledWith( + ['authentication/login'], + 'de-DE', + {}, + {} + ); + }); +}); diff --git a/tests/pages/board/[slug].spec.ts b/tests/pages/board/[slug].spec.ts index ad8549026..16b7d8a6a 100644 --- a/tests/pages/board/[slug].spec.ts +++ b/tests/pages/board/[slug].spec.ts @@ -158,12 +158,11 @@ describe('[slug] page', () => { // assert expect(response).toEqual({ - notFound: true, - props: { - primaryColor: 'red', - secondaryColor: 'blue', - primaryShade: 'green', + redirect: { + destination: "/auth/login?redirectAfterLogin=/board/my-authentication-board", + permanent: false }, + props: {}, }); expect(serverAuthModule.getServerAuthSession).toHaveBeenCalledOnce(); expect(configExistsModule.configExists).toHaveBeenCalledOnce(); diff --git a/vitest.config.ts b/vitest.config.ts index b98664c84..5c0a50629 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -9,7 +9,7 @@ export default defineConfig({ environment: 'happy-dom', coverage: { provider: 'v8', - reporter: ['html'], + reporter: ['html', 'json-summary', 'json'], all: true, exclude: ['.next/', '.yarn/', 'data/'], }, diff --git a/yarn.lock b/yarn.lock index 6bd1d686a..414b33529 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3231,6 +3231,27 @@ __metadata: languageName: node linkType: hard +"@vitest/coverage-v8@npm:^0.34.5": + version: 0.34.5 + resolution: "@vitest/coverage-v8@npm:0.34.5" + dependencies: + "@ampproject/remapping": ^2.2.1 + "@bcoe/v8-coverage": ^0.2.3 + istanbul-lib-coverage: ^3.2.0 + istanbul-lib-report: ^3.0.1 + istanbul-lib-source-maps: ^4.0.1 + istanbul-reports: ^3.1.5 + magic-string: ^0.30.1 + picocolors: ^1.0.0 + std-env: ^3.3.3 + test-exclude: ^6.0.0 + v8-to-istanbul: ^9.1.0 + peerDependencies: + vitest: ">=0.32.0 <1" + checksum: 6ffbcbd0d992b535c7fbc5d8ad82f8939e11786e28a899d1b2ef60f51c192063738d60b0037f7c5fec5545130f671b408bc5f20432abb280bf9788214467c475 + languageName: node + linkType: hard + "@vitest/expect@npm:0.33.0": version: 0.33.0 resolution: "@vitest/expect@npm:0.33.0" @@ -6948,6 +6969,7 @@ __metadata: "@typescript-eslint/parser": ^6.0.0 "@vitejs/plugin-react": ^4.0.0 "@vitest/coverage-c8": ^0.33.0 + "@vitest/coverage-v8": ^0.34.5 "@vitest/ui": ^0.34.4 axios: ^1.0.0 bcryptjs: ^2.4.3 @@ -7729,7 +7751,7 @@ __metadata: languageName: node linkType: hard -"istanbul-lib-report@npm:^3.0.0": +"istanbul-lib-report@npm:^3.0.0, istanbul-lib-report@npm:^3.0.1": version: 3.0.1 resolution: "istanbul-lib-report@npm:3.0.1" dependencies: @@ -7740,7 +7762,18 @@ __metadata: languageName: node linkType: hard -"istanbul-reports@npm:^3.1.4": +"istanbul-lib-source-maps@npm:^4.0.1": + version: 4.0.1 + resolution: "istanbul-lib-source-maps@npm:4.0.1" + dependencies: + debug: ^4.1.1 + istanbul-lib-coverage: ^3.0.0 + source-map: ^0.6.1 + checksum: 21ad3df45db4b81852b662b8d4161f6446cd250c1ddc70ef96a585e2e85c26ed7cd9c2a396a71533cfb981d1a645508bc9618cae431e55d01a0628e7dec62ef2 + languageName: node + linkType: hard + +"istanbul-reports@npm:^3.1.4, istanbul-reports@npm:^3.1.5": version: 3.1.6 resolution: "istanbul-reports@npm:3.1.6" dependencies: @@ -10904,6 +10937,13 @@ __metadata: languageName: node linkType: hard +"source-map@npm:^0.6.1": + version: 0.6.1 + resolution: "source-map@npm:0.6.1" + checksum: 59ce8640cf3f3124f64ac289012c2b8bd377c238e316fb323ea22fbfe83da07d81e000071d7242cad7a23cd91c7de98e4df8830ec3f133cb6133a5f6e9f67bc2 + languageName: node + linkType: hard + "split-ca@npm:^1.0.1": version: 1.0.1 resolution: "split-ca@npm:1.0.1" @@ -12134,7 +12174,7 @@ __metadata: languageName: node linkType: hard -"v8-to-istanbul@npm:^9.0.0": +"v8-to-istanbul@npm:^9.0.0, v8-to-istanbul@npm:^9.1.0": version: 9.1.0 resolution: "v8-to-istanbul@npm:9.1.0" dependencies: