Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: replace redirect url pattern with route key pattern #398

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
30 changes: 11 additions & 19 deletions src/components/AuthWrappers/EnforceLoginStatePageWrapper.tsx
Original file line number Diff line number Diff line change
@@ -1,45 +1,37 @@
import { useMemo, type PropsWithChildren } from 'react'
import { useEffect, type PropsWithChildren } from 'react'
import { useRouter } from 'next/router'

import { appendWithRedirect } from '~/utils/url'
import { useLoginState } from '~/features/auth'
import { SIGN_IN } from '~/lib/routes'
import { callbackUrlSchema } from '~/schemas/url'
import { HOME, SIGN_IN, type CallbackRoute } from '~/lib/routes'
import { FullscreenSpinner } from '../FullscreenSpinner'

interface EnforceLoginStatePageWrapperProps {
/**
* Route to redirect to when user is not authenticated. Defaults to
* `SIGN_IN` route if not provided.
* Route to redirect to after user is authenticated. Defaults to
* `HOME` route if not provided.
*/
redirectTo?: string
redirectTo?: CallbackRoute
}

const Redirect = ({
redirectTo = SIGN_IN,
}: EnforceLoginStatePageWrapperProps) => {
const Redirect = ({ redirectTo = HOME }: EnforceLoginStatePageWrapperProps) => {
const router = useRouter()
const redirectUrl = useMemo(() => {
if (typeof window === 'undefined') return '/'
const { pathname, search, hash } = window.location
return `${pathname}${search}${hash}`
}, [])

void router.replace(
callbackUrlSchema.parse(appendWithRedirect(redirectTo, redirectUrl)),
)
useEffect(() => {
void router.replace(appendWithRedirect(SIGN_IN, redirectTo))
}, [router, redirectTo])

return <FullscreenSpinner />
}

/**
* Page wrapper that renders children only if the login state localStorage flag has been set.
* Otherwise, will redirect to the route passed into the `redirectTo` prop.
* Otherwise, will redirect to SIGN_IN, which will redirect to route passed into the `redirectTo` prop.
*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this will redirect to HOME instead? is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component redirects to /sign-in?callbackUrl=<redirectTo> when not signed in, redirectTo defaults to HOME if not provided.

* @note 🚨 There is no authentication being performed by this component. This component is merely a wrapper that checks for the presence of the login flag in localStorage. This means that a user could add the flag and bypass the check. Any page children that require authentication should also perform authentication checks in that page itself!
*/
export const EnforceLoginStatePageWrapper = ({
redirectTo = SIGN_IN,
redirectTo = HOME,
children,
}: PropsWithChildren<EnforceLoginStatePageWrapperProps>): React.ReactElement => {
const { hasLoginStateFlag } = useLoginState()
Expand Down
27 changes: 18 additions & 9 deletions src/components/AuthWrappers/PublicPageWrapper.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { type PropsWithChildren } from 'react'
import { useEffect, useState, type PropsWithChildren } from 'react'
import { useRouter } from 'next/router'

import { CALLBACK_URL_KEY } from '~/constants/params'
import { getRedirectUrl } from '~/utils/url'
import { useLoginState } from '~/features/auth'
import { type CallbackRoute } from '~/lib/routes'
import { callbackUrlSchema } from '~/schemas/url'
import { FullscreenSpinner } from '../FullscreenSpinner'

type PublicPageWrapperProps =
| { strict: true; redirectUrl?: string }
| { strict: true; redirectUrl?: CallbackRoute }
| { strict: false }

/**
Expand All @@ -23,14 +24,22 @@ export const PublicPageWrapper = ({
const router = useRouter()
const { hasLoginStateFlag } = useLoginState()

if (hasLoginStateFlag && rest.strict) {
if (rest.redirectUrl) {
void router.replace(callbackUrlSchema.parse(rest.redirectUrl))
const [isRedirecting, setIsRedirecting] = useState(true)

useEffect(() => {
if (hasLoginStateFlag && rest.strict) {
if (rest.redirectUrl) {
// must validate redirectUrl param
void router.replace(callbackUrlSchema.parse(rest.redirectUrl))
} else {
void router.replace(getRedirectUrl(router.query))
}
} else {
void router.replace(
callbackUrlSchema.parse(router.query[CALLBACK_URL_KEY]),
)
setIsRedirecting(false)
}
}, [hasLoginStateFlag, rest, router])

if (isRedirecting) {
return <FullscreenSpinner />
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,11 @@ import { Controller } from 'react-hook-form'
import { useInterval } from 'usehooks-ts'

import { trpc } from '~/utils/trpc'
import { CALLBACK_URL_KEY } from '~/constants/params'
import { getRedirectUrl } from '~/utils/url'
import { useLoginState } from '~/features/auth'
import { OTP_LENGTH } from '~/lib/auth'
import { useZodForm } from '~/lib/form'
import { emailVerifyOtpSchema } from '~/schemas/auth/email/sign-in'
import { callbackUrlSchema } from '~/schemas/url'
import { useSignInContext } from '../SignInContext'
import { ResendOtpButton } from './ResendOtpButton'

Expand Down Expand Up @@ -61,7 +60,7 @@ export const VerificationInput = (): JSX.Element | null => {
await utils.me.get.invalidate()
// accessing router.query values returns decoded URI params automatically,
// so there's no need to call decodeURIComponent manually when accessing the callback url.
await router.push(callbackUrlSchema.parse(router.query[CALLBACK_URL_KEY]))
await router.push(getRedirectUrl(router.query))
},
onError: (error) => {
switch (error.message) {
Expand Down
39 changes: 20 additions & 19 deletions src/features/sign-in/components/SgidCallback.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ import { useEffect } from 'react'
import { useRouter } from 'next/router'

import { trpc } from '~/utils/trpc'
import { appendWithRedirect } from '~/utils/url'
import { FullscreenSpinner } from '~/components/FullscreenSpinner'
import { useLoginState } from '~/features/auth'
import { callbackUrlSchema } from '~/schemas/url'
import { SIGN_IN, SIGN_IN_SELECT_PROFILE } from '~/lib/routes'

/**
* This component is responsible for handling the callback from the SGID login.
Expand All @@ -19,27 +20,27 @@ export const SgidCallback = (): JSX.Element => {
query: { code, state },
} = router

const [{ redirectUrl, selectProfileStep }] =
trpc.auth.sgid.callback.useSuspenseQuery(
{ code: String(code), state: String(state) },
{ staleTime: Infinity },
)
const [response] = trpc.auth.sgid.callback.useSuspenseQuery(
{ code: String(code), state: String(state) },
{ staleTime: Infinity },
)

useEffect(() => {
if (!selectProfileStep) {
setHasLoginStateFlag()
// eslint-disable-next-line @typescript-eslint/no-floating-promises
utils.me.get.invalidate()
if (!response.success) {
void router.replace(
`${SIGN_IN}?${new URLSearchParams({ error: response.reason })}`,
)
} else {
const { selectProfileStep, landingUrl } = response.data
if (!selectProfileStep) {
setHasLoginStateFlag()
void utils.me.get.invalidate()
}
void router.replace(
appendWithRedirect(SIGN_IN_SELECT_PROFILE, landingUrl),
)
}
// eslint-disable-next-line @typescript-eslint/no-floating-promises
router.replace(callbackUrlSchema.parse(redirectUrl))
}, [
redirectUrl,
router,
selectProfileStep,
setHasLoginStateFlag,
utils.me.get,
])
}, [response, router, setHasLoginStateFlag, utils.me.get])

return <FullscreenSpinner />
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,21 @@ import { type FallbackProps } from 'react-error-boundary'
import { z } from 'zod'

import { safeSchemaJsonParse } from '~/utils/zod'
import { HOME } from '~/lib/routes'
import { HOME, type CallbackRoute } from '~/lib/routes'
import { callbackUrlSchema } from '~/schemas/url'
import { SgidErrorModal } from './SgidErrorModal'

export const SgidErrorFallback: ComponentType<FallbackProps> = ({ error }) => {
const router = useRouter()
const redirectUrl = useMemo(() => {
const redirectUrl: CallbackRoute = useMemo(() => {
const parsed = safeSchemaJsonParse(
z.object({
landingUrl: callbackUrlSchema,
}),
String(router.query.state),
)
if (parsed.success) {
return parsed.data.landingUrl.href
return parsed.data.landingUrl
}
return HOME
}, [router.query.state])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ import {

import { appendWithRedirect } from '~/utils/url'
import { SGID } from '~/lib/errors/auth.sgid'
import { SIGN_IN } from '~/lib/routes'
import { SIGN_IN, type CallbackRoute } from '~/lib/routes'

interface SgidErrorModalProps {
message: string
redirectUrl: string
redirectUrl: CallbackRoute
}

export const SgidErrorModal = ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { Button } from '@opengovsg/design-system-react'
import { trpc } from '~/utils/trpc'
import { getRedirectUrl } from '~/utils/url'
import { SingpassFullLogo } from '~/components/Svg/SingpassFullLogo'
import { callbackUrlSchema } from '~/schemas/url'

export const SgidLoginButton = (): JSX.Element | null => {
const router = useRouter()
Expand All @@ -15,11 +14,9 @@ export const SgidLoginButton = (): JSX.Element | null => {
},
})

const landingUrl = callbackUrlSchema.parse(getRedirectUrl(router.query)).href

const handleSgidLogin = () => {
return sgidLoginMutation.mutate({
landingUrl,
landingUrl: getRedirectUrl(router.query),
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@ import { useRouter } from 'next/router'
import { Divider, Stack } from '@chakra-ui/react'

import { trpc } from '~/utils/trpc'
import { CALLBACK_URL_KEY } from '~/constants/params'
import { getRedirectUrl } from '~/utils/url'
import { useLoginState } from '~/features/auth'
import { withSuspense } from '~/hocs/withSuspense'
import { callbackUrlSchema } from '~/schemas/url'
import { SgidProfileItem } from './SgidProfileItem'
import { SgidProfileListSkeleton } from './SgidProfileListSkeleton'

Expand All @@ -22,9 +21,7 @@ const SuspendableSgidProfileList = (): JSX.Element => {
onSuccess: async () => {
setHasLoginStateFlag()
await utils.me.get.invalidate()
await router.replace(
callbackUrlSchema.parse(router.query[CALLBACK_URL_KEY]),
)
await router.replace(getRedirectUrl(router.query))
},
})

Expand Down
12 changes: 11 additions & 1 deletion src/lib/routes.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
export const SIGN_IN = '/sign-in'
export const SIGN_IN_SELECT_PROFILE_SUBROUTE = '/select-profile'
export const SIGN_IN_SELECT_PROFILE = '/sign-in/select-profile'

export const HOME = '/home'
export const PROFILE = '/profile'
export const SETTINGS_PROFILE = '/settings/profile'

export const ALLOWED_CALLBACK_ROUTES = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this would work for dynamic routes (like /user/[userId]). If not, might break many autoredirects/ cause confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesnt, it will definitely break dynamic routes.

The issue is because router.push does too much magic (e.g. you can do router.push('/home/[x]?x=../signout') to /signout)

It became quite challenging to validate that router.push does not cause unintended navigation with user supplied input (?callbackUrl param) (especially across next versions), so the alternative is to use a hardcoded whitelist approach like the PR shows.

appUrlSchema can still be used to validate urls if the dev wishes to use dynamic urls, just that the additional friction is supposed to discourage that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to hear about alternatives though, not sure what's the best approach tbh

SIGN_IN,
SIGN_IN_SELECT_PROFILE,
HOME,
PROFILE,
SETTINGS_PROFILE,
] as const

export type CallbackRoute = (typeof ALLOWED_CALLBACK_ROUTES)[number]
12 changes: 10 additions & 2 deletions src/schemas/url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { createUrlSchema } from '@opengovsg/starter-kitty-validators/url'
import { z } from 'zod'

import { getBaseUrl } from '~/utils/getBaseUrl'
import { HOME } from '~/lib/routes'
import { ALLOWED_CALLBACK_ROUTES, HOME } from '~/lib/routes'

const baseUrl = new URL(getBaseUrl())

Expand All @@ -14,7 +14,10 @@ const urlSchema = createUrlSchema({
},
})

export const callbackUrlSchema = z
/**
* Zod schema for validating internal (same-app) URLs
*/
export const appUrlSchema = z
.string()
.optional()
.default(HOME)
Expand All @@ -30,3 +33,8 @@ export const callbackUrlSchema = z
}
})
.catch(new URL(HOME, baseUrl.origin))

