fix(webapp): account & access-control hardening (auth, RBAC, org membership, impersonation) (#42)

Author: d-cs

.server-changes/account-access-control-hardening.md

@@ -0,0 +1,6 @@
+---
+area: webapp
+type: fix
+---
+
+Harden account and access-control handling across auth, RBAC, org membership, and impersonation

apps/webapp/app/models/member.server.ts

@@ -74,43 +74,6 @@ export async function getTeamMembersAndInvites({
   return { members: org.members, invites: org.invites };
 }
 
-export async function removeTeamMember({
-  userId,
-  slug,
-  memberId,
-}: {
-  userId: string;
-  slug: string;
-  memberId: string;
-}) {
-  const org = await prisma.organization.findFirst({
-    where: { slug, members: { some: { userId } } },
-  });
-
-  if (!org) {
-    throw new Error("User does not have access to this organization");
-  }
-
-  // Scope the target to this org. A member id is a globally unique key, so
-  // deleting by id alone would remove members of other orgs; bind it to the
-  // resolved org and reject a foreign id.
-  const member = await prisma.orgMember.findFirst({
-    where: { id: memberId, organizationId: org.id },
-    include: {
-      organization: true,
-      user: true,
-    },
-  });
-
-  if (!member) {
-    throw new Error("Member not found in this organization");
-  }
-
-  await prisma.orgMember.delete({ where: { id: member.id } });
-
-  return member;
-}
-
 export async function inviteMembers({
   slug,
   emails,

apps/webapp/app/models/removeTeamMember.server.ts

@@ -0,0 +1,41 @@
+import type { PrismaClient } from "@trigger.dev/database";
+
+// Leaf module with a type-only Prisma import (caller passes the client) so it
+// can be unit-tested without importing `~/db.server`, which eagerly connects
+// the global prisma singleton.
+export async function removeTeamMember(
+  {
+    userId,
+    slug,
+    memberId,
+  }: {
+    userId: string;
+    slug: string;
+    memberId: string;
+  },
+  prismaClient: PrismaClient
+) {
+  const org = await prismaClient.organization.findFirst({
+    where: { slug, members: { some: { userId } } },
+  });
+
+  if (!org) {
+    throw new Error("User does not have access to this organization");
+  }
+
+  // Scope both the lookup and the delete to org.id, in a transaction, so the
+  // member id is only ever resolved within the actor's organization.
+  return prismaClient.$transaction(async (tx) => {
+    const target = await tx.orgMember.findFirst({
+      where: { id: memberId, organizationId: org.id },
+      include: { organization: true, user: true },
+    });
+
+    if (!target) {
+      throw new Error("Member not found in this organization");
+    }
+
+    await tx.orgMember.delete({ where: { id: target.id } });
+    return target;
+  });
+}

apps/webapp/app/models/user.server.ts

@@ -7,6 +7,7 @@ import type { DashboardPreferences } from "~/services/dashboardPreferences.serve
 import { getDashboardPreferences } from "~/services/dashboardPreferences.server";
 export type { User } from "@trigger.dev/database";
 import { assertEmailAllowed } from "~/utils/email";
+import { emailMatchesPattern } from "~/utils/emailPattern";
 import { logger } from "~/services/logger.server";
 
 type FindOrCreateMagicLink = {
@@ -74,8 +75,7 @@ export async function findOrCreateMagicLinkUser({
     },
   });
 
-  const adminEmailRegex = env.ADMIN_EMAILS ? new RegExp(env.ADMIN_EMAILS) : undefined;
-  const makeAdmin = adminEmailRegex ? adminEmailRegex.test(email) : false;
+  const makeAdmin = env.ADMIN_EMAILS ? emailMatchesPattern(env.ADMIN_EMAILS, email) : false;
 
   const user = await prisma.user.upsert({
     where: {

apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.ts

@@ -2,8 +2,10 @@ import type { LoaderFunctionArgs } from "@remix-run/server-runtime";
 import { redirect } from "remix-typedjson";
 import { $replica } from "~/db.server";
 import { clearImpersonation, redirectWithImpersonation } from "~/models/admin.server";
+import { env } from "~/env.server";
 import { logger } from "~/services/logger.server";
 import { requireUser } from "~/services/session.server";
+import { isSameOriginNavigation } from "~/utils/sameOriginNavigation";
 
 export async function loader({ request, params }: LoaderFunctionArgs) {
   const user = await requireUser(request);
@@ -29,6 +31,18 @@ export async function loader({ request, params }: LoaderFunctionArgs) {
     return clearImpersonation(request, "/admin");
   }
 
+  // CSRF gate for the SET-impersonation path. Clearing impersonation
+  // above is benign and stays reachable without the check.
+  if (!isSameOriginNavigation(request, env.LOGIN_ORIGIN)) {
+    logger.warn("Refusing cross-site impersonation entry", {
+      userId: user.id,
+      organizationSlug,
+      referer: request.headers.get("referer"),
+      secFetchSite: request.headers.get("sec-fetch-site"),
+    });
+    return redirect("/admin");
+  }
+
   const org = await $replica.organization.findFirst({
     where: {
       slug: organizationSlug,

apps/webapp/app/routes/_app.orgs.$organizationSlug.invite/route.tsx

@@ -36,6 +36,7 @@ import { rbac } from "~/services/rbac.server";
 import { ssoController } from "~/services/sso.server";
 import { dashboardAction, dashboardLoader } from "~/services/routeBuilders/dashboardBuilder";
 import { acceptInvitePath, organizationTeamPath, v3BillingPath } from "~/utils/pathBuilder";
+import { isAtOrBelow } from "~/utils/inviteRoleLadder";
 import { PurchaseSeatsModal } from "../_app.orgs.$organizationSlug.settings.team/route";
 
 const Params = z.object({
@@ -109,43 +110,6 @@ export const loader = dashboardLoader(
 // dropdown is hidden) or as a defensive default.
 const NO_RBAC_ROLE = "__no_rbac_role__";
 
-// An inviter can only assign a role at or below their own. The
-// plugin's systemRoles array is in canonical order (highest authority
-// first), so array index drives the ladder — earlier index = higher
-// rank. Plan-tier filtering happens separately via assignableRoleIds;
-// the ladder is the absolute hierarchy. Custom roles aren't in the
-// ladder yet, so they're refused for now.
-type LadderRole = { id: string };
-
-function buildRoleLevel(roles: ReadonlyArray<LadderRole>): Record<string, number> {
-  const level: Record<string, number> = {};
-  roles.forEach((r, i) => {
-    // Top of the array = highest level. Subtract from length so larger
-    // numbers always mean "more authority" — no off-by-one when a role
-    // is added or removed.
-    level[r.id] = roles.length - i;
-  });
-  return level;
-}
-
-function isAtOrBelow(
-  roles: ReadonlyArray<LadderRole>,
-  inviterRoleId: string | null,
-  invitedRoleId: string
-): boolean {
-  // No resolvable role for the inviter → fail closed: we can't confirm a
-  // target role is at or below an unknown level, so refuse it. The invite
-  // itself still proceeds (it's gated by manage:members); only assigning an
-  // explicit role is refused, and the picker offers nothing in this case.
-  if (!inviterRoleId) return false;
-  const level = buildRoleLevel(roles);
-  const inviter = level[inviterRoleId];
-  const invited = level[invitedRoleId];
-  // Custom roles aren't in the level table — refuse.
-  if (inviter === undefined || invited === undefined) return false;
-  return invited <= inviter;
-}
-
 const schema = z.object({
   emails: z.preprocess((i) => {
     if (typeof i === "string") return [i];

apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.team/route.tsx

@@ -46,11 +46,11 @@ import * as Property from "~/components/primitives/PropertyTable";
 import { Select, SelectItem, SelectLinkItem } from "~/components/primitives/Select";
 import { SpinnerWhite } from "~/components/primitives/Spinner";
 import { SimpleTooltip } from "~/components/primitives/Tooltip";
-import { $replica } from "~/db.server";
+import { $replica, prisma } from "~/db.server";
 import { useShowSelfServe } from "~/hooks/useShowSelfServe";
 import { useOrganization } from "~/hooks/useOrganizations";
 import { useUser } from "~/hooks/useUser";
-import { removeTeamMember } from "~/models/member.server";
+import { removeTeamMember } from "~/models/removeTeamMember.server";
 import { redirectWithSuccessMessage } from "~/models/message.server";
 import { resolveOrgIdFromSlug } from "~/models/organization.server";
 import { TeamPresenter } from "~/presenters/TeamPresenter.server";
@@ -262,12 +262,9 @@ export const action = dashboardAction(
       return json(submission.reply());
     }
 
-    // Default intent: remove a member or leave the org. Scope the target to
-    // the actor's organization: an orgMember id is a globally unique key, so an
-    // unscoped lookup (plus an unscoped delete in the model) would let a
-    // manager in one org remove members of another by submitting a foreign id.
-    // Self-leave is always allowed; removing someone else requires
-    // manage:members.
+    // Default intent: remove a member or leave the org, with the target scoped
+    // to the actor's organization. Self-leave is always allowed; removing
+    // someone else requires manage:members.
     const orgId = context.organizationId;
     if (!orgId) {
       return json({ ok: false, error: "Organization not found" } as const, { status: 404 });
@@ -295,11 +292,14 @@ export const action = dashboardAction(
     }
 
     try {
-      const deletedMember = await removeTeamMember({
-        userId,
-        memberId: submission.value.memberId,
-        slug: organizationSlug,
-      });
+      const deletedMember = await removeTeamMember(
+        {
+          userId,
+          memberId: submission.value.memberId,
+          slug: organizationSlug,
+        },
+        prisma
+      );
 
       // Sticky removal: record a tombstone so passive SSO-JIT won't re-add
       // them on next login (best-effort; no-op without the SSO plugin).

apps/webapp/app/routes/login.mfa/route.tsx

@@ -25,7 +25,7 @@ import {
   redirectWithErrorMessage,
 } from "~/models/message.server";
 import { authenticator } from "~/services/auth.server";
-import { checkMfaRateLimit, MfaRateLimitError } from "~/services/mfa/mfaRateLimiter.server";
+import { checkMfaRateLimit, MfaRateLimitError } from "~/services/mfa/mfaRateLimiterGlobal.server";
 import { MultiFactorAuthenticationService } from "~/services/mfa/multiFactorAuthentication.server";
 import { trackAndClearReferralSource } from "~/services/referralSource.server";
 import { commitAuthenticatedSession } from "~/services/sessionDuration.server";

apps/webapp/app/services/googleAuth.server.ts

@@ -3,6 +3,7 @@ import { GoogleStrategy } from "remix-auth-google";
 import { env } from "~/env.server";
 import { findOrCreateUser } from "~/models/user.server";
 import type { AuthUser } from "./authUser";
+import { isGoogleEmailVerified } from "./googleEmailVerification";
 import { logger } from "./logger.server";
 import { postAuthentication } from "./postAuth.server";
 import { SsoRequiredError, ssoRedirectForEmail } from "./ssoAutoDiscovery.server";
@@ -27,6 +28,14 @@ export function addGoogleStrategy(
 
       const email = emails[0].value;
 
+      // Only trust the email if Google asserts it's verified, since account
+      // linking keys off it. See isGoogleEmailVerified.
+      if (!isGoogleEmailVerified(profile)) {
+        throw new Error(
+          "Google login refused: the Google account's email is not verified. Sign in with an account whose email Google has verified, or use magic-link / GitHub."
+        );
+      }
+
       // SSO auto-discovery gate — BEFORE findOrCreateUser, so an
       // SSO-enforced domain never gets this Google identity linked onto
       // an existing account.

apps/webapp/app/services/googleEmailVerification.ts

@@ -0,0 +1,15 @@
+import type { GoogleProfile } from "remix-auth-google";
+
+/**
+ * Whether Google has asserted that the profile's email is verified. A
+ * successful OAuth flow proves control of the Google account, not ownership of
+ * the email it carries, and account linking keys off the email.
+ *
+ * Strict by design: only a real boolean `true` counts. A missing claim, missing
+ * `_json`, the string `"true"`, or a truthy `1` are all treated as unverified.
+ */
+export function isGoogleEmailVerified(profile: GoogleProfile): boolean {
+  const emailVerified = (profile as { _json?: { email_verified?: unknown } })?._json
+    ?.email_verified;
+  return emailVerified === true;
+}