[cristim]

Problem

Closes #1142 (SEC-02, P2).

requirePermission recognized only the stateless admin API key and bearer-token sessions. A user API key arrives in x-api-key with no bearer token, so every requirePermission-gated endpoint returned 401 no authorization token provided for it. As a result the per-key scoped permissions persisted at key-creation time were never enforced anywhere: ComputeEffectivePermissions, which intersects the key's scope with the owning user's group-derived permissions, had no caller on the request path (dead code).

Fix

Wires a real user-API-key authorization path into requirePermission, per the report's first recommended option:

• internal/auth/service_apikeys_api.go: new HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) validates the key (ValidateUserAPIKey) and checks the requested action/resource against the key's effective permissions (ComputeEffectivePermissions). Matching reuses AuthContext.HasPermission, so the admin:* wildcard money-spending carve-outs (issue Add Purchaser group + remove implicit admin grant for execute/approve-any/retry-any purchases (separation of duties) #923) apply to API keys exactly as to sessions. Fail closed: any validation or lookup failure returns an error and the caller denies.
• internal/api/types.go + internal/server/app.go: method added to AuthServiceInterface and the server adapter.
• internal/api/handler.go: requirePermission now authorizes a non-admin x-api-key against the key's effective permissions. In-scope: session for the owning user (account scoping then resolves against that user as before). Out-of-scope: 403 (fail closed). A key that fails validation falls through to bearer-token auth, matching resolveAuthenticatedUserID, so a stale key header cannot lock out a caller that also presents a valid session. The frontend sends either the bearer token or the API key, never both, so existing UI behavior is unchanged.

requireAdmin intentionally still accepts only the admin API key or an admin session; user API keys remain unable to reach AuthAdmin routes.

Test evidence

Regression tests replicate the real failing scenario (request with only an x-api-key header, no bearer token) and were run against the pre-fix code (handler.go reverted) to confirm they fail there: in-scope authorization failed with 401, out-of-scope expected 403 but got 401, and the end-to-end recommendations call 401'd.

• internal/auth: TestService_HasAPIKeyPermissionAPI - scoped key grants in-scope action; scoped key cannot exceed its scope even when the owner holds the permission; key permission the owner lacks is denied (intersection); unscoped key inherits owner permissions; invalid key fails closed.
• internal/api: TestRequirePermission_UserAPIKey - in-scope key authorized as its owner; out-of-scope key gets 403; invalid key with no bearer gets 401; invalid key falls through to a valid bearer session; admin API key still bypasses the per-key lookup. TestGetRecommendations_UserAPIKey - GET recommendations end to end with a view:recommendations-scoped key.

Ran: go build ./... and go test ./internal/api/... ./internal/auth/... ./internal/server/... - 2564 tests passed, go vet clean.

Summary by CodeRabbit

• Bug Fixes

  • User API keys are now authorized using their effective, per-key permissions and the owning user’s group permissions.
  • Requests with valid keys but insufficient access now return a clear 403 permission error.
  • Invalid API keys can fall back to a valid bearer-token session when provided.
  • Admin API key bypass behavior remains unchanged.

• Tests

  • Added coverage for scoped permissions, denied access, fallback authentication, and recommendations access.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 23 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b75ec485-a9ef-44d0-b30a-cc7242d3b966

📥 Commits

Reviewing files that changed from the base of the PR and between 678bafa and fda1209.

📒 Files selected for processing (7)

• internal/api/handler.go
• internal/api/handler_apikeys_test.go
• internal/api/handler_ri_exchange_test.go
• internal/api/mocks_test.go
• internal/api/types.go
• internal/auth/service_apikeys_api.go
• internal/auth/service_apikeys_test.go

📝 Walkthrough

Walkthrough

User API keys now undergo effective scoped-permission checks during request authorization. The handler returns the owning user session on success, denies insufficient permissions, and falls back to bearer authentication when key validation fails.

Changes

User API key authorization

