diff --git a/.changeset/20932-predicate-guard-comparand.md b/.changeset/20932-predicate-guard-comparand.md new file mode 100644 index 00000000000..af0178f3775 --- /dev/null +++ b/.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) + + + +**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. diff --git a/packages/plugins/plugin-security/src/predicate-guard.test.ts b/packages/plugins/plugin-security/src/predicate-guard.test.ts index bfa211b8f12..b88fbefd3f7 100644 --- a/packages/plugins/plugin-security/src/predicate-guard.test.ts +++ b/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 = {}) => ({ $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): { 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]> = [ + ...['$eq', '$ne', '$gt', '$gte', '$lt', '$lte'].map( + (op): [string, Record] => [`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; + expect(refusalOf(readable)).toBeUndefined(); + } + }); +}); diff --git a/packages/plugins/plugin-security/src/predicate-guard.ts b/packages/plugins/plugin-security/src/predicate-guard.ts index 797bddef5cb..21bb89938fd 100644 --- a/packages/plugins/plugin-security/src/predicate-guard.ts +++ b/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 = new Set()): Set { if (!condition || typeof condition !== 'object' || Array.isArray(condition)) return out; @@ -46,10 +53,40 @@ export function collectConditionFields(condition: unknown, out: Set = 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): 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)) collectComparandFields(member, out); +} + /** * Collect every field referenced by the query's row-shaping clauses: * where / orderBy / groupBy / having / aggregations (field + FILTER). diff --git a/packages/rest/src/data-field-comparand-permission.test.ts b/packages/rest/src/data-field-comparand-permission.test.ts new file mode 100644 index 00000000000..025bd82aca2 --- /dev/null +++ b/packages/rest/src/data-field-comparand-permission.test.ts @@ -0,0 +1,229 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A field the caller may not read is not a comparison target either — with + * the REAL security layer (`SecurityPlugin` on a real `ObjectQL` over a real + * `SqlDriver`), through `POST /api/v1/data/:object/query`, on the data read + * and on the aggregate path it routes to. + * + * The security layer refuses a query that filters by a hidden field + * (`assertReadableQueryFields`, `403 PERMISSION_DENIED`): row presence would + * disclose the value the field mask withholds. A cross-field comparand + * (`FieldReferenceSchema`) reads the field it names just as a condition key + * does, so it is collected by the same walk and answers the SAME refusal — + * status and body — as the same hidden field written as a key. Every position + * the filter grammar admits for a comparand is held to it: the whole + * comparand of the six scalar comparisons, the whole-day offset wrapper (its + * base and its offset column), under `$and` / `$or` / `$not`, and — on the + * aggregate path — in `where` and in a per-aggregation `filter`. + * + * A readable comparand in the same positions is the control: served. + * + * SQLite only: the check is the security layer's, before any driver dialect + * is involved. + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SecurityPlugin } from '@objectstack/plugin-security'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from './rest-server'; + +const OBJECT = 'rest_comparand_perm_ledger'; + +const SYS_CTX = { isSystem: true, userId: 'usr_system' }; + +const MEMBER_SET = PermissionSetSchema.parse({ + name: 'member_default', + label: 'Member', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + // The fields the caller may not read. + fields: { + [`${OBJECT}.sealed_n`]: { readable: false, editable: false }, + [`${OBJECT}.sealed_day`]: { readable: false, editable: false }, + [`${OBJECT}.sealed_k`]: { readable: false, editable: false }, + }, +}); + +const MEMBER_CTX = { userId: 'usr_member', positions: [], permissions: [MEMBER_SET.name], posture: 'MEMBER' }; + +const ROWS = [ + { id: 'c1', label: 'a', seen_n: 5, seen_m: 1, sealed_n: 1, seen_day: '2026-01-10', base_day: '2026-01-09', sealed_day: '2026-01-01', seen_k: 1, sealed_k: 3 }, + { id: 'c2', label: 'b', seen_n: 5, seen_m: 9, sealed_n: 9, seen_day: '2026-01-10', base_day: '2026-01-20', sealed_day: '2026-02-01', seen_k: 1, sealed_k: 1 }, +]; + +function createMockServer() { + const noop = () => {}; + return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; +} + +function makeRes() { + const res: any = { + write: () => true, end: () => {}, + header: () => res, + status: (code: number) => { res._status = code; return res; }, + json: (body: any) => { res._json = body; return res; }, + }; + return res; +} + +const ref = (field: string, extra: Record = {}) => ({ $field: field, ...extra }); +const GROUPED = { groupBy: ['label'], aggregations: [{ function: 'count', alias: 'n' }] }; +const counted = (filter: Record) => ({ + groupBy: ['label'], + aggregations: [{ function: 'count', alias: 'n', filter }], +}); + +/** Every comparand position the grammar admits, on the data read, with the hidden field it names. */ +const READ_POSITIONS: Array<[string, Record]> = [ + ...['$eq', '$ne', '$gt', '$gte', '$lt', '$lte'].map( + (op): [string, Record] => [`the whole comparand of ${op}`, { where: { seen_n: { [op]: ref('sealed_n') } } }], + ), + ['a comparand under $and', { where: { $and: [{ label: { $ne: 'z' } }, { seen_n: { $gt: ref('sealed_n') } }] } }], + ['a comparand under $or', { where: { $or: [{ label: 'z' }, { seen_n: { $gt: ref('sealed_n') } }] } }], + ['a comparand under $not', { where: { $not: { seen_n: { $gt: ref('sealed_n') } } } }], + ['a comparand under nested groups', { where: { $and: [{ $or: [{ $not: { seen_n: { $lte: ref('sealed_n') } } }] }] } }], +]; + +/** The whole-day offset wrapper, whose base and offset column name a field each. */ +const OFFSET_POSITIONS: Array<[string, Record]> = [ + ['the base of a whole-day offset comparand', { where: { seen_day: { $lte: ref('sealed_day', { addDays: 3 }) } } }], + ['the offset column of a whole-day offset comparand', { where: { seen_day: { $lte: ref('base_day', { addDays: ref('sealed_k') }) } } }], +]; + +/** The aggregate path: a comparand in `where`, and in a per-aggregation `filter`. */ +const AGGREGATE_POSITIONS: Array<[string, Record]> = [ + ['a comparand in where', { ...GROUPED, where: { seen_n: { $gt: ref('sealed_n') } } }], + ['a comparand under $or in where', { ...GROUPED, where: { $or: [{ label: 'z' }, { seen_n: { $lt: ref('sealed_n') } }] } }], + ['a comparand in a per-aggregation filter', counted({ seen_n: { $gt: ref('sealed_n') } })], +]; + +/** The same body, with each hidden field swapped for a readable one of the same type. */ +function readableTwin(body: Record): Record { + return JSON.parse( + JSON.stringify(body).replaceAll('sealed_n', 'seen_m').replaceAll('sealed_day', 'base_day').replaceAll('sealed_k', 'seen_k'), + ) as Record; +} + +describe('a hidden field as a cross-field comparand answers the key form\'s refusal — the real security layer', () => { + let engine: ObjectQL; + let query: (body: Record) => Promise<{ status: number; body: any }>; + + beforeAll(async () => { + engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as any), + true, + ); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.qa.field-comparand-permission', + name: 'Field comparand permission', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [ + { + name: OBJECT, + label: 'Ledger', + sharingModel: 'public_read_write', + fields: { + label: { name: 'label', type: 'text' }, + seen_n: { name: 'seen_n', type: 'number' }, + seen_m: { name: 'seen_m', type: 'number' }, + sealed_n: { name: 'sealed_n', type: 'number' }, + seen_day: { name: 'seen_day', type: 'date' }, + base_day: { name: 'base_day', type: 'date' }, + sealed_day: { name: 'sealed_day', type: 'date' }, + seen_k: { name: 'seen_k', type: 'number' }, + sealed_k: { name: 'sealed_k', type: 'number' }, + }, + }, + ], + } as never); + await engine.syncSchemas(); + + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (_type: string, name: string) => engine.getSchema(name) ?? null, + list: async () => [MEMBER_SET], + }, + }; + const ctx = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: vi.fn(), + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx as never); + await plugin.start(ctx as never); + vi.spyOn((engine as unknown as { logger: { warn: () => void } }).logger, 'warn').mockImplementation(() => undefined); + + await engine.insert(OBJECT, ROWS.map((r) => ({ ...r })), { context: SYS_CTX } as never); + + const protocol = new ObjectStackProtocolImplementation(engine as any); + const rest = new RestServer(createMockServer() as any, protocol as any, { api: { requireAuth: false } } as any); + (rest as any).resolveExecCtx = async () => MEMBER_CTX; + rest.registerRoutes(); + const route = rest.getRoutes().find((r: any) => r.method === 'POST' && r.path === '/api/v1/data/:object/query'); + expect(route).toBeDefined(); + query = async (body) => { + const res = makeRes(); + await route!.handler({ params: { object: OBJECT }, body: JSON.parse(JSON.stringify(body)), query: {}, headers: {} } as any, res); + return { status: res._status ?? 200, body: res._json }; + }; + }); + + afterAll(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + /** The hidden field written as a KEY: the reference refusal, per path and per field. */ + async function keyForm(field: string, shape: 'read' | 'aggregate' | 'aggregate-filter'): Promise<{ status: number; body: any }> { + const condition = { [field]: { $ne: null } }; + const body = shape === 'read' + ? { where: condition } + : shape === 'aggregate' ? { ...GROUPED, where: condition } : counted(condition); + const res = await query(body); + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(JSON.stringify(res.body)).toContain('PERMISSION_DENIED'); + return res; + } + + describe('the data read', () => { + it.each([...READ_POSITIONS, ...OFFSET_POSITIONS])('a hidden field as %s is refused exactly as the key form is', async (_position, body) => { + const hidden = JSON.stringify(body).includes('sealed_day') ? 'sealed_day' + : JSON.stringify(body).includes('sealed_k') ? 'sealed_k' : 'sealed_n'; + const reference = await keyForm(hidden, 'read'); + const res = await query(body); + expect(res.status, JSON.stringify(res.body)).toBe(reference.status); + expect(res.body).toEqual(reference.body); + }); + + it.each([...READ_POSITIONS, ...OFFSET_POSITIONS])('CONTROL a readable field as %s is served', async (_position, body) => { + const res = await query(readableTwin(body)); + expect(res.status, JSON.stringify(res.body)).toBe(200); + }); + }); + + describe('the aggregate path', () => { + it.each(AGGREGATE_POSITIONS)('a hidden field as %s is refused exactly as the key form is', async (position, body) => { + const reference = await keyForm('sealed_n', position.includes('per-aggregation') ? 'aggregate-filter' : 'aggregate'); + const res = await query(body); + expect(res.status, JSON.stringify(res.body)).toBe(reference.status); + expect(res.body).toEqual(reference.body); + }); + + it.each(AGGREGATE_POSITIONS)('CONTROL a readable field as %s is served', async (_position, body) => { + const res = await query(readableTwin(body)); + expect(res.status, JSON.stringify(res.body)).toBe(200); + }); + }); +});