From da8e8e478c319c8bf8955cece55289444d04a80b Mon Sep 17 00:00:00 2001 From: Full-Stack Developer Date: Mon, 7 Sep 2026 19:41:17 +0000 Subject: [PATCH 1/2] fix: require authentication, survive malformed headers, report failed delivery (FMG-3) 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) Co-authored-by: multica-agent --- README.md | 12 +++ backend.go | 117 +++++++++++++++++++-- backend_test.go | 271 ++++++++++++++++++++++++++++++++++++++++++++++++ email.go | 9 +- email_test.go | 90 ++++++++++++++++ 5 files changed, 487 insertions(+), 12 deletions(-) create mode 100644 email_test.go diff --git a/README.md b/README.md index 3666932..8c060b2 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,18 @@ recipients and no catch-all, a recipient without addresses or usable targets, or a mistyped table name such as `[[Recipient]]`, which TOML would otherwise accept silently. +Clients must authenticate with the configured `Username` and `Password` before +starting a mail transaction; an unauthenticated client is refused rather than +relayed. A client that used to send without issuing `AUTH` has to be configured +with the credentials above. + +The reply the sending client gets reflects what happened to the notification: +`250` once at least one target has accepted it, `451` when none did so the +sender retries, and `550` for a message that cannot be parsed at all — a +malformed `Content-Type`, for instance — which no retry could fix. A partial +failure is logged but still accepted, since a retry would redeliver to every +target and duplicate the notification on the ones that already have it. + ### From releases - Grab the latest release from the [releases page](https://git.nakama.town/fmartingr/smtp2shoutrrr/releases) diff --git a/backend.go b/backend.go index 9189d9b..30bdeea 100644 --- a/backend.go +++ b/backend.go @@ -1,6 +1,9 @@ package smtp2shoutrrr import ( + "crypto/sha256" + "crypto/subtle" + "errors" "fmt" "io" "log/slog" @@ -14,6 +17,26 @@ import ( "github.com/emersion/go-smtp" ) +var ( + // errUnreadableMessage rejects a message for good. Both causes — a + // message that is not parseable as mail, and a Content-Type this server + // cannot make sense of — fail identically on every redelivery. + errUnreadableMessage = &smtp.SMTPError{ + Code: 550, + EnhancedCode: smtp.EnhancedCode{5, 6, 0}, + Message: "Message could not be parsed", + } + + // errForwardingFailed asks the sender to try again later. The 250 this + // replaces told the client a message had been delivered when no target + // had received it, so the client dropped a notification nobody ever saw. + errForwardingFailed = &smtp.SMTPError{ + Code: 451, + EnhancedCode: smtp.EnhancedCode{4, 3, 0}, + Message: "Failed to forward message to notification targets", + } +) + type Backend struct { config *Config } @@ -23,9 +46,8 @@ func (bkd *Backend) sendNotifications(recipient ConfigRecipient, email ReceivedE targetURLs := recipient.GetTargetURLs() if len(targetURLs) == 0 { - slog.Warn("no valid targets provided for recipient", - slog.String("recipient", strings.Join(recipient.Addresses, ","))) - return nil + return fmt.Errorf("recipient %s has no usable targets", + strings.Join(recipient.Addresses, ",")) } // Prepare email body once (reuse for all targets) @@ -73,10 +95,18 @@ func (bkd *Backend) sendNotifications(recipient ConfigRecipient, email ReceivedE } } - // Return aggregated error if any failed + // Only a total failure is reported to the sender. A retry redelivers to + // every target, so pushing back on a partial one would duplicate the + // notification on the targets that already accepted it. + if len(errs) == len(targetURLs) { + return fmt.Errorf("no target accepted the notification: %w", errors.Join(errs...)) + } + if len(errs) > 0 { - return fmt.Errorf("failed to send to %d/%d targets: %v", - len(errs), len(targetURLs), errs) + slog.Warn("notification reached only some targets", + slog.Int("failed", len(errs)), + slog.Int("targets", len(targetURLs)), + slog.String("recipient", strings.Join(recipient.Addresses, ","))) } return nil @@ -123,6 +153,11 @@ func (bkd *Backend) forwardEmail(email ReceivedEmail) error { type Session struct { addresses []string + // authenticated gates the mail transaction. go-smtp advertises AUTH but + // never enforces it — that is the backend's job — so without this the + // server relays for anyone who can open a connection. + authenticated bool + config *Config forwarderFunc func(ReceivedEmail) error @@ -133,30 +168,79 @@ func (s *Session) AuthMechanisms() []string { } func (s *Session) Auth(mech string) (sasl.Server, error) { + if mech != sasl.Plain { + return nil, smtp.ErrAuthUnknownMechanism + } + return sasl.NewPlainServer(func(identity, username, password string) error { - if username != s.config.Username && password != s.config.Password { - return fmt.Errorf("invalid credentials") + // PLAIN carries an authorization identity distinct from the + // authenticating one (RFC 4616). There is nobody to act on behalf of + // here, so only the empty and self-referential forms are accepted. + if identity != "" && identity != username { + return smtp.ErrAuthFailed } + + // Both comparisons run before either is acted on: short-circuiting on + // the username would time-distinguish a wrong username from a right + // one with a wrong password, handing an attacker the username. + usernameMatches := credentialMatches(username, s.config.Username) + passwordMatches := credentialMatches(password, s.config.Password) + + if !usernameMatches || !passwordMatches { + // Logged without the attempted values: a misconfigured client + // sends its password in the username field often enough that + // recording them turns the log into a credential store. + slog.Warn("rejected authentication attempt") + return smtp.ErrAuthFailed + } + + s.authenticated = true + return nil }), nil } +// credentialMatches compares a supplied credential against the configured one +// in time independent of how much of it is right. Both sides are hashed first +// because subtle.ConstantTimeCompare returns early when the lengths differ, +// which would still leak the length of the configured credential. +func credentialMatches(supplied, configured string) bool { + suppliedSum := sha256.Sum256([]byte(supplied)) + configuredSum := sha256.Sum256([]byte(configured)) + + return subtle.ConstantTimeCompare(suppliedSum[:], configuredSum[:]) == 1 +} + func (s *Session) Mail(from string, opts *smtp.MailOptions) error { + if !s.authenticated { + return smtp.ErrAuthRequired + } + slog.Debug("Mail from", slog.String("from", from)) + return nil } func (s *Session) Rcpt(to string, opts *smtp.RcptOptions) error { + if !s.authenticated { + return smtp.ErrAuthRequired + } + slog.Debug("Rcpt to", slog.String("to", to)) s.addresses = append(s.addresses, to) + return nil } func (s *Session) Data(r io.Reader) error { + if !s.authenticated { + return smtp.ErrAuthRequired + } + msg, err := mail.ReadMessage(r) if err != nil { slog.Error("Error reading data", slog.String("err", err.Error())) - return fmt.Errorf("error reading data: %w", err) + return errUnreadableMessage } slog.Info("Received email", slog.String("destination", strings.Join(s.addresses, ","))) @@ -166,12 +250,25 @@ func (s *Session) Data(r io.Reader) error { Msg: msg, }); err != nil { slog.Error("Error forwarding email", slog.String("err", err.Error())) + + if errors.Is(err, errMalformedMessage) { + return errUnreadableMessage + } + + return errForwardingFailed } return nil } -func (s *Session) Reset() {} +// Reset ends the current mail transaction. go-smtp calls it after every +// message, so the recipients have to go: leaving them would forward the next +// message on the connection to the previous one's targets as well. +// Authentication deliberately survives, as RFC 5321 scopes RSET to the +// transaction. +func (s *Session) Reset() { + s.addresses = nil +} func (s *Session) Logout() error { return nil diff --git a/backend_test.go b/backend_test.go index 330519c..d68fae9 100644 --- a/backend_test.go +++ b/backend_test.go @@ -282,3 +282,274 @@ func TestInvalidTargetURLs(t *testing.T) { // Verify only valid URLs received notifications (2 out of 4) require.Len(t, notifications, 2) } + +// Regression: the credential check joined the two comparisons with &&, so it +// only rejected a client that got both halves wrong. A correct username with +// any password — or a correct password with any username — authenticated, and +// anyone who guessed either half could relay to every configured target. +func TestAuthenticationRejectsPartiallyCorrectCredentials(t *testing.T) { + notifications := 0 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.ReadAll(r.Body) + notifications++ + w.WriteHeader(http.StatusOK) + })) + defer mockServer.Close() + + config := &Config{ + Port: 0, + Username: "testuser", + Password: "testpass", + Recipients: []ConfigRecipient{ + { + Addresses: []string{"test@example.com"}, + Targets: []string{"generic+" + mockServer.URL + "/?template=json"}, + }, + }, + } + + addr, _, _ := startServer(t, NewSMTPServer(config)) + + for name, credentials := range map[string][2]string{ + "correct username, wrong password": {"testuser", "wrong"}, + "wrong username, correct password": {"wrong", "testpass"}, + "both wrong": {"wrong", "wrong"}, + "empty credentials": {"", ""}, + } { + t.Run(name, func(t *testing.T) { + err := smtp.SendMail( + addr, + smtp.PlainAuth("", credentials[0], credentials[1], "localhost"), + "sender@example.com", + []string{"test@example.com"}, + []byte("Subject: Should Not Arrive\r\n\r\nbody"), + ) + require.Error(t, err) + require.Contains(t, err.Error(), "535", "authentication should be refused permanently") + }) + } + + require.Zero(t, notifications, "a rejected client must not get anything forwarded") +} + +// Regression: go-smtp advertises AUTH but leaves enforcement to the backend, +// so before the session tracked it a client could skip AUTH entirely and still +// have its message forwarded. +func TestUnauthenticatedClientCannotSendMail(t *testing.T) { + notifications := 0 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.ReadAll(r.Body) + notifications++ + w.WriteHeader(http.StatusOK) + })) + defer mockServer.Close() + + config := &Config{ + Port: 0, + Username: "testuser", + Password: "testpass", + Recipients: []ConfigRecipient{ + { + Addresses: []string{"test@example.com"}, + Targets: []string{"generic+" + mockServer.URL + "/?template=json"}, + }, + }, + } + + addr, _, _ := startServer(t, NewSMTPServer(config)) + + client, err := smtp.Dial(addr) + require.NoError(t, err) + t.Cleanup(func() { _ = client.Close() }) + + require.NoError(t, client.Hello("localhost")) + + err = client.Mail("sender@example.com") + require.Error(t, err, "the transaction must not start without authentication") + require.Contains(t, err.Error(), "502") + + require.Zero(t, notifications) +} + +// Regression: a malformed Content-Type reached log.Fatalf, so one message with +// an attacker-controlled header terminated the daemon and took every other +// in-flight message with it. +func TestMalformedContentTypeIsRejectedAndServerKeepsServing(t *testing.T) { + notifications := make([]string, 0) + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + notifications = append(notifications, string(body)) + w.WriteHeader(http.StatusOK) + })) + defer mockServer.Close() + + config := &Config{ + Port: 0, + Username: "testuser", + Password: "testpass", + Recipients: []ConfigRecipient{ + { + Addresses: []string{"test@example.com"}, + Targets: []string{"generic+" + mockServer.URL + "/?template=json"}, + }, + }, + } + + addr, _, _ := startServer(t, NewSMTPServer(config)) + auth := smtp.PlainAuth("", config.Username, config.Password, "localhost") + + err := smtp.SendMail( + addr, + auth, + "sender@example.com", + []string{"test@example.com"}, + []byte("Subject: Malformed\r\nContent-Type: text/plain; charset\r\n\r\nbody"), + ) + require.Error(t, err) + require.Contains(t, err.Error(), "550", "an unparseable message should be refused permanently") + + err = smtp.SendMail( + addr, + auth, + "sender@example.com", + []string{"test@example.com"}, + []byte("Subject: Well Formed\r\n\r\nthe server is still up"), + ) + require.NoError(t, err) + + require.Len(t, notifications, 1) + require.Contains(t, notifications[0], "the server is still up") +} + +// Regression: Data logged the forwarding error and returned 250, so a client +// whose notification reached no target at all considered the message delivered, +// dropped it and never retried. +func TestTotalForwardingFailureIsReportedToSender(t *testing.T) { + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusInternalServerError) + })) + defer mockServer.Close() + + config := &Config{ + Port: 0, + Username: "testuser", + Password: "testpass", + Recipients: []ConfigRecipient{ + { + Addresses: []string{"test@example.com"}, + Targets: []string{ + "generic+" + mockServer.URL + "/one?template=json", + "generic+" + mockServer.URL + "/two?template=json", + }, + }, + }, + } + + addr, _, _ := startServer(t, NewSMTPServer(config)) + + err := smtp.SendMail( + addr, + smtp.PlainAuth("", config.Username, config.Password, "localhost"), + "sender@example.com", + []string{"test@example.com"}, + []byte("Subject: Nobody Received This\r\n\r\nbody"), + ) + require.Error(t, err) + require.Contains(t, err.Error(), "451", "the sender should be asked to retry") +} + +// A partial failure stays a 250: the sender would redeliver to every target, +// duplicating the notification on the ones that already accepted it. +func TestPartialForwardingFailureIsAcceptedFromSender(t *testing.T) { + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.ReadAll(r.Body) + if r.URL.Path == "/fail" { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + })) + defer mockServer.Close() + + config := &Config{ + Port: 0, + Username: "testuser", + Password: "testpass", + Recipients: []ConfigRecipient{ + { + Addresses: []string{"test@example.com"}, + Targets: []string{ + "generic+" + mockServer.URL + "/fail?template=json", + "generic+" + mockServer.URL + "/ok?template=json", + }, + }, + }, + } + + addr, _, _ := startServer(t, NewSMTPServer(config)) + + require.NoError(t, smtp.SendMail( + addr, + smtp.PlainAuth("", config.Username, config.Password, "localhost"), + "sender@example.com", + []string{"test@example.com"}, + []byte("Subject: Reached One Target\r\n\r\nbody"), + )) +} + +// Regression: Session.Reset was empty, so recipients accumulated across the +// messages of a single connection and the second one was also forwarded to the +// first one's targets. +func TestRecipientsDoNotLeakBetweenMessagesOnOneConnection(t *testing.T) { + received := make(map[string][]string) + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + received[r.URL.Path] = append(received[r.URL.Path], string(body)) + w.WriteHeader(http.StatusOK) + })) + defer mockServer.Close() + + config := &Config{ + Port: 0, + Username: "testuser", + Password: "testpass", + Recipients: []ConfigRecipient{ + { + Addresses: []string{"first@example.com"}, + Targets: []string{"generic+" + mockServer.URL + "/first?template=json"}, + }, + { + Addresses: []string{"second@example.com"}, + Targets: []string{"generic+" + mockServer.URL + "/second?template=json"}, + }, + }, + } + + addr, _, _ := startServer(t, NewSMTPServer(config)) + + client, err := smtp.Dial(addr) + require.NoError(t, err) + t.Cleanup(func() { _ = client.Close() }) + + require.NoError(t, client.Hello("localhost")) + require.NoError(t, client.Auth(smtp.PlainAuth("", config.Username, config.Password, "localhost"))) + + for _, recipient := range []string{"first@example.com", "second@example.com"} { + require.NoError(t, client.Mail("sender@example.com")) + require.NoError(t, client.Rcpt(recipient)) + + w, err := client.Data() + require.NoError(t, err) + _, err = w.Write([]byte("Subject: For " + recipient + "\r\n\r\nbody for " + recipient)) + require.NoError(t, err) + require.NoError(t, w.Close()) + } + + require.NoError(t, client.Quit()) + + require.Len(t, received["/first"], 1) + require.Contains(t, received["/first"][0], "body for first@example.com") + require.Len(t, received["/second"], 1) + require.Contains(t, received["/second"][0], "body for second@example.com") +} diff --git a/email.go b/email.go index 35101ce..3612bb5 100644 --- a/email.go +++ b/email.go @@ -2,9 +2,9 @@ package smtp2shoutrrr import ( "bytes" + "errors" "fmt" "io" - "log" "log/slog" "mime" "mime/multipart" @@ -12,6 +12,11 @@ import ( "strings" ) +// errMalformedMessage marks a message this server can never turn into a +// notification. Redelivering the same bytes produces the same failure, so the +// sender is told to give up rather than retry forever. +var errMalformedMessage = errors.New("malformed message") + type ReceivedEmail struct { Recipients []string Msg *mail.Message @@ -31,7 +36,7 @@ func (re *ReceivedEmail) Body() (string, error) { } else { mediaType, params, err := mime.ParseMediaType(contentType) if err != nil { - log.Fatalf("Failed to parse Content-Type: %v", err) + return "", fmt.Errorf("%w: parsing Content-Type %q: %w", errMalformedMessage, contentType, err) } if strings.HasPrefix(mediaType, "multipart/") { diff --git a/email_test.go b/email_test.go new file mode 100644 index 0000000..9a4c9db --- /dev/null +++ b/email_test.go @@ -0,0 +1,90 @@ +package smtp2shoutrrr + +import ( + "net/mail" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func readMessage(t *testing.T, raw string) *mail.Message { + t.Helper() + + msg, err := mail.ReadMessage(strings.NewReader(raw)) + require.NoError(t, err) + + return msg +} + +// Regression: this header reached log.Fatalf, which exits the process. The +// value is attacker-controlled and needs nothing but the ability to deliver a +// message. +func TestBodyRejectsMalformedContentType(t *testing.T) { + for name, contentType := range map[string]string{ + "parameter without a value": "text/plain; charset", + "duplicate parameter": "text/plain; charset=utf-8; charset=ascii", + "no media type": ";", + "unterminated quoted value": `multipart/mixed; boundary="unterminated`, + } { + t.Run(name, func(t *testing.T) { + email := ReceivedEmail{ + Msg: readMessage(t, "Subject: Test\r\nContent-Type: "+contentType+"\r\n\r\nbody\r\n"), + } + + body, err := email.Body() + require.Error(t, err) + require.ErrorIs(t, err, errMalformedMessage, + "an unparseable header is permanent, so the sender must not be told to retry") + require.Empty(t, body) + }) + } +} + +func TestBodyReadsMessageWithoutContentType(t *testing.T) { + email := ReceivedEmail{Msg: readMessage(t, "Subject: Test\r\n\r\nplain body\r\n")} + + body, err := email.Body() + require.NoError(t, err) + require.Equal(t, "plain body\r\n", body) +} + +func TestBodyPrefersPlainTextPartOverHTML(t *testing.T) { + email := ReceivedEmail{Msg: readMessage(t, strings.Join([]string{ + "Subject: Test", + `Content-Type: multipart/alternative; boundary="b"`, + "", + "--b", + "Content-Type: text/html; charset=utf-8", + "", + "

