fix(api): gate empty-account history rows on ownership + enforce API-key constraints on money paths (adversarial-review follow-ups)

internal/api/handler.go

@@ -236,15 +236,12 @@ func (h *Handler) requirePermission(ctx context.Context, req *events.LambdaFunct
 	// that fails validation falls through to bearer-token auth, matching
 	// resolveAuthenticatedUserID, so a stale x-api-key header cannot lock
 	// out a caller that also presents a valid session.
-	if apiKey != "" {
-		userID, has, err := h.auth.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource)
-		if err == nil {
-			if !has {
-				return nil, NewClientError(403, fmt.Sprintf("permission denied: requires %s on %s", action, resource))
-			}
-			return &Session{UserID: userID}, nil
-		}
-		logging.Debugf("User API key permission check failed: %v", err)
+	session, apiKeyErr := h.authorizeAPIKey(ctx, apiKey, action, resource)
+	if apiKeyErr != nil {
+		return nil, apiKeyErr
+	}
+	if session != nil {
+		return session, nil
 	}
 
 	token := h.extractBearerToken(req)
@@ -268,6 +265,37 @@ func (h *Handler) requirePermission(ctx context.Context, req *events.LambdaFunct
 	return session, nil
 }
 
+// authorizeAPIKey checks whether the given API key value has the required
+// permission. It returns:
+//   - (nil, nil)         if apiKey is absent or fails validation (caller falls
+//     through to bearer-token auth)
+//   - (session, nil)     if the key grants the permission and identity is valid
+//   - (nil, 403 error)   if the key is present and actively denies the request
+//   - (nil, other error) if an invariant is violated (e.g. incomplete identity)
+//
+// Fail closed: empty userID or keyID after a successful grant is an internal
+// error, not a fall-through, so the caller cannot silently skip key constraints.
+func (h *Handler) authorizeAPIKey(ctx context.Context, apiKey, action, resource string) (*Session, error) {
+	if apiKey == "" {
+		return nil, nil
+	}
+	userID, keyID, has, err := h.auth.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource)
+	if err != nil {
+		logging.Debugf("User API key permission check failed: %v", err)
+		return nil, nil // validation error: fall through to bearer-token auth
+	}
+	if !has {
+		return nil, NewClientError(403, fmt.Sprintf("permission denied: requires %s on %s", action, resource))
+	}
+	if userID == "" || keyID == "" {
+		return nil, fmt.Errorf("API key permission check returned incomplete identity")
+	}
+	// Thread the key's database ID so requirePermissionConstraints can evaluate
+	// constraints against the key's effective permissions (not just the owning
+	// user's group permissions).
+	return &Session{UserID: userID, UserAPIKeyID: keyID}, nil
+}
+
 // unattributedAccountConstraint is the request-side AccountIDs value passed
 // to requirePermissionConstraints when a request cannot be attributed to a
 // registered cloud account (a recommendation without a cloud_account_id, or
@@ -291,6 +319,13 @@ const unattributedAccountConstraint = "unattributed"
 // infrastructure credential with no user row, so it bypasses the check just
 // like it bypasses requirePermission's per-user lookup. Fails closed on a
 // missing auth service or a lookup error.
