Skip to content

fix(plugin-approvals): the generic data door serves sys_approval_action and sys_approval_approver rows by their request's visibility (#22589) - #22613

Merged
objectstack-fleet[bot] merged 3 commits into
mainfrom
claude/issue-22589-approval-sibling-read-gates
Oct 10, 2026
Merged

objectstack-fleet[bot] merged 3 commits into
mainfrom
claude/issue-22589-approval-sibling-read-gates

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

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 plugin-approvals: sys_approval_request has no parent-record read gate on the data door, and the ruled record-reader tier cannot be switched on by an app (17.7.0) #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

…on and sys_approval_approver rows by their request's visibility

A read of either child table through the generic data door (find, findOne,
count, aggregate) now keeps a row only when its request_id names a request
the approvals door would serve the same caller: the same visibility source
the sys_approval_request gate consumes (requestVisibilitySourceOf), keyed by
request_id. A read pinned to one request asks the targeted shape; a row read
by id resolves its request through findChildRequestId, which
authorizeFileRead now shares. Fails closed; administrator and system reads
are unnarrowed. The record-reader tier never anchors on a child-table row,
as it never anchors on a request.

Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Oct 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-approvals, touching 16 documentable anchor(s).

5 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/automation/approvals.mdx (via sys_approval_action (literal, a string literal in APPROVAL_REQUEST_CHILD_OBJECTS; a string literal in authorizeFileRead), sys_approval_request (literal, a string literal in addRecordReaderVisibleIds))
  • content/docs/automation/flows.mdx (via sys_approval_action (literal, a string literal in APPROVAL_REQUEST_CHILD_OBJECTS; a string literal in authorizeFileRead), sys_approval_request (literal, a string literal in addRecordReaderVisibleIds))
  • content/docs/automation/workflows.mdx (via sys_approval_action (literal, a string literal in APPROVAL_REQUEST_CHILD_OBJECTS; a string literal in authorizeFileRead), sys_approval_request (literal, a string literal in addRecordReaderVisibleIds))
  • content/docs/plugins/packages.mdx (via sys_approval_action (literal, a string literal in APPROVAL_REQUEST_CHILD_OBJECTS; a string literal in authorizeFileRead), sys_approval_request (literal, a string literal in addRecordReaderVisibleIds))
  • content/docs/ui/translations.mdx (via sys_approval_request (literal, a string literal in addRecordReaderVisibleIds))

⛔ 3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via sys_approval_action (literal, a string literal in APPROVAL_REQUEST_CHILD_OBJECTS; a string literal in authorizeFileRead), sys_approval_request (literal, a string literal in addRecordReaderVisibleIds))
  • content/docs/releases/v17/17-6.mdx (via sys_approval_request (literal, a string literal in addRecordReaderVisibleIds))
  • content/docs/releases/v17/17-7.mdx (via sys_approval_action (literal, a string literal in APPROVAL_REQUEST_CHILD_OBJECTS; a string literal in authorizeFileRead))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 54 of 206 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 97 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 6 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json d748ae80afe6ed63d1c61a51a69f4fb18280c844 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from d7af06e34040d2f70c24ded9d3269acfd60f5c27 — the merge of head 7721d61e0c87e7bda67001a96d1a4b206d1659f0 into base d748ae80afe6ed63d1c61a51a69f4fb18280c844, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin d7af06e34040d2f70c24ded9d3269acfd60f5c27 && git checkout d7af06e34040d2f70c24ded9d3269acfd60f5c27
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin d748ae80afe6ed63d1c61a51a69f4fb18280c844 7721d61e0c87e7bda67001a96d1a4b206d1659f0 && git checkout -B drift-repro d748ae80afe6ed63d1c61a51a69f4fb18280c844 && git merge --no-ff 7721d61e0c87e7bda67001a96d1a4b206d1659f0

node scripts/docs-audit/affected-docs.mjs --json d748ae80afe6ed63d1c61a51a69f4fb18280c844

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs d748ae80afe6ed63d1c61a51a69f4fb18280c844 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants