fix(approvals)!: 审批请求按参与者可见,而不是整个租户可见 (#3590)#3599
Merged
Conversation
…he tenant (#3590) getRequest / listRequests / countRequests deliberately query with SYSTEM_CTX to bypass RLS — as their own comments say, the approver-visibility rule spans identity forms RLS cannot model cleanly, so it has to be expressed in the service. Only the TENANT half of that rule was ever written. The comment names the participant half; the code never applied it. So any authenticated user could read any approval request in their tenant: its payload snapshot, its full decision history, and — once decision attachments derived their access from the request (#3580) — its files. `approverId` on listRequests is a FILTER, not authorization. Omitting it returned the whole tenant. A caller now sees a request when they participate in it: the submitter, a current approver (via the normalized approver index, so every identity form the write path recorded is covered), or someone who already acted on it — a past approver whose slot has moved on, a commenter. Override admins keep the unrestricted view the "all requests" console surface depends on; a tokenless context sees nothing. Keying on the concrete user id is sufficient rather than an approximation, and that is worth stating because under-granting here would silently hide approvals someone must act on: position/team/manager/field approvers are resolved to concrete user ids at open time, and the `type:value` literal is only the fallback for a spec that resolved to NOBODY — a slot no one can act on either way, since can_act is a plain membership test over the resolved ids. A write path's own result is NOT re-gated. Every operation echoes back the request it just changed via getRequest; the operation authorized itself before writing, and re-asking answers a settled question — wrongly, for a context that carries no userId (a flow-driven resume, a service-to-service call), turning a successful write into a null result. Four existing tests caught exactly that, which is why the echo now goes through readBackRequest instead. Verified in the browser on app-showcase: a non-admin approver's unfiltered list drops from 3 requests to the 1 she is on, an admin still sees all 3, her decision attachment still downloads (200, real PDF bytes — #3580 composes), and a request she has nothing to do with reads back empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 4 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
…ant-scoped The drift check flagged approvals.mdx, and rightly: it documented `approverId` as one filter among several and never said who may see what — so a reader would reasonably assume omitting it returns the whole tenant, which is exactly what this PR stops being true. Also records that a decision's attachments inherit the same rule, since that is the non-obvious half: the file is as readable as the decision, never more. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd
os-zhuang
marked this pull request as ready for review
July 27, 2026 09:27
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3590。
问题
getRequest/listRequests/countRequests故意用SYSTEM_CTX查询以绕过 RLS——代码自己的注释说明了原因:但绕过 RLS 之后,只实现了「租户」那一半。注释点名的「审批人可见性」那一半从来没有被写出来。
后果:同租户内任何登录用户可以读取任意审批请求——它的 payload 快照、完整决策历史,以及在 #3580 让决策附件从请求派生访问权之后,它的文件。
而
listRequests上的approverId是过滤器,不是授权:不传它就返回整个租户。修法
新增参与者判定:提交人 / 当前审批人(走归一化审批人索引,覆盖写路径记录的每种身份形态)/ 已经在其上行动过的人(槽位已流转的历史审批人、评论者)。具备 override 权限的管理员保留不受限视图(「所有请求」控制台视图依赖它);无 token 的上下文什么都看不到。
为什么用具体 user id 就够,而不是近似? 这点必须说清楚,因为这里授权不足会静默隐藏别人必须处理的审批:
type:value字面量只是解析到「无人」时的兜底——那种槽位任何人都无法行动(can_act就是对已解析 id 的朴素成员测试)所以这条规则不可能把请求从真正能处理它的人眼前藏起来。
每个操作都会通过
getRequest回读它刚改过的请求作为返回值。操作在写之前已经用自己的规则授过权,再问一次是在重复回答一个已定的问题——而且对不带userId的上下文(flow 驱动的 resume、服务间调用)会答错,把一次成功的写入变成null结果。4 个既有测试正好抓到了这个,所以回读改走
readBackRequest。这不是为了让测试变绿——是我第一版实现真的有这个缺陷。浏览器验证(app-showcase)
测试
approval-service.test.ts+8:提交人/审批人可读、同租户陌生人不可读、行动过的人在槽位流转后保留访问、override 管理员不受限、无 token fail closed、listRequests不带过滤器不再返回整租户、countRequests与列表一致、无userId的写路径仍能回显自己的结果。另外修正了两条断言旧(泄漏)行为的既有测试——其中
can_override那条改用提交人而不是陌生人,保留它原本要验的语义(权限有无,而不是访问有无)。全量:plugin-approvals 210 ✅ · service-storage 173 ✅ · spec 6733 ✅ · lint ✅ · nul-bytes ✅
破坏性
标
!:此前不带approverId列请求并期望拿到整个租户的客户端,现在只会拿到自己的——这正是修复的目的。🤖 Generated with Claude Code
https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd
Generated by Claude Code