plugin-sharing: getRule's by-id branch has no org filter — an org admin can read, evaluate and DELETE another organization's sharing rule (and its grants)

Symptom

SharingRuleService.getRule resolves an id without any organization filter, so an org-scoped sharing admin holding another organization's rule id can read, evaluate and delete that organization's rule.

packages/plugins/plugin-sharing/src/sharing-rule-service.ts, getRule:

const byId = await this.engine.find('sys_sharing_rule', {
  where: { id: idOrName },   // ← no organization_id predicate
  limit: 1,
  context: SYSTEM_CTX,
});
if (Array.isArray(byId) && byId[0]) return rowFromRule(byId[0]);

The read runs under SYSTEM_CTX, so nothing downstream re-scopes it either. Three verbs inherit the hole, because all three resolve through getRule:

GET /api/v1/sharing/rules/:id — read another org's rule (criteria, recipient, access level).
POST /api/v1/sharing/rules/:id/evaluate — materialise that org's grants on demand.
DELETE /api/v1/sharing/rules/:id — deleteRule calls getRule and then purges the row and every sys_record_share grant it materialised, i.e. silently revoke another tenant's record access.

The by-name path is correctly scoped ({name, organization_id: orgId}), so this is specifically the id branch. The caller still needs manage_sharing (or the legacy manage_platform_settings) in their own org, and needs to know an opaque srule_… id — but neither is a tenant boundary, and ids leak through logs, exports, support tickets and evaluation responses ({ruleId} is returned by the evaluate endpoint).

Not a regression from #7676

Found while auditing callers for #7676 / PR #7760, which widens the two org-scoped reads (listRules, getRule's by-name fallback) from organization_id = orgId to organization_id = orgId OR organization_id IS NULL. That change deliberately does not touch this branch and does not make it worse: it never admits another organization's rows. This defect is present on origin/main today and predates it.

Worth noting the interaction, though: #7760 makes seeded rules listable, and a rule's id is in the listing. That is intended (they are platform-global), but it does mean rule ids become easier to come by generally, which raises the value of closing this.

Suggested direction

Scope the by-id branch the same way the by-name path is scoped once #7760 lands — id = idOrName AND (organization_id = orgId OR organization_id IS NULL) when the caller carries an organization, unfiltered when it does not (system/boot contexts must keep working). Points to check before implementing:

Whether any existing caller depends on by-id reaching an arbitrary org's row. From the record-share-grant-revoke: package-seeded sharing rules are unaddressable by name (org filter excludes org_id-null rows) #7676 audit, every non-REST caller passes a system context (no org id), so they take the unfiltered branch regardless.
Whether a platform admin (as opposed to an org admin) should retain cross-org by-id access, and if so which capability expresses that — manage_sharing is an org-level capability and should not.
deleteRule and evaluateRule inherit whatever getRule decides, so no separate change is needed for them, but both want a test.

Tests should pin both directions: another org's rule is unreachable by id (404 rather than 200/204), and a platform-global (organization_id = null) rule stays reachable by id.

Source

Found during the mandatory caller/tenant-isolation audit for #7676 (PR #7760). Filed unassigned.

