Skip to content

Commit 83b8b80

Browse files
fix(plugin-approvals): the generic data door serves sys_approval_action and sys_approval_approver rows by their request's visibility (#22589) (#22613)
Fixes #22589 Clause-②: no ## What this does A read of a request's two child tables through the generic data door (`find`, `findOne`, `count` and `aggregate` on the engine, 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. This is the sibling half of #22559 (PR #22587, `76bc1e03a3`), which gave `sys_approval_request` the same rule. The default (no grant, `403`) is unchanged. ## Step 1: reach, measured before any code Measured through the real HTTP data routes on an in-process stack (`@objectstack/verify`'s `bootStack` with the real `SecurityPlugin`, automation, the record-change trigger and `ApprovalsServicePlugin`). The scene: one approval flow, two submitters each with one pending request, one position-holding approver, and members granted read on one child table at a time through an app permission set. The probe was a local scratch file and is not part of this PR. | caller (class) | table read | before (`76bc1e03a3`) | after (`e6d4665bc6`) | |---|---|---|---| | member, no grant (control) | either table: list, a request's list, by id | `403 PERMISSION_DENIED` | `403 PERMISSION_DENIED` | | member with a read grant, takes part in neither request | `sys_approval_action` | both requests' rows by list (total 2), by another request's pin, and by id (`200`) | 0 rows (total 0); by id `404 RECORD_NOT_FOUND` | | same, grant on the other table | `sys_approval_approver` | both requests' rows (total 2); by id `200` | 0 rows; by id `404 RECORD_NOT_FOUND` | | submitter of one request, with both grants | either table | the other submitter's rows too (by list and by id) | own request's rows only; the other's by id `404` | | current approver of both, with both grants | either table | (no grant in the first run) | both requests' rows, each request's Timeline 1 row, by id `200` | On both runs the approvals door listed 0 requests and 0 history rows for the non-participants, so the generic door was serving rows of requests the approvals door does not show the caller. `sys_approval_token` (a third request-keyed table) had no rows in this scene, so its reach is NOT MEASURED; see the acceptance notes. ## One definition, both doors - **No second rule.** The child-table gates (`bindRequestChildReadGates`, `request-read-gate.ts`) ask the same in-package source the parent's gate asks, `requestVisibilitySourceOf(service).visibleRequestIdsFor(...)`, which is the approvals door's private `visibleRequestIds`: the participant set, the override actor's unrestricted view (`null`, so administrators and system reads are never narrowed and the answer costs no read), and the default-OFF record-reader tier on top. The gate ANDs a conjunct on `request_id` built only from the ids it returned. - **Which request a read names** (`requestChildReadTarget`): `request_id` pinned (a record page's Timeline) is the request loaded by id, which carries its own anchor, as `listActions` reads it. `id` pinned (a row read by id) is the row loaded by id: the source resolves the row's request through `findChildRequestId`, which `authorizeFileRead` now shares, so the two doors cannot disagree about which request a row belongs to. Anything else is untargeted, the participant set alone, so the inbox shape is not widened by the tier. - **The parent's gate and the children's are one binder.** `bindVisibilityGate` (module-private) holds the middleware body; `bindRequestReadGate` keeps its signature and behaviour (keyed on `id`), and the children are keyed on `request_id`. `requestVisibilityFilter` takes the column. - **Fails closed.** A failed visibility answer denies the read and logs the same warning as the parent's gate, now with the object in its metadata. A row with no request is in no set. - **Absence, not refusal.** A row the caller may not see answers a by-id read as a missing one does, `404 RECORD_NOT_FOUND`; a list's `total` and a grouped count are narrowed like its rows. - **The tier never anchors on a child-table row** (`addRecordReaderVisibleIds`), as it already never anchored on a request. The anchor read runs as the caller, so on a child table it would pass through that table's gate, which asks about the request the row belongs to, whose anchor can be that very row. Ablation B below measures the unbounded re-entry this prevents. - **Published surface unchanged.** No member is added to `ApprovalService` (its published type is unchanged); `src/index.ts` is untouched; `requestVisibilitySourceOf` stays a module export the package entry does not re-export; no `packages/spec` path. Who may decide is untouched: no decision path is edited. - Bound in `ApprovalsServicePlugin.start()` beside `bindRequestReadGate`, outside `disableAutoHooks`, as the parent's is. An engine without a middleware seam gets one boot warning naming all three tables. ## The record page's Timeline (the #22567 regression pin) The request record page lists `sys_approval_action` keyed on `request_id`. Pinned in `request-child-read-gate.integration.test.ts`: - the submitter, the current approver and an administrator each keep the request's Timeline rows, by the implicit filter and by the filter-array spelling; - for every caller (non-participant, record reader, both submitters, approver, administrator) and both requests, the Timeline's rows equal `listActions`, tier off and tier on; - by id, an action row is served exactly when `authorizeFileRead` admits it, tier off and tier on. ## Pins and ablations New file `packages/plugins/plugin-approvals/src/request-child-read-gate.integration.test.ts`, on the parent's rig: a real ObjectQL engine over better-sqlite3, the real `ApprovalsServicePlugin.start()`, and the real data-door normalizer (`ObjectStackProtocolImplementation.findData` / `getData`). 25 tests: per table, a non-participant sees nothing (list, a request's list, count, grouped count), a hidden row by id answers as a missing one, a participant sees only their own request's rows, an administrator and system reads are unchanged; the Timeline pins above; tier on, per table; fail closed, per table; the re-entry fuse, per table. - **Red first:** on the unmodified code the new file ran 17 failed and 8 passed. The 8 are the positives and controls: scene, administrator, system, Timeline readers, tier-on readers. - **Green:** with the fix, both gate files ran 41 passed (25 new and 16 of the parent's). Ablations, each from the committed head `7721d61e0c` through `scripts/ablation-replace.mjs` (anchor hit x1 to x0, blob changed; restored to the HEAD blob, `git diff HEAD` empty, re-proved after each leg by the driver's own trap-armed check). The subject resolves through relative source imports, so no `dist/` is on the path and no rebuild applies. Run on both gate files: | | mutation | result | |---|---|---| | A | child gates unwired in `approvals-plugin.ts` | 17 failed / 24 passed: every negative pin of both tables, both parity pins, the fail-closed pins and the fuses. First: `expected { ids: [ …(4) ], total: 4 } to deeply equal { ids: [], total: +0 }` | | B | the tier's anchor guard no longer covers child tables | 2 failed: both fuses, `expected 9 to be 1` (re-entry until the fuse at depth 9) | | C | the gate's catch fails open | 3 failed: the parent's and both children's fail-closed pins | | D | a row read by id no longer resolves its request (untargeted) | 3 failed: the tier-on by-id pins for both tables and the tier-on parity pin (`expected null to be 'aact_…'`) | | E | a read pinned by `request_id` no longer asks the targeted shape | 3 failed: the tier-on request-list pins for both tables and the tier-on parity pin | With the tier off, the targeted and untargeted answers are identical, so D and E move tier-on pins only. That is the expected direction. ## Verification (head `7721d61e0c`, which merges `origin/main` `d748ae80af`, a spec-only commit) - `pnpm turbo run build --concurrency=1`: 73 tasks, exit 0. - `pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2`: 67 files, 988 passed, exit 0. - `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0. `check:test-typecheck` holds 8 files / 324 errors / 27 signatures; the new file adds none. - Gates: `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derives 67; all 67 ran, each exit 0. `--ran` with recorded exit codes: 67 derived, 67 run, 0 NOT-MEASURED (a derived zero). - Lint, narrowed: `eslint --no-inline-config --format json` over the 4 changed `.ts` files gives 4 files, 0 errors, 0 warnings. The population is read from `eslint.config.mjs` (`isPathIgnored` false for all four). No `parserOptions.project` or `projectService` is set, so type-aware linting is off and this diff cannot move any untouched file's verdict. ## Acceptance notes - **`sys_approval_token`, same family, not folded in.** It declares `get` / `list` on the generic door with no read middleware, and its rows carry `request_id`, `approver_id`, `token_hash` and `expires_at`. Measured: the control answers `403`; with a grant, `200` with 0 rows, because no action-link token existed in the scene, so its reach is NOT MEASURED. It is left out because who may see token rows is a policy question rather than a mechanical extension: the approvals door serves token rows to nobody, while request visibility would serve them to participants. Reported to the seat. - **On a default deployment the record page is denied to its declared audience.** No baseline set grants read on the three approval tables, so a request's submitter and approvers get `403` on the request's own data-door read and on its Timeline unless an app grants read. Measured in both probe runs; it predates this PR, and #22559's card measured the request half. With all three tables now narrowed to request visibility, a baseline grant would serve only visible rows. Whether to add one is a product call outside this PR. - **A row read by id carries the caller's whole visible set** as its `request_id` conjunct (the row's request is resolved inside the source, not in the gate). This is correct but wider in SQL than the parent's one-id by-id conjunct. It is bounded by the participant probes' window (`APPROVER_INDEX_CAP`). - **`authorizeFileRead` now reads the row's request through `findChildRequestId`**, with a `request_id` projection added and its behaviour unchanged. Its existing tests in `approval-service.test.ts` pass in the full suite. - **The anchor-guard extension narrows the approvals door too**, only for a deployment that names `sys_approval_action` or `sys_approval_approver` in `recordReaderVisibleObjects`. No in-repo caller does, and the `serve` command passes no options to the plugin. - Declared to CI: the dogfood suites. They resolve `plugin-approvals` from source; the one dogfood reader of these tables (`position-address-readers`) reads as the system, which is unnarrowed. --- _Generated by [Claude Code](https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 29a163f commit 83b8b80

