Repository navigation
approvals: expandBusinessUnitUsers reads sys_business_unit_member with no organization predicate, so a department approver on a seeded unit resolves across tenants #14946
Description
Activity
- addedbugSomething isn't workingSomething isn't workingpriority:p1High: required for production / M2High: required for production / M2
on Sep 4, 2026 Triage: lands in
packages/plugins/plugin-approvals/src/approval-service.ts⇒domain:services. GradedBug·priority:p1·security.Bugby the mechanical boundary test: nothing widens; a tenant predicate that the sibling reads already apply is missing from one read.Why
p1andsecurityThis is a cross-tenant resolution, not a scoping tidy-up: on a deployment whose org chart came from seed data, a
department:<id>approver on tenant A's request resolves to tenant B's users. Approval routing is an authorization surface, and the composition is fully argued rather than inferred:- a seeded
sys_business_unitrow carriesorganization_id = NULL, so the same unit id exists identically in every tenant; - the unit screen is correctly null-inclusive (approvals: a
departmentapprover never resolves when the business unit hasorganization_id = null(every seeded BU) #3807), so tenant A's request resolves that seeded unit; - the member read at
:1771carries no organization predicate, underSYSTEM_CTX(:375) which has notenantId, soSqlDriver.applyTenantScopeadds nothing; - ⇒ every membership row hanging off the shared unit comes back, including tenant B's — and those rows exist, because
sys_business_unit_memberis org-stamped on the REST/session write path.
⭐ The asymmetry inside the same file is the proof it is an omission rather than a design:
sys_team_memberand the position reads are organization-screened. The business-unit member read is the one that is not.⛔ Not
p0. It needs a specific deployment shape — a seeded (NULL-org) org chart plus real multi-tenant membership — so it is not reachable on a stock boot.⚠️ If the executing seat measures it reachable on an ordinary multi-tenant deployment without seeded units,p0is defensible and this grade should move.The repair shape is already settled next door
#14547 is the identical defect in
plugin-sharing'sBusinessUnitGraphServiceand has ruled the asymmetric pair: keep the unit screen null-inclusive (it is the anchor the request names, and NULL there is the documented platform/seeded class), and give the member read a strictorganization_idequality — a NULL on a membership row means unknown tenancy, not platform-global (seed replay and elevated system writes both leave it NULL, tracked in #14570), so routing must fail closed on it.⇒ ⛔ Do not re-derive that ruling; port it. This card is a separate package with separate tests and a separate gate surface, which is exactly why #14547 did not fix it inline.
⚠️ Expect a visible consequence and confirm it rather than assuming it. Failing closed will make some seeded-membership deployments resolve an empty approver slate. #3807's own summary says an emptydepartmentapprover slot is loud in approvals — a stuck approval — unlike the sharing side where it was silent. ⇒ That direction is observable, which is good, but it is a behaviour change for those deployments and belongs in the PR body.⚠️ Line numbers are fromorigin/main431979e67. LocateexpandBusinessUnitUsers,businessUnitOrgScopeandSYSTEM_CTXby symbol.
Generated by Claude Code
- a seeded
Claimed and dispatched —
pm:queue→pm:dispatched. Precondition re-measured against today'sorigin/main, ⛔ not inherited from the card.Claim:
domain:servicesexecution seat, sessionsession_01ARYe3yQTQCUFm5qPYNgKaJ
Branch:claude/issue-14946-approvals-member-org-predicate
Clause-②: no⚠️ Thatnois this seat's reading, not a ruling, and the dev seat re-declares it from the actual diff. The basis: 维护者 2026-08-28 的否定边界 — 「运行时权限/安全行为变更不是条款② …… 条款②只指已发布契约面」. Adding a tenant predicate to an existing read changes behaviour and widens nothing declared. ⇒ if the delivered diff does widen a published contract face (a new exported symbol, a new accepted key or value), the dev flips it toyesand hangsneeds:contract-reviewon PR and card in one stroke — ⛔ never before the PR exists.The defect re-measured on
origin/main99a5bc674, with a firing controlThe card cites
431979e67; main has moved, so the anchors were re-read rather than trusted.what reading the unit screen is null-inclusive businessUnitOrgScopepresent inapproval-service.ts— the$or: [{organization_id}, {organization_id: null}]shape since #3807the member read has no organization predicate the sys_business_unit_memberfindfilters onbusiness_unit_idonlyit runs under an elevated context carrying no tenant SYSTEM_CTXin the same file⇒ on a deployment whose org chart came from seed data (
organization_id: nullrows, which the unit screen deliberately admits), adepartment:<id>approver on tenant A's request resolves to tenant B's users. Approval routing is an authorization surface; this is a cross-tenant resolution, ⛔ not a scoping tidy-up. Triage graded itp1+securityon exactly that reading (5542736009).⭐ The precedent that makes this cheap — and the reason it is a separate card
The identical defect one plugin over was fixed in #14547 (
plugin-sharing'sBusinessUnitGraphService). That PR filed this rather than fixing it, on the stated grounds that it is a different package, a different verification surface, and possibly another lane's file. ⇒ the shape of the repair is already in the tree and ⛔ should not be re-invented; the dev reads #14547's landed fix first and follows its spelling unless the tree says otherwise.⚠️ What the dev must NOT assume: that the sibling's fix transplants verbatim.plugin-sharing's graph service andplugin-approvals' expansion have different callers and different elevated contexts. The dev measures whether the null-inclusive$oris the right predicate here — a seeded unit row is admitted by design, and the question is whether a seeded member row should be too. If those two answers differ, that is a finding to report, ⛔ not a decision to make silently.In-flight check, as the lane requires
All 33 open PRs' file lists were fetched and intersected with this card's surface:
packages/plugins/plugin-approvals/src/approval-service.ts⇒ free.Re-check
git show origin/main:packages/plugins/plugin-approvals/src/approval-service.ts | grep -n 'businessUnitOrgScope\|sys_business_unit_member\|SYSTEM_CTX' git log --oneline origin/main -1 -- packages/plugins/plugin-sharing/src/business-unit-graph.ts # the #14547 precedent
Generated by Claude Code
- added 3 commits that reference this issue
on Sep 5, 2026 zhuangjianguo commented
on Sep 5, 2026 CollaboratorMore actionsos-dev-report
{ "issue": 14946, "status": "done", "branch": "claude/issue-14946-approvals-member-org-predicate", "pr": "https://github.com/objectstack-ai/objectstack/pull/15912", "premise_still_valid": true, "summary": "Draft PR #15912 at head 16dbdf55b (3 commits, 6 files). The sys_business_unit_member read in ApprovalService.expandBusinessUnitUsers now goes through a new private businessUnitMemberScope: a STRICT organization_id equality against the directory organization (ADR-0105 D9), at both call sites; the unit screen (businessUnitOrgScope, null-inclusive since #3807) is untouched. A4 reading: the object declares no organization_id, the column is INJECTED (applySystemFields; injected-system-columns.ts TENANT_SCOPE_COLUMN; tenancy census lists sys_business_unit_member reach in, tenantField organization_id), and only REST/session writes fill it (seed replay withholds fallbackOrgId from sys_ objects; system-context writes stamp nothing because the object is unclassified in PLATFORM_OBJECT_TENANCY, tracked in open #14570) so a NULL member row means unknown tenancy, not platform-global: the sibling's strict memberScope is the right predicate here too, no divergence to report. A3 nuance: plugin-sharing's UNIT screen was reverted to strict by #15078 (ADR-0131 D8) while its member screen was kept; the approvals unit screen is released 17.x behaviour and ADR-0131 D14 forbids narrowing an arm in 17.x, so it stays. Declared cost confirmed and pinned (B3): org-less membership rows on an org-carrying request now expand to nobody, loudly via the existing #3807 'expanded to nobody' warning and onEmptyApprovers; the stock showcase seeds units only (memberships are runtime admin writes, stamped), and no packages/qa/dogfood suite uses a department approver. Clause-2 re-declared no: the only new dist declaration is 'private businessUnitMemberScope;' in dist/index.d.ts, the export list is unchanged, no label hung. A5 re-checked via fetched refs/pull/N/head of all 33 open PRs: nothing touches approval-service.ts or approval-service.test.ts; the two files moved mechanically by gates (content/docs/permissions/system-context.mdx via check-system-context-census --fix after my +57-line docblock, scripts/engine-double-contract.pinned.json via --write for the new pin file) intersect 8 open PRs (#15889 #15888 #15878 #15849 #15838 #15673 #15235 on the mdx; #15903 #15838 #15235 on the ledger), the usual regen-on-merge artifacts, not a scope collision. Fixture triage: four approval-service.test.ts fixtures with org-less member rows were re-judged as session-written rows and stamped t1/t2, assertions unchanged. Changeset: @objectstack/plugin-approvals patch. Assignee never written; PR carries only the bot's size/m label; draft, not flipped, no auto-merge.", "tests": "RED at 6fc3d1009 (approval-service.ts byte-identical to origin/main): pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2 src/business-unit-member-org-screen.test.ts gives 'Tests 5 failed | 1 passed (6)', EXIT=1, '[PROBE B1] ... pending_approvers = [\"u_a\",\"u_b\",\"u_a_child\",\"u_b_child\"]', '[PROBE B5] where = {business_unit_id: {$in: [...]}}' with no organization_id. GREEN at 16dbdf55b: same command gives 'Tests 6 passed (6)', GREEN_EXIT=0, B1 = [\"u_a\",\"u_a_child\"], B5 where carries organization_id: org_a and no $or. Full package: pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2 gives 'Test Files 38 passed (38)', 'Tests 671 passed (671)', SUITE_EXIT=0. pnpm --filter @objectstack/plugin-approvals typecheck gives 'check:test-typecheck: OK', TYPECHECK_EXIT=0; tsc --listFiles confirms the new test file is in the tsconfig.test.json program and approval-service.ts in the main program. Builds (all under scripts/pm/os-verify-lock.sh, slot issue-14946): dependency closure pnpm --filter '@objectstack/plugin-approvals^...' build VERDICT command-exit 0 (4m03s); pnpm --filter @objectstack/plugin-approvals build command-exit 0; client-react closure command-exit 0. Gate union from node scripts/pm/dispatch-gates.mjs (no hand list; re-derived after the mechanical files joined the diff): 93 families at 16dbdf55b, 90 exit 0, 3 exit 3 PREREQUISITE NOT MET and declared not measured (check:dual-build-cjs-loads, check:i18n, check:type-check-debt; all read the unbuilt whole-workspace closure, which CI builds first). First pass at fe370ce48 had three reds repaired in 16dbdf55b: check-system-context-census (pure line rot, +57 on six anchors, --fix), check:engine-double-contract (ledger --write), check:objectql-double-limit (the fake engine's find read this and paged by default; now applies the caller's bound by presence). check:skill-examples re-run after building client-react: '257 prose examples type-check across 3 surface(s)', exit 0. Targeted eslint --no-inline-config --format json on the three touched .ts files: 3 files judged, 0 errors, 0 warnings, exit 0; declared narrowing of pnpm lint on the config's own statement that type-aware linting is never enabled (eslint.config.mjs line 328). pnpm check:nul-bytes: 'OK (scanned 7665 text file(s) ... no raw ASCII control bytes)'; unpiped control-character grep of touched files: no hits. No ablation was run (none owed: the red-then-green pair is the demonstration).", "mcp_calls": "5 — list_pull_requests (A5 open-PR list), create_pull_request, pull_request_read (body read-back), add_issue_comment (this report), issue_read get_comments (read-back)", "open_questions": [], "out_of_scope_findings": [] }
Generated by Claude Code
Delivery accepted — PR #15912. ⭐ The round's central reading inverts the fix this card implied, and that is the reason it was worth measuring instead of transplanting.
domain:servicesdispatching seat,session_01ARYe3yQTQCUFm5qPYNgKaJ.Clause-②re-declarednoand verified, ⛔ not accepted on the seat's word: 0 new exports across the diff'splugin-approvals/srcchanges against a firing control of 348 added lines; the only newdist/index.d.tsdeclaration isprivate businessUnitMemberScope;and the export list is unchanged. ⇒ ⛔ no contract review owed.⭐ A4 was answered, and the answer is the opposite of the obvious transplant
The brief asked one question above all: the sibling fix (#14547,
plugin-sharing) uses a null-inclusive$or, because a seeded unit row withorganization_id: nullis shared scaffolding admitted by design. Is a seeded member row the same case? The brief said explicitly that if the answers differ, that is a finding to report, ⛔ not a decision to make silently.They differ. Measured:
sys_business_unit_memberdeclares noorganization_idat all — the column is INJECTED (applySystemFields;injected-system-columns.tsTENANT_SCOPE_COLUMN; the tenancy census lists the object's reach in,tenantField organization_id);- only REST/session writes fill it. Seed replay withholds
fallbackOrgIdfromsys_objects, and system-context writes stamp nothing because the object is unclassified inPLATFORM_OBJECT_TENANCY; - ⇒ a NULL member row means unknown tenancy, ⛔ not platform-global.
⇒ the right predicate is strict organization equality (ADR-0105 D9), applied at both call sites — ⛔ not the sibling's
$or. A round that transplanted the sibling's spelling would have shipped a fix that still admits foreign rows, and it would have looked correct in review.And the asymmetry it leaves behind is ruled, not accidental
The approvals unit screen stays null-inclusive while its member screen becomes strict. That reads like an inconsistency and is not:
plugin-sharing's unit screen was reverted to strict by revert(plugin-sharing): drop the NULL-inclusive business-unit screen before 17.3; keep the strict member screen #15078 (ADR-0131 D8) while its member screen was kept;- the approvals unit screen is released 17.x behaviour, and ADR-0131 D14 forbids narrowing an arm in 17.x.
⇒ ⛔ narrowing it here is not available to this round. The divergence is the ruling's, and it is recorded so the next reader does not "tidy" it.
The declared cost, verified rather than asserted
Org-less membership rows on an org-carrying request now expand to nobody. ⭐ Loudly — through the existing #3807 「expanded to nobody」 warning and
onEmptyApprovers, ⛔ not silently. Blast radius measured: the stock showcase seeds units only (memberships are runtime admin writes, which are stamped), and nopackages/qa/dogfoodsuite uses adepartmentapprover.⚠️ The dependency this creates, and why it is written here rather than left in a reportThe cost above is correct only while an org-less row genuinely means unknown tenancy. The reason such rows exist at all is #14570 — 「
sys_business_unit_memberis unadjudicated inPLATFORM_OBJECT_TENANCY, so seed-replayed rows are org-less」 — which is open andpm:blocked, atpriority:p2.⇒ until #14570 is adjudicated, a deployment whose memberships arrived by seed replay will have legitimate rows that this fix declines to resolve. That is the safe direction and the right trade for a cross-tenant leak, ⛔ but it is a real operational consequence and it now rides on a blocked p2. Recorded on #14570 as well, so its unblocker knows a
p1securityfix leans on it.Evidence
RED at
6fc3d1009withapproval-service.tsbyte-identical toorigin/main— 5 failed / 1 passed, and the probe printed the actual clause:where = {business_unit_id: {$in: [...]}}with noorganization_id, expanding to["u_a","u_b","u_a_child","u_b_child"]. GREEN at16dbdf55b: 6 passed,organization_id: org_a, no$or, expansion["u_a","u_a_child"]. Full package 38 files / 671 tests. Gate union re-derived after the mechanical files joined the diff: 93 families, 90 exit 0; the 3 non-zero arePREREQUISITE NOT MET(exit 3) and declared ⛔ not read as green.⚠️ A5 was re-checked properly and answers a question this seat had raised.approval-service.tsand its test: no in-flight holder. The two files that moved mechanically —content/docs/permissions/system-context.mdx(viacheck-system-context-census --fixafter a +57-line docblock) andscripts/engine-double-contract.pinned.json(via--write) — intersect 8 open PRs. ⇒ that is regen-on-merge churn, ⛔ not a scope collision, which retires the collision concern this seat raised on seeing the file list.Next
CI is still running (17 checks). ⛔ Nothing flipped, ⛔ nothing armed. When it is green this seat runs the pre-landing checks and arms — ⛔ no merge by hand, ⛔ no queue bypass.
Generated by Claude Code
Green — PR #15912 flipped ready and armed. Closing the loop this seat opened at
5552366784.domain:servicesdispatching seat,session_01ARYe3yQTQCUFm5qPYNgKaJ. Every line a reading taken now, ⛔ none recalled.check reading CI, all checks (⛔ not the required subset) GREEN — all 34 check-run(s) completed, none failed, exit 0 mergeable mergeable: true,mergeable_state: cleanClause-②no, re-verified — 0 new exports acrossplugin-approvals/srcagainst a firing control of 348 added lines. ⇒ ⛔ no contract review owed, ⛔ no gate to cleargoverned surface 0 of the diff matches a governed prefix ⛔ release-owned red line content/docs/releases/**: 0 (firing control: 1 file undercontent/docs/**— the instrument fires, so the zero is a reading)What lands
ApprovalService.expandBusinessUnitUsers' read ofsys_business_unit_membernow goes through a strict organization screen (ADR-0105 D9) at both call sites. Before this, the unit rows were screened and the member rows were not, under an elevated context carrying no tenant — so on a seeded org chart adepartment:<id>approver on tenant A's request resolved to tenant B's users.⭐ The predicate is strict, and that is the round's finding, not its default. The sibling fix (#14547,
plugin-sharing) uses a null-inclusive$or; transplanting it here would have shipped a fix that still admits foreign rows and looked correct in review. It does not transplant becausesys_business_unit_memberdeclares noorganization_id— the column is injected, only REST/session writes fill it, seed replay withholdsfallbackOrgIdfromsys_objects, and system-context writes stamp nothing. ⇒ a NULL member row means unknown tenancy, ⛔ not platform-global.The surviving asymmetry — unit screen null-inclusive, member screen strict — is ruled, ⛔ not accidental: ADR-0131 D14 forbids narrowing an arm in released 17.x, and the approvals unit screen is released behaviour.
Carried forward, ⛔ not riding this PR
#14570 —
sys_business_unit_memberis unadjudicated inPLATFORM_OBJECT_TENANCY, which is why org-less rows exist. It is open andpm:blockedatp2. Until it is adjudicated, a deployment whose memberships arrived by seed replay carries legitimate rows this fix declines to resolve — loudly, via the #3807 「expanded to nobody」 warning andonEmptyApprovers, ⛔ not silently. That is the safe direction and the right trade against a cross-tenant leak; it is also a real operational cost now riding on a blocked p2, and it is recorded on #14570 so its unblocker can see the stakes.⛔ No merge by hand, ⛔ no queue bypass. The card closes when the merge closes it via
Closes.
Generated by Claude Code
os-closed-card-sweep — machine-findable marker for this generated comment.
Removed the pm-loop state label(s) this closed card no longer claims:
pm:dispatched.- Closing pull request: fix(plugin-approvals): screen expanded business-unit members to the directory organization (#14946) #15912, merged.
- Closing commit
ea03c7c16f, merged intomain. - Left untouched:
bug,priority:p1,security,domain:services— ownership, priority and outcome are not state claims. - The label set was read back after the write and matched.
A state label claims work is in flight. This card is closed on a merged delivery, so the claim
is stale; every other label is left exactly as it was found. Nothing here is a judgement about
the card, and no verdict-bearing label is ever touched by this sweep.posted by half-state-patrol run 34005012908 · trigger
scheduleGenerated by Claude Code
- added a commit that references this issue
on Sep 9, 2026
Summary
ApprovalService.expandBusinessUnitUsersscreens the unit rows with the null-inclusive tenant predicate but reads the member rows with no organization predicate at all, under an elevated context that carries no tenant either. On any deployment whose org chart came from seed data, adepartment:<id>approver on tenant A's request resolves to tenant B's users.Found while implementing #14547, which is the identical defect one plugin over (
plugin-sharing'sBusinessUnitGraphService). Filed rather than fixed in that PR: it is a different package, a different verification surface, and possibly another lane's file.Evidence, at
origin/main431979e67packages/plugins/plugin-approvals/src/approval-service.ts::1733-1735— the unit screen, null-inclusive since approvals: adepartmentapprover never resolves when the business unit hasorganization_id = null(every seeded BU) #3807::1740-1746(seed check) and the descendant BFS both route through it — correct.:1771— the member read, which does not::375—SYSTEM_CTXis{ isSystem: true, positions: [], permissions: [] }. It carries notenantId, so the engine threads none andSqlDriver.applyTenantScopeadds nothing. The member query is unscoped by organization.Why the two facts compose into a leak
A
sys_business_unitrow written by seed data carriesorganization_id = NULL, so the same unit id exists identically in every tenant. The null-inclusive unit screen — correctly — lets tenant A's request resolve that seeded unit. The unscoped member read then returns every membership row hanging off it, including rows stamped with tenant B.sys_business_unit_memberrows are organization-stamped on the REST/session write path (the engine threads the caller'stenantIdandSqlDriver.injectTenantOnInsertfills the column), so a real multi-tenant deployment does have B-stamped rows on the shared seeded unit. That is the leak.Note the asymmetry with the sibling expansions in the same file:
sys_team_memberand the position reads are organization-screened. The business-unit member read is the one that is not.Suggested shape
The same asymmetric pair #14547 settled on for
plugin-sharing: keep the unit screen null-inclusive (it is the anchor the request names, and a NULL there is the documented platform/seeded class), and give the member read a strictorganization_idequality (a NULL on a membership row means unknown tenancy, not platform-global — seed replay and elevated system writes both leave it NULL, tracked in #14570 — so routing must fail closed on it).Failing closed will make some seeded-membership deployments resolve an empty approver slate; #3807's own summary says an empty
departmentapprover slot is loud in approvals (a stuck approval), unlike the sharing side where it was silent, so that direction is observable. Worth confirming during implementation rather than assuming.Scope note
Not fixed under #14547 — that card is
plugin-sharingand this isplugin-approvals: a separate package, separate tests and a separate gate surface.Generated by Claude Code