diff --git a/.changeset/expand-crud-gate-cross-persona-disclosure.md b/.changeset/expand-crud-gate-cross-persona-disclosure.md new file mode 100644 index 0000000000..620f7f2c75 --- /dev/null +++ b/.changeset/expand-crud-gate-cross-persona-disclosure.md @@ -0,0 +1,65 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(security): `$expand` no longer discloses records the caller is 403'd from — the #2850 expand waiver is removed (#7626) + +A **low-privilege authenticated user could read records they are explicitly +denied**, through the `$expand` seam. Measured on the running showcase app with a +`contributor`-only session: + +- `GET /api/v1/data/showcase_contact/` → **403 PERMISSION_DENIED**; +- same session, `GET /api/v1/data/showcase_invoice?$expand=contact` on an invoice + it owns → **200 with that contact fully materialised**, all 18 fields including + `email`, byte-identical to the admin's response; +- the body door (`POST …/query` with a nested `expand`) behaved the same. + +`showcase_contact` declares `sharingModel: 'private'` and the row was +admin-owned, so both the object-level CRUD gate and the OWD row scope were +bypassed. RLS on the DIRECT path was never affected and is unchanged. + +**Root cause.** #2850 correctly routed the engine's expand path back through the +security middleware (tagging the sub-read `__expandRead`), which is what put the +referenced object's RLS + FLS on an expansion at all. It also added a relaxation: + +```ts +operation === 'find' && __expandRead && !secMeta.isPrivate // → skip CRUD + requiredPermissions +``` + +justified as "a PUBLIC referenced object is already broadly readable via the `'*'` +wildcard grant, so gating the expansion adds no protection". Neither half held: + +1. `secMeta.isPrivate` is derived from `access.default` (ADR-0066 D2 — whether a + `'*'` wildcard COVERS the object), a **different axis** from the `sharingModel` + OWD that scopes an object's ROWS. An object that leaves `access` unset — nearly + all of them, `showcase_contact` included — read as "public", so the waiver + fired for it. +2. "already broadly readable" was never **checked**. The condition asks nothing + about the caller's grants, so it fired hardest for the caller holding none — + #2850's own unit pin waives the gate for a permission set with `objects: {}`. + Where the premise is true the waiver is inert (the CRUD gate would pass + anyway); its only non-vacuous effect was on callers the gate meant to refuse. + +The OWD half followed from the same skip: `getEffectiveScope` answers `'org'` when +no set grants the operation — safe only because such a caller is denied +separately — so waiving the denial also stamped `__readScope: 'org'` and dissolved +plugin-sharing's owner filter. + +**Fix.** The waiver is deleted; both throw-gates now run for every referenced +object. One rule, public and private alike — the rule #2850 already applied to its +private half: *an expansion may reveal only rows the caller could have read +directly.* Nothing over-blocks a legitimate lookup: `expandRelatedRecords` already +catches a refused sub-read and retains the bare FK id, so the parent read still +returns 200 with the id it had. `__expandRead` itself stays — it is the marker the +storage/comment access hooks strip as a privileged widening input, and `core`'s +operation-private-keys list is what keeps it unforgeable from the wire. + +**Regression proof.** `packages/qa/dogfood/test/showcase-expand-crud-gate.dogfood.test.ts` +drives the live HTTP stack with **two real sessions** — admin sees the expansion, +the `contributor`-only persona must not — across all three expand doors +(query-string `$expand` on list and by-id, body `expand`), and carries the +over-correction guard: the contributor's foreign-invoice query still returns +200 with 0 rows, and a lookup they DO hold a grant on still expands. The unit +pins in `security-plugin.test.ts` are re-aimed but deliberately are not the +regression story: an `__expandRead`-wiring assertion is the exact shape that +stayed green for the whole life of this disclosure. diff --git a/packages/core/src/security/operation-private-keys.ts b/packages/core/src/security/operation-private-keys.ts index a249987ae9..ea7abddb28 100644 --- a/packages/core/src/security/operation-private-keys.ts +++ b/packages/core/src/security/operation-private-keys.ts @@ -21,8 +21,10 @@ * `__delegatorReadScope` / `__delegatorWriteScope`, stamped in place by * `security-plugin.ts` (`sc.__readScope = …`); * - the engine's internal privilege markers on the same channel — - * `__expandRead` waives the object-level CRUD check for a lookup expansion, - * `__referentialFieldClear` the referential-clear write. + * `__expandRead` marks a read as a lookup EXPANSION sub-read (it no longer + * relaxes any gate — #7626 removed that waiver — but it still travels with + * one operation and must not be inherited by another), `__referentialFieldClear` + * authorizes the referential-clear write. * * plugin-security is the PRODUCER of that vocabulary and would be the most * honest owner of the rule for consuming it, but none of the three consumers diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index e782ef6afe..0865e892b5 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -6319,8 +6319,9 @@ export class ObjectStackProtocolImplementation implements // // What rides on it is total: plugin-security's middleware opens with // `if (opCtx.context?.isSystem) return next()` — the entire RLS / FLS / - // CRUD chain skipped — and `__expandRead` waives the object-level CRUD - // gate for public objects (#2850). Neither is ever schema-stripped on + // CRUD chain skipped — and `__expandRead` marks a read as an expansion + // sub-read (#2850; it waived the object-level CRUD gate for "public" + // objects until #7626 removed that). Neither is ever schema-stripped on // this path: `ExecutionContextSchema.parse` runs only in // `engine.createContext`, which the read path does not use. // diff --git a/packages/metadata-protocol/src/protocol.wire-context.test.ts b/packages/metadata-protocol/src/protocol.wire-context.test.ts index 143766f676..6afd5db338 100644 --- a/packages/metadata-protocol/src/protocol.wire-context.test.ts +++ b/packages/metadata-protocol/src/protocol.wire-context.test.ts @@ -12,8 +12,9 @@ // // What rides on that context is total: plugin-security's middleware opens with // `if (opCtx.context?.isSystem) return next()` — the whole RLS/FLS/CRUD chain -// skipped — and `__expandRead` waives the object-level CRUD gate for public -// objects. Neither is ever schema-stripped on the read path +// skipped — and `__expandRead` marks a read as an expansion sub-read (it waived +// the object-level CRUD gate for "public" objects until #7626 removed that). +// Neither is ever schema-stripped on the read path // (`ExecutionContextSchema.parse` runs only in `createContext`). // // The auth gate is what stands between that and a live exploit: `enforceAuth` diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 99dd18a5c6..248fd96f9c 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -6274,14 +6274,18 @@ export class ObjectQL implements IObjectQLEngine { // // The `__expandRead` marker (set here, never from client input — // `executionContext` is server-built) tells the security layer this is - // an expansion sub-read: it waives ONLY the object-level CRUD / - // requiredPermissions gate for PUBLIC referenced objects (already - // broadly readable, so a common status/owner lookup isn't over-blocked), - // while PRIVATE referenced objects keep the full RLS + CRUD treatment - // (you may expand only rows you could read directly). FLS masking - // applies to both. `expand` is intentionally omitted from this query so - // `find` does not re-expand — nested relations recurse below under the - // depth guard. + // an expansion sub-read. It does NOT relax that layer: since #7626 the + // referenced object takes the FULL treatment — CRUD gate, + // requiredPermissions, RLS and FLS — so you may expand only rows you + // could have read directly. (#2850 shipped a waiver for "public" + // referenced objects; it keyed on an axis almost no object declares and + // never checked the caller's grants, so it disclosed private records to + // callers the gate had already refused.) A refusal is caught below and + // the bare FK id is retained, which is why the parent read still + // succeeds. `expand` is intentionally omitted from this query so `find` + // does not re-expand — nested relations recurse below under the depth + // guard. + // // [#7537] The join key is MACHINERY, not a caller-chosen column. This // sub-read's projection is forwarded from `nestedAST.fields`, and the // map built below is keyed on `rec.id` — so a nested projection that did diff --git a/packages/plugins/plugin-audit/src/comment-read-visibility.test.ts b/packages/plugins/plugin-audit/src/comment-read-visibility.test.ts index cc4977be71..7ed85d459d 100644 --- a/packages/plugins/plugin-audit/src/comment-read-visibility.test.ts +++ b/packages/plugins/plugin-audit/src/comment-read-visibility.test.ts @@ -210,7 +210,7 @@ describe('installCommentReadVisibility', () => { // [#7141] The parent probe reads a DIFFERENT object than the one the security // middleware resolved its depth for, so the caller's envelope crosses over // but the operation-private keys must not: `__readScope` is `sys_comment`'s - // access DEPTH and `__expandRead` waives the object-level CRUD check. + // access DEPTH and `__expandRead` marks THAT read as an expansion sub-read. it('probes the parent with the caller ENVELOPE, minus the operation-private keys', async () => { const { mw, calls } = install(dataset); await runRead(mw, { diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 4c1da553e6..3def7bdf11 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -937,14 +937,24 @@ describe('SecurityPlugin', () => { await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' }); }); - // ── [#2850] $expand RLS/FLS bypass — sub-read gate relaxation ──────────── + // ── [#2850 / #7626] $expand sub-read — the gate is NOT relaxed ─────────── // The engine's expand path re-enters `find` for the referenced object with - // a server-set `__expandRead` marker. For a PUBLIC referenced object the - // object-level CRUD / requiredPermissions gate is waived (the row is already - // broadly readable, so applying it would over-block a common status/owner - // lookup) — but RLS + FLS still run. A PRIVATE referenced object keeps the - // full gate: expansion may reveal only rows the caller could read directly. - it('[#2850] __expandRead WAIVES the CRUD gate for a PUBLIC referenced object', async () => { + // a server-set `__expandRead` marker, which is what puts that object's RLS + // + FLS on the expansion (#2850). #2850 also waived the object-level CRUD / + // requiredPermissions gate whenever `access.default !== 'private'` — an + // axis almost no object declares, and one it never paired with a check that + // the caller actually held a covering grant. #7626 measured what that cost + // on the running app (a `contributor`-only session read a private, + // admin-owned contact it was 403'd from, in full, via `?$expand=contact`) + // and deleted the waiver. One rule now, for public and private alike: + // expansion may reveal only rows the caller could have read directly. + // + // These are the UNIT half. They cannot be the whole regression story and + // deliberately are not: the pin that stayed green through the disclosure + // was exactly this shape. The end-to-end, two-persona proof — admin sees + // the expansion, the denied persona does not, through every expand door — + // lives in `packages/qa/dogfood/test/showcase-expand-crud-gate.dogfood.test.ts`. + it('[#7626] __expandRead does NOT waive the CRUD gate for a caller holding no grant', async () => { const noGrantSet: PermissionSet = { name: 'member_default', label: 'Member', objects: {}, } as any; @@ -962,11 +972,38 @@ describe('SecurityPlugin', () => { }); // Control: a DIRECT read with no grant on the object is denied. await expect(harness.run(base())).rejects.toMatchObject({ name: 'PermissionDeniedError' }); - // The SAME read tagged as an expand sub-read is allowed — the gate is waived - // for the (public) referenced object. + // The SAME read tagged as an expand sub-read is denied TOO. Before #7626 + // this resolved — the object declares no `access` block, so it read as + // "public, already broadly readable" and the gate was skipped for a + // caller whose permission set grants literally nothing. const expandCtx = base(); expandCtx.context.__expandRead = true; - await expect(harness.run(expandCtx)).resolves.toBeDefined(); + await expect(harness.run(expandCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('[#7626] __expandRead does NOT waive the requiredPermissions AND-gate', async () => { + // The other throw-gate the waiver covered. `requiredPermissions` is an + // access-NARROWING declaration (ADR-0049), so a sub-read is the last + // place it may be skipped — the same stance the `unresolved` fail-closed + // above takes for the very same field. + const memberOnly: PermissionSet = { + name: 'member_default', label: 'Member', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + } as any; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ + permissionSets: [memberOnly], + objectFields: ['id', 'organization_id', 'signed_token'], + schemaExtra: { requiredPermissions: ['manage_platform_settings'] }, + orgScoping: true, + }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + const opCtx: any = { + object: 'task', operation: 'find', ast: { where: undefined }, + context: { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], __expandRead: true }, + }; + await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' }); }); it('[#2850] __expandRead does NOT waive the gate for a PRIVATE referenced object', async () => { @@ -993,7 +1030,7 @@ describe('SecurityPlugin', () => { await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' }); }); - it('[#2850] __expandRead still injects RLS on the expand sub-read (only the CRUD gate is waived)', async () => { + it('[#2850] __expandRead still injects RLS on the expand sub-read (the referenced object is scoped, not merely gated)', async () => { const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); const harness = makeMiddlewareCtx({ permissionSets: [tenantPolicySet], @@ -1006,8 +1043,9 @@ describe('SecurityPlugin', () => { context: { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], __expandRead: true }, }; await harness.run(opCtx); - // The tenant wall is still AND-injected — waiving the CRUD gate on an - // expand sub-read does NOT waive row-level scoping on the referenced object. + // The tenant wall is still AND-injected: `__expandRead` marks the sub-read + // for the layers that strip it as a privileged input, it never loosens + // row-level scoping on the referenced object. expect(opCtx.ast.where).toEqual({ $and: [{ organization_id: 'org-1' }, { organization_id: 'org-1' }] }); }); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 147a3bd7bd..24bdc96581 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -1191,29 +1191,65 @@ export class SecurityPlugin implements Plugin { ); } - // [#2850] $expand sub-read gate relaxation. The engine's expand path - // re-enters `find` for a referenced object carrying `__expandRead` (a - // server-set marker; `executionContext` is never client-built). For a - // PUBLIC referenced object — covered by the '*' wildcard grant and thus - // already broadly readable — applying the object-level CRUD / - // requiredPermissions gate to the EXPANSION would only surface "never - // designed for expand" modeling gaps (over-blocking a legitimate - // status/owner lookup) without adding protection, since the row is - // already visible. So waive those two throw-gates for PUBLIC expand - // sub-reads only. RLS injection (step 3) and FLS masking (step 4) still - // run, and a PRIVATE referenced object keeps the FULL gate — expansion - // may reveal only rows the caller could have read directly. - const expandSkipCrud = - opCtx.operation === 'find' && - opCtx.context?.__expandRead === true && - !secMeta.isPrivate; + // [#2850 / #7626] $expand sub-reads take the FULL gate — there is no + // waiver here any more, and the deletion is the fix. + // + // #2850 routed the engine's expand path back through this middleware + // (`find` re-entered for the referenced object carrying `__expandRead`, a + // server-set marker `executionContext` never takes from a client), which + // is what put the referenced object's RLS and FLS on the expansion at + // all. That part stands. What it also added was a relaxation: + // + // operation === 'find' && __expandRead && !secMeta.isPrivate + // → skip the CRUD + requiredPermissions throw-gates + // + // …justified as "a PUBLIC referenced object is covered by the '*' + // wildcard grant and thus already broadly readable, so gating the + // EXPANSION adds no protection and only surfaces never-designed-for- + // expand modeling gaps". Neither half of that premise held (#7626): + // + // 1. `secMeta.isPrivate` reads `access.default` (ADR-0066 D2 — whether + // a `'*'` wildcard COVERS the object), which is a different axis + // from the `sharingModel` OWD an object declares to scope its ROWS. + // `showcase_contact` declares `sharingModel: 'private'` and no + // `access` block, so it read as "public" and the waiver fired for it + // — as it did for every object that leaves `access` unset, which is + // almost all of them. + // 2. "already broadly readable" was never CHECKED. The waiver asks + // nothing about the caller's grants, so it fired hardest for the + // caller who holds NONE — #2850's own pin waives the gate for a + // permission set with `objects: {}`. Where the premise is true the + // waiver is inert (the CRUD gate would pass anyway); its only + // non-vacuous effect is on callers the gate meant to refuse. + // + // Measured on the real showcase app: a `contributor`-only session 403'd + // on `GET /data/showcase_contact/:id` received the same contact FULLY + // materialised — all 18 fields, `email` included — through + // `?$expand=contact` on an invoice it legitimately owns. Both gates were + // bypassed at once, because step 2.6 below stashes `__readScope` from + // `getEffectiveScope`, which answers `'org'` when NO set grants the op + // (safe only because the caller is denied separately — and the waiver is + // what stopped them being denied). So the skipped CRUD check also handed + // plugin-sharing an org-wide read depth and dissolved the OWD row scope. + // + // Removing it restores one rule for every referenced object, the rule + // #2850 already applied to the private half: an expansion may reveal only + // rows the caller could have read directly. Nothing over-blocks a + // legitimate lookup — `expandRelatedRecords` catches the refusal and + // retains the bare FK id (its documented graceful degradation), so a + // caller who cannot read the referenced object gets the id it already + // had, not an error on the parent read. + // + // `__expandRead` itself stays: it is the marker the storage/comment + // access hooks strip as a privileged widening input, and `core`'s + // operation-private-keys list is what keeps it unforgeable from the wire. // 1.5. [ADR-0066 D3/⑤] requiredPermissions AND-gate — a capability // prerequisite checked BEFORE the CRUD grant (ADR §Precedence): a // caller missing any required capability is denied regardless of how // permissive their grants are. Per-operation (⑤): only the caps for // THIS operation's CRUD class (plus any all-operations caps) apply. - if (permissionSets.length > 0 && !expandSkipCrud) { + if (permissionSets.length > 0) { const required = requiredCapsForOperation(secMeta.requiredPermissions, opCtx.operation); if (required.length > 0) { const held = this.permissionEvaluator.getSystemPermissions(permissionSets); @@ -1271,7 +1307,7 @@ export class SecurityPlugin implements Plugin { } // 2. CRUD permission check - if (permissionSets.length > 0 && !expandSkipCrud) { + if (permissionSets.length > 0) { const allowed = this.permissionEvaluator.checkObjectPermission( opCtx.operation, opCtx.object, diff --git a/packages/qa/dogfood/test/showcase-expand-crud-gate.dogfood.test.ts b/packages/qa/dogfood/test/showcase-expand-crud-gate.dogfood.test.ts new file mode 100644 index 0000000000..a4722deac8 --- /dev/null +++ b/packages/qa/dogfood/test/showcase-expand-crud-gate.dogfood.test.ts @@ -0,0 +1,252 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7626] `$expand` must not become a second, ungated read door. +// +// The defect this file pins was a CROSS-PERSONA DISCLOSURE on the real +// showcase app: a `contributor`-only member is 403'd on `showcase_contact` +// (neither `showcase_contributor` nor `showcase_member_default` grants it), +// yet `GET /data/showcase_invoice?$expand=contact` handed back the referenced +// contact FULLY MATERIALISED — byte-identical to what the admin sees — because +// the #2850 expand waiver (`expandSkipCrud`) keyed on `access.default`, an axis +// `showcase_contact` never declares, so it read as "public, already broadly +// readable" and waived the object-level gate for EVERY referenced object. +// +// WHY THIS FILE IS END-TO-END AND TWO-PERSONA. The #2850 pin lives in +// `security-plugin.test.ts` and asserts only that the expand sub-read re-enters +// `find()` tagged `__expandRead`. That is a WIRING assertion: it stayed green +// for the whole life of the disclosure because it never asked what a real +// denied caller actually receives. So this file drives the live HTTP stack with +// TWO real sessions and compares outcomes — the admin sees the expansion, the +// denied persona must not, through every expand door the API offers. +// +// It also carries the OVER-CORRECTION guard: a contributor's query for an +// invoice they do not own still returns 200 with the foreign row filtered out +// (RLS on the direct path was never broken and must stay unbroken). +// +// Boots its OWN stack rather than the shared showcase: `shared-showcase.ts` +// hands its baseline an explicit `showcase_contact` read grant for an unrelated +// fixture, which is exactly the grant whose ABSENCE this file is about. +// +// Deliberately carries no `@proof:` tag: ADR-0054 proofs gate the `live` status +// of an AUTHORABLE property, and this file gates none — it pins a security +// regression on the engine's expand seam. A tag here would only add an +// unregistered orphan to `proof-registry.mts`. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { showcaseAppDefaultSecurity } from './showcase-security.js'; + +const SYS = { isSystem: true } as const; + +const CONTRIB_EMAIL = 'expand-contrib@verify.test'; +/** The field the disclosure actually handed over — the canary for "leaked". */ +const SECRET_EMAIL = 'expand-secret-contact@verify.test'; + +const idOf = (b: any) => b?.id ?? b?.record?.id ?? b?.data?.id ?? b?.recordId; +const recordsOf = (b: any): any[] => b?.records ?? b?.data ?? (Array.isArray(b) ? b : []); + +describe('showcase: $expand honours the referenced object’s CRUD gate + OWD (#7626)', () => { + let stack: VerifyStack; + let ql: any; + let adminTok: string; + let contribTok: string; + let contactId: string; + let ownInvoiceId: string; + let foreignInvoiceId: string; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, { security: showcaseAppDefaultSecurity() }); + adminTok = await stack.signIn(); + contribTok = await stack.signUp(CONTRIB_EMAIL); + ql = await stack.kernel.getServiceAsync('objectql'); + + // The persona: `contributor` and nothing else — the app's OWN + // `showcase_contributor` set, unmodified. It names `showcase_invoice` and + // does NOT name `showcase_contact`; the `showcase_member_default` baseline + // every member also holds does not name it either. That absence IS the + // fixture. + // + // Both rows are needed and they carry different halves: the POSITION is + // what the set's RLS rules bind to (`positions: ['contributor']` on + // `invoice_own_rows`), and the direct set assignment is the CAPABILITY + // (the same system-plumbing shape `showcase-permission-zoo` uses for its + // auditor/delegate personas). + const contribId = (await ql.findOne('sys_user', { where: { email: CONTRIB_EMAIL }, context: SYS }))?.id; + expect(contribId, 'contributor user provisioned').toBeTruthy(); + await ql.insert('sys_user_position', { user_id: contribId, position: 'contributor' }, { context: SYS }); + const contribSet = await ql.findOne('sys_permission_set', { where: { name: 'showcase_contributor' }, context: SYS }); + expect(contribSet?.id, 'the app’s contributor set is seeded').toBeTruthy(); + await ql.insert( + 'sys_user_permission_set', + { user_id: contribId, permission_set_id: contribSet.id }, + { context: SYS }, + ); + + // Substrate the invoice's required `account` lookup can point at. + const acct = await stack.apiAs(adminTok, 'POST', '/data/showcase_account', { + name: 'Expand Gate Co', + status: 'prospect', + }); + expect(acct.status, 'admin creates the account').toBeLessThan(300); + const accountId = idOf(await acct.json()); + + // The withheld record: a `sharingModel: 'private'` contact OWNED BY THE + // ADMIN. The contributor holds no grant on the object AND does not own the + // row, so both gates — CRUD and OWD — say no to a direct read. + const contact = await stack.apiAs(adminTok, 'POST', '/data/showcase_contact', { + name: 'Withheld Contact', + email: SECRET_EMAIL, + phone: '+1-555-0100', + company: 'Expand Gate Co', + account: accountId, + }); + expect(contact.status, 'admin creates the private contact').toBeLessThan(300); + contactId = idOf(await contact.json()); + expect(contactId, 'contact id').toBeTruthy(); + + // The parent the contributor IS allowed to read: an invoice they own + // (`invoice_own_rows` RLS is `owner == current_user.email`) that references + // the withheld contact. + const own = await stack.apiAs(adminTok, 'POST', '/data/showcase_invoice', { + name: 'INV-7626-OWN', + account: accountId, + contact: contactId, + owner: CONTRIB_EMAIL, + status: 'draft', + }); + expect(own.status, 'admin creates the contributor-owned invoice').toBeLessThan(300); + ownInvoiceId = idOf(await own.json()); + expect(ownInvoiceId, 'own invoice id').toBeTruthy(); + + // A second invoice the contributor does NOT own — the direct-path RLS guard. + const foreign = await stack.apiAs(adminTok, 'POST', '/data/showcase_invoice', { + name: 'INV-7626-FOREIGN', + account: accountId, + contact: contactId, + owner: 'someone-else@verify.test', + status: 'draft', + }); + expect(foreign.status, 'admin creates the foreign invoice').toBeLessThan(300); + foreignInvoiceId = idOf(await foreign.json()); + expect(foreignInvoiceId, 'foreign invoice id').toBeTruthy(); + }, 120_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + // ── The premise: the direct door is shut ───────────────────────────────── + it('the contributor is 403’d reading the contact DIRECTLY (the door expand must not reopen)', async () => { + const byId = await stack.apiAs(contribTok, 'GET', `/data/showcase_contact/${contactId}`); + expect(byId.status, 'by-id read of a contact the persona holds no grant on').toBe(403); + + const list = await stack.apiAs(contribTok, 'GET', '/data/showcase_contact'); + expect(list.status, 'list read of the same object').toBe(403); + }); + + it('the contributor CAN read the parent invoice they own (so expand is reachable at all)', async () => { + const r = await stack.apiAs(contribTok, 'GET', `/data/showcase_invoice/${ownInvoiceId}`); + expect(r.status).toBe(200); + const body: any = await r.json(); + expect(body?.record?.name ?? body?.name).toBe('INV-7626-OWN'); + }); + + // ── Door 1: query-string `$expand` on the list route ───────────────────── + it('admin sees the contact materialised through `?$expand=contact` (the capability still works)', async () => { + const r = await stack.apiAs(adminTok, 'GET', `/data/showcase_invoice?$expand=contact&name=INV-7626-OWN`); + expect(r.status).toBe(200); + const body: any = await r.json(); + const row = recordsOf(body).find((x: any) => x.name === 'INV-7626-OWN'); + expect(row, 'admin sees the invoice').toBeTruthy(); + expect(row.contact, 'admin gets the expanded record, not the bare id').toMatchObject({ + id: contactId, + email: SECRET_EMAIL, + }); + }); + + it('the contributor does NOT get the contact through `?$expand=contact`', async () => { + const r = await stack.apiAs(contribTok, 'GET', `/data/showcase_invoice?$expand=contact&name=INV-7626-OWN`); + // The parent read is legitimate — it must still succeed. + expect(r.status, 'the parent read is allowed').toBe(200); + const raw = await r.text(); + expect(raw, 'no field of the withheld contact reaches the wire').not.toContain(SECRET_EMAIL); + + const body: any = JSON.parse(raw); + const row = recordsOf(body).find((x: any) => x.name === 'INV-7626-OWN'); + expect(row, 'the invoice itself is still returned').toBeTruthy(); + // Graceful degradation: the FK id is retained, the record is not expanded. + expect(typeof row.contact === 'object' && row.contact !== null).toBe(false); + }); + + // ── Door 2: query-string `$expand` on the by-id route ──────────────────── + it('the contributor does NOT get the contact through by-id `?expand=contact`', async () => { + const r = await stack.apiAs(contribTok, 'GET', `/data/showcase_invoice/${ownInvoiceId}?expand=contact`); + expect(r.status, 'the parent read is allowed').toBe(200); + const raw = await r.text(); + expect(raw, 'no field of the withheld contact reaches the wire').not.toContain(SECRET_EMAIL); + }); + + it('admin DOES get it through the same by-id door (both personas, one gate)', async () => { + const r = await stack.apiAs(adminTok, 'GET', `/data/showcase_invoice/${ownInvoiceId}?expand=contact`); + expect(r.status).toBe(200); + const raw = await r.text(); + expect(raw, 'the admin expansion is unchanged').toContain(SECRET_EMAIL); + }); + + // ── Door 3: the body door (`POST …/query` with a nested `expand`) ───────── + it('the contributor does NOT get the contact through the body `expand` door', async () => { + const r = await stack.apiAs(contribTok, 'POST', '/data/showcase_invoice/query', { + where: { name: 'INV-7626-OWN' }, + // `id` is deliberately present: without it the expansion is a silent + // no-op (#7537) and the assertion would pass for the wrong reason. + expand: { contact: { object: 'showcase_contact', fields: ['id', 'name', 'email'] } }, + }); + expect(r.status, 'the parent read is allowed').toBe(200); + const raw = await r.text(); + expect(raw, 'no field of the withheld contact reaches the wire').not.toContain(SECRET_EMAIL); + }); + + it('admin DOES get it through the body `expand` door', async () => { + const r = await stack.apiAs(adminTok, 'POST', '/data/showcase_invoice/query', { + where: { name: 'INV-7626-OWN' }, + expand: { contact: { object: 'showcase_contact', fields: ['id', 'name', 'email'] } }, + }); + expect(r.status).toBe(200); + const body: any = await r.json(); + const row = recordsOf(body).find((x: any) => x.name === 'INV-7626-OWN'); + expect(row?.contact, 'admin body-door expansion is unchanged').toMatchObject({ email: SECRET_EMAIL }); + }); + + // ── The over-correction guard ──────────────────────────────────────────── + // + // The fix narrows the EXPAND seam only. A contributor's ordinary query over + // an object they DO hold is unchanged: it succeeds (200) and the rows they + // may not see are filtered out by RLS rather than the request being refused. + it('direct-path RLS is unchanged: a foreign invoice query is 200 with 0 rows, not a 403', async () => { + const r = await stack.apiAs(contribTok, 'POST', '/data/showcase_invoice/query', { + where: { name: 'INV-7626-FOREIGN' }, + }); + expect(r.status, 'RLS filters, it does not refuse').toBe(200); + const body: any = await r.json(); + expect(recordsOf(body), 'the foreign row is filtered out').toHaveLength(0); + + // …and the persona's own row is still returned by the same door. + const mine = await stack.apiAs(contribTok, 'POST', '/data/showcase_invoice/query', { + where: { name: 'INV-7626-OWN' }, + }); + expect(mine.status).toBe(200); + expect(recordsOf(await mine.json()), 'own row still visible').toHaveLength(1); + }); + + it('a PUBLIC referenced object the persona DOES hold still expands (no over-block)', async () => { + // `showcase_account` is granted to `showcase_contributor` (read). The fix + // must leave a legitimate lookup expansion working — the capability #2850 + // shipped is what the invoice detail page depends on. + const r = await stack.apiAs(contribTok, 'GET', `/data/showcase_invoice/${ownInvoiceId}?expand=account`); + expect(r.status).toBe(200); + const body: any = await r.json(); + const rec = body?.record ?? body; + expect(rec?.account, 'a granted lookup still materialises').toMatchObject({ name: 'Expand Gate Co' }); + }); +}); diff --git a/packages/services/service-storage/src/attachment-read-visibility.test.ts b/packages/services/service-storage/src/attachment-read-visibility.test.ts index 7832def980..3065556b9c 100644 --- a/packages/services/service-storage/src/attachment-read-visibility.test.ts +++ b/packages/services/service-storage/src/attachment-read-visibility.test.ts @@ -286,8 +286,8 @@ describe('installAttachmentReadVisibility', () => { // [#7145] The parent probe reads a DIFFERENT object than the one the security // middleware resolved its depth for, so the caller's envelope crosses over // but the operation-private keys must not: `__readScope` is - // `sys_attachment`'s access DEPTH and `__expandRead` waives the object-level - // CRUD check. Mirrors the same half of #7141 / PR #7143 in the comment kit. + // `sys_attachment`'s access DEPTH and `__expandRead` marks THAT read as an + // expansion sub-read. Mirrors the same half of #7141 / PR #7143 in the comment kit. it('probes the parent with the caller ENVELOPE, minus the operation-private keys', async () => { const { mw, calls } = install(dataset); await runRead(mw, {