Skip to content

fix(identity): SECURITY — permission gates fail open on .RequireAuthorization() route-groups#1290

Merged
iammukeshm merged 2 commits into
mainfrom
fix/authz-permission-fallthrough
Jun 5, 2026
Merged

fix(identity): SECURITY — permission gates fail open on .RequireAuthorization() route-groups#1290
iammukeshm merged 2 commits into
mainfrom
fix/authz-permission-fallthrough

Conversation

@iammukeshm

Copy link
Copy Markdown
Member

Severity: High — Broken Access Control (OWASP A01)

A Basic tenant user can perform privileged writes they have no permission for — e.g. create/delete products.

Root cause

Permission checks are implemented by the RequiredPermission authorization policy, which reads each endpoint''s RequiredPermissionAttribute (attached by .RequirePermission(...)). That policy was wired only as the FallbackPolicy (JwtAuthenticationExtensions.cs).

In ASP.NET Core the fallback policy runs only on endpoints with no authorization metadata. But these modules call .RequireAuthorization() at the route-group level:

  • Catalog, Billing, Chat, Files, Notifications, Tickets, Webhooks

.RequireAuthorization() (no args) attaches the built-in default policy (authenticated-only). That auth metadata suppresses the fallback, so the RequiredPermission policy never runs — .RequirePermission(...) is present as metadata but never evaluated. Any authenticated tenant member passes.

Identity / Auditing / Multitenancy have no group-level .RequireAuthorization(), so their fallback runs — they were correctly enforced (which is why a Basic user''s view of Users/Roles/Groups is legitimately allowed — Basic holds those View permissions — while their Identity writes stayed blocked).

Fix

Set DefaultPolicy to the same RequiredPermission policy as the fallback. Now .RequireAuthorization() (which uses the default policy) also evaluates permissions. Endpoints without .RequirePermission metadata still just require authentication (the handler succeeds when no attribute is present). One wiring change fixes all 7 modules — no per-module edits, no change to the protected BuildingBlocks.

options.DefaultPolicy  = options.GetPolicy(RequiredPermissionDefaults.PolicyName)!; // added
options.FallbackPolicy = options.GetPolicy(RequiredPermissionDefaults.PolicyName);

Verification (red → green)

Added CreateProduct_Should_Return403_When_AuthenticatedUserLacksPermission — the gap the suite never covered (it only had admin→200 and unauthenticated→401, never authenticated-but-forbidden→403).

  • Before fix: test fails — a zero-permission user successfully creates a product (2xx, not 403). Live fail-open reproduced.
  • After fix: test passes (403). Full ProductsEndpointTests class stays green (24/24) — admin create/update/delete/restore/search unaffected.
  • Full integration suite re-run in progress as a cross-module regression check.

Notes

  • The dashboard already gates its nav by permission; Bob seeing Users/Roles/Groups is because View Users/Roles/Groups are flagged IsBasic (a design question to revisit — should Basic users see the tenant directory?), not a frontend leak.
  • Unrelated: the Integration.Tests project currently has pre-existing analyzer-as-error violations (S125, CA1812) across several files that block a clean dotnet test; I used -p:TreatWarningsAsErrors=false to run. Worth a separate cleanup.

🤖 Generated with Claude Code

iammukeshm and others added 2 commits June 6, 2026 00:26
…ts (broken access control)

Permission checks live in the "RequiredPermission" authorization policy, which reads
each endpoint's RequiredPermissionAttribute (added by .RequirePermission(...)). It was
wired ONLY as the FallbackPolicy. In ASP.NET Core the fallback policy runs only on
endpoints with no authorization metadata — but Catalog, Billing, Chat, Files,
Notifications, Tickets and Webhooks all call .RequireAuthorization() at the route-group
level. That attaches the built-in default policy (authenticated-only), which suppressed
the fallback, so .RequirePermission(...) was attached as metadata but NEVER evaluated.

Result: any authenticated tenant member could perform gated writes in those modules —
e.g. a Basic user creating/deleting products despite lacking Products.Create/Delete.
(Identity/Auditing/Multitenancy have no group-level .RequireAuthorization(), so their
fallback ran and they were unaffected.)

Fix: set DefaultPolicy to the same RequiredPermission policy as the fallback, so
.RequireAuthorization() (default policy) also evaluates permissions. Endpoints without
permission metadata still just require authentication (the handler succeeds when no
RequiredPermissionAttribute is present). One-line wiring change fixes all 7 modules.

Adds a regression test (CreateProduct_Should_Return403_When_AuthenticatedUserLacksPermission)
covering the suite's blind spot: it previously only tested admin->200 and
unauthenticated->401, never authenticated-but-forbidden->403. Verified red (2xx) before
the fix, green (403) after; the full ProductsEndpointTests class stays green (admin
create/update/delete/restore/search unaffected).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…from Basic users

CI: the Release `-warnaserror` build was already red on main — SonarAnalyzer S125
false-positives on 6 descriptive test comments (semicolon/code-like prose, not dead
code) in Integration.Tests and Architecture.Tests. Suppress S125 in those two test
projects (deleting the comments would lose useful documentation).

Dashboard: the Users/Roles/Groups nav items had no permission gate, so they showed to
everyone. Gate each on its *.Update permission. Basic users lack Update, so the
identity-admin pages hide for them — without touching View Users/Roles/Groups (those
stay IsBasic because the chat/user picker depends on Users.View).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@iammukeshm

Copy link
Copy Markdown
Member Author

Follow-on commits in this PR (per request to make it self-contained):

CI unblock (S125). The Release -warnaserror build was already red on main — SonarAnalyzer S125 false-positives on 6 descriptive test comments (code-like prose, not dead code) in Integration.Tests (4) and Architecture.Tests (2). Suppressed S125 in those two test projects rather than deleting the comments. Verified: dotnet build src/FSH.Starter.slnx -c Release -warnaserror0 warnings / 0 errors.

Hide identity-admin nav from Basic users. The Users/Roles/Groups nav items had no permission gate (nav-data.ts), so they showed to everyone. Each is now gated on its *.Update permission — Basic lacks Update, so the admin pages hide for them. Deliberately did not strip View Users/Roles/Groups from Basic: Users.View powers the chat/user picker (user-picker.tsxsearchUsers), so removing it would break Basic chat. Dashboard typecheck passes.

Known follow-up (not in this PR): the dashboard has no route guards, so a Basic user who types /identity/users directly can still load read-only data (they legitimately hold View). All writes are now blocked by the backend fix. Route-level guarding is a separate dashboard-hardening task.

Verification recap: new 403 test red→green; full integration suite 713 passed / 0 failed / 1 skipped; Release build clean; dashboard tsc clean.

@iammukeshm iammukeshm merged commit ea88513 into main Jun 5, 2026
13 checks passed
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