Sharing rules with a business-unit recipient silently grant nothing when the unit row has organization_id = NULL

[baozhoutao]

Summary

BusinessUnitGraphService.orgScope screens sys_business_unit with a strict organization_id equality. A sharing rule always carries the caller's organization (the engine stamps it, and an explicit organization_id: null in the payload is overridden), while a business unit created by seed data carries organization_id = NULL. The two never match, so expandUsers / expandUnitMembers return zero users — the rule is accepted, stays active: true, materialises no sys_record_share rows, and logs nothing.

This is the "second, worse copy" that sharing-rule-service.ts already warns about in criteriaContext:

Open-coding an organization_id equality into filter would be a second, worse copy — ... and it would drop the NULL arm that keeps platform-seeded rows visible to every tenant (#2734).

SqlDriver.applyTenantScope emits (organization_id = ? OR organization_id IS NULL). business-unit-graph.ts does not:

private orgScope(filter: Record): Record {
  if (this.organizationId) return { ...filter, organization_id: this.organizationId };
  return filter;
}

seedIsUsable() runs that screen first, so an org-NULL unit reads as "does not exist" and contributes nobody — for both recipient widths (business_unit and unit_and_subordinates).

Minimal reproduction

Platform 17.2.0, objectstack dev, SQLite, single tenancy, fresh DB.

1. App seed data creates sys_business_unit rows (SeedSchema, mode: 'upsert'). They land with organization_id: null — there is no way for authored seed data to name a runtime organization id.
2. Add a member: POST /api/v1/data/sys_business_unit_member {user_id, business_unit_id: 'bu_market', is_primary: true} (this row does get organization_id: org_...).
3. Create a rule as the org admin:

POST /api/v1/data/sys_sharing_rule
{ "name":"probe", "label":"probe", "object_name":"kpi_entry_sheet",
  "criteria_json":"{\"subject\":\"bu_market\"}",
  "recipient_type":"unit_and_subordinates", "recipient_id":"bu_market",
  "access_level":"edit", "active":true }

Observed: 201, rule active, sys_record_share gains 0 rows; the member sees 0 records. No error, no warning.

Control, one variable changed — give the seeded unit the organization the rule already carries, then re-touch the same rule:

BEFORE — rule org = org_mtjzvlexj91joh48 | bu_market org = null | shares from this rule = 0
patch unit org -> 200 | bu_market org now = org_mtjzvlexj91joh48
AFTER  — shares from this rule = 3

Nothing else changed. Two further controls in the same session, same DB:

• a rule with recipient_type: 'user' grants correctly (the BU graph is not consulted);
• the same unit recipient against a unit created through the REST API (org-stamped) grants correctly — this is why the defect is invisible in tests that build their fixtures through the API.

Passing organization_id: null explicitly on the rule does not work around it: the response comes back stamped with the caller's organization.

Expected

BusinessUnitGraphService.orgScope should apply the platform's own null-inclusive tenant screen — (organization_id = ? OR organization_id IS NULL) — so that org-NULL platform/app-seeded units keep participating in recipient expansion, matching SqlDriver.applyTenantScope and the #2734 rationale. Failing that, the mismatch should be loud (a warning naming the rule and the unit) rather than an active rule that grants nothing.

Impact

Any app that seeds its organization tree and then provisions sharing rules at runtime loses its entire unit-scoped data range, silently. Declared (metadata) sharing rules are unaffected because they are bootstrapped without an organization context and end up organization_id: null, so orgScope is a no-op for them — which makes the runtime-provisioned path the only one that breaks, and makes the breakage look like an application bug.

Environment

@objectstack/* 17.2.0 · Node 22 · better-sqlite3 · single tenancy · plugins include SharingServicePlugin, Security, PlatformObjects.

---

Triage — confirmed, and ⛔ do NOT dispatch the Expected fix as written

Every claim reproduces at origin/main 4a37870:

• packages/plugins/plugin-sharing/src/business-unit-graph.ts:269-272 — orgScope is the strict equality, verbatim as quoted.
• :127 — seedIsUsable runs where: this.orgScope({ id: businessUnitId }) as the first screen, and :82-85 / :165-168 return [] when it finds nothing. Both recipient widths, as the card says.
• sharing-rule-service.ts:994-999 — the warning is real and it is in the same plugin: open-coding an organization_id equality is "a second, worse copy" that "would drop the NULL arm that keeps platform-seeded rows visible to every tenant (Fresh objectstack dev boot: tenant admin sees ZERO rows in sys_position / sys_permission_set / sys_business_unit over REST (Setup Access Control renders empty) #2734)". The file next door names the mistake; this file makes it.

The card is right about the defect. But its recommended fix cannot be dispatched, and this is the finding that changes the grade:

⚠️ Making orgScope null-inclusive, on its own, converts a silent under-grant into a cross-tenant over-grant

business-unit-graph.ts:169-176 — the member lookup inside expandUnitMembers:

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

No orgScope. And SYSTEM_CTX is { isSystem: true, positions: [], permissions: [] } (:7) — it carries no tenant field, so the engine applies no scope of its own either. The member query is completely unscoped by organization. (The descendants walk at :93-98 does use orgScope; only the member step does not.)

So today the strict equality in orgScope is the only thing keeping an org-NULL unit from reaching that unscoped query. The bug is moonlighting as the tenant guard. Flip orgScope to the null-inclusive form alone, and a seeded unit id shared across tenants — exactly the bu_market shape in the reproduction — lets tenant A's sharing rule expand to tenant B's members and materialise real sys_record_share rows for them.

Silent under-grant would become silent cross-tenant over-grant. That is strictly the worse failure, and it is why this is security + needs-user-decision rather than a queued bug fix.

<!-- os-decision-facets -->

• ① 项目长远合理性(权重 ≥50%,领起推荐) —— 平台已经有一处统一决定「这一行本租户看不看得见」的口子,它发的是「本租户的 或 无归属的」。而共享插件自己在 sharing-rule-service.ts:994-999 白纸黑字写着:自己手写一遍等值判断是「第二份更差的拷贝」,会丢掉那条让平台种子行对每个租户可见的分支 —— 同一个插件里的另一个文件正好就这么写了。长远终态只有一种:租户可见性只在一处决定,别处一律复用。①指向「让部门图走平台的口子」,而不是各写各的。
• ② 实际业务拉动 —— 有,而且是外部使用者报的,带完整复现和单变量对照:只改部门行的归属字段,共享行从 0 变 3,别的什么都没动。任何「种子里建部门树 + 运行时建共享规则」的应用,整条数据范围静默丢失,而且看起来像应用自己的 bug。真实、已发生、非零。
• ③ 防 AI 犯错 —— 出错时谁看到什么:规则建成功返回 201、状态显示启用、共享表零行、日志一个字都没有。作者只能反复怀疑自己的应用写错了。静默容忍的教科书形态。 而且无论方向怎么裁,「一条启用的规则展开出零个收件人」这件事本身就该响亮说出来 —— 这是两条路都拦不住的那句告警。
• ④ 创业阶段不扩散 —— 不引入新概念,复用既有口子,不新增声明。⛔ 但这一棱在这里有个例外要写明:「少写代码」的那条路恰恰是危险的那条 —— 只改 orgScope 一行是最小 diff,也正是会开出跨租户泄露的那个改法。

推荐:A —— 两处必须同批改。 ① orgScope 换成平台的 null-inclusive 形;② expandUnitMembers 的成员查询补上租户筛选(成员行本来就带归属 —— 卡面第 2 步实测 sys_business_unit_member 会被 org 戳上)。⛔ 只改 ① 就是引入泄露;⛔ 不接受拆成两个 PR 前后脚落地 —— 中间那一刻就是敞口。
回退:B —— 不动可见性语义,只把静默变响亮。 规则展开出零收件人时告警并指名规则与部门。零泄露风险、今天就能做,代价是应用仍然拿不到它要的功能,只是不再需要靠猜。
置信缺口(本分析看不见什么): 我读的是代码,没有跑多租户实例。sys_business_unit_member 是否在所有创建路径上都带归属,只有 REST 一条路被实测过(卡面第 2 步)—— 若存在不戳 org 的写入路径(种子、导入、迁移),那么 A 的 ② 也会漏,泄露照旧。这是执行 A 之前必须先量的第一件事,不是执行中顺手确认的事。

Generated by Claude Code

[baozhoutao]

Claim — dispatched by PM session session_01VR2khJ3Me96btawVsfG6jb to an os-dev developer agent. Branch: claude/issue-14547-bu-graph-tenant-screen. Maintainer ruling relayed for this dispatch: proceed with recommendation A (both changes in one PR — null-inclusive orgScope AND a tenant screen on the expandUnitMembers member query), preceded by the measurement the triage names as prerequisite: whether sys_business_unit_member rows are organization-stamped on every write path (seed, import, migration, REST). If any path leaves them unstamped, the PR must treat NULL-org member rows as non-members for org-stamped rules (no cross-tenant expansion) and say so; the PR stays a draft for the platform's own review gates.

---

Generated by Claude Code

[baozhoutao]

os-dev-report

{
  "issue": 14547,
  "status": "done",
  "branch": "claude/issue-14547-bu-graph-tenant-screen",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/14572",
  "premise_still_valid": true,
  "summary": "Recommendation A landed in one PR at c079c35. (1) BusinessUnitGraphService.orgScope now composes the platform's own null-inclusive tenant screen ($or over organization_id equal to the rule's org OR NULL), matching SqlDriver.applyTenantScope and the #2734 rationale, so a seed-written (org-less) sys_business_unit row is usable by an org-stamped rule. (2) Both member reads are tenant-screened, which neither was before: expandUnitMembers AND expandUsers queried sys_business_unit_member with no organization predicate at all under a tenant-less system context, and the old strict unit screen was the only thing keeping an org-stamped rule away from that unscoped query — so (1) alone would have converted a silent under-grant into a silent cross-tenant over-grant. I screened the WIDE width too, not only the narrow one the triage named: unit_and_subordinates is the recipient kind the reported repro used and carried the identical hole; it is named in the PR body. (3) An active business-unit rule expanding to zero recipients now warns once per rule per process (same dedup as the inert-criteria warn), naming rule, object, recipient kind, unit and organization. The member screen is STRICT rather than null-inclusive because the prerequisite measurement came back negative (see the mcp/measurement notes below), so a NULL organization on a membership row means unknown tenancy and a grant fails closed on it. Declared residual: a rule carrying an organization whose unit AND memberships were both seeded still grants nobody — that is exactly the case change (3) makes loud, and the underlying ledger gap is filed as #14570.",
  "prerequisite_measurement": "sys_business_unit_member rows are NOT organization-stamped on every write path. REST/session write: STAMPED (engine threads execCtx.tenantId into DriverOptions; SqlDriver injectTenantOnInsert fills the applySystemFields-injected organization_id column). Seed replay: NOT STAMPED (packages/metadata-protocol/src/seed-loader.ts withholds its single-org fallbackOrgId from every sys_ / cloud_ / ai_ object; only a pinned config.organizationId or a record that spells the column itself stamps one). Elevated system-context write: NOT STAMPED (sys_business_unit_member is 'unclassified' in PLATFORM_OBJECT_TENANCY, packages/objectql/src/tenancy/platform-object-tenancy.ts, so Engine.resolveSystemInsertOrganization returns early). driver-memory / driver-mongodb: NEVER stamp a tenant column (both refuse to boot multi-tenant, which is what makes that safe). No import or migration writer for this object exists in the tree — the only non-generic writers found by grep are readers (plugin-security delegated-admin-gate, plugin-approvals approval-service, plugin-sharing primary-bu-projection). The repo's own dogfood fixture is an instance of the unstamped elevated path.",
  "tests": "All at c079c35 (git rev-parse --short HEAD after the final commit; nothing landed after the union). (a) pnpm --filter @objectstack/plugin-sharing test -> 'Test Files 31 passed (31) / Tests 731 passed (731)', EXIT=0 captured before any pipe. (b) pnpm --filter @objectstack/plugin-sharing typecheck -> EXIT=0; verdict line 'check:test-typecheck: OK — @objectstack/plugin-sharing's test layer compiles under packages/plugins/plugin-sharing/tsconfig.test.json; 2 file(s) / 3 error(s) / 3 pinned signature(s) held in test-typecheck-debt.json'. The package tsconfig excludes *.test.ts, so plain tsc does NOT cover the new tests — tsconfig.test.json does, proven by that gate naming my new file's one type error (TS2322 on the vi.fn mock) before I fixed it. (c) GATE UNION 68 families, derived by `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack` (no hand-built path list; the script reads its own change set) and RE-RUN in full at c079c35 after the last commit: 67 green, 1 NOT MEASURED — check-test-completeness exits 3, which its own text defines as 'nothing was measured' when handed no saved turbo test log, explicitly not a red. The family grew 37 -> 68 mid-run because check:system-context-census --fix added content/docs/permissions/system-context.mdx to the diff; I re-derived and ran all 31 newly-named docs families too. Two gates needed upkeep, both mechanical: check:system-context-census --fix (my +7-line insertion into sharing-rule-service.ts rotted anchors :157 -> :164 and :382 -> :389; the +7 arithmetic is exact) and check-engine-double-contract --write (the new test file pins engine doubles the ledger did not record; the gate's own text calls that GOOD and asks for the write). (d) pnpm lint over the WHOLE repo (eslint . --no-inline-config) EXIT=0 in 86s under the verify lock — no narrowing claimed, so none needs justifying. (e) pnpm check:nul-bytes EXIT=0 plus a direct grep -naP control-character scan over every file in the diff: no hits. (f) ABLATIONS, two, both with the rebuild question answered: NO rebuild is involved because the tests import './business-unit-graph.js' relative inside the package, so vitest resolves src and no dist/ can mask the mutation (check:test-source-alias green confirms no alias games). Each leg proved the mutation ON DISK by grep -c of BOTH the injected marker and the removed text (never by the editor's exit code) plus a git hash-object differing from the HEAD blob; each ran under trap '...' EXIT INT TERM with an absolute REPO_ROOT path; each restore used `git checkout HEAD -- ABSOLUTE_PATH` (never a bare checkout, which reads the index) and was proved by an empty git diff HEAD AND a restored blob hash equal to HEAD's 770a13fd. Ablation 1, orgScope back to strict equality: mutated blob f15745d8, result 'Tests 8 failed | 22 passed (30)' — red on (a) an org-NULL unit is USABLE, on the flipped divergence pin, and on both end-to-end materialisation cases. Ablation 2, both memberScope calls removed: mutated blob 3b4549fb, result 'Tests 6 failed | 24 passed (30)' — red on (b) members of ANOTHER organization are never expanded, (c) a NULL-org member row is NOT a member, and the zero-recipient warn. Neither red set is a subset of the other, which is what shows the two screens are two facts. NOT RUN LOCALLY and declared as such: the dogfood suite (needs a booted stack) and the downstream cross-package test sweep — CI's Dogfood Regression Gate and Test Core own those. My reasoning that dogfood is unaffected is reasoning, not measurement: rules.defineRule reads callerOrgId(context), the dogfood SYS context is a bare { isSystem: true } with no organizationId or tenantId, so the rule row is created with organization_id null, and with a null organization BOTH new screens are no-ops.",
  "fixture_triage": "Three pre-existing fixtures paired org-scoped UNITS with org-less MEMBER rows and passed only because the member read had no predicate — they are the gap, in fixture form. All three re-stamped (business-unit-graph.test.ts 'an org-scoped rule expands units belonging to that org'; sharing-rule.test.ts both sys_business_unit_member tables), each with a comment naming which fact it was always describing. Two more still assert [] but now for the MEMBER reason rather than the unit reason, and say so. One pin recorded the retired posture as deliberate ('[divergence] an org-scoped rule does NOT see an env-wide null-org unit') and is FLIPPED, not deleted; the file's head docblock — which had nominated exactly this $or predicate as the future fix — is rewritten to describe the asymmetric pair that landed. The consumption radius was swept by grepping every BusinessUnitGraphService consumer in packages/, examples/ and apps/: all inside plugin-sharing (the class is exported from index.ts, so out-of-repo consumers exist in principle).",
  "mcp_calls": "3 — issue_write (filing #14570), create_pull_request, add_issue_comment (this report). Every READ went through unauthenticated repo-scoped REST via curl, which answers 200 on this public repo: the issue's comments (to re-read the claim — one comment, the PM's, matching my session id and branch, no competing claim) and 6 pages of the open-issues list endpoint for the dedup scan. Channel note: the REST SEARCH endpoint answers 403 unauthenticated, so the dedup scan used the REST LIST endpoint plus a local regexp instead — 483 open non-PR issues scanned by title, with a control term ('sharing') returning 9 hits in the same pass, so the 4 candidate hits are a live reading and not an empty channel. No MCP search_issues call was needed.",
  "open_questions": [],
  "out_of_scope_findings": [
    "filed as #14570: sys_business_unit_member is unadjudicated ('unclassified') in PLATFORM_OBJECT_TENANCY, so seed-replayed and elevated-system-context membership rows land organization-less — the residual cause of the one combination that still grants nobody after this PR. Filed unassigned with the full write-path table, a Blocked-by line to #14096 (whether the seed loader should follow the per-object classification at all) and a pointer to #13636 (the conditionally-tenant-scoped third state). No backfill proposed: the 2026-08-31 ruling's execution point 3 governs that."
  ]
}

Generated by Claude Code

---

Generated by Claude Code

[os-project-manager]

Ledger note — ruled and dispatched through another PM session; needs-user-decision was left on by mistake and is corrected here

Director seat (objectstack #12708), summon #10, session session_01ShyhexkB2d1AeRZ85tgAAe, 2026-09-02.

This card is in the decision inbox by label only. The record shows it was already ruled: the claim comment (5508806533, PM session session_01VR2khJ3Me96btawVsfG6jb, 2026-09-02T11:27Z) relays the maintainer's ruling A (null-inclusive orgScope and a tenant screen on the member query, in one PR, after the write-path measurement), and the os-dev report (5509665790) delivers it as PR #14572 with the prerequisite measurement answered (membership rows are not organization-stamped on the seed and elevated-system paths, so the member screen is strict and fails closed; residual filed as #14570). The director seat does not re-adjudicate a ruling the maintainer has already given; it records that the ruling of record for this card is the one relayed in comment 5508806533, and that the provenance three-piece (who / verbatim / where) lives with that session, not here.

State correction, same stroke: needs-user-decision → pm:dispatched (PR #14572 open, assignee unchanged). The one-of rule holds again. Family note for the maintainer's next batch: this card, #14484 (sys_record_share rows written org-NULL), #13564 (the orWhereNull carve-out) and #14570 (sys_business_unit_member unclassified) are one tenant-attribution family and are presented together.

---

Generated by Claude Code