Layer / File(s)	Summary
Permission contract and effective authorization internal/api/types.go, internal/auth/service_apikeys_api.go, internal/auth/service_apikeys_test.go	Adds the API-key permission contract and computes effective permissions by combining key scope with owning-user group permissions.
Request authorization integration internal/server/app.go, internal/api/handler.go	Wires the auth service into requirePermission, returning sessions for permitted keys, 403 errors for denied permissions, and bearer-token fallback on key errors.
Handler regression coverage internal/api/handler_apikeys_test.go, internal/api/mocks_test.go, internal/api/handler_ri_exchange_test.go	Adds coverage for scoped-key access, denial, invalid-key fallback, admin bypass, recommendations access, and updated auth test doubles.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Request as requirePermission
  participant Adapter as authServiceAdapter
  participant Auth as auth.Service
  Request->>Adapter: Check API key permission
  Adapter->>Auth: Validate key and compute effective permissions
  Auth-->>Adapter: Return user ID and authorization result
  Adapter-->>Request: Return result
  Request-->>Request: Create session, deny, or use bearer token

Loading

Possibly related issues

• #1301 — Adds the same API-key permission service and effective-permission enforcement addressed by this change.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly states the main change: enforcing scoped user API key permissions in requirePermission.
Linked Issues check	✅ Passed	The PR wires API-key permission checks into requirePermission and adds regression coverage for scoped, invalid, admin, and bearer-fallback cases.
Out of Scope Changes check	✅ Passed	The changes stay focused on API-key permission enforcement and supporting tests/adapters, with no unrelated additions evident.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/sec-02-api-key-scoped-permissions

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Adversarial-review pass

Ran the playbook against every risk surface in the brief plus the matrix of requirePermission action/resource pairs (accounts, api-keys, groups, plans, users, purchases, recommendations, config, ri-exchange; view/create/update/delete/execute).

In-scope hardening pushed (9ae02e5)

Two new regression guards added; pure additions, no behavior change. All 2207 internal/api + internal/auth tests pass.

1. Key-precedence lock (internal/api/handler_apikeys_test.go). When a valid scoped key is denied (in-scope has=false, err=nil), requirePermission must 403 even if a valid bearer token is ALSO sent. The PR doc states "the frontend sends either, never both" and the code never consults the bearer once HasAPIKeyPermissionAPI returns cleanly, but there was no test locking that down. Asserts ValidateSession and HasPermissionAPI are NEVER called in that branch. Guards against a future refactor that flips the fall-through order silently widening a scoped key to the owner's full session perms.
2. Revoked / expired / inactive-owner fail-closed coverage (internal/auth/service_apikeys_test.go). The original TestService_HasAPIKeyPermissionAPI covered "key not found" only; the broader attacker-probable surface (revoked, expired, owner deactivated) all returned errors through ValidateUserAPIKey in isolation but had no test asserting the request-time entry point still fails closed. Three new sub-tests fix that.

Confirmed clean

