diff --git a/.changeset/storage-tombstone-download-live-holder.md b/.changeset/storage-tombstone-download-live-holder.md new file mode 100644 index 0000000000..1e735009af --- /dev/null +++ b/.changeset/storage-tombstone-download-live-holder.md @@ -0,0 +1,18 @@ +--- +"@objectstack/service-storage": patch +--- + +**Fix:** a tombstoned `sys_file` that something still holds is downloadable again — no 30-day 404 in between (#10246). + +Re-pointing a `sys_attachment` join row onto a file inside its 30-day grace-window tombstone has always been byte-safe: the reap guard re-verifies references at sweep time, finds the new holder, un-tombstones the row and vetoes the reap. But the sweep is the only thing that ever asked, and `sys_file`'s declared lifecycle (`ttl { field: 'deleted_at', expireAfter: '30d' }`) nominates a tombstone only **after** the window expires — measured candidates inside the window: `[]`. So the file simply sat at `status='deleted'` while `GET /api/v1/storage/files/:fileId` and `/files/:fileId/url` refused anything not `committed`. A live attachment could point at a file that 404s for up to 30 days and then silently starts working. + +**What changed:** the two download endpoints stop treating the tombstone as the last word. They now ask the reap guard's own `findFileHolder` — the single definition of "is anything still holding this file?", a union over `sys_attachment` join rows and the `ref_*` ownership columns — and serve the file for exactly as long as that answers yes. + +**What did not change**, deliberately: + +- **No lifecycle verb was added.** There is no un-tombstone, revive or resurrect on the read path; the download writes nothing to the row. Revival remains solely the sweep guard's, which is why the fix is a read-side predicate and not a second revival mechanism (the duplicate-mechanism hazard #10241 avoided). The tombstone stays, and the sweep still reaps when the last holder goes. +- **`pending` is still refused.** Only the `deleted` limb widened; an upload that was never completed has no bytes to promise. +- **Authorization is untouched.** A served tombstone goes through the same `authorizeFileRead` gate as any other file — `AUTH_REQUIRED` (401) and `ATTACHMENT_DOWNLOAD_DENIED` / `FILE_DOWNLOAD_DENIED` (403) are unaffected. Servability is not authorization. +- **Bare kernels are unaffected.** With no data engine there is no holder question to ask, so tombstones stay refused exactly as before. + +The read side and the sweep now answer the same question from the same code, so a file the download path serves is by construction a file the next sweep would veto rather than reap — and the instant the last holder goes, both flip together. That pair is what the new tests pin; the 404 text on the refusal changed from "File not found or not committed" to "File not found or not downloadable" to match (the `FILE_NOT_FOUND` code is unchanged). diff --git a/content/docs/permissions/attachments-access.mdx b/content/docs/permissions/attachments-access.mdx index 334353bd27..04df23e41a 100644 --- a/content/docs/permissions/attachments-access.mdx +++ b/content/docs/permissions/attachments-access.mdx @@ -109,6 +109,13 @@ can be shared across records). Reclamation is handled by the platform LifecycleS attachments-scope file is deleted, the file is tombstoned; a reap guard re-verifies zero references at sweep time and deletes the storage bytes before the row is reaped (abandoned `pending` uploads are reaped too). + **A tombstone is recoverable state, not a delete — and downloads treat it that + way.** Re-attaching the file, or re-claiming it through a record field, makes + it downloadable again *immediately*, with no sweep in between: the download + endpoints ask the same "is anything still holding this file?" question the + reap guard asks before it reclaims anything. The row itself stays tombstoned + until a sweep tidies it, and a file with no holder left still answers + `FILE_NOT_FOUND` (404). - **`sys_upload_session`** — abandoned/terminal chunked-upload sessions are reaped, and a reap guard aborts the underlying backend multipart upload (S3 `AbortMultipartUpload` / local parts dir) first, so already-uploaded diff --git a/docs/qa/platform-checklist/areas/attachments-storage.json b/docs/qa/platform-checklist/areas/attachments-storage.json index 0b84e553e1..32441f9e7c 100644 --- a/docs/qa/platform-checklist/areas/attachments-storage.json +++ b/docs/qa/platform-checklist/areas/attachments-storage.json @@ -433,7 +433,7 @@ "title": "sys_file status pipeline: pending → committed → deleted (tombstone) with un-tombstone on re-attach; shared files never tombstone early", "since": "v15.1", "status": "active", - "revision": 1, + "revision": 2, "priority": "P2", "surface": "api", "personas": ["seeded admin (admin@objectos.ai)"], @@ -445,11 +445,13 @@ }, "steps": [ "presign an attachments-scope upload and read the sys_file row: status 'pending'", - "attempt GET /api/v1/storage/files//url while still pending and capture the refusal (downloads only serve committed files)", + "attempt GET /api/v1/storage/files//url while still pending and capture the refusal (a never-completed upload has no bytes to promise — this is the PENDING refusal, not a 'committed-only' rule)", "complete the upload; re-read: status 'committed'", "attach the file to 'Website Relaunch' AND 'Data Platform' (two sys_attachment join rows over ONE file — the Salesforce ContentDocumentLink share pattern)", "delete the 'Data Platform' join row and re-read sys_file: still 'committed' (a remaining reference blocks the tombstone)", "delete the LAST join row and re-read: status 'deleted' with deleted_at set (the tombstone)", + "while tombstoned AND holder-less, GET /api/v1/storage/files//url: 404 FILE_NOT_FOUND", + "attach a NEW join row onto the still-tombstoned file_id and immediately GET the download URL again, WITHOUT waiting for any sweep: 200 (#10246 — the download path asks the reap guard's own holder question, so a file the sweep would refuse to reap is a file the download path serves)", "re-attach the same file_id to 'Website Relaunch' within the grace window and re-read: status back to 'committed', deleted_at null", "verify a NON-attachments-scope file (e.g. an invoice-line receipt, scope from the field-upload path) is never tombstoned by these join-row hooks" ], @@ -461,11 +463,17 @@ "evidence": "the read sequence, one per transition" }, { - "clause": "a pending (never-completed) file is not downloadable: the download routes answer 404 FILE_NOT_FOUND for status != committed", + "clause": "a pending (never-completed) file is not downloadable: the download routes answer 404 FILE_NOT_FOUND while status is 'pending'", "oracle": "api", "verify": "the /url GET during the pending window returns 404 with that code", "evidence": "the 404 body" }, + { + "clause": "the refusal is keyed to NO REMAINING HOLDER, not to the tombstone: a 'deleted' file that still has at least one live sys_attachment join row (or a live ref_* owner) downloads with 200 immediately, with no sweep in between; the tombstone row is NOT rewritten by the download (#10246)", + "oracle": "api", + "verify": "GET /url on the tombstoned file 404s while holder-less and 200s once a join row is attached, in the same grace window; a sys_file re-read after the 200 still shows status 'deleted' with deleted_at set", + "evidence": "the two /url responses plus the post-download sys_file read" + }, { "clause": "one file shared by two join rows survives losing one of them — deleting an attachment deletes only the join row; the tombstone fires only when the LAST reference goes", "oracle": "api", @@ -499,7 +507,8 @@ "docs/plans/release-15.1-test-plan.md §C4 (#2755)" ], "history": [ - { "revision": 1, "date": "2026-08-07", "change": "new item: the full status pipeline as a variants matrix over the sys_file.status enum, with the shared-file and re-attach transitions from the lifecycle-hook source", "ref": "claude/platform-test-checklist-ocwugl" } + { "revision": 1, "date": "2026-08-07", "change": "new item: the full status pipeline as a variants matrix over the sys_file.status enum, with the shared-file and re-attach transitions from the lifecycle-hook source", "ref": "claude/platform-test-checklist-ocwugl" }, + { "revision": 2, "date": "2026-08-23", "change": "clause 2 was FALSIFIED by #10246 and is repaired here, not merely re-worded. It stated the download refusal as 'status != committed', which was true when written and is not any more: the download routes now ask the reap guard's own findFileHolder before refusing a tombstone, so a 'deleted' file with a live sys_attachment join row (or a live ref_* owner) serves 200 inside the grace window instead of 404ing for up to 30 days and then silently starting to work once a sweep ran. A runner scoring the old clause would have marked the FIXED behaviour as a failure. The clause is now keyed to 'pending', which is the limb that genuinely still refuses; a new clause pins the widened one AS A PAIR (404 while holder-less, 200 once a holder exists, tombstone row unrewritten) because proving only the 200 proves that something got wider and not that the boundary held. Two steps added for the new clause, and step 2's parenthetical stopped asserting a committed-only rule. Note for the next author: docs-drift could not have caught this — it is symbol-anchor precision-first (#9192) and cannot see a prose claim inside a clause string.", "ref": "#10246" } ] }, { diff --git a/packages/services/service-storage/src/storage-routes.ts b/packages/services/service-storage/src/storage-routes.ts index 12bb09c949..7270174267 100644 --- a/packages/services/service-storage/src/storage-routes.ts +++ b/packages/services/service-storage/src/storage-routes.ts @@ -6,6 +6,10 @@ import type { IHttpServer, IHttpRequest, IHttpResponse, IStorageService } from ' import { sendOk, sendError } from '@objectstack/types'; import type { StorageMetadataStore, FileRecord, UploadSessionRecord } from './metadata-store.js'; import type { LocalStorageAdapter } from './local-storage-adapter.js'; +// Type only. The PREDICATE is never re-implemented in this file (#10246): it +// arrives through `opts.resolveFileHolder`, which the plugin binds to the reap +// guard's own `findFileHolder`. +import type { FileHolder } from './attachment-lifecycle.js'; import { contentDispositionValue } from './content-disposition.js'; /** Authorization verdict for an attachments-scope download (#2970 item 2). */ @@ -47,6 +51,35 @@ export interface StorageRoutesOptions { * When absent (bare kernels, tests), all downloads stay open (back-compat). */ authorizeFileRead?: (file: FileRecord, req: IHttpRequest) => Promise; + /** + * "Is anything still holding this file?" for a TOMBSTONED row (#10246). + * + * A `sys_file` tombstone (`status: 'deleted'` + `deleted_at`) is recoverable + * state, not a delete: re-pointing a `sys_attachment` join row onto it, or + * re-claiming it through the `ref_*` ownership columns, makes it live again + * — and the sweep already honours that, un-tombstoning and vetoing the reap + * instead of reclaiming the bytes. But the sweep is the only thing that ever + * asks, and it asks only AFTER the declared 30d TTL expires, so inside the + * grace window a live attachment pointed at a tombstone downloaded as 404 + * for up to 30 days and then silently started working. + * + * ⛔ This does NOT add a second revival mechanism. Revival stays solely the + * sweep guard's; nothing on the read path writes to the row. What moves here + * is the JUDGEMENT — the download path stops treating the tombstone as the + * last word and asks the same question the guard asks. + * + * ⚠️ Wire this to `findFileHolder` (`attachment-lifecycle.ts`) and to + * nothing else. That function is the ONE definition of "still held", a + * deliberate union of the two surfaces that can hold a `sys_file` — + * `sys_attachment` join rows AND the `ref_*` ownership columns — and it is + * what decides whether the next sweep reaps this row. A read side that + * re-derived a narrower question (join rows only, say) would refuse files + * the sweep refuses to reap: the same defect, one limb over. + * + * Absent (bare kernels, no data engine, tests that don't wire it): tombstones + * stay refused, exactly as before this option existed. + */ + resolveFileHolder?: (file: FileRecord) => Promise; /** * TTL (seconds) for the signed URL minted on a GATED attachments download. * Short by design — the link is followed immediately after an explicit @@ -133,6 +166,42 @@ export function registerStorageRoutes( return downloadTtl; }; + // ── Download servability (#10246) ──────────────────────────────────── + // Written ONCE and called by both download endpoints. They used to carry a + // copy each of `file.status !== 'committed'`, which is how a rule that needs + // to widen turns into two rules that drift; `/files/:fileId/url` and + // `/files/:fileId` are the same decision reached through two doors. + // + // - `committed` → servable, unconditionally and unchanged. + // - `pending` → refused, unconditionally and unchanged: an upload that + // was never completed has no bytes to promise. + // - `deleted` → servable for exactly as long as something still holds + // it. The tombstone is NOT the last word; it is a claim + // about the future (this row is reapable when the grace + // window ends) that the sweep re-checks and often + // withdraws. Asking the guard's own question here makes + // the two agree by construction: a file this returns + // `true` for is a file the next sweep would un-tombstone + // rather than reap, and the moment the last holder goes it + // returns `false` again — same instant the sweep starts + // reaping it. + // + // The row is never written to. Revival remains the sweep guard's alone + // (triage's ruling on this card: 复活机制仍唯一归 sweep guard,判断移到读侧, + // 不新增生命周期动词). + const isServableForDownload = async (file: FileRecord): Promise => { + if (file.status === 'committed') return true; + if (file.status !== 'deleted' || !opts.resolveFileHolder) return false; + try { + return (await opts.resolveFileHolder(file)) !== null; + } catch { + // Unreadable evidence is not evidence of a holder. Refuse — the same + // answer this route gave before #10246, and the same direction the reap + // guard fails in (it vetoes rather than reaps when it cannot tell). + return false; + } + }; + // ── Upload auth gate (#2755) ───────────────────────────────────────── // `false` ⇒ the 401 was already sent and the handler must stop. // `null` ⇒ open mode (no resolver wired) — proceed unauthenticated. @@ -603,8 +672,8 @@ export function registerStorageRoutes( try { const { fileId } = req.params; const file = await store.getFile(fileId); - if (!file || file.status !== 'committed') { - sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not committed'); + if (!file || !(await isServableForDownload(file))) { + sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not downloadable'); return; } @@ -650,8 +719,8 @@ export function registerStorageRoutes( try { const { fileId } = req.params; const file = await store.getFile(fileId); - if (!file || file.status !== 'committed') { - sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not committed'); + if (!file || !(await isServableForDownload(file))) { + sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not downloadable'); return; } diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index 62ff91c409..00fa41cd83 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -21,7 +21,7 @@ import { StorageMetadataStore } from './metadata-store.js'; import type { FileRecord } from './metadata-store.js'; import { registerStorageRoutes } from './storage-routes.js'; import type { FileReadVerdict } from './storage-routes.js'; -import { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard } from './attachment-lifecycle.js'; +import { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard, findFileHolder } from './attachment-lifecycle.js'; import { installFileReferenceHooks } from './file-reference-lifecycle.js'; import { installAttachmentAccessHooks, installAttachmentReadVisibility } from './attachment-access-hooks.js'; import { SystemFile, SystemUploadSession } from './objects/index.js'; @@ -413,6 +413,16 @@ export class StorageServicePlugin implements Plugin { sessionTtl: this.options.sessionTtl, resolveSession: buildAuthSessionResolver(ctx), authorizeFileRead: buildFileReadAuthorizer(ctx, engine), + // "Is anything still holding this tombstone?" on the READ side + // (#10246) — the reap guard's own `findFileHolder`, handed over + // rather than re-derived. One definition of "still held", asked by + // the sweep before it reaps and by the download path before it + // refuses, so the two cannot answer differently. No engine (bare + // kernel) leaves it undefined and tombstones stay refused. + resolveFileHolder: + engine && typeof (engine as any).find === 'function' + ? (file: FileRecord) => findFileHolder(engine as any, file.id, file as any) + : undefined, logger: ctx.logger, }); diff --git a/packages/services/service-storage/src/tombstone-download-live-reference.test.ts b/packages/services/service-storage/src/tombstone-download-live-reference.test.ts new file mode 100644 index 0000000000..ef50e56b53 --- /dev/null +++ b/packages/services/service-storage/src/tombstone-download-live-reference.test.ts @@ -0,0 +1,321 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #10246 — a tombstoned `sys_file` that something still holds is downloadable. + * + * ## The defect + * + * A `sys_file` tombstone (`status: 'deleted'` + `deleted_at`) is recoverable + * state: re-pointing a `sys_attachment` join row onto it, or re-claiming it + * through the `ref_*` ownership columns, brings it back, and the reap guard + * honours that at sweep time — `findFileHolder` says "still held", so the guard + * un-tombstones the row and vetoes the reap instead of deleting the bytes. + * + * But the sweep is the ONLY thing that ever asked, and `sys_file`'s declared + * lifecycle (`ttl { field: 'deleted_at', expireAfter: '30d' }`) nominates a + * tombstone only AFTER the window expires. Inside the window the file was not + * a candidate at all, so nothing asked — while the download routes refused + * anything not `committed`. Net: a live attachment could point at a file that + * 404s for up to 30 days and then silently starts working. + * + * ## What is pinned here — the PAIR, not the widening + * + * Proving "tombstoned + live join row ⇒ 200" alone proves only that something + * got wider. The boundary is the pair, and both halves are asked of ONE shared + * fixture so they cannot drift apart: + * + * - tombstoned + a live holder ⇒ **200**, and the sweep would **veto**; + * - the last holder removed ⇒ **404**, and the sweep **reaps** (bytes and + * row), exactly as before this change. + * + * `pending` is pinned unchanged on purpose: only the `deleted` limb widened. + * + * ## Why the read side calls the guard's predicate rather than counting rows + * + * `findFileHolder` is a deliberate UNION — `sys_attachment` join rows OR the + * `ref_*` ownership columns — because both surfaces can hold one `sys_file`. + * It is also what decides whether the next sweep reaps the row. A read side + * that re-derived a narrower question (join rows only) would refuse files the + * sweep refuses to reap: the same defect, one limb over. Both limbs are pinned + * below against the guard's verdict on the same row. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +// The fake engine below is a WRITE-CAPABLE double, so its `update` opens with +// the real dispatch predicate rather than a hand-written approximation +// (`check:engine-double-contract`, #4550/#5480). It declares no `delete`: this +// suite never deletes a row, and a verb a double does not need is a contract +// it cannot get wrong. +import { assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { LocalStorageAdapter } from './local-storage-adapter.js'; +import { StorageMetadataStore } from './metadata-store.js'; +import { registerStorageRoutes } from './storage-routes.js'; +import { createSysFileReapGuard, findFileHolder } from './attachment-lifecycle.js'; + +const DOWNLOAD_ROUTES = ['/api/v1/storage/files/:fileId/url', '/api/v1/storage/files/:fileId'] as const; + +const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); + +function createMockHttpServer() { + const routes = new Map(); + return { + get: vi.fn((path: string, handler: RouteHandler) => { routes.set(`GET:${path}`, handler); }), + post: vi.fn((path: string, handler: RouteHandler) => { routes.set(`POST:${path}`, handler); }), + put: vi.fn((path: string, handler: RouteHandler) => { routes.set(`PUT:${path}`, handler); }), + delete: vi.fn(), + patch: vi.fn(), + use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + _getHandler(method: string, path: string): RouteHandler | undefined { + return routes.get(`${method}:${path}`); + }, + }; +} + +function createMockRes(): IHttpResponse & { _status: number; _json: any; _headers: Record } { + const res: any = { + _status: 200, + _json: null, + _headers: {}, + json(data: any) { res._json = data; }, + send(data: any) { res._sent = data; }, + status(code: number) { res._status = code; return res; }, + header(name: string, value: string) { res._headers[name] = value; return res; }, + }; + return res; +} + +/** + * One in-memory `sys_file` + `sys_attachment` pair, wired to BOTH the metadata + * store the routes read through and the engine seam `findFileHolder` / + * `createSysFileReapGuard` read through. Sharing the tables is the point: the + * download verdict and the sweep verdict must come from the same rows, or the + * pair proves nothing. + */ +function fakeEngine(seed: { + files?: Array>; + attachments?: Array>; +}) { + const tables: Record>> = { + sys_file: (seed.files ?? []).map((r) => ({ ...r })), + sys_attachment: (seed.attachments ?? []).map((r) => ({ ...r })), + }; + const matches = (row: Record, where: Record) => + Object.entries(where).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + return String(row[k]) === String(v); + }); + const engine = { + tables, + async find(object: string, options: any) { + const rows = (tables[object] ?? []).filter((r) => matches(r, options?.where ?? {})); + return typeof options?.limit === 'number' ? rows.slice(0, options.limit) : rows; + }, + async findOne(object: string, options: any) { + return (tables[object] ?? []).find((r) => matches(r, options?.where ?? {})) ?? null; + }, + async insert(object: string, data: any) { + (tables[object] ??= []).push({ ...data }); + return data; + }, + async update(object: string, data: any, options?: any) { + assertEngineUpdateDispatch(data, options); + const row = (tables[object] ?? []).find((r) => String(r.id) === String(data.id)); + if (row) Object.assign(row, data); + return row; + }, + registerHook() { /* unused here */ }, + }; + return engine; +} + +describe('#10246 — tombstoned sys_file with a live holder is downloadable', () => { + let rootDir: string; + let adapter: LocalStorageAdapter; + + beforeEach(async () => { + rootDir = join(tmpdir(), `os-10246-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await fs.mkdir(rootDir, { recursive: true }); + adapter = new LocalStorageAdapter({ rootDir, signingSecret: 'test-secret' }); + }); + + afterEach(async () => { + if (rootDir) await fs.rm(rootDir, { recursive: true, force: true }); + }); + + /** + * A tombstoned attachments-scope file, its bytes on disk, plus whatever + * holders the case asks for. `wireHolder: false` models a bare kernel (no + * data engine) — the routes then get no `resolveFileHolder` at all. + */ + const scenario = async (opts: { + file?: Record; + attachments?: Array>; + wireHolder?: boolean; + authorizeFileRead?: any; + resolveFileHolder?: any; + }) => { + const file = { + id: 'f-10246', + key: 'attachments/f-10246.bin', + name: 'contract.bin', + scope: 'attachments', + acl: 'private', + status: 'deleted', + deleted_at: new Date().toISOString(), + ...opts.file, + }; + const engine = fakeEngine({ files: [file], attachments: opts.attachments ?? [] }); + await adapter.upload(String(file.key), Buffer.from('the bytes')); + + const store = new StorageMetadataStore(engine as any); + const server = createMockHttpServer(); + registerStorageRoutes(server as any, adapter, store, { + basePath: '/api/v1/storage', + authorizeFileRead: opts.authorizeFileRead, + resolveFileHolder: + opts.resolveFileHolder ?? + (opts.wireHolder === false + ? undefined + : (f: any) => findFileHolder(engine as any, f.id, f)), + }); + + const get = async (path: string, fileId = String(file.id)) => { + const res = createMockRes(); + const req: IHttpRequest = { params: { fileId }, query: {}, body: undefined, headers: {}, method: 'GET', path } as any; + await server._getHandler('GET', path)!(req, res); + return res; + }; + + const sweep = async () => { + const guard = createSysFileReapGuard(engine as any, () => adapter, silentLogger()); + const row = engine.tables.sys_file.find((r) => String(r.id) === String(file.id))!; + return guard('sys_file', [{ ...row }]); + }; + + return { engine, store, get, sweep, fileId: String(file.id), key: String(file.key) }; + }; + + // ── The pair. Both halves, one fixture shape, both download endpoints. ── + + it('serves a tombstoned file that still has a live join row — on BOTH download endpoints', async () => { + const { get } = await scenario({ + attachments: [{ id: 'att-1', file_id: 'f-10246', parent_object: 'project', parent_id: 'p1' }], + }); + + const url = await get(DOWNLOAD_ROUTES[0]); + expect(url._status).toBe(200); + expect(url._json.data.url).toContain('/_local/raw/'); + + const redirect = await get(DOWNLOAD_ROUTES[1]); + expect(redirect._status).toBe(302); + expect(redirect._headers.Location ?? redirect._headers.location).toContain('/_local/raw/'); + }); + + it('and the sweep AGREES: the same row is vetoed and un-tombstoned, never reaped', async () => { + const { engine, sweep, key } = await scenario({ + attachments: [{ id: 'att-1', file_id: 'f-10246', parent_object: 'project', parent_id: 'p1' }], + }); + + expect(await sweep()).toEqual([]); // vetoed — nothing confirmed for deletion + expect(await adapter.exists(key)).toBe(true); + // Revival stays the sweep guard's alone — this is the ONLY writer. + const row = engine.tables.sys_file[0]; + expect(row.status).toBe('committed'); + expect(row.deleted_at).toBeNull(); + }); + + it('404s again the moment the LAST holder goes — and the sweep then really reaps', async () => { + const { get, sweep, engine, key, fileId } = await scenario({ attachments: [] }); + + for (const path of DOWNLOAD_ROUTES) { + const res = await get(path); + expect(res._status, path).toBe(404); + expect(res._json?.error?.code, path).toBe('FILE_NOT_FOUND'); + } + + expect(await sweep()).toEqual([fileId]); // confirmed for deletion + expect(await adapter.exists(key)).toBe(false); // bytes reclaimed first + expect(engine.tables.sys_file[0].status).toBe('deleted'); // never revived + }); + + // ── The union's other limb: a re-claimed field-owned tombstone. ───────── + + it('serves a tombstone held through the ref_* ownership columns, matching the guard', async () => { + const { get, sweep } = await scenario({ + file: { scope: 'avatars', ref_object: 'sys_user', ref_id: 'u1', ref_field: 'image' }, + attachments: [], + }); + + expect((await get(DOWNLOAD_ROUTES[0]))._status).toBe(200); + expect(await sweep()).toEqual([]); // the guard vetoes this row too + }); + + it('refuses a released field file — ref_* cleared is exactly what the guard reaps on', async () => { + const { get } = await scenario({ + file: { scope: 'avatars', ref_object: null, ref_id: null, ref_field: null }, + attachments: [], + }); + + expect((await get(DOWNLOAD_ROUTES[0]))._status).toBe(404); + }); + + // ── Boundaries that must NOT have moved. ─────────────────────────────── + + it('leaves `pending` refused — only the tombstone limb widened', async () => { + const { get } = await scenario({ + file: { status: 'pending', deleted_at: null }, + attachments: [{ id: 'att-1', file_id: 'f-10246' }], + }); + + for (const path of DOWNLOAD_ROUTES) { + const res = await get(path); + expect(res._status, path).toBe(404); + expect(res._json?.error?.code, path).toBe('FILE_NOT_FOUND'); + } + }); + + it('refuses a tombstone on a bare kernel — no engine wired, no holder question to ask', async () => { + const { get } = await scenario({ + wireHolder: false, + attachments: [{ id: 'att-1', file_id: 'f-10246' }], + }); + + expect((await get(DOWNLOAD_ROUTES[0]))._status).toBe(404); + }); + + it('refuses rather than falls open when the holder question itself throws', async () => { + const { get } = await scenario({ + resolveFileHolder: vi.fn(async () => { throw new Error('engine down'); }), + attachments: [{ id: 'att-1', file_id: 'f-10246' }], + }); + + const res = await get(DOWNLOAD_ROUTES[0]); + expect(res._status).toBe(404); + expect(res._json?.error?.code).toBe('FILE_NOT_FOUND'); + }); + + it('still applies the download authorization gate to a served tombstone', async () => { + // Servability is not authorization: widening the first must not open the + // second. A caller who cannot read the parent record gets the same 403 a + // committed file would have given. + const denied = await scenario({ + authorizeFileRead: vi.fn(async () => 'deny'), + attachments: [{ id: 'att-1', file_id: 'f-10246', parent_object: 'project', parent_id: 'p1' }], + }); + const res = await denied.get(DOWNLOAD_ROUTES[0]); + expect(res._status).toBe(403); + expect(res._json?.error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); + + const anon = await scenario({ + authorizeFileRead: vi.fn(async () => 'unauthenticated'), + attachments: [{ id: 'att-2', file_id: 'f-10246', parent_object: 'project', parent_id: 'p1' }], + }); + expect((await anon.get(DOWNLOAD_ROUTES[1]))._status).toBe(401); + }); +}); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index e92c29611a..9a895cd594 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -1850,6 +1850,11 @@ "file": "packages/services/service-storage/src/stranded-orphan-inventory.test.ts", "verb": "delete", "pinned": 1 + }, + { + "file": "packages/services/service-storage/src/tombstone-download-live-reference.test.ts", + "verb": "update", + "pinned": 1 } ] }