Skip to content

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

Description

@os-warren

Filed by the domain:services PM seat on behalf of the os-dev seat that produced PR #16148 (card #15981). ⛔ That seat hit the API rate limit mid-dedup and correctly returned the finding rather than opening a card blind; this seat completed the dedup and is filing it. ⛔ Priority is triage's.

The finding

packages/plugins/plugin-approvals/src/approval-service.ts, isOverrideActor — the tenant arm still reads:

positions.includes(BUILTIN_IDENTITY_ORG_OWNER)
positions.includes(BUILTIN_IDENTITY_ORG_ADMIN)

⇒ the same name-as-authority species #15981 was filed for. PR #16148 closed the four platform-authority sites; ⛔ it deliberately did not widen into this one, and that restraint was correct — the card fenced it to four.

⚠️ Why this is not obviously harmless

#15948's own pin header records the mechanism: a delegate holding manageAssignments passes assertAssignmentWrite's boundSets.every(...) vacuously for a position that carries no position-bound set.

⇒ Such a delegate — who is not a full tenant admin — may be able to mint a sys_user_position row spelled org_owner, and thereby gain tenant-admin override authority over approvals in that organization.

⛔ NOT established — this is a reading, not a drive

Neither the #16148 seat nor this seat drove it. #15981's own instruction applies with full force here:

⛔ Do not verify this class by string search. #15948's instance was written through the constant BUILTIN_IDENTITY_PLATFORM_ADMIN, so a grep for the string literal returned zero on both the broken and the fixed head.

Step 1 for whoever takes this is to drive it, with a real sys_user_position row resolved through the actual resolveUserAuthzGrants, exactly as #16148 did for its four:

  1. Does a delegate holding only manageAssignments actually clear assertAssignmentWrite for an org_owner-spelled position? (The vacuous-boundSets claim is fix(plugin-auth)!: session payload positions[] is the security axis, not the better-auth role scalar #15948's, carried here — ⛔ not re-measured.)
  2. If so, does the resulting row actually move isOverrideActor's verdict?
  3. ⚠️ A site that turns out already gated upstream is a finding, not a failurefix(sharing,approvals,runtime,security): platform authority is the posture RUNG, never a name in positions[] #16148's fourth site was exactly that (plugin-security was already gated; what the name-read reached was a misreport in the explain panel, not an admission). Report per step rather than concluding from the shape.

⭐ And note the trap #16148 named: a rung-first read is no protection when it is one arm of an OR"an OR is only as strong as its weakest arm." Check the whole predicate, not just that a rung appears in it.

Fix shape, if driven and confirmed

Match what #15948 and #16148 landed: read the rung, not the name. ⚠️ The platform sites resolve to grants.posture === 'PLATFORM_ADMIN'; the tenant equivalent is not the same expression, so ⛔ do not copy that literal across — establish what the tenant-authority rung actually is before writing it.

Dedup — done, with a firing control

Searched and not a duplicate. The nearest neighbours, both checked:

⭐ The zero-match is a reading rather than a silent zeroing: a control query for #15972's own subject returned #15972 itself as the top hit, open, so the search channel was live at the time of filing.

Refs: #15981 / PR #16148 (the four platform sites) · #15972 (write side) · #15943 · #15136 / PR #15948 (the instance and the pin shape)

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions