From 3bb510a6ea9f3e43b845033bebc393694eb59d14 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 04:36:58 +0000 Subject: [PATCH 1/2] fix(plugin-approvals): the generic data door serves sys_approval_action and sys_approval_approver rows by their request's visibility A read of either child table through the generic data door (find, findOne, count, aggregate) now keeps a row only when its request_id names a request the approvals door would serve the same caller: the same visibility source the sys_approval_request gate consumes (requestVisibilitySourceOf), keyed by request_id. A read pinned to one request asks the targeted shape; a row read by id resolves its request through findChildRequestId, which authorizeFileRead now shares. Fails closed; administrator and system reads are unnarrowed. The record-reader tier never anchors on a child-table row, as it never anchors on a request. Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude --- .../plugin-approvals/src/approval-service.ts | 54 ++- .../plugin-approvals/src/approvals-plugin.ts | 15 +- ...equest-child-read-gate.integration.test.ts | 414 ++++++++++++++++++ .../plugin-approvals/src/request-read-gate.ts | 120 ++++- 4 files changed, 573 insertions(+), 30 deletions(-) create mode 100644 packages/plugins/plugin-approvals/src/request-child-read-gate.integration.test.ts diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index f315b629921..23d8a48b649 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -85,7 +85,7 @@ import { resolveReadableSnapshotFields, type FieldVisibilitySource, } from './payload-redaction.js'; -import type { RequestVisibilitySource } from './request-read-gate.js'; +import { APPROVAL_REQUEST_CHILD_OBJECTS, type RequestVisibilitySource } from './request-read-gate.js'; /** * Node-era approval runtime (ADR-0019). @@ -1298,6 +1298,26 @@ async function findRequestRow( return Array.isArray(rows) && rows[0] ? rows[0] : null; } +/** + * [#22589] The request a row of one of its child tables (`object`, one of + * `APPROVAL_REQUEST_CHILD_OBJECTS`) belongs to, read as the system — the one + * read the approvals door's by-row rule (`ApprovalService.authorizeFileRead`) + * and a by-id visibility question about a child row (the read gate's source + * below) both take, so the two cannot disagree about which request a row is + * anchored on. `null` when the row does not exist or names no request. + */ +async function findChildRequestId( + engine: ApprovalEngine, + object: string, + rowId: string, +): Promise { + const rows = await engine.find(object, { + where: { id: rowId }, fields: ['request_id'], limit: 1, context: SYSTEM_CTX, + }); + const requestId = (Array.isArray(rows) ? rows[0] : undefined)?.request_id; + return requestId != null && String(requestId) !== '' ? String(requestId) : null; +} + /** * [#22559] Each service's own request visibility, as the generic data door's * read gate on `sys_approval_request` asks it (`request-read-gate.ts`). @@ -1382,6 +1402,12 @@ export class ApprovalService implements IApprovalService { // is a request loaded by id, which carries its own anchor (`loadRequest`). // Absent, the read is untargeted — the participant set alone, as for the // inbox. + // + // [#22589] The same answer serves the gates on a request's child tables + // (`sys_approval_action`, `sys_approval_approver`), which keep a row when + // its `request_id` is in it. There `target.row` names a child row loaded + // by id: the request it belongs to is the anchor, as `authorizeFileRead` + // reads it — `findChildRequestId`, shared with that method. REQUEST_VISIBILITY.set(this, { visibleRequestIdsFor: async (context, target) => { // `organizationId` is not on the envelope — see isOverrideActor(). @@ -1392,9 +1418,13 @@ export class ApprovalService implements IApprovalService { let anchor: { object?: string | null; recordId?: string | null } = { object: target?.object, recordId: target?.recordId, }; - if (!(anchor.object && anchor.recordId) && target?.requestId) { - const raw = await findRequestRow(this.engine, target.requestId, tenantOrg); - anchor = raw ? { object: raw.object_name, recordId: raw.record_id } : {}; + if (!(anchor.object && anchor.recordId)) { + const requestId = target?.requestId + || (target?.row ? await findChildRequestId(this.engine, target.row.object, target.row.id) : null); + if (requestId) { + const raw = await findRequestRow(this.engine, requestId, tenantOrg); + anchor = raw ? { object: raw.object_name, recordId: raw.record_id } : {}; + } } return this.visibleRequestIds(context, tenantOrg, anchor); }, @@ -6859,7 +6889,10 @@ export class ApprovalService implements IApprovalService { // object it passes through the generic door's read gate, which asks this // very function about the row it names — a chain of requests about // requests, unbounded on a cycle. Fail closed: no tier on it. - if (object === 'sys_approval_request') return; + // [#22589] Nor is a row of one of its child tables, for the same reason: + // their gates ask this function about the request a row belongs to, whose + // anchor can be that very row. + if (object === 'sys_approval_request' || APPROVAL_REQUEST_CHILD_OBJECTS.includes(object)) return; // A tokenless/anonymous caller reads nothing. Fail closed, as above. const uid = context?.userId != null ? String(context.userId) : ''; if (!uid) return; @@ -7271,15 +7304,12 @@ export class ApprovalService implements IApprovalService { async authorizeFileRead(actionId: string, context: ExecutionContext): Promise { if (!actionId) return false; try { - const rows = await this.engine.find('sys_approval_action', { - where: { id: actionId }, - limit: 1, - context: SYSTEM_CTX, - }); - const requestId = (Array.isArray(rows) ? rows[0] : undefined)?.request_id; + // [#22589] The row's request, read the way the generic door's gate on + // this table reads it for a by-id read — `findChildRequestId`, shared. + const requestId = await findChildRequestId(this.engine, 'sys_approval_action', actionId); if (!requestId) return false; // Same gate as listActions: visibility of the decision's parent request. - return !!(await this.getRequest(String(requestId), context)); + return !!(await this.getRequest(requestId, context)); } catch { return false; } diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index 01ac2bbfe95..51fd00eb78b 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -30,7 +30,7 @@ import { } from './lifecycle-hooks.js'; import { bindSnapshotRedactionMiddleware } from './payload-redaction-middleware.js'; import { bindSnapshotPredicateGuard } from './payload-predicate-guard.js'; -import { bindRequestReadGate } from './request-read-gate.js'; +import { bindRequestChildReadGates, bindRequestReadGate } from './request-read-gate.js'; import type { FieldVisibilitySource } from './payload-redaction.js'; import { registerApprovalNode, type ApprovalAutomationSurface } from './approval-node.js'; import { backfillActionSlots } from './action-slot-backfill.js'; @@ -317,13 +317,18 @@ export class ApprovalsServicePlugin implements Plugin { // `disableAutoHooks`: that switch is the record lock's (a caller driving // the manual API), and a read through the generic door needs the gate // whichever API wrote the row. + // [#22589] …and its child tables, `sys_approval_action` (the decision log) + // and `sys_approval_approver` (the approver index): a row there is served + // only when its request is, by the same source. if (typeof (engine as any).registerMiddleware === 'function') { - bindRequestReadGate(engine as any, requestVisibilitySourceOf(this.service), ctx.logger); + const visibility = requestVisibilitySourceOf(this.service); + bindRequestReadGate(engine as any, visibility, ctx.logger); + bindRequestChildReadGates(engine as any, visibility, ctx.logger); } else { ctx.logger.warn( - 'ApprovalsServicePlugin: the ObjectQL engine has no middleware seam — reads of sys_approval_request ' - + 'through the generic data door are NOT narrowed to the approval requests the caller may see. ' - + 'Grant no read on that object to non-administrators on this stack.', + 'ApprovalsServicePlugin: the ObjectQL engine has no middleware seam — reads of sys_approval_request, ' + + 'sys_approval_action and sys_approval_approver through the generic data door are NOT narrowed to the ' + + 'approval requests the caller may see. Grant no read on those objects to non-administrators on this stack.', ); } diff --git a/packages/plugins/plugin-approvals/src/request-child-read-gate.integration.test.ts b/packages/plugins/plugin-approvals/src/request-child-read-gate.integration.test.ts new file mode 100644 index 00000000000..0cea7c52325 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/request-child-read-gate.integration.test.ts @@ -0,0 +1,414 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#22589] The two child tables of an approval request — `sys_approval_action` + * (the decision log: actor, decision, comment text) and `sys_approval_approver` + * (the pending-approver index) — on the GENERIC data door serve a caller only + * the rows of requests the approvals door serves it. The sibling half of + * #22559, which gave `sys_approval_request` itself the same rule. + * + * ## What the rulings fix, and what each pin below holds + * + * #8652 ruled the approvals door's visibility: a request's participants and + * administrators see it and its full action history; a reader of the record it + * is about sees both READ-ONLY, only on an object a deployment opted in, + * default OFF. On the approvals door the action history follows the request's + * visibility (`listActions` serves it through `getRequest`), and so does a + * decision attachment (`authorizeFileRead`). The triage of the parent ruled one + * definition for both doors, fail closed; this card keys the child rows by + * their `request_id` to that same definition: + * + * - a non-participant sees no row of either table: not in a list, not in a + * request's list, not in a count or a grouped count, and by id a hidden row + * answers exactly as a missing one does; + * - a participant sees the rows of their own requests and no one else's; + * - the record page's Timeline (a related list of `sys_approval_action` keyed + * on `request_id`) keeps its readers — the submitter, the current approver + * and an administrator — and for every caller its rows equal the approvals + * door's `listActions`; + * - by id, an action row is served exactly when the approvals door's own + * by-row rule (`authorizeFileRead`) admits it; + * - with the tier on for the object, a reader of the record sees its rows on a + * read that names the request or the row, read-only, and a read that names + * nothing stays the participant set; + * - control: an administrator, and the approval engine's own reads as the + * system, are unchanged. + * + * ## The rig + * + * The parent's (`request-read-gate.integration.test.ts`): a real ObjectQL + * engine over better-sqlite3, the real `ApprovalsServicePlugin.start()`, the + * real data-door normalizer (`ObjectStackProtocolImplementation`). No security + * plugin is mounted, so every caller holds read on both tables — the app's + * grant this card measures under. RECORD READABILITY is the one stand-in: one + * middleware on the business object refuses a caller with no read on it and + * scopes one with read to its own records. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import type { EngineAggregateOptions, EngineCountOptions, EngineQueryOptions } from '@objectstack/spec/data'; +import { ApprovalService, requestVisibilitySourceOf } from './approval-service.js'; +import { ApprovalsServicePlugin, type ApprovalsPluginOptions } from './approvals-plugin.js'; +import { SysApprovalRequest } from './sys-approval-request.object.js'; +import { SysApprovalAction } from './sys-approval-action.object.js'; +import { SysApprovalApprover } from './sys-approval-approver.object.js'; +import { SysApprovalDelegation } from './sys-approval-delegation.object.js'; + +const ACTION = 'sys_approval_action'; +const APPROVER_INDEX = 'sys_approval_approver'; +const CHILDREN = [ACTION, APPROVER_INDEX] as const; +const OBJECT = 'exam_sheet'; +const READ_OPS = new Set(['find', 'findOne', 'count', 'aggregate']); + +const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; +const asUser = (userId: string) => ({ userId, positions: [], permissions: [] }) as any; +const ADMIN = { userId: 'u_admin', positions: [], permissions: [], posture: 'PLATFORM_ADMIN' } as any; + +const SUBMITTER_A = 'u_submitter_a'; +const SUBMITTER_B = 'u_submitter_b'; +const APPROVER = 'u_approver'; +/** Reads no record of the business object and participates in nothing. */ +const OUTSIDER = 'u_outsider'; +/** Reads record A and participates in nothing. */ +const READER = 'u_reader'; + +const examSheet = { + name: OBJECT, + label: 'Exam Sheet', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + }, +}; + +const nodeConfig = { + approvers: [{ type: 'user' as const, value: APPROVER }], + behavior: 'first_response' as const, +}; + +interface Rig { + engine: ObjectQL; + svc: ApprovalService; + protocol: any; + warnings: Array<{ msg: string; meta?: Record }>; + ids: { recordA: string; recordB: string; requestA: string; requestB: string }; +} + +let current: ObjectQL | undefined; + +afterEach(async () => { + try { await current?.destroy(); } catch { /* noop */ } + current = undefined; +}); + +async function boot( + options: ApprovalsPluginOptions = {}, + /** Runs before the plugin starts, so a middleware it registers wraps the gate. */ + beforeStart?: (engine: ObjectQL) => void, +): Promise { + const engine = new ObjectQL(); + current = engine; + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), true); + await engine.init(); + for (const def of [examSheet, SysApprovalRequest, SysApprovalAction, SysApprovalApprover, SysApprovalDelegation]) { + engine.registry.registerObject(def as any, 'approvals-child-read-gate-test', 'approvals-child-read-gate-test'); + } + await engine.syncSchemas(); + + const recordA = 'sheet_a'; + const recordB = 'sheet_b'; + await engine.insert(OBJECT, { id: recordA, title: 'A' }, { context: SYSTEM } as any); + await engine.insert(OBJECT, { id: recordB, title: 'B' }, { context: SYSTEM } as any); + + // Record readability, the stand-in (see the header): userId -> readable ids. + const readable = new Map>([ + [SUBMITTER_A, new Set([recordA])], + [SUBMITTER_B, new Set([recordB])], + [APPROVER, new Set([recordA, recordB])], + [READER, new Set([recordA])], + [ADMIN.userId, new Set([recordA, recordB])], + ]); + engine.registerMiddleware(async (op: any, next: () => Promise) => { + const ctx = op.context; + if (READ_OPS.has(op.operation) && ctx && !ctx.isSystem && op.ast) { + const grants = readable.get(String(ctx.userId ?? '')); + if (!grants) { + throw Object.assign(new Error(`[Security] Access denied: no read on '${OBJECT}'`), { + code: 'PERMISSION_DENIED', status: 403, + }); + } + const scope = { id: { $in: [...grants] } }; + op.ast.where = op.ast.where ? { $and: [op.ast.where, scope] } : scope; + } + return next(); + }, { object: OBJECT }); + + const services: Record = { objectql: engine }; + const warnings: Rig['warnings'] = []; + const ctx: any = { + getService: (name: string) => { + if (!(name in services)) throw new Error(`[Kernel] Service '${name}' not found`); + return services[name]; + }, + registerService: (name: string, svc: unknown) => { services[name] = svc; }, + // Boot-time sweeps are not under test; they are collected and never fired. + hook: () => {}, + logger: { + info: () => {}, debug: () => {}, error: () => {}, + warn: (msg: string, meta?: Record) => { warnings.push({ msg: String(msg), meta }); }, + }, + }; + beforeStart?.(engine); + await new ApprovalsServicePlugin(options).start(ctx); + const svc = services.approvals as ApprovalService; + + const open = async (recordId: string, submitter: string, runId: string) => (await svc.openNodeRequest({ + object: OBJECT, recordId, runId, nodeId: 'review', flowName: 'exam_review', + config: nodeConfig as any, submitterId: submitter, record: { id: recordId, title: recordId }, + }, asUser(submitter)) as any).id as string; + const requestA = await open(recordA, SUBMITTER_A, 'run_a'); + const requestB = await open(recordB, SUBMITTER_B, 'run_b'); + // A second decision-log row on each request: its current approver's reply. + for (const requestId of [requestA, requestB]) { + await svc.comment(requestId, { actorId: APPROVER, comment: `reply on ${requestId}` }, asUser(APPROVER)); + } + + return { + engine, svc, warnings, + protocol: new ObjectStackProtocolImplementation(engine as any), + ids: { recordA, recordB, requestA, requestB }, + }; +} + +/** Every row of `object`, read as the system: the scene the pins read against. */ +async function allRows(rig: Rig, object: string): Promise { + return rig.engine.find(object, { context: SYSTEM } satisfies EngineQueryOptions) as Promise; +} + +/** The ids of `object`'s rows on `requestId`, as the system reads them. */ +async function rowIdsOf(rig: Rig, object: string, requestId: string): Promise { + return (await allRows(rig, object)).filter((r) => r.request_id === requestId).map((r) => String(r.id)).sort(); +} + +/** The data door's list, as `GET /data/OBJECT` hands it in. */ +async function listIds(rig: Rig, object: string, context: any, query: Record = {}) { + const res = await rig.protocol.findData({ object, query, context }); + return { ids: (res.records as any[]).map((r) => String(r.id)).sort(), total: res.total as number | undefined }; +} + +/** A request's related list — the record page's Timeline — by implicit field filter. */ +const forRequest = (requestId: string) => ({ request_id: requestId }); + +/** The data door's by-id read; the refusal, when it refuses. */ +async function byId(rig: Rig, object: string, context: any, id: string): Promise<{ record?: any; error?: any }> { + try { + return { record: (await rig.protocol.getData({ object, id, context })).record }; + } catch (error) { + return { error }; + } +} + +const NOT_FOUND = { code: 'RECORD_NOT_FOUND', status: 404 }; +const envelope = (error: any) => ({ code: error?.code, status: error?.status }); + +describe('the record-reader tier OFF (the default)', () => { + it('the scene: both requests carry decision-log rows and approver-index rows', async () => { + const rig = await boot(); + for (const requestId of [rig.ids.requestA, rig.ids.requestB]) { + expect((await rowIdsOf(rig, ACTION, requestId)).length, `${ACTION} ${requestId}`).toBe(2); + expect((await rowIdsOf(rig, APPROVER_INDEX, requestId)).length, `${APPROVER_INDEX} ${requestId}`).toBe(1); + } + }); + + for (const object of CHILDREN) { + it(`${object}: a non-participant sees no row — list, a request's list, count, grouped count`, async () => { + const rig = await boot(); + const ctx = asUser(OUTSIDER); + expect(await listIds(rig, object, ctx)).toEqual({ ids: [], total: 0 }); + expect(await listIds(rig, object, ctx, forRequest(rig.ids.requestA))).toEqual({ ids: [], total: 0 }); + expect(await rig.engine.count(object, { context: ctx } satisfies EngineCountOptions)).toBe(0); + const grouped: EngineAggregateOptions = { + groupBy: ['request_id'], aggregations: [{ function: 'count', alias: 'n' }], context: ctx, + }; + expect(await rig.engine.aggregate(object, grouped)).toEqual([]); + }); + + it(`${object}: by id, a hidden row answers exactly as a missing one — 404 RECORD_NOT_FOUND`, async () => { + const rig = await boot(); + const [rowId] = await rowIdsOf(rig, object, rig.ids.requestA); + const hidden = await byId(rig, object, asUser(OUTSIDER), rowId); + const missing = await byId(rig, object, asUser(OUTSIDER), 'no_such_row'); + expect(hidden.record).toBeUndefined(); + expect(envelope(hidden.error)).toEqual(NOT_FOUND); + expect(envelope(missing.error)).toEqual(NOT_FOUND); + }); + + it(`${object}: a participant sees their own request's rows and no one else's`, async () => { + const rig = await boot(); + const { requestA, requestB } = rig.ids; + const rowsA = await rowIdsOf(rig, object, requestA); + const rowsB = await rowIdsOf(rig, object, requestB); + const ctx = asUser(SUBMITTER_A); + expect(await listIds(rig, object, ctx)).toEqual({ ids: rowsA, total: rowsA.length }); + expect(await listIds(rig, object, ctx, forRequest(requestB))).toEqual({ ids: [], total: 0 }); + expect((await byId(rig, object, ctx, rowsA[0])).record?.id).toBe(rowsA[0]); + expect(envelope((await byId(rig, object, ctx, rowsB[0])).error)).toEqual(NOT_FOUND); + // The current approver of both. + expect((await listIds(rig, object, asUser(APPROVER))).ids).toEqual([...rowsA, ...rowsB].sort()); + }); + + it(`${object}: control — an administrator is unchanged: every row, by list, count and id`, async () => { + const rig = await boot(); + const all = (await allRows(rig, object)).map((r) => String(r.id)).sort(); + expect(all.length).toBeGreaterThan(0); + expect(await listIds(rig, object, ADMIN)).toEqual({ ids: all, total: all.length }); + expect(await rig.engine.count(object, { context: ADMIN } satisfies EngineCountOptions)).toBe(all.length); + const [rowB] = await rowIdsOf(rig, object, rig.ids.requestB); + expect((await byId(rig, object, ADMIN, rowB)).record?.id).toBe(rowB); + }); + + it(`${object}: control — the approval engine's own reads, as the system, are not narrowed`, async () => { + const rig = await boot(); + expect((await allRows(rig, object)).length).toBe( + (await rowIdsOf(rig, object, rig.ids.requestA)).length + (await rowIdsOf(rig, object, rig.ids.requestB)).length, + ); + }); + } + + it('the record page\'s Timeline keeps its readers: the submitter, the current approver and an administrator', async () => { + const rig = await boot(); + const timeline = await rowIdsOf(rig, ACTION, rig.ids.requestA); + const filterArray = JSON.stringify([['request_id', '=', rig.ids.requestA]]); + for (const ctx of [asUser(SUBMITTER_A), asUser(APPROVER), ADMIN]) { + expect(await listIds(rig, ACTION, ctx, forRequest(rig.ids.requestA)), ctx.userId) + .toEqual({ ids: timeline, total: timeline.length }); + // The filter-array spelling names the request too. + expect((await listIds(rig, ACTION, ctx, { filter: filterArray })).ids, ctx.userId).toEqual(timeline); + } + }); + + it('one rule, two doors: for every caller a request\'s Timeline equals the approvals door\'s listActions', async () => { + const rig = await boot(); + for (const ctx of [asUser(OUTSIDER), asUser(READER), asUser(SUBMITTER_A), asUser(SUBMITTER_B), asUser(APPROVER), ADMIN]) { + for (const requestId of [rig.ids.requestA, rig.ids.requestB]) { + const served = (await rig.svc.listActions(requestId, ctx)).map((a) => String(a.id)).sort(); + expect((await listIds(rig, ACTION, ctx, forRequest(requestId))).ids, `${ctx.userId} ${requestId}`).toEqual(served); + } + } + }); + + it('one rule, two doors, by id: an action row is served exactly when the approvals door\'s by-row rule admits it', async () => { + const rig = await boot(); + const rows = [...await rowIdsOf(rig, ACTION, rig.ids.requestA), ...await rowIdsOf(rig, ACTION, rig.ids.requestB)]; + for (const ctx of [asUser(OUTSIDER), asUser(READER), asUser(SUBMITTER_A), asUser(SUBMITTER_B), asUser(APPROVER), ADMIN]) { + for (const id of rows) { + const admitted = await rig.svc.authorizeFileRead(id, ctx); + const answer = await byId(rig, ACTION, ctx, id); + expect(answer.record?.id ?? null, `${ctx.userId} ${id}`).toBe(admitted ? id : null); + if (!admitted) expect(envelope(answer.error), `${ctx.userId} ${id}`).toEqual(NOT_FOUND); + } + } + }); +}); + +describe('the record-reader tier ON for the object', () => { + const ON = { recordReaderVisibleObjects: [OBJECT] }; + + for (const object of CHILDREN) { + it(`${object}: a reader of the record sees its request's rows on a read naming the request or the row`, async () => { + const rig = await boot(ON); + const ctx = asUser(READER); + const rowsA = await rowIdsOf(rig, object, rig.ids.requestA); + expect(await listIds(rig, object, ctx, forRequest(rig.ids.requestA))).toEqual({ ids: rowsA, total: rowsA.length }); + expect((await byId(rig, object, ctx, rowsA[0])).record?.id).toBe(rowsA[0]); + }); + + it(`${object}: a read that names nothing stays the reader's participant set — the inbox is not widened`, async () => { + const rig = await boot(ON); + expect(await listIds(rig, object, asUser(READER))).toEqual({ ids: [], total: 0 }); + }); + + it(`${object}: the tier reaches no request whose record its reader cannot read`, async () => { + const rig = await boot(ON); + const [rowB] = await rowIdsOf(rig, object, rig.ids.requestB); + expect(await listIds(rig, object, asUser(READER), forRequest(rig.ids.requestB))).toEqual({ ids: [], total: 0 }); + expect(envelope((await byId(rig, object, asUser(READER), rowB)).error)).toEqual(NOT_FOUND); + expect(await listIds(rig, object, asUser(OUTSIDER), forRequest(rig.ids.requestA))).toEqual({ ids: [], total: 0 }); + }); + } + + it('one rule, two doors, with the tier on as well: Timeline equals listActions, by id equals the by-row rule', async () => { + const rig = await boot(ON); + const rows = [...await rowIdsOf(rig, ACTION, rig.ids.requestA), ...await rowIdsOf(rig, ACTION, rig.ids.requestB)]; + for (const ctx of [asUser(OUTSIDER), asUser(READER), asUser(SUBMITTER_B), ADMIN]) { + for (const requestId of [rig.ids.requestA, rig.ids.requestB]) { + const served = (await rig.svc.listActions(requestId, ctx)).map((a) => String(a.id)).sort(); + expect((await listIds(rig, ACTION, ctx, forRequest(requestId))).ids, `${ctx.userId} ${requestId}`).toEqual(served); + } + for (const id of rows) { + const admitted = await rig.svc.authorizeFileRead(id, ctx); + expect((await byId(rig, ACTION, ctx, id)).record?.id ?? null, `${ctx.userId} ${id}`).toBe(admitted ? id : null); + } + } + }); +}); + +describe('fails closed', () => { + for (const object of CHILDREN) { + it(`${object}: a visibility answer that fails denies the read, and says so`, async () => { + const rig = await boot(); + // The very source the plugin handed the gates: the service registers one object per instance. + vi.spyOn(requestVisibilitySourceOf(rig.svc), 'visibleRequestIdsFor').mockRejectedValueOnce(new Error('probe went away')); + expect((await listIds(rig, object, ADMIN)).ids).toEqual([]); + expect(rig.warnings.some((w) => w.msg.includes('read gate') && w.msg.includes('fail closed') && w.meta?.object === object)) + .toBe(true); + }); + } + + for (const object of CHILDREN) { + it(`${object}: a request about one of its own child rows is no anchor, even when the opt-in names that object — the read never re-enters itself`, async () => { + // A FUSE around the gate, as on the parent's table: the depth of reads of + // `object` made as the reader, nested inside one another. The tier's + // anchor read runs as the caller, so on a child table it would pass + // through that table's gate again, which would ask about the request the + // row belongs to, whose anchor is that row — unbounded on the cycle below. + let depth = 0; + let deepest = 0; + const rig = await boot({ recordReaderVisibleObjects: [OBJECT, object] }, (engine) => { + engine.registerMiddleware(async (op: any, next: () => Promise) => { + if (!READ_OPS.has(op.operation) || op.context?.userId !== READER) return next(); + depth += 1; + deepest = Math.max(deepest, depth); + try { + if (depth > 8) throw new Error('recursion fuse'); + return await next(); + } finally { + depth -= 1; + } + }, { object }); + }); + // A request about a child row that belongs to that very request. + const requestId = 'req_cycle'; + const rowId = 'row_cycle'; + await rig.engine.insert('sys_approval_request', { + id: requestId, process_name: 'flow:cycle', object_name: object, record_id: rowId, status: 'pending', + submitter_id: 'u_somebody_else', + }, { context: SYSTEM } as any); + const row = object === ACTION + ? { id: rowId, request_id: requestId, step_index: 0, action: 'submit', created_at: new Date().toISOString() } + : { id: rowId, request_id: requestId, approver: 'u_somebody_else', created_at: new Date().toISOString() }; + await rig.engine.insert(object, row, { context: SYSTEM } as any); + expect(envelope((await byId(rig, object, asUser(READER), rowId)).error)).toEqual(NOT_FOUND); + expect(await listIds(rig, object, asUser(READER), forRequest(requestId))).toEqual({ ids: [], total: 0 }); + // The fuse saw the reader's reads (its control), and none ran inside another. + expect(deepest).toBe(1); + }); + } +}); diff --git a/packages/plugins/plugin-approvals/src/request-read-gate.ts b/packages/plugins/plugin-approvals/src/request-read-gate.ts index ba757837057..f7b29f84430 100644 --- a/packages/plugins/plugin-approvals/src/request-read-gate.ts +++ b/packages/plugins/plugin-approvals/src/request-read-gate.ts @@ -76,6 +76,31 @@ * door only; a row served here never carries it, so every decision action's * `visible` predicate fails closed on this door, and a record reader admitted * by the tier is served the row read-only on both doors. + * + * ## The child tables (#22589) + * + * A request's two child tables declare the same `get` / `list` shape: + * `sys_approval_action`, the decision log (actor, decision, comment text), and + * `sys_approval_approver`, the pending-approver index. On the approvals door + * the decision log follows its request's visibility (`listActions` serves it + * through `getRequest`, and a decision attachment through `authorizeFileRead`), + * so the generic door applies that same answer to both tables, keyed by the + * row's `request_id`: {@link bindRequestChildReadGates}. ⛔ Still no rule here — + * the same source answers, and the conjunct is a constraint on `request_id` + * built from the ids it returned. Which request a read names + * ({@link requestChildReadTarget}): + * + * - `request_id` pinned — a request's record page lists its Timeline this + * way — is the request loaded by id, which carries its own anchor + * (`listActions`); + * - `id` pinned — `GET /data/sys_approval_action/ID` — is the row loaded by + * id; the request it belongs to is the anchor, read by the source + * (`authorizeFileRead`'s question); + * - anything else is untargeted, the participant set alone. + * + * A read pinned to one request keeps it when visible and nothing otherwise; + * any other read keeps `request_id` among the visible set. A row whose request + * the caller may not see is not served, and by id answers as a missing one. */ import type { ExecutionContext } from '@objectstack/spec/kernel'; @@ -85,16 +110,29 @@ import { APPROVAL_REQUEST_OBJECT, type MiddlewareEngine } from './payload-redact /** The engine reads this gate narrows. */ export const REQUEST_READ_OPS: ReadonlySet = new Set(['find', 'findOne', 'count', 'aggregate']); +/** + * [#22589] A request's child tables served on the generic door, each row + * keyed to its request by `request_id`. + */ +export const APPROVAL_REQUEST_CHILD_OBJECTS: readonly string[] = Object.freeze([ + 'sys_approval_action', + 'sys_approval_approver', +]); + /** No real row matches it: the fail-closed conjunct. */ export const REQUEST_READ_DENY_ALL: Readonly> = Object.freeze({ id: '__approval_request_not_visible__', }); -/** The record a read names, in the approvals door's two spellings. */ +/** + * The record a read names, in the approvals door's two spellings — plus, for a + * child table, the row a read loads by id (`row`). + */ export interface RequestReadTarget { object?: string; recordId?: string; requestId?: string; + row?: { object: string; id: string }; } /** @@ -102,11 +140,18 @@ export interface RequestReadTarget { * written by `ApprovalService`'s constructor and reached through * `requestVisibilitySourceOf`. `null` is "every request in scope"; a set is the * ids the approvals door would serve the caller for a read naming `target`. + * `target.row` names a child-table row loaded by id: the request it belongs to + * is the anchor, as `requestId` is for a request loaded by id. */ export interface RequestVisibilitySource { visibleRequestIdsFor( context: ExecutionContext, - target?: { object?: string | null; recordId?: string | null; requestId?: string | null }, + target?: { + object?: string | null; + recordId?: string | null; + requestId?: string | null; + row?: { object: string; id: string } | null; + }, ): Promise | null>; } @@ -124,29 +169,50 @@ export function requestReadTarget(where: unknown): RequestReadTarget | undefined return undefined; } +/** + * [#22589] The request a read of child table `object` names, read off its + * `where`: `requestId` when `request_id` is pinned, `row` when only the row's + * own `id` is, `undefined` when the read is untargeted. + */ +export function requestChildReadTarget(object: string, where: unknown): RequestReadTarget | undefined { + const requestId = pinnedEquality(where, 'request_id'); + if (requestId !== null) return { requestId }; + const id = pinnedEquality(where, 'id'); + if (id !== null) return { row: { object, id } }; + return undefined; +} + /** * The conjunct that confines a read to `visible`, or `null` to add none. - * `pinnedId` is the one id the read is already pinned to, if any. + * `column` holds a request id — `id` on the request object, `request_id` on a + * child table — and `pinnedId` is the one request id the read already pins it + * to, if any. */ export function requestVisibilityFilter( visible: ReadonlySet | null, pinnedId: string | null, + column: 'id' | 'request_id' = 'id', ): Readonly> | null { if (visible === null) return null; - if (pinnedId !== null) return visible.has(pinnedId) ? { id: pinnedId } : REQUEST_READ_DENY_ALL; + if (pinnedId !== null) return visible.has(pinnedId) ? { [column]: pinnedId } : REQUEST_READ_DENY_ALL; if (visible.size === 0) return REQUEST_READ_DENY_ALL; const ids = [...visible]; - return { id: ids.length === 1 ? ids[0] : { $in: ids } }; + return { [column]: ids.length === 1 ? ids[0] : { $in: ids } }; } +type GateLogger = { warn?: (msg: string, meta?: Record) => void }; + /** - * Register the gate on `sys_approval_request`. `source` is the approvals - * service the plugin started. + * One read gate on `object`: ask `source` about the request(s) a read names + * and AND the answer into it as a constraint on `column`. Fails closed. */ -export function bindRequestReadGate( +function bindVisibilityGate( engine: MiddlewareEngine, + object: string, + column: 'id' | 'request_id', + targetOf: (where: unknown) => RequestReadTarget | undefined, source: RequestVisibilitySource, - logger?: { warn?: (msg: string, meta?: Record) => void }, + logger?: GateLogger, ): void { engine.registerMiddleware(async (opCtx: any, next: () => Promise) => { const ast = opCtx?.ast as { where?: unknown } | undefined; @@ -154,15 +220,43 @@ export function bindRequestReadGate( if (!REQUEST_READ_OPS.has(opCtx?.operation) || !ast || !context) return next(); let filter: Readonly> | null; try { - const visible = await source.visibleRequestIdsFor(context, requestReadTarget(ast.where)); - filter = requestVisibilityFilter(visible, pinnedEquality(ast.where, 'id')); + const visible = await source.visibleRequestIdsFor(context, targetOf(ast.where)); + filter = requestVisibilityFilter(visible, pinnedEquality(ast.where, column), column); } catch (err: any) { logger?.warn?.('[approvals] request read gate could not resolve which approval requests the caller may see — the read is denied (fail closed)', { - operation: opCtx.operation, error: err?.message ?? String(err), + object, operation: opCtx.operation, error: err?.message ?? String(err), }); filter = REQUEST_READ_DENY_ALL; } if (filter) ast.where = ast.where ? { $and: [ast.where, filter] } : filter; return next(); - }, { object: APPROVAL_REQUEST_OBJECT }); + }, { object }); +} + +/** + * Register the gate on `sys_approval_request`. `source` is the approvals + * service the plugin started. + */ +export function bindRequestReadGate( + engine: MiddlewareEngine, + source: RequestVisibilitySource, + logger?: GateLogger, +): void { + bindVisibilityGate(engine, APPROVAL_REQUEST_OBJECT, 'id', requestReadTarget, source, logger); +} + +/** + * [#22589] Register the gate on each of a request's child tables + * ({@link APPROVAL_REQUEST_CHILD_OBJECTS}): a row is served only when the + * request it belongs to is one `source` — the same approvals service — would + * serve the caller. + */ +export function bindRequestChildReadGates( + engine: MiddlewareEngine, + source: RequestVisibilitySource, + logger?: GateLogger, +): void { + for (const object of APPROVAL_REQUEST_CHILD_OBJECTS) { + bindVisibilityGate(engine, object, 'request_id', (where) => requestChildReadTarget(object, where), source, logger); + } } From e6d4665bc62848632e7d359d840c2e5ac4bd4b76 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 04:38:54 +0000 Subject: [PATCH 2/2] chore(changeset): plugin-approvals patch for the child-table read gates Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude --- .../22589-approval-sibling-read-gates.md | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .changeset/22589-approval-sibling-read-gates.md diff --git a/.changeset/22589-approval-sibling-read-gates.md b/.changeset/22589-approval-sibling-read-gates.md new file mode 100644 index 00000000000..516bcea1b9a --- /dev/null +++ b/.changeset/22589-approval-sibling-read-gates.md @@ -0,0 +1,20 @@ +--- +'@objectstack/plugin-approvals': patch +--- + +fix(plugin-approvals): the generic data door serves `sys_approval_action` and `sys_approval_approver` rows by their request's visibility + +Clause-②: no + +A read of a request's two child tables through the generic data door (`find`, `findOne`, `count` and `aggregate`, so the REST data routes, the export route, the MCP data tool and every other engine read) now returns only the rows whose request the approvals door (`/api/v1/approvals/*`) serves the same caller: + +- `sys_approval_action`, the decision log (actor, decision, comment text); +- `sys_approval_approver`, the pending-approver index. + +Before this, a deployment that granted read on either object served every row of the organization on that door, including the decision log and comment text of requests the caller does not take part in. + +- **One rule for both doors, and for all three tables.** The gates ask the approvals service the same question the `sys_approval_request` gate asks, keyed by the row's `request_id`: the request's submitter, its current approvers and those who acted on it see its rows; an administrator holding the approval override sees every row; and, read-only, on an object named in `recordReaderVisibleObjects`, a reader of the record sees its request's rows on a read that names the request (`request_id`) or the row (its id). The published `ApprovalService` type is unchanged. +- **The record page's Timeline keeps its readers.** A request's Timeline (a list of `sys_approval_action` keyed on `request_id`) serves the rows `listActions` serves the same caller; a row read by id is served exactly when a decision attachment on it would be (`authorizeFileRead`, which now shares the gate's read of the row's request). +- **Absence, not refusal.** A row the caller may not see answers a by-id read exactly as a missing one does, `404 RECORD_NOT_FOUND`. A list's `total` and a grouped count are narrowed like its rows. A failed visibility answer denies the read and logs a warning. +- **Unchanged.** The approvals door, an administrator's view, and the approval engine's own reads. A deployment that grants no read on either object still answers `403`. +- The record-reader tier no longer anchors on a row of either table, even when `recordReaderVisibleObjects` names it, as it already never anchored on `sys_approval_request`.