Code review fixes and improvements

samejr · samejr · commit 2726647d5a50 · 2026-04-30T21:28:06.000+01:00
diff --git a/apps/webapp/app/routes/account.security/route.tsx b/apps/webapp/app/routes/account.security/route.tsx
@@ -8,6 +8,7 @@ import {
 } from "~/components/layout/AppLayout";
 import { Header2 } from "~/components/primitives/Headers";
 import { NavBar, PageTitle } from "~/components/primitives/PageHeader";
+import { $replica } from "~/db.server";
 import { requireUser } from "~/services/session.server";
 import {
   getAllowedSessionOptions,
@@ -27,7 +28,7 @@ export const meta: MetaFunction = () => {
 export async function loader({ request }: LoaderFunctionArgs) {
   const user = await requireUser(request);
 
-  const { durationSeconds, orgCapSeconds } = await getEffectiveSessionDuration(user.id);
+  const { durationSeconds, orgCapSeconds } = await getEffectiveSessionDuration(user.id, $replica);
   const sessionDurationOptions = getAllowedSessionOptions(orgCapSeconds, durationSeconds);
 
   return typedjson({
diff --git a/apps/webapp/app/routes/auth.github.callback.tsx b/apps/webapp/app/routes/auth.github.callback.tsx
@@ -1,10 +1,10 @@
 import type { LoaderFunction } from "@remix-run/node";
 import { redirect } from "@remix-run/node";
 import { prisma } from "~/db.server";
-import { getSession, redirectWithErrorMessage } from "~/models/message.server";
+import { redirectWithErrorMessage } from "~/models/message.server";
 import { authenticator } from "~/services/auth.server";
 import { setLastAuthMethodHeader } from "~/services/lastAuthMethod.server";
-import { commitSession } from "~/services/sessionStorage.server";
+import { commitSession, getUserSession } from "~/services/sessionStorage.server";
 import { commitAuthenticatedSession } from "~/services/sessionDuration.server";
 import { trackAndClearReferralSource } from "~/services/referralSource.server";
 import { redirectCookie } from "./auth.github";
@@ -19,7 +19,7 @@ export let loader: LoaderFunction = async ({ request }) => {
     failureRedirect: "/login", // If auth fails, the failureRedirect will be thrown as a Response
   });
 
-  const session = await getSession(request.headers.get("cookie"));
+  const session = await getUserSession(request);
 
   const userRecord = await prisma.user.findFirst({
     where: {
diff --git a/apps/webapp/app/routes/auth.google.callback.tsx b/apps/webapp/app/routes/auth.google.callback.tsx
@@ -1,10 +1,10 @@
 import type { LoaderFunction } from "@remix-run/node";
 import { redirect } from "@remix-run/node";
 import { prisma } from "~/db.server";
-import { getSession, redirectWithErrorMessage } from "~/models/message.server";
+import { redirectWithErrorMessage } from "~/models/message.server";
 import { authenticator } from "~/services/auth.server";
 import { setLastAuthMethodHeader } from "~/services/lastAuthMethod.server";
-import { commitSession } from "~/services/sessionStorage.server";
+import { commitSession, getUserSession } from "~/services/sessionStorage.server";
 import { commitAuthenticatedSession } from "~/services/sessionDuration.server";
 import { trackAndClearReferralSource } from "~/services/referralSource.server";
 import { redirectCookie } from "./auth.google";
@@ -19,7 +19,7 @@ export let loader: LoaderFunction = async ({ request }) => {
     failureRedirect: "/login", // If auth fails, the failureRedirect will be thrown as a Response
   });
 
-  const session = await getSession(request.headers.get("cookie"));
+  const session = await getUserSession(request);
 
   const userRecord = await prisma.user.findFirst({
     where: {
diff --git a/apps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsx b/apps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsx
@@ -14,7 +14,7 @@ export function MfaToggle({ isEnabled, onToggle }: MfaToggleProps) {
     <Form method="post" className="w-full">
       <div className="flex w-full items-center justify-between gap-4">
         <InputGroup className="flex-1">
-          <Label>Multi-factor authentication</Label>
+          <Label htmlFor="mfa">Multi-factor authentication</Label>
           <Paragraph variant="small">
             Require a one-time code from your authenticator app (TOTP).
           </Paragraph>
diff --git a/apps/webapp/app/services/session.server.ts b/apps/webapp/app/services/session.server.ts
@@ -1,4 +1,5 @@
 import { redirect } from "@remix-run/node";
+import { $replica } from "~/db.server";
 import { getUserById } from "~/models/user.server";
 import { sanitizeRedirectPath } from "~/utils";
 import { authenticator } from "./auth.server";
@@ -11,6 +12,44 @@ import {
 } from "./sessionDuration.server";
 import { getUserSession } from "./sessionStorage.server";
 
+/**
+ * Enforces the user's effective session duration (User.sessionDuration capped
+ * by the most restrictive Organization.maxSessionDuration). If the session was
+ * issued longer ago than the cap allows, throws a redirect to `/logout` and
+ * emits a HIPAA audit log. `userId` is always the *session owner's* id (i.e.
+ * the real authenticated user), not an impersonated one — because the cap
+ * belongs to the cookie, not the impersonation target.
+ */
+async function enforceSessionExpiry(
+  request: Request,
+  userId: string,
+  impersonatedUserId: string | null = null
+): Promise<void> {
+  const session = await getUserSession(request);
+  // Hot path: every authenticated request runs this. Read from the replica
+  // when one is configured (falls back to primary). Stale-by-replica-lag is
+  // acceptable here because the worst case is a session living a few seconds
+  // past its cap on the very first request after a cap change.
+  const { durationSeconds, orgCapSeconds, userSettingSeconds } =
+    await getEffectiveSessionDuration(userId, $replica);
+  if (!isSessionExpired(session, durationSeconds)) return;
+
+  const issuedAt = getSessionIssuedAt(session);
+  // HIPAA audit trail: structured log lands in CloudWatch via stdout. Use
+  // the stable `event` field to filter/aggregate auto-logout events.
+  logger.info("Auto-logout: session exceeded effective duration", {
+    event: "session.auto_logout",
+    userId,
+    impersonatedUserId,
+    effectiveDurationSeconds: durationSeconds,
+    userSettingSeconds,
+    orgCapSeconds,
+    sessionAgeMs: issuedAt === null ? null : Date.now() - issuedAt,
+    requestPath: new URL(request.url).pathname,
+  });
+  throw redirect("/logout");
+}
+
 export async function getUserId(request: Request): Promise<string | undefined> {
   const impersonatedUserId = await getImpersonationId(request);
 
@@ -20,39 +59,24 @@ export async function getUserId(request: Request): Promise<string | undefined> {
     if (authUser?.userId) {
       const realUser = await getUserById(authUser.userId);
       if (realUser?.admin) {
+        // Enforce expiry against the admin's own session — impersonation must
+        // not be a way to bypass the admin's effective duration cap.
+        await enforceSessionExpiry(request, authUser.userId, impersonatedUserId);
         return impersonatedUserId;
       }
     }
-    // Admin revoked or session invalid — fall through to return the real user's ID
+    // Admin revoked or session invalid — fall through to return the real
+    // user's ID. Same enforcement as the regular auth path below.
+    if (authUser?.userId) {
+      await enforceSessionExpiry(request, authUser.userId);
+    }
     return authUser?.userId;
   }
 
   const authUser = await authenticator.isAuthenticated(request);
   if (!authUser?.userId) return undefined;
 
-  // Enforce the user's effective session duration (User.sessionDuration capped
-  // by the most restrictive Organization.maxSessionDuration). If the session
-  // was issued longer ago than the cap allows, force a logout.
-  const session = await getUserSession(request);
-  const { durationSeconds, orgCapSeconds, userSettingSeconds } = await getEffectiveSessionDuration(
-    authUser.userId
-  );
-  if (isSessionExpired(session, durationSeconds)) {
-    const issuedAt = getSessionIssuedAt(session);
-    // HIPAA audit trail: structured log lands in CloudWatch via stdout. Use
-    // the stable `event` field to filter/aggregate auto-logout events.
-    logger.info("Auto-logout: session exceeded effective duration", {
-      event: "session.auto_logout",
-      userId: authUser.userId,
-      effectiveDurationSeconds: durationSeconds,
-      userSettingSeconds,
-      orgCapSeconds,
-      sessionAgeMs: issuedAt === null ? null : Date.now() - issuedAt,
-      requestPath: new URL(request.url).pathname,
-    });
-    throw redirect("/logout");
-  }
-
+  await enforceSessionExpiry(request, authUser.userId);
   return authUser.userId;
 }
 
diff --git a/apps/webapp/app/services/sessionDuration.server.ts b/apps/webapp/app/services/sessionDuration.server.ts
@@ -74,7 +74,7 @@ export async function getEffectiveSessionDuration(
   client: PrismaClientOrTransaction = prisma
 ): Promise<EffectiveSessionDuration> {
   const [user, orgCap] = await Promise.all([
-    client.user.findUnique({
+    client.user.findFirst({
       where: { id: userId },
       select: { sessionDuration: true },
     }),
diff --git a/apps/webapp/app/utils.ts b/apps/webapp/app/utils.ts
@@ -5,8 +5,10 @@ const DEFAULT_REDIRECT = "/";
 
 // Pathnames that are NOT user-navigable destinations: fetcher endpoints,
 // OAuth/auth callbacks, JSON APIs, the magic-link redemption route, and the
-// auth flow routes themselves (which would create a redirect loop).
-const NON_NAVIGABLE_PREFIXES = ["/resources/", "/auth/", "/admin/", "/api/", "/engine/"];
+// auth flow routes themselves (which would create a redirect loop). Note
+// `/admin/api/` covers admin JSON endpoints while leaving `/admin`,
+// `/admin/back-office/*`, `/admin/orgs`, etc. navigable.
+const NON_NAVIGABLE_PREFIXES = ["/resources/", "/auth/", "/admin/api/", "/api/", "/engine/"];
 const NON_NAVIGABLE_EXACT = new Set(["/magic", "/logout", "/login", "/login/magic", "/login/mfa"]);
 
 function isNavigablePath(pathname: string): boolean {
