From a12751d78077f14b4f6142881a5bbd3ccd593cef Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 14:13:21 +0000 Subject: [PATCH] fix(core): DataScopeManager denies a row on an operator it does not implement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `evaluateFilter`'s `default` arm returned `true`, so a stored `RowLevelFilter` carrying an operator outside the nine the switch implements passed every record the rule existed to hide — silently. The arm now returns `false`, the answer `evaluateCondition` in @object-ui/permissions already gives from its own `default` arm. Red-first test exercises the runtime path stored JSON takes (cast past the closed union, deliberately) and pins the fail-closed answer for an operator outside every published vocabulary and for the spec's published spellings that have no arm. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3 --- ...-datascope-unknown-operator-denies-7378.md | 11 +++ .../core/src/data-scope/DataScopeManager.ts | 21 ++++- .../__tests__/DataScopeManager.test.ts | 86 ++++++++++++++++++- 3 files changed, 114 insertions(+), 4 deletions(-) create mode 100644 .changeset/core-datascope-unknown-operator-denies-7378.md diff --git a/.changeset/core-datascope-unknown-operator-denies-7378.md b/.changeset/core-datascope-unknown-operator-denies-7378.md new file mode 100644 index 0000000000..b2f09e03f8 --- /dev/null +++ b/.changeset/core-datascope-unknown-operator-denies-7378.md @@ -0,0 +1,11 @@ +--- +'@object-ui/core': minor +--- + +`DataScopeManager` now **denies** a row when a row-level scope rule carries an operator its evaluator does not implement. It used to **admit** the row. + +Behaviour change on a permission boundary, stated plainly. `evaluateFilter` implements nine operator spellings — `eq`, `ne`, `gt`, `lt`, `gte`, `lte`, `in`, `nin`, `contains` — and its `default` arm returned `true`, so a stored `RowLevelFilter` carrying any other spelling passed every record the rule existed to hide, silently: no error, no console line, only a result set that was too large, which looks exactly like a correctly configured permissive scope. The arm now returns `false`, the answer `evaluateCondition` in `@object-ui/permissions` already gives from its own `default` arm. Because `applyFilters` ANDs a scope's rules, one unrecognised rule now denies every row in that scope. + +Who this reaches, measured on this release's base rather than assumed. The `RowLevelFilter['operator']` union is closed, so no TypeScript caller can write an unimplemented spelling, and no code in this repository constructs a `RowLevelFilter` outside the evaluator's own test. The path that changes is scope configuration read back from stored or hand-written JSON and handed to `setFilters` / `registerScopeWithConfig`, where the operator arrives as a plain string the type never checked. A deployment holding such a rule with a spelling outside the nine — including the spec's canonical `equals` / `not_equals` / `greater_than` / `starts_with` and the null-ness family `is_null` / `is_not_null`, none of which have an arm — sees fewer rows from that scope after upgrading, never more. Those spellings are not implemented here; they are refused instead of admitted. Whether to canonicalise them through the spec's `canonicalAstOperator` is left open on objectui#7378. + +Graded `minor` because a release reader can observe the narrowing on stored data; the declared type is unchanged and the set of spellings the evaluator accepts has not widened. diff --git a/packages/core/src/data-scope/DataScopeManager.ts b/packages/core/src/data-scope/DataScopeManager.ts index 2189595741..90af27c88d 100644 --- a/packages/core/src/data-scope/DataScopeManager.ts +++ b/packages/core/src/data-scope/DataScopeManager.ts @@ -25,7 +25,11 @@ import type { DataScope, DataContext, DataSource } from '@object-ui/types'; export interface RowLevelFilter { /** Field to filter on */ field: string; - /** Filter operator */ + /** + * Filter operator. The set is closed: a rule whose operator is outside it + * (possible for a rule read back from stored JSON, which the type does not + * guard) evaluates to `false` and denies the row. + */ operator: 'eq' | 'ne' | 'gt' | 'lt' | 'gte' | 'lte' | 'in' | 'nin' | 'contains'; /** Filter value */ value: any; @@ -236,7 +240,10 @@ export class DataScopeManager implements DataContext { } /** - * Evaluate a single filter condition against a field value + * Evaluate a single filter condition against a field value. + * + * An operator the switch does not implement evaluates to `false` (fail + * closed); see the `default` arm. */ function evaluateFilter(fieldValue: any, operator: RowLevelFilter['operator'], filterValue: any): boolean { switch (operator) { @@ -259,7 +266,15 @@ function evaluateFilter(fieldValue: any, operator: RowLevelFilter['operator'], f case 'contains': return typeof fieldValue === 'string' && fieldValue.includes(String(filterValue)); default: - return true; + // Fail closed. A row-level rule this evaluator cannot answer must not + // admit the row it exists to hide: the same answer `evaluateCondition` + // in @object-ui/permissions gives from its own `default` arm, and the + // opposite of the admit-all this arm used to return. The declared union + // above keeps TypeScript callers off this arm; a rule read back from + // stored JSON arrives as a plain string and is not protected by it. + // Silent, like the sibling: the caller sees a narrower result set, not + // a thrown error (objectui#7378). + return false; } } diff --git a/packages/core/src/data-scope/__tests__/DataScopeManager.test.ts b/packages/core/src/data-scope/__tests__/DataScopeManager.test.ts index c28fc20836..a1286073d6 100644 --- a/packages/core/src/data-scope/__tests__/DataScopeManager.test.ts +++ b/packages/core/src/data-scope/__tests__/DataScopeManager.test.ts @@ -7,7 +7,7 @@ */ import { describe, it, expect } from 'vitest'; -import { DataScopeManager } from '../DataScopeManager'; +import { DataScopeManager, type RowLevelFilter } from '../DataScopeManager'; describe('DataScopeManager', () => { describe('Scope Registration', () => { @@ -208,4 +208,88 @@ describe('DataScopeManager', () => { expect(count).toBe(1); }); }); + + describe('Unknown operator fails closed (objectui#7378)', () => { + // Why the cast is here, and why it must stay: `RowLevelFilter['operator']` + // is a closed nine-member union, so TypeScript refuses `'equals'` or + // `'is_null'` at a call site. That protects TypeScript callers and nothing + // else. A scope rule read back from stored JSON reaches `applyFilters` as a + // plain string, and the switch keys on that string at runtime, exactly the + // way it is spelled below. The cast reproduces the path stored data takes; + // it is the point of these tests, not a shortcut to be "cleaned up". A + // test that only spells the nine declared operators cannot reach the + // `default` arm at all. + const storedRule = (field: string, operator: string, value: unknown): RowLevelFilter => + ({ field, operator, value }) as unknown as RowLevelFilter; + + it('does not admit a record it cannot evaluate (operator outside every published vocabulary)', () => { + const manager = new DataScopeManager(); + manager.registerScope('test', { data: [] }); + manager.setFilters('test', [storedRule('status', 'not_an_operator', 'active')]); + + const result = manager.applyFilters('test', [ + { id: 1, status: 'active' }, + { id: 2, status: 'inactive' }, + ]); + + // Fail closed: a rule the evaluator cannot answer denies every row in + // the scope, the same answer `evaluateCondition` in @object-ui/permissions + // gives from its own `default` arm. Before the fix this returned both + // rows, `{ id: 2 }` included, with no error and no console line. + expect(result).toEqual([]); + }); + + it('does not let an unrecognised rule widen a scope another rule narrows (AND semantics)', () => { + const manager = new DataScopeManager(); + manager.registerScope('test', { data: [] }); + manager.setFilters('test', [ + { field: 'status', operator: 'eq', value: 'active' }, + storedRule('tenant', 'not_an_operator', 'acme'), + ]); + + const result = manager.applyFilters('test', [ + { id: 1, status: 'active', tenant: 'acme' }, + { id: 2, status: 'active', tenant: 'other' }, + { id: 3, status: 'inactive', tenant: 'acme' }, + ]); + + // Before the fix the unknown `tenant` rule evaluated to `true`, so the + // `status` rule alone decided and `{ id: 2, tenant: 'other' }` passed — + // the row the second rule existed to hide. Now nothing passes. + expect(result).toEqual([]); + }); + + // The spec's published vocabularies (`VIEW_FILTER_OPERATORS` from + // `@objectstack/spec/ui`, `VALID_AST_OPERATORS` from `@objectstack/spec/data`) + // carry spellings this switch has no arm for: the canonical forms of the + // implemented abbreviations, and the whole null-ness family. Measured on + // @objectstack/spec 17.2.0: 18 of the 20 view operators and 44 of the 53 + // AST operators have no arm. Until they are either implemented or refused + // by name, they MUST take the fail-closed arm rather than the admit-all one. + // A change that implements these spellings rewrites the expectations below + // to the evaluated result; it never deletes the cases. Every case is + // chosen so that a CORRECT evaluation of the spelling admits at least one + // of the two rows: an implementation that lands without rewriting the + // expectation turns red here instead of passing by coincidence. + it.each([ + ['equals', 'status', 'active'], + ['not_equals', 'status', 'active'], + ['greater_than', 'age', 20], + ['not_in', 'status', ['inactive']], + ['starts_with', 'status', 'act'], + ['is_null', 'status', null], + ['is_not_null', 'status', null], + ])('denies rather than admits for the published-but-unimplemented spelling %s', (operator, field, value) => { + const manager = new DataScopeManager(); + manager.registerScope('test', { data: [] }); + manager.setFilters('test', [storedRule(field, operator, value)]); + + const result = manager.applyFilters('test', [ + { id: 1, status: 'active', age: 30 }, + { id: 2, status: null, age: 10 }, + ]); + + expect(result).toEqual([]); + }); + }); });