fix(gdpr): stop exporting MFA credentials and gate exports on role authority (#706)

The subject-access export selected the whole users row and subtracted only
passwordHash, so profile.json carried totpSecret and recoveryCodesHash. On a
default install DATA_ENCRYPTION_KEY is empty and the TOTP seed is stored as
cleartext base32; recovery codes are 32-bit values behind an unsalted SHA-256.

Name the profile columns instead, add the canManageTargetRole gate the sibling
purge routes already apply, and scope the export status lookup to the user in
the path plus the gdpr-export tool id.

apps/api/src/jobs/gdpr-export.ts

@@ -14,11 +14,34 @@ import { readStoredFile } from "../lib/file-storage.js";
 import { putObject } from "../lib/object-storage.js";
 
 export async function gdprExportJob(userId: string, jobId: string): Promise<{ outputRef: string }> {
-  // 1. Fetch user profile (exclude passwordHash)
-  const [user] = await db.select().from(schema.users).where(eq(schema.users.id, userId));
-  if (!user) throw new Error(`User ${userId} not found`);
-
-  const { passwordHash: _, ...profile } = user;
+  // 1. Fetch user profile.
+  //
+  // The columns are named rather than selected with `*` so that authentication
+  // material never leaves Postgres. passwordHash, totpSecret and recoveryCodesHash
+  // are credentials, not personal data a subject-access request is owed, and this
+  // archive is meant to be handed to the data subject or a regulator. Listing the
+  // allowlist here also means a future column on `users` is excluded by default
+  // instead of silently joining the export.
+  const [profile] = await db
+    .select({
+      id: schema.users.id,
+      username: schema.users.username,
+      role: schema.users.role,
+      team: schema.users.team,
+      email: schema.users.email,
+      authProvider: schema.users.authProvider,
+      externalId: schema.users.externalId,
+      mustChangePassword: schema.users.mustChangePassword,
+      legalHold: schema.users.legalHold,
+      storageUsed: schema.users.storageUsed,
+      storageQuota: schema.users.storageQuota,
+      totpEnabled: schema.users.totpEnabled,
+      createdAt: schema.users.createdAt,
+      updatedAt: schema.users.updatedAt,
+    })
+    .from(schema.users)
+    .where(eq(schema.users.id, userId));
+  if (!profile) throw new Error(`User ${userId} not found`);
 
   // 2. Fetch all user's files metadata
   const files = await db.select().from(schema.userFiles).where(eq(schema.userFiles.userId, userId));

apps/api/src/routes/enterprise/gdpr.ts

@@ -121,13 +121,23 @@ export async function registerGdprRoutes(app: FastifyInstance): Promise<void> {
 
       // Validate the target user exists
       const [targetUser] = await db
-        .select({ id: schema.users.id })
+        .select({ id: schema.users.id, role: schema.users.role })
         .from(schema.users)
         .where(eq(schema.users.id, targetUserId));
       if (!targetUser) {
         return reply.status(404).send({ error: "User not found" });
       }
 
+      // Same authority gate the purge routes apply. An export archive carries the
+      // target's whole library and profile, so exporting an account above your own
+      // role is a disclosure path, not a read-only status call.
+      if (!(await canManageTargetRole(user, targetUser.role))) {
+        return reply.status(403).send({
+          error: "Cannot manage a user beyond your role authority",
+          code: "ESCALATION_DENIED",
+        });
+      }
+
       // Create a durable job row
       const jobId = randomUUID();
       await db.insert(schema.jobs).values({
@@ -178,9 +188,20 @@ export async function registerGdprRoutes(app: FastifyInstance): Promise<void> {
         });
       }
 
-      const { jobId } = request.params;
+      const { id: targetUserId, jobId } = request.params;
 
-      const [job] = await db.select().from(schema.jobs).where(eq(schema.jobs.id, jobId));
+      // Scoped to the user named in the path and to the export job type, so this
+      // cannot be used to resolve an arbitrary job id into a download URL.
+      const [job] = await db
+        .select()
+        .from(schema.jobs)
+        .where(
+          and(
+            eq(schema.jobs.id, jobId),
+            eq(schema.jobs.userId, targetUserId),
+            eq(schema.jobs.toolId, "gdpr-export"),
+          ),
+        );
 
       if (!job) {
         return reply.status(404).send({ error: "Export job not found" });

tests/unit/api/gdpr-route-authority.test.ts

@@ -0,0 +1,189 @@
+/**
+ * Authority checks on the GDPR export routes.
+ *
+ * The purge routes have always gated on `canManageTargetRole`, so a compliance
+ * operator cannot act on an account above their own authority. Export needs the
+ * same gate: the archive carries the target's entire library plus their profile,
+ * so exporting an administrator is a data-disclosure path, not a read-only status
+ * call. These run against a bare Fastify instance because the real routes sit
+ * behind an enterprise licence that CI has no signing key to mint.
+ */
+import Fastify, { type FastifyInstance } from "fastify";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+
+const requirePermissionMock = vi.hoisted(() => vi.fn());
+const canManageTargetRoleMock = vi.hoisted(() => vi.fn());
+const selectMock = vi.hoisted(() => vi.fn());
+const insertMock = vi.hoisted(() => vi.fn());
+const queueAddMock = vi.hoisted(() => vi.fn());
+const auditMock = vi.hoisted(() => vi.fn());
+
+/** Rows handed back by the next `select(...).from(...).where(...)` chain. */
+let selectResults: unknown[][] = [];
+/** Predicates passed to each `.where(...)`, in call order, for assertions. */
+let whereArgs: unknown[] = [];
+
+function nextChain() {
+  const rows = selectResults.shift() ?? [];
+  const chain = {
+    from: vi.fn(() => chain),
+    where: vi.fn((predicate: unknown) => {
+      whereArgs.push(predicate);
+      return Promise.resolve(rows);
+    }),
+  };
+  return chain;
+}
+
+/** Flatten the nested `and(eq(...), eq(...))` shape into its leaf comparisons. */
+function eqLeaves(predicate: unknown): Array<{ col: unknown; val: unknown }> {
+  const node = predicate as { op?: string; conds?: unknown[]; col?: unknown; val?: unknown };
+  if (node?.op === "and") return (node.conds ?? []).flatMap(eqLeaves);
+  if (node?.op === "eq") return [{ col: node.col, val: node.val }];
+  return [];
+}
+
+async function buildApp(): Promise<FastifyInstance> {
+  vi.resetModules();
+  selectMock.mockReset();
+  insertMock.mockReset();
+  queueAddMock.mockReset();
+  auditMock.mockReset();
+  requirePermissionMock.mockReset();
+  canManageTargetRoleMock.mockReset();
+
+  selectMock.mockImplementation(() => nextChain());
+  insertMock.mockImplementation(() => ({ values: vi.fn(() => Promise.resolve()) }));
+  auditMock.mockImplementation(() => vi.fn(() => Promise.resolve()));
+  // Set after the resets above, otherwise buildApp() would wipe it and the route
+  // would bail at `if (!user) return`, answering 200 with an empty body.
+  requirePermissionMock.mockResolvedValue(COMPLIANCE_OPERATOR);
+
+  vi.doMock("@snapotter/enterprise", () => ({
+    isFeatureEnabled: () => true,
+  }));
+
+  // Plain objects instead of SQL nodes so a test can read back which columns the
+  // query actually constrained.
+  vi.doMock("drizzle-orm", () => ({
+    eq: (col: unknown, val: unknown) => ({ op: "eq", col, val }),
+    and: (...conds: unknown[]) => ({ op: "and", conds }),
+    inArray: (col: unknown, vals: unknown) => ({ op: "inArray", col, vals }),
+  }));
+
+  vi.doMock("../../../apps/api/src/db/index.js", () => ({
+    db: { select: selectMock, insert: insertMock, delete: vi.fn(), update: vi.fn() },
+    schema: {
+      users: { id: "users.id", role: "users.role" },
+      jobs: { id: "jobs.id", userId: "jobs.user_id", toolId: "jobs.tool_id" },
+    },
+  }));
+
+  vi.doMock("../../../apps/api/src/permissions.js", () => ({
+    requirePermission: () => requirePermissionMock,
+    canManageTargetRole: canManageTargetRoleMock,
+  }));
+
+  vi.doMock("../../../apps/api/src/jobs/queues.js", () => ({
+    getQueue: () => ({ add: queueAddMock }),
+  }));
+  vi.doMock("../../../apps/api/src/jobs/system-jobs.js", () => ({
+    SYSTEM_JOBS: { gdprExport: "gdpr-export" },
+  }));
+  vi.doMock("../../../apps/api/src/jobs/cancel.js", () => ({ requestCancel: vi.fn() }));
+  vi.doMock("../../../apps/api/src/lib/audit.js", () => ({ auditFromRequest: auditMock }));
+  vi.doMock("../../../apps/api/src/lib/file-storage.js", () => ({
+    deleteStoredFile: vi.fn(),
+    deleteThumbnail: vi.fn(),
+  }));
+  vi.doMock("../../../apps/api/src/lib/object-storage.js", () => ({ deletePrefix: vi.fn() }));
+
+  const { registerGdprRoutes } = await import("../../../apps/api/src/routes/enterprise/gdpr.js");
+  const app = Fastify();
+  await registerGdprRoutes(app);
+  await app.ready();
+  return app;
+}
+
+const COMPLIANCE_OPERATOR = { id: "operator-1", username: "compliance", role: "compliance" };
+
+describe("GDPR export route authority", () => {
+  let app: FastifyInstance;
+
+  beforeEach(() => {
+    selectResults = [];
+    whereArgs = [];
+  });
+
+  afterEach(async () => {
+    await app?.close();
+    vi.restoreAllMocks();
+  });
+
+  it("refuses to export a user whose role outranks the caller", async () => {
+    app = await buildApp();
+    selectResults = [[{ id: "admin-1", role: "admin" }]];
+    canManageTargetRoleMock.mockResolvedValue(false);
+
+    const res = await app.inject({
+      method: "POST",
+      url: "/api/v1/enterprise/users/admin-1/export",
+    });
+
+    expect(res.statusCode).toBe(403);
+    expect(res.json()).toMatchObject({ code: "ESCALATION_DENIED" });
+    // The job must never be enqueued: the archive is the disclosure.
+    expect(queueAddMock).not.toHaveBeenCalled();
+    expect(insertMock).not.toHaveBeenCalled();
+  });
+
+  it("exports a user within the caller's authority", async () => {
+    app = await buildApp();
+    selectResults = [[{ id: "user-9", role: "user" }]];
+    canManageTargetRoleMock.mockResolvedValue(true);
+
+    const res = await app.inject({
+      method: "POST",
+      url: "/api/v1/enterprise/users/user-9/export",
+    });
+
+    expect(res.statusCode).toBe(202);
+    expect(queueAddMock).toHaveBeenCalledTimes(1);
+  });
+
+  it("still 404s an unknown target before consulting role authority", async () => {
+    app = await buildApp();
+    selectResults = [[]];
+
+    const res = await app.inject({
+      method: "POST",
+      url: "/api/v1/enterprise/users/ghost/export",
+    });
+
+    expect(res.statusCode).toBe(404);
+    expect(canManageTargetRoleMock).not.toHaveBeenCalled();
+  });
+
+  it("scopes the export status lookup to the named user and the export job type", async () => {
+    app = await buildApp();
+    // A completed job row exists. Without a scoped predicate the route would hand
+    // back its download URL for any job id in the system, whoever owns it.
+    selectResults = [
+      [{ id: "job-1", status: "completed", outputRefs: ["outputs/job-1/gdpr-export.zip"] }],
+    ];
+
+    await app.inject({
+      method: "GET",
+      url: "/api/v1/enterprise/users/victim-1/export/job-1",
+    });
+
+    const leaves = eqLeaves(whereArgs.at(-1));
+    expect(leaves).toEqual(
+      expect.arrayContaining([
+        { col: "jobs.id", val: "job-1" },
+        { col: "jobs.user_id", val: "victim-1" },
+        { col: "jobs.tool_id", val: "gdpr-export" },
+      ]),
+    );
+  });
+});

tests/unit/api/jobs/gdpr-export.behavior.test.ts

@@ -5,10 +5,27 @@ const selectMock = vi.hoisted(() => vi.fn());
 const readStoredFileMock = vi.hoisted(() => vi.fn());
 const putObjectMock = vi.hoisted(() => vi.fn());
 
+/**
+ * Stands in for a Drizzle query builder, including its projection behavior: when
+ * `select()` is given a column map, only those keys come back. Without that, a
+ * mock would hand every column to the caller regardless of what was selected, and
+ * assertions like "the export omits passwordHash" would pass even if the query
+ * asked for it.
+ */
 function queryChain<T>(result: T) {
   const chain = {
     from: vi.fn(() => chain),
-    where: vi.fn(() => Promise.resolve(result)),
+    where: vi.fn(() => {
+      const callIndex = selectMock.mock.results.findIndex((r) => r.value === chain);
+      const columns = callIndex >= 0 ? selectMock.mock.calls[callIndex]?.[0] : undefined;
+      if (!columns || !Array.isArray(result)) return Promise.resolve(result);
+      const keys = Object.keys(columns as Record<string, unknown>);
+      return Promise.resolve(
+        (result as Record<string, unknown>[]).map((row) =>
+          Object.fromEntries(keys.map((k) => [k, row[k]])),
+        ),
+      );
+    }),
   };
   return chain;
 }
@@ -28,7 +45,26 @@ async function loadGdprExport() {
       select: selectMock,
     },
     schema: {
-      users: { id: "users.id" },
+      users: {
+        id: "users.id",
+        username: "users.username",
+        role: "users.role",
+        team: "users.team",
+        email: "users.email",
+        authProvider: "users.auth_provider",
+        externalId: "users.external_id",
+        mustChangePassword: "users.must_change_password",
+        legalHold: "users.legal_hold",
+        storageUsed: "users.storage_used",
+        storageQuota: "users.storage_quota",
+        totpEnabled: "users.totp_enabled",
+        createdAt: "users.created_at",
+        updatedAt: "users.updated_at",
+        // Credential material -- present on the table, never exportable.
+        passwordHash: "users.password_hash",
+        totpSecret: "users.totp_secret",
+        recoveryCodesHash: "users.recovery_codes_hash",
+      },
       userFiles: { userId: "userFiles.userId" },
       jobs: { userId: "jobs.userId" },
       auditLog: { actorId: "auditLog.actorId" },
@@ -51,6 +87,33 @@ describe("GDPR export job behavior", () => {
     vi.restoreAllMocks();
   });
 
+  // A subject-access export is handed to the data subject, a regulator, or outside
+  // counsel, so it must never carry authentication material. Naming the columns keeps
+  // totpSecret and recoveryCodesHash inside Postgres instead of relying on the caller
+  // to subtract them, which also means a future column addition cannot silently leak.
+  it("selects an explicit profile column list that omits every credential column", async () => {
+    const { gdprExportJob } = await loadGdprExport();
+    selectMock
+      .mockReturnValueOnce(queryChain([{ id: "user-1", email: "ada@example.test" }]))
+      .mockReturnValueOnce(queryChain([]))
+      .mockReturnValueOnce(queryChain([]))
+      .mockReturnValueOnce(queryChain([]));
+
+    await gdprExportJob("user-1", "export-columns");
+
+    const profileColumns = selectMock.mock.calls[0][0];
+    expect(profileColumns, "profile select must name its columns rather than select *").toBeTypeOf(
+      "object",
+    );
+
+    const selected = Object.keys(profileColumns as Record<string, unknown>);
+    expect(selected).not.toContain("passwordHash");
+    expect(selected).not.toContain("totpSecret");
+    expect(selected).not.toContain("recoveryCodesHash");
+    // Personal data the subject is genuinely owed still ships.
+    expect(selected).toEqual(expect.arrayContaining(["id", "username", "email", "createdAt"]));
+  });
+
   it("throws before writing output when the user does not exist", async () => {
     const { gdprExportJob } = await loadGdprExport();
     selectMock.mockReturnValueOnce(queryChain([]));