fix(plugin-security)!: a field the caller may not read is judged as a cross-field comparand exactly as it is as a filter key (#20932)

.changeset/20932-predicate-guard-comparand.md

@@ -0,0 +1,17 @@
+---
+'@objectstack/plugin-security': minor
+---
+
+fix(plugin-security)!: a field the caller may not read is refused as a cross-field comparand exactly as it is refused as a filter key (#20932)
+
+Clause-②: no (narrowing)
+
+<!-- adr-0087: not-required (no-migration-prescription) a refusal of a QUERY at the security layer's field predicate guard: a query that names a field the caller's field-level permissions hide as a cross-field comparand now answers the 403 PERMISSION_DENIED the same field already answers as a filter key. No authorable key, spelling, export or stored metadata shape moves: FieldReferenceSchema, every filter shape and every object definition parse as before, and @objectstack/plugin-security exports nothing new and nothing less (collectConditionFields, collectQueryFields and assertReadableQueryFields keep their signatures). There is nothing for objectstack migrate meta to rewrite, since what changes is which caller may run a query, not what any metadata says. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id covers a permission verdict and this diff adds none (not registered / already-registered); and the change is runtime behaviour, not a declaration (not runtime-interface-only / type-surface-only). -->
+
+**BREAKING for queries that compare a column against a field the caller may not read.**
+
+**What changed.** The security layer refuses a query that filters, sorts, groups or aggregates by a field the caller's field-level permissions hide, with `403 PERMISSION_DENIED`: which rows answer would disclose the value that the field mask withholds from the result. A cross-field comparand (`FieldReferenceSchema`, "compare against another column of the same row") reads the field it names in the same way, and it is now collected into the same set and judged by the same rule. A hidden field named as a comparand, in any position the filter grammar admits for one, in `where`, `having` or a per-aggregation `filter`, answers the same `403 PERMISSION_DENIED`, in the same words, as the same field written as a filter key. This covers `engine.find`, `findOne`, `count`, `aggregate` and the bulk `update` / `delete` predicate, and every route that reaches them. Before, such a comparison was answered.
+
+**What is not affected.** A comparand naming a field the caller may read answers as before. A system context, and a caller with no permission sets, are unaffected. Row-level policies may still compare against fields the caller cannot read, because they are applied after the guard. A comparand the filter grammar refuses is still refused; when it names a hidden field, that refusal may now be the `403` rather than `400 INVALID_FILTER`, as it already was for a hidden filter key.
+
+**If a query stopped answering for some users,** it compares against a field those users may not read. Grant that field's read permission to the users who need it, or compare against a field they can read.

packages/plugins/plugin-security/src/predicate-guard.test.ts

@@ -102,3 +102,99 @@ describe('assertReadableQueryFields', () => {
     expect(() => assertReadableQueryFields({ where: { salary: 1 } }, {}, 'employee')).not.toThrow();
   });
 });
