Fix path traversal bug in attachments (#60)

The export-attachment command's --output flag passes user-supplied paths
directly to os.OpenFile without validation. The command's own
documentation shows a scripted workflow where email-derived attachment
filenames are piped into -o:

  jq -r '.attachments[] | "\(.content_hash)\t\(.filename)"' | \
    while IFS=$'\t' read -r hash name; do
      msgvault export-attachment "$hash" -o "$name"
    done

An email attachment named ../../.ssh/authorized_keys would write outside
the working directory.

  The Fix (3 lines of logic)

Added ValidateOutputPath() to internal/export/attachments.go — it cleans
the path with filepath.Clean() and rejects relative paths that start
with .. (meaning they escape the working directory after normalization).
Absolute paths are allowed since they represent an explicit user choice.

The validation is called early in runExportAttachment before any file
I/O.

  Changes Made

- internal/export/attachments.go: Added ValidateOutputPath() function (7
lines)
- internal/export/attachments_test.go: Added TestValidateOutputPath with
8 test cases
- cmd/msgvault/cmd/export_attachment.go: Added validation call before
file operations (5 lines)

---------

Co-authored-by: Wes McKinney <wesmckinn+git@gmail.com>
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

Author: 3 people

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 := `{