From 94854bd8f7559accd3d9a5af824a59c3583388e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 29 Oct 2026 21:42:18 +0100 Subject: [PATCH] fix(backup): trace an account made for an upload when it is made The upload creates a passwordless account before it reads the file and deletes it again when the file is refused. The only audit row was written after a successful store, so a refused upload whose delete also failed left an account nobody could sign into and no record of where it came from. The account and its admin.user.create row (outcome "created") now commit together, and the discard writes admin.user.delete with outcome "deleted" or "failed", so a leftover account always has a trace. The runbook says how to remove one by hand. --- docs/ops/backup-restore.md | 7 +- .../baselines/source-function-lines.json | 4 +- src/app/api/admin/backups/upload/route.ts | 79 +++++++++++++------ .../backup-erased-account-restore.test.ts | 70 +++++++++++++++- 4 files changed, 132 insertions(+), 28 deletions(-) diff --git a/docs/ops/backup-restore.md b/docs/ops/backup-restore.md index 94bd55a96..8779d947a 100644 --- a/docs/ops/backup-restore.md +++ b/docs/ops/backup-restore.md @@ -103,7 +103,12 @@ exists, and the console then asks where the records should go: password or email, as an ordinary account with the user role, and nobody can sign into it yet. Before handing it over, set its first password with **Reset password** in Admin → Users; the person can change it, add an email - and enrol a passkey from their settings. + and enrol a passkey from their settings. The account is made before the + file is read, with an `admin.user.create` audit row (`outcome: "created"`), + and removed again when the file is refused, with an `admin.user.delete` row + (`reason: "backup_upload_refused"`). When that row says `outcome: "failed"`, + the removal did not happen and the account is left over, holding nothing: + delete it in Admin → Users. - **An existing account**, for someone who has signed up again. Pick the account and type its username to confirm. The account must hold no records yet: a restore replaces an account's records and does not merge them, so an diff --git a/src/__tests__/baselines/source-function-lines.json b/src/__tests__/baselines/source-function-lines.json index 1cf1294f5..a9e9c0346 100644 --- a/src/__tests__/baselines/source-function-lines.json +++ b/src/__tests__/baselines/source-function-lines.json @@ -1,10 +1,10 @@ { "$comment": "Production functions over 200 lines, keyed \"file :: name\", and their length. Maintained by src/__tests__/source-size-ratchet.test.ts.", "count": 345, - "total": 111940, + "total": 111929, "entries": { "app/achievements/page.tsx :: AchievementsPage": 206, - "app/api/admin/backups/upload/route.ts :: apiHandler(arg)": 286, + "app/api/admin/backups/upload/route.ts :: apiHandler(arg)": 275, "app/api/admin/notifications/reminder-check/route.ts :: apiHandler(arg)": 206, "app/api/admin/settings/route.ts :: apiHandler(arg) #2": 211, "app/api/auth/login/route.ts :: apiHandler(arg)": 262, diff --git a/src/app/api/admin/backups/upload/route.ts b/src/app/api/admin/backups/upload/route.ts index 7708fae2c..386062a8d 100644 --- a/src/app/api/admin/backups/upload/route.ts +++ b/src/app/api/admin/backups/upload/route.ts @@ -295,17 +295,42 @@ async function checkTargetBeforeRead( return null; } +/** Who made an account for a copy, for the trail. */ +interface UploadActor { + adminId: string; + ipAddress: string | null; +} + /** * The account made for a copy. Created before the file is read, because the * file is rewritten to its id on the way in, and deleted again when nothing * is stored. No password, no email: nobody can sign into it until an admin - * sets its first password (Admin, Users, Reset password). + * sets its first password (Admin, Users, Reset password). Its audit row + * commits with it, so an account this route made always has a trace, even + * when the upload fails and removing it again fails too. */ -async function createTargetAccount(username: string): Promise { +async function createTargetAccount( + username: string, + actor: UploadActor, +): Promise { try { - const account = await prisma.user.create({ - data: { username, role: "USER" }, - select: { id: true, username: true }, + const account = await prisma.$transaction(async (tx) => { + const row = await tx.user.create({ + data: { username, role: "USER" }, + select: { id: true, username: true }, + }); + await auditLog("admin.user.create", { + userId: actor.adminId, + ipAddress: actor.ipAddress, + client: tx, + details: { + targetUserId: row.id, + username: row.username, + reason: "backup_restore", + outcome: "created", + }, + }); + return row; }); return { ...account, created: true }; } catch (err) { @@ -355,13 +380,20 @@ async function ownerOfRetargetedFile( /** * Delete the account an upload created for its copy, when nothing was stored. * It holds nothing and nobody can have signed into it (no password, no email), - * so the row is all there is. A failure is noted, never thrown over the + * so the row is all there is. Either way the trail says what became of it: a + * removal that failed leaves an account to delete by hand + * (`docs/ops/backup-restore.md`). A failure is noted, never thrown over the * refusal the operator needs to see. */ -async function discardCreatedAccount(userId: string): Promise { +async function discardCreatedAccount( + account: ChosenTarget, + actor: UploadActor, +): Promise { + let outcome: "deleted" | "failed" = "deleted"; try { - await prisma.user.delete({ where: { id: userId } }); + await prisma.user.delete({ where: { id: account.id } }); } catch (err) { + outcome = "failed"; annotate({ meta: { backup_upload_account_discard_failed: true, @@ -370,6 +402,20 @@ async function discardCreatedAccount(userId: string): Promise { }, }); } + try { + await auditLog("admin.user.delete", { + userId: actor.adminId, + ipAddress: actor.ipAddress, + details: { + targetUserId: account.id, + username: account.username, + reason: "backup_upload_refused", + outcome, + }, + }); + } catch { + annotate({ meta: { backup_upload_account_discard_unaudited: true } }); + } } export const POST = apiHandler(async (request: NextRequest) => { @@ -379,6 +425,7 @@ export const POST = apiHandler(async (request: NextRequest) => { await assertRecentCookieProof(admin, session.id); annotate({ action: { name: "admin.backups.upload" } }); const ipAddress = getClientIp(request); + const actor: UploadActor = { adminId: admin.id, ipAddress }; // Rate-limit identical to the manual-run endpoint. Three is plenty for a // deliberate restore-prep workflow. @@ -494,7 +541,7 @@ export const POST = apiHandler(async (request: NextRequest) => { let rewrite: BackupOwnerRewrite | null = null; if (choice?.into === "new") { try { - target = await createTargetAccount(choice.username); + target = await createTargetAccount(choice.username, actor); } catch (err) { if (err instanceof UploadRefused) return refuse(err); throw err; @@ -583,7 +630,7 @@ export const POST = apiHandler(async (request: NextRequest) => { ); } catch (err) { // Nothing was stored, so the account made for the copy goes again. - if (chosen?.created) await discardCreatedAccount(chosen.id); + if (chosen?.created) await discardCreatedAccount(chosen, actor); if (err instanceof UploadRefused) return refuse(err); // The file was fine; its encrypted copy is larger than the stored-copy // limit (`BACKUP_MAX_STORED_MB`). That is the operator's to act on, with @@ -609,18 +656,6 @@ export const POST = apiHandler(async (request: NextRequest) => { }; const storedOwner = owner as { id: string; username: string } | null; - if (chosen?.created) { - await auditLog("admin.user.create", { - userId: admin.id, - ipAddress, - details: { - targetUserId: chosen.id, - username: chosen.username, - reason: "backup_restore", - backupId: created.id, - }, - }); - } await auditLog("admin.backups.upload", { userId: admin.id, ipAddress, diff --git a/tests/integration/backup-erased-account-restore.test.ts b/tests/integration/backup-erased-account-restore.test.ts index f35b59c7c..9ab37b93a 100644 --- a/tests/integration/backup-erased-account-restore.test.ts +++ b/tests/integration/backup-erased-account-restore.test.ts @@ -677,17 +677,20 @@ describe("restoring the backup of an erased account into a chosen account", () = passwordHash: null, }); + // Traced when it is made, before the file is read; the upload row + // links it to the copy. expect(await auditDetails(prisma, "admin.user.create")).toEqual([ { targetUserId: account.id, username: "returning-person", reason: "backup_restore", - backupId, + outcome: "created", }, ]); expect(await auditDetails(prisma, "admin.backups.upload")).toEqual([ - expect.objectContaining({ ownerId: account.id, into: "new" }), + expect.objectContaining({ backupId, ownerId: account.id, into: "new" }), ]); + expect(await auditDetails(prisma, "admin.user.delete")).toEqual([]); expect(await cellsNaming(prisma, OWNER_ID)).toEqual([]); // How the person gets in: the admin sets a first password with the @@ -861,7 +864,24 @@ describe("restoring the backup of an erased account into a chosen account", () = expect( await prisma.user.count({ where: { username: "would-be-copy" } }), ).toBe(0); - expect(await auditDetails(prisma, "admin.user.create")).toEqual([]); + // The account was made, traced, and removed again, traced as well. + const made = await auditDetails(prisma, "admin.user.create"); + expect(made).toEqual([ + { + targetUserId: expect.any(String), + username: "would-be-copy", + reason: "backup_restore", + outcome: "created", + }, + ]); + expect(await auditDetails(prisma, "admin.user.delete")).toEqual([ + { + targetUserId: (made[0] as { targetUserId: string }).targetUserId, + username: "would-be-copy", + reason: "backup_upload_refused", + outcome: "deleted", + }, + ]); expect(await prisma.dataBackup.count()).toBe(0); // Without a target, the same file goes to its own account as before. @@ -879,6 +899,50 @@ describe("restoring the backup of an erased account into a chosen account", () = SLOW, ); + it( + "leaves a trace of an account made for a refused file when it cannot be removed again", + async () => { + const prisma = getPrismaClient(); + const { json } = await eraseAfterExport(prisma); + await createOwner(prisma); + const { prisma: appPrisma } = await import("@/lib/db"); + const remove = vi + .spyOn(appPrisma.user, "delete") + .mockRejectedValueOnce(new Error("connection reset")); + + const refused = await uploadInto(json, { + into: "new", + username: "left-behind", + }); + remove.mockRestore(); + // The operator still sees why the file was refused. + expect(refused.status, JSON.stringify(refused.body)).toBe(409); + expect(refused.body.meta?.errorCode).toBe("backup.owner.exists"); + + const account = await prisma.user.findUniqueOrThrow({ + where: { username: "left-behind" }, + }); + expect(account).toMatchObject({ passwordHash: null, email: null }); + expect(await auditDetails(prisma, "admin.user.create")).toEqual([ + { + targetUserId: account.id, + username: "left-behind", + reason: "backup_restore", + outcome: "created", + }, + ]); + expect(await auditDetails(prisma, "admin.user.delete")).toEqual([ + { + targetUserId: account.id, + username: "left-behind", + reason: "backup_upload_refused", + outcome: "failed", + }, + ]); + }, + SLOW, + ); + it( "refuses a non-admin session and an admin's Bearer token, and creates nothing", async () => {