Skip to content

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

Open
distronode-com wants to merge 1 commit into
Calnode:mainfrom
distronode-com:up/booker-email
Open

mailer: a booker's address is validated at intake, and never written into a header unparsed#45
distronode-com wants to merge 1 commit into
Calnode:mainfrom
distronode-com:up/booker-email

Conversation

@distronode-com

Copy link
Copy Markdown
Contributor

CodeQL flags internal/mailer/smtp.go's wc.Write(raw) as email-content injection. The weak point it is pointing at is in buildRaw: the To: header is assembled from msg.To, and an address that mail.ParseAddress rejects was appended to the header verbatim. Every other header input is already safe (the subject and the From display name are Q-encoded, attachment filenames are %q-quoted, bodies sit below the blank line inside random-boundary parts); the recipient list was the one raw path, and on the public booking path it is the booker's own input with nothing but an emptiness check in front of it.

It is not exploitable today, and this PR does not claim otherwise: Send calls Rcpt before DATA, and net/smtp's validateLine refuses any CR or LF there, so a CRLF-bearing address aborts at RCPT TO. That protection is incidental, lives in the standard library one call away, and covers only this transport. This change makes the guarantee local, in two layers:

  1. Intake. normalizeBookerEmail (trim, mail.ParseAddress, keep a.Address) runs at both places a booker's address enters: CreateBooking for POST /v1/bookings, before the hourly throttle and the Stripe session read it, answering 400 email must be a valid email address; and the top of createBookingForSlug, which the assistant's book tool and MCP create_booking share, so an LLM-extracted address gets the same rule and the assistant gets a retry hint instead of the generic failure. Small deliberate behaviour change: "Bob" <bob@example.com> is stored as bob@example.com, which is what the per-invitee cap, the throttle and the manage-link lookups already assume they hold.
  2. buildRaw returns ([]byte, error) and refuses an unparseable or empty recipient list with ErrInvalidRecipient (index, never the value, since the error is logged). Send returns it before anything is dialed.

Tests cover the plain, display-name, two-recipient, CRLF, unparseable and empty cases for buildRaw; a Send that must never dial (listener counting accepts); and the three intake shapes on POST /v1/bookings, including that nothing is persisted on a refusal. The existing subject-encoding test is untouched. CHANGELOG.md gets a line under Unreleased → Security.

Noted and left alone: the invite path (admin-authenticated) does not ParseAddress either; the Resend transport accepts unparsed recipients but posts JSON, so there is no header to inject into; the 400 sentence is English while the throttle errors beside it are translated, and adding a key touches all eight locale files.

🤖 Generated with Claude Code

…into a header unparsed

The To: header is the one header field assembled from caller-supplied input
with nothing encoding it. buildRaw parsed each recipient with mail.ParseAddress
and, when that failed, appended the raw string anyway, so a CR/LF inside an
address would have closed the To: line and started a header of the sender's
choosing. Every other field was already safe: Subject and the From display name
go through mime.QEncoding, which renders CR and LF as =0D and =0A; attachment
filenames are written with %q; bodies sit below the blank line inside
random-boundary MIME parts.

The value reaching it is public and unauthenticated. POST /v1/bookings takes
`email` straight from the request body, checks only that it is non-empty, and
carries it to mailer.Message.To through dispatchBookingConfirmation. The
conversational booking assistant's `book` tool is the same hole with a looser
source: the address is whatever the model extracted from booker chat. Neither
path called mail.ParseAddress anywhere.

It is not exploitable as it stands, and the reason is worth stating precisely
rather than relying on. Send issues c.Rcpt(to) for every recipient before it
opens DATA, and net/smtp runs validateLine inside Rcpt, which refuses any
string containing CR or LF. A CRLF-bearing address therefore aborts the
exchange at RCPT TO and never reaches the body. That is incidental protection:
it lives one call away in the standard library, it is not a property of this
file, and it does nothing for a future transport that reuses buildRaw to
assemble a message. The guarantee should be local to where the header is built.

Two layers, because the two answer different questions.

Intake decides what is stored. normalizeBookerEmail trims, parses with
mail.ParseAddress, and returns a.Address — the bare address, not the input.
CreateBooking calls it immediately after the emptiness check, before anything
reads req.Email, and answers 400 "email must be a valid email address" in the
same {"error": …} shape as its neighbours. createBookingForSlug — the shared
core behind the assistant's `book` tool and the MCP create_booking tool — calls
it before it persists anything; the REST handler does not route through that
core, which is why both are touched. mail.ParseAddress is the whole rule: no
length or domain heuristics, since the property that matters is parseability,
not plausibility.

The mailer decides what can be written. buildRaw returns ([]byte, error) and
refuses an address it cannot parse, and an empty recipient list, with
ErrInvalidRecipient. Send already built the message before dialing, so the
refusal costs no connection — a test drives Send at a listener that counts
accepts and asserts zero. The rejected value is deliberately kept out of the
error, which is logged and may carry CR/LF; the error names the index instead.

Storing a.Address rather than the input is a small deliberate behaviour change:
a pasted `Bob <bob@example.com>` is now recorded as bob@example.com. That is
what the rest of the system already assumes it holds — the hourly per-email
throttle, the per-invitee active-booking cap and the To: header all compare or
emit the stored value as a plain address.

resend.go, the HTTPS transport, posts msg.To as JSON and so has no header to
inject into. It does accept unparsed recipients and is left alone; its callers
now come through the same validated intake.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes One security hardening commit: booker email is validated at intake, and SMTP no longer writes an unparsed recipient into To:.

  • Intake normalisationnormalizeBookerEmail (ParseAddress → bare a.Address) on REST CreateBooking (before throttle/Stripe) and on createBookingForSlug (assistant + MCP); assistant gets a dedicated retry hint.
  • SMTP buildRaw — returns ([]byte, error); empty/unparseable recipients yield ErrInvalidRecipient (index only) before dial; existing subject/From encoding tests updated via mustBuildRaw.
  • CoveragebuildRaw recipient matrix, Send-never-dials listener test, REST reject/normalise/ordinary-email cases; CHANGELOG Security note.

Pullfrog  | View workflow run | Using Grok𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant