Skip to content

fix(approvals): isOverrideActor resolves TENANT-admin standing from the ADR-0095 rung, never from a position name - #18252

Merged
claude[bot] merged 3 commits into
mainfrom
claude/issue-16166-override-actor-tenant-arm-rung
Sep 15, 2026
Merged

claude[bot] merged 3 commits into
mainfrom
claude/issue-16166-override-actor-tenant-arm-rung

Conversation

@claude

@claude claude Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Fixes #16166

Clause-②: no

ApprovalService.isOverrideActor's TENANT arm derived override authority from a built-in identity NAME on current_user.positions — the half PR #16148 deliberately left when it closed the PLATFORM arm of the same predicate. This lands the tenant half the same way: read the rung, never the name.

Step 1 was to DRIVE it — the three questions, in order, each with a reading

The card and the triage comment both required the mechanism be measured before anything was built, and required each step be reported rather than concluded from shape. It was, on the tree at 68fea8bc (the branch base).

Q1 — can a manageAssignments-only delegate mint an assignment row spelling org_owner? Measured on both halves, in a throwaway harness (no permanent file: #15972's landed suite already pins this door).

  • The ADR-0090 D12 delegated-admin gate approves it, for org_owner and org_admin alike — the vacuity fix(plugin-auth)!: session payload positions[] is the security axis, not the better-auth role scalar #15948 recorded, re-measured here for the org names rather than carried. Control: the same gate refuses when the scope does not carry manageAssignments.
  • The write is refused anyway, one layer up, by plugin-security's reserved_identity_position object validation on a real engine over a real SQL driver — VALIDATION_FAILED, for both names. Negative control: an ordinary position name (org_manager) still writes, so the object is not simply refusing everything.

The minting route the card names is gated upstream. That is a FINDING, not a failure — and it is exactly the shape the triage comment reserved that word for. Two neighbouring routes were checked and are closed too: sys_member.role is a closed, write-enforced select whose HTTP surface is read-only, so the projection cannot be poisoned from there either.

Q2 — does such a row, once it exists, move isOverrideActor's verdict? Yes. Measured through the REAL resolveUserAuthzGrants with the actor resolved from stored rows, on three doors — decideNode, recall, and the console's participant-visibility read. All three admitted a non-slate, non-submitter actor whose resolved posture was MEMBER and who held no org-administration capability at all. The pin's premise legs assert exactly that separation before any door is driven, so the result cannot be an artifact of the actor accidentally holding standing.

Q1 and Q2 together are why the reader still has to be fixed. The write-side ruling refused NEW writes and explicitly declined a migration, so a row predating it resolves into positions[] on every request; and a reader that trusts a name is not an invariant in any case, which is the whole reason the write door was built.

Q3 — is any site already gated such that the name-read reaches only a misreport? No. The console read here is not an explain panel: it returns the rows, with can_override set.

The tenant rung, established from the resolver's own source — ⛔ not copied across

The platform arm's expression is deliberately not reused. ADR-0095 D3 resolves the rung in derivePosture (packages/core/src/security/posture-ladder.ts) from held CAPABILITY grants: PLATFORM_ADMIN from the unscoped admin_full_access grant, and TENANT_ADMIN from ORGANIZATION_ADMIN_GRANTS.some(n => permissions.includes(n)) and from nothing else — the identical expression the predicate's second arm already spelled. packages/spec/src/identity/eval-user.zod.ts declares those two grants "the source of truth for the TENANT_ADMIN posture rung".

⇒ The question the card left open — whether the ORGANIZATION_ADMIN_GRANTS capability arm is itself a tenant-authority rung or another weak arm — resolves to rung. It and the derived posture are one authority read in two spellings, kept apart only so a transport that never resolved posture still reads the held grant. Both survive.

The two NAMES are the other thing entirely: ADR-0068 D2 declares them "a normalized PROJECTION into current_user.positions" whose sources of truth live elsewhere, while the same array also carries ADR-0057 D4 assignment values. A projection is not an authority. Both name arms are removed, and with them the predicate's last read of positions[] on either rung.

The whole predicate was read, not just the arm. posture === 'TENANT_ADMIN' sat first in that OR and protected nothing — an OR is only as strong as its weakest arm.

What does not change

The #3424 stuck-approval escape hatch is intact for anyone who actually holds org-admin standing: a genuine organization_admin grant still overrides, still only within its own organization, and the decision is still audited as via_override. Three CONTROL legs assert that on all three doors, and they were green before this change as well as after — they are the floor, not the result.

Verification

Reverse verification, both legs taken from COMMITTED states. The pin was committed RED first (bd157492c), then the fix (d40b9afd5):

  • pin at bd157492c (fix absent): Tests 10 failed | 6 passed (16)
  • pin at d40b9afd5 (fix present): Tests 16 passed (16)

The 6 that passed RED are the premise legs and the three controls — i.e. the harness was already discriminating before the fix, and the 10 failures are the escalation itself, not a broken harness.

Package suites (@objectstack/plugin-approvals): pnpm testTest Files 46 passed (46) · Tests 754 passed (754); pnpm typecheck → exit 0, including check:test-typecheck.

Gate families — derived mechanically from the change set by node scripts/pm/dispatch-gates.mjs, then reconciled with --ran carrying each recorded exit code: 73 derived · 70 run green · 3 NOT MEASURED · 0 UNRUN. The three are check:dual-build-cjs-loads, check:i18n and check:type-check-debt, each exit 3 = PREREQUISITE NOT MET (they read a full-repo build that this container does not hold). ⛔ Exit 3 is not a pass; those three are CI's.

check:engine-double-contract failed first (exit 1) because the new pin declares its own update/delete doubles and the ledger had not learned about the file. Regenerated with --write — additive only, 2 rows added, 0 lost — and the gate is green at 783180eba.

Repo-wide lint: pnpm lint (eslint . --no-inline-config) run in full, exit 0, 91s, at 783180eba. No narrowing was used, so no narrowing evidence is owed.

Base freshness: branched from 68fea8bca; four commits have landed on main since and none touches plugin-approvals, plugin-security or the authz resolver, so main was not merged in. The merge queue rebuilds this as merged onto current main anyway.

Clause-②: no — derived from the DELIVERED diff, with controls

Derived by REACHABILITY FROM THE PUBLISHED ENTRY (the barrel's re-export list plus this package's exports / files), ⛔ not from the word export and ⛔ not from a bundle grep. The package was built at the branch base and at HEAD and the two dist/index.d.ts were compared:

  • The only delta is TSDoc prose attached to private isOverrideActor;. That declaration line is byte-identical, and it carries no signature to widen — nothing was added, removed or narrowed on the published surface.
  • Positive controlApprovalService: 43 occurrences in both builds, so the artifact really is the published surface and the reading discriminates.
  • Negative controlBUILTIN_IDENTITY_ORG_OWNER: 0 in both builds. The constants this diff stopped importing were never reachable from the published entry, so dropping the import moves nothing published.
  • Negative control — a helper local to the new test file: 0, so the test contributes nothing to the entry.

The behavioural direction is a NARROWING — an authority path removed — which is the opposite of the widening tell. The changeset is patch, on @objectstack/plugin-approvals.

The before/after measurement mutated the service file on disk and restored it; the restore is evidenced by git hash-object equalling the HEAD blob (bbae6c8f…) and by an empty git diff HEAD, and dist/ was rebuilt at HEAD afterwards so nothing is left standing at the base build.

Acceptance notes

Two observations, noted and ⛔ not filed — neither is a reproducible defect, a contract violation, or a metadata-authoring trap:

  • plugin-security's sys_invitation_org_admin RLS policy takes a positions domain on the same two built-in names. It is a different kind of read — a row-visibility WIDENING inside an already org-scoped select, with its rationale written out at the declaration (the domain only ever widens, so a principal it does not match fails closed). Its reach is bounded by Layer 0 and it grants no override authority. Successor: any future sweep of built-in-name reads.
  • The delegated-admin gate's boundSets.every(...) is vacuous for a position that distributes no permission set. That is a real property, it is already recorded in plugin-security's own landed suite as the reason the gate cannot be where a reserved name is refused, and it is not this card's to change. Successor: A tenant can mint a sys_user_position row spelling any built-in identity name — PR #15948 closed every reader, nothing stops the row #15972's write-side lane.

⛔ Per the card's family this body carries no reproduction recipe; the driving harness lives in the test suite, which is where it belongs.

Authored by Claude Code in session https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj — recorded here in prose because this body's trailing footer block is the platform's to write.


Generated by Claude Code

…rg built-in name still confers override

Step 1 of the card: measure before deciding what to build. This harness
resolves a principal through the REAL `resolveUserAuthzGrants` and drives
three doors (`decideNode`, `recall`, `listRequests`) for a non-slate,
non-submitter actor in the request's own organization.

Committed RED on purpose, so the fix's before/after readings are taken
from committed states rather than from a working tree.

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
…r a position NAME

`isOverrideActor`'s tenant arm ORed `positions.includes(org_owner)` and
`positions.includes(org_admin)` onto the capability rung. `positions[]`
carries ADR-0057 D4 assignment values alongside the ADR-0068 D2
projection, so a stored row spelling either name conferred tenant-admin
override authority on approvals inside that organization — measured on
three doors with the harness committed in the previous commit.

The tenant rung is established from the resolver's own source rather
than copied from the platform side: ADR-0095 D3's `derivePosture`
resolves `TENANT_ADMIN` from `ORGANIZATION_ADMIN_GRANTS` and from
nothing else, and the spec declares those grants that rung's source of
truth — so the capability arm IS an authority read, and only the two
name arms go.

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
…-arm pin

The new pin declares its own `update`/`delete` doubles through the
engine's dispatch predicates, so `check:engine-double-contract` needs the
ledger row or the file is never protected. Additive only: 2 rows added,
0 lost.

Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx (via ApprovalService (symbol, a top-level class))
  • content/docs/permissions/system-context.mdx (via isOverrideActor (symbol, a method of class ApprovalService))
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.

Coarse fallback — 6 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 cb648cb1b29f12bfbb44fae66510dbb9d2ca8b8dpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 87c0341ede5395bafe9e64ed87358216798b2c89 — the merge of head 783180eba89e942b9103830303c2475d48e5593e into base cb648cb1b29f12bfbb44fae66510dbb9d2ca8b8d, 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 87c0341ede5395bafe9e64ed87358216798b2c89 && git checkout 87c0341ede5395bafe9e64ed87358216798b2c89
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin cb648cb1b29f12bfbb44fae66510dbb9d2ca8b8d 783180eba89e942b9103830303c2475d48e5593e && git checkout -B drift-repro cb648cb1b29f12bfbb44fae66510dbb9d2ca8b8d && git merge --no-ff 783180eba89e942b9103830303c2475d48e5593e

node scripts/docs-audit/affected-docs.mjs --json cb648cb1b29f12bfbb44fae66510dbb9d2ca8b8d

⚠️ 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 cb648cb1b29f12bfbb44fae66510dbb9d2ca8b8d → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@claude
claude Bot marked this pull request as ready for review September 15, 2026 04:34
@claude
claude Bot enabled auto-merge September 15, 2026 04:34
@claude
claude Bot added this pull request to the merge queue Sep 15, 2026
Merged via the queue into main with commit 917b87e Sep 15, 2026
49 of 50 checks passed
@claude
claude Bot deleted the claude/issue-16166-override-actor-tenant-arm-rung branch September 15, 2026 05:03
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

Development

Successfully merging this pull request may close these issues.

approvals: isOverrideActor's TENANT arm still derives override authority from a name in positions — the half PR #16148 deliberately did not take

1 participant