45ec23cc73
<img width="2284" height="2028" alt="CleanShot 2026-05-01 at 18 53 50@2x" src="https://github.com/user-attachments/assets/4f58cbb1-0168-40fb-a523-017f2ba625a1" /> ## Performance - **Per-request DB hit**: `getUserId` runs `getEffectiveSessionDuration` (User lookup + Org `aggregate`) on *every* authenticated request, including each fetcher poll. Consider caching the effective duration in the session cookie with a short TTL (e.g. 60s) and revalidating in the background. - **Double session commit in `root.tsx`**: `getUser` already runs the expiry check; then `commitAuthenticatedSessionLazy` commits the cookie again. Fine, but doubles `Set-Cookie` headers on every page load — worth a quick perf check. ## Correctness / Edge cases - **Lazy backfill assumes a root.tsx hit first**: users whose first post-deploy request is a fetcher/API route (`/resources/*`) skip the backfill until they navigate to a page. Not a security hole, but `getUserId` could backfill itself for completeness. - **No upper bound on `Organization.maxSessionDuration`**: admin API accepts `1` second, which would instant-logout every member on next request. Add a `min(60)` (or `min(300)` to match the lowest user option) to the Zod schema. - **No clock-skew tolerance**: `isSessionExpired` is exact-millisecond. Multi-instance deploys with skewed clocks could log users out a few seconds early/late. Probably fine for the 5-min minimum, but worth noting. ## Security - **Auto-logout audit log lacks IP/orgId**: HIPAA forensics typically wants source IP and which org context. Currently logs only `userId` + path. IP isn't PII for audit purposes; orgIds help correlate. Add both. - **Cookie `Max-Age` is 1 year regardless of user's setting**: intentional (server-side `issuedAt` is the source of truth), but reviewers will ask. Add a one-line comment on the cookie config explaining why. ## API surface - **`maxSessionDuration` is admin-PAT only**: no in-app UI for org owners to set/change their own cap. If this is "Trigger staff sets it during HIPAA onboarding", say so in the PR description; otherwise add an org-settings UI. - **Auto-submit dropdown has no confirmation**: misclicking "5 minutes" immediately shortens the user's session window with no undo. Consider a save button or 3-sec undo toast. ## Schema / migration - **`User.sessionDuration NOT NULL DEFAULT 31556952`**: instant on PG 11+ (metadata-only), but call out in the PR description so reviewers don't worry about a table rewrite on the User table. - **No DB-level constraint matching `SESSION_DURATION_OPTIONS`**: if the option list changes, existing users keep orphaned values. The dropdown's tag-along behaviour hides this — fine for now, but if you ever drop an option you'll need a backfill. ## UX - **Session expiry only fires on next request**: an idle authenticated tab keeps showing UI past the cap (until SSE/polling catches it, ~60s). Add a client-side timer based on the user's effective duration that triggers a fetcher to `/account` or `/logout` at expiry. - **No "you were signed out" message on logout**: users hitting their cap are bounced to `/` with no explanation. Was intentionally reverted in this PR — call that out so reviewers don't request it. ## Tests - Unit coverage on `sessionDuration.server.ts` is solid (215 lines). Missing: integration test for `getUserId` → expired session → redirect to `/logout`, and one for the loader's clamping fix (the most recent bug). Add at least the second one to lock in the regression. --------- Co-authored-by: Matt Aitken <matt@mattaitken.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
63 lines
2.1 KiB
TypeScript
63 lines
2.1 KiB
TypeScript
import type { LoaderFunction } from "@remix-run/node";
|
|
import { redirect } from "@remix-run/node";
|
|
import { prisma } from "~/db.server";
|
|
import { redirectWithErrorMessage } from "~/models/message.server";
|
|
import { authenticator } from "~/services/auth.server";
|
|
import { setLastAuthMethodHeader } from "~/services/lastAuthMethod.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";
|
|
import { sanitizeRedirectPath } from "~/utils";
|
|
|
|
export let loader: LoaderFunction = async ({ request }) => {
|
|
const cookie = request.headers.get("Cookie");
|
|
const redirectValue = await redirectCookie.parse(cookie);
|
|
const redirectTo = sanitizeRedirectPath(redirectValue);
|
|
|
|
const auth = await authenticator.authenticate("github", request, {
|
|
failureRedirect: "/login", // If auth fails, the failureRedirect will be thrown as a Response
|
|
});
|
|
|
|
const session = await getUserSession(request);
|
|
|
|
const userRecord = await prisma.user.findFirst({
|
|
where: {
|
|
id: auth.userId,
|
|
},
|
|
select: {
|
|
id: true,
|
|
mfaEnabledAt: true,
|
|
},
|
|
});
|
|
|
|
if (!userRecord) {
|
|
return redirectWithErrorMessage(
|
|
"/login",
|
|
request,
|
|
"Could not find your account. Please contact support."
|
|
);
|
|
}
|
|
|
|
if (userRecord.mfaEnabledAt) {
|
|
session.set("pending-mfa-user-id", userRecord.id);
|
|
session.set("pending-mfa-redirect-to", redirectTo);
|
|
|
|
const headers = new Headers();
|
|
headers.append("Set-Cookie", await commitSession(session));
|
|
headers.append("Set-Cookie", await setLastAuthMethodHeader("github"));
|
|
|
|
return redirect("/login/mfa", { headers });
|
|
}
|
|
|
|
session.set(authenticator.sessionKey, auth);
|
|
|
|
const headers = new Headers();
|
|
headers.append("Set-Cookie", await commitAuthenticatedSession(session, auth.userId));
|
|
headers.append("Set-Cookie", await setLastAuthMethodHeader("github"));
|
|
|
|
await trackAndClearReferralSource(request, auth.userId, headers);
|
|
|
|
return redirect(redirectTo, { headers });
|
|
};
|