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

zhongliang02 · 2025-01-15T09:44:46Z

Context

Due to yet another open redirect VAPT finding (care360: GTA-55-008), it has proved to be very challenging to validate the callbackUrl parameter.

Approach

This PR completely removes the callbackUrl parameter in favor of the redirectRouteKey parameter.

In all situations where we redirect ,redirectRouteKey is first validated to be keyof AllRoutes, falling back to HOME when invalid. (AllRoutes is a dict with all the routes in the app).

Then we resolve the route key to the actual route.

How to read this PR

Please look at all the utilities lib/ schemas/ utils/ first to familiarize with the constructs that enable this route key pattern.

Then evaluate the components involved in the email login flow followed by the SGID login flow.

Finally you can evaluate utility component wrappers under AuthWrappers/

Risks and Testing

Manually tested on https://starter-3ela7h4z5-ogp-tooling.vercel.app/

Email login
SGID login

vercel · 2025-01-15T09:44:51Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
starter-kit	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 20, 2025 4:19am

datadog-opengovsg · 2025-01-15T09:47:46Z

Datadog Report

Branch report: 01-15-feat_replace_redirect_url_with_route_key_pattern
Commit report: c1b1aa9
Test service: starter-kit

✅ 0 Failed, 11 Passed, 0 Skipped, 10.84s Total Time
⬆️ Test Sessions change in coverage: 1 increased (+0.11%)

karrui

i feel like we can keep the changes to a minimum, instead of changing all the variables to something new. The concept is still the same right, just that the "redirect URL"s are now restricted to what the user has explicitly set. should we just change the callbackUrl schema and keep everything else the same?

(trying to make it easy for starter-kit derived project to update their code too, since there might be too many things to change in here for them)

src/lib/routes.ts

src/components/AuthWrappers/EnforceLoginStatePageWrapper.tsx

src/components/AuthWrappers/PublicPageWrapper.tsx

karrui · 2025-01-20T02:35:11Z

src/features/sign-in/components/SgidCallback.tsx


  useEffect(() => {
-    if (!selectProfileStep) {
-      setHasLoginStateFlag()
+    if (!response.success) {
+      void router.replace(


should this be run through the zod validation schema too?

I think it's not necessary because SIGN_IN is a hardcoded constant

karrui · 2025-01-20T02:37:17Z

src/features/sign-in/components/SgidLogin/SgidLoginButton.tsx


 export const SgidLoginButton = (): JSX.Element | null => {
  const router = useRouter()
  const sgidLoginMutation = trpc.auth.sgid.login.useMutation({
    onSuccess: async ({ redirectUrl }) => {
-      await router.push(redirectUrl)
+      window.location.href = redirectUrl


when using nextjs, it is best practice to check for the location of window.location first, since this component might be SSR'd.
this might be a subtle bug if somehow invoked from the server.

why was the router.push call replaced with this? was there a particular reason?

hmm it's because redirectUrl is already known to be an external URL, so I thought there'd be no benefits from router.pushing it, but I was not aware of the possibility of SSR'ing the component, I thought it's always CSR unless there's getServerSideProps.

karrui · 2025-01-20T02:38:43Z

src/utils/url.ts

+export const resolveRouteKey = (
+  v: unknown,
+): (typeof AllRoutes)[keyof typeof AllRoutes] => {
+  return AllRoutes[routeKeySchema.parse(v)]


should we return a fallback here? unsure, but better than users not changing routes maybe?

routeKeySchema.parse(v) will fallback to HOME after parsing. routeKeySchema is guaranteed to produce keyof AllRoutes

zhongliang02 · 2025-01-20T02:58:21Z

Re: minimizing changes to variables, I think conceptually it is quite different though,

previous: ?callbackUrl=/home
new: ?redirectRouteKey=HOME, then ALL_ROUTES = {'HOME': '/home'} and we just router.push(ALL_ROUTES[...])

In theory we can remove this layer of indirection and change it to something like
?callbackUrl=/home and ALLOWED_ROUTES = ['/home', ...]

I'll make a commit with the above and we can see how that looks

zhongliang02 · 2025-01-20T04:09:52Z

Reverted to the redirect url pattern, but now callbackUrlSchema simply follows a whitelist.
Technically this is more of a kitchen sink PR now, lmk if you prefer if I split it up to

callbackUrlSchema changes
useEffect fixes
sgid login minor refactoring

Testing checklist

Test email happy flow
Test sgid happy flow

karrui · 2025-01-20T04:48:42Z

src/components/AuthWrappers/EnforceLoginStatePageWrapper.tsx


  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.


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

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

karrui · 2025-01-20T04:51:26Z

src/lib/routes.ts


 export const HOME = '/home'
 export const PROFILE = '/profile'
 export const SETTINGS_PROFILE = '/settings/profile'
+
+export const ALLOWED_CALLBACK_ROUTES = [


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

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

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

feat: replace redirect url pattern with route key pattern

13d8848

zhongliang02 requested a review from karrui January 15, 2025 09:56

zhongliang02 marked this pull request as ready for review January 15, 2025 09:56

karrui reviewed Jan 20, 2025

View reviewed changes

zhongliang02 added 2 commits January 20, 2025 11:00

chore: rename AllRoutes to ALL_ROUTES

c123713

fix: put PublicPageWrapper router calls in a useEffect

097298d

vercel bot deployed to Preview January 20, 2025 03:14 View deployment

fix: sgidLoginButton to use router.push instead of window.location

035ac17

vercel bot deployed to Preview January 20, 2025 03:17 View deployment

feat: retain redirect url pattern but validate urls against whitelist

bd0228d

vercel bot deployed to Preview January 20, 2025 04:02 View deployment

chore: npm format:fix

d97cb95

vercel bot deployed to Preview January 20, 2025 04:11 View deployment

fix: callbackUrlSchema should fallback gracefully instead of throwing

8ed0be6

vercel bot deployed to Preview January 20, 2025 04:17 View deployment

karrui reviewed Jan 20, 2025

View reviewed changes

karrui requested a review from spaceraccoon February 27, 2025 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zhongliang02 commented Jan 15, 2025 •

edited

Loading

vercel bot commented Jan 15, 2025 •

edited

Loading

datadog-opengovsg bot commented Jan 15, 2025 •

edited

Loading

karrui left a comment •

edited

Loading

karrui Jan 20, 2025

zhongliang02 Jan 20, 2025

karrui Jan 20, 2025

zhongliang02 Jan 20, 2025

karrui Jan 20, 2025

zhongliang02 Jan 20, 2025

zhongliang02 commented Jan 20, 2025

zhongliang02 commented Jan 20, 2025 •

edited

Loading

karrui Jan 20, 2025

zhongliang02 Jan 20, 2025

karrui Jan 20, 2025

zhongliang02 Jan 20, 2025

zhongliang02 Jan 20, 2025

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

Are you sure you want to change the base?

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

Conversation

zhongliang02 commented Jan 15, 2025 • edited Loading

Context

Approach

How to read this PR

Risks and Testing

vercel bot commented Jan 15, 2025 • edited Loading

datadog-opengovsg bot commented Jan 15, 2025 • edited Loading

Datadog Report

karrui left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongliang02 commented Jan 20, 2025

zhongliang02 commented Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongliang02 commented Jan 15, 2025 •

edited

Loading

vercel bot commented Jan 15, 2025 •

edited

Loading

datadog-opengovsg bot commented Jan 15, 2025 •

edited

Loading

karrui left a comment •

edited

Loading

zhongliang02 commented Jan 20, 2025 •

edited

Loading