PM triage (domain:identity seat, #6022, session session_01BVc1ekPpi6yaWywAUhfzfd): pm:queue, security + domain:identity applied. Held, serialized behind PR #7760 — that PR is rewriting the same function (getRule) and is ready + auto-merge armed, so dispatching now would collide for no gain.

Confirmed as filed, not a regression. The by-id branch's missing org predicate is on origin/main today and #7760's widening never admits another organization's rows. The card's own framing is right: the fix is to give the by-id branch the scope the by-name path will have after #7760 lands — id = {id} AND (organization_id = {orgId} OR organization_id IS NULL) when the caller carries an organization, unfiltered when it does not.

One axis to add to this card's scope when it dispatches, surfaced in #7760's review: besides another org's rule, there is the platform-global row. Deleting an organization_id = null seeded rule affects every tenant until the next boot reseeds it, and #7760 makes those rows discoverable by name and in the listing. So this card's fix must decide two things at once, not one:

another organization's row → unreachable by id (the filed defect);
a platform-global row → reachable by id (it must stay, for symmetry with the by-name path fix(plugin-sharing): make package-seeded (org-null) sharing rules visible and addressable by name for org-scoped admins #7760 ships) — but is an org admin allowed to delete it, given the cross-tenant effect? A read/evaluate/delete split is worth considering here even though the current code treats all three identically through getRule.

Point (2) is the part most likely to need a maintainer ruling rather than a dev's judgment; if the dev concludes it does, it escalates on this card instead of guessing. Not marked needs-user-decision now because (1) alone is unambiguous and shippable, and holding the whole card for (2) would leave a live cross-tenant delete open.

Sizing note for the dispatch: the two anti-pass-for-the-wrong-reason guards from #7760 apply here too — the fake engine in this package now conjoins $or correctly (fixed there), and bootStack cannot mint an org-scoped admin (#7762), so this card's coverage will again have to live at the service layer with a hand-modelled org context.

Not target:v17-labelled by this seat: the exposure requires a multi-tenant deployment, which needs the cloud-private organizations package, and the triage seat owns that label. Flagging the question rather than answering it.

Generated by Claude Code

Claim: PM loop (domain:identity seat, #6022)
Session: session_01BVc1ekPpi6yaWywAUhfzfd
Branch: claude/issue-7761-getrule-byid-org-scope
Worktree: objectstack-issue-7761
Mode & model: M card, mode:subagent (Claude_Code_Remote absent this shift — sanctioned fallback), model: opus.

Predecessor cleared. PR #7760 landed as squash 8669e5dab; verified on origin/main that adminOrgScope and findRuleRowByName are in place, that defineRule's strict equality survived (:156 — the deliberate non-widening), and — as the negative-space control for this card — that the by-id branch at sharing-rule-service.ts:284 is still where: { id: idOrName } with no org predicate. The defect is live on main as filed.

Scope as dispatched: give the by-id branch the same adminOrgScope the by-name path now has, so another organization's rule is unreachable by id while a platform-global (organization_id = null) row stays reachable; system/boot contexts (no org id) keep the unfiltered branch. deleteRule and evaluateRule inherit it through getRule and each get their own test.

The second axis recorded in the triage comment above travels with the dispatch: an org admin deleting a platform-global row affects every tenant until the next boot reseeds it, and #7760 made those rows discoverable. The dev is instructed to implement the unambiguous half (another org's row → unreachable) and to escalate on this card rather than guess if it concludes read/evaluate/delete should split for the null-org case — that is a maintainer call, and holding the whole card for it would leave a live cross-tenant delete open.

Generated by Claude Code

{
  "issue": 7761,
  "status": "done",
  "premise_still_valid": true,
  "pr": "https://github.com/objectstack-ai/objectstack/pull/7797",
  "branch": "claude/issue-7761-getrule-byid-org-scope",
  "commits": [
    "0cac504d5 fix(plugin-sharing): scope getRule's by-id branch to the caller's organization",
    "40d57ec3f test(plugin-sharing): pin by-id tenant isolation for getRule/evaluateRule/deleteRule",
    "ab94e40de Merge origin/main into claude/issue-7761-getrule-byid-org-scope"
  ],
  "files_changed": [
    "packages/plugins/plugin-sharing/src/sharing-rule-service.ts (+20/-1)",
    "packages/plugins/plugin-sharing/src/sharing-rule.test.ts (+144)",
    ".changeset/sharing-rule-byid-org-scope.md (+39)"
  ],
  "summary": "Premise confirmed live on main before implementing: the by-id branch was still a bare `where: {id: idOrName}` with no org predicate (re-located by symbol, found at :284 as filed). The by-id lookup now routes through the predecessor's `adminOrgScope` helper rather than a second scoping helper, so an admin read is `id = {id} AND (organization_id = {orgId} OR organization_id IS NULL)` when the caller carries an organization and unfiltered when it does not. deleteRule and evaluateRule inherit it through getRule, as the card predicted. Another organization's rule is now indistinguishable from a nonexistent one: getRule returns null (REST 404), evaluateRule throws RULE_NOT_FOUND, deleteRule is a silent no-op that touches neither the row nor its grants. Keeping delete a no-op rather than a throw is deliberate — it preserves delete idempotency and denies the caller an existence oracle for other tenants' ids.",
  "callers_audit": "VERIFIED on this branch, not inherited from #7676. Only three callers pass a non-system context, and all three are the REST routes under fix: rest-server.ts:9268 (GET), :9287 (DELETE), :9305 (POST evaluate), each passing the resolved auth context. Every other caller passes a system context with no organization and therefore takes the unfiltered branch unchanged: sharing-plugin.ts:86 (boot reconcile) and :352 (rebind) pass {isSystem: true}; evaluateAllRulesForObject passes SYSTEM_CTX; bulk-recompute.ts and rule-hooks.ts never call getRule by id at all, reaching rules through listRules / evaluateAllForRecord; and the two dogfood callers (showcase-bu-hierarchy-sharing.dogfood.test.ts:80, showcase-d3-d4-capabilities.dogfood.test.ts:89) resolve BY NAME under a system context. Conclusion matches the #7676 audit's claim: no caller depends on by-id reaching an arbitrary org's row.",
  "platform_global_delete_decision": "NOT implemented — escalated as instructed, and filed as its own issue #7795 so the question survives this card closing. Implemented only that a platform-global (organization_id = null) row stays REACHABLE by id, for symmetry with the by-name path #7760 shipped. Two facts measured while deciding to escalate rather than guess, both in #7795: (1) the delete is impermanent — bootstrapDeclaredSharingRules re-seeds at every boot (sharing-plugin.ts:568) and the backfill re-materialises grants, so deleting a seeded rule is a cross-tenant grant revocation until the next restart, after which the rule returns under a NEW id; that is an outage profile, not an administrative change. (2) The safe lever is unavailable while the destructive one is not — defineRule's existence lookup is deliberately strict, so an org admin's `active: false` creates its own org-stamped row and leaves the shared rule running; deleting it is the only lever an org admin actually has over a platform-global rule. As the dispatch notes, `manage_sharing` is declared `scope: 'org'` in packages/spec/src/security/capabilities.ts:59 while `manage_platform_settings` and `manage_metadata` are `scope: 'platform'` and a `platform_admin` position exists (ADR-0068 D2) — the vocabulary already expresses the distinction, but assertCanManageRules accepts the org and platform capabilities interchangeably and never consults `scope`. Recommendation recorded in #7795: option B (read and evaluate stay open, delete requires platform authority), with option C (refuse null-org delete for all non-system callers) as an acceptable simpler ruling. Explicitly NOT acted on. Note this PR neither introduces nor widens the exposure: after #7760, deleteRule(name, orgAdmin) already reaches a platform-global row by name on main today.",
  "tests": "Full plugin-sharing suite on top of merged origin/main: `pnpm --workspace-concurrency=2 --filter @objectstack/plugin-sharing test -- --maxWorkers=2` => 'Test Files 16 passed (16) / Tests 436 passed (436)', EXIT=0 (8 new). `pnpm --filter @objectstack/plugin-sharing typecheck` (tsc --noEmit) EXIT=0. Gates on this card's surface: `node scripts/check-engine-double-contract.mjs` => 'OK — 150 pinned, 133 in the DEBT ledger, 2 exempt' (the fake engine is untouched — its #7676 conjoining $or fix is intact and my scope predicate depends on it); `node scripts/check-nul-bytes.mjs` => 'OK (scanned 7129 text file(s) ... no raw ASCII control bytes)'. Dependency closure built first with the prefix-free upstream filter `--filter '@objectstack/plugin-sharing^...'`, and rebuilt after the main merge pulled packages/spec source, before any result was trusted. New [#7761] block pins both directions: unreachable-by-id across read/evaluate/delete; platform-global reachable for read AND evaluate; the caller's own row still readable, evaluable and DELETABLE (positive control, so no test passes by returning nothing); no-org BOOT context still resolving all three rows. Trap 1 respected — org contexts hand-modelled on #7676's ORG1_ADMIN/BOOT pattern, never this file's SYS (which carries organizationId 'org1' and would have defeated the fixtures). Trap 2 respected — no HTTP-layer test written, since bootStack cannot mint an org-scoped admin (#7762) and such a test would be green against the unfixed code.",
  "ablation": "Direction predicted BEFORE running and recorded in the test block's header comment: restoring `where: { id: idOrName }` should turn exactly the three 'unreachable by id' tests red while the platform-global, own-org, BOOT and fixture-integrity pins stay green. Measured exactly that — 3 failed / 433 passed, and the failures are for the right reasons rather than incidentally: read => 'expected { ...(15) } to be null' (the victim's full row came back); evaluate => 'promise resolved \"{ ...(6) }\" instead of rejecting' (it reconciled the other tenant's grants); delete => 'expected undefined to be truthy' — the victim's rule row was GONE, i.e. the cross-tenant deletion reproduced live under the unfixed code. The entire [#7676] block stayed green throughout, so this change does not disturb what the predecessor shipped. Fix taken out with `git checkout origin/main -- <path>` and restored with `git checkout <branch> -- <path>`; git stash never used. Suite re-run after restore: 436/436 green.",
  "neighbors": "#7729 (rule-hooks.ts / primary-bu-projection.ts, same package) had NOT landed at final push — origin/main was re-fetched and merged at ab94e40de, which brought only #7769 (platform-objects/plugin-auth), #7766 (pm-dispatch SKILL) and #7757 (spec i18n). No overlap with sharing-rule-service.ts or sharing-rule.test.ts, and rule-hooks.ts was never touched by this PR. Because the merge pulled packages/spec SOURCE, the dependency closure was rebuilt and the suite plus typecheck re-run green afterwards rather than trusting the pre-merge result. If #7729 lands before this PR does, its effect on grant-materialisation timing is worth a re-run of the delete-side assertions, which are the ones that read sys_record_share.",
  "escalations": [
    "Filed as #7795 (unassigned, labels: security / domain:identity / needs-user-decision) — 'may an org-level sharing admin DELETE a platform-global (organization_id = null) sharing rule?'. Four options costed on the three axes with a recommendation of B; explicitly not implemented here. Filed as a standalone issue rather than only as a comment so the open decision is not lost when this card closes on merge."
  ],
  "deviations": [
    "The dispatch said the PR body must include `Fixes #7761`, while the standing dev contract says to use `Part of #<n>` when a half of the card is still a pending decision. Reconciled rather than silently picking a side: used `Fixes #7761` as dispatched — the filed defect (the by-id hole) is wholly fixed — and made the undecided second axis durable by filing it as its own issue (#7795), cross-linked from both the card and the PR, so closing #7761 cannot lose it. Flagging in case the PM would rather I had used `Part of`; that is a one-line body edit.",
    "Reported at draft-PR time per the standing contract; CI had not converged when this was written. No idle polling was done — CI convergence, the ready-flip and landing are the PM's. A red gate comes back to me as a patch round."
  ],
  "changeset": ".changeset/sharing-rule-byid-org-scope.md — patch bump on @objectstack/plugin-sharing, English, stating the cross-tenant consequence plainly (read / evaluate / delete-with-grant-purge), the preserved behaviours (no-org contexts, platform-global reachability), and the resulting non-oracular semantics. No skip-changeset label: this is a user-visible security fix that ships a changeset."
}

Generated by Claude Code

Review: ACCEPT — PR #7797 (PM session session_01BVc1ekPpi6yaWywAUhfzfd; diff reviewed line-by-line).

The fix is one predicate, and it reuses the predecessor's helper rather than growing a second one — this.adminOrgScope({ id: idOrName }, orgId). adminOrgScope's docblock now carries both halves of its own history (why defineRule stays strict, why by-id is now scoped), which is the right place for it.

The tests pin the harm, not the symptom. The delete case asserts the victim's sys_record_share grants survive, not merely that the rule row does — a fix that kept the row and still purged the grants would have passed a row-only assertion while leaving the actual damage. The evaluate case asserts the same thing, correctly treating evaluate as a write. And the fixture materialises the victim's grants under BOOT, so it does not depend on the very scoping decision under test — that is the kind of fixture-independence that is easy to skip and expensive to miss.

Ablation reproduced the vulnerability live rather than merely failing an assertion: under the reverted code the victim's rule row is gone (expected undefined to be truthy), the other tenant's grants were reconciled by an unauthorized evaluate, and the full row came back on read. Three red, exactly as predicted; the whole [#7676] block stayed green, so the predecessor's shipped behaviour is undisturbed.

Non-oracular on both refusal paths, checked: getRule → null → 404 and evaluateRule → RULE_NOT_FOUND answer identically for "another tenant's id" and "no such id", and deleteRule's silent no-op keeps delete idempotent without leaking existence. The read/delete envelope asymmetry (404 vs 204) is pre-existing idempotent-delete convention, not something this PR introduces.

On the flagged Fixes vs Part of question — Fixes #7761 is correct, no edit needed. The defect as filed is the missing org predicate on the by-id branch, and that is wholly fixed. The second axis was never part of this card's defect; it is a question this card surfaced, and filing it as its own issue (#7795) rather than leaving it as a comment is exactly right — a decision recorded only in a thread dies when the thread closes. Escalating instead of guessing was the instruction and it was followed to the letter.

#7795 is the more valuable artifact of the two. Its two measured facts reframe the question: the delete is impermanent (re-seeded at boot under a new id), so it is a cross-tenant grant outage rather than an administrative removal; and the safe lever is the unavailable one — an org admin cannot deactivate a platform-global rule (that creates their own row) but can delete it. Recorded for the maintainer with four costed options; this seat concurs with the recommendation but the ruling is not mine to make.

Suites green on merged main: plugin-sharing 436 (8 new), typecheck clean, check:engine-double-contract OK (fake engine untouched — the #7676 $or-conjoining fix it depends on is intact), check:nul-bytes OK. No HTTP-layer test written, deliberately, because #7762 makes one structurally non-discriminating — the correct call, and the second time this shift that gap has shaped a deliverable.

Next: flipping #7797 ready and enabling auto-merge. If #7729 lands first, its grant-materialisation timing change touches the delete-side assertions here — the merge queue's full suite is the backstop, and the dev flagged it.

Generated by Claude Code

fix(plugin-sharing): scope getRule's by-id branch to the caller's org…

Navigation Menu

Repository navigation

plugin-sharing: `getRule`'s by-id branch has no org filter — an org admin can read, evaluate and DELETE another organization's sharing rule (and its grants) #7761

Symptom

Not a regression from #7676

Suggested direction

Source

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

plugin-sharing: getRule's by-id branch has no org filter — an org admin can read, evaluate and DELETE another organization's sharing rule (and its grants) #7761

Description

Symptom

Not a regression from #7676

Suggested direction

Source

Activity

os-help commented on Aug 11, 2026

os-help commented on Aug 11, 2026

os-help commented on Aug 11, 2026

os-help commented on Aug 11, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

plugin-sharing: `getRule`'s by-id branch has no org filter — an org admin can read, evaluate and DELETE another organization's sharing rule (and its grants) #7761