From 83c8189f3ae1ac8d5c187fa13ce1ae0722c50483 Mon Sep 17 00:00:00 2001
From: scosman <scosman@users.noreply.github.com>
Date: Mon, 2 Sep 2024 11:36:12 -0400
Subject: [PATCH] Fix a redirect loop. We were inconsistent for how we
 redirected login to account and vice versa. A expired session could enter
 redirect loop.

Also dont redirect from authHandler. I don't want to leave any page to account when logged in, just /login/*
---
 .vscode/settings.json                         |  3 +-
 src/hooks.server.ts                           | 11 +----
 src/lib/load_helpers.ts                       | 47 +++++++++++++++++++
 src/routes/(admin)/account/+layout.ts         | 29 ++----------
 .../(marketing)/login/+layout.server.ts       |  7 +--
 src/routes/(marketing)/login/+layout.ts       |  8 ++++
 6 files changed, 63 insertions(+), 42 deletions(-)
 create mode 100644 src/lib/load_helpers.ts

diff --git a/.vscode/settings.json b/.vscode/settings.json
index 78afa4dd..af9b4a9f 100644
--- a/.vscode/settings.json
+++ b/.vscode/settings.json
@@ -9,5 +9,6 @@
   "[javascript]": {
     "editor.defaultFormatter": "esbenp.prettier-vscode"
   },
-  "eslint.validate": ["javascript", "javascriptreact", "typescript", "svelte"]
+  "eslint.validate": ["javascript", "javascriptreact", "typescript", "svelte"],
+  "editor.tabSize": 2
 }
diff --git a/src/hooks.server.ts b/src/hooks.server.ts
index 461d211a..a0ba9389 100644
--- a/src/hooks.server.ts
+++ b/src/hooks.server.ts
@@ -8,7 +8,6 @@ import { createServerClient } from "@supabase/ssr"
 import { createClient } from "@supabase/supabase-js"
 import type { Handle } from "@sveltejs/kit"
 import { sequence } from "@sveltejs/kit/hooks"
-import { redirect } from "@sveltejs/kit"
 
 export const supabase: Handle = async ({ event, resolve }) => {
   event.locals.supabase = createServerClient(
@@ -85,19 +84,13 @@ export const supabase: Handle = async ({ event, resolve }) => {
   })
 }
 
+// Not called for prerendered marketing pages so generally okay to call on ever server request
+// Next-page CSR will mean relatively minimal calls to this hook
 const authGuard: Handle = async ({ event, resolve }) => {
   const { session, user } = await event.locals.safeGetSession()
   event.locals.session = session
   event.locals.user = user
 
-  if (!event.locals.session && event.url.pathname.startsWith("/account")) {
-    redirect(303, "/login")
-  }
-
-  if (event.locals.session && event.url.pathname === "/login") {
-    redirect(303, "/account")
-  }
-
   return resolve(event)
 }
 
diff --git a/src/lib/load_helpers.ts b/src/lib/load_helpers.ts
new file mode 100644
index 00000000..cf16f469
--- /dev/null
+++ b/src/lib/load_helpers.ts
@@ -0,0 +1,47 @@
+import { isBrowser } from "@supabase/ssr"
+import type { Session, SupabaseClient } from "@supabase/supabase-js"
+import type { Database } from "../DatabaseDefinitions.js"
+
+export const load_helper = async (
+  server_session: Session | null,
+  supabase: SupabaseClient<Database>,
+) => {
+  // on server populated on server by LayoutData, using authGuard hook
+  let session = server_session
+  if (isBrowser()) {
+    // Only call getSession in browser where it's safe.
+    const getSessionResponse = await supabase.auth.getSession()
+    session = getSessionResponse.data.session
+  }
+  if (!session) {
+    return {
+      session: null,
+      user: null,
+    }
+  }
+
+  // https://github.com/supabase/auth-js/issues/888#issuecomment-2189298518
+  if ("suppressGetSessionWarning" in supabase.auth) {
+    // @ts-expect-error - suppressGetSessionWarning is not part of the official API
+    supabase.auth.suppressGetSessionWarning = true
+  } else {
+    console.warn(
+      "SupabaseAuthClient#suppressGetSessionWarning was removed. See https://github.com/supabase/auth-js/issues/888.",
+    )
+  }
+  const {
+    data: { user },
+    error: userError,
+  } = await supabase.auth.getUser()
+  if (userError || !user) {
+    return {
+      session: null,
+      user: null,
+    }
+  }
+
+  return {
+    session,
+    user,
+  }
+}
diff --git a/src/routes/(admin)/account/+layout.ts b/src/routes/(admin)/account/+layout.ts
index d5ed0e3f..b3e5efc7 100644
--- a/src/routes/(admin)/account/+layout.ts
+++ b/src/routes/(admin)/account/+layout.ts
@@ -10,6 +10,7 @@ import {
 import { redirect } from "@sveltejs/kit"
 import type { Database } from "../../../DatabaseDefinitions.js"
 import { CreateProfileStep } from "../../../config"
+import { load_helper } from "$lib/load_helpers"
 
 export const load = async ({ fetch, data, depends, url }) => {
   depends("supabase:auth")
@@ -31,32 +32,8 @@ export const load = async ({ fetch, data, depends, url }) => {
         },
       })
 
-  // on server populated on server by LayoutData, using authGuard hook
-  let session = data.session
-  if (isBrowser()) {
-    // Only call getSession in browser where it's safe.
-    const getSessionResponse = await supabase.auth.getSession()
-    session = getSessionResponse.data.session
-  }
-  if (!session) {
-    redirect(303, "/login")
-  }
-
-  // https://github.com/supabase/auth-js/issues/888#issuecomment-2189298518
-  if ("suppressGetSessionWarning" in supabase.auth) {
-    // @ts-expect-error - suppressGetSessionWarning is not part of the official API
-    supabase.auth.suppressGetSessionWarning = true
-  } else {
-    console.warn(
-      "SupabaseAuthClient#suppressGetSessionWarning was removed. See https://github.com/supabase/auth-js/issues/888.",
-    )
-  }
-  const {
-    data: { user },
-    error: userError,
-  } = await supabase.auth.getUser()
-  if (userError || !user) {
-    // JWT validation has failed
+  const { session, user } = await load_helper(data.session, supabase)
+  if (!session || !user) {
     redirect(303, "/login")
   }
 
diff --git a/src/routes/(marketing)/login/+layout.server.ts b/src/routes/(marketing)/login/+layout.server.ts
index 019d6373..4749e491 100644
--- a/src/routes/(marketing)/login/+layout.server.ts
+++ b/src/routes/(marketing)/login/+layout.server.ts
@@ -1,4 +1,3 @@
-import { redirect } from "@sveltejs/kit"
 import type { LayoutServerLoad } from "./$types"
 
 export const load: LayoutServerLoad = async ({
@@ -6,13 +5,9 @@ export const load: LayoutServerLoad = async ({
   cookies,
   url,
 }) => {
-  // if the user is already logged in return them to the account page
-  if (session) {
-    redirect(303, "/account")
-  }
-
   return {
     url: url.origin,
     cookies: cookies.getAll(),
+    session,
   }
 }
diff --git a/src/routes/(marketing)/login/+layout.ts b/src/routes/(marketing)/login/+layout.ts
index 333c525c..2163909e 100644
--- a/src/routes/(marketing)/login/+layout.ts
+++ b/src/routes/(marketing)/login/+layout.ts
@@ -7,6 +7,8 @@ import {
   createServerClient,
   isBrowser,
 } from "@supabase/ssr"
+import { redirect } from "@sveltejs/kit"
+import { load_helper } from "$lib/load_helpers.js"
 
 export const load = async ({ fetch, data, depends }) => {
   depends("supabase:auth")
@@ -28,6 +30,12 @@ export const load = async ({ fetch, data, depends }) => {
         },
       })
 
+  // Redirect if already logged in
+  const { session, user } = await load_helper(data.session, supabase)
+  if (session && user) {
+    redirect(303, "/account")
+  }
+
   const url = data.url
 
   return { supabase, url }