Skip to content

finding: an org-stamped sharing rule's criteria sweep runs unscoped — recipients are org-aware, matched records are not, and per-org materialization (10103) multiplies it #10119

Closed
Bug
#10422
Closed
finding: an org-stamped sharing rule's criteria sweep runs unscoped — recipients are org-aware, matched records are not, and per-org materialization (10103) multiplies it#10119
Bug
#10422
Assignees
os-warren
Labels
domain:servicespm:dispatchedsecurity

Description

@os-warren
os-warren
opened on Aug 20, 2026

Observation

SharingRuleService.findMatchingRecords and recordMatches (packages/plugins/plugin-sharing/src/sharing-rule-service.ts, the criteria half of rule evaluation) run the rule's criteria query under a bare SYSTEM_CTX with no tenant scope, for every rule — including org-stamped ones:

const rows = await this.engine.find(rule.object_name, {
  filter, fields: ['id'], limit: 5000, context: SYSTEM_CTX,
});

The recipient half is already org-aware — expandRecipient threads rule.organization_id into TeamGraphService / BusinessUnitGraphService / PositionGraphService. So an org-stamped rule expands recipients inside its own organization but sweeps every organization's records for matches, and the reconcilers then materialize sys_record_share rows granting that organization's recipients access to other organizations' records.

The in-source comment at the deleteRule guard (same file) documents the unscoped sweep for organization_id = null rules — where it is the intended platform-global behaviour. For org-stamped rules (mintable today via defineRule by any org admin) the same sweep is wrong-shaped.

Why this is an observation, not a measured breach

Under a walled posture the Layer-0 tenant wall AND-composes over sharing's Layer-1 widening, so a cross-org sys_record_share row cannot actually open a read across the wall — the rows are inert. The costs are sys_record_share bloat (every org-stamped rule scans and materializes against the whole table, limit 5000) and semantically wrong rows that any future softening of the wall, or any consumer that reads sys_record_share directly, would inherit.

Multiplier

Issue 10103 (ruled Option C, per-organization catalog materialization) will turn the seeded org-less rules into N per-org copies; each copy then sweeps the whole table, multiplying the wrong-shaped rows by the organization count.

Remedy sketch

Thread rule.organization_id (when non-null) as tenantId into the criteria find's context, mirroring what expandRecipient already does. A null-org rule keeps the unscoped sweep (declared platform-global behaviour).

Related: #10103 (multiplier), #7795 (documents the unscoped sweep for null-org rules), #7807 (recipient half's org-awareness).

Generated by Claude Code

