fix(auth): close three account-security holes (0.20.5)

.changeset/device-2fa-survives-relogin.md

@@ -0,0 +1,42 @@
+---
+'@factiii/auth': patch
+---
+
+Stop a revoked session's 2FA secret from answering the login challenge, and
+carry the live one onto the session that replaces it
+
+In device mode the TOTP secret lives on `Session.twoFaSecret`, but it belongs to
+the phone rather than to any one session of it. `findTwoFaSecretsByUserId` did
+not filter revoked rows, so a secret from a device the user had deliberately
+revoked — an old phone, a sold one, "log out everywhere" after a compromise —
+still passed the 2FA challenge. Revoking a device did not revoke its second
+factor. Present in every 0.x.
+
+The filter alone would have turned that leak into a lockout, because nothing
+carried the secret across a session replacement: `revokeDeviceSessionsForUser`
+retires the session this device already held on every ordinary sign-in, so
+filtering revoked rows would leave a re-logged-in user with no live secret at
+all, and an account with no email on file with no way back in. So the two halves
+are one change. `carryDeviceTwoFaSecret` moves the secret onto the replacement
+in the password, OAuth and magic-link paths, and only then does the adapter
+query — both the Prisma and Drizzle implementations — filter `revokedAt`.
+
+It **moves** the secret rather than copying it, which the schema requires:
+`Session.twoFaSecret` is `@unique`, so the donor and the recipient cannot hold
+the same string even for the length of a transaction. `DeviceAuthAdapter` gains
+an optional `moveTwoFaSecret`, implemented atomically by both shipped adapters
+(clear the donor, then write the recipient, in one transaction) so a crash cannot
+leave the secret on neither row. It is optional, not required, so an adapter
+written against an earlier version still satisfies the interface; without it the
+carry falls back to a clear-then-set pair, which is not atomic but fails in the
+recoverable direction.
+
+The carry is best-effort and never throws: losing a cached second factor is
+recoverable, since the vault can mint a new one, while failing here would reject
+a sign-in whose credentials have already been accepted. It does report a failure
+rather than swallowing it. It is a no-op in standard mode, where the secret is on
+the user row and no session replacement can touch it.
+
+This also fixes an existing failure: a consumer that already filtered revoked
+rows on its own approval path found no secret at all after a re-login, so
+push approvals failed until the vault happened to re-materialize one.

.changeset/oauth-attach-trusts-proven-email.md

@@ -0,0 +1,28 @@
+---
+'@factiii/auth': patch
+---
+
+Only attach an OAuth sign-in to an existing account by an email that was proven
+
+`oAuthLogin` attaches a new Google or Apple identity to an existing passwordless
+account whose email matches the provider's. Two things let that attach land on
+the wrong account.
+
+The email could come from the client. When Apple's signed token carried no
+email claim, the verifier fell back to `user.email` from the request, so anyone
+holding a valid Apple token for their own Apple ID could name another person's
+address and be signed into that person's passwordless account. The Google branch
+trusted `payload.email` without checking `email_verified`. The verifier now takes
+the email only from the signed token, and from Google only when it is marked
+verified. A token with no trusted email still verifies — an identity already
+linked by its subject keeps signing in — but it can no longer attach or create.
+`OAuthResult.email` is now optional to say so.
+
+And the account's own email did not have to be proven. A consumer that lets a
+user store an unclaimed address unverified is open to pre-hijacking: register a
+passwordless account under a victim's address, and the victim's first genuine
+sign-in lands in an account the registrant still controls. No token is forged
+for that one. Attach-by-email now requires the matching account's
+`emailVerificationStatus` to be `VERIFIED`, and refuses otherwise; the user can
+still link the provider from a signed-in session. An adapter that does not return
+the field refuses every attach rather than allowing any.

packages/auth/src/adapters/deviceAuth.ts

