fix(security): $expand no longer discloses records the caller is 403'd from — the #2850 expand waiver is removed (#7626)

.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/<id>` → **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.

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

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.
  //

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`

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

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, {

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' }] });
  });
 

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,