html body

", + "--b", + "Content-Type: text/plain; charset=utf-8", + "", + "plain body", + "--b--", + "", + }, "\r\n"))} + + body, err := email.Body() + require.NoError(t, err) + require.Equal(t, "plain body", body) +} + +func TestBodyFallsBackToHTMLPart(t *testing.T) { + email := ReceivedEmail{Msg: readMessage(t, strings.Join([]string{ + "Subject: Test", + `Content-Type: multipart/alternative; boundary="b"`, + "", + "--b", + "Content-Type: text/html; charset=utf-8", + "", + "

html body

", + "--b--", + "", + }, "\r\n"))} + + body, err := email.Body() + require.NoError(t, err) + require.Equal(t, "

html body

", body) +} -- 2.52.0 From d028358be7437d025a69232639f9eab724a2e0bc Mon Sep 17 00:00:00 2001 From: Full-Stack Developer Date: Mon, 7 Sep 2026 20:00:57 +0000 Subject: [PATCH 2/2] fix: guard session transaction state and require SMTP credentials (FMG-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Co-authored-by: multica-agent --- README.md | 6 +- backend.go | 70 ++++++++++++++++++++--- backend_test.go | 146 ++++++++++++++++++++++++++++++++++++++++++++++++ config.go | 22 ++++---- config_test.go | 49 ++++++++++++++-- 5 files changed, 266 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 8c060b2..057a7a8 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ First generate a new configuration file following this example: # Port the SMTP server will listen on Port = 11025 -# Credentials for the SMTP server (default to username/password if not set/empty) +# Credentials for the SMTP server (required — the server refuses to start without them) Username = "user" Password = "nometokens" @@ -47,7 +47,9 @@ Targets = ["ntfy://ntfy.sh/catch-all-topic?tags=unmatched"] The server refuses to start if the configuration cannot forward anything — no recipients and no catch-all, a recipient without addresses or usable targets, or a mistyped table name such as `[[Recipient]]`, which TOML would otherwise accept -silently. +silently. It also refuses to start without a `Username` and a `Password`: these +used to fall back to `username`/`password`, which left the authentication gate +open to the most obvious guess there is. Clients must authenticate with the configured `Username` and `Password` before starting a mail transaction; an unauthenticated client is refused rather than diff --git a/backend.go b/backend.go index 30bdeea..fd05096 100644 --- a/backend.go +++ b/backend.go @@ -11,12 +11,18 @@ import ( "net/url" "slices" "strings" + "sync" "github.com/containrrr/shoutrrr" "github.com/emersion/go-sasl" "github.com/emersion/go-smtp" ) +// errNoUsableTargets marks a configuration fault rather than a delivery one: +// the recipient matched, but there is nothing to notify. No retry can add +// targets to a configuration. +var errNoUsableTargets = errors.New("recipient has no usable targets") + var ( // errUnreadableMessage rejects a message for good. Both causes — a // message that is not parseable as mail, and a Content-Type this server @@ -27,6 +33,15 @@ var ( Message: "Message could not be parsed", } + // errMisconfiguredRecipient is permanent for the same reason: the server + // has nowhere to send this recipient's mail until its configuration + // changes, which no amount of redelivery brings about. + errMisconfiguredRecipient = &smtp.SMTPError{ + Code: 550, + EnhancedCode: smtp.EnhancedCode{5, 3, 5}, + Message: "Recipient has no notification targets configured", + } + // errForwardingFailed asks the sender to try again later. The 250 this // replaces told the client a message had been delivered when no target // had received it, so the client dropped a notification nobody ever saw. @@ -46,7 +61,7 @@ func (bkd *Backend) sendNotifications(recipient ConfigRecipient, email ReceivedE targetURLs := recipient.GetTargetURLs() if len(targetURLs) == 0 { - return fmt.Errorf("recipient %s has no usable targets", + return fmt.Errorf("%w: %s", errNoUsableTargets, strings.Join(recipient.Addresses, ",")) } @@ -151,6 +166,12 @@ func (bkd *Backend) forwardEmail(email ReceivedEmail) error { } type Session struct { + // mu guards the transaction state below. go-smtp runs Data on its own + // goroutine for a BDAT transfer and keeps serving commands on the + // connection goroutine meanwhile, so an RSET can land in the middle of + // one and reset the session from under it. + mu sync.Mutex + addresses []string // authenticated gates the mail transaction. go-smtp advertises AUTH but @@ -163,6 +184,22 @@ type Session struct { forwarderFunc func(ReceivedEmail) error } +func (s *Session) isAuthenticated() bool { + s.mu.Lock() + defer s.mu.Unlock() + + return s.authenticated +} + +// recipients copies the transaction's recipients so a concurrent Reset cannot +// empty the slice a forward in progress is still reading. +func (s *Session) recipients() []string { + s.mu.Lock() + defer s.mu.Unlock() + + return slices.Clone(s.addresses) +} + func (s *Session) AuthMechanisms() []string { return []string{sasl.Plain} } @@ -194,7 +231,9 @@ func (s *Session) Auth(mech string) (sasl.Server, error) { return smtp.ErrAuthFailed } + s.mu.Lock() s.authenticated = true + s.mu.Unlock() return nil }), nil @@ -212,7 +251,7 @@ func credentialMatches(supplied, configured string) bool { } func (s *Session) Mail(from string, opts *smtp.MailOptions) error { - if !s.authenticated { + if !s.isAuthenticated() { return smtp.ErrAuthRequired } @@ -222,40 +261,51 @@ func (s *Session) Mail(from string, opts *smtp.MailOptions) error { } func (s *Session) Rcpt(to string, opts *smtp.RcptOptions) error { - if !s.authenticated { + if !s.isAuthenticated() { return smtp.ErrAuthRequired } slog.Debug("Rcpt to", slog.String("to", to)) + + s.mu.Lock() + defer s.mu.Unlock() s.addresses = append(s.addresses, to) return nil } func (s *Session) Data(r io.Reader) error { - if !s.authenticated { + if !s.isAuthenticated() { return smtp.ErrAuthRequired } + // Taken before the message is read: an RSET arriving mid-transfer aborts + // the read, and this is the transaction the recipients belong to either + // way. + recipients := s.recipients() + msg, err := mail.ReadMessage(r) if err != nil { slog.Error("Error reading data", slog.String("err", err.Error())) return errUnreadableMessage } - slog.Info("Received email", slog.String("destination", strings.Join(s.addresses, ","))) + slog.Info("Received email", slog.String("destination", strings.Join(recipients, ","))) if err := s.forwarderFunc(ReceivedEmail{ - Recipients: s.addresses, + Recipients: recipients, Msg: msg, }); err != nil { slog.Error("Error forwarding email", slog.String("err", err.Error())) - if errors.Is(err, errMalformedMessage) { + switch { + case errors.Is(err, errMalformedMessage): return errUnreadableMessage + case errors.Is(err, errNoUsableTargets): + return errMisconfiguredRecipient + default: + return errForwardingFailed } - - return errForwardingFailed } return nil @@ -267,6 +317,8 @@ func (s *Session) Data(r io.Reader) error { // Authentication deliberately survives, as RFC 5321 scopes RSET to the // transaction. func (s *Session) Reset() { + s.mu.Lock() + defer s.mu.Unlock() s.addresses = nil } diff --git a/backend_test.go b/backend_test.go index d68fae9..e4c5eb4 100644 --- a/backend_test.go +++ b/backend_test.go @@ -1,10 +1,15 @@ package smtp2shoutrrr import ( + "bufio" + "encoding/base64" + "fmt" "io" + "net" "net/http" "net/http/httptest" "net/smtp" + "strings" "testing" "time" @@ -553,3 +558,144 @@ func TestRecipientsDoNotLeakBetweenMessagesOnOneConnection(t *testing.T) { require.Len(t, received["/second"], 1) require.Contains(t, received["/second"][0], "body for second@example.com") } + +// smtpProbe drives a raw SMTP conversation. net/smtp cannot issue BDAT, and +// the transfer below needs a chunked one with an RSET pipelined behind it. +type smtpProbe struct { + t *testing.T + conn net.Conn + r *bufio.Reader +} + +func dialProbe(t *testing.T, addr string) *smtpProbe { + t.Helper() + + conn, err := net.DialTimeout("tcp", addr, 5*time.Second) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + require.NoError(t, conn.SetDeadline(time.Now().Add(30*time.Second))) + + probe := &smtpProbe{t: t, conn: conn, r: bufio.NewReader(conn)} + probe.expect("220") + + return probe +} + +func (p *smtpProbe) send(format string, args ...any) { + p.t.Helper() + + _, err := fmt.Fprintf(p.conn, format, args...) + require.NoError(p.t, err) +} + +// expect reads one full reply, skipping the continuation lines of a multiline +// one such as the EHLO capability list, and asserts its status code. +func (p *smtpProbe) expect(code string) { + p.t.Helper() + + for { + line, err := p.r.ReadString('\n') + require.NoError(p.t, err) + + if len(line) > 3 && line[3] == '-' { + continue + } + + require.Truef(p.t, strings.HasPrefix(line, code), "expected %s, got %q", code, line) + + return + } +} + +func (p *smtpProbe) authenticate(username, password string) { + p.t.Helper() + + p.send("EHLO localhost\r\n") + p.expect("250") + p.send("AUTH PLAIN %s\r\n", + base64.StdEncoding.EncodeToString([]byte("\x00"+username+"\x00"+password))) + p.expect("235") +} + +// Regression: Reset writes the transaction's recipients on the connection +// goroutine, while go-smtp runs Data on its own goroutine for a BDAT transfer +// and does not block RSET during one. Under -race this reported a data race +// between Reset and the forward in Data; it only passes while the transaction +// state stays guarded. +func TestResetDuringChunkedTransferIsSynchronized(t *testing.T) { + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusOK) + })) + defer mockServer.Close() + + config := &Config{ + Port: 0, + Username: "testuser", + Password: "testpass", + Recipients: []ConfigRecipient{ + { + Addresses: []string{"test@example.com"}, + Targets: []string{"generic+" + mockServer.URL + "/?template=json"}, + }, + }, + } + + addr, _, _ := startServer(t, NewSMTPServer(config)) + + // The overlap is a few instructions wide, so the transfer is repeated + // rather than attempted once. + for range 30 { + probe := dialProbe(t, addr) + probe.authenticate(config.Username, config.Password) + + probe.send("MAIL FROM:\r\n") + probe.expect("250") + probe.send("RCPT TO:\r\n") + probe.expect("250") + + // Headers without their terminating blank line, so the transfer + // goroutine parks inside mail.ReadMessage. + const headers = "Subject: Chunked\r\n" + probe.send("BDAT %d\r\n%s", len(headers), headers) + probe.expect("250") + + // The blank line releases mail.ReadMessage on the transfer goroutine; + // the RSET pipelined behind it reaches Session.Reset on the connection + // goroutine at the same moment. + probe.send("BDAT 2\r\n\r\nRSET\r\n") + probe.expect("250") + probe.expect("250") + + // The connection outlives the aborted transaction. + probe.send("QUIT\r\n") + probe.expect("221") + } +} + +// A recipient with no usable targets is a configuration fault, not a delivery +// one: no retry can add targets to a config, so it is refused permanently +// rather than with the 451 the other forwarding failures get. LoadConfig +// blocks such a config, so this is only reachable from one built in code. +func TestRecipientWithoutTargetsIsRejectedPermanently(t *testing.T) { + config := &Config{ + Port: 0, + Username: "testuser", + Password: "testpass", + Recipients: []ConfigRecipient{ + {Addresses: []string{"test@example.com"}, Targets: []string{"://not-a-url"}}, + }, + } + + addr, _, _ := startServer(t, NewSMTPServer(config)) + + err := smtp.SendMail( + addr, + smtp.PlainAuth("", config.Username, config.Password, "localhost"), + "sender@example.com", + []string{"test@example.com"}, + []byte("Subject: Nowhere To Go\r\n\r\nbody"), + ) + require.Error(t, err) + require.Contains(t, err.Error(), "550", "a retry cannot add targets to a configuration") +} diff --git a/config.go b/config.go index 7fc4ffb..61411e8 100644 --- a/config.go +++ b/config.go @@ -67,23 +67,25 @@ func (c *Config) SetDefaults() { if c.Port == 0 { c.Port = 11125 } +} +// Validate rejects configuration that would leave the server relaying for +// anyone, or accepting mail it can never forward. A TOML decode ignores +// unknown keys, so a mistyped table or field name ([[Recipient]], Adresses) +// otherwise yields a server that answers 250 OK to everything and sends +// nothing. +func (c *Config) Validate() error { + // Credentials used to fall back to username/password when unset, which + // left the AUTH gate open to the most obvious guess there is — for + // exactly the deployments that had configured the least. if c.Username == "" { - slog.Warn("no username provided, using default: username") - c.Username = "username" + return errors.New("no Username configured") } if c.Password == "" { - slog.Warn("no password provided, using default: password") - c.Password = "password" + return errors.New("no Password configured") } -} -// Validate rejects configuration that would leave the server accepting mail it -// can never forward. A TOML decode ignores unknown keys, so a mistyped table or -// field name ([[Recipient]], Adresses) otherwise yields a server that answers -// 250 OK to everything and sends nothing. -func (c *Config) Validate() error { for i := range c.Recipients { r := &c.Recipients[i] if len(r.Addresses) == 0 { diff --git a/config_test.go b/config_test.go index f5a1820..81d898c 100644 --- a/config_test.go +++ b/config_test.go @@ -10,6 +10,14 @@ import ( "github.com/stretchr/testify/require" ) +// credentials is prepended to fixtures that are not themselves about +// authentication, so each one still fails (or passes) for the reason it names +// rather than for the credentials it does not mention. +const credentials = ` +Username = "user" +Password = "secret" +` + func writeConfig(t *testing.T, contents string) string { t.Helper() @@ -54,7 +62,7 @@ Targets = ["ntfy://ntfy.sh/catch-all"] } func TestLoadConfigAppliesDefaults(t *testing.T) { - path := writeConfig(t, ` + path := writeConfig(t, credentials+` [[Recipients]] Addresses = ["user@example.com"] Targets = ["ntfy://ntfy.sh/topic"] @@ -64,9 +72,38 @@ Targets = ["ntfy://ntfy.sh/topic"] require.NoError(t, err) require.Equal(t, 11125, config.Port) - require.Equal(t, "username", config.Username) - require.Equal(t, "password", config.Password) require.Nil(t, config.CatchAll) + + // Credentials deliberately have no default; see + // TestLoadConfigRejectsUnsetCredentials. + require.Equal(t, "user", config.Username) + require.Equal(t, "secret", config.Password) +} + +// Regression: unset credentials used to fall back to username/password with +// only a warning, so a config naming nothing but a port and its recipients +// left the AUTH gate open to the most obvious guess there is — and the README +// promised an authentication that had never been configured. +func TestLoadConfigRejectsUnsetCredentials(t *testing.T) { + recipient := ` +[[Recipients]] +Addresses = ["user@example.com"] +Targets = ["ntfy://ntfy.sh/topic"] +` + + for name, contents := range map[string]string{ + "neither credential": recipient, + "no Username": `Password = "secret"` + recipient, + "no Password": `Username = "user"` + recipient, + "empty Username": `Username = ""` + "\n" + `Password = "secret"` + recipient, + "empty Password": `Username = "user"` + "\n" + `Password = ""` + recipient, + } { + t.Run(name, func(t *testing.T) { + config, err := LoadConfig(writeConfig(t, contents)) + require.Error(t, err) + require.Nil(t, config) + }) + } } func TestLoadConfigMissingFile(t *testing.T) { @@ -116,14 +153,14 @@ Port = 2525 `, } { t.Run(name, func(t *testing.T) { - _, err := LoadConfig(writeConfig(t, contents)) + _, err := LoadConfig(writeConfig(t, credentials+contents)) require.Error(t, err) }) } } func TestLoadConfigAcceptsCatchAllOnly(t *testing.T) { - config, err := LoadConfig(writeConfig(t, ` + config, err := LoadConfig(writeConfig(t, credentials+` [CatchAll] Targets = ["ntfy://ntfy.sh/catch-all"] `)) @@ -141,7 +178,7 @@ func TestLoadConfigWarnsAboutIgnoredKeys(t *testing.T) { slog.SetDefault(slog.New(slog.NewTextHandler(&logged, &slog.HandlerOptions{Level: slog.LevelWarn}))) t.Cleanup(func() { slog.SetDefault(restore) }) - config, err := LoadConfig(writeConfig(t, ` + config, err := LoadConfig(writeConfig(t, credentials+` [[Recipient]] Addresses = ["user@example.com"] Targets = ["ntfy://ntfy.sh/topic"] -- 2.52.0