@@ -37,6 +37,24 @@ export interface DeviceAuthAdapter {
     clearTwoFaSecrets(userId: number, excludeSessionId?: number): Promise<void>;
     /** Set the `twoFaSecret` on a single session. */
     setTwoFaSecret(sessionId: number, secret: string | null): Promise<void>;
+    /**
+     * Move a session's `twoFaSecret` to another session of the same user, in one
+     * atomic step. No-op when `fromSessionId` holds none.
+     *
+     * This exists because the secret cannot be COPIED: the reference schema
+     * declares `Session.twoFaSecret` as `@unique`, so two rows may not hold the
+     * same string for an instant, let alone a transaction. The donor must give it
+     * up in the same breath the recipient takes it, and a crash in between must
+     * not leave it on neither row.
+     *
+     * Optional so that adding it does not break an adapter written against an
+     * earlier version. `carryDeviceTwoFaSecret` falls back to a clear-then-set
+     * pair when it is absent — correct, but not atomic, and a crash between the
+     * two writes costs the device its cached second factor (recoverable: the
+     * vault mints a new one). Implement it if your database can do better, which
+     * both shipped adapters do.
+     */
+    moveTwoFaSecret?(userId: number, fromSessionId: number, toSessionId: number): Promise<void>;
     /** Find a session with its (optional) device join, scoped to a user. */
     findByIdWithDevice(id: number, userId: number): Promise<SessionWithDevice | null>;
     /** Read just the deviceId from a session, scoped to a user. */

packages/auth/src/adapters/drizzleAdapter.ts

@@ -425,11 +425,20 @@ export function createDrizzleDeviceAdapter(
 
   return {
     session: {
+      // The revoked filter is load-bearing — see the prisma twin for why a
+      // revoked device's secret must stop answering the login challenge, and why
+      // this is only safe alongside `carryDeviceTwoFaSecret`.
       async findTwoFaSecretsByUserId(userId: number): Promise<{ twoFaSecret: string | null }[]> {
         const secretRows = await db
           .select({ twoFaSecret: sessions.twoFaSecret })
           .from(sessions)
-          .where(and(eq(sessions.userId, userId), sql`${sessions.twoFaSecret} is not null`));
+          .where(
+            and(
+              eq(sessions.userId, userId),
+              sql`${sessions.twoFaSecret} is not null`,
+              isNull(sessions.revokedAt)
+            )
+          );
         return secretRows as { twoFaSecret: string | null }[];
       },
 
@@ -449,6 +458,32 @@ export function createDrizzleDeviceAdapter(
         await db.update(sessions).set({ twoFaSecret: secret }).where(eq(sessions.id, sessionId));
       },
 
+      // See the prisma twin for why this exists and why the clear must precede
+      // the write: `twoFaSecret` is unique, so the secret is moved, never copied.
+      async moveTwoFaSecret(
+        userId: number,
+        fromSessionId: number,
+        toSessionId: number
+      ): Promise<void> {
+        await db.transaction(async (tx) => {
+          const rows = await tx
+            .select({ twoFaSecret: sessions.twoFaSecret })
+            .from(sessions)
+            .where(and(eq(sessions.id, fromSessionId), eq(sessions.userId, userId)));
+          const secret = rows[0]?.twoFaSecret;
+          if (!secret) return;
+
+          await tx
+            .update(sessions)
+            .set({ twoFaSecret: null })
+            .where(and(eq(sessions.id, fromSessionId), eq(sessions.userId, userId)));
+          await tx
+            .update(sessions)
+            .set({ twoFaSecret: secret })
+            .where(and(eq(sessions.id, toSessionId), eq(sessions.userId, userId)));
+        });
+      },
+
       async findByIdWithDevice(id: number, userId: number): Promise<SessionWithDevice | null> {
         const rows = await db
           .select({

packages/auth/src/adapters/prismaAdapter.ts

@@ -347,9 +347,19 @@ export function createPrismaDeviceAdapter(prisma: unknown): DeviceAuthAdapter {
   const db = prisma as PrismaModelAccess;
   return {
     session: {
+      // `revokedAt: null` is load-bearing, not tidiness. Revoking a session sets
+      // the column and leaves the row, so without this filter the TOTP secret of
+      // a device the user deliberately revoked — an old phone, a sold one, "log
+      // out everywhere" after a compromise — still answers the login challenge.
+      // Revoking a device has to revoke its second factor with it.
+      //
+      // Safe only because `carryDeviceTwoFaSecret` moves the secret onto the
+      // replacement session on every sign-in (utilities/issueCookies.ts). Without
+      // that, this filter turns the leak into a lockout: an ordinary re-login
+      // would leave the account with no live secret at all.
       async findTwoFaSecretsByUserId(userId: number): Promise<{ twoFaSecret: string | null }[]> {
         return db.session.findMany({
-          where: { userId, twoFaSecret: { not: null } },
+          where: { userId, twoFaSecret: { not: null }, revokedAt: null },
           select: { twoFaSecret: true },
         }) as Promise<{ twoFaSecret: string | null }[]>;
       },
@@ -371,6 +381,43 @@ export function createPrismaDeviceAdapter(prisma: unknown): DeviceAuthAdapter {
         });
       },
 
+      async moveTwoFaSecret(
+        userId: number,
+        fromSessionId: number,
+        toSessionId: number
+      ): Promise<void> {
+        const apply = async (client: PrismaModelAccess) => {
+          const from = (await client.session.findUnique({
+            where: { id: fromSessionId, userId },
+            select: { twoFaSecret: true },
+          })) as { twoFaSecret: string | null } | null;
+          if (!from?.twoFaSecret) return;
+
+          // Clear BEFORE writing, even inside the transaction. `twoFaSecret` is
+          // `@unique` and Prisma does not declare the constraint DEFERRABLE, so
+          // Postgres checks it per statement: the donor has to be empty before
+          // the recipient can hold the string. Writing first fails outright, and
+          // copying — which is what this method replaced — fails the same way.
+          await client.session.updateMany({
+            where: { id: fromSessionId, userId },
+            data: { twoFaSecret: null },
+          });
+          await client.session.updateMany({
+            where: { id: toSessionId, userId },
+            data: { twoFaSecret: from.twoFaSecret },
+          });
+        };
+
+        // Both writes or neither, so a crash cannot leave the secret on no row
+        // at all. `$transaction` is optional on the client shape this adapter
+        // accepts; without it the pair still runs in the fail-safe order.
+        if (db.$transaction) {
+          await db.$transaction((tx) => apply(tx as PrismaModelAccess));
+          return;
+        }
+        await apply(db);
+      },
+
       async findByIdWithDevice(id: number, userId: number): Promise<SessionWithDevice | null> {
         const session = await db.session.findUnique({
           where: { id, userId },

packages/auth/src/procedures/base.ts

@@ -7,7 +7,11 @@ import { detectBrowser } from '../utilities/browser';
 import { isTwoFaEnabled, verifyTwoFaChallenge } from './twoFa/verifyChallenge';
 import type { ResolvedAuthConfig } from '../utilities/config';
 import { clearAuthCookies, setAuthCookies } from '../utilities/cookies';
-import { issueAuthCookies, revokeDeviceSessionsForUser } from '../utilities/issueCookies';
+import {
+  carryDeviceTwoFaSecret,
+  issueAuthCookies,
+  revokeDeviceSessionsForUser,
+} from '../utilities/issueCookies';
 import { createAuthToken } from '../utilities/jwt';
 import { comparePassword, hashPassword } from '../utilities/password';
 import type { UsernameMode } from '../types/config';
@@ -266,7 +270,11 @@ export class BaseProcedureFactory<
       // Credentials and 2FA have both passed by here, so a session this device
       // already holds for the same account is stale, not a reason to refuse.
       // Retire it and issue a fresh one.
-      await revokeDeviceSessionsForUser(this.config, ctx.headers.cookie, user.id);
+      const replacedSessionIds = await revokeDeviceSessionsForUser(
+        this.config,
+        ctx.headers.cookie,
+        user.id
+      );
 
       const extraSessionData = this.config.hooks?.getSessionData
         ? await this.config.hooks.getSessionData(typedInput)
@@ -279,6 +287,14 @@ export class BaseProcedureFactory<
         ...extraSessionData,
       });
 
+      // The device's second factor rides on the session, so it has to move to
+      // the replacement or the sign-in quietly costs the user their 2FA.
+      await carryDeviceTwoFaSecret(this.config, {
+        userId: user.id,
+        revokedSessionIds: replacedSessionIds,
+        newSessionId: session.id,
+      });
+
       if (this.config.hooks?.onUserLogin) {
         await this.config.hooks.onUserLogin(user.id, session.id);
       }

packages/auth/src/procedures/magicLink.ts

@@ -3,7 +3,7 @@ import { z } from 'zod';
 
 import { type BaseProcedure } from '../types/trpc';
 import type { ResolvedAuthConfig } from '../utilities/config';
-import { revokeDeviceSessionsForUser } from '../utilities/issueCookies';
+import { carryDeviceTwoFaSecret, revokeDeviceSessionsForUser } from '../utilities/issueCookies';
 import { createSessionWithTokenAndCookie } from '../utilities/session';
 
 /** Factory for magic link authentication procedures. */
@@ -56,7 +56,11 @@ export class MagicLinkProcedureFactory {
 
         // The link proves control of the address, so a session this device
         // already holds for the same account is stale, not a reason to refuse.
-        await revokeDeviceSessionsForUser(this.config, ctx.headers.cookie, magicLink.userId);
+        const replacedSessionIds = await revokeDeviceSessionsForUser(
+          this.config,
+          ctx.headers.cookie,
+          magicLink.userId
+        );
 
         // Mark as used (single-use)
         await db.markUsed(magicLink.id);
@@ -68,7 +72,7 @@ export class MagicLinkProcedureFactory {
           ? await this.config.hooks.onBeforeMagicLinkSession(magicLink.userId)
           : {};
 
-        await createSessionWithTokenAndCookie(
+        const { sessionId } = await createSessionWithTokenAndCookie(
           this.config,
           {
             userId: magicLink.userId,
@@ -79,6 +83,14 @@ export class MagicLinkProcedureFactory {
           ctx.res
         );
 
+        // Same rule as the other sign-in paths: the device keeps the second
+        // factor it already had. A magic link is not a reason to lose it.
+        await carryDeviceTwoFaSecret(this.config, {
+          userId: magicLink.userId,
+          revokedSessionIds: replacedSessionIds,
+          newSessionId: sessionId,
+        });
+
         return { success: true };
       });
   }

packages/auth/src/procedures/oauth.ts

@@ -6,7 +6,11 @@ import type { SchemaExtensions } from '../types/hooks';
 import { type AuthProcedure, type BaseProcedure } from '../types/trpc';
 import { detectBrowser } from '../utilities';
 import type { ResolvedAuthConfig } from '../utilities/config';
-import { issueAuthCookies, revokeDeviceSessionsForUser } from '../utilities/issueCookies';
+import {
+  carryDeviceTwoFaSecret,
+  issueAuthCookies,
+  revokeDeviceSessionsForUser,
+} from '../utilities/issueCookies';
 import { assertKeepsLoginMethod } from '../utilities/loginMethods';
 import { createOAuthVerifier, type OAuthProvider, type OAuthResult } from '../utilities/oauth';
 import { type CreatedSchemas, type OAuthSchemaInput } from '../validators';
@@ -104,6 +108,19 @@ export class OAuthLoginProcedureFactory<
           });
         }
 
+        // Attaching by email is only safe when the address was PROVEN on the
+        // account being attached to. Consumers can let a user store any unclaimed
+        // address unverified, so an unverified match may be an account someone
+        // else registered in the victim's name — and signing the victim into it
+        // hands them an account the registrant still controls (pre-hijacking).
+        // An adapter that omits the field refuses every attach: fail-closed.
+        if (existing && existing.emailVerificationStatus !== 'VERIFIED') {
+          throw new TRPCError({
+            code: 'BAD_REQUEST',
+            message: 'Sign in another way, then link this provider from Settings.',
+          });
+        }
+
         let created = false;
         if (existing) {
           user = existing;
@@ -145,7 +162,11 @@ export class OAuthLoginProcedureFactory<
 
       // The provider has vouched for this identity, so a session this device
       // already holds for the same account is stale, not a reason to refuse.
-      await revokeDeviceSessionsForUser(this.config, ctx.headers.cookie, user.id);
+      const replacedSessionIds = await revokeDeviceSessionsForUser(
+        this.config,
+        ctx.headers.cookie,
+        user.id
+      );
 
       const extraSessionData = this.config.hooks?.getSessionData
         ? await this.config.hooks.getSessionData(typedInput)
@@ -158,6 +179,14 @@ export class OAuthLoginProcedureFactory<
         ...extraSessionData,
       });
 
+      // An OAuth account can still have device 2FA enrolled from a password it
+      // used to have, so this path carries the secret like any other.
+      await carryDeviceTwoFaSecret(this.config, {
+        userId: user.id,
+        revokedSessionIds: replacedSessionIds,
+        newSessionId: session.id,
+      });
+
       if (this.config.hooks?.onUserLogin) {
         await this.config.hooks.onUserLogin(user.id, session.id);
       }