+
+/**
+ * A cross-field comparand names a field exactly as a condition key does: the
+ * comparison reads that field's value, so it is collected by the same walk and
+ * judged by the same rule. The positions below are every one the filter
+ * grammar admits for a comparand (`FieldReferenceSchema`, `data/filter.zod.ts`):
+ * the whole comparand of the six scalar comparisons, the whole-day offset
+ * wrapper (its base and its offset column), under any logical nesting, in each
+ * clause that carries a condition.
+ */
+describe('a cross-field comparand is collected and judged like a condition key', () => {
+  const SEALED = { sealed_n: { readable: false, editable: false } };
+  const ref = (field: string, extra: Record<string, unknown> = {}) => ({ $field: field, ...extra });
+
+  it.each(['$eq', '$ne', '$gt', '$gte', '$lt', '$lte'])(
+    'collects the field a %s comparand names',
+    (op) => {
+      expect([...collectConditionFields({ seen_a: { [op]: ref('seen_b') } })].sort()).toEqual(['seen_a', 'seen_b']);
+    },
+  );
+
+  it('collects a comparand under every logical nesting', () => {
+    const fields = collectConditionFields({
+      $and: [
+        { $or: [{ seen_a: 1 }, { seen_b: { $gt: ref('seen_c') } }] },
+        { $not: { seen_d: { $lte: ref('seen_e') } } },
+      ],
+    });
+    expect([...fields].sort()).toEqual(['seen_a', 'seen_b', 'seen_c', 'seen_d', 'seen_e']);
+  });
+
+  it('collects both fields a whole-day offset comparand names: its base and its offset column', () => {
+    expect([...collectConditionFields({ seen_day: { $lte: ref('base_day', { addDays: 3 }) } })].sort())
+      .toEqual(['base_day', 'seen_day']);
+    expect([...collectConditionFields({ seen_day: { $lte: ref('base_day', { addDays: ref('offset_n') }) } })].sort())
+      .toEqual(['base_day', 'offset_n', 'seen_day']);
+  });
+
+  it('gates a dotted comparand on its first segment, as it gates a dotted key', () => {
+    expect([...collectConditionFields({ seen_a: { $gt: ref('link.seen_b') } })].sort()).toEqual(['link', 'seen_a']);
+  });
+
+  it('collects comparands in where, having and a per-aggregation filter', () => {
+    const fields = collectQueryFields({
+      where: { seen_a: { $gt: ref('in_where') } },
+      having: { total: { $lt: ref('in_having') } },
+      aggregations: [{ function: 'count', alias: 'n', filter: { seen_b: { $ne: ref('in_agg_filter') } } }],
+    });
+    expect(['in_where', 'in_having', 'in_agg_filter'].filter((f) => !fields.has(f))).toEqual([]);
+  });
+
+  /** The refusal a query gets, or `undefined` when it is admitted. */
+  function refusalOf(ast: Record<string, unknown>): { code?: unknown; statusCode?: unknown; message?: unknown; details?: unknown } | undefined {
+    try {
+      assertReadableQueryFields(ast, SEALED, 'probe_object');
+    } catch (e) {
+      return e as { code?: unknown; statusCode?: unknown; message?: unknown; details?: unknown };
+    }
+    return undefined;
+  }
+
+  /** The hidden field named as a KEY — the reference answer every comparand position is held to. */
+  const KEY_FORM = refusalOf({ where: { sealed_n: { $gt: 1 } } });
+
+  it('the key form is refused 403 PERMISSION_DENIED (the reference)', () => {
+    expect(isPermissionDeniedError(KEY_FORM)).toBe(true);
+    expect({ code: KEY_FORM?.code, status: KEY_FORM?.statusCode }).toEqual({ code: 'PERMISSION_DENIED', status: 403 });
+  });
+
+  const POSITIONS: Array<[string, Record<string, unknown>]> = [
+    ...['$eq', '$ne', '$gt', '$gte', '$lt', '$lte'].map(
+      (op): [string, Record<string, unknown>] => [`the whole comparand of ${op}`, { where: { seen_a: { [op]: ref('sealed_n') } } }],
+    ),
+    ['a comparand under $and', { where: { $and: [{ seen_b: 'x' }, { seen_a: { $gt: ref('sealed_n') } }] } }],
+    ['a comparand under $or', { where: { $or: [{ seen_b: 'x' }, { seen_a: { $gt: ref('sealed_n') } }] } }],
+    ['a comparand under $not', { where: { $not: { seen_a: { $gt: ref('sealed_n') } } } }],
+    ['the base of a whole-day offset comparand', { where: { seen_day: { $lte: ref('sealed_n', { addDays: 1 }) } } }],
+    ['the offset column of a whole-day offset comparand', { where: { seen_day: { $lte: ref('base_day', { addDays: ref('sealed_n') }) } } }],
+    ['a comparand in having', { having: { total: { $gt: ref('sealed_n') } } }],
+    ['a comparand in a per-aggregation filter', { aggregations: [{ function: 'count', alias: 'n', filter: { seen_a: { $gt: ref('sealed_n') } } }] }],
+  ];
+
+  it.each(POSITIONS)('a hidden field as %s answers the key form\'s refusal', (_position, ast) => {
+    const refusal = refusalOf(ast);
+    expect(isPermissionDeniedError(refusal)).toBe(true);
+    expect({ code: refusal?.code, status: refusal?.statusCode, message: refusal?.message, details: refusal?.details })
+      .toEqual({ code: KEY_FORM?.code, status: KEY_FORM?.statusCode, message: KEY_FORM?.message, details: KEY_FORM?.details });
+  });
+
+  it('CONTROL a readable comparand in the same positions is admitted', () => {
+    for (const [, ast] of POSITIONS) {
+      const readable = JSON.parse(JSON.stringify(ast).replaceAll('sealed_n', 'seen_c')) as Record<string, unknown>;
+      expect(refusalOf(readable)).toBeUndefined();
+    }
+  });
+});

packages/plugins/plugin-security/src/predicate-guard.ts

@@ -23,6 +23,7 @@
  * the caller cannot read (e.g. `owner_id`), and must not be rejected.
  */
 
+import { FieldReferenceSchema } from '@objectstack/spec/data';
 import { PermissionDeniedError } from './errors.js';
 
 interface FieldPermissionLike {
@@ -36,6 +37,12 @@ const LOGICAL_KEYS = new Set(['$and', '$or', '$not']);
  * Collect every field name referenced by a FilterCondition. Dotted paths
  * and nested-relation conditions gate on their FIRST segment / top-level
  * relation field — local field permissions govern local traversal.
+ *
+ * A field is referenced by a condition KEY and equally by a cross-field
+ * COMPARAND (`FieldReferenceSchema`, `{ $field }` — "compare against another
+ * column of the same row"): the comparison reads the named field's value, so
+ * row presence discloses it exactly as a filter on the field itself would.
+ * Both are collected here, into the one set the one field rule judges.
  */
 export function collectConditionFields(condition: unknown, out: Set<string> = new Set()): Set<string> {
   if (!condition || typeof condition !== 'object' || Array.isArray(condition)) return out;
@@ -46,10 +53,40 @@ export function collectConditionFields(condition: unknown, out: Set<string> = ne
       continue;
     }
     out.add(key.split('.')[0]);
+    collectComparandFields(value, out);
   }
   return out;
 }
 
+/**
+ * Collect the field every cross-field comparand inside one field constraint
+ * names, gated on its first segment like a key.
+ *
+ * What a comparand reference IS is the filter grammar's answer, not this
+ * guard's: each node is asked of `FieldReferenceSchema` itself, the spec's
+ * declaration of the reference (its `$field` and the whole-day `addDays`
+ * offset's own nested reference) — the reading `objectql`'s aggregation
+ * filter also takes. The walk carries no operator list and no position list:
+ * it visits every node of the constraint (operator maps, list members, a
+ * reference's own offset), so a position the grammar admits is covered
+ * without being named here, and one it later admits is covered the day it
+ * does.
+ *
+ * A node the schema refuses is not collected: every executor refuses the
+ * same malformed reference (`INVALID_FILTER`) instead of reading a field
+ * through it, so there is no value for it to disclose.
+ */
+function collectComparandFields(operand: unknown, out: Set<string>): void {
+  if (!operand || typeof operand !== 'object') return;
+  if (Array.isArray(operand)) {
+    for (const member of operand) collectComparandFields(member, out);
+    return;
+  }
+  const reference = FieldReferenceSchema.safeParse(operand);
+  if (reference.success) out.add(reference.data.$field.split('.')[0]);
+  for (const member of Object.values(operand as Record<string, unknown>)) collectComparandFields(member, out);
+}
+
 /**
  * Collect every field referenced by the query's row-shaping clauses:
  * where / orderBy / groupBy / having / aggregations (field + FILTER).