From e54f250699194ef15931f9ed892816f374a1061f Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 06:22:27 +0000 Subject: [PATCH] chore(api): drop dead revoke binding and placeholders, resolve the Azure account once, validate registration email Three of the four internal/api findings in issue #311: - revokeViaSession no longer binds TransitionExecutionStatus's returned row only to discard it, and its doc comment now says how CancelledBy is written and that neither revoke path applies the 4-eyes policy. - Delete the two bare t.Skip revoke+4-eyes placeholders; revocation is not dual-controlled, and whether it should be is an owner decision. - The Azure exchange execute path looks the subscription's CloudAccount up once and uses that row for the scope check, the client credentials and the constraint AccountIDs, instead of three lookups that could disagree. - POST /api/register validates contact_email with validateEmailFormat and stores the bare address, instead of accepting any 5+ character string containing "@" (which stored "a@b\r\nBcc: x@y"). The findDuplicatePendingExecution removal is left to the open #1217, which already deletes it. Refs #311 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 14 ++- internal/api/handler_purchases_revoke_test.go | 10 --- internal/api/handler_purchases_test.go | 6 -- internal/api/handler_registrations.go | 28 +++++- .../api/handler_registrations_submit_test.go | 56 ++++++++++++ .../handler_registrations_validate_test.go | 48 +++++++++++ internal/api/handler_ri_exchange.go | 85 ++++++++++++++----- .../api/handler_ri_exchange_azure_test.go | 53 ++++++++++++ 8 files changed, 256 insertions(+), 44 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index ce5212ded..10573fa8b 100644 --- a/internal/api/handler_purchases.go +++ b/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{ diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index a88254cfe..7b77b372e 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 2bda5e715..aa2260576 100644 --- a/internal/api/handler_purchases_test.go +++ b/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). diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index b101d4cda..904bbbfa9 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" @@ -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 ") 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 019056b90..a84fa5209 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 8237431d4..25f04e8ce 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 1df14783b..e7a520ec5 100644 --- a/internal/api/handler_ri_exchange.go +++ b/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,6 +1073,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 @@ -1038,11 +1088,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) } @@ -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 diff --git a/internal/api/handler_ri_exchange_azure_test.go b/internal/api/handler_ri_exchange_azure_test.go index 4a78a00d7..fda08e7fd 100644 --- a/internal/api/handler_ri_exchange_azure_test.go +++ b/internal/api/handler_ri_exchange_azure_test.go @@ -602,6 +602,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.