chore(api): drop dead revoke binding and placeholders, resolve the Azure account once, validate registration email

internal/api/handler_purchases.go

@@ -2039,8 +2039,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
@@ -2049,11 +2050,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 {
@@ -2076,7 +2083,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{

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).

internal/api/handler_purchases_test.go

@@ -6363,12 +6363,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).

internal/api/handler_registrations.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"net/mail"
 	"strings"
 	"time"
 
@@ -461,12 +462,35 @@ 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
 	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 <jane@example.com>") 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 != "" {

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 <jane@example.com>"}`,
+	})
+
+	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
+}

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 <jane@example.com>",
+		"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)
+		})
+	}
+}

internal/api/handler_ri_exchange.go

@@ -258,6 +258,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.
@@ -849,16 +861,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).
 //
@@ -1031,18 +1073,26 @@ 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
 	// registered-but-out-of-scope one (generic errNotFound) would leak an
 	// 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)
 	}
@@ -1059,32 +1109,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