fix: sanitize attachment filename to prevent path traversal (#743)

Daniel Purnomo created 2 months ago

What?

Added sanitizeFilename() function and applied it to the attachment download path in main.go. The function:

    Strips path components via filepath.Base()
    Replaces remaining path separators (/, \) and .. sequences with _
    Rejects hidden files (names starting with .) and empty names, falling back to "attachment"

Why?

Fixes #725
Fixes #642

Attachment filenames come from untrusted email headers. Without sanitization, a malicious filename like ../../../etc/passwd passed to filepath.Join(downloadsPath, candidate) could write files outside the Downloads directory. This is a path traversal vulnerability that could overwrite arbitrary user files.
Testing

    16 table-driven test cases covering Unix traversal, Windows traversal, hidden files, empty names, unicode, absolute paths, and chained traversal
    Additional test verifying no sanitized output ever contains path separators
    go build . — compiles clean
    All tests pass

Change summary

main.go      | 21 +++++++++++++++++
main_test.go | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 1 deletion(-)

Detailed changes

main.go

@@ -2568,6 +2568,25 @@ func moveEmailToFolderCmd(account *config.Account, uid uint32, accountID string,
 	}
 }
 
+// sanitizeFilename prevents path traversal attacks on attachment downloads.
+// Email attachment filenames come from untrusted email headers and could
+// contain path separators or ".." sequences to escape the Downloads directory.
+func sanitizeFilename(name string) string {
+	// Normalize backslashes to forward slashes so filepath.Base works
+	// correctly on all platforms (Linux doesn't treat \ as a separator)
+	name = strings.ReplaceAll(name, "\\", "/")
+	// Strip any path components, keep only the base filename
+	name = filepath.Base(name)
+	// Replace any remaining path separators (defensive)
+	name = strings.ReplaceAll(name, "/", "_")
+	name = strings.ReplaceAll(name, "..", "_")
+	// Reject hidden files and empty names
+	if name == "" || name == "." || strings.HasPrefix(name, ".") {
+		name = "attachment"
+	}
+	return name
+}
+
 func downloadAttachmentCmd(account *config.Account, uid uint32, msg tui.DownloadAttachmentMsg) tea.Cmd {
 	return func() tea.Msg {
 		// Download and decode the attachment using encoding provided in msg.Encoding.
@@ -2600,7 +2619,7 @@ func downloadAttachmentCmd(account *config.Account, uid uint32, msg tui.Download
 
 		// Save the attachment using an exclusive create so we never overwrite an existing file.
 		// If the filename already exists, append \" (n)\" before the extension.
-		origName := msg.Filename
+		origName := sanitizeFilename(msg.Filename)
 		ext := filepath.Ext(origName)
 		base := strings.TrimSuffix(origName, ext)
 		candidate := origName

main_test.go

@@ -0,0 +1,63 @@
+package main
+
+import (
+	"testing"
+)
+
+func TestSanitizeFilename(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{"normal filename", "document.pdf", "document.pdf"},
+		{"filename with spaces", "my report.docx", "my report.docx"},
+		{"unix path traversal", "../../../etc/passwd", "passwd"},
+		{"windows path traversal", "..\\..\\..\\windows\\system32\\config", "config"},
+		{"mixed traversal", "../secret/key.pem", "key.pem"},
+		{"hidden file", ".bashrc", "attachment"},
+		{"dot only", ".", "attachment"},
+		{"dot dot", "..", "_"},
+		{"empty string", "", "attachment"},
+		{"absolute unix path", "/etc/shadow", "shadow"},
+		{"absolute windows path", "C:\\Users\\secret.txt", "secret.txt"},
+		{"double dot in middle", "file..name.txt", "file_name.txt"},
+		{"multiple slashes", "path/to/file.txt", "file.txt"},
+		{"null bytes removed", "file\x00name.txt", "file\x00name.txt"},
+		{"unicode filename", "日本語.txt", "日本語.txt"},
+		{"long traversal chain", "a/b/c/../../../d/e/f.txt", "f.txt"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := sanitizeFilename(tt.input)
+			if got != tt.expected {
+				t.Errorf("sanitizeFilename(%q) = %q, want %q", tt.input, got, tt.expected)
+			}
+			// Verify the sanitized name never allows escaping the download directory
+			if got == "" {
+				t.Error("sanitizeFilename returned empty string")
+			}
+			if got == "." || got == ".." {
+				t.Error("sanitizeFilename returned a dangerous name: " + got)
+			}
+		})
+	}
+}
+
+func TestSanitizeFilenameNoPathSeparators(t *testing.T) {
+	// Ensure no sanitized output contains path separators
+	dangerous := []string{
+		"a/b", "a\\b", "../a", "..\\a",
+		"/etc/passwd", "\\Windows\\System32",
+		"....//....//etc/passwd",
+	}
+	for _, input := range dangerous {
+		got := sanitizeFilename(input)
+		for _, ch := range got {
+			if ch == '/' || ch == '\\' {
+				t.Errorf("sanitizeFilename(%q) = %q contains path separator", input, got)
+			}
+		}
+	}
+}