From ccdfaef7cf6d352bee5a65da410638edd0e4ca22 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:23:24 +0800 Subject: [PATCH 1/3] test(service-storage): give the attachment read-visibility harness real Filter Protocol semantics (#3774) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `attachment-read-visibility.test.ts` faked its engine with a matcher that understood implicit equality and `$in` and nothing else — no `$and`, no `$or`, no `$not`. Every assertion could therefore only check the *shape* of the filter `computeParentVisibilityFilter` emits, never the rows it selects. That gap was load-bearing. The middleware's multi-parent read scope is `{$or:[{parent_object, parent_id:{$in:[…]}}, …]}`, and driver-sql used to compile each `$or` branch's own keys with OR instead of AND (#3774, fixed in #3776) — widening exactly this shape into an in-tenant unauthorized read of other users' attachments. The suite stayed green throughout: a fake that never evaluates `$or` cannot see a `$or` widening bug. - The matcher now implements the protocol for real, mirroring driver-memory's `memory-matcher.ts` and formula's `matches-filter.ts`: every key in a filter object ANDs, `$or` ORs its branches, a branch's own contents still AND. Unimplemented operators now throw rather than being ignored — a silently under-implemented test double is the failure mode this change exists to close. - New case asserts the ROWS a multi-parent-type scope returns: attachments whose `parent_object` matches a branch but whose `parent_id` is absent from that branch's id list — including one borrowing the sibling branch's id — are excluded. Under the driver-sql bug this scope returned every attachment in the tenant instead of two. - A conformance block pins the harness against the same 2x2 fixture and expectations as `memory-matcher-or-semantics.test.ts`, `matches-filter-or-semantics.test.ts` and `sql-driver-or-filter.test.ts`, so the four cannot drift apart, plus a case proving the matcher tells the correct scope from the widened one. Test-only; no runtime change. service-storage: 212/212 green. Co-Authored-By: Claude --- ...t-read-visibility-real-filter-semantics.md | 38 +++ .../src/attachment-read-visibility.test.ts | 220 +++++++++++++++++- 2 files changed, 247 insertions(+), 11 deletions(-) create mode 100644 .changeset/attachment-read-visibility-real-filter-semantics.md diff --git a/.changeset/attachment-read-visibility-real-filter-semantics.md b/.changeset/attachment-read-visibility-real-filter-semantics.md new file mode 100644 index 0000000000..9353c66825 --- /dev/null +++ b/.changeset/attachment-read-visibility-real-filter-semantics.md @@ -0,0 +1,38 @@ +--- +--- + +test(service-storage): give the attachment read-visibility harness real Filter Protocol semantics (#3774) + +Test-only — releases nothing. + +`attachment-read-visibility.test.ts` faked its engine with a matcher that +understood implicit equality and `$in` and nothing else: no `$and`, no `$or`, +no `$not`. Every assertion in the file could therefore only check the *shape* +of the filter `computeParentVisibilityFilter` emits, never the rows that filter +selects. + +That was a load-bearing gap. The middleware's multi-parent read scope is +`{$or:[{parent_object, parent_id:{$in:[…]}}, …]}`, and driver-sql used to +compile each `$or` branch's own keys with OR instead of AND (#3774, fixed in +#3776) — widening exactly this shape into an in-tenant unauthorized read of +other users' attachments. The suite stayed green throughout, because a fake +that never evaluates `$or` cannot see a `$or` widening bug. + +Changes: + +- The harness matcher now implements the protocol for real, mirroring + `driver-memory/memory-matcher.ts` and `formula/matches-filter.ts`: every key + in a filter object ANDs, `$or` ORs its branches, and a branch's own contents + still AND. Operators it does not implement now **throw** instead of being + ignored — a silently under-implemented test double is the failure mode this + change exists to close. +- A new case asserts the **rows** a multi-parent-type scope returns, not just + its shape: attachments whose `parent_object` matches a branch but whose + `parent_id` is absent from that branch's id list (including one that borrows + the sibling branch's id) are excluded. Under the driver-sql bug this scope + returned every attachment in the tenant instead of two. +- A conformance block pins the harness matcher against the same 2x2 fixture and + expectations as `memory-matcher-or-semantics.test.ts`, + `matches-filter-or-semantics.test.ts` and `sql-driver-or-filter.test.ts`, so + the four cannot drift apart, plus a case proving the matcher actually tells + the correct scope from the widened one. 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 bb495fcc9c..29599ebd9f 100644 --- a/packages/services/service-storage/src/attachment-read-visibility.test.ts +++ b/packages/services/service-storage/src/attachment-read-visibility.test.ts @@ -6,6 +6,76 @@ import type { AttachmentLifecycleEngine, AttachmentReadMiddlewareCtx } from './a const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); +/** + * Filter Protocol evaluator for the fake engine below — real `$and` / `$or` / + * `$not` semantics, not a shape check. + * + * Mirrors `driver-memory`'s `memory-matcher.ts` and `formula`'s + * `matches-filter.ts`: **every key inside one filter object ANDs**, a `$or` + * array ORs its branches, and a branch's own contents still AND. + * + * This used to understand neither logical operators nor anything but `$in`, + * so these tests could only assert the *shape* of the filter the middleware + * emits. That was a real blind spot: `computeParentVisibilityFilter` returns + * `{$or:[{parent_object, parent_id:{$in:[…]}}, …]}`, and a driver-sql + * compilation bug (#3774) OR-ed the keys *within* each branch — widening + * exactly this shape into an in-tenant unauthorized read — while every + * assertion here stayed green. A fake that silently under-implements the + * protocol cannot see a widening bug, so this one implements it for real and + * **throws** on anything it does not, rather than quietly matching. + * + * Values compare as strings on purpose: `computeParentVisibilityFilter` + * stringifies every `parent_id` when it builds the `$in` list, so a numeric + * row value still has to match its stringified filter value. + */ +function matchWhere(row: any, where: any): boolean { + if (where === null || where === undefined) return true; + if (typeof where !== 'object' || Array.isArray(where)) { + throw new Error(`harness matcher: a filter must be an object, got ${JSON.stringify(where)}`); + } + // A filter object is the AND of every one of its entries. + return Object.entries(where).every(([key, val]) => { + if (key === '$and') { + if (!Array.isArray(val)) throw new Error('harness matcher: $and expects an array'); + return val.every((branch) => matchWhere(row, branch)); + } + if (key === '$or') { + if (!Array.isArray(val)) throw new Error('harness matcher: $or expects an array'); + // Each branch is evaluated whole — the keys inside it still AND. + return val.some((branch) => matchWhere(row, branch)); + } + if (key === '$not') return !matchWhere(row, val); + if (key.startsWith('$')) { + throw new Error(`harness matcher: unsupported logical operator ${key}`); + } + return matchField(row[key], val); + }); +} + +/** One `field: ` entry. Multiple operators on the same field AND together. */ +function matchField(actual: any, spec: any): boolean { + if (spec === null || spec === undefined) return actual === null || actual === undefined; + if (Array.isArray(spec)) { + throw new Error('harness matcher: a bare array is not a field spec — use { $in: [...] }'); + } + if (typeof spec !== 'object') return String(actual) === String(spec); + return Object.entries(spec).every(([op, target]) => { + switch (op) { + case '$eq': + return String(actual) === String(target); + case '$ne': + return String(actual) !== String(target); + case '$in': + return (target as unknown[]).map(String).includes(String(actual)); + case '$nin': + return !(target as unknown[]).map(String).includes(String(actual)); + default: + // Loud on purpose — an ignored operator is how the original blind spot happened. + throw new Error(`harness matcher: unsupported operator ${op} — implement it, don't let it match silently`); + } + }); +} + /** * Fake engine modelling: a sys_attachment table (system pre-scan) + a * per-parent-object visibility map keyed by userId. A parent find under a @@ -19,16 +89,6 @@ function install(opts: { let mw!: (ctx: AttachmentReadMiddlewareCtx, next: () => Promise) => Promise; const calls = { parentFinds: [] as Array<{ object: string; ids: string[]; userId?: string }> }; - const matchWhere = (row: any, where: any): boolean => { - if (!where || typeof where !== 'object') return true; - return Object.entries(where).every(([k, v]) => { - if (v && typeof v === 'object' && Array.isArray((v as any).$in)) { - return (v as any).$in.map(String).includes(String(row[k])); - } - return String(row[k]) === String(v); - }); - }; - const engine: AttachmentLifecycleEngine = { registerHook: () => {}, registerMiddleware: (fn) => { @@ -51,7 +111,18 @@ function install(opts: { update: async () => ({}), }; installAttachmentReadVisibility(engine, silentLogger()); - return { mw, calls }; + return { + mw, + calls, + /** + * Ids the given `where` actually selects from the fixture — i.e. what a + * spec-conformant driver would return once the middleware has narrowed + * the query. Asserting on this (not just on the filter's shape) is what + * makes a widened scope visible. + */ + selectIds: (where: unknown): string[] => + opts.attachments.filter((r) => matchWhere(r, where)).map((r) => String(r.id)), + }; } /** Drive a read op through the middleware and return the resulting where. */ @@ -111,6 +182,35 @@ describe('installAttachmentReadVisibility', () => { }); }); + it('the multi-parent $or admits only rows matching BOTH keys of one branch', async () => { + // Each parent type has a visible and an invisible record, plus a row whose + // parent_object belongs to one branch while its parent_id appears only in + // the OTHER branch's id list. Nothing but a genuine per-branch AND excludes + // all three: this is the shape driver-sql widened into a cross-tenant read + // by OR-ing a branch's own keys (#3774 / PR #3776). + const { mw, selectIds } = install({ + attachments: [ + { id: 'a1', parent_object: 'att_case', parent_id: 'c1' }, // visible case + { id: 'a2', parent_object: 'att_case', parent_id: 'c2' }, // case u1 cannot read + { id: 'a3', parent_object: 'att_todo', parent_id: 't1' }, // visible todo + { id: 'a4', parent_object: 'att_todo', parent_id: 't2' }, // todo u1 cannot read + { id: 'a5', parent_object: 'att_case', parent_id: 't1' }, // object of branch 1, id of branch 2 + ], + visible: { att_case: { u1: ['c1'] }, att_todo: { u1: ['t1'] } }, + }); + const { where } = await runRead(mw, {}); + + expect(where).toEqual({ + $or: [ + { parent_object: 'att_case', parent_id: { $in: ['c1'] } }, + { parent_object: 'att_todo', parent_id: { $in: ['t1'] } }, + ], + }); + // The rows that filter actually returns — a2/a4 (right parent type, wrong + // id) and a5 (right type, an id borrowed from the sibling branch) are out. + expect(selectIds(where)).toEqual(['a1', 'a3']); + }); + it('denies all when no candidate parent is visible', async () => { const { mw } = install({ attachments: [{ id: 'a3', parent_object: 'att_secret', parent_id: 's1' }], @@ -178,3 +278,101 @@ describe('installAttachmentReadVisibility', () => { expect(calls.parentFinds).toHaveLength(0); }); }); + +/** + * Who tests the test double? The row assertions above are only worth as much + * as the matcher evaluating them, so it gets the same 2x2 fixture and the same + * expectations as the three production backends: + * `driver-memory/memory-matcher-or-semantics.test.ts`, + * `formula/matches-filter-or-semantics.test.ts`, and + * `driver-sql/sql-driver-or-filter.test.ts`. If this harness ever drifts from + * them, a read scope it declares safe would not be safe in the engine. + */ +describe('harness matcher — Filter Protocol $and/$or/$not semantics', () => { + const ROWS = [ + { id: '1', a: 'x', b: 'y', c: 'z' }, + { id: '2', a: 'x', b: 'zz', c: 'z' }, + { id: '3', a: 'qq', b: 'y', c: 'z' }, + { id: '4', a: 'qq', b: 'zz', c: 'z' }, + ]; + const ids = (filter: any): string[] => ROWS.filter((r) => matchWhere(r, filter)).map((r) => r.id); + + it('ANDs the keys of a single multi-key branch', () => { + expect(ids({ $or: [{ a: 'x', b: 'y' }] })).toEqual(['1']); + }); + + it('ANDs the keys of each branch independently', () => { + expect(ids({ $or: [{ a: 'x', b: 'y' }, { a: 'qq', b: 'zz' }] })).toEqual(['1', '4']); + }); + + it('ANDs operator-object keys within a branch', () => { + expect(ids({ $or: [{ a: { $eq: 'x' }, b: { $ne: 'zz' } }] })).toEqual(['1']); + }); + + it('ANDs keys inside a $or nested in a $or branch', () => { + expect(ids({ $or: [{ $or: [{ a: 'x', b: 'zz' }] }] })).toEqual(['2']); + }); + + it('ANDs multiple operators on ONE field within a branch', () => { + expect(ids({ $or: [{ a: { $ne: 'qq', $eq: 'x' }, b: 'y' }] })).toEqual(['1']); + }); + + it('OR-s a $and branch against a sibling multi-key branch', () => { + expect(ids({ $or: [{ $and: [{ a: 'x' }, { b: 'y' }] }, { a: 'qq', b: 'zz' }] })).toEqual(['1', '4']); + }); + + it('ANDs a $and with a sibling key in the same branch, either order', () => { + expect(ids({ $or: [{ c: 'nope' }, { $and: [{ a: 'qq' }], b: 'y' }] })).toEqual(['3']); + expect(ids({ $or: [{ c: 'nope' }, { b: 'y', $and: [{ a: 'qq' }] }] })).toEqual(['3']); + }); + + it('keeps single-key $or branches as a plain OR', () => { + expect(ids({ $or: [{ a: 'x' }, { b: 'y' }] })).toEqual(['1', '2', '3']); + }); + + it('ANDs a $or with a sibling top-level field key', () => { + expect(ids({ $or: [{ a: 'x' }, { b: 'y' }], b: 'zz' })).toEqual(['2']); + }); + + it('negates with $not', () => { + expect(ids({ $not: { a: 'x' } })).toEqual(['3', '4']); + }); + + it('tells the correct attachment scope apart from the widened one (#3774)', () => { + // The proof this matcher can go red: the same clauses, once AND-ed per + // branch and once flattened the way driver-sql compiled them. If both + // returned the same rows, every row assertion in this file would be + // decorative. + const attachments = [ + { id: 'a1', parent_object: 'att_case', parent_id: 'c1' }, + { id: 'a2', parent_object: 'att_case', parent_id: 'c2' }, + { id: 'a3', parent_object: 'att_todo', parent_id: 't1' }, + { id: 'a4', parent_object: 'att_todo', parent_id: 't2' }, + ]; + const pick = (f: any) => attachments.filter((r) => matchWhere(r, f)).map((r) => r.id); + + const correct = { + $or: [ + { parent_object: 'att_case', parent_id: { $in: ['c1'] } }, + { parent_object: 'att_todo', parent_id: { $in: ['t1'] } }, + ], + }; + const widened = { + $or: [ + { parent_object: 'att_case' }, + { parent_id: { $in: ['c1'] } }, + { parent_object: 'att_todo' }, + { parent_id: { $in: ['t1'] } }, + ], + }; + expect(pick(correct)).toEqual(['a1', 'a3']); + expect(pick(widened)).toEqual(['a1', 'a2', 'a3', 'a4']); // every row in the tenant + }); + + it('throws instead of silently ignoring an operator it does not implement', () => { + // The original blind spot in one line: an unimplemented operator must not + // quietly evaluate to a match. + expect(() => ids({ a: { $gt: 'x' } })).toThrow(/unsupported operator \$gt/); + expect(() => ids({ $nor: [{ a: 'x' }] })).toThrow(/unsupported logical operator \$nor/); + }); +}); From b27d7d295b8bef7f87f29d20610c7a083b8ae47d Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:26:46 +0800 Subject: [PATCH 2/3] test(service-storage): say "in-tenant unauthorized read", not "cross-tenant" The #3774 widening leaked attachments to other users inside the same tenant; it did not cross a tenant boundary. Overstating the blast radius in a security test's comment is its own kind of wrong. Co-Authored-By: Claude --- .../service-storage/src/attachment-read-visibility.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 29599ebd9f..08b299eab9 100644 --- a/packages/services/service-storage/src/attachment-read-visibility.test.ts +++ b/packages/services/service-storage/src/attachment-read-visibility.test.ts @@ -186,8 +186,8 @@ describe('installAttachmentReadVisibility', () => { // Each parent type has a visible and an invisible record, plus a row whose // parent_object belongs to one branch while its parent_id appears only in // the OTHER branch's id list. Nothing but a genuine per-branch AND excludes - // all three: this is the shape driver-sql widened into a cross-tenant read - // by OR-ing a branch's own keys (#3774 / PR #3776). + // all three: this is the shape driver-sql widened into an in-tenant + // unauthorized read by OR-ing a branch's own keys (#3774 / PR #3776). const { mw, selectIds } = install({ attachments: [ { id: 'a1', parent_object: 'att_case', parent_id: 'c1' }, // visible case From 6c673fbb3a8cf3c5b4070f0830d540a68f47d623 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:39:53 +0800 Subject: [PATCH 3/3] test(service-storage): describe the harness gap in neutral terms Per this repo's split-disclosure practice for filter-widening (the exploitation path lives in the private advisory, not in public issue/PR/changeset/test prose), drop the specific impact framing from the test comments, the changeset and the case name. The engineering rationale is unchanged and the assertions are untouched: the harness could only check filter shape, not the rows a narrowing predicate selects. Co-Authored-By: Claude --- ...t-read-visibility-real-filter-semantics.md | 22 ++++++---------- .../src/attachment-read-visibility.test.ts | 25 +++++++++---------- 2 files changed, 19 insertions(+), 28 deletions(-) diff --git a/.changeset/attachment-read-visibility-real-filter-semantics.md b/.changeset/attachment-read-visibility-real-filter-semantics.md index 9353c66825..48236c0264 100644 --- a/.changeset/attachment-read-visibility-real-filter-semantics.md +++ b/.changeset/attachment-read-visibility-real-filter-semantics.md @@ -1,7 +1,7 @@ --- --- -test(service-storage): give the attachment read-visibility harness real Filter Protocol semantics (#3774) +test(service-storage): give the attachment read-visibility harness real Filter Protocol semantics Test-only — releases nothing. @@ -9,14 +9,7 @@ Test-only — releases nothing. understood implicit equality and `$in` and nothing else: no `$and`, no `$or`, no `$not`. Every assertion in the file could therefore only check the *shape* of the filter `computeParentVisibilityFilter` emits, never the rows that filter -selects. - -That was a load-bearing gap. The middleware's multi-parent read scope is -`{$or:[{parent_object, parent_id:{$in:[…]}}, …]}`, and driver-sql used to -compile each `$or` branch's own keys with OR instead of AND (#3774, fixed in -#3776) — widening exactly this shape into an in-tenant unauthorized read of -other users' attachments. The suite stayed green throughout, because a fake -that never evaluates `$or` cannot see a `$or` widening bug. +selects — a poor bargain for a predicate whose whole job is narrowing. Changes: @@ -27,12 +20,11 @@ Changes: ignored — a silently under-implemented test double is the failure mode this change exists to close. - A new case asserts the **rows** a multi-parent-type scope returns, not just - its shape: attachments whose `parent_object` matches a branch but whose - `parent_id` is absent from that branch's id list (including one that borrows - the sibling branch's id) are excluded. Under the driver-sql bug this scope - returned every attachment in the tenant instead of two. + its shape: rows whose discriminator matches a branch but whose id is absent + from that branch's id list (including one that borrows the sibling branch's + id) are excluded, so the per-branch pairing itself is pinned. - A conformance block pins the harness matcher against the same 2x2 fixture and expectations as `memory-matcher-or-semantics.test.ts`, `matches-filter-or-semantics.test.ts` and `sql-driver-or-filter.test.ts`, so - the four cannot drift apart, plus a case proving the matcher actually tells - the correct scope from the widened one. + the four cannot drift apart, plus a case proving the matcher actually + distinguishes a correctly paired scope from a widened one. 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 08b299eab9..3cfdd42ded 100644 --- a/packages/services/service-storage/src/attachment-read-visibility.test.ts +++ b/packages/services/service-storage/src/attachment-read-visibility.test.ts @@ -16,13 +16,14 @@ const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); * * This used to understand neither logical operators nor anything but `$in`, * so these tests could only assert the *shape* of the filter the middleware - * emits. That was a real blind spot: `computeParentVisibilityFilter` returns - * `{$or:[{parent_object, parent_id:{$in:[…]}}, …]}`, and a driver-sql - * compilation bug (#3774) OR-ed the keys *within* each branch — widening - * exactly this shape into an in-tenant unauthorized read — while every - * assertion here stayed green. A fake that silently under-implements the - * protocol cannot see a widening bug, so this one implements it for real and - * **throws** on anything it does not, rather than quietly matching. + * emits, never the rows that filter selects. That is a poor bargain for a + * predicate whose whole job is narrowing: `computeParentVisibilityFilter` + * pairs a discriminator with an id list per branch, and any bug that widens + * `$or` — the class #3774 fixed in the SQL compiler — turns that pairing into + * something looser while leaving every shape assertion green. A fake that + * silently under-implements the protocol cannot see a widening bug, so this + * one implements it for real and **throws** on anything it does not, rather + * than quietly matching. * * Values compare as strings on purpose: `computeParentVisibilityFilter` * stringifies every `parent_id` when it builds the `$in` list, so a numeric @@ -186,8 +187,7 @@ describe('installAttachmentReadVisibility', () => { // Each parent type has a visible and an invisible record, plus a row whose // parent_object belongs to one branch while its parent_id appears only in // the OTHER branch's id list. Nothing but a genuine per-branch AND excludes - // all three: this is the shape driver-sql widened into an in-tenant - // unauthorized read by OR-ing a branch's own keys (#3774 / PR #3776). + // all three, so this pins the pairing itself rather than the filter's shape. const { mw, selectIds } = install({ attachments: [ { id: 'a1', parent_object: 'att_case', parent_id: 'c1' }, // visible case @@ -338,11 +338,10 @@ describe('harness matcher — Filter Protocol $and/$or/$not semantics', () => { expect(ids({ $not: { a: 'x' } })).toEqual(['3', '4']); }); - it('tells the correct attachment scope apart from the widened one (#3774)', () => { + it('tells a correctly paired scope apart from a widened one', () => { // The proof this matcher can go red: the same clauses, once AND-ed per - // branch and once flattened the way driver-sql compiled them. If both - // returned the same rows, every row assertion in this file would be - // decorative. + // branch and once flattened to key-level ORs. If both returned the same + // rows, every row assertion in this file would be decorative. const attachments = [ { id: 'a1', parent_object: 'att_case', parent_id: 'c1' }, { id: 'a2', parent_object: 'att_case', parent_id: 'c2' },