Repository navigation
security(rbac): superadmin is administrative but is not a Role member and holds no permissions #13854
Description
Activity
- added 7 commits that reference this issue
on Aug 24, 2026 Delivered and merged as
774527f5b(PR #14936).- CI: all 10 required contexts present and green — checked by name, not by tally. 58 success / 3 skipped / 0 failures / 0 pending, deduped to the latest run per name.
- Review: independent security-focused
code-reviewerpass. Every claim verified against source; it could not falsify any.
superadmindid not resolve two ways — it resolved four.role_has_permissionshort-circuited onis_admin_role, which madeADMIN_ROLES— documented as a predicate — silently the most permissive permission source in the system, whileSecurityLayer.check_permissiondenied it those same permissions.Role("superadmin")raised outright, andrequire_roleadmitted it by raw string. Four answers for one input.The decision:
Role.SUPERADMINwith an explicitly EMPTY permission entry. Verified in base:Role.SUPERADMIN: [],
The alternative was +54 permissions including
allow_shell_execute, and it is recorded in the source as a one-line change flagged as needing its own security review and a data migration — which an empty entry does not. That is the security carve-out to the union doctrine: union the checks, never the allowances.Review confirmed nothing grants it implicitly —
_build_system_roles()yields a safepermissions: []seed row,auth_rbac._get_user_permissionsresolves the member but still gets[],SecurityLayer._get_default_role_permissionshas nosuperadminkey, andRoleUpdateRequest's hand-written regex keeps it non-assignable via that API. The 17require_rolecall sites are unchanged and still admit it.The one behaviour change is a tightening, stated rather than buried. Removing the short-circuits refuses superadmin at two granular-permission gates.
adminis unaffected and that is measured:Permissionhas exactly 54 members andROLE_PERMISSIONS[Role.ADMIN]lists exactly those 54, now asserted continuously. A second tightening ships with zero lines changed in its file — SLM's_resolve_rolepreviously downgraded an unknownsuperadmintoRole.USER, i.e. granted it USER's 15 permissions.Two deliberate forks were found and deliberately NOT collapsed. #14024's own body says its three vocabularies are correctly separate concepts, and their member sets confirm three different questions. And
write_guard's{platform_admin, superadmin}excludesadminon purpose — the guard exists to stop an org admin reaching a parent org's KB, so folding it intoADMIN_ROLESwould have addedadminand destroyed the property. A drift guard now compares member sets and pins the exact known overlaps.A miss worth recording. Three tests asserting the old contract survived the first sweep, because that sweep grepped
Role/ROLE_PERMISSIONSsymbols while those files namesuperadminas a bare string through_would_deny/_role_satisfies— searching the symbol rather than the behaviour, which is core rule 7 and the exact trap #14024 exists over. Re-swept on the concept: 21 files, 2 genuinely broken, 16 exercising unchanged admission paths, 3 incidental. The shadow test's intent was preserved rather than flipped — its purpose was that an administrative role must never be reported as an unrecognised string, so that is now asserted directly instead of inferred fromis None.All 12
python-suiteshards pass, including shard 4 — which resolves the open caveat that the other eleven had been cancelled rather than passing on the prior head, so more failures of that class could have been hiding. There were none.Residuals filed and cross-linked: #14937, #14938, #14939, #14940, #14944.
- added a commit that references this issue
on Sep 13, 2026 - added 5 commits that reference this issue
on Sep 24, 2026
Found while implementing #13820, and deliberately not fixed there.
Problem
superadminis administrative —ADMIN_ROLES = frozenset({"admin", "superadmin"}),is_admin_role()returns True for it, and 17+ endpoints gate on
require_role("admin", "superadmin"). But it is nota
Roleenum member, soRole("superadmin")raises and it holds no entry inROLE_PERMISSIONS.The module already names this:
Why it is filed rather than fixed
The first draft of #13820 mapped
superadminonto admin's permission set viaADMIN_ROLES. Reviewmeasured the effect: +54 permissions, including
admin.system,security.manage,admin.users.writeandallow_shell_execute— none of which it previously held through any path.That reads as a bug fix and is an authorisation expansion. #13820 decided which resolver is
authoritative; it did not decide to widen a role. Those are different changes and the second should
not ride along inside the first.
Two further reasons the
ADMIN_ROLESroute is wrong even if the expansion is wanted:ADMIN_ROLESis documented as the answer to "is this role administrative?" — a predicate forrequire_role, not a permission source. Coupling them means any future addition to thatfrozenset silently becomes full admin in the live gate, shell execution included, with no edit
to
ROLE_PERMISSIONSand no review of the security layer.The consequence today
superadminresolves to[]in the canonical mapping, so it is denied everymcp.*grant andeverything else
ROLE_PERMISSIONSassigns. Whether that is correct depends on whatsuperadminismeant to be — which is the decision.
Note it also caused a live misreport in #13228 stage 2, where the shadow logged the most privileged
role in the system as denied on every tool. That was worked around at the consumer with
is_admin_role; the root cause is here.Decision needed
superadmina first-classRolewith its ownROLE_PERMISSIONSentry — the fix themodule itself points at. Requires deciding whether it equals admin or exceeds it.
superadmin, mapping existing users toadmin, and remove it fromADMIN_ROLES.require_roleis the only gate it passes.Acceptance criteria
superadminresolves identically throughRole,ROLE_PERMISSIONS,is_admin_roleand thelive permission gate — or is gone
ADMIN_ROLESis not a permission source; adding a member cannot grant permissionsADMIN_ROLESis aRolemember with aROLE_PERMISSIONSentryRelated
#13820 (which found it), #13228 (where it misreported), #12717/#12786 (where
ADMIN_ROLESmoved).