5 files changed

Lines changed: 593 additions & 30 deletions

File tree

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
fix(plugin-approvals): the generic data door serves `sys_approval_action` and `sys_approval_approver` rows by their request's visibility
6+
7+
Clause-②: no
8+
9+
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:
10+
11+
- `sys_approval_action`, the decision log (actor, decision, comment text);
12+
- `sys_approval_approver`, the pending-approver index.
13+
14+
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.
15+
16+
- **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.
17+
- **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).
18+
- **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.
19+
- **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`.
20+
- 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`.

‎packages/plugins/plugin-approvals/src/approval-service.ts‎

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ import {
8585
resolveReadableSnapshotFields,
8686
type FieldVisibilitySource,
8787
} from './payload-redaction.js';
88-
import type { RequestVisibilitySource } from './request-read-gate.js';
88+
import { APPROVAL_REQUEST_CHILD_OBJECTS, type RequestVisibilitySource } from './request-read-gate.js';
8989

9090
/**
9191
* Node-era approval runtime (ADR-0019).
@@ -1298,6 +1298,26 @@ async function findRequestRow(
12981298
return Array.isArray(rows) && rows[0] ? rows[0] : null;
12991299
}
13001300

1301+
/**
1302+
* [#22589] The request a row of one of its child tables (`object`, one of
1303+
* `APPROVAL_REQUEST_CHILD_OBJECTS`) belongs to, read as the system — the one
1304+
* read the approvals door's by-row rule (`ApprovalService.authorizeFileRead`)
1305+
* and a by-id visibility question about a child row (the read gate's source
1306+
* below) both take, so the two cannot disagree about which request a row is
1307+
* anchored on. `null` when the row does not exist or names no request.
1308+
*/
1309+
async function findChildRequestId(
1310+
engine: ApprovalEngine,
1311+
object: string,
1312+
rowId: string,
1313+
): Promise<string | null> {
1314+
const rows = await engine.find(object, {
1315+
where: { id: rowId }, fields: ['request_id'], limit: 1, context: SYSTEM_CTX,
1316+
});
1317+
const requestId = (Array.isArray(rows) ? rows[0] : undefined)?.request_id;
1318+
return requestId != null && String(requestId) !== '' ? String(requestId) : null;
1319+
}
1320+
13011321
/**
13021322
* [#22559] Each service's own request visibility, as the generic data door's
13031323
* read gate on `sys_approval_request` asks it (`request-read-gate.ts`).
@@ -1382,6 +1402,12 @@ export class ApprovalService implements IApprovalService {
13821402
// is a request loaded by id, which carries its own anchor (`loadRequest`).
13831403
// Absent, the read is untargeted — the participant set alone, as for the
13841404
// inbox.
1405+
//
1406+
// [#22589] The same answer serves the gates on a request's child tables
1407+
// (`sys_approval_action`, `sys_approval_approver`), which keep a row when
1408+
// its `request_id` is in it. There `target.row` names a child row loaded
1409+
// by id: the request it belongs to is the anchor, as `authorizeFileRead`
1410+
// reads it — `findChildRequestId`, shared with that method.
13851411
REQUEST_VISIBILITY.set(this, {
13861412
visibleRequestIdsFor: async (context, target) => {
13871413
// `organizationId` is not on the envelope — see isOverrideActor().
@@ -1392,9 +1418,13 @@ export class ApprovalService implements IApprovalService {
13921418
let anchor: { object?: string | null; recordId?: string | null } = {
13931419
object: target?.object, recordId: target?.recordId,
13941420
};
1395-
if (!(anchor.object && anchor.recordId) && target?.requestId) {
1396-
const raw = await findRequestRow(this.engine, target.requestId, tenantOrg);
1397-
anchor = raw ? { object: raw.object_name, recordId: raw.record_id } : {};
1421+
if (!(anchor.object && anchor.recordId)) {
1422+
const requestId = target?.requestId
1423+
|| (target?.row ? await findChildRequestId(this.engine, target.row.object, target.row.id) : null);
1424+
if (requestId) {
1425+
const raw = await findRequestRow(this.engine, requestId, tenantOrg);
1426+
anchor = raw ? { object: raw.object_name, recordId: raw.record_id } : {};
1427+
}
13981428
}
13991429
return this.visibleRequestIds(context, tenantOrg, anchor);
14001430
},
@@ -6859,7 +6889,10 @@ export class ApprovalService implements IApprovalService {
68596889
// object it passes through the generic door's read gate, which asks this
68606890
// very function about the row it names — a chain of requests about
68616891
// requests, unbounded on a cycle. Fail closed: no tier on it.
6862-
if (object === 'sys_approval_request') return;
6892+
// [#22589] Nor is a row of one of its child tables, for the same reason:
6893+
// their gates ask this function about the request a row belongs to, whose
6894+
// anchor can be that very row.
6895+
if (object === 'sys_approval_request' || APPROVAL_REQUEST_CHILD_OBJECTS.includes(object)) return;
68636896
// A tokenless/anonymous caller reads nothing. Fail closed, as above.
68646897
const uid = context?.userId != null ? String(context.userId) : '';
68656898
if (!uid) return;
@@ -7271,15 +7304,12 @@ export class ApprovalService implements IApprovalService {
72717304
async authorizeFileRead(actionId: string, context: ExecutionContext): Promise<boolean> {
72727305
if (!actionId) return false;
72737306
try {
7274-
const rows = await this.engine.find('sys_approval_action', {
7275-
where: { id: actionId },
7276-
limit: 1,
7277-
context: SYSTEM_CTX,
7278-
});
7279-
const requestId = (Array.isArray(rows) ? rows[0] : undefined)?.request_id;
7307+
// [#22589] The row's request, read the way the generic door's gate on
7308+
// this table reads it for a by-id read — `findChildRequestId`, shared.
7309+
const requestId = await findChildRequestId(this.engine, 'sys_approval_action', actionId);
72807310
if (!requestId) return false;
72817311
// Same gate as listActions: visibility of the decision's parent request.
7282-
return !!(await this.getRequest(String(requestId), context));
7312+
return !!(await this.getRequest(requestId, context));
72837313
} catch {
72847314
return false;
72857315
}

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

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import {
3030
} from './lifecycle-hooks.js';
3131
import { bindSnapshotRedactionMiddleware } from './payload-redaction-middleware.js';
3232
import { bindSnapshotPredicateGuard } from './payload-predicate-guard.js';
33-
import { bindRequestReadGate } from './request-read-gate.js';
33+
import { bindRequestChildReadGates, bindRequestReadGate } from './request-read-gate.js';
3434
import type { FieldVisibilitySource } from './payload-redaction.js';
3535
import { registerApprovalNode, type ApprovalAutomationSurface } from './approval-node.js';
3636
import { backfillActionSlots } from './action-slot-backfill.js';
@@ -317,13 +317,18 @@ export class ApprovalsServicePlugin implements Plugin {
317317
// `disableAutoHooks`: that switch is the record lock's (a caller driving
318318
// the manual API), and a read through the generic door needs the gate
319319
// whichever API wrote the row.
320+
// [#22589] …and its child tables, `sys_approval_action` (the decision log)
321+
// and `sys_approval_approver` (the approver index): a row there is served
322+
// only when its request is, by the same source.
320323
if (typeof (engine as any).registerMiddleware === 'function') {
321-
bindRequestReadGate(engine as any, requestVisibilitySourceOf(this.service), ctx.logger);
324+
const visibility = requestVisibilitySourceOf(this.service);
325+
bindRequestReadGate(engine as any, visibility, ctx.logger);
326+
bindRequestChildReadGates(engine as any, visibility, ctx.logger);
322327
} else {
323328
ctx.logger.warn(
324-
'ApprovalsServicePlugin: the ObjectQL engine has no middleware seam — reads of sys_approval_request '
325-
+ 'through the generic data door are NOT narrowed to the approval requests the caller may see. '
326-
+ 'Grant no read on that object to non-administrators on this stack.',
329+
'ApprovalsServicePlugin: the ObjectQL engine has no middleware seam — reads of sys_approval_request, '
330+
+ 'sys_approval_action and sys_approval_approver through the generic data door are NOT narrowed to the '
331+
+ 'approval requests the caller may see. Grant no read on those objects to non-administrators on this stack.',
327332
);
328333
}
329334

0 commit comments

Comments
 (0)