fix(security): defence-in-depth AuthUser check in Router.Route#84
Conversation
Router.Route previously enforced authentication only when the matched Route had Auth == AuthAdmin. Routes declared with Auth: AuthUser (/api/auth/logout, /api/auth/me, /api/auth/profile, /api/auth/change-password, /api/api-keys, /api/federation/iac) fell through with no router-level check, relying entirely on the validateSecurity → authenticate middleware in handler.go. That worked today, but a future refactor that reordered middleware or added a route that bypassed validateSecurity would silently expose every AuthUser endpoint. Add a switch on route.Auth in Router.Route that calls a new h.requireAuth(ctx, req) helper for AuthUser routes. requireAuth reuses the existing h.authenticate path (admin API key, user API key, or session bearer token), so the accepted credentials are identical to the middleware — this is purely a belt-and-braces re-check at dispatch time. AuthPublic remains a no-op (still gated by isPublicEndpoint in the middleware). Also tighten the AuthLevel doc comments to spell out where each level is enforced (Router.Route via requireAdmin/requireAuth, isPublicEndpoint for AuthPublic), per the issue's second ask. Tests in router_authuser_test.go cover: - AuthUser route rejects when no credential is presented (401) - AuthUser route rejects when bearer token is invalid (401) - AuthUser route accepts a valid non-admin user session - AuthPublic route still dispatches with no credentials (no regression) - requireAuth unit tests for admin API key / user session / no cred Closes #60
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 7 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
P1 — defence-in-depth security fix: router silently bypassed |
Summary
Fixes #60.
Router.Routeonly enforced authentication when the matched route hadAuth == AuthAdmin. Routes declared asAuth: AuthUser(/api/auth/logout,/api/auth/me,/api/auth/profile,/api/auth/change-password,/api/api-keys,/api/federation/iac) fell through with no router-level check, relying entirely on thevalidateSecurity → authenticatemiddleware inhandler.go.That middleware does protect these routes today (they're not listed in
isPublicEndpoint), so this isn't an exploitable gap right now — but it's a fragile single-point-of-failure. A future refactor that reorders middleware, or a new code path that bypassesvalidateSecurity, would silently expose everyAuthUserendpoint.Audit
internal/api/router.go:213-226only checksAuthAdmin.validateSecurity(handler.go:272) →authenticate(middleware.go:285) covers allAuthUserpaths today via theisPublicEndpointallow-list.AuthUserendpoint with zero compile-time signal.Fix
requireAuth(ctx, req)helper inmiddleware.go— reuses the existingh.authenticate(admin API key, user API key, or session bearer token), returns a 401ClientErroron rejection.AuthAdmin-onlyifinRouter.Routeto aswitchonroute.Auth:AuthAdmin→requireAdmin(unchanged)AuthUser→requireAuth(new)AuthPublic→ no-op (still gated byisPublicEndpointin middleware)AuthLeveldoc comments to spell out where each level is enforced — addresses the issue's second ask.This is a defence-in-depth change: the middleware still runs first; the router check is a redundant safety net that ensures the route's declared auth level is honoured at dispatch time regardless of upstream middleware decisions.
Test plan
internal/api/router_authuser_test.gocover:AuthUserroute rejects when no credential is presented (401 ClientError)AuthUserroute rejects when bearer token is invalid (401 ClientError)AuthUserroute accepts a valid non-admin user session (no regression)AuthPublicroute still dispatches with no credentials (no regression)requireAuthhelper unit tests for admin API key / user session / no credgo build ./...passesgo test ./internal/api/...passes (existing suite + new tests)go vet ./...clean