diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index a30a9b87..f80f3554 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2045,8 +2045,9 @@ func revocationWindowClosesAt(execution *config.PurchaseExecution) string { // UPDATE WHERE status IN ('completed','partially_completed'). This guards // against lost-update races: if the row changed between the GetExecutionByID // read and this write (e.g. a concurrent transition), zero rows are affected -// and we return a 409 rather than blindly overwriting. CancelledBy is stamped -// in a follow-up SavePurchaseExecution on the freshly-returned row. +// and we return a 409 rather than blindly overwriting. CancelledBy is then +// stamped by a narrow SetCancelledBy UPDATE keyed on the execution ID, so the +// row TransitionExecutionStatus returns is not used. // // transitionedBy is the acting user's UUID (resolveCreatorUserID), or nil when // the caller has no user row. It must never be an email: transitioned_by is a @@ -2055,11 +2056,17 @@ func revocationWindowClosesAt(execution *config.PurchaseExecution) string { // The revocation window is enforced here, not only in validateRevokeToken, so // the session-authed branch (tryRevokeViaSession) cannot flip an execution // whose window closed long ago to revocation_requested (issue #260). +// +// Neither revoke path (this one or the token path in revokePurchase) applies +// the 4-eyes RequireDifferentApprover policy: that policy gates approvals +// only, and a revocation records a request that an operator completes with +// AWS Support. Whether revocation should also need a second person is an +// open owner decision (issue #311). func (h *Handler) revokeViaSession(ctx context.Context, execution *config.PurchaseExecution, revokedBy string, transitionedBy *string) (any, error) { if err := checkRevocationWindow(execution); err != nil { return nil, err } - updated, err := h.config.TransitionExecutionStatus( + _, err := h.config.TransitionExecutionStatus( ctx, execution.ExecutionID, []string{"completed", "partially_completed"}, "revocation_requested", transitionedBy) if err != nil { @@ -2082,7 +2089,6 @@ func (h *Handler) revokeViaSession(ctx context.Context, execution *config.Purcha if err := h.config.SetCancelledBy(ctx, execution.ExecutionID, revokedBy); err != nil { return nil, fmt.Errorf("failed to record revocation requester for execution %s: %w", execution.ExecutionID, err) } - _ = updated // kept for future use; CancelledBy is now persisted atomically } logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, redactEmail(revokedBy)) return map[string]string{ diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index a88254cf..7b77b372 100644 --- a/internal/api/handler_purchases_revoke_test.go +++ b/internal/api/handler_purchases_revoke_test.go @@ -1603,16 +1603,6 @@ func TestRevocationWindowClosesAtFor_DSTCrossing(t *testing.T) { "naive Add(168h) must land at a different time across DST -- confirming AddDate is needed") } -// TestRevokePurchase_FourEyesApproval is a placeholder for the revoke+4-eyes -// integration test. The 4-eyes approval gate for revocations is tracked in -// issue #1005 and will be implemented in a follow-up PR. -// -// This test is intentionally skipped so the suite stays green while the -// feature is in development; remove the t.Skip when #1005 lands. -func TestRevokePurchase_FourEyesApproval(t *testing.T) { - t.Skip("placeholder until #1005 4-eyes revocation approval lands") -} - // TestRevokePurchase_GetExecutionByIDDBError_Returns500 guards that a genuine // DB error from GetExecutionByID surfaces as 500 rather than silently falling // through to the purchase_history lookup path (Finding C, second-wave CR). diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 3b3b6abf..1c008ba7 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -6395,12 +6395,6 @@ func TestRevoke_NoTimestamp_Allowed(t *testing.T) { require.NoError(t, err, "legacy rows without timestamp must not be blocked by window check") } -// TestRevoke_4EyesMode_TODO is a placeholder to surface the 4-eyes-mode -// interaction once issue #1005 lands. -func TestRevoke_4EyesMode_TODO(t *testing.T) { - t.Skip("placeholder until #1005 lands: verify that 4-eyes-mode approval flows interact correctly with the revocation window") -} - // TestRevokePurchase_GETRendersConfirmationPage_NoMutation verifies that a GET // to the revoke handler returns 200 HTML containing a confirmation form and // does NOT call TransitionExecutionStatus (no mutation). diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index 8a323122..19dafa5d 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "net/mail" "strings" "time" @@ -463,9 +464,11 @@ func validateRegistrationRequest(req *RegistrationRequest) error { if req.ContactEmail == "" { return NewClientError(400, "contact_email is required") } - if !strings.Contains(req.ContactEmail, "@") || len(req.ContactEmail) < 5 { - return NewClientError(400, "contact_email must be a valid email address") + contactEmail, err := registrationContactEmail(req.ContactEmail) + if err != nil { + return err } + req.ContactEmail = contactEmail credential, err := parseRegistrationCredential(req.Provider, req.CredentialType, req.CredentialPayload) if err != nil { return err @@ -474,6 +477,27 @@ func validateRegistrationRequest(req *RegistrationRequest) error { return nil } +// registrationContactEmail returns the bare address in a registrant's +// contact_email, the form that is stored and later handed to the notifier, +// or a 400. It applies validateEmailFormat, the check the rest of the +// package uses (issue #311), so a value with a line break or a second +// address after it, or a host with no TLD, is refused rather than stored. +// A display-name form ("Jane ") is accepted and reduced +// to its address. The check runs on that bare address, so one that would +// not parse again on its own (a quoted local part such as "a b"@example.com +// unquotes to a b@example.com) is refused too. +func registrationContactEmail(raw string) (string, error) { + invalid := NewClientError(400, "contact_email must be a valid email address") + addr, err := mail.ParseAddress(raw) + if err != nil { + return "", invalid + } + if validateEmailFormat(addr.Address) != nil { + return "", invalid + } + return addr.Address, nil +} + func buildRegistrationFilter(params map[string]string) config.AccountRegistrationFilter { var filter config.AccountRegistrationFilter if s, ok := params["status"]; ok && s != "" { diff --git a/internal/api/handler_registrations_submit_test.go b/internal/api/handler_registrations_submit_test.go index 019056b9..a84fa520 100644 --- a/internal/api/handler_registrations_submit_test.go +++ b/internal/api/handler_registrations_submit_test.go @@ -129,3 +129,59 @@ func TestSubmitRegistration_TokenCollision_GivesUpAs500(t *testing.T) { assert.False(t, isClient) store.AssertNumberOfCalls(t, "CreateAccountRegistration", maxReferenceTokenAttempts) } + +// The registration row and the admin notification both carry the bare +// contact address, not the submitted display-name form (issue #311). +func TestSubmitRegistration_StoresBareContactEmail(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + var stored *config.AccountRegistration + store.On("CreateAccountRegistration", ctx, mock.Anything).Run(func(args mock.Arguments) { + stored = args.Get(1).(*config.AccountRegistration) + }).Return(nil).Once() + notifier := &capturingRegistrationNotifier{} + h := &Handler{config: store, emailNotifier: notifier} + + _, err := h.submitRegistration(ctx, &events.LambdaFunctionURLRequest{ + Body: `{"provider":"aws","external_id":"ext-123","account_name":"Prod",` + + `"contact_email":"Jane Doe "}`, + }) + + require.NoError(t, err) + require.NotNil(t, stored) + assert.Equal(t, "jane@example.com", stored.ContactEmail) + assert.Equal(t, "jane@example.com", notifier.contactEmail) +} + +// A contact_email carrying a header after a line break is refused before +// anything is stored or sent (issue #311). +func TestSubmitRegistration_RejectsHeaderInjectionContactEmail(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + store.On("CreateAccountRegistration", ctx, mock.Anything).Return(nil).Maybe() + notifier := &countingRegistrationNotifier{} + h := &Handler{config: store, emailNotifier: notifier} + + _, err := h.submitRegistration(ctx, &events.LambdaFunctionURLRequest{ + Body: `{"provider":"aws","external_id":"ext-123","account_name":"Prod",` + + `"contact_email":"a@b\r\nBcc: x@y"}`, + }) + + ce, ok := IsClientError(err) + require.True(t, ok, "want a 400 client error, got %v", err) + assert.Equal(t, 400, ce.code) + store.AssertNotCalled(t, "CreateAccountRegistration", mock.Anything, mock.Anything) + assert.Zero(t, notifier.received) +} + +// capturingRegistrationNotifier records the contact email of the admin +// "registration received" notification. +type capturingRegistrationNotifier struct { + stubEmailNotifier + contactEmail string +} + +func (n *capturingRegistrationNotifier) SendRegistrationReceivedNotification(_ context.Context, data email.RegistrationNotificationData) error { + n.contactEmail = data.ContactEmail + return nil +} diff --git a/internal/api/handler_registrations_validate_test.go b/internal/api/handler_registrations_validate_test.go index 8237431d..25f04e8c 100644 --- a/internal/api/handler_registrations_validate_test.go +++ b/internal/api/handler_registrations_validate_test.go @@ -64,3 +64,51 @@ func TestValidateRegistrationRequest_AccountNameSanitized(t *testing.T) { assert.Contains(t, err.Error(), "account_name is required") }) } + +// TestValidateRegistrationRequest_ContactEmail pins issue #311: the public +// registration endpoint used to accept any 5+ character string containing +// "@" as contact_email, so a value carrying a line break and a Bcc header, +// a second address, or a host with no TLD was stored as-is. It now applies +// validateEmailFormat and stores the bare address. +func TestValidateRegistrationRequest_ContactEmail(t *testing.T) { + valid := map[string]string{ + "plain address": "user@example.com", + "display-name form": "Jane Doe ", + "surrounding space": " user@example.com ", + "trailing comment": "user@example.com (ops)", + } + wantStored := map[string]string{ + "plain address": "user@example.com", + "display-name form": "jane@example.com", + "surrounding space": "user@example.com", + "trailing comment": "user@example.com", + } + for name, raw := range valid { + t.Run("accepts "+name, func(t *testing.T) { + req := RegistrationRequest{Provider: "aws", ExternalID: "ext-123", AccountName: "Acme", ContactEmail: raw} + require.NoError(t, validateRegistrationRequest(&req)) + assert.Equal(t, wantStored[name], req.ContactEmail, "the bare address must be stored") + }) + } + + invalid := map[string]string{ + "CRLF header injection": "a@b\r\nBcc: x@y", + "CRLF after valid addr": "user@example.com\r\nBcc: attacker@evil.example.com", + "two addresses": "user@example.com, other@example.com", + "no TLD": "user@hostonly", + "no local part": "@example.com", + "no at sign": "user.example.com", + "quoted local part": `"a b"@example.com`, + "display name only": "Jane Doe", + } + for name, raw := range invalid { + t.Run("rejects "+name, func(t *testing.T) { + req := RegistrationRequest{Provider: "aws", ExternalID: "ext-123", AccountName: "Acme", ContactEmail: raw} + err := validateRegistrationRequest(&req) + ce, ok := IsClientError(err) + require.True(t, ok, "want a 400 client error, got %v", err) + assert.Equal(t, 400, ce.code) + assert.Equal(t, "contact_email must be a valid email address", ce.message) + }) + } +} diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 9ab4f6a2..ac0989fc 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -259,6 +259,18 @@ func (h *Handler) buildAzureExchangeClient(ctx context.Context, subscriptionID s if err != nil { return nil, fmt.Errorf("azure: look up account for subscription %q: %w", subscriptionID, err) } + return h.azureExchangeClientForAccount(ctx, subscriptionID, account) +} + +// azureExchangeClientForAccount is buildAzureExchangeClient for a caller that +// has already resolved the CloudAccount registered for subscriptionID (nil +// when none is), so the same row is not fetched again (issue #311). The +// injected factory still wins, and a nil account still yields the graceful +// (nil, nil) empty state. +func (h *Handler) azureExchangeClientForAccount(ctx context.Context, subscriptionID string, account *config.CloudAccount) (azureExchangeClient, error) { + if h.azureExchangeFactory != nil { + return h.azureExchangeFactory(subscriptionID), nil + } if account == nil { // No Azure account registered for this subscription. // Return nil client so the caller emits a graceful empty state. @@ -850,16 +862,46 @@ func (h *Handler) requireAzureSubscriptionScope(ctx context.Context, session *Se if allowed.AllowsAll() { return nil } - account, err := h.config.GetCloudAccountByExternalID(ctx, "azure", subscriptionID) + account, err := h.lookupAzureAccount(ctx, subscriptionID) if err != nil { - return fmt.Errorf("failed to resolve cloud account scope: %w", err) + return err } + return azureAccountInScope(allowed, account) +} + +// requireAzureAccountScope is requireAzureSubscriptionScope for a caller +// that has already resolved the subscription's CloudAccount (nil when none +// is registered), with the same errNotFound denials. +func (h *Handler) requireAzureAccountScope(ctx context.Context, session *Session, account *config.CloudAccount) error { + allowed, err := h.getAccountScope(ctx, session) + if err != nil { + return fmt.Errorf("failed to get allowed accounts: %w", err) + } + if allowed.AllowsAll() { + return nil + } + return azureAccountInScope(allowed, account) +} + +// azureAccountInScope refuses, with errNotFound, an unregistered account and +// one outside the session's scope alike, so the two are indistinguishable. +func azureAccountInScope(allowed auth.AccountScope, account *config.CloudAccount) error { if account == nil || !allowed.Allows(account.ID, account.Name) { return errNotFound } return nil } +// lookupAzureAccount returns the CloudAccount registered for subscriptionID, +// or nil when none is. +func (h *Handler) lookupAzureAccount(ctx context.Context, subscriptionID string) (*config.CloudAccount, error) { + account, err := h.config.GetCloudAccountByExternalID(ctx, "azure", subscriptionID) + if err != nil { + return nil, fmt.Errorf("failed to resolve cloud account scope: %w", err) + } + return account, nil +} + // requireAzureSourceOwnership refuses any source reservation that is not // paid for by the authorized subscription (issue #1527). // @@ -1032,6 +1074,14 @@ const azureMaxPurchaseAmountCurrency = "USD" // known-owned, so unknownRegionConstraint means only "owned, but Azure // reported no region" rather than doubling as "not yours" or "not real". func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session *Session, body AzureExecuteExchangeRequestBody, maxRat *big.Rat) (azureExchangeClient, []azurecompute.ExchangeableReservation, string, error) { + // The subscription's CloudAccount is resolved once and that one row + // feeds the scope check, the client's credentials and the constraint + // AccountIDs (issue #311), so the three cannot see different rows. + account, err := h.lookupAzureAccount(ctx, body.SubscriptionID) + if err != nil { + return nil, nil, "", err + } + // Scope check MUST precede building the client (mirrors // getAzureCompatibleOfferings): otherwise an unregistered subscription // (distinguishable 404: "no Azure account registered...") and a @@ -1039,11 +1089,11 @@ func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session * // enumeration signal to a scoped caller about which subscriptions exist, // and credentials for an out-of-scope account could be resolved before // the denial. - if scopeErr := h.requireAzureSubscriptionScope(ctx, session, body.SubscriptionID); scopeErr != nil { + if scopeErr := h.requireAzureAccountScope(ctx, session, account); scopeErr != nil { return nil, nil, "", scopeErr } - client, err := h.buildAzureExchangeClient(ctx, body.SubscriptionID) + client, err := h.azureExchangeClientForAccount(ctx, body.SubscriptionID, account) if err != nil { return nil, nil, "", fmt.Errorf("failed to build Azure exchange client: %w", err) } @@ -1060,32 +1110,23 @@ func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session * return nil, nil, "", ownErr } - accountID, err := h.resolveAzureExchangeAccountID(ctx, body.SubscriptionID) - if err != nil { - return nil, nil, "", err - } - + accountID := azureExchangeAccountID(account) if err := h.checkAzureExecuteConstraints(ctx, session, body, accountID, maxRat, exchangeRegions(body.Targets, body.Sources, owned)); err != nil { return nil, nil, "", err } return client, toAzureExchangeSources(body.Sources, owned), accountID, nil } -// resolveAzureExchangeAccountID looks up the CloudAccount registered for -// subscriptionID and returns its ID, or unattributedAccountConstraint when -// no account is registered (so an AccountIDs-constrained permission still -// fails closed against an unattributed request). Extracted from -// authorizeAzureExchangeExecution to keep that function under the gocyclo -// limit. -func (h *Handler) resolveAzureExchangeAccountID(ctx context.Context, subscriptionID string) (string, error) { - account, err := h.config.GetCloudAccountByExternalID(ctx, "azure", subscriptionID) - if err != nil { - return "", fmt.Errorf("failed to resolve cloud account scope: %w", err) - } +// azureExchangeAccountID returns the resolved CloudAccount's ID, or +// unattributedAccountConstraint when no account is registered (so an +// AccountIDs-constrained permission still fails closed against an +// unattributed request). Only the injected azureExchangeFactory path gets +// this far with a nil account: production refuses it with a 404 earlier. +func azureExchangeAccountID(account *config.CloudAccount) string { if account != nil { - return account.ID, nil + return account.ID } - return unattributedAccountConstraint, nil + return unattributedAccountConstraint } // checkAzureExecuteConstraints enforces the execute:ri-exchange permission diff --git a/internal/api/handler_ri_exchange_azure_test.go b/internal/api/handler_ri_exchange_azure_test.go index 594b1396..106fb146 100644 --- a/internal/api/handler_ri_exchange_azure_test.go +++ b/internal/api/handler_ri_exchange_azure_test.go @@ -603,6 +603,59 @@ func TestExecuteAzureExchange_InScopeSubscriptionAllowed(t *testing.T) { assert.Equal(t, "sess-fresh", resp.SessionID) } +// TestExecuteAzureExchange_ResolvesAccountOnce pins issue #311: the execute +// path looked the subscription's CloudAccount up separately for the scope +// check, the client build and the constraint AccountIDs. Here the store +// answers the first lookup with the caller's own account and any later one +// with another account, as a concurrent re-registration would. The scope +// check and the constraint check must both see the first row, from a single +// lookup. +func TestExecuteAzureExchange_ResolvesAccountOnce(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-once", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", mock.Anything, "sess-once").Return( + &azurecompute.ExchangeResult{SessionID: "sess-once", Status: "Succeeded"}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + var captured []auth.PermissionConstraints + mockAuth := scopedAzureAuth(t, "execute", "ri-exchange", []string{"acct-mine"}) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + captured = sets + return true + })).Return(true, nil) + + lookups := 0 + store := &MockConfigStore{} + store.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + assert.Equal(t, "azure", provider) + assert.Equal(t, "sub-1", externalID) + lookups++ + if lookups == 1 { + return &config.CloudAccount{ID: "acct-mine", Name: "My Team"}, nil + } + return &config.CloudAccount{ID: "acct-other", Name: "Other Team"}, nil + } + + h := &Handler{auth: mockAuth, config: store, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.NoError(t, err) + + assert.Equal(t, 1, lookups, "the execute path must resolve the subscription's CloudAccount exactly once") + require.Len(t, captured, 1) + assert.Equal(t, []string{"acct-mine"}, captured[0].AccountIDs, + "the constraint check must use the same account row the scope check allowed") +} + // TestExecuteAzureExchange_ScopeCheckPrecedesClientBuild_Unregistered proves // the fix for the authz-ordering finding: requireAzureSubscriptionScope must // run BEFORE buildAzureExchangeClient, exactly like getAzureCompatibleOfferings.