mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
fix(users): scrub name, profile and links on account deletion; stop persisting the OAuth name (#4970)
* fix(users): scrub name, profile and links on account deletion; stop persisting the OAuth name Account deletion is a soft delete, so no FK cascade fires. On prod, of 1,330,849 deleted accounts, 733,857 still carried `name` and 192,791 a UserProfile row. - apps/auth: stop writing User.name on OAuth signup. It is unverified, user-controlled data that outlives a soft delete. It still seeds the generated username from the transient profile. - deleteUser: also null `name` and delete the UserProfile row and every UserLink row, inside the transaction. - Move the paddleCustomerId purge out of the transaction into a `finally` after the subscription cancels. cancelSubscriptionPlan falls back to reading it, so while it was nulled in the transaction that fallback could never fire on a deletion. The `finally` keeps the purge unskippable when an earlier unwrapped await throws. customerId is deliberately NOT purged here: deleteUser's own cancelSubscription triggers a Stripe webhook that resolves the user by customerId and throws before deleting the CustomerSubscription row, so nulling it would leave the row `active` forever. It is purged by the GDPR scrub, which must reach Stripe first. Pinned by a test named for it. Staff accounts created after this change file NCMEC reports without a reporter firstName; the live report path reads `name` for nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(users): keep paddleCustomerId in the deletion transaction; harden the scrub tests Reverts the paddleCustomerId ordering change from the previous commit. Moving the null after the subscription cancels let cancelSubscriptionPlan's no-row fallback run, but with seven live Paddle subscriptions (none on a deleted account) it only added a live Paddle API call per deletion, a false cancel-paddle-subscription error for nearly every one, and an unbounded wait on a client with no timeout. paddleCustomerId is nulled inside the transaction again, exactly as on main, and the try/finally that existed only to protect that later null is removed, restoring main's tail. deleteUser's net change is now only the GDPR scrub: null `name` and delete the UserProfile and UserLink rows inside the transaction. Tests, from the five-lane review: - the customerId scan serialises BigInt instead of falling back to String(call), which turned an object into "[object Object]" and reported a false absence; a CONTROL pins it - the scan's uncovered write paths are listed (kyselyWrite, updateManyAndReturn), alongside pgDbWrite and interactive transactions - tests that only made sense for the reverted ordering are removed, and one pins paddleCustomerId inside the transaction Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(users): pin the soft delete inside the transaction; cover interactive transactions From the test-lane re-review of #4970: - Nothing asserted that the soft-delete user.update is itself one of the $transaction ops. Awaiting it outside the array passed every test, including the ones named "inside the transaction". Both the transaction test and the paddleCustomerId test now assert its identity in the ops array. - A test-local $transaction override returned its argument unrun, which hid customerId writes made inside an interactive transaction. The shared mock runs the callback, so the override is removed and a CONTROL proves the scan now sees that route. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Justin Maier
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string, unknown> })?.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<string, unknown> })?.data?.deletedAt !== undefined
|
||||
);
|
||||
return (call?.[0] as { data: Record<string, unknown> }).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']);
|
||||
});
|
||||
});
|
||||
@@ -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.
|
||||
*/
|
||||
|
||||
Reference in New Issue
Block a user