FMG-3: fix SMTP authentication bypass, remote DoS, and silent delivery failures #9
2026-09-07
fix: guard session transaction state and require SMTP credentials (FMG-3)
All checks were successful
Addresses the two merge-blocking findings from the review of PR #9. Session.Reset wrote the transaction's recipients on the connection goroutine while go-smtp runs Data on its own goroutine for a BDAT transfer, and RSET is not blocked during one — so a reset could land in the middle of a chunked transfer and race the forward. A mutex now guards the recipients and the authentication flag across Rcpt, Data and Reset, and Data snapshots the recipients up front rather than reading the slice while forwarding. The regression drives a raw BDAT transfer with an RSET pipelined behind the chunk that completes the headers; under -race it reports the race reliably without the guard. SetDefaults invented Username = "username" and Password = "password" when the config omitted them, with only a warning, so a config naming nothing but a port and its recipients relayed to anyone who tried the obvious guess — for exactly the deployments that had configured the least, and while the README promised an authentication that was never configured. Validate now rejects unset credentials the way it already rejects an unusable target, and the invented defaults are gone. The fixtures of the neighbouring config tests carry credentials so each still fails for the reason it names. Also corrects a 550/451 inversion introduced by the previous commit and flagged in the same review: a recipient with no usable targets is a configuration fault, so it now answers 550 5.3.5 rather than asking the sender to retry something no redelivery can fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
fix: require authentication, survive malformed headers, report failed delivery (FMG-3)
All checks were successful
The SMTP credential check joined its two comparisons with `&&`, so it only rejected a client that got both halves wrong: a correct username with any password authenticated, as did a correct password with any username. Both comparisons now have to pass, and both run before either is acted on so the timing does not distinguish a wrong username from a wrong password. They compare SHA-256 digests through subtle.ConstantTimeCompare, which keeps the length of the configured credential out of the timing too. Fixing the comparison alone does not close the hole. go-smtp advertises AUTH but leaves enforcement to the backend, so a client could skip AUTH entirely and still have its message forwarded. The session now tracks authentication and refuses MAIL, RCPT and DATA without it. A malformed Content-Type reached log.Fatalf, which exits the process. One message with an attacker-controlled header terminated the daemon and took every other in-flight message with it. Body() now returns the parse error like the rest of the function does. Data logged forwarding errors and returned 250, so a client whose notification reached no target at all considered the message delivered and never retried. It now answers 451 when no target accepted the message and 550 when the message cannot be parsed, which no retry could fix. A partial failure is still accepted, since a retry would redeliver to every target and duplicate the notification on the ones that already have it. Session.Reset was empty while go-smtp calls it after every message, so recipients accumulated across a connection and the second message was also forwarded to the first one's targets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>