Skip to content

fix(plugin-security): the delegated-admin gate counts a holding only in the organization the runtime grants it - #19866

Merged
huangyiirene merged 1 commit into
mainfrom
claude/issue-19860-holdings-org-scoped
Sep 23, 2026
Merged

huangyiirene merged 1 commit into
mainfrom
claude/issue-19860-holdings-org-scoped

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Closes #19860

Clause-②: no — scopes two existing reads in the delegated-admin gate to the organization the runtime grants in; no new key on a published payload (claim 5795172088, SKILL.md:524).

What changed

packages/plugins/plugin-security/src/delegated-admin-gate.ts — the two gate reads of sys_user_position that were keyed by position NAME under a bare { isSystem: true } context now count a holding only where the runtime grants it.

Repair arm: B, "not another organization's row"

Chosen: (B). A holding stamped with a different organization never counts. An organization-less holding counts in every organization.

Evidence: packages/core/src/security/resolve-authz-context.ts step 4 reads sys_user_position by user_id under a bare system context. It then runs const org = ur.organization_id ?? null; if (org && tenantId && org !== tenantId) continue;. It keeps organization-less rows for ANY tenant, keeps rows stamped with the active tenant, and drops rows stamped with another tenant. With no tenant, it keeps everything. Arm (A) would refuse holdings the runtime grants. Arm (B) is the runtime's own rule, and holdingTakesEffectIn spells it with the same truthiness: an empty organization_id counts as organization-less.

For the blast radius, the same rule applies to the bound row's organization. The runtime resolves sys_position by name through the active tenant (tryFind(..., tenantId), so its own row plus organization-less rows), then reads bindings by position_id. The holders that a binding on row P re-composes are therefore the holdings that take effect in P's organization. Keying on the caller's organization instead would under-count when a caller binds against another organization's row id, so it was not used. An organization-less P reaches every organization, so nothing is dropped.

Measurement: assignmentAnchorsOfPosition (pre-fix tree, real ObjectQL + SqlDriver)

  • Org A's field_lead is held inside the delegate's subtree, and org B's same-named field_lead is held at bu_0_sales_east. The binding in org A was refused with position 'field_lead' is held in business unit 'bu_0_sales_east', outside the delegated subtree. That is over-refusal.
  • Org B has 501 same-named assignments. The binding in org A was refused with position 'field_lead' has more than 500 assignments. That is a spurious overCap.

Both are fixed in this PR with the same arm.

Tests

New file packages/plugins/plugin-security/src/delegated-admin-gate-holding-organization.test.ts: a two-organization real-engine fixture, 12 tests. Refusal cases assert the ADR-0112 envelope (code: 'PERMISSION_DENIED', statusCode: 403) and then the message's first sentence.

Witnesses. Each fails on the pre-fix source and passes with the fix:

  • A user whose only holding is in org B cannot self-delegate org A's same-named position. In org B, the same delegation stands.
  • A direct holding in org B does not make org A's delegated-only holding re-delegatable.
  • Org B's same-named assignments do not refuse org A's binding (outside-subtree anchor).
  • Org B's same-named assignments do not push org A's binding over the cap.

Pins. These pass on both trees:

  • A user holding the position in org A can self-delegate it.
  • An organization-less holding counts in org A (arm B).
  • An organization-less unanchored holding IS in org A's blast radius.
  • A binding against org B's row is still judged by org B's holders.
  • A single-posture caller keeps counting every holding.
  • The rule 4b anchor. It was already refused pre-fix by the caller-org subtree resolution, so it is a regression pin only.
  • A dark control.

Reverse verification, from committed 4cb682ce36: I restored the pre-fix gate source from 0e90a8d1c5 onto disk. On-disk proof: the holdingTakesEffectIn count is 0 and the positionNameById count is 2. The suite then ran red, Tests 4 failed | 8 passed (12), and the 4 failures are exactly the witnesses above. I restored with git checkout HEAD -- PATH under an EXIT trap. The blob hash equals the HEAD: blob 44f891d329, and git diff HEAD is empty. With the fix: Tests 12 passed (12).

Local runs on head 4cb682ce36:

  • pnpm --filter @objectstack/plugin-security test: Test Files 120 passed (120), Tests 2289 passed (2289).
  • pnpm --filter @objectstack/plugin-security typecheck: exit 0. The new test file is compiled by tsconfig.test.json, confirmed with --listFiles.
  • ESLint, narrowed: eslint --no-inline-config --format json on the 2 changed .ts files reports 2 files, 0 errors, 0 warnings. Population: both files are matched by eslint.config.mjs's packages/**/*.{ts,...} blocks, and neither is ignored (0 "file ignored" warnings). Invariance: the config enables no type-aware linting (no parserOptions.project, per its own header), so this diff cannot move the verdict for any untouched file.
  • node scripts/pm/dispatch-gates.mjs --commands derived 62 families. 59 exited 0. check:dual-build-cjs-loads, check:i18n and check:type-check-debt exited 3 with PREREQUISITE NOT MET (they need whole-workspace dist/), so they are NOT MEASURED and left to CI.
  • Roster families beside the path, all exit 0: check-changeset-fixed, check:authz-resolver, check:tenant-chokepoint, check:error-code-casing, check:filter-alias-parity.

