Repository navigation
security(rbac): two permission resolvers disagree and the live one never reads ROLE_PERMISSIONS #13820
Description
Activity
Decision (owner, 2026-08-09):
ROLE_PERMISSIONSis authoritativeauth_rbac._get_user_permissionsgets wired into the live path.ROLE_PERMISSIONSis the dict both
backends already import and where everyPermissionmember is assigned, so this is the resolver the
codebase was written around —SecurityLayerbecame the live one by accident, not by design.What this commits us to
SecurityLayer's two behaviours need re-homing, not dropping. Its wildcard matching
(files.*) and its unconditionalTruewhenenable_authis false are real semantics that
something depends on. Re-implementing the first and deciding the second explicitly is part of the
work, not a follow-up.- Case handling must survive the move.
has_permissionlowercases the role; the
ROLE_PERMISSIONSpath does not, andsuperadminis not aRoleenum member at all. That exact
gap already bit MCP tool calls bypass canonical RBAC — three parallel authorization models, default-allow blocklist #13228 stage 2, where the most privileged role in the system was reported as
denied on every tool. - Once landed,
mcp.*permissions resolve for real and MCP tool calls bypass canonical RBAC — three parallel authorization models, default-allow blocklist #13228 stage 3 unblocks.
Acceptance criteria
- The live
require_permissionpath resolves throughROLE_PERMISSIONS - Wildcard matching preserved, with a test naming a
files.*-style grant - The
enable_auth=falsebypass is either preserved deliberately or removed deliberately —
recorded either way, never lost in the move -
superadminand mixed-case roles resolve identically through the new path - A test asserting both former paths agree for every role across the full
Permissionenum -
has_permission(admin, "mcp.browser.read")returns True
Blocks: #13228 stage 3.
- added 4 commits that reference this issue
on Aug 9, 2026 Closed by PR #13853 — squash-merged to
Dev_new_guiasd75a618b1.$ git show origin/Dev_new_gui:autobot-backend/security_layer.py | grep -c canonical_role_permissions 2 $ pytest security/canonical_permissions_test.py --collect-only -q 40 tests collectedWhat the decision produced
ROLE_PERMISSIONSis now read by the live gate. The permissions #13228 declared resolve for real:before: admin mcp.browser.read = False (every role, every mcp.* grant) after: admin mcp.browser.read = True mcp.browser.control = True user mcp.browser.read = True mcp.browser.control = False readonly False False#13228 stage 3 is unblocked.
Added as a third source rather than replacing the others: SecurityLayer's configured grants, wildcard
matching, theallow_shell_executespecial case and theenable_authbypass are live semantics
something depends on, and removing them silently would trade one invisible gap for another — which is
what this issue was. Theenable_auth=falsebypass is preserved deliberately, with a test saying
so; changing it is a separate repo-wide decision.The finding underneath
auth_rbac._get_user_permissionsalready unionedROLE_PERMISSIONSwith SecurityLayer's
defaults, correctly, and had no production callers. This was never a missing implementation. It
was a correct function nobody reached, while the live path calledcheck_permissionand skipped it.Two HIGH defects review caught in my first attempt
I opened the permission gate without opening the approval gate. Normalising the role inside
canonical_role_permissionsalone created a second identity for the same role: it cleared shell
execution via the canonical set, then reached_should_force_approval— which keys on the exact
string — with an empty permission list, so no approval branch could fire.role='admin' exec_allowed=True force_approval=True # gated role='Admin' exec_allowed=True force_approval=False # NOT gated role='superadmin' exec_allowed=True force_approval=False # NOT gatedA HIGH-risk command running unapproved. Normalisation now happens once, and both gates share it.
The superadmin expansion was withdrawn. Mapping it onto admin's set via
ADMIN_ROLESgranted
+54 permissions includingadmin.system,security.manageandallow_shell_execute— none of
which it previously held. That reads as a fix and is an authorisation expansion; this issue decided
which resolver is authoritative, not that a role should be widened. Filed as #13854, which also
carries the sharper point:ADMIN_ROLESis arequire_rolepredicate, and coupling it to
permissions means any future addition to that frozenset silently becomes full admin.A pre-existing bypass fixed on the way
Chasing the above turned up the same shape already in the code:
check_permissiondowngrades
god/root/superuserto admin via_handle_deprecated_role;_should_force_approvalnever did.
Those aliases passed the exec gate through the downgrade and skipped approval entirely.base (before any change): god exec_allowed=True force_approval=False now: god exec_allowed=True force_approval=TrueVerified against the unmodified base first, so it was clear this was pre-existing rather than
self-inflicted.Also: the canonical set is now resolved from the role as given, before the alias rebinding.
Reading it after made the documented "downgrade to admin with granular permissions" a near-no-op,
handing those aliases all 54 admin permissions in place of their 8 defaults.Verification
canonical_permissions_test + auth_rbac_test + security_layer_test + admin_guard 106 passedMutation Killed by normalise only in the canonical resolver 3 tests approval gate stops normalising 6 tests resolve canonical after the alias rebinding 3 tests reinstate the superadmin expansion 3 tests One mutation survived the first pass and is worth recording: removing the shared normalisation
left all 37 tests green, because my case-insensitivity tests asserted a canonical permission and
that resolver normalises internally. The added test assertsallow_voice_speak— a permission that
exists only in the defaults — so it can only pass if all three sources agree on who a role is.Follow-up
#13854 —
superadminis administrative, is not aRolemember, and holds no permissions.
Found while reviewing #13228 stage 2 (PR #13818).
Problem
There are two permission resolvers, they disagree, and only one has production callers.
auth_rbac._get_user_permissionsROLE_PERMISSIONS(autobot_shared/auth/permissions.py)SecurityLayer.check_permissionself.roles+_get_default_role_permissions, wildcard-matchedauth_rbac.require_permission→has_permissionSo
ROLE_PERMISSIONS— the dict both backends import, and the one everyPermissionmember isassigned in — is not what the running system consults.
require_permissionresolves throughSecurityLayer, which never reads it.Why it matters now
#13228 stage 1 added ten
MCP_*members and assigned them inROLE_PERMISSIONS. None of themexists in security config or in
_get_default_role_permissions, sohas_permission(user, "mcp.browser.read")is False for every role, including admin. A stage-3flip that enforces through the live resolver would deny all MCP access; one that enforces through
ROLE_PERMISSIONSwould work but would be the only thing in the system doing so.Two further divergences in the same pair:
SecurityLayer.check_permissionreturnsTrueunconditionally whenenable_authis false.has_permissionlowercases the role; theROLE_PERMISSIONSpath does not (see thesuperadmincase, fixed in PR feat(mcp): report the canonical-RBAC verdict without enforcing it (#13228 stage 2) #13818 for the shadow only).
Decision needed
Before #13228 stage 3 can flip anything:
ROLE_PERMISSIONS— thenSecurityLayerbecomes the second-class resolver and_get_user_permissionsneeds wiring in, orSecurityLayer— then themcp.*grants must be added to security config /defaults, and
ROLE_PERMISSIONSremains decorative for MCP, orThis is a product/architecture call, not a mechanical fix, which is why it is filed rather than
patched.
Acceptance criteria
mcp.*permissions resolve identically through both paths, or the losing path is removedPermissionenumBlocks: #13228 (stage 3)