diff --git a/CHANGELOG.md b/CHANGELOG.md index f45a54c..375ea1c 100644 --- a/CHANGELOG.md +++ b/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 ` 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 diff --git a/internal/handler/booking_assistant.go b/internal/handler/booking_assistant.go index eaa2110..4cbe512 100644 --- a/internal/handler/booking_assistant.go +++ b/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) { diff --git a/internal/handler/booking_email_test.go b/internal/handler/booking_email_test.go new file mode 100644 index 0000000..43f8b25 --- /dev/null +++ b/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 ` 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 ") + 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) + } +} diff --git a/internal/handler/booking_handler.go b/internal/handler/booking_handler.go index 6b7abe3..fc7a5f9 100644 --- a/internal/handler/booking_handler.go +++ b/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" ` 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" } diff --git a/internal/mailer/ics_test.go b/internal/mailer/ics_test.go index bcb3a21..c0faa02 100644 --- a/internal/mailer/ics_test.go +++ b/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", diff --git a/internal/mailer/mailer_test.go b/internal/mailer/mailer_test.go index 67c5d46..1c1a946 100644 --- a/internal/mailer/mailer_test.go +++ b/internal/mailer/mailer_test.go @@ -2,6 +2,7 @@ package mailer import ( "context" + "errors" "net/url" "strings" "sync" @@ -244,6 +245,17 @@ 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{ @@ -251,7 +263,7 @@ func TestBuildRaw_subjectInjectionPrevented(t *testing.T) { 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: "}, + {"display name is normalised into the header", []string{"Bob "}, `To: "Bob" `}, + {"two recipients", []string{"bob@example.com", "eve@example.com"}, "To: , "}, + {"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) + } + }) + } +} diff --git a/internal/mailer/smtp.go b/internal/mailer/smtp.go index 26a478b..5fb2d94 100644 --- a/internal/mailer/smtp.go +++ b/internal/mailer/smtp.go @@ -82,7 +82,11 @@ func newDialers(deadline time.Time, host string) (net.Dialer, tls.Dialer) { func (s *SMTP) Send(ctx context.Context, msg Message) error { addr := net.JoinHostPort(s.host, s.port) - raw := s.buildRaw(msg) + // Built before anything is dialed, so an unusable recipient costs no connection. + raw, err := s.buildRaw(msg) + if err != nil { + return err + } // Bounds the whole conversation, not just the dial (see defaultSMTPTimeout). // Whichever is EARLIER of "ctx's own deadline" and "now + the default" wins, so a @@ -174,7 +178,13 @@ func (s *SMTP) Send(ctx context.Context, msg Message) error { return nil } -func (s *SMTP) buildRaw(msg Message) []byte { +// ErrInvalidRecipient marks a recipient address that net/mail cannot parse. buildRaw +// refuses to write such an address into the To: header, so Send returns this instead of +// sending anything. The offending value is deliberately left out of the message: it is +// caller-supplied, may carry CR/LF, and this error is logged. +var ErrInvalidRecipient = errors.New("mailer: invalid recipient address") + +func (s *SMTP) buildRaw(msg Message) ([]byte, error) { from := mail.Address{Name: s.fromName, Address: s.from} // mime.QEncoding.Encode returns the string unchanged when it is pure ASCII @@ -184,16 +194,22 @@ func (s *SMTP) buildRaw(msg Message) []byte { // and satisfying RFC 2047 at the same time. subject := mime.QEncoding.Encode("utf-8", msg.Subject) - // Validate and normalise To addresses so the To: header line is properly - // quoted. Delivery uses c.Rcpt() (separate SMTP command) so a To: header - // formatting error cannot redirect mail. + // Validate and normalise To addresses so the To: header line is properly quoted. + // An address that does not parse is REFUSED rather than passed through: writing one + // verbatim is what would let a CR/LF inside it close the To: line and start a header + // of the attacker's choosing. net/smtp's Rcpt happens to reject CR/LF too, but that + // is one call away in the standard library and protects only this transport, so the + // guarantee is made here, where the header is actually assembled. + if len(msg.To) == 0 { + return nil, fmt.Errorf("%w: message has no recipient", ErrInvalidRecipient) + } toFormatted := make([]string, 0, len(msg.To)) - for _, addr := range msg.To { - if a, err := mail.ParseAddress(addr); err == nil { - toFormatted = append(toFormatted, a.String()) - } else { - toFormatted = append(toFormatted, addr) + for i, addr := range msg.To { + a, err := mail.ParseAddress(addr) + if err != nil { + return nil, fmt.Errorf("%w: recipient %d", ErrInvalidRecipient, i) } + toFormatted = append(toFormatted, a.String()) } var buf bytes.Buffer @@ -209,7 +225,7 @@ func (s *SMTP) buildRaw(msg Message) []byte { if !hasHTML && !hasAtt { buf.WriteString("Content-Type: text/plain; charset=utf-8\r\n\r\n") buf.WriteString(msg.Text) - return buf.Bytes() + return buf.Bytes(), nil } // writeBody emits the message body at the current MIME level: either a single @@ -242,7 +258,7 @@ func (s *SMTP) buildRaw(msg Message) []byte { // No attachments: the body is the whole message. if !hasAtt { writeBody(&buf) - return buf.Bytes() + return buf.Bytes(), nil } // multipart/mixed: the body part (text or multipart/alternative) followed by @@ -262,7 +278,7 @@ func (s *SMTP) buildRaw(msg Message) []byte { buf.WriteString("\r\n") } fmt.Fprintf(&buf, "--%s--\r\n", mixed) - return buf.Bytes() + return buf.Bytes(), nil } // base64Wrap base64-encodes b and wraps it at 76 characters per line (RFC 2045). diff --git a/internal/mailer/smtp_test.go b/internal/mailer/smtp_test.go index f32ebbb..9b8fbf8 100644 --- a/internal/mailer/smtp_test.go +++ b/internal/mailer/smtp_test.go @@ -2,7 +2,9 @@ package mailer import ( "context" + "errors" "net" + "sync/atomic" "testing" "time" ) @@ -95,3 +97,49 @@ func TestSMTP_Send_appliesDefaultTimeoutWithNoCtxDeadline(t *testing.T) { t.Errorf("Send took %v to fail; want it bounded by defaultSMTPTimeout (300ms), not hanging", elapsed) } } + +// TestSMTP_Send_invalidRecipientNeverDials is the half a buildRaw unit test cannot show: +// the refusal has to happen before a connection is opened, not after. +// +// The address below is stopped today by net/smtp too — Client.Rcpt runs validateLine and +// rejects any CR or LF, so the exchange would abort at RCPT TO before DATA. That is +// incidental protection living one call away in the standard library, it covers only this +// transport, and it costs a dial, an EHLO and an AUTH first. The listener here accepts and +// counts connections, so a regression that moves the check back after the dial fails loudly. +func TestSMTP_Send_invalidRecipientNeverDials(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { ln.Close() }) //nolint:errcheck + + var accepted atomic.Int32 + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + accepted.Add(1) + conn.Close() //nolint:errcheck + } + }() + + host, port, err := net.SplitHostPort(ln.Addr().String()) + if err != nil { + t.Fatalf("split addr: %v", err) + } + s := NewSMTP(host, port, "", "", false, false, "from@test.local", "Test") + + err = s.Send(context.Background(), Message{ + To: []string{"a@b.example\r\nBcc: attacker@example.com"}, + Subject: "test", + Text: "body", + }) + if !errors.Is(err, ErrInvalidRecipient) { + t.Fatalf("Send error = %v; want ErrInvalidRecipient", err) + } + if n := accepted.Load(); n != 0 { + t.Errorf("Send opened %d connection(s) before refusing the recipient; want 0", n) + } +}