fix(plugin-approvals): the generic data door serves sys_approval_request rows by the approvals door's visibility (#22559)

.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 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.

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).
@@ -1277,6 +1278,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<any | null> {
+  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<ApprovalService, RequestVisibilitySource>();
+
+/**
+ * [#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;
@@ -1319,6 +1366,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. */
@@ -6774,6 +6854,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;
@@ -6966,14 +7052,10 @@ export class ApprovalService implements IApprovalService {
     enforceVisibility: boolean,
   ): Promise<ApprovalRequestRow | null> {
     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 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
     // — and, once decision attachments derived their access from the request
@@ -6983,16 +7065,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;
   }

packages/plugins/plugin-approvals/src/approvals-plugin.ts

@@ -16,6 +16,7 @@ import { SysApprovalRequestDetailPage } from './sys-approval-request.page.js';
 import { renderConfirmPage, renderResultPage } from './action-link-pages.js';
 import {
   ApprovalService,
+  requestVisibilitySourceOf,
   ESCALATION_JOB_NAME,
   ESCALATION_SCAN_INTERVAL_MS,
   type ApprovalEngine,
@@ -29,6 +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 type { FieldVisibilitySource } from './payload-redaction.js';
 import { registerApprovalNode, type ApprovalAutomationSurface } from './approval-node.js';
 import { backfillActionSlots } from './action-slot-backfill.js';
@@ -70,7 +72,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
@@ -305,6 +310,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, requestVisibilitySourceOf(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');
 

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<string, unknown> = { objectql: engine, security };

packages/plugins/plugin-approvals/src/payload-predicate-guard.ts

@@ -138,27 +138,37 @@ export function namedSnapshotColumn(ast: Record<string, unknown>): 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<string>();
   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());
       }
     }
   };
   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');
 }
 
 /**