Skip to content

fix(auth): recognize comma-joined multi-role admin in AccessControl's fast-path bypass#4926

Merged
pedrofrxncx merged 1 commit into
mainfrom
fix/access-control-multi-role-admin-fast-path-w2
Jul 21, 2026
Merged

fix(auth): recognize comma-joined multi-role admin in AccessControl's fast-path bypass#4926
pedrofrxncx merged 1 commit into
mainfrom
fix/access-control-multi-role-admin-fast-path-w2

Conversation

@pedrofrxncx

@pedrofrxncx pedrofrxncx commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Follows #4918/#4923 hardening: same failure mode, third site.

AccessControl.checkMemberAccess (apps/mesh/src/core/access-control.ts) checked this.role === "admin" || this.role === "owner" with a strict equality match. Better Auth's organization plugin joins an assigned role array with "," before storing member.role, and AccessControl.setRole() can set this.role to a path-resolved org's role (per its own docstring), so a legitimate multi-role owner/admin (e.g. "admin,billing-manager") arrives here as that comma-joined string. The exact-match check misses it and falls through to boundAuth.hasPermission, which — per builtin-role-permission.ts's own docstring — hits a DB-backed Better Auth round-trip specifically because the in-memory fast path exists to avoid "~40ms event-loop hitches" per request.

This is not a live authz bypass (the downstream Better Auth path still grants correctly for multi-role admins), but it closes the gap so AccessControl's own bypass is self-contained, matching the shared hasAdminRole helper already used by canAssignRole (#4918) and context-factory.ts's runtime bypass (#4923). Multi-role admins now take the fast in-memory path instead of an avoidable DB round-trip on every tool call.

Added a regression test asserting the multi-role admin bypasses without ever calling boundAuth.hasPermission — before the fix it fell through to the mock, which (having no stored permissions) denied.

Reviewer check: bun test apps/mesh/src/core/access-control.test.ts

Locally verified: bun run fmt, cd apps/mesh && bunx tsc --noEmit (clean), and the targeted test file (26/26 pass). Full CI validates the rest.


Summary by cubic

Fixes AccessControl to recognize comma-joined multi-role admins/owners, so they hit the fast in-memory bypass instead of a DB round-trip. This prevents unnecessary permission checks and improves request latency.

  • Bug Fixes
    • Use shared hasAdminRole to detect admin/owner in checkMemberAccess, including values like "admin,billing-manager".
    • Add regression test ensuring multi-role admins bypass without calling boundAuth.hasPermission.

Written for commit bcb2716. Summary will update on new commits.

Review in cubic

… fast-path bypass

AccessControl.checkMemberAccess checked `this.role === "admin" || this.role
=== "owner"` with a strict equality match. Better Auth's organization plugin
joins an assigned role array with "," before storing member.role (and
`setRole()` can set `this.role` to the path-resolved org's role, per its own
docstring), so a legitimate multi-role owner/admin (e.g.
"admin,billing-manager") arrives here as that comma-joined string. The
exact-match check misses it and falls through to `boundAuth.hasPermission`,
which — per builtin-role-permission.ts's own docstring — hits a DB-backed
Better Auth round-trip specifically to avoid ("~40ms event-loop hitches").
Same failure mode already fixed for `canAssignRole`'s caller check (#4918)
and the runtime bypass in context-factory.ts (#4923), now fixed here via the
shared `hasAdminRole` helper in auth/roles.ts.

Not a live authz bypass — the downstream Better Auth path still grants
correctly — but this closes the gap so AccessControl's own bypass is
self-contained and multi-role admins take the fast in-memory path instead of
an avoidable network round-trip on every tool call.
@pedrofrxncx
pedrofrxncx merged commit 9a2c5ca into main Jul 21, 2026
15 checks passed
@pedrofrxncx
pedrofrxncx deleted the fix/access-control-multi-role-admin-fast-path-w2 branch July 21, 2026 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant