From aca9a7bc53d68b38dc3803cec24bfdcb9c7a8b2a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 22 Dec 2021 17:35:51 +0000 Subject: [PATCH 1/4] Add option to convert CRLF to LF line endings for sendmail It appears that several versions of sendmail require that the mail is sent to them with LF line endings instead of CRLF endings - which of course they will then convert back to CRLF line endings to comply with the SMTP standard. This PR adds another setting SENDMAIL_CONVERT_CRLF which will pass the message writer through a filter. This will filter out and convert CRLFs to LFs before writing them out to sendmail. Fix #18024 Signed-off-by: Andrew Thornton --- custom/conf/app.example.ini | 3 + .../doc/advanced/config-cheat-sheet.en-us.md | 1 + modules/setting/mailer.go | 12 +-- services/mailer/mailer.go | 80 ++++++++++++++++++- services/mailer/mailer_test.go | 42 ++++++++++ 5 files changed, 131 insertions(+), 7 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 1d19a343804fe..987e84a0d8ccc 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1494,6 +1494,9 @@ PATH = ;; ;; Timeout for Sendmail ;SENDMAIL_TIMEOUT = 5m +;; +;; convert \r\n to \n for Sendmail +;SENDMAIL_CONVERT_CRLF = false ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 07655a181b1e2..00816964af49b 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -667,6 +667,7 @@ Define allowed algorithms and their minimum key length (use -1 to disable a type command or full path). - `SENDMAIL_ARGS`: **_empty_**: Specify any extra sendmail arguments. - `SENDMAIL_TIMEOUT`: **5m**: default timeout for sending email through sendmail +- `SENDMAIL_CONVERT_CRLF`: **false**: some versions of sendmail require LF line endings rather than CRLF line endings. Set this to true if you require this. - `SEND_BUFFER_LEN`: **100**: Buffer length of mailing queue. **DEPRECATED** use `LENGTH` in `[queue.mailer]` ## Cache (`cache`) diff --git a/modules/setting/mailer.go b/modules/setting/mailer.go index 1bcd63a914bd1..19570ea979b25 100644 --- a/modules/setting/mailer.go +++ b/modules/setting/mailer.go @@ -37,9 +37,10 @@ type Mailer struct { IsTLSEnabled bool // Sendmail sender - SendmailPath string - SendmailArgs []string - SendmailTimeout time.Duration + SendmailPath string + SendmailArgs []string + SendmailTimeout time.Duration + SendmailConvertCRLF bool } var ( @@ -71,8 +72,9 @@ func newMailService() { IsTLSEnabled: sec.Key("IS_TLS_ENABLED").MustBool(), SubjectPrefix: sec.Key("SUBJECT_PREFIX").MustString(""), - SendmailPath: sec.Key("SENDMAIL_PATH").MustString("sendmail"), - SendmailTimeout: sec.Key("SENDMAIL_TIMEOUT").MustDuration(5 * time.Minute), + SendmailPath: sec.Key("SENDMAIL_PATH").MustString("sendmail"), + SendmailTimeout: sec.Key("SENDMAIL_TIMEOUT").MustDuration(5 * time.Minute), + SendmailConvertCRLF: sec.Key("SENDMAIL_CONVERT_CRLF").MustBool(false), } MailService.From = sec.Key("FROM").MustString(MailService.User) MailService.EnvelopeFrom = sec.Key("ENVELOPE_FROM").MustString("") diff --git a/services/mailer/mailer.go b/services/mailer/mailer.go index eac2b15c3c2ca..70bcbca8320c6 100644 --- a/services/mailer/mailer.go +++ b/services/mailer/mailer.go @@ -253,6 +253,73 @@ func (s *smtpSender) Send(from string, to []string, msg io.WriterTo) error { return client.Quit() } +type crlfConverter struct { + danglingCR bool + w io.Writer +} + +func (c *crlfConverter) Write(bs []byte) (n int, err error) { + if len(bs) == 0 { + if c.danglingCR { + _, err := c.w.Write([]byte{'\r'}) + if err != nil { + return 0, err + } + c.danglingCR = false + } + return c.w.Write(bs) + } + if c.danglingCR { + if bs[0] != '\n' { + _, err := c.w.Write([]byte{'\r'}) + if err != nil { + return 0, err + } + } + c.danglingCR = false + } + if bs[len(bs)-1] == '\r' { + c.danglingCR = true + bs = bs[:len(bs)-1] + } + idx := bytes.Index(bs, []byte{'\r', '\n'}) + for idx >= 0 { + count, err := c.w.Write(bs[:idx]) + n += count + if err != nil { + return n, err + } + count, err = c.w.Write([]byte{'\n'}) + if count == 1 { + n += 2 + } + if err != nil { + return n, err + } + bs = bs[idx+2:] + idx = bytes.Index(bs, []byte{'\r', '\n'}) + } + if len(bs) > 0 { + count, err := c.w.Write(bs) + n += count + if err != nil { + return n, err + } + } + if c.danglingCR { + n++ + } + return +} + +func (c *crlfConverter) Close() (err error) { + if c.danglingCR { + _, err = c.w.Write([]byte{'\r'}) + c.danglingCR = false + } + return +} + // Sender sendmail mail sender type sendmailSender struct { } @@ -290,13 +357,22 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { return err } - _, err = msg.WriteTo(pipe) + if setting.MailService.SendmailConvertCRLF { + converter := &crlfConverter{ + w: pipe, + } + _, err = msg.WriteTo(converter) + if err == nil { + err = converter.Close() + } + } else { + _, err = msg.WriteTo(pipe) + } // we MUST close the pipe or sendmail will hang waiting for more of the message // Also we should wait on our sendmail command even if something fails closeError = pipe.Close() waitError = cmd.Wait() - if err != nil { return err } else if closeError != nil { diff --git a/services/mailer/mailer_test.go b/services/mailer/mailer_test.go index 56f2eb52b0bcd..08e149801eaaa 100644 --- a/services/mailer/mailer_test.go +++ b/services/mailer/mailer_test.go @@ -5,6 +5,7 @@ package mailer import ( + "strings" "testing" "time" @@ -37,3 +38,44 @@ func TestGenerateMessageID(t *testing.T) { gm = m.ToMessage() assert.Equal(t, "", gm.GetHeader("Message-ID")[0]) } + +func TestCRLFConverter(t *testing.T) { + type testcaseType struct { + input []string + expected string + } + testcases := []testcaseType{ + { + input: []string{"This h\ras a \r", "\nnewline\r\n"}, + expected: "This h\ras a \nnewline\n", + }, + { + input: []string{"This\r\n has a \r\n\r", "\n\r\nnewline\r\n"}, + expected: "This\n has a \n\n\nnewline\n", + }, + { + input: []string{"This has a \r", "\nnewline\r"}, + expected: "This has a \nnewline\r", + }, + { + input: []string{"This has a \r", "newline\r"}, + expected: "This has a \rnewline\r", + }, + } + for _, testcase := range testcases { + out := &strings.Builder{} + converter := &crlfConverter{w: out} + realsum, sum := 0, 0 + for _, in := range testcase.input { + n, err := converter.Write([]byte(in)) + assert.NoError(t, err) + assert.Equal(t, len(in), n) + sum += n + realsum += len(in) + } + err := converter.Close() + assert.NoError(t, err) + assert.Equal(t, realsum, sum) + assert.Equal(t, testcase.expected, out.String()) + } +} From b5287ee93888e9977811bfd31c4eca9b2c1fe00b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 22 Dec 2021 17:58:09 +0000 Subject: [PATCH 2/4] flip the meaning of SENDMAIL_CONVERT_CRLF Signed-off-by: Andrew Thornton --- custom/conf/app.example.ini | 2 +- docs/content/doc/advanced/config-cheat-sheet.en-us.md | 2 +- modules/setting/mailer.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 987e84a0d8ccc..6a42e95d489c5 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1496,7 +1496,7 @@ PATH = ;SENDMAIL_TIMEOUT = 5m ;; ;; convert \r\n to \n for Sendmail -;SENDMAIL_CONVERT_CRLF = false +;SENDMAIL_CONVERT_CRLF = true ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 00816964af49b..a0df18c49b980 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -667,7 +667,7 @@ Define allowed algorithms and their minimum key length (use -1 to disable a type command or full path). - `SENDMAIL_ARGS`: **_empty_**: Specify any extra sendmail arguments. - `SENDMAIL_TIMEOUT`: **5m**: default timeout for sending email through sendmail -- `SENDMAIL_CONVERT_CRLF`: **false**: some versions of sendmail require LF line endings rather than CRLF line endings. Set this to true if you require this. +- `SENDMAIL_CONVERT_CRLF`: **true**: Most versions of sendmail prefer LF line endings rather than CRLF line endings. Set this to false if your version of sendmail requires CRLF line endings. - `SEND_BUFFER_LEN`: **100**: Buffer length of mailing queue. **DEPRECATED** use `LENGTH` in `[queue.mailer]` ## Cache (`cache`) diff --git a/modules/setting/mailer.go b/modules/setting/mailer.go index 19570ea979b25..d7713f3b80bef 100644 --- a/modules/setting/mailer.go +++ b/modules/setting/mailer.go @@ -74,7 +74,7 @@ func newMailService() { SendmailPath: sec.Key("SENDMAIL_PATH").MustString("sendmail"), SendmailTimeout: sec.Key("SENDMAIL_TIMEOUT").MustDuration(5 * time.Minute), - SendmailConvertCRLF: sec.Key("SENDMAIL_CONVERT_CRLF").MustBool(false), + SendmailConvertCRLF: sec.Key("SENDMAIL_CONVERT_CRLF").MustBool(true), } MailService.From = sec.Key("FROM").MustString(MailService.User) MailService.EnvelopeFrom = sec.Key("ENVELOPE_FROM").MustString("") From b61cafeec947d8e158b204ad0f8416e1dbdfd8cf Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 26 Dec 2021 10:20:23 +0000 Subject: [PATCH 3/4] Use Replacer as code is easier to understand Emails are relatively small so just buffer and replace instead of filtering \r\n through the writer. Signed-off-by: Andrew Thornton --- services/mailer/mailer.go | 75 ++------------------------------------- 1 file changed, 3 insertions(+), 72 deletions(-) diff --git a/services/mailer/mailer.go b/services/mailer/mailer.go index 70bcbca8320c6..e5e6272f102ec 100644 --- a/services/mailer/mailer.go +++ b/services/mailer/mailer.go @@ -253,73 +253,6 @@ func (s *smtpSender) Send(from string, to []string, msg io.WriterTo) error { return client.Quit() } -type crlfConverter struct { - danglingCR bool - w io.Writer -} - -func (c *crlfConverter) Write(bs []byte) (n int, err error) { - if len(bs) == 0 { - if c.danglingCR { - _, err := c.w.Write([]byte{'\r'}) - if err != nil { - return 0, err - } - c.danglingCR = false - } - return c.w.Write(bs) - } - if c.danglingCR { - if bs[0] != '\n' { - _, err := c.w.Write([]byte{'\r'}) - if err != nil { - return 0, err - } - } - c.danglingCR = false - } - if bs[len(bs)-1] == '\r' { - c.danglingCR = true - bs = bs[:len(bs)-1] - } - idx := bytes.Index(bs, []byte{'\r', '\n'}) - for idx >= 0 { - count, err := c.w.Write(bs[:idx]) - n += count - if err != nil { - return n, err - } - count, err = c.w.Write([]byte{'\n'}) - if count == 1 { - n += 2 - } - if err != nil { - return n, err - } - bs = bs[idx+2:] - idx = bytes.Index(bs, []byte{'\r', '\n'}) - } - if len(bs) > 0 { - count, err := c.w.Write(bs) - n += count - if err != nil { - return n, err - } - } - if c.danglingCR { - n++ - } - return -} - -func (c *crlfConverter) Close() (err error) { - if c.danglingCR { - _, err = c.w.Write([]byte{'\r'}) - c.danglingCR = false - } - return -} - // Sender sendmail mail sender type sendmailSender struct { } @@ -358,12 +291,10 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { } if setting.MailService.SendmailConvertCRLF { - converter := &crlfConverter{ - w: pipe, - } - _, err = msg.WriteTo(converter) + buf := &strings.Builder{} + _, err = msg.WriteTo(buf) if err == nil { - err = converter.Close() + _, err = strings.NewReplacer("\r\n", "\n").WriteString(pipe, buf.String()) } } else { _, err = msg.WriteTo(pipe) From 39327ec3ddb5e7dd3432dd3d29a3c2ad033324ab Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 26 Dec 2021 10:33:13 +0000 Subject: [PATCH 4/4] remove old test Signed-off-by: Andrew Thornton --- services/mailer/mailer_test.go | 42 ---------------------------------- 1 file changed, 42 deletions(-) diff --git a/services/mailer/mailer_test.go b/services/mailer/mailer_test.go index 08e149801eaaa..56f2eb52b0bcd 100644 --- a/services/mailer/mailer_test.go +++ b/services/mailer/mailer_test.go @@ -5,7 +5,6 @@ package mailer import ( - "strings" "testing" "time" @@ -38,44 +37,3 @@ func TestGenerateMessageID(t *testing.T) { gm = m.ToMessage() assert.Equal(t, "", gm.GetHeader("Message-ID")[0]) } - -func TestCRLFConverter(t *testing.T) { - type testcaseType struct { - input []string - expected string - } - testcases := []testcaseType{ - { - input: []string{"This h\ras a \r", "\nnewline\r\n"}, - expected: "This h\ras a \nnewline\n", - }, - { - input: []string{"This\r\n has a \r\n\r", "\n\r\nnewline\r\n"}, - expected: "This\n has a \n\n\nnewline\n", - }, - { - input: []string{"This has a \r", "\nnewline\r"}, - expected: "This has a \nnewline\r", - }, - { - input: []string{"This has a \r", "newline\r"}, - expected: "This has a \rnewline\r", - }, - } - for _, testcase := range testcases { - out := &strings.Builder{} - converter := &crlfConverter{w: out} - realsum, sum := 0, 0 - for _, in := range testcase.input { - n, err := converter.Write([]byte(in)) - assert.NoError(t, err) - assert.Equal(t, len(in), n) - sum += n - realsum += len(in) - } - err := converter.Close() - assert.NoError(t, err) - assert.Equal(t, realsum, sum) - assert.Equal(t, testcase.expected, out.String()) - } -}