require_role implements a threshold but reads as set membership #71

New issue

Closed

opened 2026-07-28 05:57:15 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-07-28 05:57:15 +00:00

Severity: LOW (latent trap, not currently exploitable)

The bug

backend/app/auth/session.py:65-73 computes:

min_required = min(role_order[r] for r in roles)

It takes the minimum of the supplied roles and treats the hierarchy as monotonic. So
require_role(UserRole.reviewer, UserRole.admin) is exactly equivalent to
require_role(UserRole.reviewer).

Every current call site happens to be monotonic, so nothing is broken today. But the variadic
signature reads like a set-membership test while implementing a threshold. A future call such
as require_role(UserRole.viewer, UserRole.admin) — intending "viewers or admins, but not
reviewers" — would silently admit reviewers.

Why it matters

#22 will expand the role model, which is exactly when this misfires, and there are no
authorization tests to catch it.

Fix

Rename to require_min_role(role) taking a single role, making the threshold semantics
explicit — or implement true set membership if that is the intent. Update all call sites.

Done when

The helper's name matches its semantics
All call sites are updated and reviewed for intent
A test covers a non-monotonic role set

References

backend/app/auth/session.py:65-73

## Severity: LOW (latent trap, not currently exploitable) ## The bug `backend/app/auth/session.py:65-73` computes: ```python min_required = min(role_order[r] for r in roles) ``` It takes the **minimum** of the supplied roles and treats the hierarchy as monotonic. So `require_role(UserRole.reviewer, UserRole.admin)` is exactly equivalent to `require_role(UserRole.reviewer)`. Every current call site happens to be monotonic, so nothing is broken today. But the variadic signature *reads* like a set-membership test while implementing a threshold. A future call such as `require_role(UserRole.viewer, UserRole.admin)` — intending "viewers or admins, but not reviewers" — would silently admit reviewers. ## Why it matters #22 will expand the role model, which is exactly when this misfires, and there are no authorization tests to catch it. ## Fix Rename to `require_min_role(role)` taking a single role, making the threshold semantics explicit — or implement true set membership if that is the intent. Update all call sites. ## Done when - [ ] The helper's name matches its semantics - [ ] All call sites are updated and reviewed for intent - [ ] A test covers a non-monotonic role set ## References - `backend/app/auth/session.py:65-73`

claude-bot added this to the v0.1.1 milestone

2026-07-28 05:57:15 +00:00

claude-bot added the

backend

security

labels

2026-07-28 05:57:15 +00:00

claude-bot referenced this issue

2026-07-28 06:07:54 +00:00

Add API contract and authorization tests for the Phase 1 surface #8

rbrooks referenced this issue from a commit

2026-07-28 20:24:13 +00:00

Auth hardening: role thresholds, bootstrap window, discovery transport

rbrooks closed this issue

2026-07-28 20:24:13 +00:00

claude-bot commented

2026-07-28 20:40:45 +00:00

Author

Fixed in 4333b5e. CI green.

Replaced with require_min_role(minimum) taking a single role, and all 14 call sites migrated.
ROLE_ORDER is now a module-level constant rather than rebuilt inside the check on every request.

Tested across the full 3x3 role matrix (viewer/reviewer/admin against each threshold), so the
semantics are pinned rather than assumed. The specific case that would have misfired —
require_role(viewer, admin) silently admitting reviewers — is now unrepresentable, because the
signature no longer accepts a set.

**Fixed** in 4333b5e. CI green. Replaced with `require_min_role(minimum)` taking a **single** role, and all 14 call sites migrated. `ROLE_ORDER` is now a module-level constant rather than rebuilt inside the check on every request. Tested across the full 3x3 role matrix (viewer/reviewer/admin against each threshold), so the semantics are pinned rather than assumed. The specific case that would have misfired — `require_role(viewer, admin)` silently admitting reviewers — is now unrepresentable, because the signature no longer accepts a set.

rbrooks referenced this issue from a commit

2026-08-02 06:52:06 +00:00

Derive the authorization matrix from the route table

claude-bot referenced this issue

2026-08-02 06:52:33 +00:00

Add API contract and authorization tests for the Phase 1 surface #8