From 3aadeac3dff63b13b2139c5d0cf895f2e4a1254f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 20 May 2026 18:29:00 +0200 Subject: [PATCH 1/3] sec(email): sanitize AccountName/Provider in SES registration subject PR #523 fixed only the SMTP registration-subject path. The SES path (SendRegistrationReceivedNotification in internal/email/templates.go) still built the "CUDly - New Account Registration: %s (%s)" subject from raw, attacker-controlled data.AccountName and data.Provider sourced from the unauthenticated POST /api/register endpoint, then passed it to the SES SendEmail API. A CR+LF in account_name could inject extra email headers. Wrap both fields in the existing sanitizeHeader helper (reused from the SMTP path) and add defense-in-depth at the data source: validateRegistrationRequest now strips CR/LF and length-caps account_name in place. Adds regression tests on the SES subject path and the handler normalizer. Closes #544. --- internal/api/handler_registrations.go | 19 +++++-- .../handler_registrations_validate_test.go | 50 +++++++++++++++++++ internal/email/templates.go | 7 ++- internal/email/templates_test.go | 46 +++++++++++++++++ 4 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 internal/api/handler_registrations_validate_test.go diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index 100d8b6ec..7b4040075 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -64,7 +64,7 @@ func (h *Handler) submitRegistration(ctx context.Context, req *events.LambdaFunc return nil, NewClientError(400, "invalid request body") } - if err := validateRegistrationRequest(body); err != nil { + if err := validateRegistrationRequest(&body); err != nil { return nil, err } @@ -394,8 +394,21 @@ func (h *Handler) deleteRegistration(ctx context.Context, id string) (any, error return map[string]string{"status": "deleted"}, nil } -// validateRegistrationRequest checks required fields for a registration submission. -func validateRegistrationRequest(req RegistrationRequest) error { +// maxAccountNameLen caps the persisted/displayed account name length as a +// defense-in-depth measure against abuse of the unauthenticated registration +// endpoint (#544 / #401). +const maxAccountNameLen = 256 + +// validateRegistrationRequest checks required fields for a registration +// submission. It also normalizes account_name in place: CR/LF characters are +// stripped (defense-in-depth against email header injection at the data +// source, complementing sanitizeHeader on the email subject path) and the +// result is length-capped. +func validateRegistrationRequest(req *RegistrationRequest) error { + req.AccountName = strings.NewReplacer("\r", "", "\n", "").Replace(req.AccountName) + if r := []rune(req.AccountName); len(r) > maxAccountNameLen { + req.AccountName = string(r[:maxAccountNameLen]) + } if !validAccountProviders[req.Provider] { return NewClientError(400, "provider must be one of: aws, azure, gcp") } diff --git a/internal/api/handler_registrations_validate_test.go b/internal/api/handler_registrations_validate_test.go new file mode 100644 index 000000000..0acada750 --- /dev/null +++ b/internal/api/handler_registrations_validate_test.go @@ -0,0 +1,50 @@ +package api + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestValidateRegistrationRequest_AccountNameSanitized is the defense-in-depth +// regression test for #544 / #401: account_name from the unauthenticated +// POST /api/register endpoint must be CR/LF-stripped and length-capped at the +// data source before it is persisted or interpolated into any email header. +func TestValidateRegistrationRequest_AccountNameSanitized(t *testing.T) { + t.Run("strips CRLF", func(t *testing.T) { + req := RegistrationRequest{ + Provider: "aws", + ExternalID: "ext-123", + AccountName: "Acme\r\nBcc: attacker@evil.example.com", + ContactEmail: "user@example.com", + } + require.NoError(t, validateRegistrationRequest(&req)) + assert.NotContains(t, req.AccountName, "\r") + assert.NotContains(t, req.AccountName, "\n") + }) + + t.Run("length-capped", func(t *testing.T) { + req := RegistrationRequest{ + Provider: "aws", + ExternalID: "ext-123", + AccountName: strings.Repeat("a", maxAccountNameLen+50), + ContactEmail: "user@example.com", + } + require.NoError(t, validateRegistrationRequest(&req)) + assert.LessOrEqual(t, len(req.AccountName), maxAccountNameLen) + }) + + t.Run("CRLF-only name is rejected as empty", func(t *testing.T) { + req := RegistrationRequest{ + Provider: "aws", + ExternalID: "ext-123", + AccountName: "\r\n", + ContactEmail: "user@example.com", + } + err := validateRegistrationRequest(&req) + require.Error(t, err) + assert.Contains(t, err.Error(), "account_name is required") + }) +} diff --git a/internal/email/templates.go b/internal/email/templates.go index ea92c2744..824218557 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -805,7 +805,12 @@ func (s *Sender) SendRegistrationReceivedNotification(ctx context.Context, data if err != nil { return fmt.Errorf("failed to render registration received email: %w", err) } - subject := fmt.Sprintf("CUDly - New Account Registration: %s (%s)", data.AccountName, data.Provider) + // sanitizeHeader strips CR/LF from the attacker-controlled AccountName / + // Provider (sourced from the unauthenticated POST /api/register endpoint) + // before they are interpolated into the SES email subject, preventing + // email header injection (#544 / #401). Mirrors the SMTP path fix. + subject := fmt.Sprintf("CUDly - New Account Registration: %s (%s)", + sanitizeHeader(data.AccountName), sanitizeHeader(data.Provider)) if data.RecipientEmail == "" { return s.SendNotification(ctx, subject, body) } diff --git a/internal/email/templates_test.go b/internal/email/templates_test.go index 43698fd9d..a4388fac5 100644 --- a/internal/email/templates_test.go +++ b/internal/email/templates_test.go @@ -613,3 +613,49 @@ func containsAnyStr(s string, subs ...string) bool { } return false } + +// TestSender_SendRegistrationReceivedNotification_SubjectHeaderInjection is the +// SES-path regression test for #544 / #401: a CR+LF in the attacker-controlled +// AccountName / Provider (sourced from the unauthenticated POST /api/register +// endpoint) must be stripped before the subject reaches the SES SendEmail API, +// so it cannot inject additional email headers. Mirrors the SMTP-path test. +func TestSender_SendRegistrationReceivedNotification_SubjectHeaderInjection(t *testing.T) { + mockSES := new(MockSESClient) + // Production mode so the send proceeds straight to SendEmail. + mockSES.On("GetAccount", mock.Anything, mock.AnythingOfType("*sesv2.GetAccountInput")). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) + + var capturedSubject string + mockSES.On("SendEmail", mock.Anything, mock.AnythingOfType("*sesv2.SendEmailInput")). + Run(func(args mock.Arguments) { + input := args.Get(1).(*sesv2.SendEmailInput) + capturedSubject = aws.ToString(input.Content.Simple.Subject.Data) + }). + Return(&sesv2.SendEmailOutput{MessageId: aws.String("msg-injection")}, nil) + + sender := NewSenderWithClients(nil, mockSES, SenderConfig{ + FromEmail: "noreply@example.com", + }) + + injectedName := "Acme\r\nBcc: attacker@evil.example.com" + injectedProvider := "aws\r\nX-Injected: yes" + data := RegistrationNotificationData{ + AccountName: injectedName, + Provider: injectedProvider, + ExternalID: "ext-123", + ContactEmail: "registrant@example.com", + RecipientEmail: "admin@example.com", + } + + ctx := context.Background() + err := sender.SendRegistrationReceivedNotification(ctx, data) + require.NoError(t, err) + mockSES.AssertExpectations(t) + + // The subject that reached SES must contain no CR/LF characters. + assert.NotContains(t, capturedSubject, "\r", "SES subject must not contain CR: %q", capturedSubject) + assert.NotContains(t, capturedSubject, "\n", "SES subject must not contain LF: %q", capturedSubject) + // The injected header names must not survive into the subject as injectable headers. + assert.NotContains(t, capturedSubject, "\nBcc:") + assert.NotContains(t, capturedSubject, "\nX-Injected:") +} From 9f61dfc23473cc4b319fb89f9ee4ca4e9d85d2bc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 20 May 2026 18:37:00 +0200 Subject: [PATCH 2/3] test(api): assert account_name cap by rune count, not byte length CodeRabbit flagged that the length-cap test measured byte length while the implementation caps by rune count, so the ASCII-only input never exercised rune-safety. Switch the input to a multibyte rune, assert via utf8.RuneCountInString, and verify the truncated result is still valid UTF-8. --- internal/api/handler_registrations_validate_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/api/handler_registrations_validate_test.go b/internal/api/handler_registrations_validate_test.go index 0acada750..a2ee68151 100644 --- a/internal/api/handler_registrations_validate_test.go +++ b/internal/api/handler_registrations_validate_test.go @@ -3,6 +3,7 @@ package api import ( "strings" "testing" + "unicode/utf8" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -25,15 +26,18 @@ func TestValidateRegistrationRequest_AccountNameSanitized(t *testing.T) { assert.NotContains(t, req.AccountName, "\n") }) - t.Run("length-capped", func(t *testing.T) { + t.Run("length-capped (rune-safe)", func(t *testing.T) { + // Use a multibyte rune so the cap is exercised by rune count, not byte + // length, and confirm truncation never splits a rune (valid UTF-8). req := RegistrationRequest{ Provider: "aws", ExternalID: "ext-123", - AccountName: strings.Repeat("a", maxAccountNameLen+50), + AccountName: strings.Repeat("é", maxAccountNameLen+50), ContactEmail: "user@example.com", } require.NoError(t, validateRegistrationRequest(&req)) - assert.LessOrEqual(t, len(req.AccountName), maxAccountNameLen) + assert.LessOrEqual(t, utf8.RuneCountInString(req.AccountName), maxAccountNameLen) + assert.True(t, utf8.ValidString(req.AccountName), "cap must not split a multibyte rune") }) t.Run("CRLF-only name is rejected as empty", func(t *testing.T) { From 923720c4724b830a88ca9bb287f9ef6fbfe23f9f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 18:35:46 +0200 Subject: [PATCH 3/3] sec(registrations): reject whitespace-only account_name after trim After stripping CR/LF the account_name could still be whitespace-only (spaces, tabs), which passed the empty-string guard and produced an SES subject line containing `Account "": `. Add strings.TrimSpace after the CR/LF replacement so the existing empty-string check also catches that case. Adds a regression test: POST with account_name " \t " must return 400 with "account_name is required". Closes finding on PR #566 (CR review 2026-05-22). --- internal/api/handler_registrations.go | 2 +- internal/api/handler_registrations_validate_test.go | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index 7b4040075..38f99c7cf 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -405,7 +405,7 @@ const maxAccountNameLen = 256 // source, complementing sanitizeHeader on the email subject path) and the // result is length-capped. func validateRegistrationRequest(req *RegistrationRequest) error { - req.AccountName = strings.NewReplacer("\r", "", "\n", "").Replace(req.AccountName) + req.AccountName = strings.TrimSpace(strings.NewReplacer("\r", "", "\n", "").Replace(req.AccountName)) if r := []rune(req.AccountName); len(r) > maxAccountNameLen { req.AccountName = string(r[:maxAccountNameLen]) } diff --git a/internal/api/handler_registrations_validate_test.go b/internal/api/handler_registrations_validate_test.go index a2ee68151..8237431d4 100644 --- a/internal/api/handler_registrations_validate_test.go +++ b/internal/api/handler_registrations_validate_test.go @@ -51,4 +51,16 @@ func TestValidateRegistrationRequest_AccountNameSanitized(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "account_name is required") }) + + t.Run("whitespace-only name is rejected as empty", func(t *testing.T) { + req := RegistrationRequest{ + Provider: "aws", + ExternalID: "ext-123", + AccountName: " \t ", + ContactEmail: "user@example.com", + } + err := validateRegistrationRequest(&req) + require.Error(t, err) + assert.Contains(t, err.Error(), "account_name is required") + }) }