diff --git a/README.md b/README.md index 3666932..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,21 @@ 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 +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 diff --git a/backend.go b/backend.go index 9189d9b..fd05096 100644 --- a/backend.go +++ b/backend.go @@ -1,6 +1,9 @@ package smtp2shoutrrr import ( + "crypto/sha256" + "crypto/subtle" + "errors" "fmt" "io" "log/slog" @@ -8,12 +11,47 @@ 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 + // 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", + } + + // 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. + 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 +61,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("%w: %s", errNoUsableTargets, + strings.Join(recipient.Addresses, ",")) } // Prepare email body once (reuse for all targets) @@ -73,10 +110,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 @@ -121,57 +166,161 @@ 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 + // 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 } +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} } 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.mu.Lock() + s.authenticated = true + s.mu.Unlock() + 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.isAuthenticated() { + 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.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.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 fmt.Errorf("error reading data: %w", err) + 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())) + + switch { + case errors.Is(err, errMalformedMessage): + return errUnreadableMessage + case errors.Is(err, errNoUsableTargets): + return errMisconfiguredRecipient + default: + 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.mu.Lock() + defer s.mu.Unlock() + s.addresses = nil +} func (s *Session) Logout() error { return nil diff --git a/backend_test.go b/backend_test.go index 330519c..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" @@ -282,3 +287,415 @@ 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") +} + +// 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"] 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) +}