diff --git a/docs/qa/platform-checklist/areas/attachments-storage.json b/docs/qa/platform-checklist/areas/attachments-storage.json index 56317a8c7a..0c199ba436 100644 --- a/docs/qa/platform-checklist/areas/attachments-storage.json +++ b/docs/qa/platform-checklist/areas/attachments-storage.json @@ -141,7 +141,7 @@ "title": "Gated downloads: 401 anonymous, 403 parent-invisible, signed URL for the entitled — per gating class (attachments-scope / field-owned / public_read opt-out / ungated)", "since": "v15.1", "status": "active", - "revision": 2, + "revision": 3, "priority": "P1", "surface": "mixed", "personas": ["admin (uploader/owner)", "signed-up member who cannot read the parent record", "anonymous"], @@ -155,8 +155,9 @@ "why": "qa_vault (private + files) is the invisible parent both 403 clauses need, qa_shared is the entitled contrast, and the two personas are the non-admin principals the deny side must be checked as. Without the recipe both deny clauses are blocked(fixture) on stock seeds." }, "knownGaps": [ + "MARKER (QA run #9401 finding 5, carried here so runs stop re-deriving it): the CROSS-TENANT clauses of this area are structurally unprovable in the OSS workspace. `@objectstack/organizations` is cloud-private, so the dogfood matrix's cross-tenant block is gated behind `describe.skipIf(!organizationsAvailable)` and skips BY DESIGN here. That is blocked(dependency), never a pass and never a defect. A run that is supposed to ship the package sets OS_TEST_MULTI_ORG_ENABLED=1, which turns the skip into a hard failure instead of a silent one.", "CLOSED by the qa-scratch-authz recipe (#7670): stock showcase's only files-enabled object (showcase_project) is public_read_write, so no stock parent is invisible to any member. The 403 deny side is otherwise reachable only through the pinned dogfood fixture (att_secret, private owner-scoped: fixtures/attachments-fixture.ts). Run the recipe; fall back to the dogfood pin only if it cannot be provisioned, and record which one the verdict rests on.", - "no stock seed mints an acl='public_read' attachments file (the anonymous-embed opt-out) — author one via a system write on a qa_vault-attached file (the recipe gives you the parent; the acl flip is still a system write) or accept the unit-test coverage in storage-routes.test.ts for that variant" + "[API half CLOSED by #9483] no stock seed mints an acl='public_read' attachments file (the anonymous-embed opt-out) — author one via a system write on a qa_vault-attached file (the recipe gives you the parent; the acl flip is still a system write) or accept the unit-test coverage in storage-routes.test.ts for that variant. What changed: attachments-public-read-acl.dogfood.test.ts performs that system flip at runtime on its own private-parent fixture and asserts both sides plus the flip back, so clause 3 no longer waits on a stock seed. Landing one in the showcase seeds would additionally unblock the BROWSER half (clause 4), which a runtime-only file cannot reach." ] }, "steps": [ @@ -205,7 +206,7 @@ "a deny that surfaces in the panel as 'Download failed (403)' instead of the mapped copy means the error-envelope dialect broke (#3689 note in the panel) — file it against objectui, not storage" ], "traps": ["wrong-persona", "dispatcher-vs-hono-route", "stale-console-bundle"], - "automated": { "kind": "api", "ref": "packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts" }, + "automated": { "kind": "api", "ref": "packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts (the CLOSED side — clauses 0, 1 and 2: anonymous 401 AUTH_REQUIRED on both download routes, 403 ATTACHMENT_DOWNLOAD_DENIED for an authenticated member who cannot read the parent, and the uploader/entitled 200 in the {success, data:{url}} envelope) + packages/qa/dogfood/test/attachments-public-read-acl.dogfood.test.ts (clause 3, the OPEN side, asserted on ONE attachments-scope file attached to a private parent: the anonymous capability URL is 401 BEFORE the flip, 302 AFTER acl='public_read' — and the redirect is FOLLOWED anonymously and the served bytes compared, because a 302 into a wall would satisfy a status-only assertion — the /url sibling likewise reopens in its declared envelope, and flipping the acl back to 'private' re-closes both routes, which is what shows the acl VALUE is the cause rather than state accrued by the earlier requests. A sibling file on the SAME parent with the same uploader and scope keeps its 401/403, so the opt-out is per FILE and not a global fail-open. The TTL is read back out of the minted capability URL to prove WHICH branch ran: public_read returns presignedTtl (measured >3000s) while an authorized gated grant returns downloadTtl (measured <=300s) — a distinction no status code exposes, so a widened gate admitting the anonymous caller through the AUTHORIZED path would still be caught. The field-owned gating class (ref_object/ref_id, ADR-0104 D3 wave 2) is covered on both sides too, including its distinct FILE_DOWNLOAD_DENIED deny code, which is the evidence that the field-owned arm — not the attachments arm — is the one that ran). STILL MANUAL: clause 4, the browser's friendly denial copy in objectui's RecordAttachmentsPanel, is a screenshot oracle over a console bundle this repo does not build. It has NO pin, must be hand-driven, and the clause-3 pin must NOT be read as covering it." }, "source": [ "docs/plans/release-15.1-test-plan.md §A12 / §C1 (#2755/#2970)", "packages/services/service-storage/src/storage-routes.ts (authorizeDownload: gating classes, verdict→status mapping, downloadTtl vs presignedTtl)", @@ -214,7 +215,13 @@ ], "history": [ { "revision": 1, "date": "2026-08-07", "change": "new item enumerating the download gating classes straight from authorizeDownload's source (attachments-scope / field-owned / public_read opt-out) with the exact deny codes and TTL contrast", "ref": "claude/platform-test-checklist-ocwugl" }, - { "revision": 2, "date": "2026-08-11", "change": "opted into the new area-level qa-scratch-authz provisioning recipe (#7670): the standing 'no private files-enabled parent on stock showcase' gap that would force blocked(fixture) on both 403 clauses is now payable at runtime instead of being rediscovered every sweep. knownGaps records it as CLOSED-by-recipe with the dogfood pin as the named fallback, and asks the run to say which of the two its verdict rests on", "ref": "#7670" } + { "revision": 2, "date": "2026-08-11", "change": "opted into the new area-level qa-scratch-authz provisioning recipe (#7670): the standing 'no private files-enabled parent on stock showcase' gap that would force blocked(fixture) on both 403 clauses is now payable at runtime instead of being rediscovered every sweep. knownGaps records it as CLOSED-by-recipe with the dogfood pin as the named fallback, and asks the run to say which of the two its verdict rests on", "ref": "#7670" }, + { + "revision": 3, + "date": "2026-08-18", + "change": "clause 3 (acl='public_read' reopens the anonymous capability URL) pinned — QA run #9401 scored this item partial with the OPEN side of the anonymous gate unpinned, which is the half a deny-only suite cannot see: a download-authz suite made only of refusals stays green on a surface that has started refusing everything. automated.ref now names the both-sides pin, records that the TTL branch is read out of the minted URL so the public_read early return is distinguishable from an authorized grant, and states that clause 4 (the panel's denial copy) stays manual with no pin. knownGaps marks the public_read seed gap CLOSED for the API half only, and carries the run's cross-tenant marker so it is not re-derived every wave.", + "ref": "#9483" + } ] }, { @@ -222,7 +229,7 @@ "title": "sys_attachment reads inherit parent visibility: a restricted member sees neither rows nor counts for invisible parents", "since": "v15.1", "status": "active", - "revision": 2, + "revision": 3, "priority": "P1", "surface": "api", "personas": ["member who can read the parent record", "member who cannot read the parent record"], @@ -236,6 +243,7 @@ "why": "the whole item turns on a parent one persona cannot read; qa_vault (private + files) is that parent, and persona B is the restricted member. On stock seeds there is no such pair." }, "knownGaps": [ + "MARKER (QA run #9401 finding 5, carried here so runs stop re-deriving it): the CROSS-TENANT clauses of this area are structurally unprovable in the OSS workspace. `@objectstack/organizations` is cloud-private, so the dogfood matrix's cross-tenant block is gated behind `describe.skipIf(!organizationsAvailable)` and skips BY DESIGN here. That is blocked(dependency), never a pass and never a defect. A run that is supposed to ship the package sets OS_TEST_MULTI_ORG_ENABLED=1, which turns the skip into a hard failure instead of a silent one.", "CLOSED by the qa-scratch-authz recipe (#7670): stock showcase's files-enabled parent (showcase_project) is public_read_write — every member sees every project, so the invisible-parent case is not demonstrable on stock seeds, and the pinned dogfood matrix proves it only on its own private fixture (att_secret). The recipe provisions the private files-enabled parent at runtime; landing one in the showcase seeds proper would retire this gap entirely, and remains the better long-term fix.", "the READ_SCAN_LIMIT fail-closed branch (clause 4) needs a broad read past the 2000-candidate cap — run #7635 reached it by bulking to 2100 join rows on the scratch parent. Budget for that write volume, or the clause degrades to 'the warning never appeared', which is indistinguishable from a silent leak." ] @@ -279,7 +287,7 @@ "rows filtered but `total` counting the raw table is equally a FAIL (the middleware-not-hook design note in attachment-access-hooks.ts exists because a find-hook leaves count() unfiltered)" ], "traps": ["wrong-persona"], - "automated": { "kind": "api", "ref": "packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts" }, + "automated": { "kind": "api", "ref": "packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts (clauses 0 and 2: the restricted member's scoped list is empty for an invisible parent while the entitled one is not, and a by-id read of the hidden row is 403/404) + packages/qa/dogfood/test/attachments-parent-rls-count-parity.dogfood.test.ts (clause 1, the COUNT: the restricted member's list `total` excludes invisible-parent rows while the entitled caller's includes them on the same request, engine.count() under the member's OWN resolved authz context (resolveAuthzContext, not a hand-built principal) is filtered identically, a count NARROWED to the invisible parent answers 0 for the member and the true number for admin, and paging the member all the way through yields exactly the reported total — so rows-vs-count parity is asserted as one fact rather than two numbers that happen to agree. ⚠ THE TRAP, and why every assertion passes $top: protocol.findData only calls engine.count() when the request carries a PAGE LIMIT; without one it sets total = records.length, which is trivially self-consistent and stays green with count() fully unfiltered because the count path never runs. Each assertion therefore also requires total > records.length, proving the count call happened rather than assuming it) + packages/qa/dogfood/test/attachments-parent-rls-scan-cap.dogfood.test.ts (clause 3, the READ_SCAN_LIMIT half: with >2000 candidate join rows the broad read's pre-scan is truncated, and the pin asserts the '[storage] attachment read visibility ... 2000-row cap ... fail-closed' warning fires (the clause's own log oracle — silence plus leaked rows is the failure), that visible rows are DROPPED rather than the un-scanned remainder being admitted, that the invisible-parent row never appears, that every row returned has a genuinely resolvable visible parent, and that a row the broad read omitted is still returned by a SCOPED read that does not hit the cap — the contrast without which 'missing' would be equally explained by 'invisible anyway'. The seed puts visible rows at BOTH ENDS of the insertion order with the filler bulk between, because the pre-scan has no ORDER BY and the driver was measured NOT to return insertion order; the control asserts the window is full and some visible parent is outside it rather than trusting either). ALSO PINNED, as the reachable half of clause 3's fail-compute arm: a row whose parent RECORD no longer resolves (a dangling parent_id, what a hard-deleted parent leaves behind) is excluded from both rows and count. NOT REACHED, and not to be read as covered: (a) the unknown parent_OBJECT arm (`catch { visible = [] }`) has no reachable fixture from this lane — the #2727 enable.files gate refuses to create a sys_attachment row against a non-files object even under system context (measured: \"File attachments are not enabled for object 'att_ghost_object'\"), so such a row cannot be seeded; (b) the middleware-level outer catch ('filter failed, denying all') is not provokable through the API, since a where malformed enough to break the system pre-scan also fails the caller's own read and surfaces as a 4xx instead." }, "source": [ "docs/plans/release-15.1-test-plan.md §C3 (#2970)", "packages/services/service-storage/src/attachment-access-hooks.ts (installAttachmentReadVisibility: middleware over find/findOne/count/aggregate, deny-all sentinel, READ_SCAN_LIMIT fail-closed cap)", @@ -287,7 +295,13 @@ ], "history": [ { "revision": 1, "date": "2026-08-07", "change": "new item grounded in the read-visibility middleware source (count-parity rationale, fail-closed sentinel and scan cap) and the dogfood matrix clause (c)", "ref": "claude/platform-test-checklist-ocwugl" }, - { "revision": 2, "date": "2026-08-11", "change": "opted into the area-level qa-scratch-authz provisioning recipe (#7670) — the invisible-parent pair this item is entirely about is now provisionable at runtime rather than blocked on stock seeds. Added the scan-cap volume note run #7635 paid for (2100 join rows to reach READ_SCAN_LIMIT), so clause 4 is not silently degraded to an unreached branch next run", "ref": "#7670" } + { "revision": 2, "date": "2026-08-11", "change": "opted into the area-level qa-scratch-authz provisioning recipe (#7670) — the invisible-parent pair this item is entirely about is now provisionable at runtime rather than blocked on stock seeds. Added the scan-cap volume note run #7635 paid for (2100 join rows to reach READ_SCAN_LIMIT), so clause 4 is not silently degraded to an unreached branch next run", "ref": "#7670" }, + { + "revision": 3, + "date": "2026-08-18", + "change": "clauses 1 and 3 pinned — QA run #9401 scored this item partial with the COUNT and the fail-closed cap unpinned. A count leak is a real RLS leak: `total` comes from engine.count(), not the find path, which is exactly why the visibility rule is a data middleware over find/findOne/count/aggregate rather than a find hook, and a suite reading only `records` cannot tell the two apart. automated.ref now names the count-parity and scan-cap pins, records the $top trap that makes a count assertion vacuous without a page limit, and enumerates the two fail-closed arms that remain unreachable from this lane so neither is read as covered. Ablation-verified: dropping 'count' from READ_OPS turns 5 of the 7 count-parity tests red and leaves the row-only assertions green.", + "ref": "#9483" + } ] }, { @@ -295,7 +309,7 @@ "title": "Attaching requires EDIT on the parent record; deleting requires uploader-or-parent-editor; unscoped multi-delete is refused outright", "since": "v15.1", "status": "active", - "revision": 2, + "revision": 3, "priority": "P1", "surface": "api", "personas": ["member with EDIT on the parent", "member with READ but not EDIT on the parent", "member who uploaded the attachment", "member who is neither uploader nor parent editor"], @@ -310,6 +324,8 @@ "why": "persona A is provisioned read-but-NOT-edit precisely for the attach gate (the whole point of clause 0: read inherits parent READ, attach requires parent EDIT — two different gates), and qa_nofiles is the purpose-built FILES_DISABLED probe target for the last clause." }, "knownGaps": [ + "MARKER (QA run #9401 finding 5, carried here so runs stop re-deriving it): the CROSS-TENANT clauses of this area are structurally unprovable in the OSS workspace. `@objectstack/organizations` is cloud-private, so the dogfood matrix's cross-tenant block is gated behind `describe.skipIf(!organizationsAvailable)` and skips BY DESIGN here. That is blocked(dependency), never a pass and never a defect. A run that is supposed to ship the package sets OS_TEST_MULTI_ORG_ENABLED=1, which turns the skip into a hard failure instead of a silent one.", + "clause 3 ('unscoped multi-delete refused outright', #4757) is NOT satisfied by the shipped engine — see #9719 and automated.ref. Do not tick it from the unit test in attachment-access-hooks.test.ts: that suite calls the handler directly with an operation-level context the engine's per-row dispatch no longer produces, so it is green while the wired path does the opposite. The REST lane cannot express the shape at all (DeleteManyDataRequestSchema strips options.where and requires ids, #3897), so the reachable callers are server-side engine holders: flows, actions, scripts, MCP tools.", "the FILES_DISABLED clause currently probes showcase_account, which is files-disabled only incidentally — if the showcase ever enables files on it, the clause goes green for the wrong reason. The recipe's qa_nofiles exists to be probed instead; prefer it, and treat showcase_account as the fallback." ] }, @@ -358,7 +374,7 @@ "a rejected write that still created the sys_attachment row (verify by re-listing) is a FAIL — the rejection must be authoritative, not cosmetic" ], "traps": ["wrong-persona"], - "automated": { "kind": "api", "ref": "packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts" }, + "automated": { "kind": "api", "ref": "packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts (clauses 0, 1, 2 and 4: attach without parent EDIT is 403 ATTACHMENT_PARENT_ACCESS while the same member's LIST still succeeds, uploaded_by is server-stamped over a spoofed value, delete splits uploader-vs-outsider, and FILES_DISABLED) + packages/qa/dogfood/test/attachments-unscoped-delete-gate.dogfood.test.ts (the delete gate under an UNSCOPED multi-delete AST, both sides: a `{ multi: true }` delete with neither id nor where cannot slip past the per-row gate and removes NOTHING — not even the one row the caller WAS entitled to, so the refusal is authoritative rather than cosmetic and cannot land as a partial delete; `where: {}` behaves identically, since an empty predicate is still every row; a predicate that MATCHES NOTHING is allowed through, which is the live half of the #4757 reasoning that 'nothing matched' is not a refusal; and the same caller's SCOPED delete of their own row succeeds, without which every assertion here would be satisfied by a delete path that is simply broken. The member is granted the sys_attachment delete bit first and that grant is asserted, because otherwise RBAC refuses ahead of the attachment hook and the whole file would be green for a reason it is not about). ⛔ CLAUSE 3 IS NOT PINNED. Measured while writing the above: the #4757 'refused outright' rule does NOT fire through ObjectQL.delete. The engine's predicate path dispatches beforeDelete PER ROW (dispatchPerRowBeforeHooks builds `input: { id: rowId, options }`), so the handler always takes its by-id branch and never reaches the `where === undefined` check; a caller who is the uploader of every matched row deletes the whole table with a predicate-less delete (measured: 2 rows before, resolves, 0 rows after). attachment-access-hooks.test.ts pins the refusal by calling the handler directly with a whole-operation context the engine no longer produces on this path, so that unit green is not evidence about the wired behaviour. Tracked as a PRODUCT gap in #9719; clause 3 is deliberately left unpinned rather than pinned against current behaviour, because pinning today's outcome would turn the eventual fix red." }, "source": [ "docs/plans/release-15.1-test-plan.md §A12 (attach 需 parent EDIT)", "packages/services/service-storage/src/attachment-access-hooks.ts (beforeInsert canEdit gate + uploaded_by stamping; beforeDelete uploader-or-editor + #4757 unscoped refusal, MULTI_DELETE_AUTH_LIMIT fail-closed)", @@ -366,7 +382,13 @@ ], "history": [ { "revision": 1, "date": "2026-08-07", "change": "new item from the access-hook source: EDIT-not-read attach gate, server stamping, delete authorization matrix and the #4757 unscoped-delete refusal, each with its exact deny code", "ref": "claude/platform-test-checklist-ocwugl" }, - { "revision": 2, "date": "2026-08-11", "change": "opted into the area-level qa-scratch-authz provisioning recipe (#7670): persona A is provisioned read-but-not-edit for the attach gate, and qa_nofiles is named as the purpose-built FILES_DISABLED probe target instead of showcase_account, which is files-disabled only incidentally and would go green for the wrong reason if the showcase ever enabled files on it. Clause 3's stale-@objectstack/core pin problem from run #7635 is deliberately NOT touched here — it is owned by #7668", "ref": "#7670" } + { "revision": 2, "date": "2026-08-11", "change": "opted into the area-level qa-scratch-authz provisioning recipe (#7670): persona A is provisioned read-but-not-edit for the attach gate, and qa_nofiles is named as the purpose-built FILES_DISABLED probe target instead of showcase_account, which is files-disabled only incidentally and would go green for the wrong reason if the showcase ever enabled files on it. Clause 3's stale-@objectstack/core pin problem from run #7635 is deliberately NOT touched here — it is owned by #7668", "ref": "#7670" }, + { + "revision": 3, + "date": "2026-08-18", + "change": "the unscoped-multi-delete surface pinned on the halves that hold, and clause 3 recorded as NOT pinned with the measurement. QA run #9401 listed clause 3 as the gap; implementing it found the #4757 refusal is unreachable through ObjectQL.delete because beforeDelete is dispatched per row with input.id bound, so the guard's own branch never runs and an uploader-of-every-row wipes the table. Filed as #9719. The new pin asserts what does hold — an unscoped AST cannot bypass the per-row gate, deletes nothing (no partial delete), an empty where is treated as every row, a zero-match predicate is allowed through, and the same caller's scoped delete still succeeds — and the ref says plainly that clause 3 stays unpinned rather than being pinned against behaviour a fix will change.", + "ref": "#9483" + } ] }, { diff --git a/packages/qa/dogfood/test/attachments-parent-rls-count-parity.dogfood.test.ts b/packages/qa/dogfood/test/attachments-parent-rls-count-parity.dogfood.test.ts new file mode 100644 index 0000000000..dceac1819d --- /dev/null +++ b/packages/qa/dogfood/test/attachments-parent-rls-count-parity.dogfood.test.ts @@ -0,0 +1,254 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// attachments-storage.read-inherits-parent-rls — clause C1, plus the +// resolution-failure half of C3. +// +// QA run #9401 scored this item PARTIAL: the pinned matrix proves the ROWS of a +// restricted member's `sys_attachment` list exclude invisible parents, and stops +// there. C1 is the COUNT — and a count leak is a real RLS leak: `total` comes +// from `engine.count()`, NOT from the find path, which is precisely why the +// visibility rule is a data MIDDLEWARE (find/findOne/count/aggregate) and not a +// find hook. A suite that only reads `records` cannot see the difference, and +// would stay green with `count()` unfiltered — leaking the true row count of +// records the caller may not read. +// +// ## The trap this pin had to step around +// +// `total` is only computed by `engine.count()` when the request carries a PAGE +// LIMIT. Without one, `protocol.findData` sets `total = records.length` — so a +// list issued with no `$top` reports a `total` that is *trivially* consistent +// with its rows, and an assertion on it proves nothing at all: it stays green +// with the count path fully unfiltered, because the count path never runs. +// Every REST assertion below therefore passes `$top`, and asserts a `total` +// STRICTLY GREATER than the rows returned, so the count call is proven to have +// happened rather than assumed. +// +// @proof: attachments-parent-rls-count-parity + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { VerifyStack } from '@objectstack/verify'; +import { resolveAuthzContext } from '@objectstack/core'; +import { + bootAttachmentsHarness, + stopAttachmentsHarness, + uploadFile, + FILE_BYTES, + type AttachmentsHarness, +} from './fixtures/attachments-authz-harness.js'; + +const SYS = { isSystem: true } as const; + +/** Attachments on the parent the restricted member CAN read. */ +const VISIBLE_COUNT = 4; +/** Attachments on the admin-owned private parent they CANNOT read. */ +const HIDDEN_COUNT = 3; +/** Page size — small enough that `total` must exceed the rows on the page. */ +const PAGE = 2; + +describe('sys_attachment reads inherit parent RLS — the COUNT too (#9483)', () => { + let harness: AttachmentsHarness; + let stack: VerifyStack; + let ql: any; + let adminTok: string, memberTok: string; + let adminId: string; + let caseId: string; + let secretId: string; + + const uid = async (email: string) => + (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; + + /** The SAME authz context the REST entry point resolves — real positions and + * permission sets off the live tables, never a hand-built principal. */ + const authzFor = async (token: string) => { + const authService: any = await stack.kernel.getServiceAsync('auth'); + let api: any = authService?.api; + if (!api && typeof authService?.getApi === 'function') api = await authService.getApi(); + const headers = new Headers({ authorization: `Bearer ${token}` }); + return resolveAuthzContext({ + ql, + headers, + getSession: async (h: any) => api?.getSession?.({ headers: h }), + }); + }; + + /** Attach through the REAL route as admin (who can edit both parents). */ + const attach = async (parentObject: string, parentId: string) => { + const fileId = await uploadFile(stack, adminTok); + const res = await stack.apiAs(adminTok, 'POST', '/data/sys_attachment', { + parent_object: parentObject, + parent_id: parentId, + file_id: fileId, + file_name: 'hello.txt', + mime_type: 'text/plain', + size: FILE_BYTES.length, + }); + expect(res.status, `attach to ${parentObject}`).toBeLessThan(300); + return fileId; + }; + + const listAs = async (token: string, query: string) => { + const res = await stack.apiAs(token, 'GET', `/data/sys_attachment${query}`); + expect(res.status).toBe(200); + return (await res.json()) as { records: any[]; total: number }; + }; + + beforeAll(async () => { + harness = await bootAttachmentsHarness(); + stack = harness.stack; + adminTok = await stack.signIn(); + memberTok = await stack.signUp('att-count-member@verify.test'); + ql = await stack.kernel.getServiceAsync('objectql'); + adminId = await uid('admin@objectos.ai'); + + // Visible parent: att_case is public_read_write — every member reads it. + const caseRes = await stack.apiAs(adminTok, 'POST', '/data/att_case', { name: 'visible case' }); + expect(caseRes.status).toBeLessThan(300); + const caseBody = (await caseRes.json()) as any; // a Response body reads ONCE + caseId = String(caseBody.id ?? caseBody.record?.id ?? caseBody.data?.id); + + // Invisible parent: att_secret defaults to PRIVATE and is owner-anchored to + // admin, so a fresh member can neither read nor edit it. + const secret = await ql.insert( + 'att_secret', + { name: 'count secret', owner_id: adminId }, + { context: { ...SYS } }, + ); + secretId = secret.id; + + for (let i = 0; i < VISIBLE_COUNT; i++) await attach('att_case', caseId); + for (let i = 0; i < HIDDEN_COUNT; i++) await attach('att_secret', secretId); + }, 180_000); + + afterAll(async () => { + await stopAttachmentsHarness(harness); + }); + + it('control: the seed is what the assertions below assume, and the parents split as intended', async () => { + const all = await ql.count('sys_attachment', { context: { ...SYS } }); + expect(all, 'total rows actually seeded').toBe(VISIBLE_COUNT + HIDDEN_COUNT); + + // The premise every count assertion rests on. If a future fixture change + // made att_secret readable, the leak assertions would pass for the wrong + // reason — so the invisibility is asserted, not assumed. + const readsCase = await stack.apiAs(memberTok, 'GET', `/data/att_case/${caseId}`); + expect(readsCase.status, 'the member CAN read the visible parent').toBe(200); + const readsSecret = await stack.apiAs(memberTok, 'GET', `/data/att_secret/${secretId}`); + expect([403, 404], 'and CANNOT read the private one').toContain(readsSecret.status); + }); + + it('C1 — the restricted member’s list `total` counts only visible-parent rows', async () => { + const body = await listAs(memberTok, `?$top=${PAGE}`); + + // The count path is PROVEN to have run: with a page of 2 and 4 visible + // rows, `total` can only exceed `records.length` if `engine.count()` + // answered it. (Were the count skipped, total would be 2 and this fails.) + expect(body.records.length, 'one page').toBe(PAGE); + expect(body.total, 'total came from engine.count(), not the page').toBeGreaterThan( + body.records.length, + ); + + // The clause itself: the count is filtered IDENTICALLY to the rows. + expect(body.total, 'total must exclude every invisible-parent row').toBe(VISIBLE_COUNT); + expect(body.total, 'and must not be the raw table count').not.toBe( + VISIBLE_COUNT + HIDDEN_COUNT, + ); + }); + + it('C1 — the entitled caller’s total on the same request INCLUDES them (the paired side)', async () => { + // Without this pair the assertion above is satisfied by a surface that + // counts nothing for anybody — a filter that always denies is not a filter. + const body = await listAs(adminTok, `?$top=${PAGE}`); + expect(body.records.length).toBe(PAGE); + expect(body.total, 'admin reads both parents').toBe(VISIBLE_COUNT + HIDDEN_COUNT); + }); + + it('C1 — rows and count agree: paging the member all the way through yields exactly `total`', async () => { + // Rows-vs-count parity stated as one assertion rather than two numbers that + // happen to match: enumerate everything the member can actually reach and + // check the reported total against it. A count filtered by a DIFFERENT + // predicate than the rows fails here even when both are non-zero. + const seen = new Set(); + for (let offset = 0; offset < 50; offset += PAGE) { + const page = await listAs(memberTok, `?$top=${PAGE}&$skip=${offset}`); + if (!page.records.length) break; + for (const r of page.records) seen.add(String(r.id)); + } + const reported = (await listAs(memberTok, `?$top=${PAGE}`)).total; + expect(seen.size, 'rows the member can actually enumerate').toBe(VISIBLE_COUNT); + expect(reported, 'and the total it is told').toBe(seen.size); + }); + + it('C1 — engine.count() under the member’s own resolved context is filtered too', async () => { + // The wire `total` is one consumer of `engine.count()`. This drives the + // middleware's `count` op directly, with the context the REST layer would + // have built, so the pin is on the middleware rather than on one route's + // arithmetic. + const memberCtx = await authzFor(memberTok); + const memberCount = await ql.count('sys_attachment', { context: memberCtx }); + const systemCount = await ql.count('sys_attachment', { context: { ...SYS } }); + + expect(memberCount).toBe(VISIBLE_COUNT); + expect(systemCount).toBe(VISIBLE_COUNT + HIDDEN_COUNT); + expect(memberCount, 'the member is told strictly less than the table holds').toBeLessThan( + systemCount, + ); + }); + + it('C1 — a count NARROWED to the invisible parent answers zero, not the true number', async () => { + // The sharpest shape of the leak: the caller already knows the parent id and + // is asking only "how many". Rows come back empty either way; only the count + // distinguishes a filtered surface from one that answers the raw table. + const memberCtx = await authzFor(memberTok); + const scoped = await ql.count('sys_attachment', { + where: { parent_object: 'att_secret', parent_id: secretId }, + context: memberCtx, + }); + expect(scoped).toBe(0); + + const asAdmin = await ql.count('sys_attachment', { + where: { parent_object: 'att_secret', parent_id: secretId }, + context: await authzFor(adminTok), + }); + expect(asAdmin, 'the same question, answered for someone entitled to it').toBe(HIDDEN_COUNT); + }); + + it('C3 (resolution-failure half) — a row whose parent RECORD no longer resolves is EXCLUDED, not leaked', async () => { + // `computeParentVisibilityFilter` resolves the visible id subset per + // parent_object through the CALLER's context; ids that come back with + // nothing simply never enter the `$in`, so their rows drop out of the + // filter. Fail-closed is the whole point: the alternative reading — "could + // not decide, so allow" — is the leak. A dangling parent_id is not exotic; + // a hard-deleted parent record leaves exactly this behind. + // + // NOTE on what this does NOT reach. The sibling branch — an unknown + // parent_OBJECT, caught by `catch { visible = [] }` — cannot be provoked + // from this lane at all: the #2727 `enable.files` gate refuses to create a + // sys_attachment row against an object that is not files-enabled, even + // under system context (measured: `File attachments are not enabled for + // object 'att_ghost_object'`). So the unknown-object arm has no reachable + // fixture here, and the ref says so rather than implying it is covered. + const before = await ql.count('sys_attachment', { context: { ...SYS } }); + await ql.insert( + 'sys_attachment', + { + parent_object: 'att_case', + parent_id: 'no-such-case-record', + file_id: await uploadFile(stack, adminTok), + file_name: 'dangling.txt', + mime_type: 'text/plain', + size: FILE_BYTES.length, + }, + { context: { ...SYS } }, + ); + expect(await ql.count('sys_attachment', { context: { ...SYS } })).toBe(before + 1); + + const memberCtx = await authzFor(memberTok); + const body = await listAs(memberTok, `?$top=50`); + expect( + body.records.some((r: any) => String(r.parent_id) === 'no-such-case-record'), + 'a parent that resolves to nothing must not admit its row', + ).toBe(false); + expect(body.total, 'and must not be counted either').toBe(VISIBLE_COUNT); + expect(await ql.count('sys_attachment', { context: memberCtx })).toBe(VISIBLE_COUNT); + }); +}); diff --git a/packages/qa/dogfood/test/attachments-parent-rls-scan-cap.dogfood.test.ts b/packages/qa/dogfood/test/attachments-parent-rls-scan-cap.dogfood.test.ts new file mode 100644 index 0000000000..069059ddbe --- /dev/null +++ b/packages/qa/dogfood/test/attachments-parent-rls-scan-cap.dogfood.test.ts @@ -0,0 +1,274 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// attachments-storage.read-inherits-parent-rls — clause C3, the 2000-row +// pre-scan cap. +// +// The read-visibility middleware pre-scans the candidate (parent_object, +// parent_id) pairs a query would touch, bounded at READ_SCAN_LIMIT = 2000. +// Past that bound the filter is built from a TRUNCATED candidate set, and the +// clause is about which way the truncation falls: rows outside the scan window +// are EXCLUDED (the caller may lose rows they could see) rather than admitted +// unfiltered (the caller gains rows they may not). The cap also logs, because a +// silent truncation is indistinguishable from a leak — the item's own wording: +// "silence plus leaked rows is the failure". +// +// ## Why the seed is shaped the way it is +// +// The pre-scan takes 2000 rows in whatever order the driver returns them, with +// no ORDER BY of its own — so which rows land inside the window is NOT a +// property this test may assume. A first attempt seeded the probe row last and +// asserted it fell outside; measured, it fell INSIDE (the control caught it), +// so the driver is not returning insertion order here. +// +// The seed therefore puts visible rows at BOTH ENDS of the insertion sequence +// with the filler bulk between them. Any ordering that takes a contiguous run +// from either end leaves one of the two groups outside the window, so some +// visible row is excluded either way — and the test then picks an actually +// excluded row at RUNTIME rather than predicting which one it will be. The +// control asserts that premise (window full, at least one visible parent +// unscanned) instead of trusting it. +// +// Volume goes in through bulk system inserts: the clause is about the READ +// path, and 2000 real presigned uploads would pay for nothing it tests. The +// filler rows share ONE committed sys_file (ContentDocumentLink semantics — a +// file legitimately carries many join rows). +// +// @proof: attachments-parent-rls-scan-cap + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import type { VerifyStack } from '@objectstack/verify'; +import { + bootAttachmentsHarness, + stopAttachmentsHarness, + uploadFile, + FILE_BYTES, + type AttachmentsHarness, +} from './fixtures/attachments-authz-harness.js'; + +const SYS = { isSystem: true } as const; + +/** READ_SCAN_LIMIT in attachment-access-hooks.ts. Mirrored, not imported: the + * test asserts the SHIPPED bound, so a silent change to it must surface here + * as a failure rather than be followed automatically. */ +const READ_SCAN_LIMIT = 2_000; + +/** Visible attachments seeded BEFORE the filler bulk, and again AFTER it. */ +const VISIBLE_PER_GROUP = 40; +/** Filler rows between them — enough that the two groups cannot both fit. */ +const FILLER = 2_400; + +function captureLog() { + const lines: string[] = []; + const sink = (chunk: unknown): boolean => { + lines.push(String(chunk)); + return true; + }; + const outSpy = vi.spyOn(process.stdout, 'write').mockImplementation(sink as never); + const errSpy = vi.spyOn(process.stderr, 'write').mockImplementation(sink as never); + return { + matching: (needle: string) => lines.filter((l) => l.includes(needle)), + restore: () => { + outSpy.mockRestore(); + errSpy.mockRestore(); + }, + }; +} + +describe('sys_attachment read visibility fails CLOSED past the pre-scan cap (#9483)', () => { + let harness: AttachmentsHarness; + let stack: VerifyStack; + let ql: any; + let adminTok: string, memberTok: string; + let adminId: string; + let sharedFile: string; + let secretId: string; + /** attachment id -> its (visible) parent id, for all 2 x VISIBLE_PER_GROUP. */ + const visibleRows = new Map(); + let hiddenAttachmentId: string; + + const uid = async (email: string) => + (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; + + /** Bulk-seed N visible att_case parents, one attachment each. */ + const seedVisibleGroup = async (tag: string, n: number) => { + const parents = Array.from({ length: n }, (_u, i) => ({ name: `${tag}-parent-${i}` })); + const created = await ql.insertMany('att_case', parents, { context: { ...SYS } }); + const parentIds = created.map((r: any) => String(r.id ?? r.record?.id)); + expect(parentIds.filter(Boolean).length, `${tag} parents created`).toBe(n); + + const rows = parentIds.map((pid: string) => ({ + parent_object: 'att_case', + parent_id: pid, + file_id: sharedFile, + file_name: 'visible.txt', + mime_type: 'text/plain', + size: FILE_BYTES.length, + })); + const made = await ql.insertMany('sys_attachment', rows, { context: { ...SYS } }); + made.forEach((r: any, i: number) => { + const id = String(r.id ?? r.record?.id); + if (id && id !== 'undefined') visibleRows.set(id, parentIds[i]); + }); + }; + + beforeAll(async () => { + harness = await bootAttachmentsHarness(); + stack = harness.stack; + adminTok = await stack.signIn(); + memberTok = await stack.signUp('att-cap-member@verify.test'); + ql = await stack.kernel.getServiceAsync('objectql'); + adminId = await uid('admin@objectos.ai'); + + sharedFile = await uploadFile(stack, adminTok, 'shared.txt'); + + const secret = await ql.insert( + 'att_secret', + { name: 'cap secret', owner_id: adminId }, + { context: { ...SYS } }, + ); + secretId = secret.id; + + // Group A — visible, seeded FIRST. + await seedVisibleGroup('capA', VISIBLE_PER_GROUP); + + // Filler — dangling parent ids, which is what a hard-deleted parent record + // leaves behind. They can widen nobody's visibility; they only consume the + // scan window. + const filler = Array.from({ length: FILLER }, (_u, i) => ({ + parent_object: 'att_case', + parent_id: `cap-ghost-${i}`, + file_id: sharedFile, + file_name: 'shared.txt', + mime_type: 'text/plain', + size: FILE_BYTES.length, + })); + for (let i = 0; i < filler.length; i += 400) { + await ql.insertMany('sys_attachment', filler.slice(i, i + 400), { context: { ...SYS } }); + } + + // The invisible-parent row, and Group B — visible, seeded LAST. + const hidden = await ql.insert( + 'sys_attachment', + { + parent_object: 'att_secret', + parent_id: secretId, + file_id: sharedFile, + file_name: 'secret.txt', + mime_type: 'text/plain', + size: FILE_BYTES.length, + }, + { context: { ...SYS } }, + ); + hiddenAttachmentId = String(hidden.id); + await seedVisibleGroup('capB', VISIBLE_PER_GROUP); + }, 600_000); + + afterAll(async () => { + await stopAttachmentsHarness(harness); + }); + + it('control: the window is full and some visible parent really is outside it', async () => { + // The premise, asserted. An unreached branch reads exactly like a satisfied + // one, so if the seed ever stopped crossing the bound this must fail loudly + // rather than let the clause pass on a code path it never entered. + const total = await ql.count('sys_attachment', { context: { ...SYS } }); + expect(total, 'candidate rows must exceed READ_SCAN_LIMIT').toBeGreaterThan(READ_SCAN_LIMIT); + expect(visibleRows.size).toBe(VISIBLE_PER_GROUP * 2); + + const scanned = await ql.find('sys_attachment', { + where: {}, + fields: ['id'], + limit: READ_SCAN_LIMIT, + context: { ...SYS }, + }); + expect(scanned.length, 'the pre-scan window is exactly the cap').toBe(READ_SCAN_LIMIT); + + const scannedIds = new Set(scanned.map((r: any) => String(r.id))); + const unscannedVisible = [...visibleRows.keys()].filter((id) => !scannedIds.has(id)); + expect( + unscannedVisible.length, + 'at least one visible row must fall outside the window, or the clause is unreachable', + ).toBeGreaterThan(0); + }); + + it('C3 — the capped broad read warns AND fails closed: visible rows are dropped, the hidden one never appears', async () => { + const log = captureLog(); + let body: any; + try { + const res = await stack.apiAs(memberTok, 'GET', '/data/sys_attachment?$top=200'); + expect(res.status).toBe(200); + body = await res.json(); + } finally { + log.restore(); + } + + // 1. The truncation is LOUD. This is the clause's own oracle: silence plus + // leaked rows is the failure, so the warning is what makes a truncated + // answer distinguishable from a complete one. + const warned = log.matching('[storage] attachment read visibility'); + expect(warned.length, 'the cap must announce itself').toBeGreaterThan(0); + expect( + warned.some((l) => l.includes(`${READ_SCAN_LIMIT}-row cap`) && l.includes('fail-closed')), + 'the warning names the cap and the direction it falls', + ).toBe(true); + + // 2. It falls CLOSED: rows the member could otherwise see are excluded, + // rather than the un-scanned remainder being admitted unfiltered. + const returned = new Set(body.records.map((r: any) => String(r.id))); + const returnedVisible = [...visibleRows.keys()].filter((id) => returned.has(id)); + expect( + returnedVisible.length, + 'a truncated scan must return FEWER visible rows than exist, not more', + ).toBeLessThan(visibleRows.size); + + // 3. And the security direction, which must hold under truncation too. + expect(returned.has(hiddenAttachmentId), 'an invisible parent’s row must never appear').toBe( + false, + ); + expect( + body.records.every((r: any) => r.parent_object !== 'att_secret'), + 'no att_secret row under any id', + ).toBe(true); + + // 4. Every row that DID come back is one the member may genuinely see — + // truncation must not admit a dangling-parent row either. + expect( + body.records.every((r: any) => visibleRows.has(String(r.id))), + 'no row with an unresolvable parent is admitted by the truncated filter', + ).toBe(true); + }); + + it('the dropped rows are genuinely visible: a scoped read returns one the broad read omitted', async () => { + // The contrast that makes assertion 2 above mean "excluded by the cap" + // rather than "invisible anyway". Without it, a surface that hid these rows + // from this member for some entirely different reason would satisfy the + // fail-closed assertion while proving nothing about the cap. + const broad = await stack.apiAs(memberTok, 'GET', '/data/sys_attachment?$top=200'); + const records = ((await broad.json()) as any).records as any[]; + + // The page must NOT be full, or "absent from the page" means paged out + // rather than filtered out and the conclusion below does not follow. This + // guard is not decoration: the fail-open ablation of the cap was caught by + // it and by nothing else — with the filter removed the page fills with + // unfiltered rows, every visible row falls off the end, and the test would + // otherwise have gone green while the surface leaked the whole table. + expect(records.length, 'the page is not full, so absence means excluded').toBeLessThan(200); + + const returned = new Set(records.map((r: any) => String(r.id))); + const dropped = [...visibleRows.entries()].find(([id]) => !returned.has(id)); + expect(dropped, 'the broad read dropped at least one visible row').toBeTruthy(); + + const [droppedId, parentId] = dropped as [string, string]; + const scoped = await stack.apiAs( + memberTok, + 'GET', + `/data/sys_attachment?$top=10&parent_object=att_case&parent_id=${parentId}`, + ); + expect(scoped.status).toBe(200); + const rows = ((await scoped.json()) as any).records; + expect( + rows.some((r: any) => String(r.id) === droppedId), + 'the same row, reachable when the read does not hit the cap', + ).toBe(true); + }); +}); diff --git a/packages/qa/dogfood/test/attachments-public-read-acl.dogfood.test.ts b/packages/qa/dogfood/test/attachments-public-read-acl.dogfood.test.ts new file mode 100644 index 0000000000..bb9065fdff --- /dev/null +++ b/packages/qa/dogfood/test/attachments-public-read-acl.dogfood.test.ts @@ -0,0 +1,229 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// attachments-storage.download-authz-both-sides — clause C3. +// +// QA run #9401 scored this item PARTIAL: the pinned matrix proves the CLOSED +// side of the anonymous gate (401 anonymous, 403 parent-invisible, 200 for the +// entitled) and nothing else. C3 is the OPEN side — `acl: 'public_read'` opts a +// gated file back out to the stable anonymous capability URL, the explicit +// declaration that exists because `< img src>` cannot carry a bearer token. +// +// Why the positive side is the half that matters here: a download-authz suite +// made only of denials stays green when the surface denies EVERYTHING, which is +// the exact failure a `public_read` regression produces. So every assertion +// below is paired on ONE file — closed, opened, closed again — and the TTL +// branch is read back out of the minted URL so "it opened" cannot be satisfied +// by some other grant path (owner bypass, a widened gate) that happens to also +// answer 200. +// +// C4 (the browser's friendly denial copy, objectui's RecordAttachmentsPanel) is +// NOT pinned here and is not pinnable from this lane — it is a screenshot +// oracle over a console bundle this repo does not build. The item's ref says so +// in as many words. +// +// @proof: attachments-public-read-acl + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { VerifyStack } from '@objectstack/verify'; +import { + bootAttachmentsHarness, + stopAttachmentsHarness, + uploadFile, + toPath, + ttlSecondsOf, + FILE_BYTES, + type AttachmentsHarness, +} from './fixtures/attachments-authz-harness.js'; + +const SYS = { isSystem: true } as const; + +describe('attachments download authz — acl=public_read reopens the anonymous capability URL (#9483)', () => { + let harness: AttachmentsHarness; + let stack: VerifyStack; + let ql: any; + let adminTok: string, memberTok: string; + let adminId: string; + let secretId: string; + + /** The file under test: attachments-scope, attached to a PRIVATE parent. */ + let gatedFile: string; + /** A second file on the same parent, whose acl is never touched. */ + let siblingFile: string; + + const uid = async (email: string) => + (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; + + const setAcl = (fileId: string, acl: 'private' | 'public_read') => + ql.update('sys_file', { acl }, { where: { id: fileId }, context: { ...SYS } }); + + const attach = (token: string, parentId: string, fileId: string) => + stack.apiAs(token, 'POST', '/data/sys_attachment', { + parent_object: 'att_secret', + parent_id: parentId, + file_id: fileId, + file_name: 'hello.txt', + mime_type: 'text/plain', + size: FILE_BYTES.length, + }); + + beforeAll(async () => { + harness = await bootAttachmentsHarness(); + stack = harness.stack; + adminTok = await stack.signIn(); + memberTok = await stack.signUp('att-acl-member@verify.test'); + ql = await stack.kernel.getServiceAsync('objectql'); + adminId = await uid('admin@objectos.ai'); + + // att_secret is the fixture's PRIVATE, owner-anchored parent: a fresh + // member can neither read it nor edit it, so every file hanging off it is + // gated for that member. Admin owns it and uploads both files, so the + // member is never the file owner (the authorizer's owner bypass would + // otherwise answer `allow` before the gate is consulted). + const secret = await ql.insert( + 'att_secret', + { name: 'acl secret', owner_id: adminId }, + { context: { ...SYS } }, + ); + secretId = secret.id; + + gatedFile = await uploadFile(stack, adminTok); + expect((await attach(adminTok, secretId, gatedFile)).status).toBeLessThan(300); + siblingFile = await uploadFile(stack, adminTok, 'sibling.txt'); + expect((await attach(adminTok, secretId, siblingFile)).status).toBeLessThan(300); + }, 120_000); + + afterAll(async () => { + await stopAttachmentsHarness(harness); + }); + + it('BEFORE the flip: the gated file is closed to anonymous on both download routes', async () => { + const file = await ql.findOne('sys_file', { where: { id: gatedFile }, context: SYS }); + expect(file?.scope, 'the file is attachments-scope, i.e. gated').toBe('attachments'); + expect(file?.acl ?? 'private', 'and starts NOT public_read').not.toBe('public_read'); + + const url = await stack.api(`/storage/files/${gatedFile}/url`); + expect(url.status).toBe(401); + expect(((await url.json()) as any).error?.code).toBe('AUTH_REQUIRED'); + + const redirect = await stack.api(`/storage/files/${gatedFile}`); + expect(redirect.status).toBe(401); + expect(((await redirect.json()) as any).error?.code).toBe('AUTH_REQUIRED'); + }); + + it('BEFORE the flip: an authenticated member who cannot read the parent is refused 403', async () => { + const denied = await stack.apiAs(memberTok, 'GET', `/storage/files/${gatedFile}/url`); + expect(denied.status).toBe(403); + expect(((await denied.json()) as any).error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); + }); + + it('AFTER the flip: acl=public_read 302s anonymously to the REAL BYTES, on the same file', async () => { + await setAcl(gatedFile, 'public_read'); + + const redirect = await stack.api(`/storage/files/${gatedFile}`); + expect(redirect.status, 'the stable capability URL is reopened').toBe(302); + const location = redirect.headers.get('location'); + expect(location, 'a 302 with no Location is not a capability URL').toBeTruthy(); + + // Follow it ANONYMOUSLY. A 302 whose target refuses the same caller would + // be a redirect into a wall — the clause is about bytes, not a status. + const bytes = await stack.raw(toPath(String(location))); + expect(bytes.status, 'the redirect target serves the bytes').toBe(200); + expect(await bytes.text()).toBe(FILE_BYTES); + }); + + it('AFTER the flip: the JSON sibling route is reopened too, in its declared envelope', async () => { + const res = await stack.api(`/storage/files/${gatedFile}/url`); + expect(res.status).toBe(200); + const body = (await res.json()) as any; + expect(body.success).toBe(true); + expect(body.data?.url).toBeTruthy(); + + const bytes = await stack.raw(toPath(String(body.data.url))); + expect(bytes.status).toBe(200); + expect(await bytes.text()).toBe(FILE_BYTES); + }); + + it('the opt-out is per FILE: the sibling on the same parent stays closed', async () => { + // Same parent, same uploader, same scope — only `acl` differs. Without + // this pair a global fail-open would satisfy every assertion above. + const anon = await stack.api(`/storage/files/${siblingFile}`); + expect(anon.status).toBe(401); + expect(((await anon.json()) as any).error?.code).toBe('AUTH_REQUIRED'); + + const member = await stack.apiAs(memberTok, 'GET', `/storage/files/${siblingFile}/url`); + expect(member.status).toBe(403); + expect(((await member.json()) as any).error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); + }); + + it('the acl VALUE is the cause: flipping back to private re-closes both routes', async () => { + // The state oracle for a persistence-shaped clause. Everything above could + // in principle be explained by state accrued during the earlier requests + // (a cached verdict, a warmed authorizer); only the round trip back to 401 + // shows the acl column is what the gate reads, every time. + await setAcl(gatedFile, 'private'); + + const redirect = await stack.api(`/storage/files/${gatedFile}`); + expect(redirect.status).toBe(401); + expect(((await redirect.json()) as any).error?.code).toBe('AUTH_REQUIRED'); + + const url = await stack.api(`/storage/files/${gatedFile}/url`); + expect(url.status).toBe(401); + + await setAcl(gatedFile, 'public_read'); // leave it open for the TTL case + }); + + it('public_read takes the UNGATED ttl branch, while an entitled gated caller gets the short one', async () => { + // `authorizeDownload` returns `presignedTtl` (3600s) on the public_read + // early return and `downloadTtl` (300s) after an `allow` verdict. Same + // file, same route, two callers — so a widened gate that let the anonymous + // request through the AUTHORIZED path (rather than the opt-out) would show + // up here as the short TTL, even though the status code stayed 302. + const anon = await stack.api(`/storage/files/${gatedFile}`); + expect(anon.status).toBe(302); + const anonTtl = ttlSecondsOf(String(anon.headers.get('location'))); + + const owner = await stack.apiAs(adminTok, 'GET', `/storage/files/${siblingFile}/url`); + expect(owner.status, 'the uploader may always download').toBe(200); + const ownerTtl = ttlSecondsOf(String(((await owner.json()) as any).data.url)); + + expect(anonTtl, 'public_read → presignedTtl (3600s)').toBeGreaterThan(3000); + expect(ownerTtl, 'an authorized gated grant → downloadTtl (300s)').toBeLessThanOrEqual(300); + expect(anonTtl - ownerTtl, 'the two branches are not the same branch').toBeGreaterThan(1000); + }); + + it('a FIELD-owned file opts out the same way, and its closed side carries the other deny code', async () => { + // The second gating class in `authorizeDownload`: `ref_object`/`ref_id` + // (ADR-0104 D3 wave 2) rather than a sys_attachment join row. Stamping the + // reference is the system write the item's own steps prescribe; what is + // being pinned is the ROUTE's treatment of a field-owned file, and the + // distinct deny code is the evidence that the field-owned branch — not the + // attachments branch — is the one that ran. + const fieldFile = await uploadFile(stack, adminTok, 'receipt.txt'); + await ql.update( + 'sys_file', + // `scope` moves off `attachments` so this is a PURELY field-owned file: + // it carries no sys_attachment join row, and the attachments-scope arm of + // `gated` cannot be what admits it. + { ref_object: 'att_secret', ref_id: secretId, scope: 'private' }, + { where: { id: fieldFile }, context: { ...SYS } }, + ); + + const anonBefore = await stack.api(`/storage/files/${fieldFile}`); + expect(anonBefore.status).toBe(401); + expect(((await anonBefore.json()) as any).error?.code).toBe('AUTH_REQUIRED'); + + const memberBefore = await stack.apiAs(memberTok, 'GET', `/storage/files/${fieldFile}/url`); + expect(memberBefore.status).toBe(403); + expect( + ((await memberBefore.json()) as any).error?.code, + 'field-owned denies say FILE_DOWNLOAD_DENIED, not ATTACHMENT_DOWNLOAD_DENIED', + ).toBe('FILE_DOWNLOAD_DENIED'); + + await setAcl(fieldFile, 'public_read'); + const anonAfter = await stack.api(`/storage/files/${fieldFile}`); + expect(anonAfter.status).toBe(302); + const bytes = await stack.raw(toPath(String(anonAfter.headers.get('location')))); + expect(bytes.status).toBe(200); + expect(await bytes.text()).toBe(FILE_BYTES); + }); +}); diff --git a/packages/qa/dogfood/test/attachments-unscoped-delete-gate.dogfood.test.ts b/packages/qa/dogfood/test/attachments-unscoped-delete-gate.dogfood.test.ts new file mode 100644 index 0000000000..e324136f24 --- /dev/null +++ b/packages/qa/dogfood/test/attachments-unscoped-delete-gate.dogfood.test.ts @@ -0,0 +1,214 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// attachments-storage.attach-requires-parent-edit — the delete gate under an +// UNSCOPED (predicate-less) multi-delete AST. +// +// ## What this file does and does not claim +// +// The item's clause C3 says an unscoped multi-delete — no id AND no where — is +// "refused outright (#4757)", on the reasoning that "nothing was ever queried" +// must not read as "nothing to authorize". `attachment-access-hooks.ts` carries +// exactly that refusal, and `attachment-access-hooks.test.ts` pins it by calling +// the handler directly. +// +// Driven end to end through `ObjectQL.delete`, the refusal does NOT fire: the +// engine's predicate path dispatches `beforeDelete` PER ROW with `input.id` +// bound to each matched row (dispatchPerRowBeforeHooks), so the hook always +// takes its by-id branch and never reaches the `where === undefined` check. The +// measurement is recorded on the issue this file was written for; it is a +// PRODUCT gap, not a test gap, and this file deliberately does NOT pin the +// current behaviour — asserting today's outcome would turn the eventual fix red. +// +// What IS pinned here is the half that does hold and is worth guarding: an +// unscoped AST is not a way around the per-row gate, and the refusal is +// authoritative rather than cosmetic (nothing is deleted). Both sides are +// asserted, because a delete suite that only shows denials stays green on a +// surface that has stopped deleting anything at all. +// +// @proof: attachments-unscoped-delete-gate + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { VerifyStack } from '@objectstack/verify'; +import { resolveAuthzContext } from '@objectstack/core'; +import { + bootAttachmentsHarness, + stopAttachmentsHarness, + uploadFile, + FILE_BYTES, + type AttachmentsHarness, +} from './fixtures/attachments-authz-harness.js'; + +const SYS = { isSystem: true } as const; + +describe('sys_attachment delete gate under an unscoped multi-delete (#9483)', () => { + let harness: AttachmentsHarness; + let stack: VerifyStack; + let ql: any; + let adminTok: string, memberTok: string; + let adminId: string, memberId: string; + let caseId: string, readonlyId: string; + /** The member's OWN attachment on a parent they may edit. */ + let ownAttachmentId: string; + /** Admin's attachment on a parent the member may READ but not EDIT. */ + let foreignAttachmentId: string; + + const uid = async (email: string) => + (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; + + const authzFor = async (token: string) => { + const authService: any = await stack.kernel.getServiceAsync('auth'); + let api: any = authService?.api; + if (!api && typeof authService?.getApi === 'function') api = await authService.getApi(); + return resolveAuthzContext({ + ql, + headers: new Headers({ authorization: `Bearer ${token}` }), + getSession: async (h: any) => api?.getSession?.({ headers: h }), + }); + }; + + const attach = async (token: string, parentObject: string, parentId: string) => { + const fileId = await uploadFile(stack, token); + const res = await stack.apiAs(token, 'POST', '/data/sys_attachment', { + parent_object: parentObject, + parent_id: parentId, + file_id: fileId, + file_name: 'hello.txt', + mime_type: 'text/plain', + size: FILE_BYTES.length, + }); + expect(res.status, `attach to ${parentObject}`).toBeLessThan(300); + const row = await ql.findOne('sys_attachment', { where: { file_id: fileId }, context: SYS }); + return String(row.id); + }; + + const rowCount = () => ql.count('sys_attachment', { context: { ...SYS } }); + + beforeAll(async () => { + harness = await bootAttachmentsHarness(); + stack = harness.stack; + adminTok = await stack.signIn(); + memberTok = await stack.signUp('att-unscoped-member@verify.test'); + ql = await stack.kernel.getServiceAsync('objectql'); + adminId = await uid('admin@objectos.ai'); + memberId = await uid('att-unscoped-member@verify.test'); + + // The delete BIT first. Without it RBAC refuses ahead of the attachment + // hook with PERMISSION_DENIED, and every assertion below would be green for + // the wrong reason — a denial from a layer this file is not about. + const managerSet = await ql.findOne('sys_permission_set', { + where: { name: 'att_attachment_manager' }, + context: SYS, + }); + expect(managerSet?.id, 'fixture permission set seeded').toBeTruthy(); + await ql.insert( + 'sys_user_permission_set', + { user_id: memberId, permission_set_id: managerSet.id }, + { context: { ...SYS } }, + ); + + const caseRes = await stack.apiAs(memberTok, 'POST', '/data/att_case', { name: 'del case' }); + expect(caseRes.status).toBeLessThan(300); + const caseBody = (await caseRes.json()) as any; + caseId = String(caseBody.id ?? caseBody.record?.id ?? caseBody.data?.id); + + // att_readonly is public_read: the member READS it but only the owner + // EDITS it — so admin's attachment on it is one the member may not detach. + const ro = await ql.insert( + 'att_readonly', + { name: 'del ro', owner_id: adminId }, + { context: { ...SYS } }, + ); + readonlyId = ro.id; + + ownAttachmentId = await attach(memberTok, 'att_case', caseId); + foreignAttachmentId = await attach(adminTok, 'att_readonly', readonlyId); + }, 180_000); + + afterAll(async () => { + await stopAttachmentsHarness(harness); + }); + + it('control: the member holds the delete bit and the two rows really do split entitled/not', async () => { + // Asserting the premise rather than assuming it. If the member lost the + // delete grant, every refusal below would still be a refusal — from RBAC, + // about a different question. + expect(await rowCount()).toBe(2); + const readsParent = await stack.apiAs(memberTok, 'GET', `/data/att_readonly/${readonlyId}`); + expect(readsParent.status, 'the member CAN read the foreign parent').toBe(200); + + const own = await ql.findOne('sys_attachment', { where: { id: ownAttachmentId }, context: SYS }); + expect(own.uploaded_by, 'and uploaded one of the two rows').toBe(memberId); + }); + + it('an unscoped multi-delete cannot slip past the per-row gate, and deletes NOTHING', async () => { + // `{ multi: true }` with neither id nor where composes an AST over the whole + // table. The member is the uploader of one row and neither uploader nor + // parent-editor of the other, so the operation must fail — and fail as a + // unit. A partial delete that took the row it was entitled to would be the + // worst outcome: a refusal the caller reads as "nothing happened". + const before = await rowCount(); + const ctx = await authzFor(memberTok); + + await expect(ql.delete('sys_attachment', { multi: true, context: ctx })).rejects.toMatchObject({ + code: 'ATTACHMENT_DELETE_DENIED', + }); + + expect(await rowCount(), 'the refusal is authoritative, not cosmetic').toBe(before); + expect( + await ql.findOne('sys_attachment', { where: { id: foreignAttachmentId }, context: SYS }), + ).toBeTruthy(); + expect( + await ql.findOne('sys_attachment', { where: { id: ownAttachmentId }, context: SYS }), + 'not even the row the caller WAS entitled to is taken', + ).toBeTruthy(); + }); + + it('an empty `where: {}` is treated the same way — an empty predicate is still every row', async () => { + const before = await rowCount(); + const ctx = await authzFor(memberTok); + + await expect( + ql.delete('sys_attachment', { where: {}, multi: true, context: ctx }), + ).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED' }); + + expect(await rowCount()).toBe(before); + }); + + it('a predicate that MATCHES NOTHING is allowed through — "nothing matched" is not a refusal', async () => { + // The distinction the #4757 reasoning draws, and the half of it that is + // live: a real query that resolved zero rows has genuinely nothing to + // authorize, so it must not be refused. Without this case, a suite could be + // satisfied by a surface that refuses every multi-delete outright — which + // would look identical on the denial cases above. + const before = await rowCount(); + const ctx = await authzFor(memberTok); + + await expect( + ql.delete('sys_attachment', { + where: { parent_id: 'no-such-parent-id' }, + multi: true, + context: ctx, + }), + ).resolves.toBeDefined(); + + expect(await rowCount(), 'and it removed nothing').toBe(before); + }); + + it('the caller is not globally denied: a SCOPED delete of their own row succeeds', async () => { + // The positive side. Every assertion above is a denial, and denials alone + // are satisfied by a delete path that is simply broken. This is the same + // caller, the same object, the same session — differing only in scope. + const before = await rowCount(); + const res = await stack.apiAs(memberTok, 'DELETE', `/data/sys_attachment/${ownAttachmentId}`); + expect(res.status, 'the uploader may detach their own attachment').toBeLessThan(300); + + expect(await rowCount()).toBe(before - 1); + expect( + await ql.findOne('sys_attachment', { where: { id: ownAttachmentId }, context: SYS }), + ).toBeFalsy(); + expect( + await ql.findOne('sys_attachment', { where: { id: foreignAttachmentId }, context: SYS }), + 'and the row they were never entitled to is still there', + ).toBeTruthy(); + }); +}); diff --git a/packages/qa/dogfood/test/fixtures/attachments-authz-harness.ts b/packages/qa/dogfood/test/fixtures/attachments-authz-harness.ts new file mode 100644 index 0000000000..9f4060e198 --- /dev/null +++ b/packages/qa/dogfood/test/fixtures/attachments-authz-harness.ts @@ -0,0 +1,114 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Shared boot + upload helpers for the attachment AUTHORIZATION pins added by +// #9483 (QA run #9401 coverage gaps on three attachments-storage items). +// +// These live here rather than in `attachments-fixture.ts` because that module +// is the fixture's DATA (objects, permission sets, stack) and is imported by +// the standing permission matrix. This module is the harness the three new +// clause pins share: the same real `objectstack dev` pairing the matrix boots +// (storage + audit), and the same REAL three-step presigned upload — no +// short-cut that writes a `sys_file` row directly, because a hand-written file +// row would not carry the server-stamped `owner_id` every download verdict +// below turns on. + +import { mkdtempSync, promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { expect } from 'vitest'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { StorageServicePlugin } from '@objectstack/service-storage'; +import { AuditPlugin } from '@objectstack/plugin-audit'; +import { attachmentsFixtureStack, attachmentsFixtureSecurity } from './attachments-fixture.js'; + +/** A booted attachments fixture plus the temp dir its local adapter writes to. */ +export interface AttachmentsHarness { + readonly stack: VerifyStack; + readonly rootDir: string; +} + +/** + * Boot the attachments fixture with the real storage + audit pairing. + * + * `bindToSettings:false` mirrors the matrix: the settings live-wire would swap + * in a './storage' adapter and move the bytes out from under `rootDir`. + */ +export async function bootAttachmentsHarness(): Promise { + const rootDir = mkdtempSync(join(tmpdir(), 'att-authz-')); + const stack = await bootStack(attachmentsFixtureStack as never, { + security: attachmentsFixtureSecurity(), + extraPlugins: [ + new StorageServicePlugin({ adapter: 'local', local: { rootDir }, bindToSettings: false }), + new AuditPlugin(), + ], + }); + return { stack, rootDir }; +} + +export async function stopAttachmentsHarness(h: AttachmentsHarness | undefined): Promise { + await h?.stack?.stop(); + if (h?.rootDir) await fs.rm(h.rootDir, { recursive: true, force: true }); +} + +/** The bytes every helper below uploads — 5 bytes, matching the declared size. */ +export const FILE_BYTES = 'hello'; + +/** + * Drive the REAL presigned three-step upload (presign -> raw PUT -> complete) + * and return the committed `sys_file` id. + */ +export async function uploadFile( + stack: VerifyStack, + token: string | null, + name = 'hello.txt', +): Promise { + const auth: Record = token ? { Authorization: `Bearer ${token}` } : {}; + const presignRes = await stack.api('/storage/upload/presigned', { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...auth }, + body: JSON.stringify({ + filename: name, + mimeType: 'text/plain', + size: FILE_BYTES.length, + scope: 'attachments', + }), + }); + expect(presignRes.status, 'presign').toBe(200); + const { data } = (await presignRes.json()) as any; + const putRes = await stack.raw(toPath(String(data.uploadUrl)), { + method: 'PUT', + headers: data.headers ?? { 'content-type': 'text/plain' }, + body: FILE_BYTES, + }); + expect(putRes.status, 'raw PUT').toBeLessThan(300); + const completeRes = await stack.api('/storage/upload/complete', { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...auth }, + body: JSON.stringify({ fileId: data.fileId }), + }); + expect(completeRes.status, 'complete').toBe(200); + return String(data.fileId); +} + +/** Strip the origin off an absolute adapter URL so it can be re-injected. */ +export function toPath(url: string): string { + return url.replace(/^https?:\/\/[^/]+/, ''); +} + +/** + * Seconds of life left in a LocalStorageAdapter capability URL. + * + * The adapter signs `base64url(JSON).hmac`, and the payload carries `exp` (an + * epoch-seconds deadline). Reading it is how the two TTL BRANCHES of + * `authorizeDownload` are told apart from the outside: the ungated/`public_read` + * return hands back `presignedTtl` (3600s) while an authorized gated grant + * hands back the much shorter `downloadTtl` (300s). Without this the two + * branches are indistinguishable — both answer 200/302 with a working URL — + * and a test asserting only "a URL came back" cannot tell which one ran. + */ +export function ttlSecondsOf(capabilityUrl: string): number { + const token = capabilityUrl.split('/').pop() ?? ''; + const [b64] = token.split('.'); + const payload = JSON.parse(Buffer.from(b64, 'base64url').toString('utf8')) as { exp: number }; + return payload.exp - Math.floor(Date.now() / 1000); +}