fix(security): enforce AuthUser at the router layer, not just middleware (closes #60)#178
Closed
cristim wants to merge 1 commit into
Closed
fix(security): enforce AuthUser at the router layer, not just middleware (closes #60)#178cristim wants to merge 1 commit into
cristim wants to merge 1 commit into
Conversation
Closes #60. `Router.Route` previously checked the route's Auth level only when it equalled `AuthAdmin`. Routes declared as `Auth: AuthUser` (`/api/auth/logout`, `/api/auth/me`, `/api/auth/profile`, `/api/auth/change-password`, `/api/api-keys/*`, `/api/commitment-options`, `/api/federation/iac`) fell through the if-block with no router-level enforcement. The endpoints were still protected by `validateSecurity → authenticate` middleware, but a future refactor that reorders middleware or adds a path bypassing `validateSecurity` would silently expose every AuthUser endpoint without any error at the router boundary. Fix: replace the `if route.Auth == AuthAdmin` branch with a switch that dispatches AuthAdmin → `requireAdmin`, AuthUser → new `requireUser`, AuthPublic → no router-level check. New `requireUser` mirrors `requireAdmin` but does not require the admin role — admin API key OR any valid session passes; missing/invalid auth returns 401 via ClientError before the handler runs. Updated the AuthLevel doc comment so the router-layer enforcement contract is explicit. Tests: 5 new router-auth tests pin the contract: - AuthUser rejects missing Authorization header (401). - AuthUser rejects invalid Bearer token (401). - AuthUser accepts a valid session. - AuthUser accepts an admin API key (admins are users). - AuthPublic still runs with no auth (guard against the switch's default branch tightening accidentally). No behavioural change for routes that were already passing through `validateSecurity` — the router-layer check is now belt-and-braces. Routes that bypass validateSecurity for any reason (e.g., a future test runner, a misconfigured handler chain) are now protected at the router boundary.
Member
Author
|
@coderabbitai review |
📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant AdminKey
participant AuthService
participant Handler
Client->>Router: HTTP Request + Authorization Header
Router->>Router: Check route.Auth level
alt AuthUser Route
Router->>AdminKey: checkAdminAPIKey()
alt Admin key valid
AdminKey-->>Router: Session OK
else Admin key missing/invalid
Router->>AuthService: ValidateSession(Bearer Token)
alt Valid session
AuthService-->>Router: Session + User Info
else Invalid/expired token
Router-->>Client: 401 ClientError
end
end
Router->>Handler: Invoke handler with context
Handler-->>Client: Response
else AuthAdmin Route
Router->>AdminKey: requireAdmin (existing)
AdminKey-->>Router: Admin validation
else AuthPublic Route
Router->>Handler: Invoke handler directly
Handler-->>Client: Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/middleware.go`:
- Around line 220-239: The requireUser function only handles admin API keys and
bearer tokens but must also accept user API keys like authenticate does; after
calling extractAPIKey and checkAdminAPIKey, add the same user-API-key branch
authenticate uses (e.g., call the existing helper used by authenticate such as
h.checkUserAPIKey or h.auth.ValidateAPIKey) and if it validates return a Session
with Role "user" and the associated UserID, then continue to the bearer token
path; ensure this user-API-key check runs before falling back to
extractBearerToken/ValidateSession so valid user-key requests are accepted.
In `@internal/api/router_auth_test.go`:
- Around line 53-92: In both TestRouter_AuthUser_RejectsInvalidSession and
TestRouter_AuthUser_AcceptsValidSession, ensure the mock expectations on
MockAuth are asserted after exercising the Router: call
mockAuth.AssertExpectations(t) (or mockAuth.AssertExpectations) at the end of
each test so the ValidateSession expectation set with
mockAuth.On("ValidateSession", ...) is verified; this ensures MockAuth's
ValidateSession call actually occurred for the Handler/Router code paths.
In `@internal/api/router.go`:
- Around line 245-256: The switch over route.Auth
(AuthAdmin/AuthUser/AuthPublic) currently lacks a default so unknown auth values
fall through and run the handler; update the switch in the router to include a
default branch that fails closed by returning an authorization error (e.g., a
descriptive error like "unsupported auth level" or a forbidden error) instead of
allowing execution, and ensure this default returns from the surrounding
function the same way r.h.requireAdmin/ r.h.requireUser errors are returned so
no handler runs for unknown auth values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c867d377-315f-4cd5-af3b-4846dca710c7
📒 Files selected for processing (3)
internal/api/middleware.gointernal/api/router.gointernal/api/router_auth_test.go
Comment on lines
+220
to
+239
| func (h *Handler) requireUser(ctx context.Context, req *events.LambdaFunctionURLRequest) (*Session, error) { | ||
| // Admin API key first (stateless). | ||
| apiKey := extractAPIKey(req) | ||
| if h.checkAdminAPIKey(apiKey) { | ||
| return &Session{Role: "admin", UserID: "admin-api-key"}, nil | ||
| } | ||
|
|
||
| if h.auth == nil { | ||
| return nil, fmt.Errorf("authentication service not configured") | ||
| } | ||
|
|
||
| token := h.extractBearerToken(req) | ||
| if token == "" { | ||
| return nil, NewClientError(401, "no authorization token provided") | ||
| } | ||
|
|
||
| session, err := h.auth.ValidateSession(ctx, token) | ||
| if err != nil || session == nil { | ||
| return nil, NewClientError(401, "invalid session") | ||
| } |
There was a problem hiding this comment.
AuthUser router enforcement is missing the user API key path.
requireUser currently allows only admin API key or Bearer session, while authenticate also accepts user API keys. This creates inconsistent auth semantics and can reject valid AuthUser requests that authenticate via user API key.
Suggested fix
func (h *Handler) requireUser(ctx context.Context, req *events.LambdaFunctionURLRequest) (*Session, error) {
// Admin API key first (stateless).
apiKey := extractAPIKey(req)
if h.checkAdminAPIKey(apiKey) {
return &Session{Role: "admin", UserID: "admin-api-key"}, nil
}
+
+ // User API key (stateless).
+ if h.checkUserAPIKey(ctx, apiKey) {
+ return &Session{Role: "user", UserID: "user-api-key"}, nil
+ }
if h.auth == nil {
return nil, fmt.Errorf("authentication service not configured")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/middleware.go` around lines 220 - 239, The requireUser function
only handles admin API keys and bearer tokens but must also accept user API keys
like authenticate does; after calling extractAPIKey and checkAdminAPIKey, add
the same user-API-key branch authenticate uses (e.g., call the existing helper
used by authenticate such as h.checkUserAPIKey or h.auth.ValidateAPIKey) and if
it validates return a Session with Role "user" and the associated UserID, then
continue to the bearer token path; ensure this user-API-key check runs before
falling back to extractBearerToken/ValidateSession so valid user-key requests
are accepted.
Comment on lines
+53
to
+92
| func TestRouter_AuthUser_RejectsInvalidSession(t *testing.T) { | ||
| ctx := context.Background() | ||
| mockAuth := new(MockAuthService) | ||
| mockAuth.On("ValidateSession", ctx, "stale-token").Return(nil, errors.New("session not found")) | ||
| h := &Handler{auth: mockAuth} | ||
| r := &Router{h: h} | ||
| r.routes = []Route{ | ||
| {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, | ||
| } | ||
|
|
||
| req := &events.LambdaFunctionURLRequest{ | ||
| Headers: map[string]string{"Authorization": "Bearer stale-token"}, | ||
| } | ||
| _, err := r.Route(ctx, "GET", "/api/test/user", req) | ||
| require.Error(t, err) | ||
| ce, ok := IsClientError(err) | ||
| require.True(t, ok) | ||
| assert.Equal(t, 401, ce.code) | ||
| } | ||
|
|
||
| // TestRouter_AuthUser_AcceptsValidSession confirms the router-layer | ||
| // check passes through to the handler when a valid session is present. | ||
| // This is the positive case for the new requireUser helper. | ||
| func TestRouter_AuthUser_AcceptsValidSession(t *testing.T) { | ||
| ctx := context.Background() | ||
| mockAuth := new(MockAuthService) | ||
| mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession(), nil) | ||
| h := &Handler{auth: mockAuth} | ||
| r := &Router{h: h} | ||
| r.routes = []Route{ | ||
| {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, | ||
| } | ||
|
|
||
| req := &events.LambdaFunctionURLRequest{ | ||
| Headers: map[string]string{"Authorization": "Bearer user-token"}, | ||
| } | ||
| result, err := r.Route(ctx, "GET", "/api/test/user", req) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, map[string]string{"status": "ok"}, result) | ||
| } |
There was a problem hiding this comment.
Mock expectations are not asserted in the ValidateSession tests.
mockAuth.On(...) is configured, but AssertExpectations is never called. That can allow false positives if the call path changes.
Suggested fix
func TestRouter_AuthUser_RejectsInvalidSession(t *testing.T) {
ctx := context.Background()
mockAuth := new(MockAuthService)
+ t.Cleanup(func() { mockAuth.AssertExpectations(t) })
mockAuth.On("ValidateSession", ctx, "stale-token").Return(nil, errors.New("session not found"))
...
}
func TestRouter_AuthUser_AcceptsValidSession(t *testing.T) {
ctx := context.Background()
mockAuth := new(MockAuthService)
+ t.Cleanup(func() { mockAuth.AssertExpectations(t) })
mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession(), nil)
...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestRouter_AuthUser_RejectsInvalidSession(t *testing.T) { | |
| ctx := context.Background() | |
| mockAuth := new(MockAuthService) | |
| mockAuth.On("ValidateSession", ctx, "stale-token").Return(nil, errors.New("session not found")) | |
| h := &Handler{auth: mockAuth} | |
| r := &Router{h: h} | |
| r.routes = []Route{ | |
| {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, | |
| } | |
| req := &events.LambdaFunctionURLRequest{ | |
| Headers: map[string]string{"Authorization": "Bearer stale-token"}, | |
| } | |
| _, err := r.Route(ctx, "GET", "/api/test/user", req) | |
| require.Error(t, err) | |
| ce, ok := IsClientError(err) | |
| require.True(t, ok) | |
| assert.Equal(t, 401, ce.code) | |
| } | |
| // TestRouter_AuthUser_AcceptsValidSession confirms the router-layer | |
| // check passes through to the handler when a valid session is present. | |
| // This is the positive case for the new requireUser helper. | |
| func TestRouter_AuthUser_AcceptsValidSession(t *testing.T) { | |
| ctx := context.Background() | |
| mockAuth := new(MockAuthService) | |
| mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession(), nil) | |
| h := &Handler{auth: mockAuth} | |
| r := &Router{h: h} | |
| r.routes = []Route{ | |
| {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, | |
| } | |
| req := &events.LambdaFunctionURLRequest{ | |
| Headers: map[string]string{"Authorization": "Bearer user-token"}, | |
| } | |
| result, err := r.Route(ctx, "GET", "/api/test/user", req) | |
| require.NoError(t, err) | |
| assert.Equal(t, map[string]string{"status": "ok"}, result) | |
| } | |
| func TestRouter_AuthUser_RejectsInvalidSession(t *testing.T) { | |
| ctx := context.Background() | |
| mockAuth := new(MockAuthService) | |
| t.Cleanup(func() { mockAuth.AssertExpectations(t) }) | |
| mockAuth.On("ValidateSession", ctx, "stale-token").Return(nil, errors.New("session not found")) | |
| h := &Handler{auth: mockAuth} | |
| r := &Router{h: h} | |
| r.routes = []Route{ | |
| {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, | |
| } | |
| req := &events.LambdaFunctionURLRequest{ | |
| Headers: map[string]string{"Authorization": "Bearer stale-token"}, | |
| } | |
| _, err := r.Route(ctx, "GET", "/api/test/user", req) | |
| require.Error(t, err) | |
| ce, ok := IsClientError(err) | |
| require.True(t, ok) | |
| assert.Equal(t, 401, ce.code) | |
| } | |
| // TestRouter_AuthUser_AcceptsValidSession confirms the router-layer | |
| // check passes through to the handler when a valid session is present. | |
| // This is the positive case for the new requireUser helper. | |
| func TestRouter_AuthUser_AcceptsValidSession(t *testing.T) { | |
| ctx := context.Background() | |
| mockAuth := new(MockAuthService) | |
| t.Cleanup(func() { mockAuth.AssertExpectations(t) }) | |
| mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession(), nil) | |
| h := &Handler{auth: mockAuth} | |
| r := &Router{h: h} | |
| r.routes = []Route{ | |
| {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, | |
| } | |
| req := &events.LambdaFunctionURLRequest{ | |
| Headers: map[string]string{"Authorization": "Bearer user-token"}, | |
| } | |
| result, err := r.Route(ctx, "GET", "/api/test/user", req) | |
| require.NoError(t, err) | |
| assert.Equal(t, map[string]string{"status": "ok"}, result) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/router_auth_test.go` around lines 53 - 92, In both
TestRouter_AuthUser_RejectsInvalidSession and
TestRouter_AuthUser_AcceptsValidSession, ensure the mock expectations on
MockAuth are asserted after exercising the Router: call
mockAuth.AssertExpectations(t) (or mockAuth.AssertExpectations) at the end of
each test so the ValidateSession expectation set with
mockAuth.On("ValidateSession", ...) is verified; this ensures MockAuth's
ValidateSession call actually occurred for the Handler/Router code paths.
Comment on lines
+245
to
256
| switch route.Auth { | ||
| case AuthAdmin: | ||
| if _, err := r.h.requireAdmin(ctx, req); err != nil { | ||
| return nil, err | ||
| } | ||
| case AuthUser: | ||
| if _, err := r.h.requireUser(ctx, req); err != nil { | ||
| return nil, err | ||
| } | ||
| case AuthPublic: | ||
| // no router-level auth — handler runs directly. | ||
| } |
There was a problem hiding this comment.
Auth switch should fail closed on unknown auth levels.
The switch route.Auth has no default, so unsupported values currently execute handlers with no auth guard. For an auth boundary, this should deny by default.
Suggested fix
switch route.Auth {
case AuthAdmin:
if _, err := r.h.requireAdmin(ctx, req); err != nil {
return nil, err
}
case AuthUser:
if _, err := r.h.requireUser(ctx, req); err != nil {
return nil, err
}
case AuthPublic:
// no router-level auth — handler runs directly.
+ default:
+ return nil, fmt.Errorf("unsupported auth level: %d", route.Auth)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| switch route.Auth { | |
| case AuthAdmin: | |
| if _, err := r.h.requireAdmin(ctx, req); err != nil { | |
| return nil, err | |
| } | |
| case AuthUser: | |
| if _, err := r.h.requireUser(ctx, req); err != nil { | |
| return nil, err | |
| } | |
| case AuthPublic: | |
| // no router-level auth — handler runs directly. | |
| } | |
| switch route.Auth { | |
| case AuthAdmin: | |
| if _, err := r.h.requireAdmin(ctx, req); err != nil { | |
| return nil, err | |
| } | |
| case AuthUser: | |
| if _, err := r.h.requireUser(ctx, req); err != nil { | |
| return nil, err | |
| } | |
| case AuthPublic: | |
| // no router-level auth — handler runs directly. | |
| default: | |
| return nil, fmt.Errorf("unsupported auth level: %d", route.Auth) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/router.go` around lines 245 - 256, The switch over route.Auth
(AuthAdmin/AuthUser/AuthPublic) currently lacks a default so unknown auth values
fall through and run the handler; update the switch in the router to include a
default branch that fails closed by returning an authorization error (e.g., a
descriptive error like "unsupported auth level" or a forbidden error) instead of
allowing execution, and ensure this default returns from the surrounding
function the same way r.h.requireAdmin/ r.h.requireUser errors are returned so
no handler runs for unknown auth values.
cristim
added
triaged
Member
Author
|
Closing as superseded by #84 ( Both PRs implement defence-in-depth router-layer enforcement of
Rebasing this branch onto current The one ergonomic idea worth keeping from this PR — having the auth helper return the resolved principal so handlers don't re-fetch it — is filed as a follow-up at #194. That refactor will build on #84's Thanks for the parallel attempt — closing without merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #60 (P1, severity:high, all-users impact).
Router.Routepreviously checked the route's Auth level only when it equalledAuthAdmin. Routes declared asAuth: AuthUser(/api/auth/logout,/api/auth/me,/api/auth/profile,/api/auth/change-password,/api/api-keys/*,/api/commitment-options,/api/federation/iac) fell through the if-block with no router-level enforcement. The endpoints were still protected byvalidateSecurity → authenticatemiddleware, but a future refactor that reorders middleware or adds a path bypassingvalidateSecuritywould silently expose every AuthUser endpoint without any error at the router boundary.Fix
Replace the
if route.Auth == AuthAdminbranch with a switch that dispatches:AuthAdminh.requireAdmin(admin API key OR admin-role session) — unchangedAuthUserh.requireUser(admin API key OR any valid session) — newAuthPublicNew
requireUsermirrorsrequireAdminbut does not require the admin role; missing/invalid auth returns 401 via ClientError before the handler runs. AuthLevel doc comment now explicitly describes the router-layer enforcement contract.Tests
5 new router-auth tests pin the contract:
TestRouter_AuthUser_RejectsMissingAuthTestRouter_AuthUser_RejectsInvalidSessionTestRouter_AuthUser_AcceptsValidSessionTestRouter_AuthUser_AcceptsAdminAPIKeyTestRouter_AuthPublic_NoAuthCheckAll existing internal/api tests still pass.
Behavioural impact
No behavioural change for routes already passing through
validateSecurity— the router-layer check is now belt-and-braces. Routes that bypass validateSecurity for any reason (a future test runner, a misconfigured handler chain, a path that gets added toisPublicEndpointby mistake) are now protected at the router boundary.🤖 Generated with claude-flow
Summary by CodeRabbit
Release Notes