Skip to content

Commit 1d6866b

Browse files
committed
fix(objectql): a related row outside every organization wall is answered by the caller's own read
A validation rule reading one hop through a reference field reads the related row under system authority, scoped by the caller's organization. A related object with no tenant column (sys_user), `tenancy.enabled: false` or `external` is not scoped by any organization, so a rule could be evaluated on a user the caller cannot read, and its pass/fail revealed that user's field. For a user caller, such a related object's rows are now the ones the caller's OWN read returns, through every enforcement layer (CRUD, row-level security, sharing). Any other stored id binds as 'unreadable' and the rule refuses the write with the not-readable prescription; it is never evaluated on that row and its columns are never read. Tenant-scoped related objects and system callers are unchanged. Pinned on the real SecurityPlugin + SqlDriver with the shipped member_default sys_user wall, on insert, validate() and update. Claude-Session: https://claude.ai/code/session_019c3Hi6ZMU1p6m6aA6Bz45d Co-authored-by: Claude <noreply@anthropic.com>
1 parent 53f960f commit 1d6866b

3 files changed

Lines changed: 107 additions & 13 deletions

File tree

‎packages/objectql/src/engine.ts‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7274,12 +7274,13 @@ export class ObjectQL implements IObjectQLEngine {
72747274
*
72757275
* What bounds the elevation is the PROJECTION: only the
72767276
* columns the predicate names, intersected with the related object's declared
7277-
* fields. ⛔ Never the whole row.
7277+
* fields. ⛔ Never the whole row. On a related object no organization wall
7278+
* scopes, a user caller's own read of it also bounds the ROWS.
72787279
*
72797280
* ## An unresolved row is left UNAVAILABLE, and it says which kind
72807281
*
72817282
* No reference stored, the related record not found, the related object
7282-
* declares no such column, or the read failed: each is its own reason, and
7283+
* declares no such column, or the row could not be read: each is its own reason, and
72837284
* {@link checkPredicate} turns it into a refusal naming the related object and
72847285
* column. The write is REJECTED rather than judged on a rule that produced no
72857286
* verdict. ⛔ Never silently true, and never silently false.
@@ -7322,6 +7323,8 @@ export class ObjectQL implements IObjectQLEngine {
73227323
undeclared?: string[];
73237324
/** Set when the read itself failed, whatever the id. */
73247325
blocked?: boolean;
7326+
/** The ids the caller's OWN read returned, when that read decides (below). */
7327+
ownRead?: ReadonlySet<string>;
73257328
};
73267329
const resolved = new Map<string, Resolved>();
73277330

@@ -7336,7 +7339,7 @@ export class ObjectQL implements IObjectQLEngine {
73367339
// object does not declare must not put that name into a system-authority
73377340
// query: the read is elevated. An undeclared name is also a real
73387341
// authoring fault and is reported as one rather than silently dropped.
7339-
const targetSchema = this._registry.getObject(target) as { fields?: Record<string, unknown> } | undefined;
7342+
const targetSchema = this._registry.getObject(target) as { fields?: Record<string, unknown>; external?: unknown } | undefined;
73407343
const declared = targetSchema?.fields;
73417344
// [#8215] The PRIMARY KEY is declared by the platform, not by the author,
73427345
// so it is absent from every object's field map — the map carries the
@@ -7387,8 +7390,21 @@ export class ObjectQL implements IObjectQLEngine {
73877390
// The value itself never appears — not in the row handed to CEL beyond
73887391
// the predicate's own use of it, and not in the refusal text, which
73897392
// names the field and the rule and never the value.
7393+
//
7394+
// ⛔ No organization wall scopes a related object with no tenant column
7395+
// (e.g. `sys_user`), `tenancy.enabled: false` or `external`, so for a
7396+
// USER caller its rows are the ones the caller's OWN read returns, through
7397+
// every enforcement layer; any other id is 'unreadable', stored or not.
7398+
let ownRead: Set<string> | undefined;
7399+
if (caller?.userId && (targetSchema?.external != null || resolveTenantFieldName(targetSchema) === null)) {
7400+
const own = await this.find(target, {
7401+
where: { id: { $in: [...ids] } }, fields: ['id'], context: caller as EngineQueryOptions['context'],
7402+
}) as Array<Record<string, unknown>>;
7403+
ownRead = new Set((Array.isArray(own) ? own : []).flatMap((r) => (r?.id == null ? [] : [String(r.id)])));
7404+
if (ownRead.size === 0) { resolved.set(fk, { object: target, byId: new Map(), ownRead }); continue; }
7405+
}
73907406
const query: EngineQueryOptions = {
7391-
where: { id: { $in: [...ids] } },
7407+
where: { id: { $in: [...(ownRead ?? ids)] } },
73927408
fields: [...new Set(['id', ...named])],
73937409
context: { ...(context as Record<string, unknown> ?? {}), isSystem: true } as EngineQueryOptions['context'],
73947410
};
@@ -7406,7 +7422,7 @@ export class ObjectQL implements IObjectQLEngine {
74067422
for (const name of named) if (!(name in copy)) copy[name] = null;
74077423
byId.set(String(row.id), copy);
74087424
}
7409-
resolved.set(fk, { object: target, byId });
7425+
resolved.set(fk, { object: target, byId, ownRead });
74107426
} catch (err) {
74117427
this.logger?.warn?.('predicate relationship read failed — the rule will reject the write', {
74127428
object: target, field: fk, error: err,
@@ -7433,7 +7449,7 @@ export class ObjectQL implements IObjectQLEngine {
74337449
const found = entry.byId.get(String(value));
74347450
binding[fk] = found
74357451
? { object: entry.object, row: found }
7436-
: { object: entry.object, unavailable: 'unresolved' };
7452+
: { object: entry.object, unavailable: entry.ownRead ? 'unreadable' : 'unresolved' };
74377453
}
74387454
return binding;
74397455
};

‎packages/objectql/src/validation/rule-validator.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,7 @@ export type ParentBinding = Record<string, unknown> | null | undefined;
543543
export type RelatedUnavailableReason =
544544
/** The record stores no reference — the FK is null/empty, so there is no row. */
545545
| 'no-reference'
546-
/** The related read failed outright. */
546+
/** The related read failed, or — for a related object no organization wall scopes — the caller's own read does not return the row. */
547547
| 'unreadable'
548548
/**
549549
* The predicate names a column the RELATED object does not declare. A real
@@ -3343,7 +3343,7 @@ function traversalRefusal(
33433343
return {
33443344
summary: `could not read '${binding.object}'`,
33453345
detail:
3346-
` The rule reads ${columns} through ${on}, and that read failed. The rule has no`
3346+
` The rule reads ${columns} through ${on}, and that row could not be read. The rule has no`
33473347
+ ' verdict, so the write is rejected rather than allowed on an unchecked rule.',
33483348
};
33493349
}

‎packages/plugins/plugin-security/src/predicate-related-read-tenant-scope.test.ts‎

Lines changed: 83 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,9 @@ import { describe, it, expect, afterEach, vi } from 'vitest';
2121
import { ObjectQL } from '@objectstack/objectql';
2222
import { SqlDriver } from '@objectstack/driver-sql';
2323
import type { PermissionSet } from '@objectstack/spec/security';
24+
import { SysUser } from '@objectstack/platform-objects/identity';
2425
import { SecurityPlugin } from './security-plugin.js';
26+
import { defaultPermissionSets } from './objects/default-permission-sets.js';
2527

2628
const RULE_MESSAGE = 'Inspections on a secret line are frozen.';
2729

@@ -65,16 +67,38 @@ const OBJECTS = [
6567
message: RULE_MESSAGE, condition: "record.line.kind == 'secret'",
6668
}],
6769
},
70+
SysUser,
71+
{
72+
// A rule reading through a `user` field into `sys_user`, which has no tenant column.
73+
name: 'qa_review',
74+
label: 'Review',
75+
sharingModel: 'public_read_write',
76+
fields: {
77+
id: { name: 'id', type: 'text', primaryKey: true },
78+
name: { name: 'name', type: 'text' },
79+
reviewer: { name: 'reviewer', type: 'user' },
80+
},
81+
validations: [{
82+
name: 'no_banned_reviewer', type: 'script', severity: 'error',
83+
message: RULE_MESSAGE, condition: 'record.reviewer.banned == true',
84+
}],
85+
},
6886
];
6987

88+
/** The shipped `sys_user` wall: `member_default`'s grant and row-level security. */
89+
const SHIPPED_MEMBER = defaultPermissionSets.find((s) => s.name === 'member_default')!;
90+
7091
const MEMBER: PermissionSet = {
7192
name: 'member_default',
7293
label: 'Member',
7394
objects: {
7495
qa_inspection: { allowRead: true, allowCreate: true, allowEdit: true },
7596
qa_note: { allowRead: true, allowCreate: true, allowEdit: true },
7697
qa_line: { allowRead: true },
98+
qa_review: { allowRead: true, allowCreate: true, allowEdit: true },
99+
sys_user: SHIPPED_MEMBER.objects.sys_user,
77100
},
101+
rowLevelSecurity: SHIPPED_MEMBER.rowLevelSecurity?.filter((p) => p.object === 'sys_user'),
78102
} as unknown as PermissionSet;
79103

80104
/** The driver's own query builder, reached past its `protected` modifier. */
@@ -136,18 +160,26 @@ async function boot(kind: 'secret' | 'public', posture?: 'group' | 'isolated') {
136160
{ id: 'line_x', kind: 'secret', organization_id: 'org_x' },
137161
{ id: 'line_y', kind, organization_id: 'org_y' },
138162
]);
163+
// The caller u_x and its peer u_x2 (org X); u_y belongs to org Y only.
164+
await table('sys_user').insert([
165+
{ id: 'u_x', name: 'X', email: 'x@x.test', banned: false },
166+
{ id: 'u_x2', name: 'X2', email: 'x2@x.test', banned: true },
167+
{ id: 'u_y', name: 'Y', email: 'y@y.test', banned: kind === 'secret' },
168+
]);
139169

140170
// What the driver hands back for every read of the related object.
141171
const readsOfLine: unknown[][] = [];
172+
const userColumnsRead: unknown[] = [];
142173
const find = driver.find.bind(driver);
143174
vi.spyOn(driver, 'find').mockImplementation(async (object, ast, options) => {
144175
const rows = await find(object, ast, options);
145176
if (object === 'qa_line') readsOfLine.push(rows as unknown[]);
177+
if (object === 'sys_user') userColumnsRead.push(...((ast as { fields?: unknown[] }).fields ?? ['*']));
146178
return rows;
147179
});
148180

149181
const stored = async (object: string, name: string) => (await table(object).where({ name }).select('id')).length;
150-
return { engine, readsOfLine, stored, table };
182+
return { engine, readsOfLine, userColumnsRead, stored, table };
151183
}
152184

153185
/** Everything the caller sees of one write and one preview naming `line`. */
@@ -156,18 +188,19 @@ async function observe(
156188
posture?: 'group' | 'isolated', object = 'qa_inspection',
157189
) {
158190
const h = await boot(kind, posture);
191+
const ref = object === 'qa_review' ? 'reviewer' : 'line';
159192
const refusal = await h.engine
160-
.insert(object, { name: 'probe', line }, { context: caller } as never)
193+
.insert(object, { name: 'probe', [ref]: line }, { context: caller } as never)
161194
.then(() => null, (e: { code?: string; message?: string }) => ({ code: e.code, message: e.message }));
162195
const preview = await h.engine.validate(
163-
object, { name: 'probe', line }, { mode: 'insert', context: caller } as never,
196+
object, { name: 'probe', [ref]: line }, { mode: 'insert', context: caller } as never,
164197
);
165198
// Given a posture, the by-id UPDATE door too: a seeded row the caller may edit, repointed at `line`.
166199
let update: unknown;
167200
if (posture) {
168-
await h.table(object).insert([{ id: 'row_x', name: 'seed', ...(object === 'qa_inspection' ? { organization_id: 'org_x' } : {}) }]);
201+
await h.table(object).insert([{ id: 'row_x', name: 'seed', ...(object !== 'qa_note' ? { organization_id: 'org_x' } : {}) }]);
169202
update = await h.engine
170-
.update(object, { line }, { where: { id: 'row_x' }, context: caller } as never)
203+
.update(object, { [ref]: line }, { where: { id: 'row_x' }, context: caller } as never)
171204
.then(() => 'committed', (e: { code?: string; message?: string }) => ({ code: e.code, message: e.message }));
172205
}
173206
return {
@@ -178,6 +211,7 @@ async function observe(
178211
update,
179212
},
180213
readsOfLine: h.readsOfLine,
214+
userColumnsRead: h.userColumnsRead,
181215
};
182216
}
183217

@@ -265,3 +299,47 @@ describe('#18682 — under `isolated`, an org-less writer of an unwalled object
265299
expect(own.readsOfLine.map((rows) => rows.length)).toEqual([1, 1, 1]);
266300
});
267301
});
302+
303+
/**
304+
* `sys_user` has no tenant column, so no organization wall scopes the related
305+
* read; its own wall is the shipped `member_default` row-level security. A user
306+
* the caller's own read of `sys_user` does not return is NOT READABLE: the rule
307+
* faults loudly, is never evaluated on that user, and never reads its columns.
308+
*/
309+
describe('#18682 — a related user the caller cannot read makes the rule fault loudly', () => {
310+
const PEERS = { ...CALLER, org_user_ids: ['u_x', 'u_x2'] };
311+
const NOT_READABLE = "could not read 'sys_user'";
312+
313+
it('CONTROL: the caller’s own read of sys_user returns org X’s users, never org Y’s', async () => {
314+
const h = await boot('secret', 'isolated');
315+
const own = await h.engine.find('sys_user', { where: { id: { $in: ['u_x2', 'u_y'] } }, context: PEERS } as never);
316+
317+
expect((own as Array<{ id: string }>).map((row) => row.id)).toEqual(['u_x2']);
318+
});
319+
320+
it('a user only org Y holds: every door refuses identically, whatever that user’s value', async () => {
321+
const banned = await observe('secret', 'u_y', PEERS, 'isolated', 'qa_review');
322+
const clear = await observe('public', 'u_y', PEERS, 'isolated', 'qa_review');
323+
324+
expect(clear.seen).toEqual(banned.seen);
325+
for (const door of [banned.seen.refusal, banned.seen.update]) {
326+
expect(door).toMatchObject({ code: 'VALIDATION_FAILED', message: expect.stringContaining(NOT_READABLE) });
327+
}
328+
expect(banned.seen.committed).toBe(0);
329+
expect(banned.seen.preview).toEqual({ valid: false, errors: [expect.stringContaining(NOT_READABLE)] });
330+
expect(banned.userColumnsRead).not.toContain('banned');
331+
});
332+
333+
it('CONTROL: a user in org X — the rule evaluates on every door, in both directions', async () => {
334+
const peer = await observe('public', 'u_x2', PEERS, 'isolated', 'qa_review');
335+
const self = await observe('public', 'u_x', PEERS, 'isolated', 'qa_review');
336+
337+
expect(peer.seen.refusal).toEqual({ code: 'VALIDATION_FAILED', message: RULE_MESSAGE });
338+
expect(peer.seen.preview).toEqual({ valid: false, errors: [RULE_MESSAGE] });
339+
expect(peer.seen.update).toEqual({ code: 'VALIDATION_FAILED', message: RULE_MESSAGE });
340+
expect(peer.userColumnsRead).toContain('banned');
341+
expect(self.seen).toEqual({
342+
refusal: null, committed: 1, preview: { valid: true, errors: [] }, update: 'committed',
343+
});
344+
});
345+
});

0 commit comments

Comments
 (0)