From 0abfcc5976bec30d66ff7a7d99aefade20863918 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Tue, 11 Aug 2026 14:19:29 +0000 Subject: [PATCH] fix(spec): NormalizedFilterSchema judges its $and/$or/$not members instead of catching them all (#7711) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The member union's second branch was a non-strict z.object({ $and, $or, $not }) with every key optional, so it accepted any object whatsoever — including every field condition the record branch had just rejected. The group branch is now .strict() and the field-condition branch rules out $-prefixed keys, so a member FieldOperatorsSchema refuses has nowhere to land. Refusal names the offending keys and both valid member shapes. Flips #7713's looseness-control pin (which documented the pre-fix behaviour) and adds a dedicated #7711 pin block. Blast radius measured at zero: no runtime .parse caller outside packages/spec's own tests; drivers and the evaluator consume the lowered FilterCondition, not this AST. --- .../normalized-filter-member-union-strict.md | 53 ++++++ ...07-unknown-key-strictness-ledger.counts.md | 16 +- packages/spec/src/data/filter.test.ts | 163 +++++++++++++++--- packages/spec/src/data/filter.zod.ts | 138 ++++++++++++--- 4 files changed, 321 insertions(+), 49 deletions(-) create mode 100644 .changeset/normalized-filter-member-union-strict.md diff --git a/.changeset/normalized-filter-member-union-strict.md b/.changeset/normalized-filter-member-union-strict.md new file mode 100644 index 0000000000..7af19dda9d --- /dev/null +++ b/.changeset/normalized-filter-member-union-strict.md @@ -0,0 +1,53 @@ +--- +"@objectstack/spec": minor +--- + +fix(spec): `NormalizedFilterSchema` judges its members instead of catching them all (#7711) + +Each `$and` / `$or` member and the `$not` operand was +`z.union([z.record(z.string(), FieldOperatorsSchema), NormalizedFilterSchema])`, +and the second branch was a NON-strict `z.object({ $and, $or, $not })` with every +key optional. "All three of my optional keys are absent" is true of any object +whatsoever, so that branch was a catch-all: whenever the record branch **rejected** +a field condition, the group branch accepted the very same value. The whole-filter +face validated the logical skeleton and nothing else — no comparand shape it +declared could ever make it fail. + +Measured before the change, every one of these parsed green while +`FieldOperatorsSchema` — the copy this schema is documented as validating +against — refused the identical operator map: + +``` +{ $and: [{ c: { $null: 'not-a-boolean' } }] } +{ $and: [{ c: { $between: [1, 2, 3] } }] } +{ $and: [{ hello: 'world' }] } +``` + +The green was also lossy: an admitted member came back parsed to `{}`, so the +accepted output no longer carried the condition it was asked about — a whole +`{ $not: … }` subtree parsed as a field named `$not` and returned empty. + +Now the group branch is `.strict()` and the field-condition branch rules out +`$`-prefixed keys, so a member the operator map refuses has nowhere to land. The +refusal names the offending keys and both valid member shapes: + +``` +Not a valid $and member — got an object with key(s) "c". A $and member is either +a FIELD CONDITION ({ "field": { "$op": value } }, whose keys are field names and +whose operator map must satisfy FieldOperatorsSchema — comparand shapes +included), or a nested LOGICAL GROUP carrying only $and / $or / $not and nothing +else. Ruled on #7711: declared = enforced (ADR-0049). +``` + +Graded a narrowing of the accepted surface, so `minor` rather than `patch` — but +the blast radius is measured at zero: nothing in the repo called `.parse` / +`.safeParse` on `NormalizedFilterSchema` outside this package's own tests +(swept with `FilterConditionSchema`'s 20-plus call sites as the positive +control), no driver or evaluator references the normalized AST at all, and the +exported `NormalizedFilter` TYPE is unchanged — every shape that stops parsing +was already outside it. Nothing is removed from the declared surface, which is +what separates this from the ADR-0087 removal grade. + +`FilterConditionSchema`, the AUTHORING face every driver, `read-scope-sql` and +`cel-to-filter` actually consume, is untouched and still admits sugar by design, +so no request path's row set can move with this. diff --git a/docs/audits/2026-07-unknown-key-strictness-ledger.counts.md b/docs/audits/2026-07-unknown-key-strictness-ledger.counts.md index 766f67aa10..14210ab097 100644 --- a/docs/audits/2026-07-unknown-key-strictness-ledger.counts.md +++ b/docs/audits/2026-07-unknown-key-strictness-ledger.counts.md @@ -22,7 +22,7 @@ regenerate. |---|---| | Triaged directories | 5 | | Object sites in them | 438 | -| Still-open (strip) sites | 181 | +| Still-open (strip) sites | 180 | | Files carrying at least one | 27 | Remaining strip sites by class: @@ -31,7 +31,7 @@ Remaining strip sites by class: |---|---| | authorable — the ruling's forced scope | 40 | | unresolved — needs a per-schema verdict | 34 | -| wire / open — out of forced scope | 105 | +| wire / open — out of forced scope | 104 | | no door — no carrier, ADR-0049 territory | 1 | | no gate — carrier live, no parse | 0 | | covered — no carrier, no parse, guarded at every consumer | 1 | @@ -45,11 +45,11 @@ The `strict` column is the one the campaign schedules against; it counts both th | Dir | Sites | strict | passthrough | catchall | strip | |---|---|---|---|---|---| | `ui/` | 161 | 119 | 5 | 0 | 37 | -| `data/` | 165 | 56 | 1 | 0 | 108 | +| `data/` | 165 | 57 | 1 | 0 | 107 | | `automation/` | 65 | 42 | 0 | 0 | 23 | | `security/` | 20 | 7 | 0 | 0 | 13 | | `studio/` | 27 | 27 | 0 | 0 | 0 | -| **total** | **438** | **251** | **6** | **0** | **181** | +| **total** | **438** | **252** | **6** | **0** | **180** | ## File-level triage — site counts @@ -179,7 +179,7 @@ over it is here. ### `data/` — open -**108 strip of 165**, in 16 file(s). +**107 strip of 165**, in 16 file(s). | File | Strip | Sites | |---|---|---| @@ -194,18 +194,18 @@ over it is here. | `external-lookup.zod.ts` | 12 | 12 | | `field-value.zod.ts` | 2 | 3 | | `field.zod.ts` | 2 | 10 | -| `filter.zod.ts` | 11 | 11 | +| `filter.zod.ts` | 10 | 11 | | `hook.zod.ts` | 5 | 7 | | `object.zod.ts` | 1 | 20 | | `query.zod.ts` | 4 | 5 | | `seed-loader.zod.ts` | 12 | 12 | -| **total** | **108** | **165** | +| **total** | **107** | **165** | | Bucket | Sites | |---|---| | authorable — the ruling's forced scope | 8 | | unresolved — needs a per-schema verdict | 34 | -| wire / open — out of forced scope | 66 | +| wire / open — out of forced scope | 65 | | no door — no carrier, ADR-0049 territory | 0 | | no gate — carrier live, no parse | 0 | | covered — no carrier, no parse, guarded at every consumer | 0 | diff --git a/packages/spec/src/data/filter.test.ts b/packages/spec/src/data/filter.test.ts index e2fc01458c..b8bbb1f602 100644 --- a/packages/spec/src/data/filter.test.ts +++ b/packages/spec/src/data/filter.test.ts @@ -432,33 +432,41 @@ describe('RangeOperatorSchema', () => { }); /** - * ## `NormalizedFilterSchema` cannot go red on this, and that is NOT this - * ruling's doing — measured, and pinned so the next reader does not mistake - * the green for enforcement. + * ## The whole-filter face now goes red on this too — #7711 closed the union's catch-all * - * A `$and` member is `z.union([z.record(z.string(), FieldOperatorsSchema), - * NormalizedFilterSchema])`. When the record branch rejects a field - * condition, the SECOND branch is a non-strict `z.object({ $and, $or, $not - * })` with every key optional — which accepts any object whatsoever. So the - * whole-filter face admits every field-condition shape, and the control - * below shows it does so for a comparand nobody has ever declared valid. - * The enforcement that this ruling moves lives one level down, in - * `FieldOperatorsSchema`, which the tests above assert directly. Filed - * separately as its own finding; asserting a red here would have been a - * fabricated pin. + * This case was written by #7596 as the OPPOSITE pin: both inputs parsed + * GREEN through `NormalizedFilterSchema`, and it said so on purpose, so the + * green would not be read as enforcement. The reason was structural — a + * `$and` member was `z.union([z.record(z.string(), FieldOperatorsSchema), + * NormalizedFilterSchema])` against a NON-strict group whose every key is + * optional, and "all three of my optional keys are absent" is true of any + * object at all. When the record branch rejected a field condition, the + * group branch accepted the very same value. + * + * #7711 made that branch `.strict()` and ruled `$`-prefixed keys out of the + * field-condition branch, so a member the record branch refuses now has + * nowhere to land. The case is kept in place, flipped rather than deleted: + * the two `FieldOperatorsSchema` assertions below are unchanged (that level + * always rejected both), and the pair above them is what moved — which is + * exactly the evidence that the two faces now agree. */ - it('the whole-filter face is loose about field conditions — pre-existing, control included', () => { + it('the whole-filter face refuses these field conditions too — #7711 (was green pre-#7711)', () => { const withReference = NormalizedFilterSchema.safeParse({ $and: [{ amount: { $between: [{ $field: 'budget' }, 100] } }], }); const alreadyInvalidComparand = NormalizedFilterSchema.safeParse({ $and: [{ close_date: { $null: 'not-a-boolean' } }], }); - // Both green, for the same structural reason — the second has nothing to - // do with #7596 and was green before it. - expect(withReference.success).toBe(true); - expect(alreadyInvalidComparand.success).toBe(true); - // And the level that DOES judge comparands rejects both. + // Both red now, for the same structural reason they were both green + // before: one union branch used to accept whatever the other refused. + expect(withReference.success).toBe(false); + expect(alreadyInvalidComparand.success).toBe(false); + // Message-bearing, and it names the member position rather than the + // generic "Invalid input" zod gives an unexplained union. + expect(withReference.error?.issues[0]?.code).toBe('invalid_union'); + expect(withReference.error?.issues[0]?.message).toContain('Not a valid $and member'); + expect(alreadyInvalidComparand.error?.issues[0]?.message).toContain('#7711'); + // And the level that DOES judge comparands rejects both — unchanged. expect(FieldOperatorsSchema.safeParse({ $between: [{ $field: 'budget' }, 100] }).success) .toBe(false); expect(FieldOperatorsSchema.safeParse({ $null: 'not-a-boolean' }).success).toBe(false); @@ -1206,9 +1214,124 @@ describe('NormalizedFilterSchema', () => { deleted: { $eq: true } } }; - + expect(() => NormalizedFilterSchema.parse(filter)).not.toThrow(); }); + + // ========================================================================== + // #7711 — the union's group branch is no longer a catch-all. + // + // Every member below parsed GREEN before this change, measured on main @ + // `8669e5d`: a member is `z.union([, NormalizedFilterSchema])` + // and the second branch was a non-strict `z.object({ $and, $or, $not })` with + // every key optional, so it accepted any object whatsoever — including the + // ones the first branch had just refused. The whole-filter face therefore + // validated the logical SKELETON and nothing else, and no comparand shape it + // declared could make it fail. + // + // The green was also LOSSY, which is what these cases pin on the accept side + // as well: the catch-all returned `{}` for the member it admitted, so the + // parse output no longer carried the condition it was asked about. + // ========================================================================== + describe('#7711 — a member the field-condition branch refuses has nowhere to land', () => { + /** The card's clean control: `$null` has never been anything but a boolean. */ + it('rejects a $and member whose operator map FieldOperatorsSchema refuses', () => { + const result = NormalizedFilterSchema.safeParse({ + $and: [{ c: { $null: 'not-a-boolean' } }], + }); + expect(result.success).toBe(false); + expect(result.error?.issues[0]?.code).toBe('invalid_union'); + expect(result.error?.issues[0]?.path).toEqual(['$and', 0]); + expect(result.error?.issues[0]?.message).toContain('Not a valid $and member'); + // Message-bearing per lane convention: it names what was rejected and + // what the two valid member shapes are. + expect(result.error?.issues[0]?.message).toContain('"c"'); + expect(result.error?.issues[0]?.message).toContain('FieldOperatorsSchema'); + expect(result.error?.issues[0]?.message).toContain('$and / $or / $not'); + // The other face always said no — that disagreement is the defect. + expect(FieldOperatorsSchema.safeParse({ $null: 'not-a-boolean' }).success).toBe(false); + }); + + it('rejects the same shape in $or and in the $not operand', () => { + const inOr = NormalizedFilterSchema.safeParse({ $or: [{ c: { $between: [1, 2, 3] } }] }); + const inNot = NormalizedFilterSchema.safeParse({ $not: { c: { $null: 'nope' } } }); + expect(inOr.success).toBe(false); + expect(inNot.success).toBe(false); + expect(inOr.error?.issues[0]?.message).toContain('Not a valid $or member'); + expect(inNot.error?.issues[0]?.message).toContain('Not a valid $not operand'); + expect(FieldOperatorsSchema.safeParse({ $between: [1, 2, 3] }).success).toBe(false); + }); + + it('rejects a bad member nested inside a legitimate group', () => { + const result = NormalizedFilterSchema.safeParse({ + $and: [{ $or: [{ c: { $null: 'nope' } }] }], + }); + expect(result.success).toBe(false); + }); + + /** + * The `$not`-as-a-field-name leak, and why the field-condition branch has to + * refuse `$`-keys rather than the group branch merely being `.strict()`. + * `FieldOperatorsSchema` is not `.strict()`, so `{ c: … }` read as an + * OPERATOR MAP has every key stripped and returns `{}` — which made + * `{ $not: }` parse as a field condition named `$not`. + */ + it('does not let a $not group be read as a field named "$not"', () => { + const result = NormalizedFilterSchema.safeParse({ + $and: [{ $not: { c: { $null: 'nope' } } }], + }); + expect(result.success).toBe(false); + expect(result.error?.issues[0]?.message).toContain('Not a valid $and member'); + }); + + it('rejects members that are not field conditions at all', () => { + // Arbitrary keys — the shape the catch-all could never refuse. + expect(NormalizedFilterSchema.safeParse({ $and: [{ hello: 'world' }] }).success).toBe(false); + // Implicit equality is AUTHORING sugar; this AST is post-normalization, + // so `{ age: 18 }` is `{ age: { $eq: 18 } }` by the time it gets here. + expect(NormalizedFilterSchema.safeParse({ $and: [{ age: 18 }] }).success).toBe(false); + // A group key and a field key at one level. Pre-#7711 this parsed to + // `{ $and: [{ $or: [] }] }` — the field condition dropped on the floor — + // so no mixed member ever survived a round-trip to begin with. + expect(NormalizedFilterSchema.safeParse({ $and: [{ $or: [], c: { $eq: 1 } }] }).success) + .toBe(false); + }); + + it('rejects unknown keys at the top level', () => { + const bogusCombinator = NormalizedFilterSchema.safeParse({ $nand: [] }); + expect(bogusCombinator.success).toBe(false); + expect(bogusCombinator.error?.issues[0]?.code).toBe('unrecognized_keys'); + // A field condition is not a top-level normalized filter either; the AST's + // root is a group. Pre-#7711 this parsed to `{}`. + expect(NormalizedFilterSchema.safeParse({ c: { $null: 'nope' } }).success).toBe(false); + }); + + /** + * The empty combinators are boolean IDENTITIES under the #5322 ruling + * documented on `FilterConditionSchema`, not malformed members. `{}` reaches + * the field-condition branch as an empty record, which is what lets the + * group branch be `.strict()` without touching any of them. + */ + it('leaves the #5322 empty-combinator identities accepted', () => { + for (const identity of [{}, { $and: [] }, { $or: [] }, { $or: [{}] }, { $not: {} }]) { + expect(NormalizedFilterSchema.safeParse(identity).success).toBe(true); + } + }); + + /** + * The accept side of the same fix. The catch-all did not merely admit these + * shapes' bad siblings — it ERASED the member it admitted, so a `$not` + * subtree came back as `{}`. Judged by the right branch, the parse output + * now carries what went in. + */ + it('preserves a nested group in the parse output instead of erasing it', () => { + const input = { $and: [{ $not: { deleted: { $eq: true } } }] }; + const result = NormalizedFilterSchema.safeParse(input); + expect(result.success).toBe(true); + // Pre-#7711 this was `{ $and: [{ $not: {} }] }` — green, and empty. + expect(result.data).toEqual(input); + }); + }); }); // ============================================================================ diff --git a/packages/spec/src/data/filter.zod.ts b/packages/spec/src/data/filter.zod.ts index 6fabd0698e..d9ee061114 100644 --- a/packages/spec/src/data/filter.zod.ts +++ b/packages/spec/src/data/filter.zod.ts @@ -1268,6 +1268,77 @@ export type NormalizedFilter = { $not?: Record | NormalizedFilter; }; +/** + * [#7711] The only keys a normalized LOGICAL GROUP may carry. + * + * Written out as a value rather than derived from the schema because the + * field-condition branch needs it too — it is what tells a field NAME from a + * combinator, and a normalized field condition never spells either. + */ +const NORMALIZED_LOGICAL_KEYS = ['$and', '$or', '$not'] as const; + +/** + * [#7711] The author-facing refusal for a `$and` / `$or` member, or a `$not` + * operand, that is neither a field condition nor a logical group. + * + * It names the offending keys because that is the only part a reader cannot + * reconstruct: the two valid member shapes are fixed, the input's own keys are + * what decided which branch it was judged by and why that branch said no. + */ +function normalizedMemberMessage(position: string, input: unknown): string { + const isPlainObject = !!input && typeof input === 'object' && !Array.isArray(input); + const found = !isPlainObject + ? `got ${Array.isArray(input) ? 'an array' : input === null ? 'null' : typeof input}` + : `got an object with key(s) ${Object.keys(input as Record) + .map((key) => JSON.stringify(key)).join(', ')}`; + return ( + `Not a valid ${position} — ${found}. A ${position} is either a FIELD CONDITION ` + + '({ "field": { "$op": value } }, whose keys are field names and whose operator map ' + + 'must satisfy FieldOperatorsSchema — comparand shapes included), or a nested LOGICAL ' + + `GROUP carrying only ${NORMALIZED_LOGICAL_KEYS.join(' / ')} and nothing else. ` + + 'Ruled on #7711: declared = enforced (ADR-0049).' + ); +} + +/** + * [#7711] A field condition, with `$`-prefixed keys ruled out. + * + * The refusal is what DISCRIMINATES the member union. Without it a member is + * judged by whichever branch happens to answer first, and `{ $not: … }` answers + * on this one: `$not` reads as a field NAME, its operand reads as an operator + * map, and `FieldOperatorsSchema` — not `.strict()`, by the design its own + * docblock argues — strips every key of it and returns `{}`. So a whole negated + * subtree used to parse green *as a field condition* and come back erased. + * A normalized field condition's keys are field names (`amount`, + * `account.name`); none of them starts with `$`. + */ +const normalizedFieldConditionSchema = () => + z.record(z.string(), FieldOperatorsSchema).refine( + (condition) => !Object.keys(condition).some((key) => key.startsWith('$')), + { + message: 'A field condition\'s keys are field names, never $-prefixed operators (#7711).', + // `abort` so this branch cannot become the union's spokesman. Measured on + // zod 4.4.3: a union whose other options all abort returns a lone + // CONTINUABLE failure verbatim, which made `{ $not: { c: } }` — a + // legitimate group shape with a bad operand — report "keys are field + // names", blaming `$not` for being spelled at all. Aborting keeps the + // union's own `error` in charge of every rejection. + abort: true, + }, + ); + +/** + * [#7711] One `$and` / `$or` member, or the `$not` operand. + * + * `position` only shapes the refusal text; the two accepted shapes are the same + * in all three slots, which is why they are built here once rather than spelled + * out per key as they were before. + */ +const normalizedMemberSchema = (position: string) => + z.union([normalizedFieldConditionSchema(), NormalizedFilterSchema], { + error: (issue) => normalizedMemberMessage(position, issue.input), + }); + /** * Zod schema for the normalized filter AST. * @@ -1281,30 +1352,55 @@ export type NormalizedFilter = { * rather than authored, and carries no `.default()` or `.transform()`, so input * and output are the same type. Leaving `Input` at its `unknown` default would * make `z.input` of anything embedding it `unknown` for no reason. + * + * ## The group branch is `.strict()`, and the field branch refuses `$`-keys (#7711) + * + * Each member used to be `z.union([z.record(z.string(), FieldOperatorsSchema), + * NormalizedFilterSchema])` against a NON-strict group whose every key is + * optional — and "all three of my optional keys are absent" is true of any + * object whatsoever. So the second branch was a catch-all: whenever the record + * branch REJECTED a field condition, the group branch accepted the very same + * value, and this face answered `true` for comparands no backend resolves. + * Measured on `main` @ `8669e5d`, all green before: `{ $and: [{ c: { $null: + * 'not-a-boolean' } } ] }`, `{ $and: [{ c: { $between: [1, 2, 3] } }] }`, + * `{ $and: [{ hello: 'world' }] }` — while `FieldOperatorsSchema`, which this + * schema is documented as validating against, refuses each one. + * + * The green was also LOSSY, which is the part that decided the fix over the + * alternative of deleting the "ENFORCED" claim from the two comments above: + * a value admitted by the catch-all came back parsed to `{}`, so the accepted + * output no longer contained the condition it was asked about. That is what + * makes the mixed shape the strictness might have cost — a group key and a + * field key at one level, `{ $or: [], c: { $eq: 1 } }` — a non-loss: it parsed + * to `{ $or: [] }`, the field condition dropped on the floor, so no mixed + * member ever survived a round-trip to begin with. Nothing legitimate spells it, + * and the normalizer's own output is `$and`/`$or`/`$not` groups over + * single-purpose field conditions. + * + * What stays accepted, deliberately: `{}` at any position, and the empty + * combinators — `{}`, `{ $and: [] }`, `{ $or: [] }`, `{ $or: [{}] }`, + * `{ $not: {} }` are boolean identities under the #5322 ruling documented on + * {@link FilterConditionSchema}, not malformed members, and each still parses. + * `{}` reaches the field-condition branch as an empty record, so the group + * branch is free to be strict without touching them. + * + * Scope, stated because the neighbouring face reads similar: this narrows the + * NORMALIZED AST only. {@link FilterConditionSchema} — the AUTHORING face every + * driver, `read-scope-sql` and `cel-to-filter` actually consume — is untouched + * and still admits sugar (implicit equality, relation nesting) by design, so no + * request path's row set can move with this. At the time of the change nothing + * in the repo called `.parse` / `.safeParse` on this schema outside this + * package's own tests (swept with `FilterConditionSchema`'s 20+ call sites as + * the positive control), so the narrowing has no measured internal caller. */ export const NormalizedFilterSchema: z.ZodType = z.lazy(() => z.object({ - $and: z.array( - z.union([ - // Field condition: { field: { $op: value } } - z.record(z.string(), FieldOperatorsSchema), - // Nested logical group - NormalizedFilterSchema, - ]) - ).optional(), - - $or: z.array( - z.union([ - z.record(z.string(), FieldOperatorsSchema), - NormalizedFilterSchema, - ]) - ).optional(), - - $not: z.union([ - z.record(z.string(), FieldOperatorsSchema), - NormalizedFilterSchema, - ]).optional(), - }) + $and: z.array(normalizedMemberSchema('$and member')).optional(), + + $or: z.array(normalizedMemberSchema('$or member')).optional(), + + $not: normalizedMemberSchema('$not operand').optional(), + }).strict() ); // ============================================================================