Skip to content

Commit c282e60

Browse files
committed
fix(plugin-security): leave an operator object to the engine, and look a placeholder-shaped name up literally
Measured on 0e5f4c7: an operator object ({ $in: [...] }) already answered the engine's invalid_type, but only because the catalog read of its JSON text threw FILTER_TOKEN_UNKNOWN (a fully-wrapped {...} comparand is resolved as a filter placeholder) and the refusal failed open. The same fail-open let { a: 1 }, { $foo: 1 } and '{nope_tok}' be stored unchecked with 201. judgedName now stands down on an operator object, mirroring the engine's #5922 predicate from the same spec inputs (isPlainRecord, ALL_OPERATORS, the retired operators), so invalid_type stays the one answer. A name that classifyFilterToken reads as a placeholder is compared literally against the catalog names sharing its first character, so it is judged, never resolved or failed open; the id hint skips such names. Claude-Session: https://claude.ai/code/session_01TEah6PeJGjxJfbHaySJjLQ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0e5f4c7 commit c282e60

2 files changed

Lines changed: 128 additions & 20 deletions

File tree

‎packages/plugins/plugin-security/src/position-catalog-refusal.test.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,52 @@ describe('a non-string position is judged by its string form', () => {
416416
expect((await assignmentsOf(h, 'u_nsu'))[0]?.position).toBe('qa_auditor');
417417
});
418418

