critical: admin audit-log endpoints have no role gate - #129
Conversation
|
Please rebase/merge the base branch into your branch and resolve the conflicts — a fresh audit will run automatically once new commits land. |
EmeditWeb
left a comment
There was a problem hiding this comment.
⚠️ Automated Audit: partial
@Godfrey-Delight Good start — please look into the gaps identified below.
The code changes genuinely address the root cause: the new AdminGuard performs server-truth authorization via UserStatusService/Supabase, falls back to Supabase direct query, logs denied attempts via AuditService, and covers all required rejection branches (non-admin 403, blocked 401, stale JWT 403, unauthenticated 401). Tests in admin.guard.spec.ts comprehensively cover every branch specified in the issue. However, the PR has merge conflicts with the base branch and cannot be merged as-is, blocking deployment of the fix.
Gaps identified:
- Merge conflicts with base branch must be resolved before this PR can be merged
- The guard falls back to trusting JWT claims (user.role) when both UserStatusService and SupabaseService are unavailable — in a degraded state this re-opens the original vulnerability
CI checks: ✅ PASSED: build-test
Merge conflicts:
Audited by stepfi-audit-bot 🤖
EmeditWeb
left a comment
There was a problem hiding this comment.
✅ Automated Audit: solves
@Godfrey-Delight Excellent work, thank you! 🎉
The PR directly addresses Issue #115's root cause by replacing an existing AdminGuard (at src/auth/guards/admin.guard.ts) with a server-truth implementation in src/modules/admin/admin.guard.ts that queries role and status from the database via UserStatusService/Supabase rather than trusting JWT claims. The guard enforces deny-by-default, throws structured 403/401 exceptions, and logs denied attempts via AuditService. Unit tests cover all required rejection branches (non-admin 403, blocked 401, unauthenticated 401, stale JWT with revoked role 403, active admin 200). CI passed. The only gap is that the audit.controller.spec.ts tests do not verify guard integration at the controller level (they test the controller method in isolation without guard context), but the dedicated admin.guard.spec.ts tests adequately cover the guard's security logic.
CI checks: ✅ PASSED: build-test
Merge conflicts: ✅ none — but the PR is blocked (failing/missing required checks or reviews).
Audited by stepfi-audit-bot 🤖
🔗 Related Issue
Closes #115
🔖 Title
critical: admin audit-log endpoints have no role gate — any authenticated wallet can read full audit trail
📝 Description
This PR resolves an authorization vulnerability on
/adminroutes where endpoints relied on basic JWT signature validation without checking server-side admin role privileges.We introduced a server-truth
AdminGuardundersrc/modules/admin/admin.guard.tsthat validates user role (role === 'admin') and account status fresh from the database (viaUserStatusService/ Supabase). Requests with stale JWT claims or non-admin roles are denied with a structured403 Forbidden(ADMIN_FORBIDDEN) response, and denied access attempts are logged viaAuditService(ADMIN_ACCESS_DENIED) for security visibility.🔄 Changes Made
AdminGuardinsrc/modules/admin/admin.guard.tsvalidating role and status from datastore./admincontrollers (AuditController,AdminRolesController) and registered/exportedAdminGuardinAdminModule.ADMIN_ACCESS_DENIED) viaAuditServiceand structured exception responses (ADMIN_FORBIDDEN,AUTH_USER_BLOCKED).admin.guard.spec.tsandaudit.controller.spec.tscovering non-admin (403), active admin (200), blocked admin (401), missing token (401), stale JWT claim with revoked role (403), and audit log emission.context/progress-tracker.mdto document the security fix.📸 Screenshots (if applicable)
N/A (Backend API changes only)
🗒️ Additional Notes
npm run build.npm test(34 test suites, 388 tests passing).