approvals: `expandBusinessUnitUsers` reads `sys_business_unit_member` with no organization predicate, so a `department` approver on a seeded unit resolves across tenants

[os-sales]

Summary

ApprovalService.expandBusinessUnitUsers screens 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, a department:<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's BusinessUnitGraphService). 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/main 431979e67

packages/plugins/plugin-approvals/src/approval-service.ts:

• :1733-1735 — the unit screen, null-inclusive since approvals: a department approver never resolves when the business unit has organization_id = null (every seeded BU) #3807:

private businessUnitOrgScope(filter, organizationId) {
  if (!organizationId) return filter;
  return { ...filter, $or: [{ organization_id: organizationId }, { organization_id: null }] };
}

• :1740-1746 (seed check) and the descendant BFS both route through it — correct.
• :1771 — the member read, which does not:

rows = await this.engine.find('sys_business_unit_member', {
  where: { business_unit_id: { $in: Array.from(seen) } },
  fields: ['user_id'],
  limit: 10000,
  context: SYSTEM_CTX,
});

• :375 — SYSTEM_CTX is { isSystem: true, positions: [], permissions: [] }. It carries no tenantId, so the engine threads none and SqlDriver.applyTenantScope adds nothing. The member query is unscoped by organization.

Why the two facts compose into a leak

A sys_business_unit row written by seed data carries organization_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_member rows are organization-stamped on the REST/session write path (the engine threads the caller's tenantId and SqlDriver.injectTenantOnInsert fills 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_member and 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 strict organization_id equality (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 department approver 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-sharing and this is plugin-approvals: a separate package, separate tests and a separate gate surface.

Generated by Claude Code

[os-zhuang]

Triage: lands in packages/plugins/plugin-approvals/src/approval-service.ts ⇒ domain:services. Graded Bug · priority:p1 · security.

Bug by the mechanical boundary test: nothing widens; a tenant predicate that the sibling reads already apply is missing from one read.

Why p1 and security

This 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:

1. a seeded sys_business_unit row carries organization_id = NULL, so the same unit id exists identically in every tenant;
2. the unit screen is correctly null-inclusive (approvals: a department approver never resolves when the business unit has organization_id = null (every seeded BU) #3807), so tenant A's request resolves that seeded unit;
3. the member read at :1771 carries no organization predicate, under SYSTEM_CTX (:375) which has no tenantId, so SqlDriver.applyTenantScope adds nothing;
4. ⇒ every membership row hanging off the shared unit comes back, including tenant B's — and those rows exist, because sys_business_unit_member is 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_member and 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, p0 is defensible and this grade should move.

The repair shape is already settled next door

#14547 is the identical defect in plugin-sharing's BusinessUnitGraphService and 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 strict organization_id equality — 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 empty department approver 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 from origin/main 431979e67. Locate expandBusinessUnitUsers, businessUnitOrgScope and SYSTEM_CTX by symbol.

---

Generated by Claude Code

[claude]

Claimed and dispatched — pm:queue → pm:dispatched. Precondition re-measured against today's origin/main, ⛔ not inherited from the card.

Claim: domain:services execution seat, session session_01ARYe3yQTQCUFm5qPYNgKaJ
Branch: claude/issue-14946-approvals-member-org-predicate
Clause-②: no

⚠️ That no is 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 to yes and hangs needs:contract-review on PR and card in one stroke — ⛔ never before the PR exists.

The defect re-measured on origin/main 99a5bc674, with a firing control

The card cites 431979e67; main has moved, so the anchors were re-read rather than trusted.

what	reading
the unit screen is null-inclusive	businessUnitOrgScope present in approval-service.ts — the $or: [{organization_id}, {organization_id: null}] shape since #3807
the member read has no organization predicate	the sys_business_unit_member find filters on business_unit_id only
it runs under an elevated context carrying no tenant	SYSTEM_CTX in the same file

⇒ on a deployment whose org chart came from seed data (organization_id: null rows, which the unit screen deliberately admits), a department:<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 it p1 + security on 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's BusinessUnitGraphService). 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 and plugin-approvals' expansion have different callers and different elevated contexts. The dev measures whether the null-inclusive $or is 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