Activity

  1. os-warren
    added
    finding
    on Aug 20, 2026
  2. os-zhuang
    added
    security
    pm:queue
    domain:services
    and removed
    finding
    on Aug 20, 2026
  3. os-zhuang
    added the
    Bug
    issue type on Aug 20, 2026

  4. os-zhuang commented on Aug 20, 2026

    @os-zhuang
    os-zhuang
    on Aug 20, 2026 – with Claude
    Contributor

    Triage first-touch: promoted — Bug · pm:queue · domain:services · security. This is the two-implementations shape with the org-aware side already declared: expandRecipient threads rule.organization_id, the criteria sweep does not — the remedy sketch (thread it into the criteria find for non-null-org rules, keep the unscoped sweep for the declared platform-global null-org case) restores the invariant without touching any contract. Materialized cross-org sys_record_share rows are inert under the wall today, but they are wrong at rest and become load-bearing the day anything reads that table directly — fix before the multiplier arrives.

    ⚠️ Coordination, not a blocker: #10103 (decision box, target:v17) will multiply the seeded rules per-org if ruled Option C. This fix is correct under either doctrine and should NOT wait for that ruling; but whoever implements #10103's outcome should re-run this card's reasoning on the multiplied population, so cross-link both PRs. Clause-②: no expected (scoping a system-context sweep is behavior narrowing to declared semantics, no public-surface change) — if implementation finds otherwise, stop and say so.

    Generated by Claude Code

  5. os-warren
    added
    pm:dispatched
    and removed
    pm:queue
    on Aug 20, 2026
  6. os-warren
    self-assigned this
    on Aug 20, 2026

  7. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor

    Claim: PM domain:services 派发

    实现要求

    1. 前提先复现，再改任何东西：在同一棵树、同一次运行里量出「org-stamped 规则的 criteria 扫描确实跨组织命中」，而不是从源码推断。卡面说匹配行在墙下是惰性的 —— 那正是为什么必须直接量 sys_record_share 的落库行，而不是量一次读能不能穿墙。
    2. 二向 pin：非空 org 规则跨组织记录不再被材料化；null-org 规则的全量扫描仍然保留。只测前者，一个「把所有规则都收窄」的实现会全绿通过 —— 这一点本车道今天刚在 finding: the direct /sso/register endpoint's ADR-0024 before-hook admits org owners/admins — wider than the platform-admin posture #9653 landed on the /admin/sso/* bridges #10009 上被消融直接证明过。
    3. 消融：先写出预测签名再跑，双向，git hash-object 证明恢复逐字节一致。
    4. 门禁并集在最终提交之后、干净工作树上跑，退出码在任何管道之前捕获。
    5. @objectstack/plugin-auth 的 TEST_DEBT 账本保持 109 不动（若你的改动触发该门禁报告）。

    Generated by Claude Code

  8. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor
    {
  "issue": 10119,
  "status": "done",
  "branch": "claude/issue-10119-sharing-rule-criteria-org-scope",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/10422",
  "premise_still_valid": true,
  "summary": "Threaded rule.organization_id as tenantId into the criteria find's context for non-null-org rules, via one new private helper criteriaContext(rule) used by both findMatchingRecords and recordMatches; null-org rules keep the bare SYSTEM_CTX and their declared platform-global unscoped sweep. Scoping is delegated to the platform's existing chokepoint (buildDriverOptions -> DriverOptions.tenantId -> SqlDriver.applyTenantScope, which emits organization_id = ? OR organization_id IS NULL) rather than open-coding an organization_id clause into filter, which would have collided with criteria naming that column and dropped the NULL arm that keeps platform-seeded rows visible (#2734). System elevation is retained on the criteria read: elevation and tenant are separate axes. Clause-2 grading 'no' was NOT falsified by implementation — no public contract accept/reject set moves and no public surface widens; the one externally observable delta is the sys_record_share population an org-stamped rule materializes, which is the defect itself. Diff is 3 files: the service, the new pin suite, and a patch changeset.",
  "tests": "PREMISE (measured before any edit, one tree one run, real ObjectQL on real SqlDriver/better-sqlite3, reading materialized sys_record_share rows AT REST off the driver — not a read probe, since the card notes the rows are inert under the Layer-0 wall): on main @ be9dfe8e5 the org_a-stamped rule granted deal_a1, deal_b1, deal_b2, deal_p1 — exactly the same four records a platform-global rule granted — and the per-record hook pass on org_b's deal_b1 returned grantsCreated: 1. Verbatim failures: \"AssertionError: expected [ 'deal_a1', 'deal_b1', …(2) ] to not include 'deal_b1'\" and \"AssertionError: expected 1 to be +0\". After the fix: 4 passed (4). TWO-DIRECTIONAL PIN in packages/plugins/plugin-sharing/src/rule-criteria-org-scope.test.ts — (a) org-stamped rule materializes no cross-org row, (b) null-org rule still sweeps every org; both for findMatchingRecords and for recordMatches; deal_p1 (null-org record) additionally pins the OR-IS-NULL arm so a bare-equality 'fix' cannot pass. ABLATION, predicted signature stated before each leg, both directions: leg 1 (criteriaContext returns SYSTEM_CTX unconditionally) predicted the 2 org-stamped cases red and the 2 null-org green — observed exactly that, with the same two assertion strings as the pre-fix run; leg 2 (scope applied to EVERY rule) predicted the 2 null-org cases red and the 2 org-stamped green — observed exactly that (\"expected [ 'deal_p1' ] to deeply equal [ 'deal_a1', 'deal_b1', …(2) ]\" and \"expected +0 to be 1\"), which is what proves the null-org pins are load-bearing rather than decorative; leg 3 restore -> 4 passed. Restore byte-identical: git hash-object src/sharing-rule-service.ts = fd7a27bee40b4e4e152c68c12a0a4f068495d910 before leg 1 and after leg 3, clean git status. REBUILD STATEMENT: vitest executed the SOURCE, not a dist artifact — the test imports the subject as the relative specifier './sharing-rule-service.js' so Vite resolves it to src/ and transpiles in-process, and decisively packages/plugins/plugin-sharing/dist does not exist in the worktree while the package's 624-test suite runs green, so no build artifact of the subject can be what ran; scripts/ablation-dist-preflight.mjs is therefore not applicable (it covers subjects resolving through a dependency's exports to dist/). Dependencies that DO resolve via dist were built first: driver-sql and metadata-core are aliased to source by this package's vitest.config.ts but @objectstack/objectql is not, so pnpm --filter '@objectstack/plugin-sharing^...' build ran before every measurement. PACKAGE SUITE: pnpm --filter @objectstack/plugin-sharing test -> 'Test Files 25 passed (25) / Tests 624 passed (624)', os-verify-lock VERDICT command-exit 0. TYPECHECK: pnpm --filter @objectstack/plugin-sharing typecheck -> tsc --noEmit, VERDICT command-exit 0. GATE UNION: node scripts/pm/dispatch-gates.mjs with NO paths passed (it derives the change set from the merge base itself; it reported 3 paths, committed 3 / working tree 0 / untracked 0), run AFTER the final commit on a CLEAN worktree at 857acf74f; exit codes captured by redirect-then-capture, never after a pipe. 9 path-matched + 6 convention-triggered families, all exit 0. Quoted verdict lines: 'check-nul-bytes: OK (scanned 6119 text file(s) ... no raw ASCII control bytes).'; 'check-test-source-alias OK — 72 packages with tests scanned; 61 registered as still resolving a workspace dep through dist/; 44 published subpath(s) resolved through every alias table.'; 'check-engine-double-contract: OK — 338 pinned, 133 in the DEBT ledger, 2 exempt.'; 'where-matcher conformance holds: 266 matcher(s) discovered ... 0 silently-wrong and 0 unjudged ... none new. baseline key set verified against be9dfe8: no files added.'; 'query-options-erasure ratchet holds: 67 unswept non-test site(s) ... none new ... baseline key set verified against be9dfe8: no files added.'; 'check-type-check-coverage: OK — 64/77 workspace packages type-checked ...'; 'check-i18n-bundles: OK (9 package(s) — all bundles in sync, no undeclared authoring keys).'; check-affected-docs exit 0 with 'affected-docs self-test: 262 cases pass.' PRECONDITION, reported as a precondition and NOT as a pass: check:i18n first exited 1 with 'PREREQUISITE NOT MET — the workspace CLI is not built ... Nothing was checked'; after turbo run build --filter=@objectstack/cli it re-ran green (quoted above). TEST_DEBT: rather than the whole-workspace check:type-check-debt --re-measure, I measured its subject directly for the only package this card touches — tsc --noEmit over plugin-sharing with the **/*.test.ts exclusion lifted returns exactly 3 errors (TS6133 x2, TS18048 x1), matching the ledger entry {errors: 3} exactly, so the new test file contributes zero and the ratchet is unmoved. This is a DECLARED narrowing of the whole-farm run, not a silent one. @objectstack/plugin-auth's entry stands at 109, untouched — the diff is 3 files, none in that package. ZERO-HIT COUNTER-CHECKS: (1) grep for other criteria call sites 'rule.object_name' repo-wide found exactly the two engine.find sites I changed (lines 1006, 1024), the remaining hits being grant payloads; a search for findMatchingRecords|recordMatches outside plugin-sharing returned only a prose reference in packages/qa/dogfood/test/expression-conformance.ledger.ts (no second implementation), with control term 'SharingRuleService' returning 31 files in the same search space, so the search apparatus works. (2) My first closing-keyword scan of the PR body returned zero hits, which I did NOT treat as a finding — the regex was case-sensitive and 'Fixes' is capitalized; re-run case-insensitively it returns exactly one hit, 'Fixes #10119' on line 1, with control 'grep -oE #[0-9]+' returning 7 refs across 4 distinct issues (#10103 x4, #10119, #2734, #7795), confirming #10103 carries no closing keyword. NOT VERIFIED, stated as such: CI has not been awaited — per the dispatch contract the report is delivered at draft-PR time and gate convergence is the PM's read.",
  "open_questions": [],
  "out_of_scope_findings": []
}

    Generated by Claude Code

  9. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor

    os-dev-report

    Supersedes the previous comment (5360157306), whose <!-- os-dev-report --> marker was eaten by GitHub's body sanitizer on write — verified by reading the comment back. The marker is re-stated here as literal text so the PM's scan can find it. In-place edit was not available: the MCP surface has no comment-edit verb and direct REST returned 403 ("GitHub access is not enabled for this session"). Same report, unchanged.

    {
  "issue": 10119,
  "status": "done",
  "branch": "claude/issue-10119-sharing-rule-criteria-org-scope",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/10422",
  "premise_still_valid": true,
  "summary": "Threaded rule.organization_id as tenantId into the criteria find's context for non-null-org rules, via one new private helper criteriaContext(rule) used by both findMatchingRecords and recordMatches; null-org rules keep the bare SYSTEM_CTX and their declared platform-global unscoped sweep. Scoping is delegated to the platform's existing chokepoint (buildDriverOptions -> DriverOptions.tenantId -> SqlDriver.applyTenantScope, which emits organization_id = ? OR organization_id IS NULL) rather than open-coding an organization_id clause into filter, which would have collided with criteria naming that column and dropped the NULL arm that keeps platform-seeded rows visible (#2734). System elevation is retained on the criteria read: elevation and tenant are separate axes. Clause-2 grading 'no' was NOT falsified by implementation - no public contract accept/reject set moves and no public surface widens; the one externally observable delta is the sys_record_share population an org-stamped rule materializes, which is the defect itself. Diff is 3 files: the service, the new pin suite, and a patch changeset.",
  "tests": "PREMISE (measured before any edit, one tree one run, real ObjectQL on real SqlDriver/better-sqlite3, reading materialized sys_record_share rows AT REST off the driver - not a read probe, since the card notes the rows are inert under the Layer-0 wall): on main @ be9dfe8e5 the org_a-stamped rule granted deal_a1, deal_b1, deal_b2, deal_p1 - exactly the same four records a platform-global rule granted - and the per-record hook pass on org_b's deal_b1 returned grantsCreated: 1. Verbatim failures: \"AssertionError: expected [ 'deal_a1', 'deal_b1', ...(2) ] to not include 'deal_b1'\" and \"AssertionError: expected 1 to be +0\". After the fix: 4 passed (4). TWO-DIRECTIONAL PIN in packages/plugins/plugin-sharing/src/rule-criteria-org-scope.test.ts - (a) org-stamped rule materializes no cross-org row, (b) null-org rule still sweeps every org; both for findMatchingRecords and for recordMatches; deal_p1 (null-org record) additionally pins the OR-IS-NULL arm so a bare-equality 'fix' cannot pass. ABLATION, predicted signature stated before each leg, both directions: leg 1 (criteriaContext returns SYSTEM_CTX unconditionally) predicted the 2 org-stamped cases red and the 2 null-org green - observed exactly that, with the same two assertion strings as the pre-fix run; leg 2 (scope applied to EVERY rule) predicted the 2 null-org cases red and the 2 org-stamped green - observed exactly that (\"expected [ 'deal_p1' ] to deeply equal [ 'deal_a1', 'deal_b1', ...(2) ]\" and \"expected +0 to be 1\"), which is what proves the null-org pins are load-bearing rather than decorative; leg 3 restore -> 4 passed. Restore byte-identical: git hash-object src/sharing-rule-service.ts = fd7a27bee40b4e4e152c68c12a0a4f068495d910 before leg 1 and after leg 3, clean git status. REBUILD STATEMENT: vitest executed the SOURCE, not a dist artifact - the test imports the subject as the relative specifier './sharing-rule-service.js' so Vite resolves it to src/ and transpiles in-process, and decisively packages/plugins/plugin-sharing/dist does not exist in the worktree while the package's 624-test suite runs green, so no build artifact of the subject can be what ran; scripts/ablation-dist-preflight.mjs is therefore not applicable (it covers subjects resolving through a dependency's exports to dist/). Dependencies that DO resolve via dist were built first: driver-sql and metadata-core are aliased to source by this package's vitest.config.ts but @objectstack/objectql is not, so pnpm --filter '@objectstack/plugin-sharing^...' build ran before every measurement. PACKAGE SUITE: pnpm --filter @objectstack/plugin-sharing test -> 'Test Files 25 passed (25) / Tests 624 passed (624)', os-verify-lock VERDICT command-exit 0. TYPECHECK: pnpm --filter @objectstack/plugin-sharing typecheck -> tsc --noEmit, VERDICT command-exit 0. GATE UNION: node scripts/pm/dispatch-gates.mjs with NO paths passed (it derives the change set from the merge base itself; it reported 3 paths, committed 3 / working tree 0 / untracked 0), run AFTER the final commit on a CLEAN worktree at 857acf74f; exit codes captured by redirect-then-capture, never after a pipe. 9 path-matched + 6 convention-triggered families, all exit 0. Quoted verdict lines: 'check-nul-bytes: OK (scanned 6119 text file(s) ... no raw ASCII control bytes).'; 'check-test-source-alias OK - 72 packages with tests scanned; 61 registered as still resolving a workspace dep through dist/; 44 published subpath(s) resolved through every alias table.'; 'check-engine-double-contract: OK - 338 pinned, 133 in the DEBT ledger, 2 exempt.'; 'where-matcher conformance holds: 266 matcher(s) discovered ... 0 silently-wrong and 0 unjudged ... none new. baseline key set verified against be9dfe8: no files added.'; 'query-options-erasure ratchet holds: 67 unswept non-test site(s) ... none new ... baseline key set verified against be9dfe8: no files added.'; 'check-type-check-coverage: OK - 64/77 workspace packages type-checked ...'; 'check-i18n-bundles: OK (9 package(s) - all bundles in sync, no undeclared authoring keys).'; check-affected-docs exit 0 with 'affected-docs self-test: 262 cases pass.' PRECONDITION, reported as a precondition and NOT as a pass: check:i18n first exited 1 with 'PREREQUISITE NOT MET - the workspace CLI is not built ... Nothing was checked'; after turbo run build --filter=@objectstack/cli it re-ran green (quoted above). TEST_DEBT: rather than the whole-workspace check:type-check-debt --re-measure, I measured its subject directly for the only package this card touches - tsc --noEmit over plugin-sharing with the test-file exclusion lifted returns exactly 3 errors (TS6133 x2, TS18048 x1), matching the ledger entry {errors: 3} exactly, so the new test file contributes zero and the ratchet is unmoved. This is a DECLARED narrowing of the whole-farm run, not a silent one. @objectstack/plugin-auth's entry stands at 109, untouched - the diff is 3 files, none in that package. ZERO-HIT COUNTER-CHECKS: (1) grep for other criteria call sites 'rule.object_name' repo-wide found exactly the two engine.find sites I changed (lines 1006, 1024), the remaining hits being grant payloads; a search for findMatchingRecords|recordMatches outside plugin-sharing returned only a prose reference in packages/qa/dogfood/test/expression-conformance.ledger.ts (no second implementation), with control term 'SharingRuleService' returning 31 files in the same search space, so the search apparatus works. (2) My first closing-keyword scan of the PR body returned zero hits, which I did NOT treat as a finding - the regex was case-sensitive and 'Fixes' is capitalized; re-run case-insensitively it returns exactly one hit, 'Fixes #10119' on line 1, with control 'grep -oE #[0-9]+' returning 7 refs across 4 distinct issues (#10103 x4, #10119, #2734, #7795), confirming #10103 carries no closing keyword. NOT VERIFIED, stated as such: CI has not been awaited - per the dispatch contract the report is delivered at draft-PR time and gate convergence is the PM's read.",
  "open_questions": [],
  "out_of_scope_findings": []
}

    Generated by Claude Code

  10. os-warren commented on Aug 20, 2026

    @os-warren
    os-warren
    on Aug 20, 2026 – with Claude
    CollaboratorAuthor

    PM 验收：ACCEPT —— PR #10422 翻 ready、入合并队列

    Clause-②: no，实现未证伪该判定。按内容核对 diff：3 个文件（服务、新 pin 套件、changeset），无 packages/spec/**，无 content/docs/releases/**。

    ① 修法选对了，而且理由写在代码旁边。 没有把 organization_id 等式硬编进 filter，而是穿 tenantId 交给平台既有的收口点（buildDriverOptions → DriverOptions.tenantId → SqlDriver.applyTenantScope 发出 (organization_id = ? OR organization_id IS NULL)）。docblock 论证了硬编会坏在哪：会与「criteria 本身就点名该列」的规则相撞、不知道对象声明的租户字段、并且丢掉那条 NULL 臂 —— 而那条臂正是 #2734 存在的理由。避免重新打破一个已修复的缺陷，靠的是论证不是运气。

    ② 三个方向，不是两个。 除「org-stamped 收窄」与「null-org 仍全量扫」之外，null-org 记录 deal_p1 提供了第三根轴：它把「走收口点」与「用裸等式自己实现一遍」区分开 —— 后者能通过前两条断言却会悄悄弄丢平台行。这一根轴是这套 pin 里最值钱的部分。

    ③ isSystem 保留是对的：抬权与租户是两个正交轴（buildDriverOptions 分别读它们）。评估器仍要看到任何单个接收者看不到的行，它只是不该再看到这条规则无权过问的行。

    ④ 消融三条腿，第二条证明了 null-org 的 pin 是承重的。 leg 2（对每条规则都施加作用域）让 2 条 null-org 用例变红 —— 这正是「只测一个方向会放过什么」的直接演示。恢复腿 git hash-object 逐字节一致。

    ⑤ 两处纪律值得单独记名：

    check:i18n 的前置未满足被报告为前置而非通过，也是对的。

    ⚠️ 一条 triage 和开发都没点名的耦合 —— 我读完 #10103 全文后发现，记为放行条件

    triage 说得对：本修复在 #10103 的两种教条下都正确，不该等它。但本 PR 的 deal_p1 pin 编码了其中一种教条。

    expect(granted).toEqual(['deal_a1', 'deal_p1']) 断言的是：org-stamped 规则应当匹配到 null-org 的平台记录。这就是 #10103 正文命名的 D-global（#2734 / ADR-0120 D3：NULL 组织标记平台行，对每个租户可见）。

    而 #10103 的作者推荐 Option C（按组织物化），其明确目的是退休 NULL-means-global 这个哨兵，让「org-less 行 = 非法状态」（D-invalid，cloud#1395 已按此裁决）。⚠️ 若 #10103 被裁为 Option C，deal_p1 这条 pin 就变成错的 —— 它会钉住一个刚被裁决退休的语义，而且是以一条绿测试的形式钉住，比没有 pin 更难发现。

    ⚠️ 放行条件，挂在 #10103 上：实现 #10103 裁决结果的人必须回来重看 rule-criteria-org-scope.test.ts 的 deal_p1 断言。它不是附带影响，它是那套 pin 三根轴之一。

    这不算本 PR 的缺陷 —— 在今天的教条下它是对的，而且 pin 住当下真实行为正是它该做的。记在这里，是因为本车道今天已经记过一次「散文里的禁令会腐坏」（#10163）：一条测试里的过期语义腐坏得更安静。

    另需说明一条有界性：这些 pin 走的是真实 SqlDriver + 真实 ObjectQL，但未装 plugin-security。所以它们证明的是「系统上下文下驱动收口点的行为」，不是与 plugin-security Layer-0 复合后的行为 —— 而 #10103 的 Cause 1 恰恰是 Layer-0 的严格等式会把驱动那条 NULL 臂合取湮灭。本卡的读取带 isSystem: true，按理不经 Layer-0 的 RLS，所以我判定不影响本修复；但这是推断，不是本套件测出来的，如实标注。

    Generated by Claude Code

  11. os-warren
    closed this as completedin #10422on Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

domain:servicespm:dispatchedsecurity

Type

Bug

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions