From cb6088cf1fa8a79d73f0238972b77920cc34c3c0 Mon Sep 17 00:00:00 2001 From: isshaddad Date: Tue, 11 Aug 2026 11:46:01 -0400 Subject: [PATCH 1/9] fix(webapp): accept Plain customers without an external id on customer cards --- .../app/routes/api.v1.plain.customer-cards.ts | 30 ++----- .../app/utils/plainCustomerCards.test.ts | 87 +++++++++++++++++++ apps/webapp/app/utils/plainCustomerCards.ts | 57 ++++++++++++ 3 files changed, 149 insertions(+), 25 deletions(-) create mode 100644 apps/webapp/app/utils/plainCustomerCards.test.ts create mode 100644 apps/webapp/app/utils/plainCustomerCards.ts diff --git a/apps/webapp/app/routes/api.v1.plain.customer-cards.ts b/apps/webapp/app/routes/api.v1.plain.customer-cards.ts index 5e09fb08544..e10b6f7eeff 100644 --- a/apps/webapp/app/routes/api.v1.plain.customer-cards.ts +++ b/apps/webapp/app/routes/api.v1.plain.customer-cards.ts @@ -1,31 +1,11 @@ import { json, type ActionFunctionArgs } from "@remix-run/server-runtime"; import { timingSafeEqual } from "crypto"; import { uiComponent } from "@team-plain/ui-components"; -import { z } from "zod"; import { prisma } from "~/db.server"; import { env } from "~/env.server"; import { logger } from "~/services/logger.server"; import { generateImpersonationToken } from "~/services/impersonation.server"; - -// Schema for the request body from Plain -const PlainCustomerCardRequestSchema = z.object({ - cardKeys: z.array(z.string()), - customer: z - .object({ - id: z.string(), - email: z.string().optional(), - externalId: z.string().optional(), - }) - .refine((data) => data.email || data.externalId, { - message: "Either customer.email or customer.externalId must be provided", - path: ["customer"], - }), - thread: z - .object({ - id: z.string(), - }) - .optional(), -}); +import { answerAllCardKeys, PlainCustomerCardRequestSchema } from "~/utils/plainCustomerCards"; function sanitizeHeaders( request: Request, @@ -141,14 +121,14 @@ export async function action({ request }: ActionFunctionArgs) { const user = where ? await prisma.user.findFirst({ where, include: userInclude }) : null; - // If user not found, return empty cards + // No matching user: still answer every requested key, with no data so Plain hides the cards. if (!user) { logger.info("User not found for Plain customer card request", { customerId: customer.id, externalId: customer.externalId, hasEmail: !!customer.email, }); - return json({ cards: [] }); + return json({ cards: answerAllCardKeys(cardKeys, []) }); } // Build cards based on requested cardKeys @@ -420,13 +400,13 @@ export async function action({ request }: ActionFunctionArgs) { } default: - // Unknown card key - skip it + // Unknown card key - answered with no data by answerAllCardKeys below. logger.info("Unknown card key requested", { cardKey }); break; } } - return json({ cards }); + return json({ cards: answerAllCardKeys(cardKeys, cards) }); } catch (error) { logger.error("Error processing Plain customer card request", { error: error instanceof Error ? error.message : String(error), diff --git a/apps/webapp/app/utils/plainCustomerCards.test.ts b/apps/webapp/app/utils/plainCustomerCards.test.ts new file mode 100644 index 00000000000..53f45e62e36 --- /dev/null +++ b/apps/webapp/app/utils/plainCustomerCards.test.ts @@ -0,0 +1,87 @@ +import { describe, expect, it } from "vitest"; +import { answerAllCardKeys, PlainCustomerCardRequestSchema } from "./plainCustomerCards"; + +const request = (overrides: Record = {}) => ({ + cardKeys: ["account-details"], + customer: { id: "c_1", email: "dev@example.com", externalId: "user_1" }, + ...overrides, +}); + +describe("PlainCustomerCardRequestSchema", () => { + it("accepts a fully populated request", () => { + expect( + PlainCustomerCardRequestSchema.safeParse(request({ thread: { id: "th_1" } })).success + ).toBe(true); + }); + + // Plain sends explicit nulls rather than omitting these keys. Rejecting them meant every + // customer created outside our own writes got a 400 instead of a card. + it("accepts a null externalId when there is an email", () => { + const result = PlainCustomerCardRequestSchema.safeParse( + request({ customer: { id: "c_1", email: "dev@example.com", externalId: null } }) + ); + + expect(result.success).toBe(true); + }); + + it("accepts a null email when there is an externalId", () => { + const result = PlainCustomerCardRequestSchema.safeParse( + request({ customer: { id: "c_1", email: null, externalId: "user_1" } }) + ); + + expect(result.success).toBe(true); + }); + + it("accepts a null thread", () => { + expect(PlainCustomerCardRequestSchema.safeParse(request({ thread: null })).success).toBe(true); + }); + + it("accepts an omitted thread", () => { + expect(PlainCustomerCardRequestSchema.safeParse(request()).success).toBe(true); + }); + + it("still requires one of email or externalId", () => { + const result = PlainCustomerCardRequestSchema.safeParse( + request({ customer: { id: "c_1", email: null, externalId: null } }) + ); + + expect(result.success).toBe(false); + }); + + it("rejects a body with no card keys field", () => { + expect(PlainCustomerCardRequestSchema.safeParse({ customer: { id: "c_1" } }).success).toBe( + false + ); + }); +}); + +describe("answerAllCardKeys", () => { + it("adds a no-data card for every unanswered key", () => { + expect(answerAllCardKeys(["a", "b"], [])).toEqual([ + { key: "a", components: null }, + { key: "b", components: null }, + ]); + }); + + it("leaves answered cards untouched", () => { + const answered = { key: "a", components: [{ componentText: { text: "hi" } }] }; + + expect(answerAllCardKeys(["a"], [answered])).toEqual([answered]); + }); + + it("fills only the gaps, keeping answered cards first", () => { + const answered = { key: "b", components: [] }; + + expect(answerAllCardKeys(["a", "b", "c"], [answered])).toEqual([ + answered, + { key: "a", components: null }, + { key: "c", components: null }, + ]); + }); + + it("ignores extra cards that were not requested", () => { + const extra = { key: "unrequested", components: [] }; + + expect(answerAllCardKeys([], [extra])).toEqual([extra]); + }); +}); diff --git a/apps/webapp/app/utils/plainCustomerCards.ts b/apps/webapp/app/utils/plainCustomerCards.ts new file mode 100644 index 00000000000..c0be8d94538 --- /dev/null +++ b/apps/webapp/app/utils/plainCustomerCards.ts @@ -0,0 +1,57 @@ +import { z } from "zod"; + +/** + * The request Plain sends to a customer card endpoint. + * + * `email`, `externalId` and `thread` are nullish rather than optional because Plain sends these + * keys as explicit nulls rather than omitting them — `externalId` whenever the customer was + * created outside our own writes (its Slack integration, for one), `thread` when the card is + * loaded on the customer page rather than in a thread. `.optional()` accepts `undefined` but + * rejects `null`, which failed the whole request before any lookup could run. + */ +export const PlainCustomerCardRequestSchema = z.object({ + cardKeys: z.array(z.string()), + customer: z + .object({ + id: z.string(), + email: z.string().nullish(), + externalId: z.string().nullish(), + }) + .refine((data) => data.email || data.externalId, { + message: "Either customer.email or customer.externalId must be provided", + path: ["customer"], + }), + thread: z + .object({ + id: z.string(), + }) + .nullish(), +}); + +export type PlainCustomerCardRequest = z.infer; + +type NoDataCard = { key: string; components: null }; + +/** + * Fills in a `components: null` card for every requested key that wasn't answered. + * + * Plain records an integration error against any key it asked for and didn't get back, so a + * partial response surfaces in the support app as a broken card. `components: null` is how you + * say "this card has no data" and have Plain hide it instead. + */ +export function answerAllCardKeys( + cardKeys: string[], + cards: TCard[] +): (TCard | NoDataCard)[] { + const answered = new Set(cards.map((card) => card.key)); + + return [ + ...cards, + ...cardKeys + .filter((key) => !answered.has(key)) + .map((key): NoDataCard => ({ + key, + components: null, + })), + ]; +} From 5dc5f70010467ffe4da0853db0935b44727028e5 Mon Sep 17 00:00:00 2001 From: isshaddad Date: Tue, 11 Aug 2026 12:39:07 -0400 Subject: [PATCH 2/9] fix(webapp): let an admin switch impersonation target without stopping first --- apps/webapp/app/models/admin.server.ts | 56 ++++++++++++++----- .../_app.@.orgs.$organizationSlug.$.tsx | 4 +- apps/webapp/app/routes/admin.impersonate.tsx | 28 +++++++--- apps/webapp/app/services/session.server.ts | 27 +++++++++ 4 files changed, 91 insertions(+), 24 deletions(-) diff --git a/apps/webapp/app/models/admin.server.ts b/apps/webapp/app/models/admin.server.ts index e93844dbaed..eee3ecde009 100644 --- a/apps/webapp/app/models/admin.server.ts +++ b/apps/webapp/app/models/admin.server.ts @@ -9,7 +9,7 @@ import { setImpersonationId, } from "~/services/impersonation.server"; import { authenticator } from "~/services/auth.server"; -import { requireUser } from "~/services/session.server"; +import { getRealUser, requireUser } from "~/services/session.server"; import { extractClientIp } from "~/utils/extractClientIp.server"; import { impersonationDestinationPath } from "~/utils/pathBuilder"; @@ -210,35 +210,62 @@ export async function adminGetOrganizations(userId: string, { page, search }: Se }; } +/** + * Starts (or switches) impersonation. + * + * The admin gate resolves the *real* authenticated user itself. `requireUser` returns the + * impersonation target while impersonating, so callers that gated on it refused an admin who was + * already impersonating someone — they had to stop first — and would have attributed the audit row + * to the target rather than the admin. + * + * `verifiedAdmin` exists only so tests can supply an admin without a session cookie. Production + * callers must not pass it: passing a `requireUser` result is exactly the bug described above. + */ export async function redirectWithImpersonation( request: Request, userId: string, path: string, - currentUser?: { id: string; admin: boolean }, + verifiedAdmin?: { id: string; admin: boolean }, prismaClient: PrismaClientOrTransaction = prisma ) { - const user = currentUser ?? (await requireUser(request)); - if (!user.admin) { + const admin = verifiedAdmin ?? (await getRealUser(request, prismaClient)); + if (!admin?.admin) { throw new Error("Unauthorized"); } const xff = request.headers.get("x-forwarded-for"); const ipAddress = extractClientIp(xff); + const previousTargetId = await getImpersonationId(request); try { - await prismaClient.impersonationAuditLog.create({ - data: { - action: "START", - adminId: user.id, - targetId: userId, - ipAddress, - }, + await prismaClient.impersonationAuditLog.createMany({ + data: [ + // Switching straight from one target to another never passes through `clearImpersonation`, + // so close the previous session here or the trail shows two overlapping STARTs. + ...(previousTargetId && previousTargetId !== userId + ? [ + { + action: "STOP" as const, + adminId: admin.id, + targetId: previousTargetId, + ipAddress, + }, + ] + : []), + { + action: "START" as const, + adminId: admin.id, + targetId: userId, + ipAddress, + }, + ], }); } catch (error) { logger.error("Failed to create impersonation audit log", { error, - adminId: user.id, + adminId: admin.id, targetId: userId, + previousTargetId, }); } @@ -308,7 +335,8 @@ export async function startImpersonation( request: Request, organizationSlug: string, path: string, - currentUser: { id: string; admin: boolean }, + // Test-only, forwarded to `redirectWithImpersonation` — see its docstring. + verifiedAdmin?: { id: string; admin: boolean }, clients: { read: PrismaClientOrTransaction; write: PrismaClientOrTransaction } = { read: $replica, write: prisma, @@ -325,7 +353,7 @@ export async function startImpersonation( request, target.userId, impersonationDestinationPath(organizationSlug, path, new URL(request.url).search), - currentUser, + verifiedAdmin, clients.write ); } diff --git a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx index 2923a6fdeeb..5cecc06e56d 100644 --- a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx +++ b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx @@ -61,7 +61,7 @@ export async function loader({ request, params }: LoaderFunctionArgs) { // the consent page below instead, whose "Impersonate" button posts back from // our own page and so satisfies the same check. if (isSameOriginNavigation(request, env.LOGIN_ORIGIN)) { - throw await startImpersonation(request, organizationSlug, path, user); + throw await startImpersonation(request, organizationSlug, path); } // Expected for any link opened outside the app (address bar, bookmark, a link @@ -148,7 +148,7 @@ export async function action({ request, params }: ActionFunctionArgs) { // The consent form posts to an explicit absolute path (see // `impersonationConsentPostBackPath`), so the organization slug, the splat // path and the query string all arrive here intact. - return startImpersonation(request, organizationSlug, params["*"] ?? "", user); + return startImpersonation(request, organizationSlug, params["*"] ?? ""); } export default function Page() { diff --git a/apps/webapp/app/routes/admin.impersonate.tsx b/apps/webapp/app/routes/admin.impersonate.tsx index 458ed5b2a7e..e11dc5420ad 100644 --- a/apps/webapp/app/routes/admin.impersonate.tsx +++ b/apps/webapp/app/routes/admin.impersonate.tsx @@ -5,18 +5,30 @@ import { } from "@remix-run/server-runtime"; import { z } from "zod"; import { redirectWithImpersonation } from "~/models/admin.server"; -import { requireUser } from "~/services/session.server"; +import { getRealUser } from "~/services/session.server"; import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server"; import { logger } from "~/services/logger.server"; const FormSchema = z.object({ id: z.string() }); +/** + * The real authenticated user, or null when they aren't an admin. + * + * Must not use `requireUser`: while impersonating it resolves to the impersonation target, whose + * `admin` is false, so an admin switching to a second target was bounced to `/` and left on the + * first one. + */ +async function requireRealAdmin(request: Request) { + const admin = await getRealUser(request); + return admin?.admin ? admin : null; +} + async function handleImpersonationRequest(request: Request, userId: string): Promise { - const user = await requireUser(request); - if (!user.admin) { + const admin = await requireRealAdmin(request); + if (!admin) { return redirect("/"); } - return redirectWithImpersonation(request, userId, "/", user); + return redirectWithImpersonation(request, userId, "/"); } export const loader = async ({ request }: LoaderFunctionArgs) => { @@ -33,9 +45,9 @@ export const loader = async ({ request }: LoaderFunctionArgs) => { return redirect("/"); } - // Check admin BEFORE consuming the one-time token - const user = await requireUser(request); - if (!user.admin) { + // Check admin BEFORE consuming the one-time token, so a rejected request leaves the token usable. + const admin = await requireRealAdmin(request); + if (!admin) { return redirect("/"); } @@ -46,7 +58,7 @@ export const loader = async ({ request }: LoaderFunctionArgs) => { return redirect("/"); } - return redirectWithImpersonation(request, impersonateUserId, "/", user); + return redirectWithImpersonation(request, impersonateUserId, "/"); }; export async function action({ request }: ActionFunctionArgs) { diff --git a/apps/webapp/app/services/session.server.ts b/apps/webapp/app/services/session.server.ts index 753bc7f6a17..22e619acc69 100644 --- 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 { prisma, type PrismaClientOrTransaction } from "~/db.server"; import { getUserById } from "~/models/user.server"; import { sanitizeRedirectPath } from "~/utils"; import { extractClientIp } from "~/utils/extractClientIp.server"; @@ -124,6 +125,32 @@ export async function requireUserId(request: Request, redirectTo?: string) { return userId; } +/** + * The user the request actually authenticated as, ignoring any impersonation cookie. + * + * `getUserId` deliberately resolves to the *impersonated* id while impersonating, so `getUser` / + * `requireUser` answer "who is this request acting as". That is the wrong question for anything + * gating on admin rights or attributing an admin action: while impersonating a customer, + * `requireUser().admin` is that customer's flag, so an admin check silently fails and an audit + * record would name the customer as the actor. + * + * Returns null when unauthenticated or the row is gone. + */ +export async function getRealUser( + request: Request, + prismaClient: PrismaClientOrTransaction = prisma +) { + const authUser = await authenticator.isAuthenticated(request); + if (!authUser?.userId) return null; + + // Narrow select: callers only ever need the id and the admin flag. Takes a client so a caller + // already scoped to one reads the admin from the same database it writes to. + return prismaClient.user.findFirst({ + where: { id: authUser.userId }, + select: { id: true, admin: true }, + }); +} + export type UserFromSession = Awaited>; export async function requireUser(request: Request) { From 6551a587aa3da4da0dbb8438904b8624e4f82c19 Mon Sep 17 00:00:00 2001 From: isshaddad Date: Tue, 11 Aug 2026 13:01:33 -0400 Subject: [PATCH 3/9] fix(webapp): scope impersonation to id-matched customers and enforce session controls --- .../app/routes/api.v1.plain.customer-cards.ts | 42 +++++++++++++------ apps/webapp/app/services/session.server.ts | 20 +++++++-- apps/webapp/app/utils/plainCustomerCards.ts | 10 +++-- 3 files changed, 51 insertions(+), 21 deletions(-) diff --git a/apps/webapp/app/routes/api.v1.plain.customer-cards.ts b/apps/webapp/app/routes/api.v1.plain.customer-cards.ts index e10b6f7eeff..cb9e60b2187 100644 --- a/apps/webapp/app/routes/api.v1.plain.customer-cards.ts +++ b/apps/webapp/app/routes/api.v1.plain.customer-cards.ts @@ -121,11 +121,22 @@ export async function action({ request }: ActionFunctionArgs) { const user = where ? await prisma.user.findFirst({ where, include: userInclude }) : null; + /** + * Impersonation is offered only when the customer was matched on `externalId` — a value we set + * ourselves from `User.id`. + * + * Matching on email is a weaker claim: the address on a Plain customer isn't verified, and for + * customers created outside our own writes it comes from whoever sent the message. Offering a + * one-click impersonation link off the back of that would let an unverified address stand in + * for an account, so email-matched customers get the account rows without it. + */ + const canImpersonate = !!customer.externalId; + // No matching user: still answer every requested key, with no data so Plain hides the cards. if (!user) { + // Presence flags only — the identifiers themselves don't need to persist in log storage. logger.info("User not found for Plain customer card request", { - customerId: customer.id, - externalId: customer.externalId, + hasExternalId: !!customer.externalId, hasEmail: !!customer.email, }); return json({ cards: answerAllCardKeys(cardKeys, []) }); @@ -138,10 +149,21 @@ export async function action({ request }: ActionFunctionArgs) { for (const cardKey of cardKeys) { switch (cardKey) { case accountDetailsKey: { - // Generate a signed one-time token for impersonation - const impersonationToken = await generateImpersonationToken(user.id); - // Build the impersonate URL with token for CSRF protection - const impersonateUrl = `${env.APP_ORIGIN}/admin/impersonate?impersonate=${user.id}&impersonationToken=${encodeURIComponent(impersonationToken)}`; + // Only mint a token when the button will actually be rendered — see `canImpersonate`. + const impersonationComponents = canImpersonate + ? [ + uiComponent.spacer({ size: "M" }), + uiComponent.divider({ spacingSize: "M" }), + uiComponent.spacer({ size: "M" }), + uiComponent.linkButton({ + label: "Impersonate User", + // The one-time token is what protects this link against CSRF. + url: `${env.APP_ORIGIN}/admin/impersonate?impersonate=${user.id}&impersonationToken=${encodeURIComponent( + await generateImpersonationToken(user.id) + )}`, + }), + ] + : []; cards.push({ key: accountDetailsKey, @@ -221,13 +243,7 @@ export async function action({ request }: ActionFunctionArgs) { }), ], }), - uiComponent.spacer({ size: "M" }), - uiComponent.divider({ spacingSize: "M" }), - uiComponent.spacer({ size: "M" }), - uiComponent.linkButton({ - label: "Impersonate User", - url: impersonateUrl, - }), + ...impersonationComponents, ], }), ], diff --git a/apps/webapp/app/services/session.server.ts b/apps/webapp/app/services/session.server.ts index 22e619acc69..a34c346d433 100644 --- a/apps/webapp/app/services/session.server.ts +++ b/apps/webapp/app/services/session.server.ts @@ -141,14 +141,26 @@ export async function getRealUser( prismaClient: PrismaClientOrTransaction = prisma ) { const authUser = await authenticator.isAuthenticated(request); + + // Apply the same session controls `getUserId`/`getUser` apply to the real user, so this helper + // can't become a way around them: a session the IdP has revoked throws to /logout here, and one + // past its effective duration is caught below. Skipping either would let an admin whose session + // should have ended still start impersonation. + await revalidateSsoSession(request, authUser); if (!authUser?.userId) return null; - // Narrow select: callers only ever need the id and the admin flag. Takes a client so a caller - // already scoped to one reads the admin from the same database it writes to. - return prismaClient.user.findFirst({ + // Narrow select — callers need the id and the admin flag, plus `nextSessionEnd` for the deadline + // check. Takes a client so a caller already scoped to one reads the admin from the same database + // it writes to. + const user = await prismaClient.user.findFirst({ where: { id: authUser.userId }, - select: { id: true, admin: true }, + select: { id: true, admin: true, nextSessionEnd: true }, }); + if (!user) return null; + + maybeAutoLogout(request, user); + + return user; } export type UserFromSession = Awaited>; diff --git a/apps/webapp/app/utils/plainCustomerCards.ts b/apps/webapp/app/utils/plainCustomerCards.ts index c0be8d94538..e9ed1f8c4d5 100644 --- a/apps/webapp/app/utils/plainCustomerCards.ts +++ b/apps/webapp/app/utils/plainCustomerCards.ts @@ -49,9 +49,11 @@ export function answerAllCardKeys( ...cards, ...cardKeys .filter((key) => !answered.has(key)) - .map((key): NoDataCard => ({ - key, - components: null, - })), + .map( + (key): NoDataCard => ({ + key, + components: null, + }) + ), ]; } From 3c90ce311643c58d39a121a2c73f25e06dcde65c Mon Sep 17 00:00:00 2001 From: isshaddad Date: Tue, 11 Aug 2026 14:21:24 -0400 Subject: [PATCH 4/9] chore(webapp): drop the now-unused requireUser import --- apps/webapp/app/models/admin.server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/webapp/app/models/admin.server.ts b/apps/webapp/app/models/admin.server.ts index eee3ecde009..d5b3ae93c62 100644 --- a/apps/webapp/app/models/admin.server.ts +++ b/apps/webapp/app/models/admin.server.ts @@ -9,7 +9,7 @@ import { setImpersonationId, } from "~/services/impersonation.server"; import { authenticator } from "~/services/auth.server"; -import { getRealUser, requireUser } from "~/services/session.server"; +import { getRealUser } from "~/services/session.server"; import { extractClientIp } from "~/utils/extractClientIp.server"; import { impersonationDestinationPath } from "~/utils/pathBuilder"; From 4aadcd56fdc62ff8409a5afe0ee81a3b9c9a7858 Mon Sep 17 00:00:00 2001 From: isshaddad Date: Tue, 11 Aug 2026 16:35:13 -0400 Subject: [PATCH 5/9] fix(webapp): keep the impersonation link through login, order audit rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signing in from an impersonation link dropped the destination: the admin gate answered both "not signed in" and "signed in but not an admin" with a redirect to /, so an agent who clicked Impersonate while logged out landed on the dashboard afterwards. Unauthenticated requests now redirect to login carrying the original URL as redirectTo — the one-time token is validated after the gate, so it survives the round trip. The paired STOP/START audit rows were written with createMany, a single insert, so both took the same createdAt and a view ordered by that column could show the new START ahead of the STOP closing the previous session. They are now two statements so the sequence is unambiguous. --- apps/webapp/app/models/admin.server.ts | 40 +++++++++++--------- apps/webapp/app/routes/admin.impersonate.tsx | 15 +++++++- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/apps/webapp/app/models/admin.server.ts b/apps/webapp/app/models/admin.server.ts index d5b3ae93c62..799154f6b82 100644 --- a/apps/webapp/app/models/admin.server.ts +++ b/apps/webapp/app/models/admin.server.ts @@ -238,27 +238,31 @@ export async function redirectWithImpersonation( const previousTargetId = await getImpersonationId(request); try { - await prismaClient.impersonationAuditLog.createMany({ - data: [ - // Switching straight from one target to another never passes through `clearImpersonation`, - // so close the previous session here or the trail shows two overlapping STARTs. - ...(previousTargetId && previousTargetId !== userId - ? [ - { - action: "STOP" as const, - adminId: admin.id, - targetId: previousTargetId, - ipAddress, - }, - ] - : []), - { - action: "START" as const, + // Switching straight from one target to another never passes through `clearImpersonation`, so + // close the previous session here or the trail shows two overlapping STARTs. + // + // Two statements rather than one `createMany`: `createdAt` defaults to `now()`, which is fixed + // for the duration of a statement, so a single insert would stamp both rows identically and an + // audit view ordered by `createdAt` couldn't tell which came first — the very ambiguity the + // STOP row exists to remove. + if (previousTargetId && previousTargetId !== userId) { + await prismaClient.impersonationAuditLog.create({ + data: { + action: "STOP", adminId: admin.id, - targetId: userId, + targetId: previousTargetId, ipAddress, }, - ], + }); + } + + await prismaClient.impersonationAuditLog.create({ + data: { + action: "START", + adminId: admin.id, + targetId: userId, + ipAddress, + }, }); } catch (error) { logger.error("Failed to create impersonation audit log", { diff --git a/apps/webapp/app/routes/admin.impersonate.tsx b/apps/webapp/app/routes/admin.impersonate.tsx index e11dc5420ad..437e1ae7b74 100644 --- a/apps/webapp/app/routes/admin.impersonate.tsx +++ b/apps/webapp/app/routes/admin.impersonate.tsx @@ -5,20 +5,33 @@ import { } from "@remix-run/server-runtime"; import { z } from "zod"; import { redirectWithImpersonation } from "~/models/admin.server"; +import { authenticator } from "~/services/auth.server"; import { getRealUser } from "~/services/session.server"; import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server"; import { logger } from "~/services/logger.server"; +import { sanitizeRedirectPath } from "~/utils"; const FormSchema = z.object({ id: z.string() }); /** - * The real authenticated user, or null when they aren't an admin. + * The real authenticated user, or null when they're signed in but not an admin. * * Must not use `requireUser`: while impersonating it resolves to the impersonation target, whose * `admin` is false, so an admin switching to a second target was bounced to `/` and left on the * first one. + * + * Throws a login redirect when nobody is signed in, keeping this URL as `redirectTo` so the + * impersonation survives the round trip — the one-time token is validated after this gate, so it's + * still unconsumed when the browser comes back. Collapsing that into the non-admin `/` redirect + * would drop the link the agent clicked. */ async function requireRealAdmin(request: Request) { + if (!(await authenticator.isAuthenticated(request))) { + const url = new URL(request.url); + const redirectTo = sanitizeRedirectPath(`${url.pathname}${url.search}`); + throw redirect(`/login?${new URLSearchParams([["redirectTo", redirectTo]])}`); + } + const admin = await getRealUser(request); return admin?.admin ? admin : null; } From f1c6eb383007f6fd4a63cde41dcc9aaa7ee76ec0 Mon Sep 17 00:00:00 2001 From: isshaddad Date: Tue, 11 Aug 2026 16:59:38 -0400 Subject: [PATCH 6/9] fix(webapp): write the impersonation audit pair atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closing the previous impersonation and opening the new one were two separate statements sharing one error handler, so a failure between them could start an impersonation whose only record was the STOP for the previous target — an admin acting as someone with no trace of it. Both rows now go through the $transaction helper. createdAt is stamped explicitly because Postgres now() is the transaction timestamp: inside one transaction the default would give both rows the same value, leaving an audit view ordered by that column unable to tell which came first. --- apps/webapp/app/models/admin.server.ts | 52 +++++++++++++++----------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/apps/webapp/app/models/admin.server.ts b/apps/webapp/app/models/admin.server.ts index 799154f6b82..9ea767db538 100644 --- a/apps/webapp/app/models/admin.server.ts +++ b/apps/webapp/app/models/admin.server.ts @@ -1,5 +1,5 @@ import { redirect } from "@remix-run/server-runtime"; -import { $replica, prisma, type PrismaClientOrTransaction } from "~/db.server"; +import { $replica, $transaction, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { logger } from "~/services/logger.server"; import type { SearchParams } from "~/routes/admin._index"; import { @@ -237,32 +237,42 @@ export async function redirectWithImpersonation( const ipAddress = extractClientIp(xff); const previousTargetId = await getImpersonationId(request); + // Switching straight from one target to another never passes through `clearImpersonation`, so the + // previous session is closed here, or the trail shows two overlapping STARTs. + // + // Both rows are written in one transaction: as separate statements, a failure between them could + // start an impersonation whose only audit row is the STOP for the previous target — an admin + // acting as someone with no record of it. + // + // `createdAt` is stamped explicitly rather than left to `@default(now())`, because Postgres `now()` + // is the *transaction* timestamp: inside one transaction both rows would take the same value, and + // an audit view ordered by that column couldn't tell which came first. + const startedAt = new Date(); + const closedAt = new Date(startedAt.getTime() - 1); + try { - // Switching straight from one target to another never passes through `clearImpersonation`, so - // close the previous session here or the trail shows two overlapping STARTs. - // - // Two statements rather than one `createMany`: `createdAt` defaults to `now()`, which is fixed - // for the duration of a statement, so a single insert would stamp both rows identically and an - // audit view ordered by `createdAt` couldn't tell which came first — the very ambiguity the - // STOP row exists to remove. - if (previousTargetId && previousTargetId !== userId) { - await prismaClient.impersonationAuditLog.create({ + await $transaction(prismaClient, "startImpersonationAudit", async (tx) => { + if (previousTargetId && previousTargetId !== userId) { + await tx.impersonationAuditLog.create({ + data: { + action: "STOP", + adminId: admin.id, + targetId: previousTargetId, + ipAddress, + createdAt: closedAt, + }, + }); + } + + await tx.impersonationAuditLog.create({ data: { - action: "STOP", + action: "START", adminId: admin.id, - targetId: previousTargetId, + targetId: userId, ipAddress, + createdAt: startedAt, }, }); - } - - await prismaClient.impersonationAuditLog.create({ - data: { - action: "START", - adminId: admin.id, - targetId: userId, - ipAddress, - }, }); } catch (error) { logger.error("Failed to create impersonation audit log", { From 4f7fc9609d7448ea0fe1107e36fec349525bb894 Mon Sep 17 00:00:00 2001 From: isshaddad Date: Tue, 11 Aug 2026 17:11:46 -0400 Subject: [PATCH 7/9] fix(webapp): reset the view-as-user flag when switching impersonation target The flag is scoped to a single impersonation session, which is why stopping impersonation drops it. Switching straight from one target to another only became possible in this branch, and that path sets the impersonated id without touching the flag, so an admin viewing target A as the user landed on target B with the toggle still on. It is now cleared whenever the target changes. --- apps/webapp/app/services/impersonation.server.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/apps/webapp/app/services/impersonation.server.ts b/apps/webapp/app/services/impersonation.server.ts index e69a3fb305b..7d93390f704 100644 --- a/apps/webapp/app/services/impersonation.server.ts +++ b/apps/webapp/app/services/impersonation.server.ts @@ -45,6 +45,14 @@ export async function getImpersonationId(request: Request) { export async function setImpersonationId(userId: string, request: Request) { const session = await getImpersonationSession(request); + // Switching straight to a different target begins a new impersonation session, so the view-as-user + // flag must not carry over from the previous one — it's scoped to a single impersonation, which is + // why `clearImpersonationId` drops it too. Reachable only since switching stopped requiring a stop + // first; before that, every second target arrived via `clearImpersonationId`. + if (session.get(IMPERSONATED_USER_ID_KEY) !== userId) { + session.unset(VIEWING_AS_USER_KEY); + } + session.set(IMPERSONATED_USER_ID_KEY, userId); return session; From 7be4c3bde00f3b635df79423bbc6f54b1531a03d Mon Sep 17 00:00:00 2001 From: isshaddad Date: Tue, 11 Aug 2026 17:50:52 -0400 Subject: [PATCH 8/9] fix(webapp): take the impersonate route out of the admin layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin layout gates on requireSuper via getUserId, which resolves to the impersonated user while impersonating — so starting on a second target ran that gate against the target and it answered with its own redirect to /. The switch worked only because the router prefers the deepest redirect, which is too subtle a thing for an auth path to depend on. The trailing underscore keeps the route at /admin/impersonate while opting it out of the layout. Nothing is lost: the route only ever redirects, so it never rendered inside it. --- ...{admin.impersonate.tsx => admin_.impersonate.tsx} | 12 ++++++++++++ 1 file changed, 12 insertions(+) rename apps/webapp/app/routes/{admin.impersonate.tsx => admin_.impersonate.tsx} (81%) diff --git a/apps/webapp/app/routes/admin.impersonate.tsx b/apps/webapp/app/routes/admin_.impersonate.tsx similarity index 81% rename from apps/webapp/app/routes/admin.impersonate.tsx rename to apps/webapp/app/routes/admin_.impersonate.tsx index 437e1ae7b74..8b71bf94f1e 100644 --- a/apps/webapp/app/routes/admin.impersonate.tsx +++ b/apps/webapp/app/routes/admin_.impersonate.tsx @@ -11,6 +11,18 @@ import { validateAndConsumeImpersonationToken } from "~/services/impersonation.s import { logger } from "~/services/logger.server"; import { sanitizeRedirectPath } from "~/utils"; +/** + * Served at `/admin/impersonate`, but the trailing `_` on `admin_` keeps it out of the `admin.tsx` + * layout on purpose. + * + * That layout's loader is `dashboardLoader({ authorization: { requireSuper: true } })`, which + * resolves the user through `getUserId` — the impersonated id while impersonating. So starting on a + * second target ran the parent gate against the target, which isn't a super admin, and it answered + * with its own `redirect("/")`. Nesting would leave this route's behaviour depending on the router + * preferring the deepest redirect; opting out removes the question. Nothing is lost — this route + * only ever redirects, so it never rendered inside the layout anyway. + */ + const FormSchema = z.object({ id: z.string() }); /** From e5e910bd0abd159f125b76098e3fcc20c5648982 Mon Sep 17 00:00:00 2001 From: isshaddad Date: Tue, 11 Aug 2026 18:01:13 -0400 Subject: [PATCH 9/9] fix(webapp): gate the impersonate route on canSuper, not the admin column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opting out of the admin layout also opted out of its requireSuper check, leaving this the only admin entry point gated on the raw User.admin column. canSuper() equals that column in the OSS fallback, but an RBAC plugin is free to be stricter. The ability is now built explicitly for the real admin's id and canSuper() is checked directly. dashboardLoader can't be used: it resolves its subject with getUserId, which is the impersonated id while impersonating — the bug this route exists to fix. --- apps/webapp/app/routes/admin_.impersonate.tsx | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/apps/webapp/app/routes/admin_.impersonate.tsx b/apps/webapp/app/routes/admin_.impersonate.tsx index 8b71bf94f1e..f55d258a9f6 100644 --- a/apps/webapp/app/routes/admin_.impersonate.tsx +++ b/apps/webapp/app/routes/admin_.impersonate.tsx @@ -6,6 +6,7 @@ import { import { z } from "zod"; import { redirectWithImpersonation } from "~/models/admin.server"; import { authenticator } from "~/services/auth.server"; +import { rbac } from "~/services/rbac.server"; import { getRealUser } from "~/services/session.server"; import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server"; import { logger } from "~/services/logger.server"; @@ -45,7 +46,18 @@ async function requireRealAdmin(request: Request) { } const admin = await getRealUser(request); - return admin?.admin ? admin : null; + if (!admin) return null; + + // Same gate `dashboardLoader({ authorization: { requireSuper: true } })` applies, evaluated + // against the real admin. It can't be reached through the builder here, because the builder + // resolves its subject with `getUserId` — the impersonated id while impersonating, which is the + // bug this route exists to fix. So the ability is built explicitly for `admin.id` instead of + // trusting the raw `User.admin` column: `canSuper()` is only equal to that column in the OSS + // fallback, and a plugin is free to be stricter. requireSuper needs no org/project scope. + const auth = await rbac.authenticateSession(request, { userId: admin.id }); + if (!auth.ok || !auth.ability.canSuper()) return null; + + return admin; } async function handleImpersonationRequest(request: Request, userId: string): Promise {