+//
+// For user-API-key sessions (session.UserAPIKeyID != ""), constraints are
+// evaluated against the KEY's effective permissions (the intersection of the
+// key's own constraints and the owning user's group permissions). This
+// prevents a CI key with MaxPurchaseAmount=$100 from spending up to the
+// owning user's full group limit by inheriting the broader group permissions
+// (adversarial-review F2).
 func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Session, action, resource string, constraintSets []auth.PermissionConstraints) error {
 	if session == nil {
 		return fmt.Errorf("internal error: nil session passed to requirePermissionConstraints")
@@ -301,6 +336,18 @@ func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Ses
 	if h.auth == nil {
 		return fmt.Errorf("authentication service not configured")
 	}
+	// User API key: evaluate constraints against the key's effective permissions,
+	// not the owning user's full group permissions.
+	if session.UserAPIKeyID != "" {
+		has, err := h.auth.HasAPIKeyPermissionForConstraintsAPI(ctx, session.UserAPIKeyID, session.UserID, action, resource, constraintSets)
+		if err != nil {
+			return fmt.Errorf("permission constraint check failed: %w", err)
+		}
+		if !has {
+			return NewClientError(403, fmt.Sprintf("permission denied: this request exceeds the constraints configured on your %s permission for %s", action, resource))
+		}
+		return nil
+	}
 	has, err := h.auth.HasPermissionForConstraintsAPI(ctx, session.UserID, action, resource, constraintSets)
 	if err != nil {
 		return fmt.Errorf("permission constraint check failed: %w", err)

internal/api/handler_apikeys_test.go

@@ -672,20 +672,23 @@ func TestRequirePermission_UserAPIKey(t *testing.T) {
 		mockAuth := new(MockAuthService)
 		t.Cleanup(func() { mockAuth.AssertExpectations(t) })
 		mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations").
-			Return("user-123", true, nil)
+			Return("user-123", "key-abc", true, nil)
 
 		handler := &Handler{auth: mockAuth}
 		session, err := handler.requirePermission(ctx, newReq(), "view", "recommendations")
 		require.NoError(t, err)
 		require.NotNil(t, session)
 		assert.Equal(t, "user-123", session.UserID)
+		// Key ID must be threaded so requirePermissionConstraints can evaluate
+		// key-scoped caps (adversarial-review F2).
+		assert.Equal(t, "key-abc", session.UserAPIKeyID)
 	})
 
 	t.Run("regression #1142: scoped key is denied an out-of-scope permission with 403", func(t *testing.T) {
 		mockAuth := new(MockAuthService)
 		t.Cleanup(func() { mockAuth.AssertExpectations(t) })
 		mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "execute", "purchases").
-			Return("user-123", false, nil)
+			Return("user-123", "", false, nil)
 
 		handler := &Handler{auth: mockAuth}
 		session, err := handler.requirePermission(ctx, newReq(), "execute", "purchases")
@@ -701,7 +704,7 @@ func TestRequirePermission_UserAPIKey(t *testing.T) {
 		mockAuth := new(MockAuthService)
 		t.Cleanup(func() { mockAuth.AssertExpectations(t) })
 		mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations").
-			Return("", false, errors.New("invalid API key"))
+			Return("", "", false, errors.New("invalid API key"))
 
 		handler := &Handler{auth: mockAuth}
 		session, err := handler.requirePermission(ctx, newReq(), "view", "recommendations")
@@ -716,7 +719,7 @@ func TestRequirePermission_UserAPIKey(t *testing.T) {
 		mockAuth := new(MockAuthService)
 		t.Cleanup(func() { mockAuth.AssertExpectations(t) })
 		mockAuth.On("HasAPIKeyPermissionAPI", ctx, "stale-key", "view", "recommendations").
-			Return("", false, errors.New("invalid API key"))
+			Return("", "", false, errors.New("invalid API key"))
 		mockAuth.On("ValidateSession", ctx, "session-token").
 			Return(&Session{UserID: "user-456"}, nil)
 		mockAuth.On("HasPermissionAPI", ctx, "user-456", "view", "recommendations").
@@ -761,7 +764,7 @@ func TestGetRecommendations_UserAPIKey(t *testing.T) {
 	mockAuth := new(MockAuthService)
 	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
 	mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations").
-		Return("user-123", true, nil)
+		Return("user-123", "key-abc", true, nil)
 	// Account scoping resolves against the key's owning user.
 	mockAuth.On("GetAllowedAccountsAPI", ctx, "user-123").
 		Return([]string(nil), nil)

internal/api/handler_history.go

@@ -955,14 +955,14 @@ func appendMissing(dst []string, vals ...string) []string {
 // through unchanged.
 //
 // Rows with an empty AccountID are exempt from the drop for scoped users
-// (issue #1032, regression of #621). An empty AccountID means the execution
-// was ambient (exec.CloudAccountID == nil) AND its recommendations carry no
-// common account. These are in-flight financial actions the user owns that
-// cannot be attributed to a specific cloud account — dropping them silently
-// re-introduces the #621 disappearance bug for non-admin users. Passing them
-// through is safe: the session's allowed_accounts gate already ensures the
-// user has view:purchases permission, and unattributed rows carry no
-// account-specific data that would violate cross-tenant isolation.
+// only when the requesting user created the row (issue #1032, regression of
+// #621). An empty AccountID means the execution was ambient
+// (exec.CloudAccountID == nil) AND its recommendations carry no common
+// account. These are in-flight financial actions the user owns that cannot be
+// attributed to a specific cloud account; dropping them silently re-introduces
+// the #621 disappearance bug. The exemption is now gated on ownership so that
+// another user's multi-account in-flight row (including CreatedByUserEmail PII
+// and dollar amounts) is not visible to unrelated scoped users.
 func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, session *Session, purchases []config.PurchaseHistoryRecord) ([]config.PurchaseHistoryRecord, error) {
 	allowed, err := h.getAllowedAccounts(ctx, session)
 	if err != nil {
@@ -976,10 +976,14 @@ func (h *Handler) filterPurchaseHistoryByAllowedAccounts(ctx context.Context, se
 	for _rvc := range purchases {
 		p := purchases[_rvc]
 		// Empty AccountID: unattributed ambient/multi-account synthesized row.
-		// Pass through so scoped users see in-flight financial actions that
-		// cannot be pinned to a single account (issue #1032 / #621 regression).
+		// Pass through only when the requesting user created the row (issue
+		// #1032 / #621 regression + adversarial-review F1). Dropping rows
+		// owned by other users prevents PII (CreatedByUserEmail) and dollar
+		// amounts from leaking across user boundaries.
 		if p.AccountID == "" {
-			filtered = append(filtered, p)
+			if p.CreatedByUserID == session.UserID {
+				filtered = append(filtered, p)
+			}
 			continue
 		}
 		if auth.MatchesAccount(allowed, p.AccountID, nameByID[p.AccountID]) {

internal/api/handler_history_test.go

@@ -483,12 +483,17 @@ func TestHandler_getHistory_GetIsReadOnly(t *testing.T) {
 func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) {
 	ctx := context.Background()
 
-	// Ambient execution: CloudAccountID == nil, recommendations also have no
-	// account — this is the multi-account/ambient-credentials path.
+	const scopedUserID = "scoped-user-id"
+
+	// Ambient execution created by the scoped user: CloudAccountID == nil and
+	// recommendations carry no common account. CreatedByUserID must match the
+	// requesting session so the ownership gate (adversarial-review F1) passes.
+	creatorID := scopedUserID
 	ambientExec := config.PurchaseExecution{
-		ExecutionID:   "ambient-exec-1",
-		Status:        "pending",
-		ScheduledDate: time.Now(),
+		ExecutionID:     "ambient-exec-1",
+		Status:          "pending",
+		ScheduledDate:   time.Now(),
+		CreatedByUserID: &creatorID,
 		Recommendations: []config.RecommendationRecord{
 			// No CloudAccountID on either rec → collapseRecommendationAccount
 			// returns "" → executionToHistoryRow sets AccountID = "".
@@ -510,14 +515,16 @@ func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) {
 
 	// Scoped user: only allowed to access one specific account UUID.
 	scopedUser := &Session{
-		UserID: "scoped-user-id",
+		UserID: scopedUserID,
 		Email:  "scoped@example.com",
 	}
 	mockAuth := new(MockAuthService)
 	mockAuth.On("ValidateSession", ctx, "scoped-token").Return(scopedUser, nil)
-	mockAuth.On("HasPermissionAPI", ctx, "scoped-user-id", "view", "purchases").Return(true, nil)
+	mockAuth.On("HasPermissionAPI", ctx, scopedUserID, "view", "purchases").Return(true, nil)
 	// allowed_accounts is non-empty → user is scoped (not unrestricted).
-	mockAuth.On("GetAllowedAccountsAPI", ctx, "scoped-user-id").Return([]string{"aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa"}, nil)
+	mockAuth.On("GetAllowedAccountsAPI", ctx, scopedUserID).Return([]string{"aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa"}, nil)
+	// resolveUserEmails resolves creator emails; stub for the scoped user's own ID.
+	mockAuth.On("GetUser", ctx, scopedUserID).Return(&User{Email: "scoped@example.com"}, nil).Maybe()
 
 	handler := &Handler{
 		auth:   mockAuth,
@@ -533,14 +540,75 @@ func TestHandler_getHistory_ScopedUserSeesEmptyAccountRows(t *testing.T) {
 
 	resp := result.(HistoryResponse)
 	require.Len(t, resp.Purchases, 1,
-		"issue #1032 / #621 regression: a scoped user must see in-flight ambient-account executions; dropping them re-creates the financial-action-vanishes bug")
+		"issue #1032 / #621 regression: a scoped user must see their own in-flight ambient-account executions; dropping them re-creates the financial-action-vanishes bug")
 	assert.Equal(t, "ambient-exec-1", resp.Purchases[0].PurchaseID)
 	assert.Equal(t, "", resp.Purchases[0].AccountID,
-		"the AccountID is empty (ambient execution) and must remain visible to scoped users")
+		"the AccountID is empty (ambient execution) and must remain visible to scoped users who created the row")
 	assert.Equal(t, "pending", resp.Purchases[0].Status)
 	assert.Equal(t, 1, resp.Summary.TotalPending)
 }
 
+// TestHandler_getHistory_ScopedUserCannotSeeOtherUsersEmptyAccountRows is the
+// adversarial-review F1 regression test: a scoped user must NOT see in-flight
+// ambient-account rows (AccountID == "") created by OTHER users. Before the
+// fix, filterPurchaseHistoryByAllowedAccounts passed ALL empty-AccountID rows
+// through unconditionally, leaking other users' CreatedByUserEmail (PII) and
+// dollar amounts to any scoped user who had view:purchases.
+//
+// Fails pre-fix: user B's ambient row passes through the empty-AccountID
+// exemption and resp.Purchases has length 1 instead of 0.
+func TestHandler_getHistory_ScopedUserCannotSeeOtherUsersEmptyAccountRows(t *testing.T) {
+	ctx := context.Background()
+
+	const userAID = "user-a-id"
+	const userBID = "user-b-id"
+
+	// Ambient execution created by user B with PII-carrying fields.
+	creatorID := userBID
+	otherUsersExec := config.PurchaseExecution{
+		ExecutionID:     "other-user-ambient-exec",
+		Status:          "pending",
+		ScheduledDate:   time.Now(),
+		CreatedByUserID: &creatorID,
+		Recommendations: []config.RecommendationRecord{
+			// No cloud account → AccountID will be "".
+			{Provider: "aws", Service: "ec2", Region: "us-east-1"},
+		},
+	}
+
+	mockStore := new(MockConfigStore)
+	approverEmail := "ops@example.com"
+	mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil)
+	mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{otherUsersExec}, nil)
+	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil)
+	mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) {
+		return nil, nil
+	}
+
+	// User A: scoped to one account, different from user B.
+	userASession := &Session{UserID: userAID, Email: "a@example.com"}
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "user-a-token").Return(userASession, nil)
+	mockAuth.On("HasPermissionAPI", ctx, userAID, "view", "purchases").Return(true, nil)
+	mockAuth.On("GetAllowedAccountsAPI", ctx, userAID).Return([]string{"aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa"}, nil)
+	// resolveUserEmails may try to look up user B's email; stub it to return a
+	// value so the test exercises the actual row filtering, not an email lookup error.
+	mockAuth.On("GetUser", ctx, userBID).Return(&User{Email: "b@example.com"}, nil).Maybe()
+
+	handler := &Handler{auth: mockAuth, config: mockStore}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"Authorization": "Bearer user-a-token"},
+	}
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	result, err := handler.getHistory(ctx, req, map[string]string{})
+	require.NoError(t, err)
+
+	resp := result.(HistoryResponse)
+	assert.Empty(t, resp.Purchases,
+		"adversarial-review F1: scoped user A must NOT see another user's empty-account in-flight row (PII/financial leak)")
+}
+
 // TestHandler_expireStaleExecutionsAsync_SystemActorIsNil asserts that the
 // async stale-expire sweep passes nil as the actor param to
 // TransitionExecutionStatus. Expiry is a system-initiated path (no human

internal/api/handler_purchases_test.go

@@ -3618,6 +3618,65 @@ func TestHandler_executePurchase_PermissionConstraintsDenied(t *testing.T) {
 	assert.Contains(t, ce.Error(), "constraints")
 }
 
+// TestHandler_executePurchase_UserAPIKeyConstraintsDenied is the
+// adversarial-review F2 regression test: a user API key with a
+// MaxPurchaseAmount cap must be denied when the request exceeds that cap, even
+// if the OWNING USER's group permissions allow a higher limit. Pre-fix,
+// requirePermissionConstraints always called HasPermissionForConstraintsAPI
+// with session.UserID, which re-derives from the user's GROUP permissions and
+// silently ignores the key's own Constraints — so a $100-capped CI key could
+// spend up to the user's full limit.
+//
+// Fails pre-fix: HasAPIKeyPermissionForConstraintsAPI is never called;
+// HasPermissionForConstraintsAPI (the user path) is called instead and returns
+// true (user has no cap), so the request is not rejected and falls through to
+// execution.
+func TestHandler_executePurchase_UserAPIKeyConstraintsDenied(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+	// No store expectations: the request must be rejected before saving.
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+
+	const ownerUserID = "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee"
+	const keyID = "key-id-ci-100-cap"
+
+	// requirePermission routes to HasAPIKeyPermissionAPI for x-api-key headers.
+	// Returns the owning user's ID and the key's DB ID so the session carries
+	// UserAPIKeyID for the downstream constraint check.
+	mockAuth.On("HasAPIKeyPermissionAPI", ctx, "ci-key-value", "execute", "purchases").
+		Return(ownerUserID, keyID, true, nil)
+	// validatePurchaseRecommendationScope uses GetAllowedAccountsAPI to scope
+	// the request; return empty so no account filter is applied.
+	mockAuth.On("GetAllowedAccountsAPI", ctx, ownerUserID).Return([]string{}, nil)
+	// requirePermissionConstraints must call HasAPIKeyPermissionForConstraintsAPI
+	// (not HasPermissionForConstraintsAPI) when session.UserAPIKeyID != "".
+	// The key's cap blocks the $500 purchase.
+	mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, keyID, ownerUserID, "execute", "purchases",
+		mock.MatchedBy(func(sets []auth.PermissionConstraints) bool {
+			if len(sets) != 1 {
+				return false
+			}
+			return sets[0].MaxPurchaseAmount == 500.0
+		})).Return(false, nil)
+
+	handler := &Handler{config: mockStore, auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"x-api-key": "ci-key-value"},
+		Body: `{"recommendations": [
+			{"id": "rec-1", "provider": "aws", "service": "ec2", "region": "us-east-1", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 500.0, "savings": 50.0}
+		]}`,
+	}
+	_, err := handler.executePurchase(ctx, req)
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a 403 clientError, got: %v", err)
+	assert.Equal(t, 403, ce.code)
+	assert.Contains(t, ce.Error(), "constraints",
+		"adversarial-review F2: user API key's MaxPurchaseAmount cap must be enforced at execution time")
+}
+
 // TestPurchaseConstraintSets_AccountDimensionAlwaysPopulated pins the SEC-01
 // fail-closed shape of the AccountIDs dimension: every constraint set must
 // carry a non-empty AccountIDs list. The auth matcher treats an empty