mailer: a booker's address is validated at intake, and never written into a header unparsed

CHANGELOG.md

@@ -11,6 +11,31 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad
 
 ## [Unreleased]
 
+### Security
+- **A booker's email address is validated where it enters, and is never written into an
+  email header unparsed.** The `To:` header was the one header field assembled from
+  caller-supplied input with no encoder in front of it: `buildRaw` parsed each recipient
+  with `net/mail` and, when that failed, appended the raw string anyway, so a CR/LF inside
+  an address would have ended the `To:` line and started a header of the sender's
+  choosing. Subject and the `From` display name already go through `mime.QEncoding`
+  (which renders CR/LF as `=0D`/`=0A`) and attachment filenames through `%q`.
+
+  Not exploitable as shipped: `Send` issues `c.Rcpt(to)` before `DATA`, and `net/smtp`
+  runs `validateLine` inside `Rcpt`, refusing any CR or LF - so a CRLF-bearing address
+  aborted the exchange at `RCPT TO` and never reached the body. That protection is
+  incidental, lives one call away in the standard library, and covers only this
+  transport. `buildRaw` now refuses an unparsed address (and an empty recipient list)
+  outright, returning `mailer.ErrInvalidRecipient` before anything is dialed; the
+  rejected value is kept out of the error, which is logged.
+
+  The public booking paths validate at intake rather than relying on the mailer: the REST
+  handler (`POST /v1/bookings`) answers 400 "email must be a valid email address", and
+  the shared core behind the conversational assistant's `book` tool and the MCP
+  `create_booking` tool checks the address before it persists anything. Both store the
+  parsed bare address, so a pasted `Bob <bob@example.com>` is recorded as
+  `bob@example.com` - a small deliberate behaviour change, matching what the hourly
+  throttle, the per-invitee cap and the `To:` header already assume they hold.
+
 ## [0.9.0] - 2026-09-10
 
 ### Added

internal/handler/booking_assistant.go

