Fix path traversal bug in attachments

cmd/msgvault/cmd/export_attachment.go

@@ -64,6 +64,13 @@ func runExportAttachment(cmd *cobra.Command, args []string) error {
 		}
 	}
 
+	// Validate output path before doing any work
+	if exportAttachmentOutput != "" && exportAttachmentOutput != "-" {
+		if err := export.ValidateOutputPath(exportAttachmentOutput); err != nil {
+			return err
+		}
+	}
+
 	// Construct storage path: attachmentsDir/hash[:2]/hash
 	attachmentsDir := cfg.AttachmentsDir()
 	storagePath := filepath.Join(attachmentsDir, contentHash[:2], contentHash)

internal/export/attachments.go

@@ -197,6 +197,32 @@ func SanitizeFilename(s string) string {
 	return string(result)
 }
 
+// ValidateOutputPath checks that an output file path does not escape the
+// current working directory. This guards against email-supplied filenames
+// being passed to --output (e.g., an attachment named
+// "../../.ssh/authorized_keys" or "/etc/cron.d/evil").
+// Both absolute paths and ".." traversal are rejected.
+func ValidateOutputPath(outputPath string) error {
+	cleaned := filepath.Clean(outputPath)
+	if filepath.IsAbs(cleaned) {
+		return fmt.Errorf("output path %q is absolute; use a relative path", outputPath)
+	}
+	// Reject Windows drive-relative (C:foo) and UNC (\\server\share) paths,
+	// which filepath.IsAbs does not catch.
+	if filepath.VolumeName(cleaned) != "" {
+		return fmt.Errorf("output path %q contains a drive or UNC prefix; use a relative path", outputPath)
+	}
+	// Reject rooted paths (leading / or \) which are drive-relative on Windows
+	// and absolute on Unix. filepath.IsAbs misses these on Windows.
+	if len(cleaned) > 0 && (cleaned[0] == '/' || cleaned[0] == '\\') {
+		return fmt.Errorf("output path %q is rooted; use a relative path", outputPath)
+	}
+	if cleaned == ".." || strings.HasPrefix(cleaned, ".."+string(filepath.Separator)) {
+		return fmt.Errorf("output path %q escapes the working directory", outputPath)
+	}
+	return nil
+}
+
 // StoragePath returns the content-addressed file path for an attachment:
 // attachmentsDir/<hash[:2]>/<hash>. Returns an error if the content hash
 // is invalid (prevents panics from short/empty strings).

internal/export/attachments_test.go

@@ -393,6 +393,67 @@ func TestCreateExclusiveFile(t *testing.T) {
 	})
 }
 
+func TestValidateOutputPath(t *testing.T) {
+	tests := []struct {
+		name    string
+		path    string
+		wantErr bool
+	}{
+		{"simple filename", "invoice.pdf", false},
+		{"filename with dot prefix", "./invoice.pdf", false},
+		{"subdirectory", "subdir/file.pdf", false},
+		{"stdout dash", "-", false},
+		{"dot-dot prefix filename", "..backup", false},
+		{"dot-dot middle filename", "foo..bar.txt", false},
+
+		// Rooted/absolute paths (could be malicious attachment names)
+		{"absolute unix path", "/tmp/file.pdf", true},
+		{"absolute path with traversal", "/etc/cron.d/evil", true},
+		{"backslash rooted path", `\tmp\file.pdf`, true},
+
+		// Path traversal attacks (e.g., from email-supplied filenames)
+		{"parent traversal", "../evil.txt", true},
+		{"deep traversal", "../../.ssh/authorized_keys", true},
+		{"traversal via subdir", "foo/../../evil.txt", true},
+	}
+
+	// Windows drive and UNC paths — only meaningful on Windows where
+	// filepath.VolumeName returns non-empty for these forms.
+	if runtime.GOOS == "windows" {
+		tests = append(tests,
+			struct {
+				name    string
+				path    string
+				wantErr bool
+			}{"windows absolute", `C:\tmp\file.pdf`, true},
+			struct {
+				name    string
+				path    string
+				wantErr bool
+			}{"windows drive-relative", `C:tmp\file.pdf`, true},
+			struct {
+				name    string
+				path    string
+				wantErr bool
+			}{"windows drive-relative traversal", `C:..\evil`, true},
+			struct {
+				name    string
+				path    string
+				wantErr bool
+			}{"windows UNC path", `\\server\share\file.pdf`, true},
+		)
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateOutputPath(tt.path)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ValidateOutputPath(%q) error = %v, wantErr %v", tt.path, err, tt.wantErr)
+			}
+		})
+	}
+}
+
 func TestAttachments(t *testing.T) {
 	tests := []struct {
 		name           string

internal/oauth/oauth.go

@@ -295,7 +295,34 @@ func (m *Manager) saveToken(email string, token *oauth2.Token, scopes []string)
 	}
 
 	path := m.tokenPath(email)
-	return os.WriteFile(path, data, 0600)
+
+	// Atomic write via temp file + rename to avoid TOCTOU symlink races.
+	// If an attacker creates a symlink between tokenPath() returning and
+	// the write, os.Rename replaces the symlink itself rather than following it.
+	tmpFile, err := os.CreateTemp(m.tokensDir, ".token-*.tmp")
+	if err != nil {
+		return fmt.Errorf("create temp token file: %w", err)
+	}
+	tmpPath := tmpFile.Name()
+
+	if _, err := tmpFile.Write(data); err != nil {
+		tmpFile.Close()
+		os.Remove(tmpPath)
+		return fmt.Errorf("write temp token file: %w", err)
+	}
+	if err := tmpFile.Close(); err != nil {
+		os.Remove(tmpPath)
+		return fmt.Errorf("close temp token file: %w", err)
+	}
+	if err := os.Chmod(tmpPath, 0600); err != nil {
+		os.Remove(tmpPath)
+		return fmt.Errorf("chmod temp token file: %w", err)
+	}
+	if err := os.Rename(tmpPath, path); err != nil {
+		os.Remove(tmpPath)
+		return fmt.Errorf("rename temp token file: %w", err)
+	}
+	return nil
 }
 
 // tokenPath returns the path to the token file for an email.
