fix(approvals): refuse cross-org targeting on directory-less approver types (ADR-0105 D9) - #3873
Merged
Merged
Conversation
… types `user`, `field` and `manager` return EARLY in `resolveApproverSpec` — they name a person outright instead of expanding a directory. D9's org resolution (#3824) sat AFTER those returns, so an `organization` declared on one of them never reached the check and was silently INERT. That is precisely the behaviour ADR-0105 D9 rules out and the authoring docs promise against ("`organization` on those is refused at runtime"). The `os lint` rule caught it at author time, but the runtime claim was false — and a flow stored before the lint existed, or assembled programmatically, got no signal. Resolution moves to the top of `resolveApproverSpec`, above every early return, so the refusal reaches all three types. The ordinary path is unchanged and still free: with no `organization` declared the resolver returns the request's organization without reading anything. Why the existing tests missed it: `approver-org-scope.test.ts` calls the resolver DIRECTLY, so it never traverses the early return, and the service-level integration test only covered `position`. Both gaps are closed — the new case opens a real request for each of the three directory-less types and asserts the refusal, plus one that a directory-less approver with NO declaration is untouched, so the guard cannot start costing the ordinary case. Found by cloud's group-posture dogfood on a real `group` boot. The two links that dogfood was written to check both hold: a cross-organization approver does reach her queue through the D2 union wall, and `decide()` does accept her. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015FebXPaaGrLhGKw1LHPbpL
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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:
|
os-zhuang
marked this pull request as ready for review
July 28, 2026 13:30
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
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.
Follow-up to #3824 (ADR-0105 D9). Found by cloud's group-posture dogfood, not by review.
The defect
user,fieldandmanagerreturn early inresolveApproverSpec— they name a person outright rather than expanding a directory:I put D9's resolution after those returns. So
{ type: 'user', value: 'u1', organization: '$root' }never reached the check and was silently inert.That is exactly the behaviour ADR-0105 D9 rules out, and it contradicts two things #3824 shipped:
content/docs/automation/approvals.mdx— "organizationon those is refused at runtime and flagged byos lint".The lint half was true. The runtime half was not — and the lint only helps at author time: a flow stored before the rule existed, or one assembled programmatically, got no signal at all.
The fix
Resolution moves to the top of
resolveApproverSpec, above every early return. One line moved, one comment explaining why it must stay there.The ordinary path is untouched and still free: with no
organizationdeclared the resolver returns the request's organization without reading anything — there's now a test pinning that too, so the guard can't start costing the common case.Why the existing tests missed it
Both gaps are worth naming, because they're the reusable lesson:
approver-org-scope.test.tscallsresolveApproverDirectoryOrgdirectly. A unit test of a helper cannot see that its caller returns before invoking it.approver-cross-org.integration.test.tswent through the service, but only ever withtype: 'position'— an org-scoped type, which reaches the resolution fine.The new case opens a real request for each of the three directory-less types and asserts the refusal, so the early-return path is covered by construction rather than by remembering to.
What the dogfood confirmed while finding this
The cloud e2e (cloud PR to follow) was written specifically to check the two links D9's unit tests structurally cannot — and both hold:
decide()really does accept a foreign-org actor, and the request finalises.So D9's core is sound end to end; this is the refusal path only.
Verification
Docs and ADR need no change — they already describe the behaviour this restores.
🤖 Generated with Claude Code
https://claude.ai/code/session_015FebXPaaGrLhGKw1LHPbpL
Generated by Claude Code