From f118f1211dc263aed2ae577dbef449ce2de87b7d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 06:17:21 +0200 Subject: [PATCH 1/4] sec(api): apply account-scope gate to approve/cancel/retry/revoke purchase endpoints approvePurchase, cancelPurchase, retryPurchase, the session-authed branch of revokeViaEmailToken, and revokeScheduledExecution relied only on the approve/cancel/retry/revoke -any/-own RBAC matrix and never consulted the session's allowed_accounts, unlike the sibling pause/resume/run/delete-planned endpoints which all call requireExecutionAccess. A session scoped to one account but holding an -any permission could approve, cancel, retry, or revoke a purchase execution belonging to a different account, including firing the real SDK purchase. Adds a requireExecutionAccess call to each session-authed branch, after the existing RBAC check so a caller who fails RBAC still falls through to the per-account contact_email token path unaffected. Folds the new check plus the existing RBAC/4-eyes checks into per-endpoint authorize*Session helpers (authorizeApproveSession, authorizeCancelSession, authorizeRetrySession) to keep the callers under the project's gocyclo budget. Also covers audit finding A16-009: authorizeSessionRevokeExecution's hasAny allow, the !hasOwn 403, and both permission-lookup error paths had no test coverage; adds a table-driven test for all four branches. Regression tests: TestHandler_approvePurchase_OutOfScope and TestHandler_retryPurchase_OutOfScope drive the real HTTP handler with a session scoped to allowed_accounts=[Production] holding approve-any/ retry-any against an execution scoped to a different account, and assert the same 404 requireExecutionAccess already returns for pause/resume/ delete-planned. Verified both fail pre-fix (one panics reaching the nil purchase manager, the other hits an unmocked downstream call) via `git stash` on the production files only, and pass post-fix. Docker is unavailable on this host; testcontainers-based DB tests were not run locally. All internal/api unit tests (2223) pass under -race -short, go vet is clean, go mod tidy -diff is empty, and golangci-lint v2.10.1 (pinned in ci.yml) run with the exact CI invocation reports 0 issues. Closes #92 --- internal/api/handler_purchases.go | 102 ++++++++++++--- internal/api/handler_purchases_revoke.go | 10 ++ internal/api/handler_purchases_revoke_test.go | 119 ++++++++++++++++++ internal/api/handler_purchases_test.go | 112 +++++++++++++++++ 4 files changed, 323 insertions(+), 20 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 713ce6d37..86fb0c1d7 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -728,15 +728,7 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be approved (status=%s)", execution.ExecutionID, execution.Status)) } - if err := h.authorizeSessionApprove(ctx, session, execution); err != nil { - return nil, err - } - - // 4-eyes mode (issue #1005): enforce after RBAC so only users who already - // pass the approve-any / approve-own gate reach this check. The admin - // wildcard inside authorizeSessionApprove short-circuits RBAC but NOT this - // check; admins who created the row must disable the mode first. - if err := h.requireDifferentApprover(ctx, session, execution); err != nil { + if err := h.authorizeApproveSession(ctx, session, execution); err != nil { return nil, err } @@ -780,6 +772,33 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam return map[string]string{"status": "completed"}, nil } +// authorizeApproveSession runs every gate approvePurchaseViaSession must pass +// before mutating state: the approve-any/approve-own RBAC matrix, the +// account-scope check (issue #92), and the 4-eyes distinct-approver rule. +// Extracted from approvePurchaseViaSession to keep that function under the +// cyclomatic-complexity ceiling; the three checks stay in this order so the +// security boundary is unchanged (RBAC first, so a caller who fails it and +// falls through to the contact_email token path is unaffected by the scope +// or 4-eyes checks; 4-eyes last, since only a caller admin's own RBAC has +// already approved needs it). +func (h *Handler) authorizeApproveSession(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { + if err := h.authorizeSessionApprove(ctx, session, execution); err != nil { + return err + } + // Account-scope gate (issue #92): approve is a money-moving endpoint + // that, unlike pause/resume/run/delete-planned, relied only on the + // approve-any/approve-own RBAC matrix above and never consulted the + // session's allowed_accounts. + if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { + return err + } + // 4-eyes mode (issue #1005): enforce after RBAC so only users who already + // pass the approve-any / approve-own gate reach this check. The admin + // wildcard inside authorizeSessionApprove short-circuits RBAC but NOT this + // check; admins who created the row must disable the mode first. + return h.requireDifferentApprover(ctx, session, execution) +} + // authorizeSessionApprove returns nil when the session is permitted to // approve the given execution under the approve-any / approve-own RBAC // rules added in issue #286. Returns a 403 ClientError otherwise. @@ -1262,7 +1281,7 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb return nil, err } - if err := h.authorizeSessionCancel(ctx, session, execution); err != nil { + if err := h.authorizeCancelSession(ctx, session, execution); err != nil { return nil, err } @@ -1445,6 +1464,16 @@ func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFun } switch sessErr := h.authorizeSessionCancel(ctx, session, execution); { case sessErr == nil: + // Account-scope gate (issue #92): the session-authed revoke branch + // relied only on the cancel-any/cancel-own RBAC matrix above and never + // consulted allowed_accounts. Runs after RBAC succeeds, alongside the + // CSRF check below, so a caller who fails RBAC and falls through to + // the contact_email token path is unaffected. Unlike a + // permission-denied result, a scope miss has no legitimate fallback to + // the token branch, so it is a hard failure. + if scopeErr := h.requireExecutionAccess(ctx, session, execution.ExecutionID); scopeErr != nil { + return nil, true, scopeErr + } // These endpoints are AuthPublic so the outer middleware skips CSRF. // Enforce it here for the session-authed revoke sub-path, consistent // with cancelPurchaseViaSession and approvePurchaseViaSession which both @@ -1594,6 +1623,23 @@ func (h *Handler) revokeViaSession(ctx context.Context, execution *config.Purcha }, nil } +// authorizeCancelSession combines the cancel-any/cancel-own RBAC matrix with +// the account-scope check (issue #92) for cancelPurchaseViaSession. Extracted +// to keep that function under the cyclomatic-complexity ceiling. Runs RBAC +// first so a caller who fails it and falls through to the contact_email +// token path (in cancelPurchase's dispatcher) is unaffected by the scope +// check. Not used by the OTHER two callers of authorizeSessionCancel +// (cancelPurchase's dispatcher, tryRevokeViaSession) which call the +// unwrapped RBAC-only check directly, since a permission-denied result there +// must fall through to the token path rather than the scope check ever +// running. +func (h *Handler) authorizeCancelSession(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { + if err := h.authorizeSessionCancel(ctx, session, execution); err != nil { + return err + } + return h.requireExecutionAccess(ctx, session, execution.ExecutionID) +} + // authorizeSessionCancel returns nil when the session is permitted to cancel // the given execution under the cancel-any / cancel-own RBAC rules added in // issue #46. Returns a 403 ClientError otherwise. @@ -1807,16 +1853,7 @@ func (h *Handler) loadAndValidateRetryRequest(ctx context.Context, req *events.L return nil, nil, err } - if failedExec.Status != "failed" { - return nil, nil, NewClientError(409, fmt.Sprintf("execution %s cannot be retried (status=%s)", failedExec.ExecutionID, failedExec.Status)) - } - - // Authorize BEFORE the already-retried guard so an unauthorized - // caller can't enumerate descendant execution IDs by probing - // failed-row UUIDs (CR #168 review). The status check above is - // allowed pre-RBAC because non-admins can already see the row's - // status via the History endpoint they're entitled to read. - if err := h.authorizeSessionRetry(ctx, session, failedExec); err != nil { + if err := h.authorizeRetrySession(ctx, session, failedExec); err != nil { return nil, nil, err } @@ -1838,6 +1875,31 @@ func (h *Handler) loadAndValidateRetryRequest(ctx context.Context, req *events.L return failedExec, session, nil } +// authorizeRetrySession runs the account-scope gate (issue #92), the status +// guard, and the retry-any/retry-own RBAC matrix, in that order. Extracted +// from loadAndValidateRetryRequest to keep that function under the +// cyclomatic-complexity ceiling; the order is preserved from the original +// inline sequence, so the security boundary is unchanged. +func (h *Handler) authorizeRetrySession(ctx context.Context, session *Session, failedExec *config.PurchaseExecution) error { + // Account-scope gate (issue #92): retry relied only on the retry-any/ + // retry-own RBAC matrix below and never consulted allowed_accounts. Runs + // before the status check so an out-of-scope caller learns nothing about + // the execution, matching the 404 a scoped-out session already gets from + // the History endpoint for the same row. + if err := h.requireExecutionAccess(ctx, session, failedExec.ExecutionID); err != nil { + return err + } + if failedExec.Status != "failed" { + return NewClientError(409, fmt.Sprintf("execution %s cannot be retried (status=%s)", failedExec.ExecutionID, failedExec.Status)) + } + // Authorize BEFORE the already-retried guard so an unauthorized + // caller can't enumerate descendant execution IDs by probing + // failed-row UUIDs (CR #168 review). The status check above is + // allowed pre-RBAC because non-admins can already see the row's + // status via the History endpoint they're entitled to read. + return h.authorizeSessionRetry(ctx, session, failedExec) +} + // checkRetryEligibilityGates runs the provider re-drive-safety (issue // #1668), persistent-failure (Q3) and retry-attempt-threshold (Q2) // gates and returns the appropriate 409 ClientError when any fires. diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index 884863d7a..fbc135fd5 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -250,6 +250,16 @@ func (h *Handler) revokeScheduledExecution(ctx context.Context, session *Session return nil, err } + // Account-scope gate (issue #92): this endpoint relied only on the + // revoke-any/revoke-own RBAC matrix above and never consulted the + // session's allowed_accounts, unlike the sibling pause/resume/run/ + // delete-planned execution endpoints. Runs after RBAC so a session that + // fails the permission check is denied on that (existing) ground without + // requiring an account-scope lookup. + if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { + return nil, err + } + // Atomically transition from scheduled -> canceled and remove suppressions. var cancelledBy *string if session.Email != "" { diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index 7e889f8af..711354809 100644 --- a/internal/api/handler_purchases_revoke_test.go +++ b/internal/api/handler_purchases_revoke_test.go @@ -727,6 +727,9 @@ func TestRevokePurchase_ScheduledExecution_RevokeOwnCreator(t *testing.T) { mockAuth.On("ValidateSession", ctx, "tok").Return(sess, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "revoke-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "revoke-own", "purchases").Return(true, nil) + // requireExecutionAccess (issue #92) runs after RBAC succeeds; unrestricted + // here since scope is not under test. + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) exec := scheduledExecution(execID, userID) mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) @@ -806,6 +809,122 @@ func TestAuthorizeSessionRevokeExecution_NilCreatorDenied(t *testing.T) { assert.Contains(t, ce.message, "cannot revoke another user") } +// TestAuthorizeSessionRevokeExecution_Matrix is the audit finding A16-009 +// regression guard. Before this test, the only three tests reaching +// authorizeSessionRevokeExecution (TestRevokePurchase_ScheduledExecution_ +// RevokeOwnCreator/RevokeOwnWrongCreator and +// TestAuthorizeSessionRevokeExecution_NilCreatorDenied) all stubbed +// revoke-any=false and revoke-own=true, so nothing proved a session holding +// NEITHER permission was refused, nor that the revoke-any allow path or +// either permission-lookup error path worked. Table-drives the four +// remaining branches of the function (handler_purchases_revoke.go): +// +// - hasAny allow (:312): revoke-any:purchases grants access regardless of +// creator, mirroring the sibling authorizeSessionRevoke's revoke-any path. +// - !hasOwn 403 (:320): neither revoke-any nor revoke-own must fail closed. +// - revoke-any lookup error (:309): a HasPermissionAPI failure on the FIRST +// check must propagate rather than silently falling through to the +// revoke-own check. +// - revoke-own lookup error (:317): a HasPermissionAPI failure on the +// SECOND check must propagate rather than being read as "no own +// permission" and denied with the wrong (403) error. +func TestAuthorizeSessionRevokeExecution_Matrix(t *testing.T) { + t.Parallel() + + creator := "u-owner" + permErr := errors.New("permission service unavailable") + + tests := []struct { + name string + hasAnyReturn bool + hasAnyErr error + hasOwnReturn bool + hasOwnErr error + callsHasOwn bool + creatorUserID *string + wantErr bool + wantClientErr bool + wantCode int + }{ + { + // :312 hasAny allow -- grants access without ever consulting + // revoke-own or the execution's creator. + name: "revoke-any allows regardless of creator", + hasAnyReturn: true, + callsHasOwn: false, + wantErr: false, + }, + { + // :320 !hasOwn 403 -- neither permission held. + name: "neither permission denies with 403", + hasAnyReturn: false, + hasOwnReturn: false, + callsHasOwn: true, + creatorUserID: &creator, + wantErr: true, + wantClientErr: true, + wantCode: 403, + }, + { + // :309 revoke-any lookup error -- must propagate as a bare error, + // not get swallowed into a 403 permission-denied. + name: "revoke-any lookup error propagates", + hasAnyErr: permErr, + callsHasOwn: false, + wantErr: true, + }, + { + // :317 revoke-own lookup error -- must propagate as a bare error + // once revoke-any has already returned false. + name: "revoke-own lookup error propagates", + hasAnyReturn: false, + hasOwnErr: permErr, + callsHasOwn: true, + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockAuth.On("HasPermissionAPI", ctx, "u-1", "revoke-any", "purchases"). + Return(tc.hasAnyReturn, tc.hasAnyErr) + if tc.callsHasOwn { + mockAuth.On("HasPermissionAPI", ctx, "u-1", "revoke-own", "purchases"). + Return(tc.hasOwnReturn, tc.hasOwnErr) + } + + h := &Handler{auth: mockAuth} + sess := &Session{UserID: "u-1"} + exec := &config.PurchaseExecution{ExecutionID: "e-1", CreatedByUserID: tc.creatorUserID} + err := h.authorizeSessionRevokeExecution(ctx, sess, exec) + + if !tc.wantErr { + require.NoError(t, err) + return + } + require.Error(t, err) + if !tc.wantClientErr { + // A propagated lookup error must NOT be misread as a + // ClientError (which would surface as a clean 403/404 to the + // caller instead of the 500 a transient auth-backend failure + // deserves). + _, ok := IsClientError(err) + assert.False(t, ok, "expected a bare propagated error, got ClientError: %v", err) + assert.ErrorIs(t, err, permErr) + return + } + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, tc.wantCode, ce.code) + }) + } +} + // --- Two-step quote-then-confirm: Finding #4 TOCTOU tests --- // stubCalcRefundClientWithAmount is a CalculateRefund stub that returns a diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 798a71e4f..026fdebe1 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -2896,6 +2896,101 @@ func TestHandler_pausePlannedPurchase_OutOfScope(t *testing.T) { mockStore.AssertNotCalled(t, "TransitionExecutionStatus") } +// TestHandler_approvePurchase_OutOfScope is the issue #92 regression guard: +// approvePurchase (unlike pause/resume/run/delete-planned) relied only on the +// approve-any/approve-own RBAC matrix and never consulted the session's +// allowed_accounts, so a scoped operator holding approve-any could approve +// -- and fire the real SDK purchase for -- an execution belonging to an +// account outside their scope. A session scoped to allowed_accounts= +// ["Production"] holding approve-any:purchases must get the same 404 that +// requireExecutionAccess already returns for pause/resume/delete-planned +// when acting on an execution whose plan is scoped to a different account +// ("Staging"). Locks down that approvePurchaseViaSession never reaches +// GetGlobalConfig (the first store call past the scope gate) for an +// out-of-scope execution. +func TestHandler_approvePurchase_OutOfScope(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + executionID := "aaaaaaaa-1111-1111-1111-111111111111" + planID := "bbbbbbbb-1111-1111-1111-111111111111" + + mockAuth.On("ValidateSession", ctx, "operator-token").Return(&Session{ + UserID: "operator-1", Email: "operator@example.com", + }, nil) + // approve-any:purchases -- the RBAC matrix that pre-fix was the ONLY gate + // on this endpoint. + mockAuth.On("HasPermissionAPI", ctx, "operator-1", "approve-any", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "operator-1").Return([]string{"Production"}, nil) + mockAuth.On("ValidateCSRFToken", ctx, "operator-token", "").Return(nil) + + mockStore.On("GetExecutionByID", ctx, executionID).Return(&config.PurchaseExecution{ + ExecutionID: executionID, PlanID: planID, Status: "pending", + }, nil) + + store := &mockStoreWithPlanAccounts{ + MockConfigStore: mockStore, + planAccounts: map[string][]config.CloudAccount{ + planID: {{ID: "acc-stage", Name: "Staging"}}, + }, + } + + handler := &Handler{auth: mockAuth, config: store} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer operator-token"}, + } + _, err := handler.approvePurchase(ctx, req, executionID, "") + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + mockStore.AssertNotCalled(t, "GetGlobalConfig") +} + +// TestHandler_retryPurchase_OutOfScope is the issue #92 regression guard for +// the retry endpoint: retryPurchase relied only on the retry-any/retry-own +// RBAC matrix and never consulted allowed_accounts, so a scoped operator +// holding retry-any could relaunch a failed purchase belonging to an account +// outside their scope. A session scoped to allowed_accounts=["Production"] +// holding retry-any:purchases must get the same 404 requireExecutionAccess +// already returns for pause/resume/delete-planned when acting on an +// execution whose plan is scoped to a different account ("Staging"). +func TestHandler_retryPurchase_OutOfScope(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + executionID := "cccccccc-1111-1111-1111-111111111111" + planID := "dddddddd-1111-1111-1111-111111111111" + + mockAuth.On("ValidateSession", ctx, "operator-token").Return(&Session{ + UserID: "operator-1", Email: "operator@example.com", + }, nil) + // retry-any:purchases -- the RBAC matrix that pre-fix was the ONLY gate + // on this endpoint. + mockAuth.On("HasPermissionAPI", ctx, "operator-1", "retry-any", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "operator-1").Return([]string{"Production"}, nil) + + mockStore.On("GetExecutionByID", ctx, executionID).Return(&config.PurchaseExecution{ + ExecutionID: executionID, PlanID: planID, Status: "failed", + }, nil) + + store := &mockStoreWithPlanAccounts{ + MockConfigStore: mockStore, + planAccounts: map[string][]config.CloudAccount{ + planID: {{ID: "acc-stage", Name: "Staging"}}, + }, + } + + handler := &Handler{auth: mockAuth, config: store} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer operator-token"}, + } + _, err := handler.retryPurchase(ctx, req, executionID) + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + mockStore.AssertNotCalled(t, "SavePurchaseExecution") +} + // ─── Session-authed Cancel (issue #46) ───────────────────────────────────── // // Covers the full cancel-any / cancel-own RBAC matrix on the @@ -2936,6 +3031,10 @@ func buildSessionCancelHandler(exec *config.PurchaseExecution, session *Session, if session != nil { mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-any", "purchases").Return(hasAny, nil).Maybe() mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-own", "purchases").Return(hasOwn, nil).Maybe() + // cancelPurchaseViaSession now consults requireExecutionAccess (issue + // #92); an empty allowed_accounts list resolves to unrestricted, which + // is what these RBAC-matrix tests intend to exercise. + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, session.UserID).Return([]string{}, nil).Maybe() } return &Handler{config: mockConfig, auth: mockAuth}, mockConfig, mockAuth @@ -3490,6 +3589,10 @@ func buildSessionRetryHandler(failed *config.PurchaseExecution, session *Session if session != nil { mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-any", "purchases").Return(hasAny, nil).Maybe() mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-own", "purchases").Return(hasOwn, nil).Maybe() + // retryPurchase now consults requireExecutionAccess (issue #92); an + // empty allowed_accounts list resolves to unrestricted, which is what + // these RBAC-matrix tests intend to exercise. + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, session.UserID).Return([]string{}, nil).Maybe() } // SEC-01 constraint check (adversarial review follow-up to #1210): the // retry path now re-evaluates the retrying session's execute:purchases @@ -3976,6 +4079,9 @@ func TestHandler_retryPurchase_PermissionConstraintsDenied(t *testing.T) { // Caller owns the row; retry-own authorizes it (issue #907). mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-own", "purchases").Return(true, nil) + // requireExecutionAccess (issue #92) resolves the session's scope before + // the constraints check; unrestricted here since scope is not under test. + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, session.UserID).Return([]string{}, nil) // The retrying session's execute:purchases permission is capped below // the failed batch's $5,000 total commitment. mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, session.UserID, "execute", "purchases", @@ -5587,6 +5693,9 @@ func TestHandler_revokePurchase_SessionAdminCancelAny(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token"). Return(&Session{UserID: adminUserID, Email: adminEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, adminUserID, "cancel-any", "purchases").Return(true, nil) + // requireExecutionAccess (issue #92) runs after RBAC succeeds; unrestricted + // here since scope is not under test. + mockAuth.On("GetAllowedAccountsAPI", ctx, adminUserID).Return([]string{}, nil) // CSRF is enforced for the session-authed revoke path (tryRevokeViaSession). // No CSRF token header in this request, so csrfToken is "". mockAuth.On("ValidateCSRFToken", ctx, "admin-token", "").Return(nil) @@ -5630,6 +5739,9 @@ func TestHandler_revokePurchase_SessionOwnerCancelOwn(t *testing.T) { Return(&Session{UserID: ownerUserID, Email: ownerEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-own", "purchases").Return(true, nil) + // requireExecutionAccess (issue #92) runs after RBAC succeeds; unrestricted + // here since scope is not under test. + mockAuth.On("GetAllowedAccountsAPI", ctx, ownerUserID).Return([]string{}, nil) // CSRF is enforced for the session-authed revoke path (tryRevokeViaSession). mockAuth.On("ValidateCSRFToken", ctx, "owner-token", "").Return(nil) From 764370897dab5807f84273f96ec23e7a33b1f5d1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 18:58:47 +0200 Subject: [PATCH 2/4] sec(api): scope purchase executions by their own account, scope gate first requireExecutionAccess only checked that the execution's plan accounts intersect the session's allowed_accounts. With an operator scoped to A, a plan spanning {A, B} and an execution for B, the check passed, so approve, cancel, retry, revoke and run-now all let the operator act on B's money. It now also requires the session to be allowed the execution's own CloudAccountID when one is set (via requireAccountAccess, which resolves the account name for name-based allow-lists). A nil CloudAccountID keeps the plan-only check, and an empty PlanID still fails closed. The scope gate now runs before RBAC and Constraints on every session path: the approve, cancel and revoke dispatchers, approvePurchaseViaSession, cancelPurchaseViaSession, tryRevokeViaSession and revokeScheduledExecution. Previously an in-RBAC but out-of-scope caller could get a Constraints 403 before the scope 404, confirming the execution exists; after #378 the approve dispatcher also evaluated Constraints before the gate. A scope miss in a dispatcher falls through to the email-token branch like an RBAC 403, so the contact_email token path stays unscoped as issue #92 intends; without a token the session branch re-runs the gate and returns 404 (401 from the revoke dispatcher, as for an RBAC denial). Tests: TestExecutionEndpoints_OutOfScopeExecutionRefused drives approve, cancel, retry, both revoke paths and run-now with a Production-scoped caller holding every money verb, against a plan-out-of-scope execution and a cross-account execution, asserting refusal and no mutation. TestRequireExecutionAccess_InScopeCloudAccountAllowed is the positive control. Tests reaching the dispatchers with an unscoped session now mock GetAllowedAccountsAPI, since the gate runs first. --- internal/api/coverage_extras_test.go | 4 + .../api/executed_notification_flow_test.go | 4 + internal/api/execution_scope_test.go | 191 ++++++++++++++++++ internal/api/handler_purchases.go | 96 +++++---- internal/api/handler_purchases_revoke.go | 13 +- internal/api/handler_purchases_revoke_test.go | 4 +- internal/api/handler_purchases_test.go | 37 +++- internal/api/handler_test.go | 4 + internal/api/middleware_test.go | 1 + internal/api/scoping.go | 16 +- 10 files changed, 305 insertions(+), 65 deletions(-) create mode 100644 internal/api/execution_scope_test.go diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 3ddb854dc..e8380f42b 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -84,6 +84,8 @@ func TestHandler_approvePurchase_PurchaseError(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) // After issue #286, approvePurchase is session-first: with a Bearer // header present the dispatch consults the approve-{any,own} RBAC @@ -155,6 +157,8 @@ func TestHandler_cancelPurchase_PurchaseError(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) // Session has no admin role / cancel permissions → cancelPurchase's // session-authed pre-check falls through to authorizeApprovalAction → diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go index 5b73ff6ea..2e3f223ab 100644 --- a/internal/api/executed_notification_flow_test.go +++ b/internal/api/executed_notification_flow_test.go @@ -103,6 +103,8 @@ func TestExecutedNotification_TokenApprovePath(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() @@ -166,6 +168,8 @@ func TestExecutedNotification_TokenApprovePath_RefetchFailureSuppressesPanel(t * }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go new file mode 100644 index 000000000..ed2ecfeb5 --- /dev/null +++ b/internal/api/execution_scope_test.go @@ -0,0 +1,191 @@ +package api + +import ( + "context" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Issue #92 regression guards for the per-execution account-scope gate on the +// money-moving execution endpoints. The caller is an admin restricted to +// allowed_accounts=["Production"] who holds every money verb, so every RBAC +// check passes and only the scope gate can stop the mutation. +const ( + scopeTestExecID = "92929292-0000-0000-0000-000000000001" + scopeTestPlanID = "92929292-0000-0000-0000-000000000002" +) + +var ( + scopeTestProd = config.CloudAccount{ID: "acc-prod", Name: "Production"} + scopeTestStage = config.CloudAccount{ID: "acc-stage", Name: "Staging"} +) + +type scopeTestCase struct { + name string + planAccounts []config.CloudAccount + cloudAccountID *string +} + +var scopeTestCases = []scopeTestCase{ + // The plan is attributed only to an account outside the caller's scope. + {name: "plan out of scope", planAccounts: []config.CloudAccount{scopeTestStage}}, + // The plan spans an in-scope and an out-of-scope account, so the plan + // check passes; the execution itself targets the out-of-scope account. + {name: "cross-account execution", planAccounts: []config.CloudAccount{scopeTestProd, scopeTestStage}, cloudAccountID: &scopeTestStage.ID}, +} + +type scopeTestEndpoint struct { + name string + status string + call func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) + // mutation is the store method the endpoint reaches only once every + // gate has passed; empty when the mutation goes through the purchase + // manager instead (purchaseMutation). + mutation string + purchaseMutation string +} + +var scopeTestEndpoints = []scopeTestEndpoint{ + { + name: "approve", status: "pending", purchaseMutation: "ApproveAndExecute", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.approvePurchase(context.Background(), req, scopeTestExecID, "") + }, + }, + { + name: "cancel", status: "pending", mutation: "CancelExecutionAtomic", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.cancelPurchase(context.Background(), req, scopeTestExecID, "") + }, + }, + { + name: "retry", status: "failed", mutation: "SavePurchaseExecution", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.retryPurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "revoke completed via session", status: "completed", mutation: "TransitionExecutionStatus", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }, + }, + { + name: "revoke scheduled", status: "scheduled", mutation: "CancelScheduledExecutionAtomic", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokePurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "run now", status: "pending", purchaseMutation: "RunPlannedPurchaseNow", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.runPlannedPurchase(context.Background(), req, scopeTestExecID) + }, + }, +} + +func TestExecutionEndpoints_OutOfScopeExecutionRefused(t *testing.T) { + for _, sc := range scopeTestCases { + for _, ep := range scopeTestEndpoints { + t.Run(sc.name+"/"+ep.name, func(t *testing.T) { + store := new(MockConfigStore) + mockAuth := new(MockAuthService) + mockPurchase := new(MockPurchaseManager) + + mockAuth.On("ValidateSession", mock.Anything, "operator-token").Return(&Session{ + UserID: "operator-1", Email: "operator@example.com", + }, nil) + mockAuth.On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + // admin:* plus the money verbs it deliberately does not cover + // (adminCarvedOuts), so every RBAC gate passes. + mockAuth.grantPermissionsScoped([]auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases}, + {Action: auth.ActionApproveAny, Resource: auth.ResourcePurchases}, + {Action: auth.ActionRetryAny, Resource: auth.ResourcePurchases}, + }, []string{scopeTestProd.Name}) + + creator := "operator-1" + store.On("GetExecutionByID", mock.Anything, scopeTestExecID).Return(&config.PurchaseExecution{ + ExecutionID: scopeTestExecID, + PlanID: scopeTestPlanID, + Status: ep.status, + CloudAccountID: sc.cloudAccountID, + CreatedByUserID: &creator, + }, nil) + store.GetPlanAccountsFn = func(_ context.Context, planID string) ([]config.CloudAccount, error) { + if planID != scopeTestPlanID { + return nil, nil + } + return sc.planAccounts, nil + } + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + for _, a := range []config.CloudAccount{scopeTestProd, scopeTestStage} { + if a.ID == id { + return &a, nil + } + } + return nil, nil + } + + h := &Handler{auth: mockAuth, config: store, purchase: mockPurchase} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer operator-token"}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "POST"}, + }, + } + + _, err := ep.call(h, req) + require.Error(t, err) + if ep.mutation != "" { + store.AssertNotCalled(t, ep.mutation) + } + if ep.purchaseMutation != "" { + mockPurchase.AssertNotCalled(t, ep.purchaseMutation) + } + // The session-authed revoke hands an out-of-scope session to + // the token branch, which answers 401 without a token; every + // other endpoint returns the enumeration-safe 404. + if ep.name == "revoke completed via session" { + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error, got %v", err) + assert.Equal(t, 401, ce.code) + return + } + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + }) + } + } +} + +// TestRequireExecutionAccess_InScopeCloudAccountAllowed is the positive +// control for the cross-account case above: the same plan, with the execution +// targeting the in-scope account, passes the gate. +func TestRequireExecutionAccess_InScopeCloudAccountAllowed(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + mockAuth := new(MockAuthService) + mockAuth.grantScoped(scopeTestProd.Name) + store.On("GetExecutionByID", ctx, scopeTestExecID).Return(&config.PurchaseExecution{ + ExecutionID: scopeTestExecID, PlanID: scopeTestPlanID, CloudAccountID: &scopeTestProd.ID, + }, nil) + store.GetPlanAccountsFn = func(context.Context, string) ([]config.CloudAccount, error) { + return []config.CloudAccount{scopeTestProd, scopeTestStage}, nil + } + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + require.Equal(t, scopeTestProd.ID, id) + return &scopeTestProd, nil + } + + h := &Handler{auth: mockAuth, config: store} + require.NoError(t, h.requireExecutionAccess(ctx, &Session{UserID: "operator-1"}, scopeTestExecID)) + store.AssertCalled(t, "GetCloudAccount", ctx, scopeTestProd.ID) +} diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 86fb0c1d7..ff6d8f846 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -594,13 +594,15 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio // approvePurchaseViaSession runs the approve-any / // approve-own RBAC matrix and rejects sessions without it. if session := h.tryGetSession(ctx, req); session != nil { - switch err := h.authorizeSessionApprove(ctx, session, execution); { + switch err := h.authorizeScopedSessionApprove(ctx, session, execution); { case err == nil: return h.approvePurchaseViaSession(ctx, req, execution) - case isPermissionDenied(err): - // Explicit 403 → fall through to the token branch so the - // contact_email gate gets a chance (a logged-in user without - // approve-* may still be the per-account contact recipient). + case fallsThroughToToken(err): + // Explicit 403 or out-of-scope 404 → fall through to the token + // branch so the contact_email gate gets a chance (a logged-in + // user without approve-* or outside the account scope may still + // be the per-account contact recipient). Without a token the + // session branch below re-runs the gate and returns the error. default: // Transient failure — propagate instead of silently widening. return nil, err @@ -773,23 +775,11 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam } // authorizeApproveSession runs every gate approvePurchaseViaSession must pass -// before mutating state: the approve-any/approve-own RBAC matrix, the -// account-scope check (issue #92), and the 4-eyes distinct-approver rule. -// Extracted from approvePurchaseViaSession to keep that function under the -// cyclomatic-complexity ceiling; the three checks stay in this order so the -// security boundary is unchanged (RBAC first, so a caller who fails it and -// falls through to the contact_email token path is unaffected by the scope -// or 4-eyes checks; 4-eyes last, since only a caller admin's own RBAC has -// already approved needs it). +// before mutating state: the account-scope check (issue #92), the +// approve-any/approve-own RBAC matrix with its Constraints, and the 4-eyes +// distinct-approver rule. func (h *Handler) authorizeApproveSession(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { - if err := h.authorizeSessionApprove(ctx, session, execution); err != nil { - return err - } - // Account-scope gate (issue #92): approve is a money-moving endpoint - // that, unlike pause/resume/run/delete-planned, relied only on the - // approve-any/approve-own RBAC matrix above and never consulted the - // session's allowed_accounts. - if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { + if err := h.authorizeScopedSessionApprove(ctx, session, execution); err != nil { return err } // 4-eyes mode (issue #1005): enforce after RBAC so only users who already @@ -799,6 +789,16 @@ func (h *Handler) authorizeApproveSession(ctx context.Context, session *Session, return h.requireDifferentApprover(ctx, session, execution) } +// authorizeScopedSessionApprove runs the account-scope gate (issue #92) +// before the approve RBAC and Constraints checks, so an out-of-scope caller +// gets the enumeration-safe 404 before a 403 can reveal the execution exists. +func (h *Handler) authorizeScopedSessionApprove(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { + if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { + return err + } + return h.authorizeSessionApprove(ctx, session, execution) +} + // authorizeSessionApprove returns nil when the session is permitted to // approve the given execution under the approve-any / approve-own RBAC // rules added in issue #286. Returns a 403 ClientError otherwise. @@ -1197,15 +1197,16 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction // token. cancelPurchaseViaSession runs the cancel-any / // cancel-own RBAC matrix and rejects sessions without it. if session := h.tryGetSession(ctx, req); session != nil { - switch err := h.authorizeSessionCancel(ctx, session, execution); { + switch err := h.authorizeCancelSession(ctx, session, execution); { case err == nil: // Session is RBAC-authorized → run the session-authed cancel. return h.cancelPurchaseViaSession(ctx, req, execution) - case isPermissionDenied(err): - // Explicit "permission denied" (403) → fall through to the - // token branch so the contact_email gate still gets a chance - // (a logged-in user without admin / cancel-* may still be - // the per-account contact email recipient). + case fallsThroughToToken(err): + // Explicit "permission denied" (403) or out-of-scope (404) → + // fall through to the token branch so the contact_email gate + // still gets a chance (a logged-in user without admin / + // cancel-* may still be the per-account contact email + // recipient). default: // Transient failure (auth-service down, HasPermissionAPI // returning a wrapped error, h.auth==nil 500). Propagate @@ -1462,18 +1463,8 @@ func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFun if session == nil { return nil, false, nil } - switch sessErr := h.authorizeSessionCancel(ctx, session, execution); { + switch sessErr := h.authorizeCancelSession(ctx, session, execution); { case sessErr == nil: - // Account-scope gate (issue #92): the session-authed revoke branch - // relied only on the cancel-any/cancel-own RBAC matrix above and never - // consulted allowed_accounts. Runs after RBAC succeeds, alongside the - // CSRF check below, so a caller who fails RBAC and falls through to - // the contact_email token path is unaffected. Unlike a - // permission-denied result, a scope miss has no legitimate fallback to - // the token branch, so it is a hard failure. - if scopeErr := h.requireExecutionAccess(ctx, session, execution.ExecutionID); scopeErr != nil { - return nil, true, scopeErr - } // These endpoints are AuthPublic so the outer middleware skips CSRF. // Enforce it here for the session-authed revoke sub-path, consistent // with cancelPurchaseViaSession and approvePurchaseViaSession which both @@ -1483,7 +1474,7 @@ func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFun } res, revokeErr := h.revokeViaSession(ctx, execution, session.Email) return res, true, revokeErr - case isPermissionDenied(sessErr): + case fallsThroughToToken(sessErr): // Fall through to the token branch. return nil, false, nil default: @@ -1623,21 +1614,15 @@ func (h *Handler) revokeViaSession(ctx context.Context, execution *config.Purcha }, nil } -// authorizeCancelSession combines the cancel-any/cancel-own RBAC matrix with -// the account-scope check (issue #92) for cancelPurchaseViaSession. Extracted -// to keep that function under the cyclomatic-complexity ceiling. Runs RBAC -// first so a caller who fails it and falls through to the contact_email -// token path (in cancelPurchase's dispatcher) is unaffected by the scope -// check. Not used by the OTHER two callers of authorizeSessionCancel -// (cancelPurchase's dispatcher, tryRevokeViaSession) which call the -// unwrapped RBAC-only check directly, since a permission-denied result there -// must fall through to the token path rather than the scope check ever -// running. +// authorizeCancelSession runs the account-scope gate (issue #92) before the +// cancel-any/cancel-own RBAC matrix, so an out-of-scope caller gets the +// enumeration-safe 404 before a 403 can reveal the execution exists. Shared +// by the cancel and revoke session paths. func (h *Handler) authorizeCancelSession(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { - if err := h.authorizeSessionCancel(ctx, session, execution); err != nil { + if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { return err } - return h.requireExecutionAccess(ctx, session, execution.ExecutionID) + return h.authorizeSessionCancel(ctx, session, execution) } // authorizeSessionCancel returns nil when the session is permitted to cancel @@ -2262,6 +2247,15 @@ func wrapConstraintDenied(err error) error { // failures into the contact_email gate — exactly the misclassification // the propagate-vs-fall-through split is meant to prevent. constraintDeniedError // above deliberately exploits the same strictness from the other direction. +// fallsThroughToToken reports whether a session-authorization failure should +// hand the request to the email-token branch: an RBAC denial (403) or an +// account-scope miss (404). The token branch's contact_email gate is +// deliberately not account-scoped (issue #92). Both matches are strict, like +// isPermissionDenied, so a wrapped error from deeper in the chain propagates. +func fallsThroughToToken(err error) bool { + return isPermissionDenied(err) || err == errNotFound //nolint:errorlint // strict sentinel identity is deliberate +} + func isPermissionDenied(err error) bool { ce, ok := err.(*clientError) //nolint:errorlint // strict (unwrapped) assertion is deliberate; see comment above return ok && ce.code == 403 diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index fbc135fd5..b4ced4e15 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -246,17 +246,12 @@ func (h *Handler) revokeScheduledExecution(ctx context.Context, session *Session // cancel it before any cloud call. Let CancelScheduledExecutionAtomic be the // sole arbiter: it returns canceled=false (-> 410) only when the row has // actually moved out of "scheduled". - if err := h.authorizeSessionRevokeExecution(ctx, session, execution); err != nil { + // Account-scope gate (issue #92), ahead of RBAC so an out-of-scope caller + // gets the enumeration-safe 404 rather than a 403 that confirms the row. + if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { return nil, err } - - // Account-scope gate (issue #92): this endpoint relied only on the - // revoke-any/revoke-own RBAC matrix above and never consulted the - // session's allowed_accounts, unlike the sibling pause/resume/run/ - // delete-planned execution endpoints. Runs after RBAC so a session that - // fails the permission check is denied on that (existing) ground without - // requiring an account-scope lookup. - if err := h.requireExecutionAccess(ctx, session, execution.ExecutionID); err != nil { + if err := h.authorizeSessionRevokeExecution(ctx, session, execution); err != nil { return nil, err } diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index 711354809..5c3dee2e3 100644 --- a/internal/api/handler_purchases_revoke_test.go +++ b/internal/api/handler_purchases_revoke_test.go @@ -727,7 +727,7 @@ func TestRevokePurchase_ScheduledExecution_RevokeOwnCreator(t *testing.T) { mockAuth.On("ValidateSession", ctx, "tok").Return(sess, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "revoke-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "revoke-own", "purchases").Return(true, nil) - // requireExecutionAccess (issue #92) runs after RBAC succeeds; unrestricted + // requireExecutionAccess (issue #92) runs before RBAC; unrestricted // here since scope is not under test. mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) @@ -749,6 +749,7 @@ func TestRevokePurchase_ScheduledExecution_RevokeOwnWrongCreator(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() t.Cleanup(func() { mockStore.AssertExpectations(t) mockAuth.AssertExpectations(t) @@ -1663,6 +1664,7 @@ func TestRevokePurchase_ConcurrentScheduledRevoke_OneWinsOneGets410(t *testing.T t.Parallel() mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() t.Cleanup(func() { mockStore.AssertExpectations(t) mockAuth.AssertExpectations(t) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 026fdebe1..382e06c97 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -58,6 +58,8 @@ func TestHandler_approvePurchase(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) // After issue #286 the session-authed approve dispatch consults the // approve-{any,own} verb matrix BEFORE falling through to the token @@ -99,6 +101,8 @@ func TestHandler_cancelPurchase(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) // Session has no admin role and no cancel permissions, so cancelPurchase's // session-authed pre-check (added to fix the deep-link contact_email gate) @@ -135,6 +139,8 @@ func TestHandler_approvePurchase_RejectsMismatchedSession(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Session belongs to someone who is NOT the authorized approver. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: "wrong@example.com"}, nil) // After issue #286 the dispatch consults approve-{any,own} BEFORE @@ -193,6 +199,8 @@ func TestHandler_approvePurchase_RejectsMissingContactEmail(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: globalNotify}, nil) // Issue #286 dispatch consults approve-{any,own} BEFORE the contact_email // gate. globalNotify session has neither verb → 403 → fall through to @@ -259,6 +267,8 @@ func TestHandler_approvePurchase_AcceptsContactEmailSession(t *testing.T) { } mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Session email matches the account contact email — global notify is // NOT enough here because a contact email exists for the account. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) @@ -427,6 +437,8 @@ func TestHandler_approvePurchase_SessionApproveAny_PermissionConstraintsDenied(t mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() t.Cleanup(func() { mockAuth.AssertExpectations(t) }) userSession := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", Email: "capped@example.com"} mockAuth.On("ValidateSession", ctx, "sess-tok").Return(userSession, nil) @@ -503,6 +515,8 @@ func TestHandler_approvePurchase_SessionApproveAny_TokenPresent_ConstraintsDenie // (wired to succeed) to isolate approveConstraintsForSession's own check. mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockAuth.On("ValidateSession", ctx, "capped-token").Return(capped, nil) mockAuth.On("HasPermissionAPI", ctx, capped.UserID, "approve-any", "purchases").Return(true, nil) @@ -584,6 +598,8 @@ func TestHandler_approveViaToken_GlobalConfigError_FailsClosed(t *testing.T) { mockConfig.On("GetGlobalConfig", ctx).Return(nil, errors.New("db transient error")) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) // No approve-any / approve-own permissions — dispatch falls to token path. mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() @@ -910,6 +926,8 @@ func TestHandler_approvePurchase_RejectsGlobalNotifyWhenContactSet(t *testing.T) } mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: globalNotify}, nil) // Issue #286: dispatch consults approve-{any,own} BEFORE the // contact_email gate. Returning false for both verbs lets the @@ -959,6 +977,8 @@ func TestHandler_approvePurchase_RejectsCreatorWithoutApprovePermission(t *testi mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Session belongs to the creator themselves but holds no approve verb. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{ UserID: creatorUserID, @@ -3499,6 +3519,8 @@ func TestHandler_cancelPurchase_DeepLink_TransientAuthErrorPropagates(t *testing mockConfig.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, nil) // Simulate a transient auth-backend failure on the cancel-any check. // authorizeSessionCancel wraps this as "permission check failed: …" @@ -5514,6 +5536,8 @@ func TestHandler_revokePurchase_ValidToken(t *testing.T) { mockStore.On("SetCancelledBy", ctx, execID, revokerEmail).Return(nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Provide a session for the revoker so authorizeApprovalAction resolves actor. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: revokerEmail}, nil) // RBAC: revoker has no cancel-any or cancel-own, so falls through to token path. @@ -5559,6 +5583,8 @@ func TestHandler_revokePurchase_InvalidToken(t *testing.T) { } mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -5650,6 +5676,8 @@ func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { } mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -5693,7 +5721,7 @@ func TestHandler_revokePurchase_SessionAdminCancelAny(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token"). Return(&Session{UserID: adminUserID, Email: adminEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, adminUserID, "cancel-any", "purchases").Return(true, nil) - // requireExecutionAccess (issue #92) runs after RBAC succeeds; unrestricted + // requireExecutionAccess (issue #92) runs before RBAC; unrestricted // here since scope is not under test. mockAuth.On("GetAllowedAccountsAPI", ctx, adminUserID).Return([]string{}, nil) // CSRF is enforced for the session-authed revoke path (tryRevokeViaSession). @@ -5739,7 +5767,7 @@ func TestHandler_revokePurchase_SessionOwnerCancelOwn(t *testing.T) { Return(&Session{UserID: ownerUserID, Email: ownerEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-own", "purchases").Return(true, nil) - // requireExecutionAccess (issue #92) runs after RBAC succeeds; unrestricted + // requireExecutionAccess (issue #92) runs before RBAC; unrestricted // here since scope is not under test. mockAuth.On("GetAllowedAccountsAPI", ctx, ownerUserID).Return([]string{}, nil) // CSRF is enforced for the session-authed revoke path (tryRevokeViaSession). @@ -5773,6 +5801,8 @@ func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "user-token"). Return(&Session{UserID: userID, Email: userEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) @@ -6140,6 +6170,8 @@ func TestRevokePurchase_POSTPerformsRevoke(t *testing.T) { mockStore.On("SetCancelledBy", ctx, execID, revokerEmail).Return(nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: revokerEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -6330,6 +6362,7 @@ func TestRequireDifferentApprover_EmailTokenPath_ModeOn(t *testing.T) { // Session identifies the user as the CREATOR via UserID. creatorSession := &Session{UserID: creatorID, Email: contactEmail} mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Session exists but lacks approve-* permission, so the dispatch falls // through to the token branch. The 4-eyes check then uses the session // identity against CreatedByUserID and must deny. diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 340304de5..c01779b87 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -864,6 +864,8 @@ func TestHandler_HandleRequest_ApprovePurchase(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil) // Issue #286: dispatch consults approve-{any,own} BEFORE the // contact_email gate. Returning false for both verbs lets the @@ -924,6 +926,8 @@ func TestHandler_HandleRequest_CancelPurchase(t *testing.T) { }, nil) mockAuth := new(MockAuthService) + + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil) // Session has no admin role / cancel permissions → cancelPurchase's // session-authed pre-check falls through to authorizeApprovalAction → diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 8a319ccf4..9988e8a84 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -662,6 +662,7 @@ func TestTokenOnlyApprove_BypassesCSRF(t *testing.T) { // falls through to the token branch. CSRF is NOT called on the token // branch (approvePurchaseViaSession is never reached). mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 6ab6c671b..2566459a9 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -144,7 +144,11 @@ func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, sessi } // requireExecutionAccess rejects with errNotFound when the execution's plan's -// associated accounts don't intersect with the session's allowed_accounts. +// associated accounts don't intersect with the session's allowed_accounts, +// or when the execution targets a CloudAccountID outside them. The plan +// check alone is not enough: a plan spanning accounts {A, B} intersects a +// session scoped to A, yet its execution for B still spends B's money +// (issue #92). // Convenience wrapper around requirePlanAccess for the pause/resume/run/ // delete/details handlers that key on executionID. Returns errNotFound when // the execution itself doesn't exist, so unauthenticated probing can't @@ -169,7 +173,15 @@ func (h *Handler) requireExecutionAccess(ctx context.Context, session *Session, if err != nil { return fmt.Errorf("failed to get execution: %w", err) } - return h.requirePlanAccess(ctx, session, execution.PlanID) + if err := h.requirePlanAccess(ctx, session, execution.PlanID); err != nil { + return err + } + if execution.CloudAccountID != nil { + if _, err := h.requireAccountAccess(ctx, session, *execution.CloudAccountID); err != nil { + return err + } + } + return nil } // resolveAccountFilterIDs maps requested cloud_accounts UUIDs to the From 5907bb80fa8345c3916b9503f3d0fcb68564ffb9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 19:29:33 +0200 Subject: [PATCH 3/4] sec(api): scope executions by every account they can buy in Replace the ad-hoc plan-intersection and CloudAccountID checks in requireExecutionAccess with one helper, executionAccounts, which returns the full set of cloud accounts an execution can buy in: its CloudAccountID when set; otherwise every account of its plan (a plan-level parent fans out to all of them); otherwise, for an ad-hoc purchase with no plan, every recommendation's account, refusing any recommendation without one. A scoped session must hold every account in that set, and an empty set is a deny. - A session scoped to A can no longer approve, retry or run-now a plan parent that buys in B. - Ad-hoc executions no longer call GetPlanAccounts with an empty plan ID, which 500ed for scoped sessions and token holders. - An out-of-scope session falls through to the email-token branch only when a token is present; otherwise it gets the same 404 as a missing row instead of status-revealing 409/410/401 answers. - Purchase details and the planned/upcoming lists use the same rule. Refs #92 --- internal/api/execution_scope_test.go | 361 +++++++++++++++++++-------- internal/api/handler_dashboard.go | 12 +- internal/api/handler_purchases.go | 89 +++---- internal/api/scoping.go | 116 +++++++-- 4 files changed, 412 insertions(+), 166 deletions(-) diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index ed2ecfeb5..a4b747496 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -2,6 +2,7 @@ package api import ( "context" + "errors" "testing" "github.com/aws/aws-lambda-go/events" @@ -14,31 +15,67 @@ import ( ) // Issue #92 regression guards for the per-execution account-scope gate on the -// money-moving execution endpoints. The caller is an admin restricted to -// allowed_accounts=["Production"] who holds every money verb, so every RBAC -// check passes and only the scope gate can stop the mutation. +// execution endpoints. The caller is an admin restricted by allowed_accounts +// who holds every money verb, so every RBAC check passes and only the scope +// gate can stop the action. const ( scopeTestExecID = "92929292-0000-0000-0000-000000000001" scopeTestPlanID = "92929292-0000-0000-0000-000000000002" ) +var errScopeTestReached = errors.New("mutation reached") + var ( - scopeTestProd = config.CloudAccount{ID: "acc-prod", Name: "Production"} - scopeTestStage = config.CloudAccount{ID: "acc-stage", Name: "Staging"} + scopeTestProd = config.CloudAccount{ID: "acc-prod", Name: "Production", ContactEmail: "prod-owner@example.com"} + scopeTestStage = config.CloudAccount{ID: "acc-stage", Name: "Staging", ContactEmail: "operator@example.com"} ) -type scopeTestCase struct { +// scopeTestKind is one shape of execution, differing in how its buy-in +// accounts are derived (executionAccounts). +type scopeTestKind struct { name string - planAccounts []config.CloudAccount + planID string cloudAccountID *string + recs []config.RecommendationRecord + // partialScope holds some but not all of the accounts the execution can + // buy in (or, for the single-account child, the plan's other account). + partialScope []string } -var scopeTestCases = []scopeTestCase{ - // The plan is attributed only to an account outside the caller's scope. - {name: "plan out of scope", planAccounts: []config.CloudAccount{scopeTestStage}}, - // The plan spans an in-scope and an out-of-scope account, so the plan - // check passes; the execution itself targets the out-of-scope account. - {name: "cross-account execution", planAccounts: []config.CloudAccount{scopeTestProd, scopeTestStage}, cloudAccountID: &scopeTestStage.ID}, +// scopeTestRec is a purchasable recommendation, so an allowed call clears +// the post-scope constraint checks and reaches the mutation. +func scopeTestRec(id string, account *string) config.RecommendationRecord { + return config.RecommendationRecord{ + ID: id, Provider: "aws", Service: "ec2", Region: "us-east-1", + Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, CloudAccountID: account, + } +} + +var scopeTestBothRecs = []config.RecommendationRecord{ + scopeTestRec("rec-prod", &scopeTestProd.ID), + scopeTestRec("rec-stage", &scopeTestStage.ID), +} + +var scopeTestKinds = []scopeTestKind{ + // Plan-level parent: CloudAccountID nil, fans out to every plan account. + {name: "plan parent", planID: scopeTestPlanID, recs: scopeTestBothRecs, partialScope: []string{scopeTestProd.Name}}, + // Fan-out child of the same plan, buying only in Staging. + {name: "fan-out child", planID: scopeTestPlanID, cloudAccountID: &scopeTestStage.ID, + recs: []config.RecommendationRecord{scopeTestRec("rec-stage", &scopeTestStage.ID)}, partialScope: []string{scopeTestProd.Name}}, + // Ad-hoc web purchase: plan_id NULL, one recommendation per account. + {name: "ad-hoc", recs: scopeTestBothRecs, partialScope: []string{scopeTestProd.Name}}, +} + +type scopeTestScope struct { + name string + scope func(k scopeTestKind) []string + allow bool +} + +var scopeTestScopes = []scopeTestScope{ + {name: "all in scope", allow: true, scope: func(scopeTestKind) []string { return []string{scopeTestProd.Name, scopeTestStage.Name} }}, + {name: "partial scope", scope: func(k scopeTestKind) []string { return k.partialScope }}, + {name: "no scope", scope: func(scopeTestKind) []string { return []string{"Other"} }}, } type scopeTestEndpoint struct { @@ -46,8 +83,8 @@ type scopeTestEndpoint struct { status string call func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) // mutation is the store method the endpoint reaches only once every - // gate has passed; empty when the mutation goes through the purchase - // manager instead (purchaseMutation). + // gate has passed; purchaseMutation is the purchase-manager equivalent. + // Both empty for read-only endpoints. mutation string purchaseMutation string } @@ -89,103 +126,223 @@ var scopeTestEndpoints = []scopeTestEndpoint{ return h.runPlannedPurchase(context.Background(), req, scopeTestExecID) }, }, + { + name: "pause", status: "pending", mutation: "TransitionExecutionStatus", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.pausePlannedPurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "resume", status: "paused", mutation: "TransitionExecutionStatus", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.resumePlannedPurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "delete", status: "pending", mutation: "TransitionExecutionStatus", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.deletePlannedPurchase(context.Background(), req, scopeTestExecID) + }, + }, + { + name: "details", status: "completed", + call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.getPurchaseDetails(context.Background(), req, scopeTestExecID) + }, + }, } -func TestExecutionEndpoints_OutOfScopeExecutionRefused(t *testing.T) { - for _, sc := range scopeTestCases { - for _, ep := range scopeTestEndpoints { - t.Run(sc.name+"/"+ep.name, func(t *testing.T) { - store := new(MockConfigStore) - mockAuth := new(MockAuthService) - mockPurchase := new(MockPurchaseManager) - - mockAuth.On("ValidateSession", mock.Anything, "operator-token").Return(&Session{ - UserID: "operator-1", Email: "operator@example.com", - }, nil) - mockAuth.On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() - // admin:* plus the money verbs it deliberately does not cover - // (adminCarvedOuts), so every RBAC gate passes. - mockAuth.grantPermissionsScoped([]auth.Permission{ - {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, - {Action: auth.ActionExecute, Resource: auth.ResourcePurchases}, - {Action: auth.ActionApproveAny, Resource: auth.ResourcePurchases}, - {Action: auth.ActionRetryAny, Resource: auth.ResourcePurchases}, - }, []string{scopeTestProd.Name}) - - creator := "operator-1" - store.On("GetExecutionByID", mock.Anything, scopeTestExecID).Return(&config.PurchaseExecution{ - ExecutionID: scopeTestExecID, - PlanID: scopeTestPlanID, - Status: ep.status, - CloudAccountID: sc.cloudAccountID, - CreatedByUserID: &creator, - }, nil) - store.GetPlanAccountsFn = func(_ context.Context, planID string) ([]config.CloudAccount, error) { - if planID != scopeTestPlanID { - return nil, nil - } - return sc.planAccounts, nil - } - store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { - for _, a := range []config.CloudAccount{scopeTestProd, scopeTestStage} { - if a.ID == id { - return &a, nil - } - } - return nil, nil - } - - h := &Handler{auth: mockAuth, config: store, purchase: mockPurchase} - req := &events.LambdaFunctionURLRequest{ - Headers: map[string]string{"Authorization": "Bearer operator-token"}, - RequestContext: events.LambdaFunctionURLRequestContext{ - HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "POST"}, - }, - } - - _, err := ep.call(h, req) - require.Error(t, err) - if ep.mutation != "" { - store.AssertNotCalled(t, ep.mutation) - } - if ep.purchaseMutation != "" { - mockPurchase.AssertNotCalled(t, ep.purchaseMutation) - } - // The session-authed revoke hands an out-of-scope session to - // the token branch, which answers 401 without a token; every - // other endpoint returns the enumeration-safe 404. - if ep.name == "revoke completed via session" { - ce, ok := IsClientError(err) - require.True(t, ok, "expected a client error, got %v", err) - assert.Equal(t, 401, ce.code) - return - } - assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) - }) - } +// scopeTestExecution builds the execution row for a kind. +func scopeTestExecution(k scopeTestKind, status string) *config.PurchaseExecution { + creator := "operator-1" + return &config.PurchaseExecution{ + ExecutionID: scopeTestExecID, + PlanID: k.planID, + Status: status, + CloudAccountID: k.cloudAccountID, + Recommendations: k.recs, + CreatedByUserID: &creator, } } -// TestRequireExecutionAccess_InScopeCloudAccountAllowed is the positive -// control for the cross-account case above: the same plan, with the execution -// targeting the in-scope account, passes the gate. -func TestRequireExecutionAccess_InScopeCloudAccountAllowed(t *testing.T) { - ctx := context.Background() +// newScopeTestHandler wires a handler whose session is scoped to scope and +// whose store serves exec. GetPlanAccounts("") errors the way the UUID +// plan_id column does in Postgres, so a gate that queries it for an ad-hoc +// execution surfaces as a 500 rather than a quiet deny. +func newScopeTestHandler(t *testing.T, exec *config.PurchaseExecution, scope []string) (*Handler, *MockConfigStore, *MockPurchaseManager) { + t.Helper() store := new(MockConfigStore) mockAuth := new(MockAuthService) - mockAuth.grantScoped(scopeTestProd.Name) - store.On("GetExecutionByID", ctx, scopeTestExecID).Return(&config.PurchaseExecution{ - ExecutionID: scopeTestExecID, PlanID: scopeTestPlanID, CloudAccountID: &scopeTestProd.ID, + mockPurchase := new(MockPurchaseManager) + + mockAuth.On("ValidateSession", mock.Anything, "operator-token").Return(&Session{ + UserID: "operator-1", Email: "operator@example.com", }, nil) - store.GetPlanAccountsFn = func(context.Context, string) ([]config.CloudAccount, error) { - return []config.CloudAccount{scopeTestProd, scopeTestStage}, nil + mockAuth.On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + // admin:* plus the money verbs it deliberately does not cover + // (adminCarvedOuts), so every RBAC gate passes. + mockAuth.grantPermissionsScoped([]auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases}, + {Action: auth.ActionApproveAny, Resource: auth.ResourcePurchases}, + {Action: auth.ActionRetryAny, Resource: auth.ResourcePurchases}, + }, scope) + + store.On("GetExecutionByID", mock.Anything, scopeTestExecID).Return(exec, nil) + store.GetPlanAccountsFn = func(_ context.Context, planID string) ([]config.CloudAccount, error) { + switch planID { + case "": + return nil, errors.New(`invalid input syntax for type uuid: ""`) + case scopeTestPlanID: + return []config.CloudAccount{scopeTestProd, scopeTestStage}, nil + } + return nil, nil } store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { - require.Equal(t, scopeTestProd.ID, id) - return &scopeTestProd, nil + for _, a := range []config.CloudAccount{scopeTestProd, scopeTestStage} { + if a.ID == id { + return &a, nil + } + } + return nil, nil + } + // Every mutation answers errScopeTestReached, so an allowed call proves + // it got past all gates without wiring each endpoint's success path. + store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(errScopeTestReached).Maybe() + store.On("TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errScopeTestReached).Maybe() + store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errScopeTestReached).Maybe() + store.On("CancelScheduledExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errScopeTestReached).Maybe() + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errScopeTestReached).Maybe() + mockPurchase.On("RunPlannedPurchaseNow", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errScopeTestReached).Maybe() + mockPurchase.On("CancelExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + + return &Handler{auth: mockAuth, config: store, purchase: mockPurchase}, store, mockPurchase +} + +func scopeTestRequest() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer operator-token", "X-CSRF-Token": "csrf"}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "POST"}, + }, + } +} + +// TestExecutionEndpoints_ScopeMatrix requires EVERY account an execution can +// buy in to be in scope: a session holding only some of them (or none) gets +// the enumeration-safe 404 and never reaches the mutation, while a session +// holding all of them passes the gate. +func TestExecutionEndpoints_ScopeMatrix(t *testing.T) { + for _, k := range scopeTestKinds { + for _, sc := range scopeTestScopes { + for _, ep := range scopeTestEndpoints { + t.Run(k.name+"/"+sc.name+"/"+ep.name, func(t *testing.T) { + h, store, mockPurchase := newScopeTestHandler(t, scopeTestExecution(k, ep.status), sc.scope(k)) + + _, err := ep.call(h, scopeTestRequest()) + + if !sc.allow { + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + if ep.mutation != "" { + store.AssertNotCalled(t, ep.mutation) + } + if ep.purchaseMutation != "" { + mockPurchase.AssertNotCalled(t, ep.purchaseMutation) + } + return + } + if ep.mutation == "" && ep.purchaseMutation == "" { + require.NoError(t, err) + return + } + assert.ErrorContains(t, err, errScopeTestReached.Error()) + }) + } + } } +} + +// TestExecutionEndpoints_OutOfScopeNoTokenIs404 guards the token fall-through: +// without a token an out-of-scope session must get the same 404 as a missing +// row, not the token/session branch's status guards (409/410/401), which +// would confirm the execution exists and leak its status. +func TestExecutionEndpoints_OutOfScopeNoTokenIs404(t *testing.T) { + adhoc := scopeTestKinds[2] + for _, tc := range []struct { + name string + status string + call func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) + }{ + {"approve wrong status", "completed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.approvePurchase(context.Background(), req, scopeTestExecID, "") + }}, + {"cancel wrong status", "completed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.cancelPurchase(context.Background(), req, scopeTestExecID, "") + }}, + {"revoke", "completed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }}, + } { + t.Run(tc.name, func(t *testing.T) { + h, _, _ := newScopeTestHandler(t, scopeTestExecution(adhoc, tc.status), []string{"Other"}) + _, err := tc.call(h, scopeTestRequest()) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + }) + } +} + +// TestCancel_OutOfScopeSessionWithTokenUsesContactGate is the positive +// control for the fall-through: a session outside the account scope that +// carries the email token reaches the token branch, and succeeds because the +// session email is the account's contact_email. +func TestCancel_OutOfScopeSessionWithTokenUsesContactGate(t *testing.T) { + exec := scopeTestExecution(scopeTestKind{recs: []config.RecommendationRecord{ + scopeTestRec("rec-stage", &scopeTestStage.ID), + }}, "pending") + h, _, mockPurchase := newScopeTestHandler(t, exec, []string{scopeTestProd.Name}) - h := &Handler{auth: mockAuth, config: store} - require.NoError(t, h.requireExecutionAccess(ctx, &Session{UserID: "operator-1"}, scopeTestExecID)) - store.AssertCalled(t, "GetCloudAccount", ctx, scopeTestProd.ID) + res, err := h.cancelPurchase(context.Background(), scopeTestRequest(), scopeTestExecID, "email-token") + + require.NoError(t, err) + assert.Equal(t, map[string]string{"status": "canceled"}, res) + mockPurchase.AssertCalled(t, "CancelExecution", mock.Anything, scopeTestExecID, "email-token", "operator@example.com") +} + +// TestExecutionAccounts_AdHocUnattributedDenied: an ad-hoc execution with a +// recommendation lacking a cloud account cannot be attributed, so a scoped +// session is denied even when every attributed account is in scope. +func TestExecutionAccounts_AdHocUnattributedDenied(t *testing.T) { + empty := "" + for _, missing := range []*string{nil, &empty} { + exec := scopeTestExecution(scopeTestKind{recs: []config.RecommendationRecord{ + scopeTestRec("rec-prod", &scopeTestProd.ID), + scopeTestRec("rec-none", missing), + }}, "pending") + h, _, _ := newScopeTestHandler(t, exec, []string{scopeTestProd.Name, scopeTestStage.Name}) + err := h.requireExecutionAccess(context.Background(), &Session{UserID: "operator-1"}, scopeTestExecID) + assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + } +} + +// TestPlannedPurchases_HidesPartiallyScopedParent: the list filters with the +// same rule as the per-row gate, so a plan parent that also buys in an +// out-of-scope account is hidden from a partially-scoped session. +func TestPlannedPurchases_HidesPartiallyScopedParent(t *testing.T) { + for _, tc := range []struct { + scope []string + want int + }{ + {[]string{scopeTestProd.Name}, 0}, + {[]string{scopeTestProd.Name, scopeTestStage.Name}, 1}, + } { + exec := scopeTestExecution(scopeTestKinds[0], "pending") + h, store, _ := newScopeTestHandler(t, exec, tc.scope) + store.On("GetPlannedExecutions", mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{*exec}, nil) + store.On("ListPurchasePlans", mock.Anything, mock.Anything).Return([]config.PurchasePlan{{ID: scopeTestPlanID, Name: "p"}}, nil) + + resp, err := h.getPlannedPurchases(context.Background(), scopeTestRequest()) + require.NoError(t, err) + assert.Len(t, resp.Purchases, tc.want, "scope %v", tc.scope) + } } diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 20294e64d..b468029b0 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -429,10 +429,12 @@ func (h *Handler) getUpcomingPurchases(ctx context.Context, req *events.LambdaFu planMap[plans[i].ID] = &plans[i] } - // Per-plan access cache mirrors getPlannedPurchases — all executions - // for the same plan share the same account scope, so we resolve once - // per plan. - allowedPlan := make(map[string]bool) + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + // Memoises GetPlanAccounts, mirroring getPlannedPurchases. + planAccounts := make(map[string][]config.CloudAccount) var upcoming []UpcomingPurchase for i := range executions { @@ -443,7 +445,7 @@ func (h *Handler) getUpcomingPurchases(ctx context.Context, req *events.LambdaFu // Hide rather than crash; cleanup is a separate concern. continue } - ok, err := h.isPlanAllowedCached(ctx, session, exec.PlanID, allowedPlan) + ok, err := h.isExecutionInScope(ctx, scope, &exec, planAccounts) if err != nil { return nil, err } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index ff6d8f846..cc0352fd9 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -128,9 +128,12 @@ func (h *Handler) getPlannedPurchases(ctx context.Context, req *events.LambdaFun planMap[plans[i].ID] = &plans[i] } - // Cache per-plan access decisions — all executions for the same plan share - // the same account scope. Avoids GetPlanAccounts round-trips per execution. - allowedPlan := make(map[string]bool) + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + // Memoises GetPlanAccounts: plan-level parents of the same plan share it. + planAccounts := make(map[string][]config.CloudAccount) var purchases []PlannedPurchase for _rvc := range executions { @@ -139,7 +142,7 @@ func (h *Handler) getPlannedPurchases(ctx context.Context, req *events.LambdaFun if plan == nil { continue } - ok, err := h.isPlanAllowedCached(ctx, session, exec.PlanID, allowedPlan) + ok, err := h.isExecutionInScope(ctx, scope, &exec, planAccounts) if err != nil { return nil, err } @@ -154,20 +157,15 @@ func (h *Handler) getPlannedPurchases(ctx context.Context, req *events.LambdaFun }, nil } -// isPlanAllowedCached resolves and memoises whether the session may see the given plan. -// NotFound errors are treated as "not allowed" (not an error) so missing plans don't -// surface as 500s, mirroring the previous inline behavior. -func (h *Handler) isPlanAllowedCached(ctx context.Context, session *Session, planID string, cache map[string]bool) (bool, error) { - if ok, cached := cache[planID]; cached { - return ok, nil - } - planErr := h.requirePlanAccess(ctx, session, planID) - if planErr != nil && !IsNotFoundError(planErr) { - return false, planErr +// isExecutionInScope is requireExecutionInScope for list filtering: an +// out-of-scope execution is hidden (false) rather than an error, while store +// failures still propagate. +func (h *Handler) isExecutionInScope(ctx context.Context, scope auth.AccountScope, exec *config.PurchaseExecution, planAccounts map[string][]config.CloudAccount) (bool, error) { + err := h.requireExecutionInScope(ctx, scope, exec, planAccounts) + if IsNotFoundError(err) { + return false, nil } - ok := planErr == nil - cache[planID] = ok - return ok, nil + return err == nil, err } // buildPlannedPurchase converts a (plan, execution) pair into the API-facing PlannedPurchase. @@ -597,12 +595,11 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio switch err := h.authorizeScopedSessionApprove(ctx, session, execution); { case err == nil: return h.approvePurchaseViaSession(ctx, req, execution) - case fallsThroughToToken(err): - // Explicit 403 or out-of-scope 404 → fall through to the token - // branch so the contact_email gate gets a chance (a logged-in - // user without approve-* or outside the account scope may still - // be the per-account contact recipient). Without a token the - // session branch below re-runs the gate and returns the error. + case fallsThroughToToken(err, token): + // Explicit 403, or an out-of-scope 404 with a token → fall + // through to the token branch so the contact_email gate gets a + // chance (a logged-in user without approve-* or outside the + // account scope may still be the per-account contact recipient). default: // Transient failure — propagate instead of silently widening. return nil, err @@ -1201,12 +1198,12 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction case err == nil: // Session is RBAC-authorized → run the session-authed cancel. return h.cancelPurchaseViaSession(ctx, req, execution) - case fallsThroughToToken(err): - // Explicit "permission denied" (403) or out-of-scope (404) → - // fall through to the token branch so the contact_email gate - // still gets a chance (a logged-in user without admin / - // cancel-* may still be the per-account contact email - // recipient). + case fallsThroughToToken(err, token): + // Explicit "permission denied" (403), or an out-of-scope 404 + // with a token → fall through to the token branch so the + // contact_email gate still gets a chance (a logged-in user + // without admin / cancel-* may still be the per-account contact + // email recipient). default: // Transient failure (auth-service down, HasPermissionAPI // returning a wrapped error, h.auth==nil 500). Propagate @@ -1397,7 +1394,7 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun } // Three-mode dispatch — same shape as cancelPurchase. - result, handled, revokeErr := h.tryRevokeViaSession(ctx, req, execution) + result, handled, revokeErr := h.tryRevokeViaSession(ctx, req, execution, token) if handled { return result, revokeErr } @@ -1458,7 +1455,7 @@ func renderRevokeConfirmPage(execID, token string) *rawResponse { // (nil, false, nil) when the session was absent or returned a permission-denied // error so the caller can fall through to the token branch. Extracted from // revokeViaEmailToken to keep that function under the cyclomatic limit. -func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution) (result any, handled bool, err error) { +func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, token string) (result any, handled bool, err error) { session := h.tryGetSession(ctx, req) if session == nil { return nil, false, nil @@ -1474,7 +1471,7 @@ func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFun } res, revokeErr := h.revokeViaSession(ctx, execution, session.Email) return res, true, revokeErr - case fallsThroughToToken(sessErr): + case fallsThroughToToken(sessErr, token): // Fall through to the token branch. return nil, false, nil default: @@ -2232,6 +2229,18 @@ func wrapConstraintDenied(err error) error { return &constraintDeniedError{clientError: ce} } +// fallsThroughToToken reports whether a session-authorization failure should +// hand the request to the email-token branch: an RBAC denial (403), or an +// account-scope miss (404) when a token is present. The token branch's +// contact_email gate is deliberately not account-scoped (issue #92). Without +// a token the 404 is returned as-is, so the token branch's status guards +// cannot reveal an out-of-scope execution's existence or status. Both +// matches are strict, like isPermissionDenied, so a wrapped error from +// deeper in the chain propagates. +func fallsThroughToToken(err error, token string) bool { + return isPermissionDenied(err) || (token != "" && err == errNotFound) //nolint:errorlint // strict sentinel identity is deliberate +} + // isPermissionDenied reports whether err is *directly* a 403 ClientError // (not merely something that wraps one). Used by the cancel-from-email // session pre-check to distinguish a legitimate "your session lacks @@ -2247,15 +2256,6 @@ func wrapConstraintDenied(err error) error { // failures into the contact_email gate — exactly the misclassification // the propagate-vs-fall-through split is meant to prevent. constraintDeniedError // above deliberately exploits the same strictness from the other direction. -// fallsThroughToToken reports whether a session-authorization failure should -// hand the request to the email-token branch: an RBAC denial (403) or an -// account-scope miss (404). The token branch's contact_email gate is -// deliberately not account-scoped (issue #92). Both matches are strict, like -// isPermissionDenied, so a wrapped error from deeper in the chain propagates. -func fallsThroughToToken(err error) bool { - return isPermissionDenied(err) || err == errNotFound //nolint:errorlint // strict sentinel identity is deliberate -} - func isPermissionDenied(err error) bool { ce, ok := err.(*clientError) //nolint:errorlint // strict (unwrapped) assertion is deliberate; see comment above return ok && ce.code == 403 @@ -2329,8 +2329,11 @@ func (h *Handler) getPurchaseDetails(ctx context.Context, req *events.LambdaFunc return nil, fmt.Errorf("failed to get execution: %w", err) } - // Scope: reject if the execution's plan isn't accessible to the session. - if err := h.requirePlanAccess(ctx, session, execution.PlanID); err != nil { + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + if err := h.requireExecutionInScope(ctx, scope, execution, nil); err != nil { return nil, err } diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 2566459a9..8d16cf25a 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) @@ -143,15 +144,11 @@ func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, sessi return nil } -// requireExecutionAccess rejects with errNotFound when the execution's plan's -// associated accounts don't intersect with the session's allowed_accounts, -// or when the execution targets a CloudAccountID outside them. The plan -// check alone is not enough: a plan spanning accounts {A, B} intersects a -// session scoped to A, yet its execution for B still spends B's money -// (issue #92). -// Convenience wrapper around requirePlanAccess for the pause/resume/run/ -// delete/details handlers that key on executionID. Returns errNotFound when -// the execution itself doesn't exist, so unauthenticated probing can't +// requireExecutionAccess rejects with errNotFound unless every cloud account +// the execution can buy in (executionAccounts) is inside the session's +// allowed_accounts (issue #92). Used by the approve/cancel/retry/revoke and +// pause/resume/run/delete handlers that key on executionID. Returns +// errNotFound when the execution itself doesn't exist, so probing can't // distinguish "no such execution" from "you can't see it". // // Short-circuits for admin / unrestricted sessions BEFORE the @@ -159,31 +156,118 @@ func (h *Handler) validatePurchaseRecommendationScope(ctx context.Context, sessi // round-trip (and to keep existing unit-test fixtures for admin operations // working without adding execution mocks). func (h *Handler) requireExecutionAccess(ctx context.Context, session *Session, executionID string) error { - allowed, err := h.getAccountScope(ctx, session) + scope, err := h.getAccountScope(ctx, session) if err != nil { return fmt.Errorf("failed to get allowed accounts: %w", err) } - if allowed.AllowsAll() { + if scope.AllowsAll() { return nil } execution, err := h.config.GetExecutionByID(ctx, executionID) - if errors.Is(err, config.ErrNotFound) { + if errors.Is(err, config.ErrNotFound) || (err == nil && execution == nil) { return errNotFound } if err != nil { return fmt.Errorf("failed to get execution: %w", err) } - if err := h.requirePlanAccess(ctx, session, execution.PlanID); err != nil { + return h.requireExecutionInScope(ctx, scope, execution, nil) +} + +// requireExecutionInScope returns errNotFound unless the scope allows every +// account in executionAccounts(execution). An empty set is a deny: an +// execution that cannot be attributed must not be actionable by a scoped +// user. planAccounts memoises GetPlanAccounts for list callers; nil disables +// it. +func (h *Handler) requireExecutionInScope(ctx context.Context, scope auth.AccountScope, execution *config.PurchaseExecution, planAccounts map[string][]config.CloudAccount) error { + if scope.AllowsAll() { + return nil + } + accounts, err := h.executionAccounts(ctx, execution, planAccounts) + if err != nil { return err } - if execution.CloudAccountID != nil { - if _, err := h.requireAccountAccess(ctx, session, *execution.CloudAccountID); err != nil { - return err + if len(accounts) == 0 { + return errNotFound + } + for i := range accounts { + if !scope.Allows(accounts[i].ID, accounts[i].Name) { + return errNotFound } } return nil } +// executionAccounts returns every cloud account the execution can buy in: +// - CloudAccountID set: that account only (a fan-out child or a +// single-account execution). +// - otherwise PlanID set: all of the plan's accounts, since a plan-level +// parent fans out to every one of them (purchase.Manager.executeScopeAware). +// - otherwise (an ad-hoc web purchase, plan_id NULL): the account of every +// recommendation. One without an account is unattributable, so the whole +// execution is refused with errNotFound. +// +// An account ID with no cloud_accounts row comes back without a name, so it +// can only match an ID entry in the scope. +func (h *Handler) executionAccounts(ctx context.Context, execution *config.PurchaseExecution, planAccounts map[string][]config.CloudAccount) ([]config.CloudAccount, error) { + if execution.CloudAccountID != nil { + a, err := h.lookupCloudAccount(ctx, *execution.CloudAccountID) + if err != nil { + return nil, err + } + return []config.CloudAccount{a}, nil + } + if execution.PlanID != "" { + if cached, ok := planAccounts[execution.PlanID]; ok { + return cached, nil + } + accounts, err := h.config.GetPlanAccounts(ctx, execution.PlanID) + if err != nil { + return nil, fmt.Errorf("failed to get plan accounts: %w", err) + } + if planAccounts != nil { + planAccounts[execution.PlanID] = accounts + } + return accounts, nil + } + return h.recommendationAccounts(ctx, execution.Recommendations) +} + +// recommendationAccounts returns the distinct accounts of recs, or +// errNotFound when any recommendation has no account. +func (h *Handler) recommendationAccounts(ctx context.Context, recs []config.RecommendationRecord) ([]config.CloudAccount, error) { + seen := make(map[string]bool, len(recs)) + accounts := make([]config.CloudAccount, 0, len(recs)) + for i := range recs { + id := recs[i].CloudAccountID + if id == nil || *id == "" { + return nil, errNotFound + } + if seen[*id] { + continue + } + seen[*id] = true + a, err := h.lookupCloudAccount(ctx, *id) + if err != nil { + return nil, err + } + accounts = append(accounts, a) + } + return accounts, nil +} + +// lookupCloudAccount resolves an account ID for a scope check; a missing row +// yields an ID-only account rather than an error (see executionAccounts). +func (h *Handler) lookupCloudAccount(ctx context.Context, id string) (config.CloudAccount, error) { + a, err := h.config.GetCloudAccount(ctx, id) + if err != nil { + return config.CloudAccount{}, fmt.Errorf("accounts: %w", err) + } + if a == nil { + return config.CloudAccount{ID: id}, nil + } + return *a, nil +} + // resolveAccountFilterIDs maps requested cloud_accounts UUIDs to the // (uuid, provider, external_id) data needed for the dual-column purchase-history // filter. purchase_history rows carry either the UUID FK (cloud_account_id) or From 226ae9761ce42df1467a212938b1221a9061d39c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 20:39:12 +0200 Subject: [PATCH 4/4] sec(api): check revoke status only after authorization revokeViaEmailToken returned the 409 status guard before any session or token check, so an out-of-scope session or an unauthenticated caller learned that a pending/notified/failed execution exists and its status. The status check now runs after the session scope/RBAC and CSRF checks, and after the token is validated. An authorized caller still gets 409. The scope matrix now asserts every mutation is absent on deny with literal method names and resolvable mock types, so TestNoUnfailableMockAssertions can check each site. --- internal/api/execution_scope_test.go | 113 ++++++++++++++++++------- internal/api/handler_purchases.go | 14 +-- internal/api/handler_purchases_test.go | 9 +- 3 files changed, 94 insertions(+), 42 deletions(-) diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index a4b747496..4e32047e2 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -82,70 +82,68 @@ type scopeTestEndpoint struct { name string status string call func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) - // mutation is the store method the endpoint reaches only once every - // gate has passed; purchaseMutation is the purchase-manager equivalent. - // Both empty for read-only endpoints. - mutation string - purchaseMutation string + // readOnly endpoints reach no mutation, so an allowed call succeeds + // instead of answering errScopeTestReached. + readOnly bool } var scopeTestEndpoints = []scopeTestEndpoint{ { - name: "approve", status: "pending", purchaseMutation: "ApproveAndExecute", + name: "approve", status: "pending", call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.approvePurchase(context.Background(), req, scopeTestExecID, "") }, }, { - name: "cancel", status: "pending", mutation: "CancelExecutionAtomic", + name: "cancel", status: "pending", call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.cancelPurchase(context.Background(), req, scopeTestExecID, "") }, }, { - name: "retry", status: "failed", mutation: "SavePurchaseExecution", + name: "retry", status: "failed", call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.retryPurchase(context.Background(), req, scopeTestExecID) }, }, { - name: "revoke completed via session", status: "completed", mutation: "TransitionExecutionStatus", + name: "revoke completed via session", status: "completed", call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") }, }, { - name: "revoke scheduled", status: "scheduled", mutation: "CancelScheduledExecutionAtomic", + name: "revoke scheduled", status: "scheduled", call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.revokePurchase(context.Background(), req, scopeTestExecID) }, }, { - name: "run now", status: "pending", purchaseMutation: "RunPlannedPurchaseNow", + name: "run now", status: "pending", call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.runPlannedPurchase(context.Background(), req, scopeTestExecID) }, }, { - name: "pause", status: "pending", mutation: "TransitionExecutionStatus", + name: "pause", status: "pending", call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.pausePlannedPurchase(context.Background(), req, scopeTestExecID) }, }, { - name: "resume", status: "paused", mutation: "TransitionExecutionStatus", + name: "resume", status: "paused", call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.resumePlannedPurchase(context.Background(), req, scopeTestExecID) }, }, { - name: "delete", status: "pending", mutation: "TransitionExecutionStatus", + name: "delete", status: "pending", call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.deletePlannedPurchase(context.Background(), req, scopeTestExecID) }, }, { - name: "details", status: "completed", + name: "details", status: "completed", readOnly: true, call: func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.getPurchaseDetails(context.Background(), req, scopeTestExecID) }, @@ -169,11 +167,12 @@ func scopeTestExecution(k scopeTestKind, status string) *config.PurchaseExecutio // whose store serves exec. GetPlanAccounts("") errors the way the UUID // plan_id column does in Postgres, so a gate that queries it for an ad-hoc // execution surfaces as a 500 rather than a quiet deny. -func newScopeTestHandler(t *testing.T, exec *config.PurchaseExecution, scope []string) (*Handler, *MockConfigStore, *MockPurchaseManager) { +// +// The caller constructs the mocks so their types resolve at its assertion +// sites for TestNoUnfailableMockAssertions. +func newScopeTestHandler(t *testing.T, exec *config.PurchaseExecution, scope []string, store *MockConfigStore, mockPurchase *MockPurchaseManager) *Handler { t.Helper() - store := new(MockConfigStore) mockAuth := new(MockAuthService) - mockPurchase := new(MockPurchaseManager) mockAuth.On("ValidateSession", mock.Anything, "operator-token").Return(&Session{ UserID: "operator-1", Email: "operator@example.com", @@ -216,7 +215,7 @@ func newScopeTestHandler(t *testing.T, exec *config.PurchaseExecution, scope []s mockPurchase.On("RunPlannedPurchaseNow", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errScopeTestReached).Maybe() mockPurchase.On("CancelExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() - return &Handler{auth: mockAuth, config: store, purchase: mockPurchase}, store, mockPurchase + return &Handler{auth: mockAuth, config: store, purchase: mockPurchase} } func scopeTestRequest() *events.LambdaFunctionURLRequest { @@ -237,22 +236,26 @@ func TestExecutionEndpoints_ScopeMatrix(t *testing.T) { for _, sc := range scopeTestScopes { for _, ep := range scopeTestEndpoints { t.Run(k.name+"/"+sc.name+"/"+ep.name, func(t *testing.T) { - h, store, mockPurchase := newScopeTestHandler(t, scopeTestExecution(k, ep.status), sc.scope(k)) + store := new(MockConfigStore) + mockPurchase := new(MockPurchaseManager) + h := newScopeTestHandler(t, scopeTestExecution(k, ep.status), sc.scope(k), store, mockPurchase) _, err := ep.call(h, scopeTestRequest()) if !sc.allow { require.Error(t, err) assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) - if ep.mutation != "" { - store.AssertNotCalled(t, ep.mutation) - } - if ep.purchaseMutation != "" { - mockPurchase.AssertNotCalled(t, ep.purchaseMutation) - } + // A denied call reaches no mutation of any endpoint. + store.AssertNotCalled(t, "SavePurchaseExecution") + store.AssertNotCalled(t, "TransitionExecutionStatus") + store.AssertNotCalled(t, "CancelExecutionAtomic") + store.AssertNotCalled(t, "CancelScheduledExecutionAtomic") + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "RunPlannedPurchaseNow", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "CancelExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) return } - if ep.mutation == "" && ep.purchaseMutation == "" { + if ep.readOnly { require.NoError(t, err) return } @@ -280,18 +283,62 @@ func TestExecutionEndpoints_OutOfScopeNoTokenIs404(t *testing.T) { {"cancel wrong status", "completed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.cancelPurchase(context.Background(), req, scopeTestExecID, "") }}, - {"revoke", "completed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + {"revoke completed", "completed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }}, + {"revoke pending", "pending", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }}, + {"revoke notified", "notified", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { + return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") + }}, + {"revoke failed", "failed", func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.revokeViaEmailToken(context.Background(), req, scopeTestExecID, "") }}, } { t.Run(tc.name, func(t *testing.T) { - h, _, _ := newScopeTestHandler(t, scopeTestExecution(adhoc, tc.status), []string{"Other"}) + h := newScopeTestHandler(t, scopeTestExecution(adhoc, tc.status), []string{"Other"}, new(MockConfigStore), new(MockPurchaseManager)) _, err := tc.call(h, scopeTestRequest()) assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) }) } } +// TestRevoke_AuthorizedNonRevocableStatusIs409: moving the status check +// behind authorization must not drop it. An authorized caller, via the +// session or the email token, still gets the 409 for a status with nothing +// to revoke, and no mutation runs. +func TestRevoke_AuthorizedNonRevocableStatusIs409(t *testing.T) { + stageOnly := scopeTestKind{recs: []config.RecommendationRecord{scopeTestRec("rec-stage", &scopeTestStage.ID)}} + for _, tc := range []struct { + name string + scope []string + token string + }{ + {"session", []string{scopeTestStage.Name}, ""}, + {"token", []string{"Other"}, "email-token"}, + } { + for _, status := range []string{"pending", "notified", "failed"} { + t.Run(tc.name+"/"+status, func(t *testing.T) { + exec := scopeTestExecution(stageOnly, status) + exec.ApprovalToken = "email-token" + store := new(MockConfigStore) + h := newScopeTestHandler(t, exec, tc.scope, store, new(MockPurchaseManager)) + + _, err := h.revokeViaEmailToken(context.Background(), scopeTestRequest(), scopeTestExecID, tc.token) + + var ce *clientError + require.ErrorAs(t, err, &ce) + assert.Equal(t, 409, ce.code, "got %v", err) + if status == "pending" { + assert.Contains(t, ce.message, "Cancel") + } + store.AssertNotCalled(t, "TransitionExecutionStatus") + }) + } + } +} + // TestCancel_OutOfScopeSessionWithTokenUsesContactGate is the positive // control for the fall-through: a session outside the account scope that // carries the email token reaches the token branch, and succeeds because the @@ -300,7 +347,8 @@ func TestCancel_OutOfScopeSessionWithTokenUsesContactGate(t *testing.T) { exec := scopeTestExecution(scopeTestKind{recs: []config.RecommendationRecord{ scopeTestRec("rec-stage", &scopeTestStage.ID), }}, "pending") - h, _, mockPurchase := newScopeTestHandler(t, exec, []string{scopeTestProd.Name}) + mockPurchase := new(MockPurchaseManager) + h := newScopeTestHandler(t, exec, []string{scopeTestProd.Name}, new(MockConfigStore), mockPurchase) res, err := h.cancelPurchase(context.Background(), scopeTestRequest(), scopeTestExecID, "email-token") @@ -319,7 +367,7 @@ func TestExecutionAccounts_AdHocUnattributedDenied(t *testing.T) { scopeTestRec("rec-prod", &scopeTestProd.ID), scopeTestRec("rec-none", missing), }}, "pending") - h, _, _ := newScopeTestHandler(t, exec, []string{scopeTestProd.Name, scopeTestStage.Name}) + h := newScopeTestHandler(t, exec, []string{scopeTestProd.Name, scopeTestStage.Name}, new(MockConfigStore), new(MockPurchaseManager)) err := h.requireExecutionAccess(context.Background(), &Session{UserID: "operator-1"}, scopeTestExecID) assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) } @@ -337,7 +385,8 @@ func TestPlannedPurchases_HidesPartiallyScopedParent(t *testing.T) { {[]string{scopeTestProd.Name, scopeTestStage.Name}, 1}, } { exec := scopeTestExecution(scopeTestKinds[0], "pending") - h, store, _ := newScopeTestHandler(t, exec, tc.scope) + store := new(MockConfigStore) + h := newScopeTestHandler(t, exec, tc.scope, store, new(MockPurchaseManager)) store.On("GetPlannedExecutions", mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{*exec}, nil) store.On("ListPurchasePlans", mock.Anything, mock.Anything).Return([]config.PurchasePlan{{ID: scopeTestPlanID, Name: "p"}}, nil) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index cc0352fd9..d9e386c6a 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1387,12 +1387,6 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun return nil, NewClientError(404, "execution not found") } - // Only completed/partially_completed purchases have anything to revoke. - // A pending/notified purchase should be canceled instead. - if statusErr := checkRevokableStatus(execution); statusErr != nil { - return nil, statusErr - } - // Three-mode dispatch — same shape as cancelPurchase. result, handled, revokeErr := h.tryRevokeViaSession(ctx, req, execution, token) if handled { @@ -1422,6 +1416,9 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun if err := validateRevokeToken(execution, token); err != nil { return nil, err } + if err := checkRevokableStatus(execution); err != nil { + return nil, err + } return h.revokeViaSession(ctx, execution, actor) } @@ -1469,6 +1466,11 @@ func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFun if csrfErr := h.validateCSRF(ctx, req); csrfErr != nil { return nil, true, NewClientError(403, "CSRF validation failed") } + // Status is checked only once the caller is authorized, so an + // out-of-scope caller cannot learn it from a 409. + if statusErr := checkRevokableStatus(execution); statusErr != nil { + return nil, true, statusErr + } res, revokeErr := h.revokeViaSession(ctx, execution, session.Email) return res, true, revokeErr case fallsThroughToToken(sessErr, token): diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 382e06c97..b75ac8d5d 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -5602,8 +5602,10 @@ func TestHandler_revokePurchase_InvalidToken(t *testing.T) { assert.Equal(t, 403, ce.code) } -// TestHandler_revokePurchase_PendingExecution verifies that a pending execution -// returns 409 with a friendly message directing the user to Cancel instead. +// TestHandler_revokePurchase_PendingExecution verifies that a caller with no +// session gets the 401 for a pending execution, not the status 409: status is +// only disclosed once the caller is authorized. The authorized 409 is covered by +// TestRevoke_AuthorizedNonRevocableStatusIs409. func TestHandler_revokePurchase_PendingExecution(t *testing.T) { ctx := context.Background() execID := "33333333-3333-3333-3333-333333333333" @@ -5625,8 +5627,7 @@ func TestHandler_revokePurchase_PendingExecution(t *testing.T) { require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok, "expected a client error") - assert.Equal(t, 409, ce.code) - assert.Contains(t, ce.message, "Cancel") + assert.Equal(t, 401, ce.code) } // TestHandler_revokePurchase_NotFound verifies 404 when the execution does not exist.