/**
* Zod schema for validating callbackUrls
*/
export const callbackUrlSchema = z.enum(ALLOWED_CALLBACK_ROUTES).catch(HOME)
34 changes: 21 additions & 13 deletions src/server/modules/auth/sgid/sgid.router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import set from 'lodash/set'
import { z } from 'zod'

import { trpcAssert } from '~/utils/trpcAssert'
import { appendWithRedirect } from '~/utils/url'
import { normaliseEmail, safeSchemaJsonParse } from '~/utils/zod'
import {
createResponseSchema,
normaliseEmail,
safeSchemaJsonParse,
} from '~/utils/zod'
import { SGID } from '~/lib/errors/auth.sgid'
import { SIGN_IN, SIGN_IN_SELECT_PROFILE_SUBROUTE } from '~/lib/routes'
import { APP_SGID_SCOPE, sgid } from '~/lib/sgid'
import { callbackUrlSchema } from '~/schemas/url'
import { publicProcedure, router } from '~/server/trpc'
Expand Down Expand Up @@ -74,6 +76,14 @@ export const sgidRouter = router({
code: z.string(),
}),
)
.output(
createResponseSchema(
z.object({
selectProfileStep: z.boolean(),
landingUrl: callbackUrlSchema,
}),
),
)
.query(async ({ ctx, input: { state, code } }) => {
if (!sgid) {
ctx.logger.error('SGID is not enabled')
Expand Down Expand Up @@ -107,15 +117,13 @@ export const sgidRouter = router({

try {
sgidUserInfo = await getUserInfo({ code, codeVerifier, nonce })
} catch (error) {
} catch {
ctx.logger.warn({ state }, 'Unable to fetch user info from sgID')
// Redirect back to sign in page with error.
// TODO: Change this to throw an error instead, and then handle it in the client.
return {
redirectUrl: `/sign-in?error=${
(error as Error).message ||
'Something went wrong whilst fetching SGID user info'
}`,
success: false,
reason: 'Something went wrong whilst fetching SGID user info',
}
}

Expand Down Expand Up @@ -151,11 +159,11 @@ export const sgidRouter = router({
set(ctx.session, 'sgid.profiles', sgidProfileToStore.data)
await ctx.session.save()
return {
selectProfileStep: true,
redirectUrl: appendWithRedirect(
`${SIGN_IN}${SIGN_IN_SELECT_PROFILE_SUBROUTE}`,
parsedState.data.landingUrl.href,
),
success: true,
data: {
selectProfileStep: true,
landingUrl: parsedState.data.landingUrl,
},
}
}),
listStoredProfiles: publicProcedure.query(({ ctx }) => {
Expand Down
Loading
Loading