Changeset: patch for @objectstack/plugin-security. Zero edits under packages/spec or content/docs/releases/**.

Acceptance notes

These are observations only. None is filed and none is reproduced as a defect.

  • The driver and the runtime disagree on an empty-string organization_id. SqlDriver.applyTenantScope keeps only = :org OR IS NULL. The runtime resolver's truthiness check treats '' as organization-less. The blast-radius read goes through the driver, so a ''-stamped holding would be left out of the radius, while the runtime would grant it everywhere. The insert path normalises '' to the tenant, so this needs a hand-written row. Not measured. Carrier: none.
  • Binding against another organization's sys_position row id. positionById reads by id under a bare system context, so the gate does not ask whether a caller in org A may bind org B's row at all. After this PR, such a write is at least judged by org B's holders, which is the fail-closed direction. Not measured. Carrier: none.

Generated by Claude Code

…in the organization the runtime grants it

Self-delegation rule 4 (activeHoldings) and the binding blast radius
(assignmentAnchorsOfPosition) read sys_user_position by position NAME under
a bare system context, so another organization's same-named holding
answered "you currently hold it", and another organization's same-named
assignments refused a binding (outside subtree / over cap).

Both now apply the runtime resolver's own rule (resolveAuthzContext step
4): a holding stamped for a different organization never counts; an
organization-less holding counts in every organization. activeHoldings is
keyed on the caller's organization; the blast radius on the bound
sys_position row's organization, with the scope in the read's context so
the cap counts only reachable holdings. An organization-less caller is
unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AhQASwqJr2Z7XfGWUdvnbF
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-security, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/permissions/delegated-administration.mdx (via DelegatedAdminGate (symbol, a top-level class))

⛔ 1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v13.mdx (via DelegatedAdminGate (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 15 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 48c91e9e460154716114c042e9bb92c05bd8dbfc → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 186a69ac0165ea623f887a1bc437f1255072e6c0 — the merge of head 4cb682ce36423cdb1ad3c79ff5cad10e182db773 into base 48c91e9e460154716114c042e9bb92c05bd8dbfc, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 186a69ac0165ea623f887a1bc437f1255072e6c0 && git checkout 186a69ac0165ea623f887a1bc437f1255072e6c0
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 48c91e9e460154716114c042e9bb92c05bd8dbfc 4cb682ce36423cdb1ad3c79ff5cad10e182db773 && git checkout -B drift-repro 48c91e9e460154716114c042e9bb92c05bd8dbfc && git merge --no-ff 4cb682ce36423cdb1ad3c79ff5cad10e182db773

node scripts/docs-audit/affected-docs.mjs --json 48c91e9e460154716114c042e9bb92c05bd8dbfc

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 48c91e9e460154716114c042e9bb92c05bd8dbfc → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 23, 2026
@huangyiirene
huangyiirene marked this pull request as ready for review September 23, 2026 13:40
@huangyiirene
huangyiirene added this pull request to the merge queue Sep 23, 2026
Merged via the queue into main with commit b940f32 Sep 23, 2026
42 of 43 checks passed
@huangyiirene
huangyiirene deleted the claude/issue-19860-holdings-org-scoped branch September 23, 2026 13:58
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

Contract review (retroactive — PR landed as b940f32 before this record was written)

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 4cb682ce36423cdb1ad3c79ff5cad10e182db773

⚠️ Why this record is late: the domain:services seat enqueued this PR without the served-tier contract review that .claude/skills/pm-dispatch/references/contract-review.md:8 owes to .changeset prose and :25 requires on file before landing. That lapse is the seat's own. The review below was run afterwards by an isolated reviewer at the served tier. It saw only card #19860, its comments, and the landed diff b940f32a56^1..b940f32a56, which is identical to the reviewed head. The seat adopts the verdict.

① Derived judgments

The changeset prose was checked claim by claim. Every factual claim is TRUE:

  • holdingTakesEffectIn (delegated-admin-gate.ts:114-118) is the same predicate as the runtime resolver's resolve-authz-context.ts:801-802, with the same truthiness on ''.
  • The rule-4 holdings path is organization-scoped (:591, :679), and organization-less holdings still count (:721, :801-802).
  • The blast radius is keyed on the bound row's organization (:832-844, :1247, :1282-1305), and the driver applies the tenant scope before the cap (sql-driver.ts:13771, :13807).
  • The premise holds: sys_position.name is unique per organization (sys-position.object.ts:357).

Clause-② value no is RIGHT: no new key, export, arm or registration.

Two citation defects:

  • The changeset attributes the "currently holds" check to ADR-0091 D3 rule 4. In the ADR (docs/adr/0091-...md:107-112) that is rule 3; rule 4 is the no-re-delegation cut. The gate's own internal "4." folds ADR rules 3 and 4 together. This text ships into CHANGELOG.
  • The PR body and claim 5795172088 cite SKILL.md:524 for the negative boundary. That line exists only in a stale charter. On origin/main the rule lives at .claude/skills/pm-dispatch/references/execution-duties.md:98. The seat wrote that citation from a stale tree; the value no stands.

② Semver level

patch on @objectstack/plugin-security is consistent with AGENTS.md:1081-1085. It is a bug fix, with no public removal or rename (positionNameById → positionById is private), so no ADR-0087 marker is owed.

③ Boundary flags

Zero packages/spec, zero content/docs/releases/**, zero CHANGELOG edits. The scope is exactly the declared file face. Both card asks are covered, and arm B is recorded on the card in 5795616364. The serial constraint was honoured.

Implemented-by: claude/issue-19860-holdings-org-scoped
Reviewed-by: session_01AhQASwqJr2Z7XfGWUdvnbF

VERDICT: PASS

Non-blocking defects:

  1. The ADR rule mis-cite in the changeset: it should read rule 3, or "rules 3–4". It needs correcting through the route AGENTS.md:697 prescribes before the release consumes the changeset.
  2. The dangling SKILL.md:524 citation, as above.
  3. Over-general wording in 「an organization-less (single) caller is unchanged」. For the blast radius, the key is the bound row's organization, not the caller's. The claim holds only because a single catalog carries no stamped rows.
  4. Already noted in the PR: the driver's = :org OR IS NULL treats '' differently from the resolver.

Generated by Claude Code

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

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants