Skip to content

fix(approvals): 记录锁对谓词式(multi)更新同样生效 (#4778) - #4838

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-4778-approval-lock-multi-update
Aug 3, 2026
Merged

fix(approvals): 记录锁对谓词式(multi)更新同样生效 (#4778)#4838
os-zhuang merged 2 commits into
mainfrom
claude/issue-4778-approval-lock-multi-update

Conversation

@os-zhuang

@os-zhuang os-zhuang commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #4778

问题

bindApprovalLockHook 注册的全局 beforeUpdate(ADR-0019 记录锁)开头是 if (!id) return。而 update() 只在 where.id 是标量时才提取 input.id,任何谓词式更新都走 updateMany 且 input.id 为 undefined —— 于是「没解析到行」被当成「没有东西需要授权」,实际是「从来没有查询过」。

这是 #4757 / #4630 同一族的 fail-open,但绕过成本最低:不需要 admin、不需要 isSystem、不需要 lockRecord: false、也不需要命中 approvalStatusField 白名单,只要把同一次编辑写成 multi: true:

await ql.update('crm_opportunity', { amount: 999 }, { where: { id: 'rec_1' } });                        // RECORD_LOCKED
await ql.update('crm_opportunity', { amount: 999 }, { where: { id: { $in: ['rec_1'] } }, multi: true }); // 曾经通过
await ql.update('crm_opportunity', { amount: 999 }, { where: { name: 'x' }, multi: true });              // 曾经通过

修法

hook 现在先解析这次写入会碰到哪些行,再逐行判定。

  • 按 id 的路径不变:driver 按主键写,where 的其余部分不参与,所以这里不做任何收窄 —— 收窄反而会变成新的 fail-open(where: { id: 'rec_1', stage: 'x' } 匹配不上时会误放行,而 driver 照写不误)。
  • 谓词路径:把调用方的 where 与该对象当前被锁的记录求交({ $and: [where, { id: { $in: lockedIds } }] })。方向是反过来的,这正是它便宜的原因 —— 上界落在对象上的 pending 审批数,而不是更新的匹配集:5 万行的无锁批量更新只多一次 bookkeeping 查询,照常放行。
  • 完全无谓词的整表 multi 更新碰到该对象的每一条被锁记录,只要有一条被锁就拒绝。
  • 失败关闭:被锁记录超过 1000 条(与 MULTI_DELETE_AUTH_LIMIT / MULTI_WRITE_AUTH_LIMIT 同一上界),或求交查询本身失败 —— 都拒绝写入,因为锁无法证明这次写入避开了被锁行。
  • 唯一保留的 fail-open 是「审批表整个读不出来」:这个 hook 对所有对象全局生效,若在此失败关闭,一个没有 sys_approval_request 的 kernel 会拒绝掉部署里的每一次更新。这一条是既有行为,注释里写明了为什么它与前面几条不同(攻击者无法操纵它)。
  • bookkeeping 与匹配集解析都用 system context 读:守卫自己的输入不能被调用方的可见性收窄 —— 读不到的被锁行,仍然是不许写的行。

豁免逐条随守卫一起搬过去

把守卫扩到多行最常见的错误是只搬拒绝逻辑,于是从 fail open 直接翻成误伤。五条既有豁免在多行路径上都逐一有测试(且在 $in 与非 id 谓词两种形状上各测一遍):

豁免 多行路径
isSystem(引擎自写 / 状态镜像) ✅
admin 覆盖 ✅
approvalStatusField 镜像写 ✅(只改镜像字段才放行,多带一个字段仍然拒绝)
lockRecord: false ✅
flowRunId 同源写(#3456 / #3712) ✅(别的 run 仍然拒绝)

测试

  • approval-service.test.ts —— 在现有 record-lock hook (node era) 用例旁新增 record-lock hook — predicate (multi) updates (#4778):拒绝、不误伤(谓词匹配不到被锁行时放行)、上界失败关闭、求交查询失败时失败关闭、无 pending 时完全不扫描业务表(用 _finds 钉住),以及上表五条豁免 × 两种谓词形状。
  • 新增 record-lock-multi-update.integration.test.ts —— 真实 ObjectQL 引擎 + 支持 $in / $and / updateMany 的内存 driver,由引擎自己决定走 by-id 还是 updateMany 并构造 hook context(即产生这个 bug 的那一跳,不 stub),复现 issue 里的三行。
  • 回归证明:把 if (!id) return 临时放回去,这两个文件里 14 条用例失败;恢复后 pnpm --filter @objectstack/plugin-approvals test 402 passed / 18 files,typecheck 干净,eslint 干净。

范围

改动只在 packages/plugins/plugin-approvals/src/ 与一个 changeset(patch:没有新增包根导出)。未改 packages/objectql/src/engine.ts —— engine 的 id 提取规则是刻意的,这次修的是消费方对「解析不到行」的错误推理。

顺带发现(未在本 PR 处理)

session.roles 在 ObjectQL 的 buildSession() 里从未被填充,所以记录锁与 delegation 守卫里的 roles.includes('admin') 在真实引擎路径上大概率永不生效,而且与 ADR-0095 / isOverrideActor 的 permissions/positions/posture 词汇不一致。两处都是失败关闭(admin 被误拒,不是越权),因此按 PD #10 单独立 issue,不在本 PR 扩范围。

🤖 Generated with Claude Code

https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny

Generated by Claude Code

zhuangjianguo and others added 2 commits August 3, 2026 09:37
@zhuangjianguo @claude
fix(approvals): hold the record lock for predicate (multi) updates (#…
677eeb0 
…4778)

The ADR-0019 record lock only ran for updates carrying an `input.id`, which the
engine extracts from a SCALAR `where.id` alone. Any other predicate is a
multi-row write that routes to `updateMany` and reached the hook with no id, so
`if (!id) return` read "no row was resolved" as "there is nothing to authorize"
when the truth was "nothing was ever queried" — the #4757 / #4630 fail-open
shape, here reachable with no privilege at all: rewriting the same edit as
`multi: true` bypassed the lock without admin, `isSystem`, `lockRecord: false`
or a whitelisted field.

The hook now resolves the rows a write touches before deciding. By-id writes are
unchanged. A predicate write is decided by intersecting the caller's predicate
with the object's LOCKED records, so the query is bounded by pending approvals
rather than by the update's match set; an unscoped whole-table `multi` update
reaches every locked row and is refused while any is held. Past 1 000 locked
records, or if the intersection query fails, the write fails closed.

Every exemption moves with the guard — `isSystem`, admin, the
`approvalStatusField` mirror, `lockRecord: false` and the owning run's
`flowRunId` (#3456 / #3712) — each pinned on both predicate shapes, plus a
real-engine integration test that reproduces the issue's three lines.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny
@claude
Merge remote-tracking branch 'origin/main' into claude/issue-4778-app…
63a6167 
…roval-lock-multi-update
@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 3, 2026 9:39am

Request Review

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling size/l labels Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-approvals.

5 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/automation/approvals.mdx (via @objectstack/plugin-approvals)
  • content/docs/kernel/services-checklist.mdx (via @objectstack/plugin-approvals)
  • content/docs/plugins/packages.mdx (via @objectstack/plugin-approvals)
  • content/docs/releases/implementation-status.mdx (via @objectstack/plugin-approvals)
  • content/docs/releases/v9.mdx (via @objectstack/plugin-approvals)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

This was referenced Aug 3, 2026
Open
Closed
@os-zhuang
os-zhuang marked this pull request as ready for review August 3, 2026 10:07
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 3, 2026
@os-zhuang Claude

os-zhuang commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

复核通过 —— ACCEPT,已标 ready 并送合并队列

逐条核过,记录几点判断依据:

1. 修复位置对。 守卫不是在六个调用点各补一遍,而是把"这次写入会碰到哪些行"的解析放进 hook 本身,与 #4757(sys_attachment)/ #4630(sys_comment)同源。if (!id) return 把「没解析出行」读成「没有东西需要授权」,而真相是「压根没查过」——这个判断在注释里写清楚了,不是靠猜。

2. 复杂度的方向选对了,这是本 PR 最容易做错而没做错的地方。 上界落在该对象上待审批的记录数(正常为 0,一次查询就返回),而不是更新的匹配集。所以 5 万行无锁的批量更新只多一次 bookkeeping 查询、照常放行。若把上界放在匹配集上,这个守卫会把每一次大批量更新都变成灾难。

3. 五条豁免在多行路径上确实各有测试。 我第一次核查时用 ^\+\s*it\( 去 grep,漏掉了 it.each(SHAPES)('allows an admin override via %s') 这条,一度以为 admin 那条缺测。是我查法不对,更正在此。五条齐:isSystem / admin / approvalStatusField 镜像写 / lockRecord: false / flowRunId 同源写。派发时我强调过「扩守卫最常见的错误是只搬拒绝逻辑、不搬豁免逻辑,从 fail open 直接翻到误伤合法路径」——这一条守住了。

4. 唯一保留的 fail-open 是被论证过的,不是遗漏。 pendingRequestsForObject 读不到 bookkeeping 时当作「无锁」。理由站得住:这个 hook 对每个对象全局生效,若在此 fail closed,一个 sys_approval_request 缺表或瞬时不可读的 kernel 会拒绝整个部署的所有更新。而攻击者真正能操纵的那些决策(自选谓词、超上界、交集查询失败)全部 fail closed。这个取舍写进了代码注释,符合"静默降级必须响亮"的要求。

5. changeset patch 正确。 我查了仓内同族先例:#4757(附件未限定多删)、#3456 / #3712(审批锁的两次修复)、#4728(DDL 响亮化)全部是 patch。lifecycle-hooks 不在包根 index.ts 导出,无新增公共 API,与既有做法一致。

6. 反向验证做得对。 把 if (!id) return 临时放回去后 14 failed | 258 passed,恢复后全绿——证明这批测试确实钉住了缺陷,而不是恰好能过。

范围外发现已妥善处理

#4839(admin 豁免读 session.roles,而 buildSession() 从不填充该字段,故记录锁与 delegation 守卫的 admin 覆盖在真实引擎路径上永不生效)没有在本 PR 里顺手改——这是正确的。它涉及权限放宽或 spec 字段退役,需维护者裁定,混进一个安全修复里会让两件事互相绑架。

值得指出它与 #4837 是同一个形态:声明了、消费端也有读取代码、gate 全绿,但没有任何生产者传值。#4837 是 Seed.env(六个调用点都不传),#4839 是 session.roles(全仓仅两处读、无一处写)。这已经是今天第三次遇到这个形状了。

⚠️ 若合并队列把本 PR 踢出,大概率是 #4796 那条 spec flaky(今晚已 5 次命中无关 PR),止血单 #4850 在飞。届时原样重投,不必返工。

Generated by Claude Code

Merged via the queue into main with commit 0b795da Aug 3, 2026
21 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-4778-approval-lock-multi-update branch August 3, 2026 10:19
This was referenced Aug 3, 2026
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

审批记录锁 bindApprovalLockHook 对谓词式(multi)更新完全失效:if (!id) return 把「没解析到行」当成「允许」

3 participants

@os-zhuang @zhuangjianguo @claude