@@ -439,6 +439,10 @@ func assistantBookError(err error) string {
 		return "that time was just taken — please choose another slot"
 	case errors.Is(err, booking.ErrBookingLimitReached):
 		return "you already have the maximum number of upcoming bookings for this event"
+	case errors.Is(err, errInvalidBookerEmail):
+		// Worth its own case: the model can fix this by asking again, which the generic
+		// fallback below does not tell it.
+		return "that email address is not valid — please ask for it again"
 	default:
 		var ae *answerError
 		if errors.As(err, &ae) {

internal/handler/booking_email_test.go

@@ -0,0 +1,122 @@
+package handler_test
+
+import (
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/calnode/calnode/internal/handler"
+)
+
+// bookWithEmail posts a booking with the given raw email value and returns the response.
+func bookWithEmail(t *testing.T, h *handler.Handler, slug, hhmm, email string) *httptest.ResponseRecorder {
+	t.Helper()
+	body := fmt.Sprintf(`{"event_type_slug":%q,"start_at":"2026-06-15T%s:00Z","name":"Bob","email":%q}`,
+		slug, hhmm, email)
+	req := httptest.NewRequest(http.MethodPost, "/v1/bookings", strings.NewReader(body))
+	req.Header.Set("Content-Type", "application/json")
+	rec := httptest.NewRecorder()
+	h.CreateBooking(rec, req)
+	return rec
+}
+
+// organizerEmail reads back the stored organizer address for the workspace's single
+// booking. The public create response carries no attendees, so this goes through the
+// admin list, which does.
+func organizerEmail(t *testing.T, h *handler.Handler, apiKey string) string {
+	t.Helper()
+	req := authReq(http.MethodGet, "/v1/bookings", "", apiKey)
+	rec := httptest.NewRecorder()
+	h.RequireAuth(h.ListBookings)(rec, req)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("GET /v1/bookings: %d - %s", rec.Code, rec.Body.String())
+	}
+	var out struct {
+		Items []struct {
+			Attendees []struct {
+				Email string `json:"email"`
+			} `json:"attendees"`
+		} `json:"items"`
+	}
+	if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil {
+		t.Fatalf("decode bookings: %v", err)
+	}
+	if len(out.Items) != 1 || len(out.Items[0].Attendees) != 1 {
+		t.Fatalf("want exactly one booking with one organizer; got %+v", out.Items)
+	}
+	return out.Items[0].Attendees[0].Email
+}
+
+// TestCreateBooking_rejectsMalformedEmail: the booker's address ends up in an outbound
+// message's To: header, so it is checked at intake rather than at the mailer alone. The
+// emptiness check above it only proves the field was filled in.
+func TestCreateBooking_rejectsMalformedEmail(t *testing.T) {
+	h, key, _ := setupWorkspace(t)
+	slug, _ := seedEventTypeHTTP(t, h, key)
+
+	for _, tc := range []struct {
+		name  string
+		email string
+	}{
+		{"not an address", "not-an-address"},
+		{"header injection", "a@b.example\r\nBcc: x@y.example"},
+		{"two addresses", "a@b.example, c@d.example"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			rec := bookWithEmail(t, h, slug, "09:00", tc.email)
+			if rec.Code != http.StatusBadRequest {
+				t.Fatalf("got %d - %s; want 400", rec.Code, rec.Body.String())
+			}
+			var resp struct {
+				Error string `json:"error"`
+			}
+			if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil {
+				t.Fatalf("decode error body: %v", err)
+			}
+			if resp.Error != "email must be a valid email address" {
+				t.Errorf("error = %q; want the plain intake sentence", resp.Error)
+			}
+		})
+	}
+
+	// Nothing was stored on any of those attempts.
+	req := authReq(http.MethodGet, "/v1/bookings", "", key)
+	rec := httptest.NewRecorder()
+	h.RequireAuth(h.ListBookings)(rec, req)
+	if strings.Contains(rec.Body.String(), "b.example") {
+		t.Errorf("a rejected booking was persisted: %s", rec.Body.String())
+	}
+}
+
+// TestCreateBooking_normalisesDisplayNameEmail: a pasted `Bob <bob@example.com>` is
+// stored as the bare address. Deliberate — every later reader (the hourly throttle, the
+// per-invitee cap, the To: header) treats the stored value as a plain address.
+func TestCreateBooking_normalisesDisplayNameEmail(t *testing.T) {
+	h, key, _ := setupWorkspace(t)
+	slug, _ := seedEventTypeHTTP(t, h, key)
+
+	rec := bookWithEmail(t, h, slug, "09:00", " Bob <bob@example.com> ")
+	if rec.Code != http.StatusCreated {
+		t.Fatalf("got %d - %s; want 201", rec.Code, rec.Body.String())
+	}
+	if got := organizerEmail(t, h, key); got != "bob@example.com" {
+		t.Errorf("stored organizer email = %q; want %q", got, "bob@example.com")
+	}
+}
+
+// TestCreateBooking_ordinaryEmailUnchanged: normalisation must not rewrite the common case.
+func TestCreateBooking_ordinaryEmailUnchanged(t *testing.T) {
+	h, key, _ := setupWorkspace(t)
+	slug, _ := seedEventTypeHTTP(t, h, key)
+
+	rec := bookWithEmail(t, h, slug, "09:00", "Bob.Booker@example.com")
+	if rec.Code != http.StatusCreated {
+		t.Fatalf("got %d - %s; want 201", rec.Code, rec.Body.String())
+	}
+	if got := organizerEmail(t, h, key); got != "Bob.Booker@example.com" {
+		t.Errorf("stored organizer email = %q; want it unchanged", got)
+	}
+}

internal/handler/booking_handler.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/mail"
 	"net/url"
 	"strconv"
 	"strings"
@@ -24,6 +25,31 @@ import (
 	"github.com/calnode/calnode/internal/zoom"
 )
 
