Skip to content

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

Description

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

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

Activity

  1. added theissue type on Sep 4, 2026
  2. os-zhuang commented on Sep 4, 2026

    @os-zhuang
    Contributor

    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

  3. claude commented on Sep 5, 2026

    @claude
    Contributor

    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

  4. zhuangjianguo commented on Sep 5, 2026

    @zhuangjianguo
    Collaborator

    os-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

  5. claude commented on Sep 5, 2026

    @claude
    Contributor

    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:services dispatching seat, session_01ARYe3yQTQCUFm5qPYNgKaJ. Clause-② re-declared no and verified, ⛔ not accepted on the seat's word: 0 new exports across the diff's plugin-approvals/src changes against a firing control of 348 added lines; the only new dist/index.d.ts declaration is private 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 with organization_id: null is 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_member declares no organization_id at all — the column is INJECTED (applySystemFields; injected-system-columns.ts TENANT_SCOPE_COLUMN; the tenancy census lists the object's reach in, tenantField organization_id);
    • only REST/session writes fill it. Seed replay withholds fallbackOrgId from sys_ objects, and system-context writes stamp nothing because the object is unclassified in PLATFORM_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:

    ⇒ ⛔ 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 no packages/qa/dogfood suite uses a department approver.

    ⚠️ The dependency this creates, and why it is written here rather than left in a report

    The 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_member is unadjudicated in PLATFORM_OBJECT_TENANCY, so seed-replayed rows are org-less」 — which is open and pm:blocked, at priority: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 p1 security fix leans on it.

    Evidence

    RED at 6fc3d1009 with approval-service.ts byte-identical to origin/main — 5 failed / 1 passed, and the probe printed the actual clause: where = {business_unit_id: {$in: [...]}} with no organization_id, expanding to ["u_a","u_b","u_a_child","u_b_child"]. GREEN at 16dbdf55b: 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 are PREREQUISITE 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.ts and its test: no in-flight holder. The two files that moved mechanically — content/docs/permissions/system-context.mdx (via check-system-context-census --fix after a +57-line docblock) and scripts/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

  6. claude commented on Sep 5, 2026

    @claude
    Contributor

    Green — PR #15912 flipped ready and armed. Closing the loop this seat opened at 5552366784.

    domain:services dispatching 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: clean
    Clause-② no, re-verified — 0 new exports across plugin-approvals/src against a firing control of 348 added lines. ⇒ ⛔ no contract review owed, ⛔ no gate to clear
    governed surface 0 of the diff matches a governed prefix
    ⛔ release-owned red line content/docs/releases/**: 0 (firing control: 1 file under content/docs/** — the instrument fires, so the zero is a reading)

    What lands

    ApprovalService.expandBusinessUnitUsers' read of sys_business_unit_member now 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 a department:<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 because sys_business_unit_member declares no organization_id — the column is injected, only REST/session writes fill it, seed replay withholds fallbackOrgId from sys_ 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_member is unadjudicated in PLATFORM_OBJECT_TENANCY, which is why org-less rows exist. It is open and pm:blocked at p2. 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 and onEmptyApprovers, ⛔ 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

  7. github-actions commented on Sep 6, 2026

    @github-actions
    Contributor

    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.

    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 schedule

    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions