diff --git a/apps/auth/src/lib/server/auth/__tests__/account-scope.test.ts b/apps/auth/src/lib/server/auth/__tests__/account-scope.test.ts index 0db56f94bd..f0e151e7ea 100644 --- a/apps/auth/src/lib/server/auth/__tests__/account-scope.test.ts +++ b/apps/auth/src/lib/server/auth/__tests__/account-scope.test.ts @@ -160,3 +160,15 @@ describe('findOrCreateUser — canonical email only stored when verified', () => expect(h.userInsert?.emailVerified).toBeInstanceOf(Date); }); }); + +describe('findOrCreateUser — the provider name is never persisted', () => { + it('writes name as null even when the provider supplies one', async () => { + // Deliberate (GDPR): an unverified, user-controlled value that outlives a soft-deleted + // account. Staff accounts created after this file NCMEC reports with no reporter firstName. + // Asserting null rather than absent: an omitted key falls back to the column default, + // which is a separate decision this test should not silently depend on. + await findOrCreateUser('discord', profile({ name: 'Mod' }), DISCORD_SCOPE); + expect(h.userCreated).toBe(true); + expect(h.userInsert).toHaveProperty('name', null); + }); +}); diff --git a/apps/auth/src/lib/server/auth/users.ts b/apps/auth/src/lib/server/auth/users.ts index ff8f7430db..d2ae94f4e8 100644 --- a/apps/auth/src/lib/server/auth/users.ts +++ b/apps/auth/src/lib/server/auth/users.ts @@ -138,7 +138,11 @@ export async function findOrCreateUser( // victim's real login into this account (takeover). Mirrors the emailVerified gate just below. email: profile.email && profile.emailVerified ? profile.email : null, username: null, - name: profile.name ?? null, + // Deliberately never persisted: the OAuth provider's name is unverified, user-controlled, + // and outlives a (soft) account deletion. It still seeds the username below, from the profile. + // The live NCMEC path (csam.service-new.ts) reads it only as the REPORTER's firstName, so staff + // accounts created from here on file reports without one — expected; they carry their email. + name: null, // Legacy behavior: never store the provider's avatar — users set their own profile picture, and an // unmoderated provider avatar shouldn't be displayed by default. image: null, diff --git a/src/server/services/__tests__/delete-user-pii-scrub.test.ts b/src/server/services/__tests__/delete-user-pii-scrub.test.ts new file mode 100644 index 0000000000..66cd14b616 --- /dev/null +++ b/src/server/services/__tests__/delete-user-pii-scrub.test.ts @@ -0,0 +1,246 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { dbMock } from '~/__tests__/mocks/db.mock'; +import { userFollowsCache } from '~/server/redis/caches'; + +/** + * Account deletion is a SOFT delete, so no FK cascade fires and nothing is removed for + * free. Measured on prod before this change, across 1,330,849 deleted accounts: + * 733,857 still carried `name`, 157,633 a Stripe `customerId`, 192,791 a UserProfile row. + * + * These assertions exist to keep that set scrubbed. If one fails, the account is leaking + * personal data again — do not relax it without saying what replaced it. + * + * Scope: this covers NEW deletions only. The historical rows above are untouched by this + * file and by the change it guards; they are the GDPR backfill's job. And `customerId` is + * deliberately left in place here — see the webhook test below for why. + */ + +import * as UserService from '~/server/services/user.service'; + +const USER_ID = 42; +const user = dbMock.dbWrite.user; + +const deleteUser = () => + UserService.deleteUser({ id: USER_ID, username: 'gone' } as Parameters< + typeof UserService.deleteUser + >[0]); + +/** + * The write paths this scan covers, named explicitly. `dbMock.dbWrite` is a proxy that + * materialises delegates on access, so Object.keys() over it enumerates NOTHING — a scan + * written that way returns [] for every input and the guard below passes forever. That was + * tried; the CONTROL tests are what caught it. Add a path here if a new one appears. + */ +const WRITE_PATHS = [ + () => ['dbWrite.user.update', dbMock.dbWrite.user.update] as const, + () => ['dbWrite.user.updateMany', dbMock.dbWrite.user.updateMany] as const, + () => ['dbWrite.$executeRaw', dbMock.dbWrite.$executeRaw] as const, + () => ['dbWrite.$executeRawUnsafe', dbMock.dbWrite.$executeRawUnsafe] as const, + // UPDATE ... RETURNING goes through the query methods, and user.service.ts uses them. + () => ['dbWrite.$queryRaw', dbMock.dbWrite.$queryRaw] as const, + () => ['dbWrite.$queryRawUnsafe', dbMock.dbWrite.$queryRawUnsafe] as const, + () => ['dbWrite.user.upsert', dbMock.dbWrite.user.upsert] as const, +]; +// NOT covered: pgDbWrite and kyselyWrite (which runs over it), and user.updateManyAndReturn. +// Writes inside an interactive $transaction ARE covered: the shared mock runs the callback +// against dbMock.dbWrite, so a `tx.user.update` lands on the paths above. Add a path above +// if one of the uncovered ones starts writing customerId. + +/** Labels of the covered dbWrite calls whose arguments mention `needle`. */ +const dbWriteCallsMentioning = (needle: string) => { + const hits: string[] = []; + for (const get of WRITE_PATHS) { + const [label, fn] = get(); + const calls = (fn as unknown as { mock?: { calls?: unknown[][] } })?.mock?.calls ?? []; + for (const call of calls) { + // BigInt-safe, and no silent fallback: String(call) turns an object into + // "[object Object]", which would drop the needle and report a false absence. + const serialized = JSON.stringify(call, (_k, v) => + typeof v === 'bigint' ? v.toString() : v + ); + if (serialized?.includes(needle)) hits.push(label); + } + } + return hits; +}; + +/** Index of the soft-delete update (the one carrying deletedAt) in user.update.mock.calls. */ +const softDeleteIndex = () => + user.update.mock.calls.findIndex( + ([arg]) => (arg as { data?: Record })?.data?.deletedAt !== undefined + ); + +/** The data object of the soft-delete update. */ +const softDeleteData = () => { + const call = user.update.mock.calls.find( + ([arg]) => (arg as { data?: Record })?.data?.deletedAt !== undefined + ); + return (call?.[0] as { data: Record }).data; +}; + +beforeEach(() => { + vi.clearAllMocks(); + vi.restoreAllMocks(); + dbMock.dbWrite.user.findFirst.mockResolvedValue({ id: USER_ID, meta: {} }); + dbMock.dbWrite.user.update.mockResolvedValue({}); + dbMock.dbWrite.model.updateMany.mockResolvedValue({ count: 0 }); + dbMock.dbWrite.account.deleteMany.mockResolvedValue({ count: 0 }); + dbMock.dbWrite.session.deleteMany.mockResolvedValue({ count: 0 }); + dbMock.dbWrite.userEngagement.deleteMany.mockResolvedValue({ count: 0 }); + dbMock.dbWrite.userProfile.deleteMany.mockResolvedValue({ count: 0 }); + dbMock.dbWrite.userLink.deleteMany.mockResolvedValue({ count: 0 }); + vi.spyOn(userFollowsCache, 'bust').mockResolvedValue(undefined); +}); + +describe('deleteUser — what the soft delete scrubs', () => { + it('nulls the provider-supplied name', async () => { + await deleteUser(); + + // 733,857 deleted accounts carried one, ~490k shaped "First Last". Nothing displays + // it, so a survivor is pure retained PII. + expect(softDeleteData().name).toBeNull(); + }); + + it('deletes the UserProfile row', async () => { + await deleteUser(); + + // Holds bio, location and showcase. deleteMany, not delete: most accounts have no + // row and `delete` throws on a miss. + expect(dbMock.dbWrite.userProfile.deleteMany).toHaveBeenCalledWith({ + where: { userId: USER_ID }, + }); + }); + + it('deletes every UserLink row', async () => { + await deleteUser(); + + expect(dbMock.dbWrite.userLink.deleteMany).toHaveBeenCalledWith({ + where: { userId: USER_ID }, + }); + }); + + it('soft-deletes and removes the profile and links INSIDE one transaction', async () => { + await deleteUser(); + + // Outside it they stop being atomic with the soft delete: a failure between the two + // leaves an account that is deleted with its profile live, or intact with it gone. + // Identity against the delegate's own return value — prisma builds every element of + // the array eagerly, so what lands in it is that PROMISE, not its result. + const [ops] = dbMock.dbWrite.$transaction.mock.calls[0] as [unknown[]]; + expect(ops).toContain(dbMock.dbWrite.userProfile.deleteMany.mock.results[0].value); + expect(ops).toContain(dbMock.dbWrite.userLink.deleteMany.mock.results[0].value); + // The soft delete itself too: without this, awaiting the user.update outside the array + // passed every test here, including the ones that say "inside the transaction". + expect(ops).toContain(user.update.mock.results[softDeleteIndex()].value); + }); +}); + +describe('deleteUser — payment-provider ids', () => { + it('nulls paddleCustomerId inside the transaction, as part of the soft delete', async () => { + await deleteUser(); + + // Atomic with the soft delete, so no later failure can leave it behind. This does mean + // cancelSubscriptionPlan's no-row fallback, which reads the id, cannot fire on a deletion; + // moving the null after the cancels was tried and reverted, because with seven live Paddle + // subscriptions it bought a live API call per deletion for almost nothing to cancel. + expect(softDeleteData()).toHaveProperty('paddleCustomerId', null); + const [ops] = dbMock.dbWrite.$transaction.mock.calls[0] as [unknown[]]; + expect(ops).toContain(user.update.mock.results[softDeleteIndex()].value); + }); + + it('does NOT purge the Stripe customerId — deleting it breaks our own webhook', async () => { + await deleteUser(); + + // Deliberate, and the reason is not local to this file, so read it before "fixing" it: + // deleteUser's own cancelSubscription calls stripe.subscriptions.del, and the resulting + // customer.subscription.deleted is resolved by findFirst({ where: { customerId } }) in + // upsertSubscription (stripe.service.ts:601-616). That throws before reaching either + // customerSubscription.delete below it, so nulling customerId here leaves the row `active` + // forever while Stripe retries the webhook for days. + // + // The GDPR scrub purges it instead, and must scrub Stripe FIRST: once the id is gone the + // customer record cannot be found again. + // + // Every path in WRITE_PATHS, not just user.update: a raw-SQL or updateMany purge + // reintroduces the identical webhook break, and reaching for raw SQL to null a column is + // an ordinary thing to do. Each path has a CONTROL test below proving the scan can see it, + // so this zero is a measured absence rather than a selector that matches nothing. + expect(dbWriteCallsMentioning('customerId')).toEqual([]); + }); + + it('CONTROL: the scan sees a customerId write via user.update', () => { + // An empty-array assertion is the shape that passes forever when the selector is broken, + // so the zero above is only worth anything with these beside it. Not hypothetical: the + // first version of this scan walked Object.keys(dbMock.dbWrite), which enumerates nothing + // on a proxy, and returned [] for every input. + void dbMock.dbWrite.user.update({ where: { id: USER_ID }, data: { customerId: null } }); + + expect(dbWriteCallsMentioning('customerId')).toEqual(['dbWrite.user.update']); + }); + + it('CONTROL: the scan sees a customerId write via raw SQL', () => { + // The route someone would actually reach for to null a column, and the one a user.update + // assertion cannot see. + void dbMock.dbWrite.$executeRawUnsafe('UPDATE "User" SET "customerId" = NULL WHERE id = 1'); + + expect(dbWriteCallsMentioning('customerId')).toEqual(['dbWrite.$executeRawUnsafe']); + }); + + it('CONTROL: the scan sees a customerId write via updateMany', () => { + void dbMock.dbWrite.user.updateMany({ where: { id: USER_ID }, data: { customerId: null } }); + + expect(dbWriteCallsMentioning('customerId')).toEqual(['dbWrite.user.updateMany']); + }); + + it('CONTROL: the scan still sees customerId when a BigInt is in the same call', () => { + // JSON.stringify throws on a BigInt. The previous fallback, String(call), turned the whole + // argument into "[object Object]" and dropped the needle — a false absence, not a failure. + void dbMock.dbWrite.user.update({ + where: { id: 1n as never }, + data: { customerId: null }, + } as never); + + expect(dbWriteCallsMentioning('customerId')).toEqual(['dbWrite.user.update']); + }); + + it('CONTROL: the scan sees a customerId write via $queryRawUnsafe', () => { + void dbMock.dbWrite.$queryRawUnsafe('UPDATE "User" SET "customerId" = NULL RETURNING id'); + + expect(dbWriteCallsMentioning('customerId')).toEqual(['dbWrite.$queryRawUnsafe']); + }); + + it('CONTROL: the scan sees a customerId write via tagged $queryRaw', () => { + void dbMock.dbWrite.$queryRaw(['UPDATE "User" SET "customerId" = NULL RETURNING id'] as never); + + expect(dbWriteCallsMentioning('customerId')).toEqual(['dbWrite.$queryRaw']); + }); + + it('CONTROL: the scan sees a customerId write via upsert', () => { + void dbMock.dbWrite.user.upsert({ + where: { id: USER_ID }, + create: { customerId: null }, + update: { customerId: null }, + } as never); + + expect(dbWriteCallsMentioning('customerId')).toEqual(['dbWrite.user.upsert']); + }); + + it('CONTROL: the scan sees a customerId write inside an interactive $transaction', async () => { + // Only true because the shared mock runs the callback. A test-local override returning + // its argument unrun used to hide exactly this route. + await dbMock.dbWrite.$transaction(async (tx: typeof dbMock.dbWrite) => + tx.user.update({ where: { id: USER_ID }, data: { customerId: null } }) + ); + + expect(dbWriteCallsMentioning('customerId')).toEqual(['dbWrite.user.update']); + }); + + it('CONTROL: the scan sees a customerId write via tagged raw SQL', () => { + // One control per path in WRITE_PATHS. A path named in that list but never demonstrated + // observable is a claim of coverage the scan may not have — the same failure as the + // Object.keys version, just narrower. + void dbMock.dbWrite.$executeRaw(['UPDATE "User" SET "customerId" = NULL'] as never); + + expect(dbWriteCallsMentioning('customerId')).toEqual(['dbWrite.$executeRaw']); + }); +}); diff --git a/src/server/services/user.service.ts b/src/server/services/user.service.ts index a0becbaca0..0423cd7f04 100644 --- a/src/server/services/user.service.ts +++ b/src/server/services/user.service.ts @@ -1141,12 +1141,19 @@ export const deleteUser = async ({ id, username, removeModels, removeImages }: D type: { not: UserEngagementType.Block }, }, }), + // deleteMany, not delete: most accounts have no row here, and `delete` throws on a + // miss. The FK cascade never fires for either of these because this is a SOFT delete. + dbWrite.userProfile.deleteMany({ where: { userId: user.id } }), + dbWrite.userLink.deleteMany({ where: { userId: user.id } }), dbWrite.user.update({ where: { id: user.id }, data: { deletedAt: new Date(), email: null, username: null, + name: null, + // customerId is deliberately absent: see the webhook test in + // __tests__/delete-user-pii-scrub.test.ts before adding it. paddleCustomerId: null, image: null, profilePictureId: null, @@ -1190,8 +1197,9 @@ export async function setLeaderboardEligibility({ id, setTo }: { id: number; set /** * Restore a soft-deleted user account (the inverse of deleteUser). * - * deleteUser scrubs username, email, paddleCustomerId, image, profilePictureId from the User row - * and sets deletedAt. It also hard-deletes Account / Session rows and every + * deleteUser scrubs username, email, name, paddleCustomerId, image, profilePictureId from the + * User row and sets deletedAt. It also hard-deletes Account / Session / UserProfile / UserLink + * rows and every * UserEngagement row the account appears in EXCEPT Blocks — those survive precisely so * a restore cannot leave someone unblocked without telling them — and reassigns * the user's Models to userId = -1. @@ -1205,6 +1213,9 @@ export async function setLeaderboardEligibility({ id, setTo }: { id: number; set * so restoring inside the window brings the images back. * Posts are hard-deleted on the immediate path only and are not recoverable. * + * UserProfile and UserLink rows are unrecoverable too, so a restored account comes back with an + * empty profile. Nothing restores name. + * * Account (OAuth links) and Session rows are unrecoverable; the user signs in fresh post-restore * (email magic-link or OAuth) which creates new rows. */