fix(api): gate empty-account history rows on ownership + enforce API-key constraints on money paths (adversarial-review follow-ups) - #1454
Conversation
…key constraints on money paths F1 (adversarial-review, medium/cross-scope PII+financial leak): filterPurchaseHistoryByAllowedAccounts previously passed ALL empty-AccountID rows through to scoped users unconditionally (issue #1032 / #621 fix). Any user with view:purchases could see other users' multi-account in-flight rows, including CreatedByUserEmail (PII) and dollar amounts. Fix: gate the exemption on ownership -- only pass through when p.CreatedByUserID == session.UserID. Preserves the #621/#1032 behavior for each user's own ambient in-flight rows; drops other users' rows silently. F2 (adversarial-review, low/med -- money path): requirePermissionConstraints always called HasPermissionForConstraintsAPI with session.UserID, which re-derives constraints from the owning user's GROUP permissions and ignores the user API key's own Constraints (MaxPurchaseAmount, AccountIDs, Providers, etc.). A CI key capped at $100 could spend up to the user's full group limit. Fix: thread the key's DB ID via Session.UserAPIKeyID (set in requirePermission when authenticating via HasAPIKeyPermissionAPI). requirePermissionConstraints now calls HasAPIKeyPermissionForConstraintsAPI when session.UserAPIKeyID != "", evaluating constraints against the key's effective permissions (intersection of key + user group permissions). Falls closed on any lookup error. New methods: - auth.Service.HasAPIKeyPermissionForConstraintsAPI - AuthServiceInterface.HasAPIKeyPermissionForConstraintsAPI - Session.UserAPIKeyID field (json:"-") HasAPIKeyPermissionAPI now returns (userID, keyID, allowed, err) instead of (userID, allowed, err) to avoid a redundant DB lookup. Regression tests: - TestHandler_getHistory_ScopedUserSeesEmptyAccountRows: updated to set CreatedByUserID so the scoped user's own row still passes - TestHandler_getHistory_ScopedUserCannotSeeOtherUsersEmptyAccountRows: F1 guard (fails pre-fix) - TestHandler_executePurchase_UserAPIKeyConstraintsDenied: F2 guard (fails pre-fix, asserts HasAPIKeyPermissionForConstraintsAPI is called)
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAPI-key authentication now preserves the key ID through the session and applies constraints against the key’s effective permissions. Scoped history filtering also restricts empty-account rows to their creators, with authorization and regression tests updated. ChangesAuthorization and history isolation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 1 minute. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handler.go`:
- Around line 240-248: In the successful permission branch of
HasAPIKeyPermissionAPI, validate that both userID and keyID are non-empty before
constructing the Session. Reject the request with the existing client-error path
when either identifier is missing, and only return Session with UserAPIKeyID
after both values pass validation.
In `@internal/auth/service_apikeys_api.go`:
- Around line 193-198: Update ComputeEffectivePermissions and the authorization
flow around permissionsAllow so each constraint set is validated independently
against both the owner’s group permissions and the scoped API key permissions.
Preserve the action/resource checks, but intersect constraint limits rather than
retaining only the key constraints, ensuring authorization succeeds only when
both sources allow the same request.
🪄 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: 8682645e-5849-41eb-983b-48fb98afc142
📒 Files selected for processing (11)
internal/api/handler.gointernal/api/handler_apikeys_test.gointernal/api/handler_history.gointernal/api/handler_history_test.gointernal/api/handler_purchases_test.gointernal/api/handler_ri_exchange_test.gointernal/api/mocks_test.gointernal/api/types.gointernal/auth/service_apikeys_api.gointernal/auth/service_apikeys_test.gointernal/server/app.go
C1 (handler.go): extract authorizeAPIKey helper and fail closed when HasAPIKeyPermissionAPI returns an empty userID or keyID despite has=true. Without the guard, an empty keyID produces a bearer-session-shaped Session (UserAPIKeyID="") that bypasses the API-key constraint path in requirePermissionConstraints. Extraction keeps requirePermission below the gocyclo-10 pre-commit threshold. C2 (service_apikeys_api.go + service_apikeys.go): enforce the owner's group constraint limits independently in HasAPIKeyPermissionForConstraintsAPI. ComputeEffectivePermissions retains the key's constraint values (e.g. MaxPurchaseAmount) even when they exceed the owner's group limits; a key capped at $1000 owned by a user capped at $100 could previously authorize a $500 request. The fix fetches ownerAuthCtx once (replacing the implicit GetAuthContext inside ComputeEffectivePermissions) and checks each constraint set against both the key's effective permissions and the owner's group permissions. computeEffectivePermissionsFromAuthCtx is extracted as a pure helper so the single authCtx fetch is shared between both checks. Regression test: TestService_HasAPIKeyPermissionForConstraintsAPI_OwnerCapEnforced confirms the $500 request is denied when owner cap is $100, and a $50 request within both caps is allowed.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 30 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 6 minutes. |
…#1492) Service.HasPermission (the bearer-session auth path) granted the three money-spending verbs carved out for separation of duties (execute, approve-any, and retry-any on purchases; issue #923) to any admin:* holder, unconditionally. AuthContext.HasPermission already enforced the carve-out, so the two paths disagreed: a bare Administrators-group member could execute or approve-any purchases through the bearer path while being correctly denied through the auth-context path. Wire permissionsAllow through the same adminCarvedOuts map so admin falls through to the explicit-permission check for the carved-out verbs instead of short-circuiting, mirroring AuthContext.HasPermission. Also updates TestGroupOnlyAuthz_AdminEquivalence, which asserted the pre-fix (buggy) behavior for execute:purchases and approve-any:purchases, to match the intended carve-out semantics. Closes #1454.
Summary
Two security defects found by adversarial review of the history/permission subsystem, fixed with regression tests that fail pre-fix.
F1 -- GET /api/history leaks other users' in-flight purchases (medium, PII + financial)
Root cause:
filterPurchaseHistoryByAllowedAccountsexempted ALL empty-AccountID rows from account scoping (the #1032/#621 fix).fetchExecutionsAsHistoryloads ALL users' non-completed executions with no creator filter; executions spanning multiple accounts collapse to AccountID="" viacollapseRecommendationAccount. Net: a scoped user with view:purchases saw every other user's multi-account in-flight row, includingCreatedByUserEmail(PII) and dollar amounts.Fix: Gate the empty-AccountID exemption on ownership -- pass through only when
p.CreatedByUserID == session.UserID. Preserves the #621/#1032 "see your own unattributed in-flight purchases" behavior without leaking others'.Files:
internal/api/handler_history.goTests:
TestHandler_getHistory_ScopedUserSeesEmptyAccountRows-- updated to set CreatedByUserID on the ambient exec so the scoped user's own row still passesTestHandler_getHistory_ScopedUserCannotSeeOtherUsersEmptyAccountRows-- new; fails pre-fixF2 -- User API key permission CONSTRAINTS never enforced at execution (low/med, money)
Root cause:
requirePermissionConstraintsalways calledHasPermissionForConstraintsAPI(ctx, session.UserID, ...), which re-derives from the owning user's GROUP permissions and ignores the key's ownConstraints(MaxPurchaseAmount, AccountIDs, Providers, etc.). A CI key capped at $100 could spend up to the user's full group limit.Fix: Thread the key's DB ID via
Session.UserAPIKeyID(set inrequirePermissionwhen the x-api-key path succeeds).requirePermissionConstraintsnow calls the newHasAPIKeyPermissionForConstraintsAPIwhensession.UserAPIKeyID != "", evaluating constraints against the key's EFFECTIVE permissions (intersection of key's own constraints and the owning user's group-derived permissions). Fails closed on any lookup error.New API surface:
auth.Service.HasAPIKeyPermissionForConstraintsAPI(ctx, keyID, userID, action, resource, constraintSets) (bool, error)AuthServiceInterface.HasAPIKeyPermissionForConstraintsAPI(added to interface + adapter)Session.UserAPIKeyID string(json:"-")HasAPIKeyPermissionAPIreturn signature extended:(userID, keyID string, allowed bool, err error)Files:
internal/auth/service_apikeys_api.go,internal/api/types.go,internal/api/handler.go,internal/server/app.goTests:
TestHandler_executePurchase_UserAPIKeyConstraintsDenied-- new; fails pre-fix (assertsHasAPIKeyPermissionForConstraintsAPIis called instead of the user-group path)Test plan
go build ./...cleango vet ./internal/api/... ./internal/auth/... ./internal/server/...cleango test ./internal/api/...-- 1769 passedgo test ./internal/auth/...-- 596 passedgo test ./internal/server/...-- 410 passedTestHandler_getHistory_ScopedUserCannotSeeOtherUsersEmptyAccountRowsfails on pre-fix code and passes afterTestHandler_executePurchase_UserAPIKeyConstraintsDeniedfails on pre-fix code and passes afterSummary by CodeRabbit