419+
it('an operator object is the engine\'s to refuse: invalid_type (#5922), never reference_not_found', async () => {
420+
const h = await boot();
421+
for (const position of [{ $in: ['x'] }, { $in: [], a: 1 }]) {
422+
const label = JSON.stringify(position);
423+
const env = envelopeOf(await refusalOf(() => h.engine.insert(
424+
'sys_user_position', { user_id: 'u_op', position }, { context: ADMIN } as any,
425+
)));
426+
expect([env.code, env.status], label).toEqual(['VALIDATION_FAILED', 400]);
427+
expect(env.fields[0], label).toMatchObject({ field: 'position', code: 'invalid_type' });
428+
}
429+
expect(await assignmentsOf(h, 'u_op')).toHaveLength(0);
430+
});
431+
432+
it('an object with no declared operator key is judged: { a: 1 } and { $foo: 1 } are refused reference_not_found', async () => {
433+
const h = await boot();
434+
for (const [position, text] of [[{ a: 1 }, '{"a":1}'], [{ $foo: 1 }, '{"$foo":1}']] as const) {
435+
const env = envelopeOf(await refusalOf(() => h.engine.insert(
436+
'sys_user_position', { user_id: 'u_plain', position }, { context: ADMIN } as any,
437+
)));
438+
expect([env.code, env.status], text).toEqual(['VALIDATION_FAILED', 400]);
439+
expect(env.fields[0], text).toMatchObject({ field: 'position', code: 'reference_not_found', value: text });
440+
expect(env.fields[0].message, text).toBe(positionNotInCatalogMessage(text));
441+
}
442+
expect(await assignmentsOf(h, 'u_plain')).toHaveLength(0);
443+
// Judged, not failed open: the catalog read never gave up on these names.
444+
expect(h.warn.mock.calls.filter((c) => String(c[0]).includes('could not be read'))).toHaveLength(0);
445+
});
446+
447+
it('a placeholder-shaped name is compared literally, never resolved as a filter token', async () => {
448+
const h = await boot();
449+
for (const text of ['{nope_tok}', '{current_user_id}']) {
450+
const env = envelopeOf(await refusalOf(() => h.engine.insert(
451+
'sys_user_position', { user_id: 'u_tok', position: text }, { context: ADMIN } as any,
452+
)));
453+
expect([env.code, env.status], text).toEqual(['VALIDATION_FAILED', 400]);
454+
expect(env.fields[0], text).toMatchObject({ field: 'position', code: 'reference_not_found', value: text });
455+
}
456+
// A catalog row that really carries such a name is found, so the predicate holds.
457+
await h.engine.insert('sys_position', { id: 'pos_lit', name: '{lit_pos}', label: 'Literal', active: true }, { context: SYS } as any);
458+
const created = await h.engine.insert(
459+
'sys_user_position', { user_id: 'u_tok_ok', position: '{lit_pos}' }, { context: ADMIN } as any,
460+
);
461+
expect(created).toMatchObject({ position: '{lit_pos}' });
462+
expect(h.warn.mock.calls.filter((c) => String(c[0]).includes('could not be read'))).toHaveLength(0);
463+
});
464+
419465
it("update by id: 123 echoed over a stored '123' is an unchanged value, not judged", async () => {
420466
const h = await boot();
421467
// A row written where it is not judged (a system write), whose text names no catalog row.

‎packages/plugins/plugin-security/src/position-catalog-refusal.ts‎

Lines changed: 82 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -80,17 +80,19 @@
8080
* refusal there would turn every authored assignment seed into a failed boot
8181
* that succeeds on the second one. It also covers invitation acceptance and
8282
* the platform's own bootstraps;
83-
* - a value the engine answers itself: `null` or a blank string (`required`),
84-
* or one whose `String()` form is longer than the column (`max_length`).
85-
* Nothing else is the engine's: its `text` validation reads `String(value)`
86-
* and refuses no number, boolean, object or array, and the write stores it
87-
* with `201` (measured over SQLite: `123`, `true`, `{}` and `['x']` all
88-
* stored). So those are judged by their string form — a scalar by
89-
* `String(value)`, an object or array by its JSON text — and refused like
90-
* any name no catalog row carries. The stored text is the driver's, not
91-
* that string form (SQLite stores `123` as `'123.0'`), so a non-string whose
92-
* string form happens to equal a catalog name is accepted and resolves
93-
* nothing: the engine's `text` leniency, outside this refusal;
83+
* - a value the engine answers itself, and only those: `null` or a blank
84+
* string (`required`); one whose `String()` form is longer than the column
85+
* (`max_length`); and an operator object, a plain object carrying a declared
86+
* filter operator as an own key such as `{ $in: [...] }` (`invalid_type`,
87+
* #5922, mirrored from the engine's own predicate). The engine's `text`
88+
* validation refuses no other number, boolean, object or array, and the
89+
* write stores it with `201` (measured over SQLite: `123`, `true`, `{}`,
90+
* `{ a: 1 }` and `['x']` all stored). So those are judged by their string
91+
* form — a scalar by `String(value)`, an object or array by its JSON text —
92+
* and refused like any name no catalog row carries. The stored text is the
93+
* driver's, not that string form (SQLite stores `123` as `'123.0'`), so a
94+
* non-string whose string form happens to equal a catalog name is accepted
95+
* and resolves nothing: the engine's `text` leniency, outside this refusal;
9496
* - an update the engine refuses on its own dispatch predicate.
9597
*
9698
* ## Where it runs
@@ -129,11 +131,19 @@
129131
* composition, or the read throws) refuses nothing — an integrity check that
130132
* cannot run must not invent a rejection, the stance the engine's own lookup
131133
* probe takes. A failed read is reported at `warn`: the write proceeds, and the
132-
* operator is told the check did not run.
134+
* operator is told the check did not run. A name the query layer would read as
135+
* a filter placeholder (`{…}`) is not a failed read: it is compared literally
136+
* (`catalogCarries`), so its verdict never falls open.
133137
*/
134138

135139
import { resolveEngineUpdateDispatch, type EngineUpdateDispatchData } from '@objectstack/metadata-core';
136140
import type { FieldErrorCode } from '@objectstack/spec/api';
141+
import {
142+
ALL_OPERATORS,
143+
RETIRED_FILTER_OPERATORS,
144+
classifyFilterToken,
145+
isPlainRecord,
146+
} from '@objectstack/spec/data';
137147
import { validationFailure } from '@objectstack/types';
138148
import { SysUserPosition } from './objects/sys-user-position.object.js';
139149

@@ -204,7 +214,8 @@ function rowsOf(data: unknown): any[] {
204214
* text (`{}`, `["x"]`) — never `String(['x'])`, which reads `'x'` and would
205215
* accept an array naming a real position that then resolves nothing. It is not
206216
* the stored text, which is the driver's (SQLite stores `123` as `'123.0'`).
207-
* `undefined` for a value that is no JSON value at all.
217+
* `undefined` only for `null`, `undefined`, a symbol, a function, and an object
218+
* that can be neither serialised nor stringified; a bigint is `String(value)`.
208219
*/
209220
function stringForm(value: unknown): string | undefined {
210221
switch (typeof value) {
@@ -233,16 +244,41 @@ function stringForm(value: unknown): string | undefined {
233244
}
234245
}
235246

247+
/**
248+
* [#5922] The filter-operator keys the engine refuses as a `text` value: the
249+
* spec's `ALL_OPERATORS` plus the retired ones, the same two inputs as
250+
* `FILTER_OPERATOR_KEYS` in `@objectstack/objectql`'s `record-validator.ts`.
251+
*/
252+
const FILTER_OPERATOR_KEYS: ReadonlySet<string> = new Set<string>([
253+
...ALL_OPERATORS,
254+
...Object.keys(RETIRED_FILTER_OPERATORS),
255+
]);
256+
257+
/**
258+
* [#5922] Is this an operator object — a `where` node pasted into the write —
259+
* which the engine itself refuses on a `text` field (`invalid_type`)? A narrow
260+
* mirror of `filterOperatorKeysIn` in `record-validator.ts`, module-private
261+
* there: the spec's `isPlainRecord` test, no `Date`, and at least one own key in
262+
* {@link FILTER_OPERATOR_KEYS}. ⛔ Not a `$`-prefix test: `{ $foo: 1 }` carries
263+
* no declared operator, the engine stores it, and it is judged here.
264+
*/
265+
function isFilterOperatorObject(value: unknown): boolean {
266+
if (!isPlainRecord(value) || value instanceof Date) return false;
267+
return Object.keys(value).some((key) => FILTER_OPERATOR_KEYS.has(key));
268+
}
269+
236270
/**
237271
* The name this refusal judges a `position` value by, or `undefined` for a
238-
* value the engine answers itself: `null` and a blank string (`required`), and
239-
* a value whose `String()` form is longer than the column (`max_length`, which
240-
* the engine reads on `String(value)` for every type). Every other value is
241-
* judged, strings or not (module note, "Which writes it judges").
272+
* value the engine answers itself: `null` and a blank string (`required`), a
273+
* value whose `String()` form is longer than the column (`max_length`, which
274+
* the engine reads on `String(value)` for every type), and an operator object
275+
* (`invalid_type`, #5922). Every other value is judged, strings or not (module
276+
* note, "Which writes it judges").
242277
*/
243278
function judgedName(value: unknown): string | undefined {
244279
if (value === null || value === undefined) return undefined;
245280
if (typeof value === 'string' && value.trim() === '') return undefined;
281+
if (isFilterOperatorObject(value)) return undefined;
246282
let engineForm: string;
247283
try {
248284
engineForm = String(value);
@@ -333,9 +369,9 @@ export async function namesWithoutCatalogRow(
333369
const readCtx = catalogReadContext(context);
334370
const missing: string[] = [];
335371
for (const name of names) {
336-
let rows: unknown;
372+
let carried: boolean;
337373
try {
338-
rows = await ql.find(POSITION_CATALOG_OBJECT, { where: { name }, limit: 1, context: readCtx });
374+
carried = await catalogCarries(ql, name, readCtx);
339375
} catch (e) {
340376
logger?.warn?.(
341377
`[security] the ${POSITION_CATALOG_OBJECT} catalog could not be read, so a ` +
@@ -345,11 +381,34 @@ export async function namesWithoutCatalogRow(
345381
);
346382
return null;
347383
}
348-
if (!Array.isArray(rows) || rows.length === 0) missing.push(name);
384+
if (!carried) missing.push(name);
349385
}
350386
return missing;
351387
}
352388

389+
/**
390+
* Does the catalog, read under `readCtx`, hold a row whose `name` is exactly
391+
* `name`? A name the query layer would read as a filter PLACEHOLDER (a
392+
* fully-wrapped `{…}`, recognised by the spec's `classifyFilterToken`, the
393+
* predicate `@objectstack/core`'s `resolveFilterTokens` applies to every `where`
394+
* comparand) cannot go in `where` as itself: an unknown token throws
395+
* `FILTER_TOKEN_UNKNOWN` and a known one is replaced by its value, so the read
396+
* would fail open or judge a different name. Such a name is compared here
397+
* instead, against the catalog names that share its first character.
398+
*/
399+
async function catalogCarries(ql: any, name: string, readCtx: Record<string, unknown>): Promise<boolean> {
400+
if (classifyFilterToken(name) === null) {
401+
const rows = await ql.find(POSITION_CATALOG_OBJECT, { where: { name }, limit: 1, context: readCtx });
402+
return Array.isArray(rows) && rows.length > 0;
403+
}
404+
const rows = await ql.find(POSITION_CATALOG_OBJECT, {
405+
where: { name: { $startsWith: name.charAt(0) } },
406+
fields: ['name'],
407+
context: readCtx,
408+
});
409+
return Array.isArray(rows) && rows.some((row: any) => row?.name === name);
410+
}
411+
353412
/**
354413
* For each missing value that is the record ID of a position the writer's
355414
* catalog holds, that position's NAME — the exact fix. Read under the same
@@ -367,6 +426,9 @@ export async function idSpellingHints(
367426
if (!ql || typeof ql.find !== 'function') return hints;
368427
const readCtx = catalogReadContext(context);
369428
for (const value of values) {
429+
// A placeholder-shaped value is never a record id, and in `where` it would
430+
// be resolved as a filter token rather than compared (see catalogCarries).
431+
if (classifyFilterToken(value) !== null) continue;
370432
try {
371433
const rows = await ql.find(POSITION_CATALOG_OBJECT, { where: { id: value }, limit: 1, context: readCtx });
372434
const row = Array.isArray(rows) ? rows[0] : null;

0 commit comments

Comments
 (0)