From 2100ba5d5367b20bf7176d5d48c9001e355bee4a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 01:25:23 +0000 Subject: [PATCH 1/7] fix(plugin-approvals): the generic data door serves sys_approval_request rows by the approvals door's visibility A read gate on sys_approval_request (find, findOne, count, aggregate) narrows each read to the request ids ApprovalService.visibleRequestIdsFor answers for the caller: the approvals door's own visibleRequestIds, so the participant set, the override actor and the default-off record-reader tier are one rule on both doors. Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude --- .../plugin-approvals/src/approval-service.ts | 79 +++- .../plugin-approvals/src/approvals-plugin.ts | 23 +- .../src/payload-predicate-guard.ts | 22 +- .../src/request-read-gate.integration.test.ts | 337 ++++++++++++++++++ .../plugin-approvals/src/request-read-gate.ts | 160 +++++++++ .../src/sys-approval-request.object.ts | 3 +- 6 files changed, 606 insertions(+), 18 deletions(-) create mode 100644 packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts create mode 100644 packages/plugins/plugin-approvals/src/request-read-gate.ts diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 9a1822845a2..352cc2f4d91 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -6528,6 +6528,63 @@ export class ApprovalService implements IApprovalService { return [...new Set(list.map(r => String(r.request_id)))]; } + /** + * [#22559] This door's request visibility, asked by a read that arrives + * through ANOTHER door: the generic data door's read gate on + * `sys_approval_request` (`request-read-gate.ts`). It is + * {@link ApprovalService.visibleRequestIds} itself — the participant set, + * the override actor's unrestricted view and the default-OFF record-reader + * tier (#8652) — so the two doors serve one rule and cannot drift apart. + * ⛔ No second visibility rule lives in the gate. + * + * `null` means "every request in scope": the override actor, a system + * context included (its own reads are this service's, so the gate never + * narrows them, and answering costs no read). Otherwise the set of request + * ids this door would serve the caller for a read naming `target`; empty + * means none. + * + * `target` names the record the record-reader tier anchors on, exactly as on + * this door: `object` + `recordId` is a list filtered to one record + * ({@link ApprovalService.listRequests}); `requestId` is a request loaded by + * id, which carries its own anchor ({@link ApprovalService.loadRequest}). + * Absent, the read is untargeted — the participant set alone, as for the + * inbox. + */ + async visibleRequestIdsFor( + context: ExecutionContext, + target?: { object?: string | null; recordId?: string | null; requestId?: string | null }, + ): Promise | null> { + // `organizationId` is not on the envelope — see isOverrideActor(). + const tenantOrg = (context as any)?.organizationId ?? context?.tenantId ?? null; + // The same first question `visibleRequestIds` asks, asked before the + // by-id anchor is fetched so the unrestricted answer costs no read. + if (this.isOverrideActor(context, tenantOrg)) return null; + let anchor: { object?: string | null; recordId?: string | null } = { + object: target?.object, recordId: target?.recordId, + }; + if (!(anchor.object && anchor.recordId) && target?.requestId) { + const raw = await this.findRequestRow(target.requestId, tenantOrg); + anchor = raw ? { object: raw.object_name, recordId: raw.record_id } : {}; + } + return this.visibleRequestIds(context, tenantOrg, anchor); + } + + /** + * The stored row request `requestId` names inside `tenantOrg` (every + * organization when `null`), read as the system — the one read a by-id serve + * ({@link ApprovalService.loadRequest}) and a by-id visibility question + * ({@link ApprovalService.visibleRequestIdsFor}) both take, so the two cannot + * disagree about which record an id is anchored on. + */ + private async findRequestRow(requestId: string, tenantOrg: string | null): Promise { + const where: any = { id: requestId }; + if (tenantOrg) where.organization_id = tenantOrg; + const rows = await this.engine.find('sys_approval_request', { + where, limit: 1, context: SYSTEM_CTX, + }); + return Array.isArray(rows) && rows[0] ? rows[0] : null; + } + /** * The request ids this caller is a PARTICIPANT of, or `null` for a caller * who may see everything in scope (#3590). @@ -6712,6 +6769,12 @@ export class ApprovalService implements IApprovalService { const recordId = String(target?.recordId ?? '').trim(); if (!object || !recordId) return; if (!this.recordReaderVisibleObjects.has(object)) return; + // [#22559] An approval request is never the anchor, even when the opt-in + // names its object: the anchor read below runs as the caller, so on this + // 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; // A tokenless/anonymous caller reads nothing. Fail closed, as above. const uid = context?.userId != null ? String(context.userId) : ''; if (!uid) return; @@ -6904,14 +6967,10 @@ export class ApprovalService implements IApprovalService { enforceVisibility: boolean, ): Promise { if (!requestId) return null; - const where: any = { id: requestId }; // `organizationId` is not on the envelope — see isOverrideActor(). const tenantOrg = (context as any)?.organizationId ?? context?.tenantId; - if (tenantOrg) where.organization_id = tenantOrg; - const rows = await this.engine.find('sys_approval_request', { - where, limit: 1, context: SYSTEM_CTX, - }); - if (!Array.isArray(rows) || !rows[0]) return null; + const raw = await this.findRequestRow(requestId, tenantOrg ?? null); + if (!raw) return null; const caller = await this.actingCaller(context); // #3590: tenant scoping alone let any authenticated user read any request // — and, once decision attachments derived their access from the request @@ -6921,16 +6980,16 @@ export class ApprovalService implements IApprovalService { // id carries its own anchor — which is what makes `listActions` and the // decision-attachment gate follow this rule without a second copy of it. const visible = await this.visibleRequestIds(context, tenantOrg ?? null, { - object: rows[0].object_name, recordId: rows[0].record_id, + object: raw.object_name, recordId: raw.record_id, }, caller); - if (visible && !visible.has(String(rows[0].id))) return null; + if (visible && !visible.has(String(raw.id))) return null; } - const row = rowFromRequest(rows[0]); + const row = rowFromRequest(raw); // [#10749] Redact before enrichment — see `redactPayloads`. await this.redactPayloads([row], context); await this.enrichRows([row]); await this.attachFlowSteps(row); - await this.attachDecisionProgress(row, rows[0]); + await this.attachDecisionProgress(row, raw); this.attachViewers([row], context, caller); return row; } diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index 70ec3ecd12a..b313162b53d 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -28,6 +28,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 type { FieldVisibilitySource } from './payload-redaction.js'; import { registerApprovalNode, type ApprovalAutomationSurface } from './approval-node.js'; import { backfillActionSlots } from './action-slot-backfill.js'; @@ -69,7 +70,10 @@ export interface ApprovalsPluginOptions { * on upgrade. Opting in is per object and deliberate: enabling it for a * ledger object does not enable it anywhere else. * - * **What an enabled object exposes**, so the opt-in is informed: the request + * **What an enabled object exposes**, so the opt-in is informed — on the + * approvals door and, for a deployment that grants read on + * `sys_approval_request`, on the generic data door alike (one rule serves + * both, `request-read-gate.ts`): the request * row (including its `payload` snapshot of the record at submission time) and * the full action history — actor, decision, timestamp, the action's COMMENT * text, and any decision attachments. Enable it on objects whose approval @@ -298,6 +302,23 @@ export class ApprovalsServicePlugin implements Plugin { } } + // [#22559] The generic data door's read gate: a read of + // `sys_approval_request` there returns only the requests this service's + // own door would serve the caller — one visibility rule for both doors + // (`request-read-gate.ts`). Bound whenever the service runs, NOT behind + // `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. + if (typeof (engine as any).registerMiddleware === 'function') { + bindRequestReadGate(engine as any, this.service, 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.', + ); + } + ctx.registerService('approvals', this.service); ctx.logger.info('ApprovalsServicePlugin: service registered'); diff --git a/packages/plugins/plugin-approvals/src/payload-predicate-guard.ts b/packages/plugins/plugin-approvals/src/payload-predicate-guard.ts index 25bf3e267f7..31155ff2ddf 100644 --- a/packages/plugins/plugin-approvals/src/payload-predicate-guard.ts +++ b/packages/plugins/plugin-approvals/src/payload-predicate-guard.ts @@ -138,18 +138,19 @@ export function namedSnapshotColumn(ast: Record): NamedSnapshot } /** - * The ONE subject object `where` pins — an equality on `object_name`, plain or + * The ONE string value `where` pins `column` to — an equality, plain or * `{ $eq }`, at the root or inside a root `$and` — or `null` when it pins none - * (or two different ones). + * (or two different ones). A pin is a conjunct every matching row satisfies; + * nothing under `$or` / `$not` is one. */ -export function pinnedSubjectObject(where: unknown): string | null { +export function pinnedEquality(where: unknown, column: string): string | null { const pins = new Set(); const walk = (condition: unknown) => { if (!isRecord(condition)) return; for (const [key, value] of Object.entries(condition)) { if (key === '$and' && Array.isArray(value)) { for (const sub of value) walk(sub); - } else if (key === 'object_name') { + } else if (key === column) { if (typeof value === 'string') pins.add(value.trim()); else if (isRecord(value) && Object.keys(value).length === 1 && typeof value.$eq === 'string') pins.add(value.$eq.trim()); } @@ -157,8 +158,17 @@ export function pinnedSubjectObject(where: unknown): string | null { }; walk(where); if (pins.size !== 1) return null; - const object = [...pins][0]; - return object ? object : null; + const pinned = [...pins][0]; + return pinned ? pinned : null; +} + +/** + * The ONE subject object `where` pins — an equality on `object_name`, plain or + * `{ $eq }`, at the root or inside a root `$and` — or `null` when it pins none + * (or two different ones). + */ +export function pinnedSubjectObject(where: unknown): string | null { + return pinnedEquality(where, 'object_name'); } /** diff --git a/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts new file mode 100644 index 00000000000..45e4c27c2b1 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts @@ -0,0 +1,337 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#22559] `sys_approval_request` on the GENERIC data door serves a caller only + * the requests the approvals door serves it — one rule for both doors. + * + * ## What the rulings fix, and what each pin below holds + * + * #8652 ruled the approvals door's visibility: a request's participants + * (submitter, current approver, past actor) and administrators see it; a + * reader of the record it is about sees it READ-ONLY, and only on an object a + * deployment opted in, default OFF. The triage direction for this card is that + * the data door's reads (`find`, `findOne`, `count`, `aggregate`) apply that + * same definition, and ⛔ NOT the activity stream's parent-record gate on its + * own: admitting every reader of the record would switch the tier on for every + * object. + * + * - a non-participant who cannot read the record sees no row: not in a list, + * not in a record's list, not in a count or a grouped count, and by id the + * request answers exactly as a missing one does; + * - a reader of the record who participates in nothing sees nothing while the + * tier is off — the pin that tells this gate from the activity stream's; + * - a participant sees their own requests and no one else's; + * - with the tier on for the object, a reader of the record sees its requests + * on a read that names the record, read-only; + * - control: an administrator, and the approval engine's own reads as the + * system, are unchanged; + * - for every caller, the data door's ids equal the approvals door's. + * + * ## The rig + * + * A real ObjectQL engine over better-sqlite3 (`@objectstack/driver-sql`), so + * the conjunct the gate adds is compiled and executed by the SQL builder; the + * real `ApprovalsServicePlugin.start()`, so a gate that stops being bound fails + * here like one never written; the real data-door normalizer + * (`ObjectStackProtocolImplementation.findData` / `getData`), so a record + * page's related list and a by-id read arrive in the shape that door gives + * them. + * + * The one stand-in is RECORD READABILITY: in a deployment the business + * object's own CRUD, sharing and RLS decide whether a caller can read a record, + * and the tier only ever asks the engine as the caller. One middleware on the + * business object plays that part — a caller with no read on it is refused, + * one with read sees only its own records. No security plugin is mounted, so + * every caller here holds read on `sys_approval_request` itself: the app's + * grant the card's measurement used. + */ + +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 { ApprovalService } 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 REQUEST = 'sys_approval_request'; +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: string[]; + /** The two requests: A on record A by submitter A, B on record B by submitter B. */ + 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 = {}): 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-read-gate-test', 'approvals-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: string[] = []; + 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) => { warnings.push(String(msg)); }, + }, + }; + 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'); + + return { + engine, svc, warnings, + protocol: new ObjectStackProtocolImplementation(engine as any), + ids: { recordA, recordB, requestA, requestB }, + }; +} + +/** The data door's list, as `GET /data/sys_approval_request` hands it in. */ +async function listIds(rig: Rig, context: any, query: Record = {}) { + const res = await rig.protocol.findData({ object: REQUEST, query, context }); + return { ids: (res.records as any[]).map((r) => String(r.id)).sort(), total: res.total as number | undefined }; +} + +/** A record page's related list: the record named by implicit field filters. */ +const forRecord = (recordId: string) => ({ object_name: OBJECT, record_id: recordId }); + +/** The data door's by-id read; the refusal, when it refuses. */ +async function byId(rig: Rig, context: any, id: string): Promise<{ record?: any; error?: any }> { + try { + return { record: (await rig.protocol.getData({ object: REQUEST, id, context })).record }; + } catch (error) { + return { error }; + } +} + +describe('the record-reader tier OFF (the default)', () => { + it('a non-participant who cannot read the record sees no request: list, a record\'s list, count, grouped count', async () => { + const rig = await boot(); + const ctx = asUser(OUTSIDER); + expect(await listIds(rig, ctx)).toEqual({ ids: [], total: 0 }); + expect(await listIds(rig, ctx, forRecord(rig.ids.recordA))).toEqual({ ids: [], total: 0 }); + expect(await rig.engine.count(REQUEST, { context: ctx } as any)).toBe(0); + const groups = await rig.engine.aggregate(REQUEST, { + groupBy: ['status'], aggregations: [{ function: 'count', alias: 'n' }], context: ctx, + } as any); + expect(groups).toEqual([]); + }); + + it('by id, a hidden request answers exactly as a missing one: 404 RECORD_NOT_FOUND', async () => { + const rig = await boot(); + const hidden = await byId(rig, asUser(OUTSIDER), rig.ids.requestA); + const missing = await byId(rig, asUser(OUTSIDER), 'no_such_request'); + expect(hidden.record).toBeUndefined(); + expect({ code: hidden.error?.code, status: hidden.error?.status }).toEqual({ code: 'RECORD_NOT_FOUND', status: 404 }); + expect({ code: missing.error?.code, status: missing.error?.status }).toEqual({ code: 'RECORD_NOT_FOUND', status: 404 }); + }); + + it('a reader of the record who participates in nothing sees nothing while the tier is off', async () => { + const rig = await boot(); + const ctx = asUser(READER); + // The control half first: this caller really can read the record. + expect((await rig.engine.find(OBJECT, { where: { id: rig.ids.recordA }, context: ctx } as any)).length).toBe(1); + expect(await listIds(rig, ctx, forRecord(rig.ids.recordA))).toEqual({ ids: [], total: 0 }); + expect((await byId(rig, ctx, rig.ids.requestA)).error?.status).toBe(404); + }); + + it('a participant sees their own requests and no one else\'s', async () => { + const rig = await boot(); + const { requestA, requestB } = rig.ids; + expect(await listIds(rig, asUser(SUBMITTER_A))).toEqual({ ids: [requestA], total: 1 }); + expect((await byId(rig, asUser(SUBMITTER_A), requestA)).record?.id).toBe(requestA); + expect((await byId(rig, asUser(SUBMITTER_A), requestB)).error?.status).toBe(404); + // The current approver of both. + expect(await listIds(rig, asUser(APPROVER))).toEqual({ ids: [requestA, requestB].sort(), total: 2 }); + }); + + it('control: an administrator is unchanged — every request, by list, count and id', async () => { + const rig = await boot(); + const all = [rig.ids.requestA, rig.ids.requestB].sort(); + expect(await listIds(rig, ADMIN)).toEqual({ ids: all, total: 2 }); + expect(await rig.engine.count(REQUEST, { context: ADMIN } as any)).toBe(2); + expect((await byId(rig, ADMIN, rig.ids.requestB)).record?.id).toBe(rig.ids.requestB); + }); + + it('control: the approval engine\'s own reads, as the system, are not narrowed', async () => { + const rig = await boot(); + expect((await rig.engine.find(REQUEST, { context: SYSTEM } as any)).length).toBe(2); + // …and the approvals door, which reads that way, still serves its callers. + expect((await rig.svc.listRequests({}, asUser(SUBMITTER_A))).map((r) => r.id)).toEqual([rig.ids.requestA]); + }); + + it('one rule, two doors: for every caller the data door serves the ids the approvals door lists', async () => { + const rig = await boot(); + for (const ctx of [asUser(OUTSIDER), asUser(READER), asUser(SUBMITTER_A), asUser(SUBMITTER_B), asUser(APPROVER), ADMIN]) { + for (const [filter, query] of [ + [{}, {}], + [{ object: OBJECT, recordId: rig.ids.recordA }, forRecord(rig.ids.recordA)], + ] as const) { + const served = (await rig.svc.listRequests(filter as any, ctx)).map((r) => String(r.id)).sort(); + expect((await listIds(rig, ctx, query as any)).ids, `${ctx.userId} ${JSON.stringify(query)}`).toEqual(served); + } + } + }); +}); + +describe('the record-reader tier ON for the object', () => { + const ON = { recordReaderVisibleObjects: [OBJECT] }; + + it('a reader of the record sees its request on a read that names the record, read-only', async () => { + const rig = await boot(ON); + const ctx = asUser(READER); + expect(await listIds(rig, ctx, forRecord(rig.ids.recordA))).toEqual({ ids: [rig.ids.requestA], total: 1 }); + const served = (await byId(rig, ctx, rig.ids.requestA)).record; + expect(served?.id).toBe(rig.ids.requestA); + // Read-only on this door: the per-caller `viewer` block (`attachedOnRead`) + // is the approvals door's, so no decision action's predicate holds here… + expect(served).not.toHaveProperty('viewer'); + // …and on the approvals door the tier confers no decision either. + expect((await rig.svc.getRequest(rig.ids.requestA, ctx))?.viewer) + .toEqual({ can_act: false, can_override: false, is_submitter: false }); + }); + + it('the filter-array spelling of a record\'s list names the record too', async () => { + const rig = await boot(ON); + const filter = JSON.stringify([['object_name', '=', OBJECT], ['record_id', '=', rig.ids.recordA]]); + expect(await listIds(rig, asUser(READER), { filter })).toEqual({ ids: [rig.ids.requestA], total: 1 }); + }); + + it('a read that names no record stays the reader\'s participant set — the inbox is not widened', async () => { + const rig = await boot(ON); + expect(await listIds(rig, asUser(READER))).toEqual({ ids: [], total: 0 }); + }); + + it('the tier reaches no record its reader cannot read', async () => { + const rig = await boot(ON); + expect(await listIds(rig, asUser(READER), forRecord(rig.ids.recordB))).toEqual({ ids: [], total: 0 }); + expect((await byId(rig, asUser(READER), rig.ids.requestB)).error?.status).toBe(404); + expect(await listIds(rig, asUser(OUTSIDER), forRecord(rig.ids.recordA))).toEqual({ ids: [], total: 0 }); + expect((await byId(rig, asUser(OUTSIDER), rig.ids.requestA)).error?.status).toBe(404); + }); + + it('one rule, two doors, with the tier on as well', async () => { + const rig = await boot(ON); + for (const ctx of [asUser(OUTSIDER), asUser(READER), asUser(SUBMITTER_B), ADMIN]) { + for (const recordId of [rig.ids.recordA, rig.ids.recordB]) { + const served = (await rig.svc.listRequests({ object: OBJECT, recordId }, ctx)).map((r) => String(r.id)).sort(); + expect((await listIds(rig, ctx, forRecord(recordId))).ids, `${ctx.userId} ${recordId}`).toEqual(served); + } + } + }); +}); + +describe('fails closed', () => { + it('a visibility answer that fails denies the read, and says so', async () => { + const rig = await boot(); + vi.spyOn(rig.svc, 'visibleRequestIdsFor').mockRejectedValueOnce(new Error('probe went away')); + expect((await listIds(rig, ADMIN)).ids).toEqual([]); + expect(rig.warnings.some((w) => w.includes('request read gate') && w.includes('fail closed'))).toBe(true); + }); + + it('a request about a request is no anchor, even when the opt-in names that object — and the read ends', async () => { + const rig = await boot({ recordReaderVisibleObjects: [OBJECT, REQUEST] }); + // Two rows naming each other: a read that followed the anchor would never end. + const first = 'req_cycle_1'; + const second = 'req_cycle_2'; + for (const [id, other] of [[first, second], [second, first]]) { + await rig.engine.insert(REQUEST, { + id, process_name: 'flow:cycle', object_name: REQUEST, record_id: other, status: 'pending', + submitter_id: 'u_somebody_else', + }, { context: SYSTEM } as any); + } + expect((await byId(rig, asUser(READER), first)).error?.status).toBe(404); + expect(await listIds(rig, asUser(READER), { object_name: REQUEST, record_id: second })).toEqual({ ids: [], total: 0 }); + }, 5_000); +}); diff --git a/packages/plugins/plugin-approvals/src/request-read-gate.ts b/packages/plugins/plugin-approvals/src/request-read-gate.ts new file mode 100644 index 00000000000..cba86887126 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/request-read-gate.ts @@ -0,0 +1,160 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#22559] The read gate on `sys_approval_request` at the GENERIC data door: a + * read there returns only the requests the approvals door would serve the same + * caller. + * + * ## The defect this closes + * + * The object declares `enable.apiMethods: ['get', 'list']`, so once an app + * grants read on it — the natural way to put a record's approval history on + * its page — the generic door served every request of the organization, the + * payload snapshots of records the caller cannot open included. The approvals + * door (`ApprovalService.listRequests` / `getRequest`) never did: it serves a + * request to its participants, to the override actor, and — where a + * deployment opted in per object — read-only to a reader of the record it is + * about (#8652). + * + * ## One rule, two doors + * + * ⛔ Nothing here decides who may see a request. The answer is + * `ApprovalService.visibleRequestIdsFor`, which IS the approvals door's + * `visibleRequestIds`: the participant set (submitter, current approver, past + * actor), the override actor's unrestricted view, and the record-reader tier, + * which stays default OFF. This module only reads which record a query names, + * hands that to the service, and ANDs the answer into the read. + * + * ⛔ This is deliberately NOT `plugin-audit`'s parent-record read gate (the + * activity stream's shape), which keeps a row whenever its parent record is + * readable. Here that would switch the record-reader tier on for every object, + * against the ruling's default OFF. + * + * ## Which record a read names + * + * As on the approvals door, the record-reader tier answers only where a record + * is named, and the gate reads the name off the query's conjunctive pins + * ({@link pinnedEquality}): + * + * - `object_name` and `record_id` both pinned — a record page's related list — + * is `listRequests` with `object` + `recordId`; + * - `id` pinned — `GET /data/sys_approval_request/ID` — is a request loaded by + * id, which carries its own anchor (`getRequest`); + * - anything else is untargeted, the participant set alone: the inbox's + * answer. A pin under `$or` / `$not` names nothing, so it reads untargeted — + * narrower, never wider. + * + * ## The narrowing + * + * Added as a conjunct of `ast.where` on `find`, `findOne`, `count` and + * `aggregate`, so a list's `total` and a grouped count are narrowed exactly + * like its rows. It is always a constraint on `id` built from the ids the + * service returned, so it can only keep rows the service named: + * + * - the unrestricted answer (`null`) adds nothing — Setup's all-requests list + * for an administrator is unchanged, and so is every read the approval + * engine makes as the system; + * - a read pinned to one id keeps that id when it is visible and nothing + * otherwise, so a by-id read never carries the caller's whole set; + * - any other read keeps `id` among the visible set; an empty set keeps + * nothing. + * + * Fails CLOSED: a failed visibility answer denies the read (logged), and a + * row with no participant and no admitted record is simply not in the set. + * A context-less programmatic call on a bare kernel is not narrowed, as for + * the sibling read seams: every real transport carries a context. + * + * ## What it composes with + * + * The other seams on this object stay as they were: the snapshot redaction + * (`payload-redaction-middleware.ts`) narrows what a served row's + * `payload_json` holds, and the snapshot query guard + * (`payload-predicate-guard.ts`) refuses a query over it. The per-caller + * `viewer` block declared under `attachedOnRead` is attached by the approvals + * 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. + */ + +import { pinnedEquality } from './payload-predicate-guard.js'; +import { APPROVAL_REQUEST_OBJECT, type MiddlewareEngine } from './payload-redaction-middleware.js'; + +/** The engine reads this gate narrows. */ +export const REQUEST_READ_OPS: ReadonlySet = new Set(['find', 'findOne', 'count', 'aggregate']); + +/** 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. */ +export interface RequestReadTarget { + object?: string; + recordId?: string; + requestId?: string; +} + +/** The one visibility definition the gate reads: the approvals service's. */ +export interface RequestVisibilitySource { + visibleRequestIdsFor( + context: any, + target?: { object?: string | null; recordId?: string | null; requestId?: string | null }, + ): Promise | null>; +} + +/** + * The record a read of `sys_approval_request` names, read off its `where`: + * `object` + `recordId` when both are pinned, `requestId` when only the id is, + * `undefined` when the read is untargeted. + */ +export function requestReadTarget(where: unknown): RequestReadTarget | undefined { + const object = pinnedEquality(where, 'object_name'); + const recordId = pinnedEquality(where, 'record_id'); + if (object !== null && recordId !== null) return { object, recordId }; + const requestId = pinnedEquality(where, 'id'); + if (requestId !== null) return { requestId }; + 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. + */ +export function requestVisibilityFilter( + visible: ReadonlySet | null, + pinnedId: string | null, +): Readonly> | null { + if (visible === null) return null; + if (pinnedId !== null) return visible.has(pinnedId) ? { id: 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 } }; +} + +/** + * Register the gate on `sys_approval_request`. `source` is the approvals + * service the plugin started. + */ +export function bindRequestReadGate( + engine: MiddlewareEngine, + source: RequestVisibilitySource, + logger?: { warn?: (msg: string, meta?: Record) => void }, +): void { + engine.registerMiddleware(async (opCtx: any, next: () => Promise) => { + const ast = opCtx?.ast as { where?: unknown } | undefined; + const context = opCtx?.context; + 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')); + } 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), + }); + filter = REQUEST_READ_DENY_ALL; + } + if (filter) ast.where = ast.where ? { $and: [ast.where, filter] } : filter; + return next(); + }, { object: APPROVAL_REQUEST_OBJECT }); +} diff --git a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts index c4b45c28b39..3deeee3ea60 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts +++ b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts @@ -651,7 +651,8 @@ export const SysApprovalRequest = ObjectSchema.create({ enable: { // [ADR-0103] Engine-owned: the approval engine owns the request lifecycle // (SYSTEM_CTX); users act via domain actions (Submit/Approve/Recall), never - // generic CRUD. Reads stay open. + // generic CRUD. Reads stay open, and serve each caller only the requests + // the approvals door would (#22559, `request-read-gate.ts`). apiMethods: ['get', 'list'], }, }); From 672b5a6ab48dd0d4800928f675bb720024d57f1f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 01:27:11 +0000 Subject: [PATCH 2/7] chore(changeset): plugin-approvals patch for the sys_approval_request read gate Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude --- .../22559-approval-request-read-gate.md | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .changeset/22559-approval-request-read-gate.md diff --git a/.changeset/22559-approval-request-read-gate.md b/.changeset/22559-approval-request-read-gate.md new file mode 100644 index 00000000000..e379f2c0157 --- /dev/null +++ b/.changeset/22559-approval-request-read-gate.md @@ -0,0 +1,20 @@ +--- +'@objectstack/plugin-approvals': patch +--- + +fix(plugin-approvals): the generic data door serves `sys_approval_request` rows by the approvals door's visibility + +Clause-②: no + +A read of `sys_approval_request` 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 requests the approvals door (`/api/v1/approvals/*`) serves the same caller: + +- the requests the caller submitted, is a current approver of, or has acted on; +- every request, for an administrator holding the approval override; +- read-only, on an object named in `recordReaderVisibleObjects`, the requests of a record the caller can read, on a read that names that record (`object_name` and `record_id`, or the request's id). + +Before this, a deployment that granted read on the object served every request of the organization on that door, including the payload snapshots of records the caller cannot open. + +- **One rule for both doors.** The gate asks `ApprovalService.visibleRequestIdsFor`, which is the approvals door's own visibility. It is not the activity stream's parent-record gate, which admits every reader of the record: the record-reader tier stays off unless the deployment names the object. +- **Absence, not refusal.** A request 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 (Setup → Approvals → Requests), and the approval engine's own reads. A deployment that grants no read on the object still answers `403`. +- The record-reader tier no longer anchors on `sys_approval_request` itself, even when `recordReaderVisibleObjects` names it. From db91f7f5451565878fffc420d139a3192fc0f216 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 01:41:48 +0000 Subject: [PATCH 3/7] test(plugin-approvals): the cycle pin reads the gate's re-entry depth through a fuse Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude --- .../src/request-read-gate.integration.test.ts | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts index 45e4c27c2b1..0de1fd93366 100644 --- a/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts @@ -103,7 +103,11 @@ afterEach(async () => { current = undefined; }); -async function boot(options: ApprovalsPluginOptions = {}): Promise { +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({ @@ -160,6 +164,7 @@ async function boot(options: ApprovalsPluginOptions = {}): Promise { warn: (msg: string) => { warnings.push(String(msg)); }, }, }; + beforeStart?.(engine); await new ApprovalsServicePlugin(options).start(ctx); const svc = services.approvals as ApprovalService; @@ -320,9 +325,30 @@ describe('fails closed', () => { expect(rig.warnings.some((w) => w.includes('request read gate') && w.includes('fail closed'))).toBe(true); }); - it('a request about a request is no anchor, even when the opt-in names that object — and the read ends', async () => { - const rig = await boot({ recordReaderVisibleObjects: [OBJECT, REQUEST] }); - // Two rows naming each other: a read that followed the anchor would never end. + it('a request about a request is no anchor, even when the opt-in names that object — the read never re-enters itself', async () => { + // A FUSE around the gate: the depth of reads of the request object made as + // the reader, nested inside one another. The tier's anchor read runs as the + // caller, so on this object it would pass through the gate again, which + // would ask about the row that one names — unbounded on the cycle below. + // The fuse stops a runaway at a bound (a sqlite driver answers in + // microtasks, so an unbounded chain starves every timer, the test timeout + // included), and the depth it saw is the reading. + let depth = 0; + let deepest = 0; + const rig = await boot({ recordReaderVisibleObjects: [OBJECT, REQUEST] }, (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: REQUEST }); + }); + // Two rows naming each other. const first = 'req_cycle_1'; const second = 'req_cycle_2'; for (const [id, other] of [[first, second], [second, first]]) { @@ -333,5 +359,7 @@ describe('fails closed', () => { } expect((await byId(rig, asUser(READER), first)).error?.status).toBe(404); expect(await listIds(rig, asUser(READER), { object_name: REQUEST, record_id: second })).toEqual({ ids: [], total: 0 }); - }, 5_000); + // The fuse saw the reader's reads (its control), and none ran inside another. + expect(deepest).toBe(1); + }); }); From 6e839b359c14942e23ce6b0639836fe5b47024a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 01:47:34 +0000 Subject: [PATCH 4/7] test(plugin-approvals): the snapshot guard's readers are the requests' approvers The generic door now serves a request only to whom the approvals door does, so the guard's fixture readers hold a pending-approver slot on the rows they query; what the file measures stays the guard's answer. Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude --- .../src/payload-predicate-guard.test.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/payload-predicate-guard.test.ts b/packages/plugins/plugin-approvals/src/payload-predicate-guard.test.ts index 12561ed1333..0d6e260408a 100644 --- a/packages/plugins/plugin-approvals/src/payload-predicate-guard.test.ts +++ b/packages/plugins/plugin-approvals/src/payload-predicate-guard.test.ts @@ -30,6 +30,8 @@ import { SqlDriver } from '@objectstack/driver-sql'; import type { EngineAggregateOptions, EngineQueryOptions } from '@objectstack/spec/data'; import { ApprovalsServicePlugin } from './approvals-plugin.js'; import { SysApprovalRequest } from './sys-approval-request.object.js'; +import { SysApprovalApprover } from './sys-approval-approver.object.js'; +import { SysApprovalAction } from './sys-approval-action.object.js'; import { namedSnapshotColumn, pinnedSubjectObject } from './payload-predicate-guard.js'; const REQUEST_OBJECT = 'sys_approval_request'; @@ -132,7 +134,7 @@ describe('[#21154] a query over the approval snapshot by a reader withheld a sub useNullAsDefault: true, }), true); await engine.init(); - for (const o of [SysApprovalRequest, subjectObject, openSubject]) { + for (const o of [SysApprovalRequest, SysApprovalApprover, SysApprovalAction, subjectObject, openSubject]) { engine.registry.registerObject(o as any, HARNESS_PACKAGE); } await engine.syncSchemas(); @@ -141,8 +143,19 @@ describe('[#21154] a query over the approval snapshot by a reader withheld a sub object_name: subject, record_id: String(row.id), status: 'pending', process_name: 'flow:ppg', submitter_id: 'u_ppg_submitter', payload_json: JSON.stringify(row), }); - await engine.insert(REQUEST_OBJECT, snapshot(SUBJECT, { id: 's1', name: 'subject', f_masked: V.masked, f_unserved: V.unserved }), { context: SYS }); - await engine.insert(REQUEST_OBJECT, snapshot(OPEN_SUBJECT, { id: 'o1', name: 'open', f_open: V.open }), { context: SYS }); + const requests = [ + await engine.insert(REQUEST_OBJECT, snapshot(SUBJECT, { id: 's1', name: 'subject', f_masked: V.masked, f_unserved: V.unserved }), { context: SYS }), + await engine.insert(REQUEST_OBJECT, snapshot(OPEN_SUBJECT, { id: 'o1', name: 'open', f_open: V.open }), { context: SYS }), + ]; + // [#22559] The generic door serves a request only to whom the approvals + // door does, so the three readers are the requests' current approvers (the + // pending-approver index): the rows they query are rows they may read, and + // what this file measures stays the snapshot guard's answer alone. + for (const request of requests) { + for (const reader of [MASKED_READER, UNSERVED_READER, CONTROL]) { + await engine.insert('sys_approval_approver', { request_id: String(request.id), approver: reader!.userId }, { context: SYS }); + } + } // The plugin mounts its own generic-door seams onto this engine. const services: Record = { objectql: engine, security }; From 9ec53a7a91ce0ae6baa4d56d3406d8c045bcf711 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 02:04:43 +0000 Subject: [PATCH 5/7] test(plugin-approvals): the read-gate pins type their engine options Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude --- .../src/request-read-gate.integration.test.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts index 0de1fd93366..b4a0cd3d05a 100644 --- a/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts @@ -50,6 +50,7 @@ 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 } from './approval-service.js'; import { ApprovalsServicePlugin, type ApprovalsPluginOptions } from './approvals-plugin.js'; import { SysApprovalRequest } from './sys-approval-request.object.js'; @@ -206,10 +207,11 @@ describe('the record-reader tier OFF (the default)', () => { const ctx = asUser(OUTSIDER); expect(await listIds(rig, ctx)).toEqual({ ids: [], total: 0 }); expect(await listIds(rig, ctx, forRecord(rig.ids.recordA))).toEqual({ ids: [], total: 0 }); - expect(await rig.engine.count(REQUEST, { context: ctx } as any)).toBe(0); - const groups = await rig.engine.aggregate(REQUEST, { + expect(await rig.engine.count(REQUEST, { context: ctx } satisfies EngineCountOptions)).toBe(0); + const grouped: EngineAggregateOptions = { groupBy: ['status'], aggregations: [{ function: 'count', alias: 'n' }], context: ctx, - } as any); + }; + const groups = await rig.engine.aggregate(REQUEST, grouped); expect(groups).toEqual([]); }); @@ -226,7 +228,7 @@ describe('the record-reader tier OFF (the default)', () => { const rig = await boot(); const ctx = asUser(READER); // The control half first: this caller really can read the record. - expect((await rig.engine.find(OBJECT, { where: { id: rig.ids.recordA }, context: ctx } as any)).length).toBe(1); + expect((await rig.engine.find(OBJECT, { where: { id: rig.ids.recordA }, context: ctx } satisfies EngineQueryOptions)).length).toBe(1); expect(await listIds(rig, ctx, forRecord(rig.ids.recordA))).toEqual({ ids: [], total: 0 }); expect((await byId(rig, ctx, rig.ids.requestA)).error?.status).toBe(404); }); @@ -245,13 +247,13 @@ describe('the record-reader tier OFF (the default)', () => { const rig = await boot(); const all = [rig.ids.requestA, rig.ids.requestB].sort(); expect(await listIds(rig, ADMIN)).toEqual({ ids: all, total: 2 }); - expect(await rig.engine.count(REQUEST, { context: ADMIN } as any)).toBe(2); + expect(await rig.engine.count(REQUEST, { context: ADMIN } satisfies EngineCountOptions)).toBe(2); expect((await byId(rig, ADMIN, rig.ids.requestB)).record?.id).toBe(rig.ids.requestB); }); it('control: the approval engine\'s own reads, as the system, are not narrowed', async () => { const rig = await boot(); - expect((await rig.engine.find(REQUEST, { context: SYSTEM } as any)).length).toBe(2); + expect((await rig.engine.find(REQUEST, { context: SYSTEM } satisfies EngineQueryOptions)).length).toBe(2); // …and the approvals door, which reads that way, still serves its callers. expect((await rig.svc.listRequests({}, asUser(SUBMITTER_A))).map((r) => r.id)).toEqual([rig.ids.requestA]); }); From 3f6587c883237c185dbbc9a20e012bc569cf19c7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 02:39:53 +0000 Subject: [PATCH 6/7] fix(plugin-approvals): the read gate reaches the approvals door's visibility in-package, the published ApprovalService type unchanged The visibility source moves off the class into a module-scoped WeakMap the constructor fills, reached through requestVisibilitySourceOf, which the package entry does not re-export; the by-id row read becomes a module function. ApprovalService's member list equals origin/main's. Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude --- .../22559-approval-request-read-gate.md | 2 +- .../plugin-approvals/src/approval-service.ts | 139 ++++++++++-------- .../plugin-approvals/src/approvals-plugin.ts | 3 +- .../src/request-read-gate.integration.test.ts | 5 +- .../plugin-approvals/src/request-read-gate.ts | 24 ++- 5 files changed, 103 insertions(+), 70 deletions(-) diff --git a/.changeset/22559-approval-request-read-gate.md b/.changeset/22559-approval-request-read-gate.md index e379f2c0157..9fa42e24f85 100644 --- a/.changeset/22559-approval-request-read-gate.md +++ b/.changeset/22559-approval-request-read-gate.md @@ -14,7 +14,7 @@ A read of `sys_approval_request` through the generic data door (`find`, `findOne Before this, a deployment that granted read on the object served every request of the organization on that door, including the payload snapshots of records the caller cannot open. -- **One rule for both doors.** The gate asks `ApprovalService.visibleRequestIdsFor`, which is the approvals door's own visibility. It is not the activity stream's parent-record gate, which admits every reader of the record: the record-reader tier stays off unless the deployment names the object. +- **One rule for both doors.** The gate asks the approvals service for its own visibility (the same rule `listRequests` and `getRequest` apply), through an in-package handle: the published `ApprovalService` type is unchanged. It is not the activity stream's parent-record gate, which admits every reader of the record: the record-reader tier stays off unless the deployment names the object. - **Absence, not refusal.** A request 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 (Setup → Approvals → Requests), and the approval engine's own reads. A deployment that grants no read on the object still answers `403`. - The record-reader tier no longer anchors on `sys_approval_request` itself, even when `recordReaderVisibleObjects` names it. diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 352cc2f4d91..15c6eb19b2c 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -85,6 +85,7 @@ import { resolveReadableSnapshotFields, type FieldVisibilitySource, } from './payload-redaction.js'; +import type { RequestVisibilitySource } from './request-read-gate.js'; /** * Node-era approval runtime (ADR-0019). @@ -1250,6 +1251,52 @@ export interface ApprovalServiceOptions { messageTranslator?: () => ValidationMessageTranslator | undefined; } +/** + * The stored row request `requestId` names inside `tenantOrg` (every + * organization when `null`), read as the system — the one read a by-id serve + * (`ApprovalService.loadRequest`) and a by-id visibility question (the read + * gate's source below) both take, so the two cannot disagree about which record + * an id is anchored on. + */ +async function findRequestRow( + engine: ApprovalEngine, + requestId: string, + tenantOrg: string | null, +): Promise { + const where: any = { id: requestId }; + if (tenantOrg) where.organization_id = tenantOrg; + const rows = await engine.find('sys_approval_request', { + where, limit: 1, context: SYSTEM_CTX, + }); + return Array.isArray(rows) && rows[0] ? rows[0] : 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`). + * + * In-package only, and deliberately not a member of {@link ApprovalService}: + * the published class type stays exactly what it was, and the one consumer — + * `ApprovalsServicePlugin`, binding the gate — reaches it through + * {@link requestVisibilitySourceOf}, which the package entry does not + * re-export. The answer is written by the class itself, in its constructor, + * so it calls the class's private rule rather than restating it. + */ +const REQUEST_VISIBILITY = new WeakMap(); + +/** + * [#22559] The visibility source the read gate consumes for `service`: the + * approvals door's own `visibleRequestIds`, asked for a read that arrives + * through another door. Every constructed service registers one. + */ +export function requestVisibilitySourceOf(service: ApprovalService): RequestVisibilitySource { + const source = REQUEST_VISIBILITY.get(service); + if (!source) { + throw new Error('[approvals] no request visibility source is registered for this ApprovalService instance'); + } + return source; +} + export class ApprovalService implements IApprovalService { private readonly engine: ApprovalEngine; private readonly clock: ApprovalClock; @@ -1292,6 +1339,39 @@ export class ApprovalService implements IApprovalService { .map((n) => String(n ?? '').trim()) .filter(Boolean), ); + // [#22559] This door's request visibility, asked by a read that arrives + // through ANOTHER door: the generic data door's read gate on + // `sys_approval_request`. It is `visibleRequestIds` itself — the + // participant set, the override actor's unrestricted view and the + // default-OFF record-reader tier (#8652) — so the two doors serve one rule + // and cannot drift apart. ⛔ No second visibility rule lives in the gate. + // + // `null` is "every request in scope": the override actor, a system context + // included (its own reads are this service's, so the gate never narrows + // them, and answering costs no read). Otherwise the ids this door would + // serve the caller for a read naming `target`; empty means none. `target` + // names the record the tier anchors on, exactly as on this door: `object` + + // `recordId` is a list filtered to one record (`listRequests`); `requestId` + // 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. + REQUEST_VISIBILITY.set(this, { + visibleRequestIdsFor: async (context, target) => { + // `organizationId` is not on the envelope — see isOverrideActor(). + const tenantOrg = (context as any)?.organizationId ?? context?.tenantId ?? null; + // The same first question `visibleRequestIds` asks, asked before the + // by-id anchor is fetched so the unrestricted answer costs no read. + if (this.isOverrideActor(context, tenantOrg)) return null; + 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 } : {}; + } + return this.visibleRequestIds(context, tenantOrg, anchor); + }, + }); } /** Attach (or replace) the ADR-0105 D9 posture provider. */ @@ -6528,63 +6608,6 @@ export class ApprovalService implements IApprovalService { return [...new Set(list.map(r => String(r.request_id)))]; } - /** - * [#22559] This door's request visibility, asked by a read that arrives - * through ANOTHER door: the generic data door's read gate on - * `sys_approval_request` (`request-read-gate.ts`). It is - * {@link ApprovalService.visibleRequestIds} itself — the participant set, - * the override actor's unrestricted view and the default-OFF record-reader - * tier (#8652) — so the two doors serve one rule and cannot drift apart. - * ⛔ No second visibility rule lives in the gate. - * - * `null` means "every request in scope": the override actor, a system - * context included (its own reads are this service's, so the gate never - * narrows them, and answering costs no read). Otherwise the set of request - * ids this door would serve the caller for a read naming `target`; empty - * means none. - * - * `target` names the record the record-reader tier anchors on, exactly as on - * this door: `object` + `recordId` is a list filtered to one record - * ({@link ApprovalService.listRequests}); `requestId` is a request loaded by - * id, which carries its own anchor ({@link ApprovalService.loadRequest}). - * Absent, the read is untargeted — the participant set alone, as for the - * inbox. - */ - async visibleRequestIdsFor( - context: ExecutionContext, - target?: { object?: string | null; recordId?: string | null; requestId?: string | null }, - ): Promise | null> { - // `organizationId` is not on the envelope — see isOverrideActor(). - const tenantOrg = (context as any)?.organizationId ?? context?.tenantId ?? null; - // The same first question `visibleRequestIds` asks, asked before the - // by-id anchor is fetched so the unrestricted answer costs no read. - if (this.isOverrideActor(context, tenantOrg)) return null; - let anchor: { object?: string | null; recordId?: string | null } = { - object: target?.object, recordId: target?.recordId, - }; - if (!(anchor.object && anchor.recordId) && target?.requestId) { - const raw = await this.findRequestRow(target.requestId, tenantOrg); - anchor = raw ? { object: raw.object_name, recordId: raw.record_id } : {}; - } - return this.visibleRequestIds(context, tenantOrg, anchor); - } - - /** - * The stored row request `requestId` names inside `tenantOrg` (every - * organization when `null`), read as the system — the one read a by-id serve - * ({@link ApprovalService.loadRequest}) and a by-id visibility question - * ({@link ApprovalService.visibleRequestIdsFor}) both take, so the two cannot - * disagree about which record an id is anchored on. - */ - private async findRequestRow(requestId: string, tenantOrg: string | null): Promise { - const where: any = { id: requestId }; - if (tenantOrg) where.organization_id = tenantOrg; - const rows = await this.engine.find('sys_approval_request', { - where, limit: 1, context: SYSTEM_CTX, - }); - return Array.isArray(rows) && rows[0] ? rows[0] : null; - } - /** * The request ids this caller is a PARTICIPANT of, or `null` for a caller * who may see everything in scope (#3590). @@ -6969,7 +6992,7 @@ export class ApprovalService implements IApprovalService { if (!requestId) return null; // `organizationId` is not on the envelope — see isOverrideActor(). const tenantOrg = (context as any)?.organizationId ?? context?.tenantId; - const raw = await this.findRequestRow(requestId, tenantOrg ?? null); + const raw = await findRequestRow(this.engine, requestId, tenantOrg ?? null); if (!raw) return null; const caller = await this.actingCaller(context); // #3590: tenant scoping alone let any authenticated user read any request diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index b313162b53d..5ea13580fcd 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -15,6 +15,7 @@ import { SysApprovalDelegation } from './sys-approval-delegation.object.js'; import { renderConfirmPage, renderResultPage } from './action-link-pages.js'; import { ApprovalService, + requestVisibilitySourceOf, ESCALATION_JOB_NAME, ESCALATION_SCAN_INTERVAL_MS, type ApprovalEngine, @@ -310,7 +311,7 @@ export class ApprovalsServicePlugin implements Plugin { // the manual API), and a read through the generic door needs the gate // whichever API wrote the row. if (typeof (engine as any).registerMiddleware === 'function') { - bindRequestReadGate(engine as any, this.service, ctx.logger); + bindRequestReadGate(engine as any, requestVisibilitySourceOf(this.service), ctx.logger); } else { ctx.logger.warn( 'ApprovalsServicePlugin: the ObjectQL engine has no middleware seam — reads of sys_approval_request ' diff --git a/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts index b4a0cd3d05a..4cd422c5933 100644 --- a/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts @@ -51,7 +51,7 @@ 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 } from './approval-service.js'; +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'; @@ -322,7 +322,8 @@ describe('the record-reader tier ON for the object', () => { describe('fails closed', () => { it('a visibility answer that fails denies the read, and says so', async () => { const rig = await boot(); - vi.spyOn(rig.svc, 'visibleRequestIdsFor').mockRejectedValueOnce(new Error('probe went away')); + // The very source the plugin handed the gate: the service registers one object per instance. + vi.spyOn(requestVisibilitySourceOf(rig.svc), 'visibleRequestIdsFor').mockRejectedValueOnce(new Error('probe went away')); expect((await listIds(rig, ADMIN)).ids).toEqual([]); expect(rig.warnings.some((w) => w.includes('request read gate') && w.includes('fail closed'))).toBe(true); }); diff --git a/packages/plugins/plugin-approvals/src/request-read-gate.ts b/packages/plugins/plugin-approvals/src/request-read-gate.ts index cba86887126..ba757837057 100644 --- a/packages/plugins/plugin-approvals/src/request-read-gate.ts +++ b/packages/plugins/plugin-approvals/src/request-read-gate.ts @@ -18,12 +18,14 @@ * * ## One rule, two doors * - * ⛔ Nothing here decides who may see a request. The answer is - * `ApprovalService.visibleRequestIdsFor`, which IS the approvals door's - * `visibleRequestIds`: the participant set (submitter, current approver, past - * actor), the override actor's unrestricted view, and the record-reader tier, - * which stays default OFF. This module only reads which record a query names, - * hands that to the service, and ANDs the answer into the read. + * ⛔ Nothing here decides who may see a request. The answer is the approvals + * service's own: {@link RequestVisibilitySource}, which the plugin reaches + * with `requestVisibilitySourceOf(service)` (`approval-service.ts`, in-package + * only) and which IS the approvals door's private `visibleRequestIds`: the + * participant set (submitter, current approver, past actor), the override + * actor's unrestricted view, and the record-reader tier, which stays default + * OFF. This module only reads which record a query names, hands that to the + * service, and ANDs the answer into the read. * * ⛔ This is deliberately NOT `plugin-audit`'s parent-record read gate (the * activity stream's shape), which keeps a row whenever its parent record is @@ -76,6 +78,7 @@ * by the tier is served the row read-only on both doors. */ +import type { ExecutionContext } from '@objectstack/spec/kernel'; import { pinnedEquality } from './payload-predicate-guard.js'; import { APPROVAL_REQUEST_OBJECT, type MiddlewareEngine } from './payload-redaction-middleware.js'; @@ -94,10 +97,15 @@ export interface RequestReadTarget { requestId?: string; } -/** The one visibility definition the gate reads: the approvals service's. */ +/** + * The one visibility definition the gate reads: the approvals service's, + * 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`. + */ export interface RequestVisibilitySource { visibleRequestIdsFor( - context: any, + context: ExecutionContext, target?: { object?: string | null; recordId?: string | null; requestId?: string | null }, ): Promise | null>; } From a11d87ccaa9f3e738233185afdb168f99162308e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 03:01:47 +0000 Subject: [PATCH 7/7] test(plugin-approvals): by-id parity of the two doors, the read a request's record page makes Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude --- .../src/request-read-gate.integration.test.ts | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts index 4cd422c5933..08cf2c58924 100644 --- a/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts @@ -25,7 +25,8 @@ * on a read that names the record, read-only; * - control: an administrator, and the approval engine's own reads as the * system, are unchanged; - * - for every caller, the data door's ids equal the approvals door's. + * - for every caller, the data door's ids equal the approvals door's, by + * list and by id (the by-id read is what a request's record page makes). * * ## The rig * @@ -201,6 +202,26 @@ async function byId(rig: Rig, context: any, id: string): Promise<{ record?: any; } } +/** + * By id, the data door against the approvals door, for every caller and both + * requests: the record read a `sys_approval_request` record page makes (its + * highlights and details) is served exactly when `getRequest` serves the + * request — and a refused one answers as a missing one. + */ +async function expectByIdParity(rig: Rig, callers: any[]): Promise { + for (const ctx of callers) { + for (const id of [rig.ids.requestA, rig.ids.requestB]) { + const served = (await rig.svc.getRequest(id, ctx)) !== null; + const answer = await byId(rig, ctx, id); + expect(answer.record?.id ?? null, `${ctx.userId} ${id}`).toBe(served ? id : null); + if (!served) { + expect({ code: answer.error?.code, status: answer.error?.status }, `${ctx.userId} ${id}`) + .toEqual({ code: 'RECORD_NOT_FOUND', status: 404 }); + } + } + } +} + describe('the record-reader tier OFF (the default)', () => { it('a non-participant who cannot read the record sees no request: list, a record\'s list, count, grouped count', async () => { const rig = await boot(); @@ -270,6 +291,11 @@ describe('the record-reader tier OFF (the default)', () => { } } }); + + it('one rule, two doors, by id: a request\'s record page reads it exactly when the approvals door serves it', async () => { + const rig = await boot(); + await expectByIdParity(rig, [asUser(OUTSIDER), asUser(READER), asUser(SUBMITTER_A), asUser(SUBMITTER_B), asUser(APPROVER), ADMIN]); + }); }); describe('the record-reader tier ON for the object', () => { @@ -317,6 +343,11 @@ describe('the record-reader tier ON for the object', () => { } } }); + + it('one rule, two doors, by id, with the tier on as well', async () => { + const rig = await boot(ON); + await expectByIdParity(rig, [asUser(OUTSIDER), asUser(READER), asUser(SUBMITTER_A), asUser(SUBMITTER_B), asUser(APPROVER), ADMIN]); + }); }); describe('fails closed', () => {