• No PII / key leak in logs. logging.Debugf("User API key permission check failed: %v", err) in requirePermission and lookupAPIKeyUser's error messages do not embed the raw key, key prefix, email, or anything else covered by feedback_pii_in_logs.md.
• feedback_fail_closed_middleware.md — h.auth == nil is checked before the new user-key block; h.checkAdminAPIKey("") returns false (empty key cannot match an empty configured admin key thanks to the explicit apiKey != "" && h.apiKey != "" guard); a stale x-api-key falls through to bearer auth and the absence of a bearer correctly returns 401.
• feedback_empty_string_vs_error.md — "API key absent" vs "API key present but invalid" both end at 401 if no bearer is present, but that's correct: neither case authenticates the caller. A present-but-invalid key combined with a valid bearer correctly falls through to the bearer (covered by the existing "invalid key falls through to a valid bearer session" test).
• feedback_no_silent_fallbacks.md on the money paths — requirePermission(execute, purchases) and requirePermission(execute, ri-exchange) correctly go through ComputeEffectivePermissions intersection, and the admin:* carve-outs for execute:purchases / approve-any:purchases / retry-any:purchases (Add Purchaser group + remove implicit admin grant for execute/approve-any/retry-any purchases (separation of duties) #923) are honored via AuthContext.HasPermission for API keys exactly as for sessions.
• requireAdmin is NOT widened. User API keys (including one scoped to {admin, *}) still cannot reach AuthAdmin routes; only the stateless admin API key or an admin bearer session pass.
• No new scope-key matrix gaps for the 9 resources / 5 actions in the codebase: every requirePermission call site goes through the same HasAPIKeyPermissionAPI -> ComputeEffectivePermissions -> HasPermission chain, and the key cannot grant a permission the owner doesn't hold (intersection at service_apikeys.go:319-325, validateAPIKeyPermissions at creation).
• Stale x-api-key + valid bearer fallthrough is tested. Valid scoped key + valid bearer precedence is now also tested (added above).
• Account scoping correctly resolves against the OWNING USER (session.UserID = user.ID from HasAPIKeyPermissionAPI), so GetAllowedAccountsAPI returns the user's accounts. UserAPIKey has no account-scope field, so there's no key-level account scope to enforce.

Out-of-scope follow-ups filed with full triage labels

• #1301 — sec(auth): unscoped user API key silently inherits owner's full permissions (severity/high, urgency/this-sprint, type/security). The exact "no-scope == full-user-permission" backward-compat trap the brief flagged. Dormant pre-fix(api): enforce user API key scoped permissions in requirePermission #1226 (unscoped keys 401'd); LIVE post-fix(api): enforce user API key scoped permissions in requirePermission #1226. Proposed fix: ComputeEffectivePermissions returns [] on empty scope AND CreateAPIKey rejects empty scope. This is a deliberate policy decision the team should land separately (it changes documented + tested behavior in TestService_ComputeEffectivePermissions / "admin with unscoped key returns full group permissions").
• #1302 — fix(api/auth): /me/permissions returns owner's full perms instead of API key's effective scope (severity/medium, urgency/this-quarter, type/bug). getCurrentUserPermissions resolves the userID then fetches GetUserPermissionsAPI un-intersected; a view:recommendations-scoped key gets {admin:*} + is_admin: true from the owner. Not an authz bypass (the actual admin endpoints still 403 via requirePermission), but defeats the abstraction at the most visible advertisement point and misleads frontend gating. Pre-dates fix(api): enforce user API key scoped permissions in requirePermission #1226 but is newly reachable now that scoped keys actually authenticate on the request path.

CI / merge gate

PR is UNSTABLE for reasons unrelated to this PR's logic: the pre-existing gocyclo violation in internal/config/store_postgres_recommendations.go:320:1 (recEffectiveSavingsPct, complexity 12), the pre-existing Trivy DS-0002 finding on the Dockerfile USER directive, and the pre-existing 000074_repair_partial_migration_058_067 migration-collision blocking E2E. None of these are introduced by 9ae02e5. They want separate fixes; flagging here so they don't block #1226 itself from merge once CR is clean.

Re-pinging CR.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Rebased onto origin/main (73166c2). Clean rebase. Lint fix: named return values (userID string, allowed bool, err error) added to HasAPIKeyPermissionAPI in service_apikeys_api.go and app.go adapter to satisfy gocritic/unnamedResult. Gates: go build, go vet, gocyclo -over 10, golangci-lint (0 new issues). Tests: internal/auth (591 passed), internal/api (1668 passed).

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Rebased onto origin/main@27fdb06b5 (50 commits ahead of the original branch point). Resolved handler.go cleanly (main's redactURL change in HandleRequest is in a different function from the PR's requirePermission additions). Flake-fixed files (validator_test.go, fanout.go, fanout_test.go) now match main exactly. Gates: go build ./... (0), go vet ./... (0), tests for touched packages (0), scheduledauth -count=5 (0), TestFanOut -count=10 (0), gocyclo -over 10 (0), golangci-lint (0). New HEAD: fda1209.

[cristim]

Merged to main on the substantive CI signal (all of Lint Code/Unit/Integration/Security/Build/E2E/Validate-Terraform green; the only red check is the non-blocking tflint/trivy pre-commit infra flake, which this repo's unprotected main does not gate on) closes #1142. enforce user API-key scoped permissions in requirePermission (fails closed with 403 on scope miss).