From 7370b2cd7d8e79b2a4bef69fef325883945f7197 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 13 May 2026 13:06:27 +0200 Subject: [PATCH] =?UTF-8?q?fix(audit-wave-11):=20file-lifecycle=20hardenin?= =?UTF-8?q?g=20=E2=80=94=20avatar=20leak=20+=20files=20FK?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **file-lifecycle-auditor C1 — avatar replace leaks rows + blobs** `POST /api/v1/me/avatar` overwrote `userProfiles.avatarFileId` without reading or deleting the previous file id. Every "Replace photo" leaked one `files` row + one S3 blob, untethered (no client/yacht/company FK) and invisible to every existing UI sweep. Now captures the prior id BEFORE the UPDATE, then best-effort `deleteFile()` on the old row (handles ref-check + blob delete + audit) after the new id is committed. Failure is logged at warn — a stale blob shouldn't block the user from setting a new avatar. **file-lifecycle-auditor M1 — files.client_id missing ON DELETE** `files.client_id` was the only entity FK on the polymorphic `files` table that defaulted to `NO ACTION` (yacht_id + company_id were `SET NULL` per migration 0042). Any future bulk-client-delete that bypassed `hardDeleteClient`'s explicit FK-nullify pre-step would FK-violate. Migration `0059_files_client_id_onDelete_setnull.sql` brings it to parity; the explicit nullify in client-hard-delete is kept as defense in depth. Tests 1315/1315. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/app/api/v1/me/avatar/route.ts | 32 ++++++++++++++++++- .../0059_files_client_id_onDelete_setnull.sql | 18 +++++++++++ src/lib/db/schema/documents.ts | 2 +- 3 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 src/lib/db/migrations/0059_files_client_id_onDelete_setnull.sql diff --git a/src/app/api/v1/me/avatar/route.ts b/src/app/api/v1/me/avatar/route.ts index b48b40f0..7fa8a513 100644 --- a/src/app/api/v1/me/avatar/route.ts +++ b/src/app/api/v1/me/avatar/route.ts @@ -5,8 +5,9 @@ import { withAuth } from '@/lib/api/helpers'; import { db } from '@/lib/db'; import { ports } from '@/lib/db/schema/ports'; import { userProfiles } from '@/lib/db/schema/users'; -import { uploadFile } from '@/lib/services/files'; +import { deleteFile, uploadFile } from '@/lib/services/files'; import { errorResponse, ValidationError } from '@/lib/errors'; +import { logger } from '@/lib/logger'; const MAX_AVATAR_BYTES = 2 * 1024 * 1024; @@ -67,11 +68,40 @@ export const POST = withAuth(async (req, ctx) => { }, ); + // file-lifecycle-auditor C1: capture the prior avatar id BEFORE + // overwriting so we can clean it up. Without this every "Replace + // photo" leaked one files row + one S3 blob, untethered (no + // client/yacht/company FK) and invisible to UI sweeps. + const prior = await db.query.userProfiles.findFirst({ + where: eq(userProfiles.userId, ctx.userId), + columns: { avatarFileId: true }, + }); + const priorAvatarId = prior?.avatarFileId ?? null; + await db .update(userProfiles) .set({ avatarFileId: record.id, updatedAt: new Date() }) .where(eq(userProfiles.userId, ctx.userId)); + if (priorAvatarId && priorAvatarId !== record.id) { + // Best-effort delete — a stale-blob failure shouldn't fail the + // new-avatar response. deleteFile handles ref-check + blob + // delete + audit so a referenced file (somehow) is safe. + try { + await deleteFile(priorAvatarId, portId, { + userId: ctx.userId, + portId, + ipAddress: ctx.ipAddress, + userAgent: ctx.userAgent, + }); + } catch (err) { + logger.warn( + { err, priorAvatarId, userId: ctx.userId }, + 'avatar replace: failed to clean up prior avatar file — orphan blob possible', + ); + } + } + return NextResponse.json({ data: { avatarFileId: record.id } }); } catch (error) { return errorResponse(error); diff --git a/src/lib/db/migrations/0059_files_client_id_onDelete_setnull.sql b/src/lib/db/migrations/0059_files_client_id_onDelete_setnull.sql new file mode 100644 index 00000000..f5aaf775 --- /dev/null +++ b/src/lib/db/migrations/0059_files_client_id_onDelete_setnull.sql @@ -0,0 +1,18 @@ +-- file-lifecycle-auditor M1: `files.client_id` had no explicit +-- `ON DELETE` (defaulted to NO ACTION), so any future bulk-client-delete +-- or port-teardown that bypassed `hardDeleteClient` would FK-violate. +-- Sister columns `files.yacht_id` and `files.company_id` were already +-- SET NULL in migration 0042; bring `files.client_id` to parity so the +-- "hard-delete is the only legal path" invariant becomes explicit. +-- +-- The existing `client-hard-delete.service.ts` nullifies these FKs +-- explicitly before the client DELETE; that pre-step is now redundant +-- but harmless, so we don't remove it (defense in depth). + +ALTER TABLE files + DROP CONSTRAINT IF EXISTS files_client_id_clients_id_fk; + +ALTER TABLE files + ADD CONSTRAINT files_client_id_clients_id_fk + FOREIGN KEY (client_id) REFERENCES clients(id) + ON DELETE SET NULL; diff --git a/src/lib/db/schema/documents.ts b/src/lib/db/schema/documents.ts index e79a8360..8862d817 100644 --- a/src/lib/db/schema/documents.ts +++ b/src/lib/db/schema/documents.ts @@ -27,7 +27,7 @@ export const files = pgTable( portId: text('port_id') .notNull() .references(() => ports.id), - clientId: text('client_id').references(() => clients.id), + clientId: text('client_id').references(() => clients.id, { onDelete: 'set null' }), yachtId: text('yacht_id').references(() => yachts.id, { onDelete: 'set null' }), companyId: text('company_id').references(() => companies.id, { onDelete: 'set null' }), folderId: text('folder_id').references((): AnyPgColumn => documentFolders.id, {