Repository navigation
Commit 1a42b37
fix(plugin-approvals): every door that acts on an approval request answers a request the caller may not read as the read door does (#28916)
domain:devx#1 · sunhaolin · session_017xRBcVy4VXmkQjn2MGeaWk
Clause-②: no
Metadata-protocol: none
## 维护者速读
-
**改了什么**:审批请求上所有"对请求动手"的入口(批准、驳回、撤回、退回修改、重新提交、转交、催办、要求补充信息、评论)在做任何判断之前,先用"读取请求"入口的同一条可见性规则判断调用者能否看到这条请求;看不到的,一律按"请求不存在"回答
`404 REQUEST_NOT_FOUND`,且不写任何数据。规则只有一份:从读取路径抽出为
`requestRowForCaller`,读取与动手两边共用。
- **为什么改**:QA 第三轮发现的安全加固(细节按披露裁决不公开)
-
**风险与回滚**:能看到请求的人(提交人、审批人、已处理过的人、开启了"记录读者"开关时的记录读者)行为不变;管理员越权处理(含跨组织的平台管理员)与系统上下文(SLA
升级、邮件操作链接)不经此判断、行为不变。一处可感知变化(维护者已接受):审批人若在"看不到该请求的组织上下文"中直接调接口(读取入口本就对他返回
404),现在动手入口也返回 404、不再落库后报 500,需切换到请求所在组织或由管理员处理。正常审批每次多一次可见性查询。回滚 =
revert 本 PR。
- **先看**:① `packages/plugins/plugin-approvals/src/approval-service.ts` 的
`requestRowForCaller` 与
`refuseUnreadableRequest`(风险点:越权判定按请求所在组织豁免,其余一律走读取规则);② 同文件
`loadPendingRow`、`decideNode`、`recall`、`resubmit` 中紧跟取行之后的那一行调用;③
`packages/qa/dogfood/test/approval-act-doors-read-visibility.dogfood.test.ts`。
- **两分钟验证**:`pnpm --filter @objectstack/dogfood exec vitest run
test/approval-act-doors-read-visibility.dogfood.test.ts` —— 预期 1/1
通过(九个动手入口对"看不到的已存在请求"与"不存在的 id"回答逐字节相同,另有读者 403/409、审批人可批、管理员越权可批三组对照)。
- **需要你做的**:无,席位落地。
## What changes
A hardening of the approval request doors.
- **One read rule, two consumers.** The read door's rule for a single
request — the row read inside the caller's organization, kept only when
the caller is among the requests it may see (participants, the override
actor's view, the opt-in record-reader tier) — moves out of
`loadRequest` into `requestRowForCaller`. `getRequest` serves by it
exactly as before.
- **Every door that acts on a request asks it first.** `decideNode` (and
so `decide`, the `approve` / `reject` doors), `recall`, `resubmit`, and
`loadPendingRow` (`sendBack`, `reassign`, `remind`, `requestInfo`,
`comment`) call `refuseUnreadableRequest` right after the row is found,
before any check that depends on the row. A caller the read door would
not serve the request to is answered `REQUEST_NOT_FOUND: ID` — the very
error an id that names no request gets, built from the id the caller
sent — and nothing is written.
- **Who is not asked.** The override actor, judged against the request's
organization exactly as each door judges it (a platform administrator's
override crosses the tenancy wall the read door keeps), and system
contexts (the SLA sweep, the email action link). Both act as before.
`issueActionTokens`, which mints for a slot on behalf of no reader,
reads as the system.
- **A caller who can read the request** is judged by the door's own
rule, unchanged: `403 FORBIDDEN` for a reader who may not take the
action, `409 INVALID_STATE` for a request no longer in the state the
action needs.
- **Docs**: the decision section and the error-code table of
`content/docs/automation/approvals.mdx` state the rule.
Clause-② is `no` and the changeset is `patch`: the change swaps a
refusal for the read door's own refusal for callers who cannot read the
request, admits no one new and refuses no reader anything new — the same
reading as the by-id update refusal-order hardening.
## Tests
- **HTTP door (dogfood)** —
`packages/qa/dogfood/test/approval-act-doors-read-visibility.dogfood.test.ts`,
on a real boot (real `SecurityPlugin`, a real flow, the approvals
plugin). A plain member who is no participant gets the same status and
body for an existing request as for an id that names none on all nine
act doors (ten bodies: `approve` with and without decision outputs), on
a pending request and on a decided one, and the row and its history are
unchanged. Controls: the submitter is `403 FORBIDDEN` on the approver's
verbs and `409 INVALID_STATE` on `resubmit` of a pending request and on
a decided one; the approver is `403 FORBIDDEN` on `recall` and `remind`,
and still approves (`200`); the administrator, on no slate, approves
through the override (`200`) and is told `404` for an unknown id.
- **Service** — `approval-service.test.ts` gains a block of four: every
act door answers a non-participant and another organization's
administrator `REQUEST_NOT_FOUND: ID`, byte for byte the missing-id
answer, and writes nothing (pending and decided); a reader keeps the
door's own `FORBIDDEN` / `INVALID_STATE` and the approver decides; with
the record-reader tier on, a tier reader is judged by the door
(`FORBIDDEN`), which shows the doors follow the read rule rather than a
copy of it; the platform administrator's cross-organization override and
a system context act as before.
## Verification
Head `7f95ac12a`: the change (`100cb412d`), its tests (`c47eb917a`),
`origin/main` merged at `bdb19ca6d` (`7fe58dddb`, one append-only
conflict in `approval-service.test.ts`, both blocks kept), and one stale
dogfood expectation the full battery named (`7f95ac12a`). After the
merge: `pnpm install --frozen-lockfile`, `packages/runtime/.objectstack`
removed, the dogfood dependency closure rebuilt; the built
`@objectstack/plugin-approvals` carries the new helper (`dist/index.js`,
7 occurrences). Every test, build and ablation run below held the shared
verify lock; the gates do not take it.
<details><summary>Stock showcase — a throwaway probe (never
committed)</summary>
The showcase booted with its own demo bootstrap; a seeded persona who is
no participant of a pending demo request called the read door, its audit
trail and the nine act doors on that request and on an id that names
nothing. With this change built, every one of the twelve calls answered
both ids with the same status and body, `404 REQUEST_NOT_FOUND`. The
probe file was deleted after the reading.
</details>
<details><summary>Touched packages — tests and typecheck (on 7fe58dd
unless named)</summary>
```
pnpm --filter @objectstack/plugin-approvals typecheck exit 0
check:test-typecheck: OK — 8 file(s) / 321 error(s) / 27 pinned signature(s) held in test-typecheck-debt.json
pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2
Test Files 107 passed (107) Tests 1380 passed (1380)
pnpm --filter @objectstack/dogfood exec vitest run --maxWorkers=1 test/approval-act-doors-read-visibility.dogfood.test.ts
Test Files 1 passed (1) Tests 1 passed (1)
pnpm check:type-check-debt exit 0
check-type-check-coverage --re-measure: OK — 0 ledger entr(ies) re-measured, 0 raw tsc error(s) total
on 7f95ac1:
pnpm --filter @objectstack/dogfood exec vitest run --maxWorkers=1 test/position-address-readers.dogfood.test.ts test/approval-act-doors-read-visibility.dogfood.test.ts
Test Files 2 passed (2) Tests 2 passed (2)
pnpm --filter @objectstack/dogfood typecheck exit 0
pnpm --filter @objectstack/http-conformance exec vitest run --maxWorkers=1 src/hono-approvals-act.conformance.test.ts
Test Files 1 passed (1) Tests 4 passed (4) (the email action-link door, a system context)
pnpm --filter @objectstack/verify exec vitest run --maxWorkers=1 src/action-door-api-forward-denied-subject.test.ts
Test Files 1 passed (1) Tests 3 passed (3)
```
</details>
<details><summary>Ablation of the read-rule call — one-shot, restored
(on 7fe58dd)</summary>
Mutation through `scripts/ablation-replace.mjs`, wrapped, under a trap
restore with absolute paths: the read-rule call in
`refuseUnreadableRequest` replaced by an unconditional return carrying a
marker. Anchor `if (await this.requestRowForCaller(requestId, context,
true)) return;` hit 1 → 0; blob `be0dd52a6d60` → `fa2f535376e2`; marker
count in the source 1. Rebuilt; `ablation-dist-preflight` found the
marker in 2 built files.
- dogfood pin under the mutation: 1 of 1 red (the stranger's `approve`
no longer answers what the missing id gets);
- the service block under the mutation: 1 of 4 red — the every-door
case; the reader, tier and override cases stay green, as they do not
depend on the read-rule call.
Restore: blob back to the `HEAD` blob `be0dd52a6d60` (checked against
`git rev-parse HEAD:path`), `git diff HEAD` empty, whole-tree `git
status --porcelain` empty; rebuilt; `ablation-dist-preflight --absent`:
marker absent from all 6 built files, tree clean; the helper back in
`dist/index.js` (7). Dogfood pin 1/1 and the service block 4/4 green
again.
</details>
<details><summary>Full dogfood battery — `pnpm --filter
@objectstack/dogfood test`, once</summary>
On `7fe58dddb`: `Test Files 1 failed | 382 passed | 3 skipped (386)`,
`Tests 1 failed | 3156 passed | 28 skipped (3185)`, 1404.7 s. The one
failure is this change's consequence:
`position-address-readers.dogfood.test.ts` expected the door's own
refusal for a bystander the read door already hides the request from (it
asserts that `404` two lines earlier). `7f95ac12a` updates that
expectation; the file and the new pin are 2/2 on `7f95ac12a`. The rest
of the battery was not re-run: `7f95ac12a` changes that one test file
only.
</details>
<details><summary>Gates — dispatch-gates.mjs --commands on
7f95ac1</summary>
93 derived families, run locally, each exit code recorded before any
pipe; every one exit 0. `dispatch-gates.mjs --ran`: `93 derived, 93 run,
0 NOT-MEASURED, 0 UNRUN`. Among them:
```
check-nul-bytes: OK (scanned 23654 text file(s) … no raw ASCII control bytes)
check-adr-0087-registration: this PR adds no declared-breaking changeset (1 non-breaking changeset(s) seen).
check:skill-examples: 280 prose examples type-check across 3 surface(s)
check:type-check-debt: surplus: none — every entry sits exactly at its measurement
```
</details>
## Acceptance notes
- **Fixture triage.** A caller who is no participant of the request is
asserted `REQUEST_NOT_FOUND` (admin-exemption, the two name-authority
suites, later-step submitter, the admin-override block, the reassign /
comment outsider cases, the slot-reader block, and the bystander in the
`position-address-readers` dogfood file). Where a pin's subject is the
door's own refusal — the decision slot test, the submitter-only
`resubmit` guard, the localized non-submitter `recall` sentence, the
reassign target that must not be looked up — it now asks a caller who
READS the request (the submitter holding another position, a pending
approver, the override actor), so that refusal is still the one that
answers, and the non-reader is asserted beside it.
- **Doubles.** Two engine doubles (`approval-revise`,
`stranded-resubmit-second-door`) threw on `$or`, which the participant
probe sends; the probe swallows a failed read and keeps what it
collected, so a past actor read as a non-participant there. They now
compose `$or` as the real engine does. A legacy request row inserted by
hand in `approval-list-display-labels` gets the approver index the
plugin's boot backfill (`rebuildApproverIndex`) gives such a row.
- **One behaviour change for a legitimate caller — accepted by the
maintainer.** A pending approver whose active organization does not hold
the request (an org-scoped approver on an org-less request) does not
read it: the read door already answers them `404`. The act doors now
answer them `404 REQUEST_NOT_FOUND` with nothing written, where the
decision used to be recorded and the call then failed `500
READ_BACK_FAILED` on the echo. They act from the request's organization,
or an administrator's override or the email action link decides it. The
maintainer's ruling on it, verbatim:
> 一个连带的行为变化,我接受
The same rule applies to a submitter whose active organization does not
hold the request: `recall`, `remind`, `resubmit` and `comment` now
answer `404 REQUEST_NOT_FOUND` with nothing written. The mechanism is
the same as on the approver arm. The maintainer accepted this arm too in
the PM session, when asked about it explicitly, verbatim:
> 同意
Contract review 6568970094, should-fix 1: a pin for this arm follows in
the next change on these doors.
In `approval-service.test.ts`'s org-filtered read-back block the
read-back refusal stays pinned on `decide` (through the platform
administrator's override) and on `comment` (an org-scoped system
context), and a new arm pins the before-any-write refusal for the
org-scoped approver.
- **Cost.** A non-override caller's act pays one by-id read of the
request inside their organization and the participant probe the read
door already pays. The override actor and system contexts pay nothing.
- **Unchanged on purpose.** `GET /approvals/requests/:id` and `/actions`
(already one answer for both ids), the operator verb
`continueRestoredRun` (no door, no caller context), the action-link
redemption (a system context), `issueActionTokens` (reads as the system,
stated in place).
---
_Generated by [Claude
Code](https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 2c3574c commit 1a42b37
14 files changed
Lines changed: 709 additions & 86 deletions
File tree
- .changeset
- content/docs/automation
- packages
- plugins/plugin-approvals/src
- qa/dogfood/test
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
653 | 653 | | |
654 | 654 | | |
655 | 655 | | |
656 | | - | |
657 | | - | |
658 | | - | |
659 | | - | |
| 656 | + | |
| 657 | + | |
| 658 | + | |
| 659 | + | |
| 660 | + | |
| 661 | + | |
| 662 | + | |
660 | 663 | | |
661 | 664 | | |
662 | 665 | | |
| |||
987 | 990 | | |
988 | 991 | | |
989 | 992 | | |
990 | | - | |
991 | | - | |
| 993 | + | |
| 994 | + | |
992 | 995 | | |
993 | 996 | | |
994 | 997 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
322 | 322 | | |
323 | 323 | | |
324 | 324 | | |
325 | | - | |
326 | | - | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
327 | 330 | | |
328 | 331 | | |
329 | 332 | | |
330 | 333 | | |
331 | 334 | | |
332 | | - | |
333 | | - | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
334 | 339 | | |
335 | 340 | | |
336 | 341 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
242 | 242 | | |
243 | 243 | | |
244 | 244 | | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
245 | 248 | | |
246 | 249 | | |
247 | 250 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
326 | 326 | | |
327 | 327 | | |
328 | 328 | | |
329 | | - | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
330 | 332 | | |
331 | 333 | | |
332 | 334 | | |
| |||
345 | 347 | | |
346 | 348 | | |
347 | 349 | | |
348 | | - | |
349 | | - | |
350 | | - | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
351 | 354 | | |
352 | 355 | | |
353 | 356 | | |
| |||
431 | 434 | | |
432 | 435 | | |
433 | 436 | | |
| 437 | + | |
434 | 438 | | |
435 | 439 | | |
436 | | - | |
| 440 | + | |
437 | 441 | | |
438 | 442 | | |
439 | 443 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
47 | | - | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
48 | 51 | | |
49 | 52 | | |
50 | 53 | | |
| |||
565 | 568 | | |
566 | 569 | | |
567 | 570 | | |
568 | | - | |
| 571 | + | |
| 572 | + | |
569 | 573 | | |
570 | 574 | | |
571 | 575 | | |
| |||
0 commit comments