From 0421afed902ed93f0ca5e3fb8b68b304f8e4247a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 05:49:36 +0200 Subject: [PATCH] fix(api): scope executeExchange to session's allowed_accounts executeExchange (POST /api/ri-exchange/execute) was the only one of the four RI-exchange endpoints that did not call reshapeCloudAccountInScope. The three sibling reads (listConvertibleRIs, getRIUtilization, getReshapeRecommendations) already refuse a deployment cloud account outside the session's allowed_accounts by returning an empty list; a session scoped to one account with an execute:ri-exchange permission carrying no AccountIDs constraint could still execute an exchange against a different, unscoped deployment account, since the separate requirePermissionConstraints check (SEC-01, issue #1141) is satisfied unconditionally by an unconstrained permission. Call the existing reshapeCloudAccountInScope helper at the top of executeExchange and return 403 (not an empty list, since this endpoint is a mutating, financially irreversible action) when out of scope. Extracted two small helpers (requireReshapeAccountScope and resolveReshapeCloudAccountIDOrUnattributed) so the added branch keeps executeExchange's cyclomatic complexity at the project's CI-enforced gate of 10. Added TestExecuteExchange_SessionAccountScopeMismatchReturns403, confirmed to fail against the pre-fix code (stashed diff) and pass after. Updated five existing tests to stub the session's account scope via the existing allowAnyAccountScope helper, since they exercise unrelated paths that now also reach the new check. --- internal/api/handler_ri_exchange.go | 56 +++++++++++++- internal/api/handler_ri_exchange_test.go | 74 +++++++++++++++++++ .../ri_exchange_idempotency_guards_test.go | 4 + internal/api/ri_exchange_idempotency_test.go | 3 + .../api/router_660_permission_flips_test.go | 3 + 5 files changed, 136 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 6c46b260d..7ee582f30 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1298,6 +1298,50 @@ func (h *Handler) reshapeCloudAccountInScope(ctx context.Context, session *Sessi return allowed.Allows(cloudAccountID, nameByID[cloudAccountID]), nil } +// requireReshapeAccountScope enforces the session's allowed_accounts scope +// for executeExchange (issue #93). The three sibling read endpoints +// (listConvertibleRIs, getRIUtilization, getReshapeRecommendations) all call +// reshapeCloudAccountInScope and return an empty list when the deployment's +// cloud account falls outside the session's scope; executeExchange is the +// one mutating, financially irreversible endpoint among the four, so it must +// fail closed with a 403 ClientError instead of an empty response. This is +// independent of the requirePermissionConstraints check executeExchange also +// runs, which evaluates the granting permission's own Constraints (SEC-01, +// issue #1141): a permission with no AccountIDs constraint satisfies that +// check unconditionally, so it does not substitute for the session-level +// scope enforced here. Split out as its own function (rather than inlined in +// executeExchange) to keep that handler's cyclomatic complexity under the +// project's gate. +func (h *Handler) requireReshapeAccountScope(ctx context.Context, session *Session) error { + inScope, err := h.reshapeCloudAccountInScope(ctx, session) + if err != nil { + return err + } + if !inScope { + return NewClientError(403, "permission denied: this deployment's cloud account is not covered by your session's allowed accounts") + } + return nil +} + +// resolveReshapeCloudAccountIDOrUnattributed resolves the deployment's cloud +// account ID, substituting the unattributedAccountConstraint sentinel when +// the deployment maps to no registered account. Used by executeExchange to +// build the AccountIDs dimension of its permission-constraints check +// (SEC-01, issue #1141): an AccountIDs-constrained permission must deny +// rather than silently matching an empty/absent constraint. Split out from +// executeExchange to keep that handler's cyclomatic complexity under the +// project's gate. +func (h *Handler) resolveReshapeCloudAccountIDOrUnattributed(ctx context.Context) (string, error) { + cloudAccountID, err := h.resolveReshapeCloudAccountID(ctx) + if err != nil { + return "", err + } + if cloudAccountID == "" { + return unattributedAccountConstraint, nil + } + return cloudAccountID, nil +} + // resolveReshapeCloudAccountID returns the cloud account ID for the running // deployment, using the test-injected reshapeAccountResolver when set and // falling back to the production resolveAWSCloudAccountID (STS-backed). @@ -1774,6 +1818,13 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio return nil, err } + // Apply the session's allowed_accounts scope (issue #93). See + // requireReshapeAccountScope for what this guards against. + err = h.requireReshapeAccountScope(ctx, session) + if err != nil { + return nil, err + } + var body ExchangeExecuteRequestBody err = json.Unmarshal([]byte(req.Body), &body) if err != nil { @@ -1802,13 +1853,10 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio // max_payment_due_usd guardrail, which ExecuteExchange independently // enforces against the actual quoted payment due. maxPayment, _ := maxRat.Float64() - cloudAccountID, err := h.resolveReshapeCloudAccountID(ctx) + cloudAccountID, err := h.resolveReshapeCloudAccountIDOrUnattributed(ctx) if err != nil { return nil, fmt.Errorf("failed to resolve cloud account scope: %w", err) } - if cloudAccountID == "" { - cloudAccountID = unattributedAccountConstraint - } err = h.requirePermissionConstraints(ctx, session, "ri-exchange", []auth.PermissionConstraints{{ AccountIDs: []string{cloudAccountID}, Providers: []string{string(common.ProviderAWS)}, diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index f281ec2a5..85734f4ae 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1551,6 +1551,10 @@ func TestExecuteExchange_PermissionConstraintsDenied(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + // The session-scope gate (reshapeCloudAccountInScope, issue #93) runs + // before the permission-constraints check this test targets; unrestrict + // it so the request reaches the constraints check under test. + allowAnyAccountScope(mockAuth) mockAuth.On("HasPermissionForConstraintsAPI", ctx, userSession.UserID, "execute", "ri-exchange", mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { if len(sets) != 1 { @@ -1598,6 +1602,10 @@ func TestExecuteExchange_AccountResolutionErrorFailsClosed(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93) short-circuits without needing the resolver, and this + // test's own resolver failure is the one that fails closed below. + allowAnyAccountScope(mockAuth) h := &Handler{ auth: mockAuth, @@ -1630,6 +1638,10 @@ func TestExecuteExchange_UnattributedAccountStillConstrained(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93) is satisfied without consulting the resolver, leaving this + // test's own single resolver call (for the constraint set below) intact. + allowAnyAccountScope(mockAuth) mockAuth.On("HasPermissionForConstraintsAPI", ctx, userSession.UserID, "execute", "ri-exchange", mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { return len(sets) == 1 && @@ -1652,6 +1664,68 @@ func TestExecuteExchange_UnattributedAccountStillConstrained(t *testing.T) { assert.Equal(t, 403, ce.code) } +// TestExecuteExchange_SessionAccountScopeMismatchReturns403 is the regression +// test for issue #93: listConvertibleRIs, getRIUtilization and +// getReshapeRecommendations each call reshapeCloudAccountInScope and return an +// empty list when the deployment's registered AWS cloud account falls outside +// the session's allowed_accounts, but executeExchange did not, even though it +// is the only one of the four RI-exchange endpoints that is financially +// irreversible. +// +// This replays the issue's exact scenario: a session scoped to +// allowed_accounts=[acct-A] holding an execute:ri-exchange grant with NO +// AccountIDs constraint (so the SEC-01/#1141 requirePermissionConstraints +// check below would allow it unconditionally) attempts to execute against a +// deployment whose ambient AWS account resolves to acct-B. Before the fix +// this reached requirePermissionConstraints and beyond; it must instead be +// refused with 403 by the session-scope gate before either the constraints +// check or the AWS call. +func TestExecuteExchange_SessionAccountScopeMismatchReturns403(t *testing.T) { + ctx := context.Background() + const allowedAccountID = "11111111-2222-3333-4444-555555555555" // acct-A: in the session's scope + const deploymentAccountID = "99999999-8888-7777-6666-555555555555" // acct-B: the deployment's ambient account + + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + userSession := &Session{ + UserID: "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee", + Email: "exchanger@example.com", + } + mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + // The session is restricted to acct-A only. + mockAuth.On("GetAllowedAccountsAPI", ctx, userSession.UserID).Return([]string{allowedAccountID}, nil) + // No HasPermissionForConstraintsAPI expectation: the permission carries no + // AccountIDs constraint (matching the issue's failure scenario) and would + // satisfy that check unconditionally, so it must never be reached -- the + // session-scope gate above must refuse the request first. + + mockStore := new(MockConfigStore) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return nil, nil + } + + h := &Handler{ + auth: mockAuth, + config: mockStore, + reshapeAccountResolver: func(_ context.Context) (string, error) { + return deploymentAccountID, nil + }, + } + _, err := h.executeExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer exchange-token"}, + Body: `{"ri_ids":["ri-123"],"target_offering_id":"off-1","target_count":1,"max_payment_due_usd":"250.50","region":"eu-central-1"}`, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "allowed accounts") + mockAuth.AssertNotCalled(t, "HasPermissionForConstraintsAPI", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + // TestGetExchangeQuote_EmptyRegionResolvesFromSDK pins finding 01-L4: // getExchangeQuote must resolve the region from the AWS SDK chain when // the caller omits it, matching getReshapeRecommendations, instead of diff --git a/internal/api/ri_exchange_idempotency_guards_test.go b/internal/api/ri_exchange_idempotency_guards_test.go index 8918a5dac..5a57829d2 100644 --- a/internal/api/ri_exchange_idempotency_guards_test.go +++ b/internal/api/ri_exchange_idempotency_guards_test.go @@ -87,6 +87,10 @@ func TestExecuteExchange_InvalidBodyNeverClaims(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93), which now runs before body validation, does not block the + // request before reaching the validation failure this test targets. + allowAnyAccountScope(mockAuth) // Maybe(): the validation below must return before this is reached. It is // registered anyway so that a regression letting the request through fails // on the assertions rather than panicking on an unexpected mock call. diff --git a/internal/api/ri_exchange_idempotency_test.go b/internal/api/ri_exchange_idempotency_test.go index b50152a6d..ad0efd192 100644 --- a/internal/api/ri_exchange_idempotency_test.go +++ b/internal/api/ri_exchange_idempotency_test.go @@ -226,6 +226,9 @@ func TestExecuteExchange_DuplicateSubmitRefusedBeforeAWS(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93) does not interfere with the claim-ledger behavior under test. + allowAnyAccountScope(mockAuth) mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", mock.Anything).Return(true, nil) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go index 3c075921f..8cc4f32e6 100644 --- a/internal/api/router_660_permission_flips_test.go +++ b/internal/api/router_660_permission_flips_test.go @@ -457,6 +457,9 @@ func TestExecuteExchange_PermissionGate(t *testing.T) { // AWS SDK call will fail with a non-403 (connection refused / 500). t.Run("user with execute:ri-exchange clears 403 gate", func(t *testing.T) { mockAuth := authForUserWith(ctx, t, userID, "execute", "ri-exchange", true) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93) does not itself produce the 403 this sub-test must not see. + allowAnyAccountScope(mockAuth) h := &Handler{auth: mockAuth} body := `{"ri_ids":["ri-abc"],"targets":[{"offering_id":"of-1"}],"max_payment_due_usd":"1000"}` _, err := h.executeExchange(ctx, reqWithBearerAndBody("user-token", body))