diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 780e83814..78f3a6313 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -303,7 +303,7 @@ func validateCloudAccountRequest(req CloudAccountRequest) error { } if err := validateEmailFormat(req.ContactEmail); err != nil { - return err + return NewClientError(400, "invalid contact_email format") } return validateAuthMode(req) diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index ffd94c9d5..4332fdbec 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -205,6 +205,44 @@ func TestCreateAccount_EmptyContactEmail(t *testing.T) { assert.Empty(t, got.ContactEmail) } +// TestCreateAccount_RejectsTLDlessContactEmail is a regression guard for +// issue #868: account-create must reject a TLD-less contact_email such as +// "user@host" with 400, applying the same constraint as sign-up. +func TestCreateAccount_RejectsTLDlessContactEmail(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + store := setupAdminMock(ctx) + handler := &Handler{auth: mockAuth, config: store} + + body := `{"name":"Acme","provider":"aws","external_id":"123456789012","contact_email":"admin@intranet"}` + result, err := handler.createAccount(ctx, adminRequest(body)) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.message, "contact_email") +} + +// TestCreateAccount_AcceptsValidContactEmail verifies that a well-formed +// contact_email passes validation and the account is stored correctly. +func TestCreateAccount_AcceptsValidContactEmail(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + store := setupAdminMock(ctx) + handler := &Handler{auth: mockAuth, config: store} + + body := `{"name":"Acme","provider":"aws","external_id":"123456789012","contact_email":"admin@example.com"}` + result, err := handler.createAccount(ctx, adminRequest(body)) + require.NoError(t, err) + got := result.(*config.CloudAccount) + assert.Equal(t, "admin@example.com", got.ContactEmail) +} + // --- getAccount --- func TestGetAccount_Success(t *testing.T) { diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index b8a03b8af..017b7b0ac 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -282,19 +282,15 @@ func (h *Handler) updateProfile(ctx context.Context, req *events.LambdaFunctionU return nil, NewClientError(400, "invalid request body") } - // Decode base64-encoded passwords if provided - var currentPassword, newPassword string - if profileReq.CurrentPassword != "" { - currentPassword, err = decodeBase64Password(profileReq.CurrentPassword) - if err != nil { - return nil, err - } + // Validate email format before decoding passwords (cheap check first). + if err := validateEmailFormat(profileReq.Email); err != nil { + return nil, err } - if profileReq.NewPassword != "" { - newPassword, err = decodeBase64Password(profileReq.NewPassword) - if err != nil { - return nil, err - } + + // Decode base64-encoded passwords if provided + currentPassword, newPassword, err := decodeProfilePasswords(profileReq) + if err != nil { + return nil, err } // Update profile through auth service @@ -305,6 +301,25 @@ func (h *Handler) updateProfile(ctx context.Context, req *events.LambdaFunctionU return map[string]string{"status": "profile updated"}, nil } +// decodeProfilePasswords decodes the optional base64-encoded current and new +// passwords from a ProfileUpdateRequest. Pulled out of updateProfile to keep +// that function under the cyclomatic limit. +func decodeProfilePasswords(req ProfileUpdateRequest) (current, next string, err error) { + if req.CurrentPassword != "" { + current, err = decodeBase64Password(req.CurrentPassword) + if err != nil { + return "", "", err + } + } + if req.NewPassword != "" { + next, err = decodeBase64Password(req.NewPassword) + if err != nil { + return "", "", err + } + } + return current, next, nil +} + // decodeChangePasswordRequest validates and decodes both passwords from a ChangePasswordRequest. func decodeChangePasswordRequest(pwdReq ChangePasswordRequest) (current, next string, err error) { if pwdReq.CurrentPassword == "" || pwdReq.NewPassword == "" { diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index c5bdb27ec..f7fe30b48 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -899,6 +899,60 @@ func TestHandler_updateProfile_InvalidJSON(t *testing.T) { assert.Contains(t, err.Error(), "invalid request body") } +// TestHandler_updateProfile_RejectsInvalidEmail is a regression guard for +// issue #868: the profile-update handler must reject TLD-less addresses such +// as "user@host" with a 400 before reaching the auth service, mirroring the +// constraint that sign-up already enforces. +func TestHandler_updateProfile_RejectsInvalidEmail(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + + session := &Session{UserID: "12345678-1234-1234-1234-123456789abc"} + mockAuth.On("ValidateSession", ctx, "test-token").Return(session, nil) + // UpdateUserProfile must NOT be called — validation should short-circuit first. + + handler := &Handler{auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer test-token"}, + Body: `{"email": "user@host"}`, + } + + result, err := handler.updateProfile(ctx, req) + assert.Nil(t, result) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.message, "email") + // Confirm UpdateUserProfile was never reached. + mockAuth.AssertNotCalled(t, "UpdateUserProfile") +} + +// TestHandler_updateProfile_AcceptsValidEmail verifies that a well-formed +// address passes validation and reaches the auth service unchanged. +func TestHandler_updateProfile_AcceptsValidEmail(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + + session := &Session{UserID: "12345678-1234-1234-1234-123456789abc"} + mockAuth.On("ValidateSession", ctx, "test-token").Return(session, nil) + mockAuth.On("UpdateUserProfile", ctx, "12345678-1234-1234-1234-123456789abc", "new@example.com", "", "").Return(nil) + + handler := &Handler{auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer test-token"}, + Body: `{"email": "new@example.com"}`, + } + + result, err := handler.updateProfile(ctx, req) + require.NoError(t, err) + resp := result.(map[string]string) + assert.Equal(t, "profile updated", resp["status"]) + mockAuth.AssertCalled(t, "UpdateUserProfile", ctx, "12345678-1234-1234-1234-123456789abc", "new@example.com", "", "") +} + // TestHandler_resetPassword_DecodesBase64 verifies issue #356: the // resetPassword handler must base64-decode new_password before forwarding to // the service, matching the pattern used by login / change-password / diff --git a/internal/api/validation.go b/internal/api/validation.go index 92a41af84..39c5760dc 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -126,14 +126,23 @@ func validateProvider(provider string) error { } // validateEmailFormat returns a 400 error when email is non-empty but does not -// parse as an RFC 5322 address. Empty strings are accepted (contact email is -// optional on cloud accounts). +// parse as an RFC 5322 address or lacks a TLD (e.g. name@hostonly). Empty +// strings are accepted (contact email is optional on cloud accounts). func validateEmailFormat(email string) error { if email == "" { return nil } - if _, err := mail.ParseAddress(email); err != nil { - return NewClientError(400, "contact_email is not a valid email address") + addr, err := mail.ParseAddress(email) + if err != nil { + return NewClientError(400, "invalid email format") + } + // mail.ParseAddress is RFC 5322-compliant and accepts single-label domains + // like "name@host" that have no TLD. Reject them here so the profile-update + // path applies the same constraint as sign-up. The address portion always + // contains exactly one "@" after a successful parse. + at := strings.LastIndex(addr.Address, "@") + if at < 0 || !strings.Contains(addr.Address[at+1:], ".") { + return NewClientError(400, "invalid email format") } return nil } diff --git a/internal/api/validation_test.go b/internal/api/validation_test.go index f6bb6a094..5e07fb396 100644 --- a/internal/api/validation_test.go +++ b/internal/api/validation_test.go @@ -414,6 +414,55 @@ func TestValidateAWSRoleARN(t *testing.T) { } } +// TestValidateEmailFormat covers issue #868: validateEmailFormat must reject TLD-less +// addresses like "user@host" that RFC 5322 accepts but that sign-up also rejects. +// The profile-update and account-create paths now call the same validator, so this +// table locks in parity across all three entry points. +func TestValidateEmailFormat(t *testing.T) { + t.Parallel() + tests := []struct { + name string + email string + wantError bool + }{ + // Happy paths + {"empty is allowed (optional field)", "", false}, + {"typical address", "user@example.com", false}, + {"dotted local part", "user.name@example.com", false}, + {"plus tag", "user+tag@sub.example.com", false}, + {"minimum valid (short TLD)", "u@a.b", false}, + {"subdomain", "admin@mail.corp.example.com", false}, + + // Issue #868 cases — TLD-less addresses that were previously accepted + {"no TLD (bare host)", "user@host", true}, + {"trailing dot on domain", "user@host.", true}, + {"dot before host (no host part)", "user@.com", true}, + + // Other invalid shapes + {"empty string is OK (see first row), but just @", "@", true}, + {"no local part", "@host.com", true}, + {"space in local part", "user @host.com", true}, + {"space in domain", "user@host .com", true}, + {"no at-sign", "notanemail", true}, + {"double at-sign", "a@@b.com", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateEmailFormat(tt.email) + if tt.wantError { + assert.Error(t, err) + ce, ok := IsClientError(err) + if assert.True(t, ok, "expected ClientError") { + assert.Equal(t, 400, ce.code) + } + } else { + assert.NoError(t, err) + } + }) + } +} + // TestValidateAWSWebIdentityTokenFile covers issue #403: aws_web_identity_token_file // must be restricted to known-safe mount prefixes to prevent arbitrary host file reads. func TestValidateAWSWebIdentityTokenFile(t *testing.T) {