@@ -333,14 +360,23 @@ func (m *Manager) tokenPath(email string) string {
 	return cleanPath
 }
 
-// hasPathPrefix checks if path starts with dir using proper separator handling.
-// This prevents prefix attacks like tokensDir-evil matching tokensDir.
+// hasPathPrefix checks if path is equal to or a child of dir.
+// This prevents prefix attacks like tokensDir-evil matching tokensDir,
+// and correctly handles filesystem roots (/, C:\).
 // Does not resolve symlinks - use isPathWithinDir when symlink resolution is needed.
 func hasPathPrefix(path, dir string) bool {
-	cleanPath := filepath.Clean(path)
-	cleanDir := filepath.Clean(dir)
-	return strings.HasPrefix(cleanPath, cleanDir+string(filepath.Separator)) ||
-		cleanPath == cleanDir
+	rel, err := filepath.Rel(dir, path)
+	if err != nil {
+		return false
+	}
+	// rel must not escape via ".." and must not be absolute
+	if rel == "." {
+		return true
+	}
+	if filepath.IsAbs(rel) {
+		return false
+	}
+	return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))
 }
 
 // isPathWithinDir checks if path is within dir, resolving symlinks in dir.
@@ -351,14 +387,7 @@ func isPathWithinDir(path, dir string) bool {
 	if err != nil {
 		resolvedDir = dir // fallback to original if dir doesn't exist yet
 	}
-	resolvedDir = filepath.Clean(resolvedDir)
-
-	// Clean and check the path
-	cleanPath := filepath.Clean(path)
-
-	// Ensure path is within dir (with proper separator handling)
-	return strings.HasPrefix(cleanPath, resolvedDir+string(filepath.Separator)) ||
-		cleanPath == resolvedDir
+	return hasPathPrefix(filepath.Clean(path), filepath.Clean(resolvedDir))
 }
 
 // scopesToString joins scopes with spaces.

internal/oauth/oauth_test.go

@@ -8,6 +8,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -165,6 +166,37 @@ func TestTokenFileScopesRoundTrip(t *testing.T) {
 	}
 }
 
+func TestSaveToken_OverwriteExisting(t *testing.T) {
+	mgr := setupTestManager(t, Scopes)
+
+	token1 := &oauth2.Token{
+		AccessToken:  "first",
+		RefreshToken: "refresh1",
+		TokenType:    "Bearer",
+	}
+	if err := mgr.saveToken("test@gmail.com", token1, Scopes); err != nil {
+		t.Fatal(err)
+	}
+
+	// Save again with a different access token — must overwrite (not fail).
+	token2 := &oauth2.Token{
+		AccessToken:  "second",
+		RefreshToken: "refresh2",
+		TokenType:    "Bearer",
+	}
+	if err := mgr.saveToken("test@gmail.com", token2, Scopes); err != nil {
+		t.Fatalf("second saveToken should overwrite existing file: %v", err)
+	}
+
+	loaded, err := mgr.loadToken("test@gmail.com")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if loaded.AccessToken != "second" {
+		t.Errorf("expected access token 'second' after overwrite, got %q", loaded.AccessToken)
+	}
+}
+
 func TestHasScope_LegacyToken(t *testing.T) {
 	mgr := setupTestManager(t, Scopes)
 
@@ -299,6 +331,54 @@ func TestTokenPath_SymlinkEscape(t *testing.T) {
 	}
 }
 
+func TestHasPathPrefix(t *testing.T) {
+	tests := []struct {
+		name string
+		path string
+		dir  string
+		want bool
+	}{
+		{"child path", "/a/b/c", "/a/b", true},
+		{"exact match", "/a/b", "/a/b", true},
+		{"prefix attack", "/a/b-evil/c", "/a/b", false},
+		{"sibling", "/a/c", "/a/b", false},
+		{"parent escape", "/a", "/a/b", false},
+		{"root dir child", "/foo", "/", true},
+		{"root dir exact", "/", "/", true},
+		{"unrelated", "/x/y", "/a/b", false},
+		{"dotdot prefix child", "/a/b/..backup", "/a/b", true},
+	}
+
+	// Add Windows drive-root cases when running on Windows.
+	if runtime.GOOS == "windows" {
+		vol := filepath.VolumeName(os.TempDir())
+		root := vol + string(filepath.Separator)
+		tests = append(tests,
+			struct {
+				name string
+				path string
+				dir  string
+				want bool
+			}{"windows drive root exact", root, root, true},
+			struct {
+				name string
+				path string
+				dir  string
+				want bool
+			}{"windows drive root child", root + "Users", root, true},
+		)
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := hasPathPrefix(tt.path, tt.dir)
+			if got != tt.want {
+				t.Errorf("hasPathPrefix(%q, %q) = %v, want %v", tt.path, tt.dir, got, tt.want)
+			}
+		})
+	}
+}
+
 func TestParseClientSecrets(t *testing.T) {
 	// Valid Desktop application credentials
 	validDesktop := `{