+// errInvalidBookerEmail is returned by normalizeBookerEmail for an address net/mail
+// cannot parse. Callers map it to their own protocol (400 on the REST path, a short
+// retry hint for the assistant).
+var errInvalidBookerEmail = errors.New("email must be a valid email address")
+
+// normalizeBookerEmail validates a booker-supplied address and returns the bare address
+// to store. Every booking path ends with that value in an outbound message's To: header,
+// so it must be something a header can hold: an unparsed address is the one raw input the
+// mailer used to pass through verbatim, and a CR/LF inside it would end the To: line.
+//
+// Returning a.Address rather than the input also normalises `"Bob" <bob@example.com>` to
+// `bob@example.com`, which is what the rest of the system already assumes it holds — the
+// per-invitee cap, the hourly throttle and the manage-link lookups all compare the stored
+// value as a plain address.
+//
+// mail.ParseAddress is the whole rule. No length or domain heuristics: they reject valid
+// addresses, and the property that matters here is parseability, not plausibility.
+func normalizeBookerEmail(raw string) (string, error) {
+	a, err := mail.ParseAddress(strings.TrimSpace(raw))
+	if err != nil {
+		return "", errInvalidBookerEmail
+	}
+	return a.Address, nil
+}
+
 // maxBookingsPerEmailPerHour caps how many bookings one email address can create
 // across the workspace in a rolling hour — a per-identity backstop to the per-IP
 // rate limit, for the openly-public booking page.
@@ -479,6 +505,16 @@ func (h *Handler) loadBookableEventType(ctx context.Context, slug string) (*book
 // booking.ErrDoubleBooked, booking.ErrBookingLimitReached, errNoHostAvailable) for callers to
 // map to their own protocol.
 func (h *Handler) createBookingForSlug(ctx context.Context, slug string, startAt time.Time, organizer booking.Attendee, rawAnswers []booking.Answer) (*booking.Booking, error) {
+	// Both callers take the address from something they do not control — an LLM's
+	// extraction from booker chat, or an MCP client's tool arguments — so it is checked
+	// here, once, rather than in each of them. (The REST handler does not come through
+	// this core; it normalises at its own intake, before its throttle reads the value.)
+	email, err := normalizeBookerEmail(organizer.Email)
+	if err != nil {
+		return nil, err
+	}
+	organizer.Email = email
+
 	et, err := h.loadBookableEventType(ctx, slug)
 	if err != nil {
 		return nil, err
@@ -722,6 +758,15 @@ func (h *Handler) CreateBooking(w http.ResponseWriter, r *http.Request) {
 		h.writeError(w, http.StatusBadRequest, "event_type_slug, start_at, name, and email are required")
 		return
 	}
+	// Normalise before anything reads req.Email — the hourly throttle, the Stripe
+	// Checkout session, the attendee row and the confirmation email all take it from
+	// here, and they must all see the same bare address.
+	email, err := normalizeBookerEmail(req.Email)
+	if err != nil {
+		h.writeError(w, http.StatusBadRequest, err.Error())
+		return
+	}
+	req.Email = email
 	if req.Timezone == "" {
 		req.Timezone = "UTC"
 	}

internal/mailer/ics_test.go

@@ -113,7 +113,7 @@ func TestBuildRaw_multipartWithICS(t *testing.T) {
 	s := smtpForTest()
 	d := testBookingData()
 	d.AttachICS = true
-	raw := string(s.buildRaw(Message{
+	raw := string(mustBuildRaw(t, s, Message{
 		To:          []string{"bob@example.com"},
 		Subject:     "Booking confirmed",
 		Text:        "body text",

internal/mailer/mailer_test.go

@@ -2,6 +2,7 @@ package mailer
 
 import (
 	"context"
+	"errors"
 	"net/url"
 	"strings"
 	"sync"
@@ -244,14 +245,25 @@ func smtpForTest() *SMTP {
 	return &SMTP{from: "noreply@example.com", fromName: "Calnode"}
 }
 
+// mustBuildRaw builds msg and fails the test if the recipients are refused — for the
+// cases below, which are about header encoding rather than recipient validation.
+func mustBuildRaw(t *testing.T, s *SMTP, msg Message) []byte {
+	t.Helper()
+	raw, err := s.buildRaw(msg)
+	if err != nil {
+		t.Fatalf("buildRaw: %v", err)
+	}
+	return raw
+}
+
 func TestBuildRaw_subjectInjectionPrevented(t *testing.T) {
 	s := smtpForTest()
 	msg := Message{
 		To:      []string{"user@example.com"},
 		Subject: "Evil\r\nBcc: attacker@evil.com",
 		Text:    "body",
 	}
-	raw := string(s.buildRaw(msg))
+	raw := string(mustBuildRaw(t, s, msg))
 
 	if strings.Contains(raw, "Bcc: attacker@evil.com") {
 		t.Error("header injection: injected Bcc header found in raw message")
@@ -273,7 +285,7 @@ func TestBuildRaw_nonASCIISubjectEncoded(t *testing.T) {
 		Subject: "Réunion d'équipe",
 		Text:    "body",
 	}
-	raw := string(s.buildRaw(msg))
+	raw := string(mustBuildRaw(t, s, msg))
 
 	// The raw Subject: line must not contain bare UTF-8 bytes (> 0x7E).
 	for _, line := range strings.Split(raw, "\r\n") {
@@ -298,7 +310,7 @@ func TestBuildRaw_pureASCIISubjectNotEncoded(t *testing.T) {
 		Subject: "Booking confirmed: 30-min call",
 		Text:    "body",
 	}
-	raw := string(s.buildRaw(msg))
+	raw := string(mustBuildRaw(t, s, msg))
 
 	for _, line := range strings.Split(raw, "\r\n") {
 		if strings.HasPrefix(line, "Subject:") {
@@ -313,7 +325,7 @@ func TestBuildRaw_pureASCIISubjectNotEncoded(t *testing.T) {
 func TestBuildRaw_fromNameFormatted(t *testing.T) {
 	s := smtpForTest()
 	msg := Message{To: []string{"x@example.com"}, Subject: "Hi", Text: "body"}
-	raw := string(s.buildRaw(msg))
+	raw := string(mustBuildRaw(t, s, msg))
 
 	if !strings.Contains(raw, "Calnode") {
 		t.Error("From: header missing sender name")
@@ -322,3 +334,60 @@ func TestBuildRaw_fromNameFormatted(t *testing.T) {
 		t.Error("From: header missing sender address")
 	}
 }
+
+// TestBuildRaw_recipientAddresses covers the To: header, which is the one header field
+// assembled from caller-supplied input without an encoder in front of it. Subject and the
+// From display name go through mime.QEncoding and attachment filenames through %q; a
+// recipient that mail.ParseAddress refuses used to be appended verbatim, so a CR/LF inside
+// it would have ended the To: line and started a header of the sender's choosing.
+func TestBuildRaw_recipientAddresses(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		to   []string
+		// want is the expected To: header line. Empty means buildRaw must refuse.
+		want string
+	}{
+		{"plain address", []string{"bob@example.com"}, "To: <bob@example.com>"},
+		{"display name is normalised into the header", []string{"Bob <bob@example.com>"}, `To: "Bob" <bob@example.com>`},
+		{"two recipients", []string{"bob@example.com", "eve@example.com"}, "To: <bob@example.com>, <eve@example.com>"},
+		{"CRLF in the address", []string{"a@b.example\r\nBcc: attacker@example.com"}, ""},
+		{"not an address at all", []string{"not-an-address"}, ""},
+		{"no recipient", nil, ""},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			raw, err := smtpForTest().buildRaw(Message{To: tc.to, Subject: "Hi", Text: "body"})
+
+			if tc.want == "" {
+				if err == nil {
+					t.Fatalf("buildRaw accepted %q; want a refusal", tc.to)
+				}
+				if !errors.Is(err, ErrInvalidRecipient) {
+					t.Errorf("error = %v; want it to wrap ErrInvalidRecipient", err)
+				}
+				if raw != nil {
+					t.Errorf("buildRaw returned %d bytes alongside its error; want none", len(raw))
+				}
+				// The offending value is attacker-controlled and this error is logged,
+				// so it must not carry the raw bytes back out.
+				if strings.Contains(err.Error(), "attacker@example.com") {
+					t.Errorf("error %q quotes the rejected address back", err)
+				}
+				return
+			}
+
+			if err != nil {
+				t.Fatalf("buildRaw(%q): %v", tc.to, err)
+			}
+			var got string
+			for _, line := range strings.Split(string(raw), "\r\n") {
+				if strings.HasPrefix(line, "To:") {
+					got = line
+					break
+				}
+			}
+			if got != tc.want {
+				t.Errorf("To: header = %q; want %q", got, tc.want)
+			}
+		})
+	}
+}