Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .changeset/23732-attachment-readable-ids.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
'@objectstack/service-storage': minor
---

feat(service-storage): the `sys_attachment` read gate writes one `ReadableIds` leaf per parent object and does no pre-scan, so a broad read is judged over every row, not a 2,000-row window (ADR-0139 D4, D5)

Clause-②: yes

The gate serves an attachment only when the caller may read the record it is attached to. Until now each read first scanned at most 2,000 candidate rows as the system, then asked which of those rows' parent records the caller could read. Past that window the gate failed closed: the unscanned rows were left out, the list's `total` agreed with the short answer, and a warning said so.

The gate now writes one branch per object the metadata registry holds (except `sys_attachment` itself): `{ parent_object: P, parent_id: { $in: ReadableIds.of(P) } }`. The engine composes the leaf from the security service's `canReadObject` and `getReadFilter` for `P`, and the SQL drivers evaluate it as one subquery against `P`'s table, inside their own tenant wall. A list, its `total` and a grouped count are each one statement over every row the caller may read. A parent object the caller may not read is answered with no rows, without reading it as the caller, so no denial is logged for it.

What a reader sees:

- A broad read of `sys_attachment` is served every attachment whose parent record the caller may read, with a true `total`, whatever the table size. An attachment whose parent the caller may not read, whose parent no longer exists, or that names no parent is still never served. A read scoped to one record (the attachments panel) is unchanged.
- Three classes still read through the bounded probe, chosen before any leaf is written: a delegated (on-behalf-of) caller; a parent an object-scoped read middleware narrows (for example `sys_comment`, `sys_comment_reaction`, `sys_activity` and `sys_audit_log`); and a parent bound to another datasource than `sys_attachment`. The whole read also stays on the probe when the security service offers no `canReadObject`, or when none is registered.
- A read whose probe finds no candidate row is now narrowed to no rows rather than left unfiltered. It served nothing before either.
- On a driver that does not compile the leaf (`driver-mongodb`), a read of `sys_attachment` now fails with `INTERNAL_ERROR` / 500 naming the driver and the leaf, instead of serving a capped list. The gate does not turn that refusal into an empty answer. This is ADR-0139's ruled behaviour until a pipeline form is measured for that driver.

For a host that installs the gate itself: `installAttachmentReadVisibility(engine, logger, getAdmission?)` takes an optional third argument that resolves the security service on every read. `StorageServicePlugin` passes it. A call without it keeps every read on the bounded probe and says so once.
14 changes: 10 additions & 4 deletions docs/qa/platform-checklist/areas/attachments-storage.json
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,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": 3,
"revision": 4,
"priority": "P1",
"surface": "api",
"personas": ["member who can read the parent record", "member who cannot read the parent record"],
Expand All @@ -283,7 +283,7 @@
"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 open core since ADR-0132 (Apache-2.0, published on npm) — ⛔ it being closed-source is NOT the reason and has not been since #16215. The reason is ADR-0132's entitlement boundary: no framework package may declare `@objectstack/organizations` (pinned by `packages/plugins/organizations/src/no-framework-dependents.pin.test.ts` — apps declare it, packages do not), so `packages/qa/dogfood` cannot depend on it and 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."
"a broad read over an ordinary parent no longer meets the 2000-candidate cap (ADR-0139 E7, #23767): the gate writes one ReadableIds leaf per parent object and pre-scans nothing, so the scratch parents' broad read is served every visible row with a true total and no '[storage] attachment read visibility' warning. The bounded 2000-row fail-closed probe (READ_SCAN_LIMIT, clause 4's warning) remains for ADR-0139 D5's three classes only: a delegated (on-behalf-of) caller, a parent narrowed by an object-scoped read middleware (sys_comment, sys_comment_reaction, sys_activity and sys_audit_log among them), and a parent bound to another datasource than sys_attachment. None of the three is provisioned by the qa-scratch-authz recipe, so clause 4's probe half is not reachable from a run here: it is pinned in packages/services/service-storage/src/attachment-read-leaf.integration.test.ts and attachment-read-leaf.test.ts, and a run cites them. On driver-mongodb, which compiles no ReadableIds leaf until ADR-0139's O3 is measured, a member's sys_attachment read fails INTERNAL_ERROR / 500 naming the driver instead of serving a short list: a ruled limit (ADR-0139 D3, Consequences), never a pass and never a leak."
]
},
"steps": [
Expand Down Expand Up @@ -325,10 +325,10 @@
"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 (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." },
"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 broad read past the old 2000-row pre-scan window, @proof attachments-parent-rls-scan-cap: since ADR-0139 E7 the gate writes one ReadableIds leaf per parent object and pre-scans nothing for an ordinary parent, so with >2000 candidate join rows the restricted member is served EVERY visible row on one page, `total` equals the rows served (the read passes $top, so total comes from engine.count()), no '-row cap' warning is logged, the invisible-parent row and every dangling-parent row stay excluded, and the owner is served the hidden row on the same read. The seed still puts visible rows at BOTH ENDS of the insertion order with the filler bulk between, and the control asserts that a 2000-row window would leave a visible row outside it, so the pin cannot pass on a fixture the old bound would have served whole) + packages/services/service-storage/src/attachment-read-leaf.integration.test.ts and packages/services/service-storage/src/attachment-read-leaf.test.ts (clause 3, the half that still bounds: ADR-0139 D5's three classes, a delegated caller, a parent narrowed by an object-scoped read middleware and a federated parent, keep the bounded 2000-row fail-closed probe, chosen before any leaf is written; past the cap the probe warns naming its class, and a probe failure denies all and says so. Also there: on a driver that compiles no leaf, as driver-mongodb until ADR-0139's O3, find and count fail { code: 'INTERNAL_ERROR', status: 500 } naming the driver, never the deny-all sentinel). 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 from this lane, and not to be read as covered by a run here: (a) an attachment naming a parent OBJECT the registry does not hold cannot be seeded — 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'\"); such a row names no enumerated parent, so no branch admits it, which attachment-read-leaf.integration.test.ts pins below the API; (b) the middleware no longer wraps the read in an outer catch (ADR-0139 D3): the probe's own failure ('the bounded probe (...) failed, denying all') is pinned in attachment-read-leaf.test.ts and 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 (installAttachmentReadVisibility: middleware over find/findOne/count/aggregate, deny-all sentinel, READ_SCAN_LIMIT fail-closed cap)",
"packages/services/service-storage/src/attachment-access-hooks.ts#installAttachmentReadVisibility (installAttachmentReadVisibility: middleware over find/findOne/count/aggregate, one ReadableIds leaf per parent object (ADR-0139 D4), deny-all sentinel, the READ_SCAN_LIMIT fail-closed probe for ADR-0139 D5's three classes)",
"packages/services/service-storage/src/attachment-read-visibility.test.ts"
],
"history": [
Expand All @@ -339,6 +339,12 @@
"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"
},
{
"revision": 4,
"date": "2026-10-16",
"change": "clause 3's broad-read half re-pinned for ADR-0139 E7: the attachment gate writes one ReadableIds leaf per parent object and pre-scans nothing, so a broad read over an ordinary parent past 2000 candidates is served whole with a true total, and the scan-cap pin is rewritten for that side of the bound (its @proof tag kept). knownGaps and automated.ref now name the limits that remain: ADR-0139 D5's three classes keep the bounded 2000-row probe (pinned in service-storage), and driver-mongodb answers INTERNAL_ERROR / 500 until O3. The unknown-parent-object and outer-catch arms are restated for the new gate.",
"ref": "#23767"
}
]
},
Expand Down
Loading
Loading