From 8f8b5079c3036401bfeed3bf4ac2b6bc559a83eb Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 26 Nov 2024 11:37:26 +0100 Subject: [PATCH 1/6] Improve filename sanitization in MIME headers Sanitize filenames to replace invalid characters before encoding them. This prevents control and special characters from causing issues in MIME headers and file systems. The `sanitizeFilename` function ensures these characters are replaced with underscores. --- msgwriter.go | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/msgwriter.go b/msgwriter.go index ed2b41e0..18fa187e 100644 --- a/msgwriter.go +++ b/msgwriter.go @@ -273,7 +273,7 @@ func (mw *msgWriter) addFiles(files []*File, isAttachment bool) { mimeType = string(file.ContentType) } file.setHeader(HeaderContentType, fmt.Sprintf(`%s; name="%s"`, mimeType, - mw.encoder.Encode(mw.charset.String(), file.Name))) + mw.encoder.Encode(mw.charset.String(), sanitizeFilename(file.Name)))) } if _, ok := file.getHeader(HeaderContentTransferEnc); !ok { @@ -285,7 +285,7 @@ func (mw *msgWriter) addFiles(files []*File, isAttachment bool) { if file.Desc != "" { if _, ok := file.getHeader(HeaderContentDescription); !ok { - file.setHeader(HeaderContentDescription, file.Desc) + file.setHeader(HeaderContentDescription, mw.encoder.Encode(mw.charset.String(), file.Desc)) } } @@ -295,12 +295,12 @@ func (mw *msgWriter) addFiles(files []*File, isAttachment bool) { disposition = "attachment" } file.setHeader(HeaderContentDisposition, fmt.Sprintf(`%s; filename="%s"`, - disposition, mw.encoder.Encode(mw.charset.String(), file.Name))) + disposition, mw.encoder.Encode(mw.charset.String(), sanitizeFilename(file.Name)))) } if !isAttachment { if _, ok := file.getHeader(HeaderContentID); !ok { - file.setHeader(HeaderContentID, fmt.Sprintf("<%s>", file.Name)) + file.setHeader(HeaderContentID, fmt.Sprintf("<%s>", sanitizeFilename(file.Name))) } } if mw.depth == 0 { @@ -498,3 +498,33 @@ func (mw *msgWriter) writeBody(writeFunc func(io.Writer) (int64, error), encodin mw.bytesWritten += n } } + +// sanitizeFilename sanitizes a given filename string by replacing specific unwanted characters with +// an underscore ('_'). +// +// This method replaces any control character and any special character that is problematic for +// MIME headers and file systems with an underscore ('_') character. +// +// The following characters are replaced +// - Any control character (US-ASCII < 32) +// - ", /, :, <, >, ?, \, |, [DEL] +// +// Parameters: +// - input: A string of a filename that is supposed to be sanitized +// +// Returns: +// - A string representing the sanitized version of the filename +func sanitizeFilename(input string) string { + var sanitized strings.Builder + for i := 0; i < len(input); i++ { + // We do not allow control characters in file names. + if input[i] < 32 || input[i] == 34 || input[i] == 47 || input[i] == 58 || + input[i] == 60 || input[i] == 62 || input[i] == 63 || input[i] == 92 || + input[i] == 124 || input[i] == 127 { + sanitized.WriteRune('_') + continue + } + sanitized.WriteByte(input[i]) + } + return sanitized.String() +} From b051471f8d5cc8e5b13c7259269588c010d47115 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 26 Nov 2024 12:02:15 +0100 Subject: [PATCH 2/6] Add tests for the sanitizeFilename function This commit introduces a series of tests for the sanitizeFilename function in msgwriter_test.go. The tests cover various edge cases to ensure filenames are sanitized correctly by replacing or removing invalid characters. These additions will help maintain the integrity and reliability of filename sanitization in the codebase. --- msgwriter_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/msgwriter_test.go b/msgwriter_test.go index 1b2a2540..a3d2e565 100644 --- a/msgwriter_test.go +++ b/msgwriter_test.go @@ -676,3 +676,33 @@ func TestMsgWriter_writeBody(t *testing.T) { } }) } + +func TestMsgWriter_sanitizeFilename(t *testing.T) { + tests := []struct { + given string + want string + }{ + {"test.txt", "test.txt"}, + {"test file.txt", "test file.txt"}, + {"test\\ file.txt", "test_ file.txt"}, + {`"test" file.txt`, "_test_ file.txt"}, + {`test file .txt`, "test_file_.txt"}, + {"test\r\nfile.txt", "test__file.txt"}, + {"test\x22file.txt", "test_file.txt"}, + {"test\x2ffile.txt", "test_file.txt"}, + {"test\x3afile.txt", "test_file.txt"}, + {"test\x3cfile.txt", "test_file.txt"}, + {"test\x3efile.txt", "test_file.txt"}, + {"test\x3ffile.txt", "test_file.txt"}, + {"test\x5cfile.txt", "test_file.txt"}, + {"test\x7cfile.txt", "test_file.txt"}, + {"test\x7ffile.txt", "test_file.txt"}, + } + for _, tt := range tests { + t.Run(tt.given+"=>"+tt.want, func(t *testing.T) { + if got := sanitizeFilename(tt.given); got != tt.want { + t.Errorf("sanitizeFilename failed, expected: %q, got: %q", tt.want, got) + } + }) + } +} From fe765cd49e6a55fb1637488e50b8407769ed566e Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 26 Nov 2024 16:17:34 +0100 Subject: [PATCH 3/6] Add filename sanitization tests for attachments Introduced tests to validate filename sanitization for attachments, ensuring disallowed characters are handled correctly. These tests cover various scenarios, including different character sets such as Japanese, Chinese, and Cyrillic. --- msgwriter_test.go | 76 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/msgwriter_test.go b/msgwriter_test.go index a3d2e565..31aae211 100644 --- a/msgwriter_test.go +++ b/msgwriter_test.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "mime" + "os" "runtime" "strings" "testing" @@ -304,6 +305,81 @@ func TestMsgWriter_addFiles(t *testing.T) { charset: CharsetUTF8, encoder: getEncoder(EncodingQP), } + tests := []struct { + name string + filename string + expect string + }{ + {"normal US-ASCII filename", "test.txt", "test.txt"}, + {"normal US-ASCII filename with space", "test file.txt", "test file.txt"}, + {"filename with new lines", "test\r\n.txt", "test__.txt"}, + {"filename with disallowed character:\x22", "test\x22.txt", "test_.txt"}, + {"filename with disallowed character:\x2f", "test\x2f.txt", "test_.txt"}, + {"filename with disallowed character:\x3a", "test\x3a.txt", "test_.txt"}, + {"filename with disallowed character:\x3c", "test\x3c.txt", "test_.txt"}, + {"filename with disallowed character:\x3e", "test\x3e.txt", "test_.txt"}, + {"filename with disallowed character:\x3f", "test\x3f.txt", "test_.txt"}, + {"filename with disallowed character:\x5c", "test\x5c.txt", "test_.txt"}, + {"filename with disallowed character:\x7c", "test\x7c.txt", "test_.txt"}, + {"filename with disallowed character:\x7f", "test\x7f.txt", "test_.txt"}, + { + "japanese characters filename", "添付ファイル.txt", + "=?UTF-8?q?=E6=B7=BB=E4=BB=98=E3=83=95=E3=82=A1=E3=82=A4=E3=83=AB.txt?=", + }, + { + "simplified chinese characters filename", "测试附件文件.txt", + "=?UTF-8?q?=E6=B5=8B=E8=AF=95=E9=99=84=E4=BB=B6=E6=96=87=E4=BB=B6.txt?=", + }, + { + "cyrillic characters filename", "Тестовый прикрепленный файл.txt", + "=?UTF-8?q?=D0=A2=D0=B5=D1=81=D1=82=D0=BE=D0=B2=D1=8B=D0=B9_=D0=BF=D1=80?= " + + "=?UTF-8?q?=D0=B8=D0=BA=D1=80=D0=B5=D0=BF=D0=BB=D0=B5=D0=BD=D0=BD=D1=8B?= " + + "=?UTF-8?q?=D0=B9_=D1=84=D0=B0=D0=B9=D0=BB.txt?=", + }, + } + for _, tt := range tests { + t.Run("addFile with filename sanitization: "+tt.name, func(t *testing.T) { + buffer := bytes.NewBuffer(nil) + msgwriter.writer = buffer + message := testMessage(t) + tmpfile, err := os.CreateTemp("", "attachment.*.tmp") + if err != nil { + t.Fatalf("failed to create tempfile: %s", err) + } + t.Cleanup(func() { + if err = os.Remove(tmpfile.Name()); err != nil { + t.Errorf("failed to remove tempfile: %s", err) + } + }) + + source, err := os.Open("testdata/attachment.txt") + if err != nil { + t.Fatalf("failed to open source file: %s", err) + } + if _, err = io.Copy(tmpfile, source); err != nil { + t.Fatalf("failed to copy source file: %s", err) + } + if err = tmpfile.Close(); err != nil { + t.Fatalf("failed to close tempfile: %s", err) + } + if err = source.Close(); err != nil { + t.Fatalf("failed to close source file: %s", err) + } + message.AttachFile(tmpfile.Name(), WithFileName(tt.filename)) + msgwriter.writeMsg(message) + if msgwriter.err != nil { + t.Errorf("msgWriter failed to write: %s", msgwriter.err) + } + ctExpect := fmt.Sprintf(`Content-Type: text/plain; charset=utf-8; name="%s"`, tt.expect) + cdExpect := fmt.Sprintf(`Content-Disposition: attachment; filename="%s"`, tt.expect) + if !strings.Contains(buffer.String(), ctExpect) { + t.Errorf("expected content-type: %q, got: %q", ctExpect, buffer.String()) + } + if !strings.Contains(buffer.String(), cdExpect) { + t.Errorf("expected content-disposition: %q, got: %q", cdExpect, buffer.String()) + } + }) + } t.Run("message with a single file attached", func(t *testing.T) { buffer := bytes.NewBuffer(nil) msgwriter.writer = buffer From 75c0e3319be3ab43833c9864d993297f74984837 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 26 Nov 2024 16:26:13 +0100 Subject: [PATCH 4/6] Simplify attachment test setup Removed temporary file creation and copying in msgwriter_test.go. Directly attach the source file during the test to streamline the setup process. This change reduces complexity and potential points of failure in the test code. --- msgwriter_test.go | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-) diff --git a/msgwriter_test.go b/msgwriter_test.go index 31aae211..d72c40c7 100644 --- a/msgwriter_test.go +++ b/msgwriter_test.go @@ -10,7 +10,6 @@ import ( "fmt" "io" "mime" - "os" "runtime" "strings" "testing" @@ -342,30 +341,7 @@ func TestMsgWriter_addFiles(t *testing.T) { buffer := bytes.NewBuffer(nil) msgwriter.writer = buffer message := testMessage(t) - tmpfile, err := os.CreateTemp("", "attachment.*.tmp") - if err != nil { - t.Fatalf("failed to create tempfile: %s", err) - } - t.Cleanup(func() { - if err = os.Remove(tmpfile.Name()); err != nil { - t.Errorf("failed to remove tempfile: %s", err) - } - }) - - source, err := os.Open("testdata/attachment.txt") - if err != nil { - t.Fatalf("failed to open source file: %s", err) - } - if _, err = io.Copy(tmpfile, source); err != nil { - t.Fatalf("failed to copy source file: %s", err) - } - if err = tmpfile.Close(); err != nil { - t.Fatalf("failed to close tempfile: %s", err) - } - if err = source.Close(); err != nil { - t.Fatalf("failed to close source file: %s", err) - } - message.AttachFile(tmpfile.Name(), WithFileName(tt.filename)) + message.AttachFile("testdata/attachment.txt", WithFileName(tt.filename)) msgwriter.writeMsg(message) if msgwriter.err != nil { t.Errorf("msgWriter failed to write: %s", msgwriter.err) From f61da9cc2ee066df9ab2e1d8ac2f91039710b525 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 26 Nov 2024 16:34:09 +0100 Subject: [PATCH 5/6] Update content-type handling for FreeBSD in msgwriter test Adjusted the expected content-type header in msgwriter tests to account for FreeBSD by setting it to 'application/octet-stream' instead of 'text/plain'. This ensures compatibility and correct behavior across different operating systems. --- msgwriter_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/msgwriter_test.go b/msgwriter_test.go index d72c40c7..3fd158c6 100644 --- a/msgwriter_test.go +++ b/msgwriter_test.go @@ -346,8 +346,15 @@ func TestMsgWriter_addFiles(t *testing.T) { if msgwriter.err != nil { t.Errorf("msgWriter failed to write: %s", msgwriter.err) } - ctExpect := fmt.Sprintf(`Content-Type: text/plain; charset=utf-8; name="%s"`, tt.expect) + + var ctExpect string cdExpect := fmt.Sprintf(`Content-Disposition: attachment; filename="%s"`, tt.expect) + switch runtime.GOOS { + case "freebsd": + ctExpect = fmt.Sprintf(`Content-Type: application/octet-stream; charset=utf-8; name="%s"`, tt.expect) + default: + ctExpect = fmt.Sprintf(`Content-Type: text/plain; charset=utf-8; name="%s"`, tt.expect) + } if !strings.Contains(buffer.String(), ctExpect) { t.Errorf("expected content-type: %q, got: %q", ctExpect, buffer.String()) } From 06bee90a1ea068fe7d5a56463c74d06bf9715847 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 26 Nov 2024 16:38:30 +0100 Subject: [PATCH 6/6] Remove charset from octet-stream Content-Type on FreeBSD The charset parameter in the Content-Type header for octet-stream files on FreeBSD was removed. This aligns the behavior with the expected MIME type format for such files. Other platforms remain unchanged. --- msgwriter_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msgwriter_test.go b/msgwriter_test.go index 3fd158c6..2c11406c 100644 --- a/msgwriter_test.go +++ b/msgwriter_test.go @@ -351,7 +351,7 @@ func TestMsgWriter_addFiles(t *testing.T) { cdExpect := fmt.Sprintf(`Content-Disposition: attachment; filename="%s"`, tt.expect) switch runtime.GOOS { case "freebsd": - ctExpect = fmt.Sprintf(`Content-Type: application/octet-stream; charset=utf-8; name="%s"`, tt.expect) + ctExpect = fmt.Sprintf(`Content-Type: application/octet-stream; name="%s"`, tt.expect) default: ctExpect = fmt.Sprintf(`Content-Type: text/plain; charset=utf-8; name="%s"`, tt.expect) }