From fe3db9bef088c7aeb57195eae31bc50ae8b4d26e Mon Sep 17 00:00:00 2001 From: ambigois Date: Tue, 10 Feb 2026 10:24:58 +0000 Subject: [PATCH] feat: add file drag-and-drop support for images and PDFs - Add ParsePastedFiles() to detect file paths from paste events - Support Unix (space-separated with escaping) and Windows (quoted) formats - Validate file types (PNG, JPG, GIF, WebP, BMP, SVG, PDF) - Enforce 5MB size limit per file (>= boundary) - Add visual file type indicators with emoji icons - Integrate with existing attachment system - Handle edge cases (null chars, trailing backslash, etc.) - Add comprehensive test coverage Security hardening: - Add validateFilePath() to reject path traversal (..) and symlinks - Check for '..' BEFORE filepath.Clean() to prevent bypass - Use os.Lstat() instead of os.Stat() in all file validation paths - Return errors from addFileAttachment() and AttachFile() - Return error notification on failed attach instead of opening file picker - Eliminate TOCTOU by removing redundant allFilesValid() pre-check; AttachFile() is the single validation point with rollback on failure - Strip trailing backslash as malformed input - Add TestValidateFilePath, TestValidateFilePath_TraversalBeforeClean, and TestAddFileAttachment_SizeLimit Assisted-By: cagent --- pkg/tui/components/editor/editor.go | 83 +++- pkg/tui/components/editor/paste.go | 228 +++++++++++ pkg/tui/components/editor/paste_test.go | 502 ++++++++++++++++++++++++ pkg/tui/handlers.go | 22 +- pkg/tui/page/chat/chat.go | 8 +- 5 files changed, 818 insertions(+), 25 deletions(-) create mode 100644 pkg/tui/components/editor/paste.go diff --git a/pkg/tui/components/editor/editor.go b/pkg/tui/components/editor/editor.go index 78d11dc5d..93a145fe6 100644 --- a/pkg/tui/components/editor/editor.go +++ b/pkg/tui/components/editor/editor.go @@ -73,7 +73,7 @@ type Editor interface { // InsertText inserts text at the current cursor position InsertText(text string) // AttachFile adds a file as an attachment and inserts @filepath into the editor - AttachFile(filePath string) + AttachFile(filePath string) error Cleanup() GetSize() (width, height int) BannerHeight() int @@ -690,7 +690,9 @@ func (e *editor) Update(msg tea.Msg) (layout.Model, tea.Cmd) { } // Track file references when using @ completion (but not paste placeholders) if e.currentCompletion != nil && e.currentCompletion.Trigger() == "@" && !strings.HasPrefix(msg.Value, "@paste-") { - e.addFileAttachment(msg.Value) + if err := e.addFileAttachment(msg.Value); err != nil { + slog.Warn("failed to add file attachment from completion", "value", msg.Value, "error", err) + } } e.clearSuggestion() return e, nil @@ -1287,14 +1289,17 @@ func (e *editor) InsertText(text string) { } // AttachFile adds a file as an attachment and inserts @filepath into the editor -func (e *editor) AttachFile(filePath string) { +func (e *editor) AttachFile(filePath string) error { placeholder := "@" + filePath - e.addFileAttachment(placeholder) + if err := e.addFileAttachment(placeholder); err != nil { + return fmt.Errorf("failed to attach %s: %w", filePath, err) + } currentValue := e.textarea.Value() e.textarea.SetValue(currentValue + placeholder + " ") e.textarea.MoveToEnd() e.userTyped = true e.updateAttachmentBanner() + return nil } // tryAddFileRef checks if word is a valid @filepath and adds it as attachment. @@ -1315,33 +1320,41 @@ func (e *editor) tryAddFileRef(word string) { return // not a path-like reference (e.g., @username) } - e.addFileAttachment(word) + if err := e.addFileAttachment(word); err != nil { + slog.Debug("speculative file ref not valid", "word", word, "error", err) + } } // addFileAttachment adds a file reference as an attachment if valid. // The path is resolved to an absolute path so downstream consumers // (e.g. processFileAttachment) always receive a fully qualified path. -func (e *editor) addFileAttachment(placeholder string) { +func (e *editor) addFileAttachment(placeholder string) error { path := strings.TrimPrefix(placeholder, "@") // Resolve to absolute path so the attachment carries a fully qualified // path regardless of the working directory at send time. absPath, err := filepath.Abs(path) if err != nil { - slog.Warn("skipping attachment: cannot resolve path", "path", path, "error", err) - return + return fmt.Errorf("cannot resolve path %s: %w", path, err) } - // Check if it's an existing file (not directory) - info, err := os.Stat(absPath) - if err != nil || info.IsDir() { - return + info, err := validateFilePath(absPath) + if err != nil { + return fmt.Errorf("invalid file path %s: %w", absPath, err) + } + if info.IsDir() { + return fmt.Errorf("path is a directory: %s", absPath) + } + + const maxFileSize = 5 * 1024 * 1024 + if info.Size() >= maxFileSize { + return fmt.Errorf("file too large: %s (%s)", absPath, units.HumanSize(float64(info.Size()))) } // Avoid duplicates for _, att := range e.attachments { if att.placeholder == placeholder { - return + return nil } } @@ -1352,6 +1365,7 @@ func (e *editor) addFileAttachment(placeholder string) { sizeBytes: int(info.Size()), isTemp: false, }) + return nil } // collectAttachments returns structured attachments for all items referenced in @@ -1451,6 +1465,28 @@ func (e *editor) SendContent() tea.Cmd { } func (e *editor) handlePaste(content string) bool { + // First, try to parse as file paths (drag-and-drop) + filePaths := ParsePastedFiles(content) + if len(filePaths) > 0 { + var attached int + for _, path := range filePaths { + if !IsSupportedFileType(path) { + break + } + if err := e.AttachFile(path); err != nil { + slog.Debug("paste path not attachable, treating as text", "path", path, "error", err) + break + } + attached++ + } + if attached == len(filePaths) { + return true + } + // Not all files could be attached; undo partial attachments and fall through to text paste + e.removeLastNAttachments(attached) + } + + // Not file paths, handle as text paste // Count lines (newlines + 1 for content without trailing newline) lines := strings.Count(content, "\n") + 1 if strings.HasSuffix(content, "\n") { @@ -1477,6 +1513,27 @@ func (e *editor) handlePaste(content string) bool { return true } +// removeLastNAttachments removes the last n non-temp attachments and their +// placeholder text from the textarea. Used to roll back partial file-drop +// attachments when not all files in a paste are valid. +func (e *editor) removeLastNAttachments(n int) { + if n <= 0 { + return + } + value := e.textarea.Value() + removed := 0 + for i := len(e.attachments) - 1; i >= 0 && removed < n; i-- { + if !e.attachments[i].isTemp { + // Strip the placeholder text ("@/path/file.png ") that AttachFile inserted + value = strings.Replace(value, e.attachments[i].placeholder+" ", "", 1) + e.attachments = append(e.attachments[:i], e.attachments[i+1:]...) + removed++ + } + } + e.textarea.SetValue(value) + e.textarea.MoveToEnd() +} + func (e *editor) updateAttachmentBanner() { if e.banner == nil { return diff --git a/pkg/tui/components/editor/paste.go b/pkg/tui/components/editor/paste.go new file mode 100644 index 000000000..9c7d3a724 --- /dev/null +++ b/pkg/tui/components/editor/paste.go @@ -0,0 +1,228 @@ +package editor + +import ( + "os" + "path/filepath" + "slices" + "strings" +) + +// validateFilePath checks that a path is safe: no path traversal, no symlinks. +func validateFilePath(path string) (os.FileInfo, error) { + if strings.Contains(path, "..") { + return nil, os.ErrPermission + } + + clean := filepath.Clean(path) + + info, err := os.Lstat(clean) + if err != nil { + return nil, err + } + if info.Mode()&os.ModeSymlink != 0 { + return nil, os.ErrPermission + } + return info, nil +} + +// Supported file extensions for drag-and-drop attachments +var supportedFileExtensions = []string{ + // Images + ".png", ".jpg", ".jpeg", ".gif", ".webp", ".bmp", ".svg", + // PDFs + ".pdf", + // Text files (future) + // ".txt", ".md", ".json", ".yaml", ".yml", ".toml", +} + +// ParsePastedFiles attempts to parse pasted content as file paths. +// It handles different terminal formats: +// - Unix: space-separated with backslash escaping +// - Windows Terminal: quote-wrapped paths +// - Single file: just the path +// +// Returns nil if the content doesn't look like file paths. +func ParsePastedFiles(s string) []string { + s = strings.TrimSpace(s) + if s == "" { + return nil + } + + // NOTE: Rio terminal on Windows adds NULL chars for some reason. + s = strings.ReplaceAll(s, "\x00", "") + + // Try simple stat first - if all lines are valid files, use them + if attemptStatAll(s) { + return strings.Split(s, "\n") + } + + // Detect Windows Terminal format (quote-wrapped) + if os.Getenv("WT_SESSION") != "" { + return windowsTerminalParsePastedFiles(s) + } + + // Default to Unix format (space-separated with backslash escaping) + return unixParsePastedFiles(s) +} + +// attemptStatAll tries to stat each line as a file path. +// Returns true if ALL lines exist as regular files (not directories or symlinks). +func attemptStatAll(s string) bool { + lines := strings.Split(s, "\n") + if len(lines) == 0 { + return false + } + + for _, line := range lines { + line = strings.TrimSpace(line) + if line == "" { + continue + } + info, err := validateFilePath(line) + if err != nil || info.IsDir() { + return false + } + } + return true +} + +// windowsTerminalParsePastedFiles parses Windows Terminal format. +// Windows Terminal wraps file paths in quotes: "C:\path\to\file.png" +func windowsTerminalParsePastedFiles(s string) []string { + if strings.TrimSpace(s) == "" { + return nil + } + + var ( + paths []string + current strings.Builder + inQuotes = false + ) + + for i := range len(s) { + ch := s[i] + + switch { + case ch == '"': + if inQuotes { + // End of quoted section + if current.Len() > 0 { + paths = append(paths, current.String()) + current.Reset() + } + inQuotes = false + } else { + // Start of quoted section + inQuotes = true + } + case inQuotes: + current.WriteByte(ch) + case ch != ' ' && ch != '\n' && ch != '\r': + // Text outside quotes is not allowed + return nil + } + } + + // Add any remaining content if quotes were properly closed + if current.Len() > 0 && !inQuotes { + paths = append(paths, current.String()) + } + + // If quotes were not closed, return nil (malformed input) + if inQuotes { + return nil + } + + return paths +} + +// unixParsePastedFiles parses Unix terminal format. +// Unix terminals use space-separated paths with backslash escaping. +// Example: /path/to/file1.png /path/to/my\ file\ with\ spaces.jpg +func unixParsePastedFiles(s string) []string { + if strings.TrimSpace(s) == "" { + return nil + } + + var ( + paths []string + current strings.Builder + escaped = false + ) + + for i := range len(s) { + ch := s[i] + + switch { + case escaped: + // After a backslash, add the character as-is (including space) + current.WriteByte(ch) + escaped = false + case ch == '\\': + if i == len(s)-1 { + // Trailing backslash is malformed input; strip it + break + } + escaped = true + case ch == ' ' || ch == '\n' || ch == '\r': + // Space/newline separates paths (unless escaped) + if current.Len() > 0 { + paths = append(paths, current.String()) + current.Reset() + } + default: + current.WriteByte(ch) + } + } + + // Handle trailing backslash if present + if escaped { + current.WriteByte('\\') + } + + // Add the last path if any + if current.Len() > 0 { + paths = append(paths, current.String()) + } + + return paths +} + +// IsSupportedFileType checks if a file has a supported extension. +func IsSupportedFileType(path string) bool { + ext := strings.ToLower(filepath.Ext(path)) + return slices.Contains(supportedFileExtensions, ext) +} + +// GetFileType returns a human-readable file type for display. +func GetFileType(path string) string { + ext := strings.ToLower(filepath.Ext(path)) + switch ext { + case ".png", ".jpg", ".jpeg", ".gif", ".webp", ".bmp", ".svg": + return "image" + case ".pdf": + return "pdf" + case ".txt", ".md": + return "text" + case ".json", ".yaml", ".yml", ".toml": + return "config" + default: + return "file" + } +} + +// GetFileIcon returns an emoji icon for a file type. +func GetFileIcon(fileType string) string { + switch fileType { + case "image": + return "🖼️" + case "pdf": + return "📄" + case "text": + return "📝" + case "config": + return "⚙️" + default: + return "📎" + } +} diff --git a/pkg/tui/components/editor/paste_test.go b/pkg/tui/components/editor/paste_test.go index 0ca257e3f..c6c3d374b 100644 --- a/pkg/tui/components/editor/paste_test.go +++ b/pkg/tui/components/editor/paste_test.go @@ -1,11 +1,13 @@ package editor import ( + "fmt" "os" "path/filepath" "strings" "testing" + "charm.land/bubbles/v2/textarea" "github.com/docker/go-units" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -216,3 +218,503 @@ func createPasteAttachmentInDir(dir, content string) (attachment, error) { isTemp: true, }, nil } + +// File path parsing tests for drag-and-drop feature +func TestParsePastedFiles(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + expected []string + }{ + { + name: "empty string", + input: "", + expected: nil, + }, + { + name: "single file path", + input: "/path/to/file.png", + expected: []string{"/path/to/file.png"}, + }, + { + name: "multiple space-separated", + input: "/path/to/file1.png /path/to/file2.jpg", + expected: []string{"/path/to/file1.png", "/path/to/file2.jpg"}, + }, + { + name: "escaped spaces (Unix)", + input: `/path/to/my\ file.png`, + expected: []string{"/path/to/my file.png"}, + }, + { + name: "multiple with escaped spaces", + input: `/path/to/file\ 1.png /path/to/file\ 2.jpg`, + expected: []string{"/path/to/file 1.png", "/path/to/file 2.jpg"}, + }, + { + name: "newline separated", + input: "/path/to/file1.png\n/path/to/file2.jpg", + expected: []string{"/path/to/file1.png", "/path/to/file2.jpg"}, + }, + { + name: "trailing backslash", + input: "/path/to/file.png\\", + expected: []string{"/path/to/file.png"}, + }, + { + name: "null chars removed", + input: "/path/to/file\x00.png", + expected: []string{"/path/to/file.png"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := ParsePastedFiles(tt.input) + if len(result) != len(tt.expected) { + t.Errorf("ParsePastedFiles() got %d paths, expected %d", len(result), len(tt.expected)) + t.Errorf(" got: %v", result) + t.Errorf(" expected: %v", tt.expected) + return + } + for i, path := range result { + if path != tt.expected[i] { + t.Errorf("ParsePastedFiles() path[%d] = %q, expected %q", i, path, tt.expected[i]) + } + } + }) + } +} + +func TestWindowsTerminalParsePastedFiles(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + expected []string + }{ + { + name: "single quoted path", + input: `"C:\path\to\file.png"`, + expected: []string{`C:\path\to\file.png`}, + }, + { + name: "multiple quoted paths", + input: `"C:\path\to\file1.png" "C:\path\to\file2.jpg"`, + expected: []string{`C:\path\to\file1.png`, `C:\path\to\file2.jpg`}, + }, + { + name: "unclosed quotes", + input: `"C:\path\to\file.png`, + expected: nil, + }, + { + name: "text outside quotes", + input: `"C:\path\to\file.png" extra`, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := windowsTerminalParsePastedFiles(tt.input) + if len(result) != len(tt.expected) { + t.Errorf("windowsTerminalParsePastedFiles() got %d paths, expected %d", len(result), len(tt.expected)) + t.Errorf(" got: %v", result) + t.Errorf(" expected: %v", tt.expected) + return + } + for i, path := range result { + if path != tt.expected[i] { + t.Errorf("windowsTerminalParsePastedFiles() path[%d] = %q, expected %q", i, path, tt.expected[i]) + } + } + }) + } +} + +func TestIsSupportedFileType(t *testing.T) { + t.Parallel() + + tests := []struct { + path string + expected bool + }{ + {"/path/to/image.png", true}, + {"/path/to/image.jpg", true}, + {"/path/to/image.JPEG", true}, // Case insensitive + {"/path/to/doc.pdf", true}, + {"/path/to/file.txt", false}, // Not supported yet + {"/path/to/script.sh", false}, + {"/path/to/noext", false}, + } + + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + t.Parallel() + result := IsSupportedFileType(tt.path) + if result != tt.expected { + t.Errorf("IsSupportedFileType(%q) = %v, expected %v", tt.path, result, tt.expected) + } + }) + } +} + +func TestGetFileType(t *testing.T) { + t.Parallel() + + tests := []struct { + path string + expected string + }{ + {"/path/to/image.png", "image"}, + {"/path/to/image.JPG", "image"}, + {"/path/to/doc.pdf", "pdf"}, + {"/path/to/readme.md", "text"}, + {"/path/to/config.json", "config"}, + {"/path/to/unknown.xyz", "file"}, + } + + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + t.Parallel() + result := GetFileType(tt.path) + if result != tt.expected { + t.Errorf("GetFileType(%q) = %q, expected %q", tt.path, result, tt.expected) + } + }) + } +} + +func TestGetFileIcon(t *testing.T) { + t.Parallel() + + tests := []struct { + fileType string + expected string + }{ + {"image", "🖼️"}, + {"pdf", "📄"}, + {"text", "📝"}, + {"config", "⚙️"}, + {"unknown", "📎"}, + } + + for _, tt := range tests { + t.Run(tt.fileType, func(t *testing.T) { + t.Parallel() + result := GetFileIcon(tt.fileType) + if result != tt.expected { + t.Errorf("GetFileIcon(%q) = %q, expected %q", tt.fileType, result, tt.expected) + } + }) + } +} + +func TestParsePastedFilesWithRealFiles(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + file1 := filepath.Join(tmpDir, "test1.png") + file2 := filepath.Join(tmpDir, "test2.jpg") + + if err := os.WriteFile(file1, []byte("fake png"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(file2, []byte("fake jpg"), 0o644); err != nil { + t.Fatal(err) + } + + input := file1 + "\n" + file2 + result := ParsePastedFiles(input) + + if len(result) != 2 { + t.Errorf("Expected 2 paths, got %d", len(result)) + } + if len(result) >= 2 { + if result[0] != file1 { + t.Errorf("Expected first path to be %q, got %q", file1, result[0]) + } + if result[1] != file2 { + t.Errorf("Expected second path to be %q, got %q", file2, result[1]) + } + } +} + +func TestValidateFilePath(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + regularFile := filepath.Join(tmpDir, "regular.txt") + if err := os.WriteFile(regularFile, []byte("content"), 0o644); err != nil { + t.Fatal(err) + } + + symlink := filepath.Join(tmpDir, "symlink.txt") + if err := os.Symlink(regularFile, symlink); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + path string + wantErr bool + }{ + {"regular file", regularFile, false}, + {"symlink rejected", symlink, true}, + {"path traversal rejected", filepath.Join(tmpDir, "..", "etc", "passwd"), true}, + {"nonexistent file", filepath.Join(tmpDir, "nonexistent.txt"), true}, + {"directory", tmpDir, false}, // validateFilePath itself doesn't reject dirs; callers do + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, err := validateFilePath(tt.path) + if (err != nil) != tt.wantErr { + t.Errorf("validateFilePath(%q) error = %v, wantErr %v", tt.path, err, tt.wantErr) + } + }) + } +} + +func TestValidateFilePath_TraversalBeforeClean(t *testing.T) { + t.Parallel() + + // This is the specific exploit the reviewer flagged: + // filepath.Clean resolves ".." so checking after Clean is useless. + // We must reject before Clean. + _, err := validateFilePath("/tmp/app/../../../etc/passwd") + if err == nil { + t.Error("expected validateFilePath to reject path traversal, but it succeeded") + } +} + +func TestAddFileAttachment_SizeLimit(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + // Exactly 5MB — should be rejected (>= 5MB) + exactly5MB := filepath.Join(tmpDir, "exact5mb.png") + if err := os.WriteFile(exactly5MB, make([]byte, 5*1024*1024), 0o644); err != nil { + t.Fatal(err) + } + + // Just under 5MB — should be accepted + justUnder := filepath.Join(tmpDir, "under5mb.png") + if err := os.WriteFile(justUnder, make([]byte, 5*1024*1024-1), 0o644); err != nil { + t.Fatal(err) + } + + e := &editor{} + + err := e.addFileAttachment("@" + exactly5MB) + if err == nil { + t.Error("expected addFileAttachment to reject file exactly 5MB, but it succeeded") + } + + err = e.addFileAttachment("@" + justUnder) + if err != nil { + t.Errorf("expected addFileAttachment to accept file just under 5MB, got error: %v", err) + } +} + +func newPasteTestEditor() *editor { + ta := textarea.New() + ta.Focus() + return &editor{ + textarea: ta, + banner: newAttachmentBanner(), + } +} + +func TestHandlePaste_DragDropSingleFile(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + file := filepath.Join(tmpDir, "photo.png") + require.NoError(t, os.WriteFile(file, []byte("PNG"), 0o644)) + + e := newPasteTestEditor() + handled := e.handlePaste(file) + + assert.True(t, handled, "valid file path should be handled as drag-and-drop") + assert.Len(t, e.attachments, 1) + assert.Contains(t, e.textarea.Value(), "@"+file) +} + +func TestHandlePaste_DragDropMultipleFiles(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + file1 := filepath.Join(tmpDir, "a.png") + file2 := filepath.Join(tmpDir, "b.jpg") + require.NoError(t, os.WriteFile(file1, []byte("PNG"), 0o644)) + require.NoError(t, os.WriteFile(file2, []byte("JPG"), 0o644)) + + e := newPasteTestEditor() + handled := e.handlePaste(file1 + " " + file2) + + assert.True(t, handled) + assert.Len(t, e.attachments, 2) + assert.Contains(t, e.textarea.Value(), "@"+file1) + assert.Contains(t, e.textarea.Value(), "@"+file2) +} + +func TestHandlePaste_RollbackOnPartialFailure(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + goodFile := filepath.Join(tmpDir, "valid.png") + require.NoError(t, os.WriteFile(goodFile, []byte("PNG"), 0o644)) + + // Second file is too large (>= 5MB) + bigFile := filepath.Join(tmpDir, "huge.png") + require.NoError(t, os.WriteFile(bigFile, make([]byte, 5*1024*1024), 0o644)) + + e := newPasteTestEditor() + handled := e.handlePaste(goodFile + " " + bigFile) + + assert.False(t, handled, "should fall through to text paste when any file fails") + assert.Empty(t, e.attachments, "partial attachments should be rolled back") + assert.NotContains(t, e.textarea.Value(), "@"+goodFile, + "rolled-back placeholder text should be removed from textarea") +} + +func TestHandlePaste_UnsupportedTypeRollback(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + png := filepath.Join(tmpDir, "ok.png") + sh := filepath.Join(tmpDir, "script.sh") + require.NoError(t, os.WriteFile(png, []byte("PNG"), 0o644)) + require.NoError(t, os.WriteFile(sh, []byte("#!/bin/sh"), 0o644)) + + e := newPasteTestEditor() + handled := e.handlePaste(png + " " + sh) + + assert.False(t, handled, "unsupported file type should cause fallback to text") + assert.Empty(t, e.attachments, "no attachments when file type is unsupported") + assert.Empty(t, e.textarea.Value(), "textarea should be clean after rollback") +} + +func TestHandlePaste_SymlinkRejected(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + realFile := filepath.Join(tmpDir, "real.png") + link := filepath.Join(tmpDir, "link.png") + require.NoError(t, os.WriteFile(realFile, []byte("PNG"), 0o644)) + require.NoError(t, os.Symlink(realFile, link)) + + e := newPasteTestEditor() + handled := e.handlePaste(link) + + assert.False(t, handled, "symlink should be rejected") + assert.Empty(t, e.attachments) +} + +func TestHandlePaste_PathTraversalRejected(t *testing.T) { + t.Parallel() + + e := newPasteTestEditor() + handled := e.handlePaste("../../etc/passwd") + + assert.False(t, handled, "path traversal should be rejected") + assert.Empty(t, e.attachments) +} + +func TestRemoveLastNAttachments_CleansTextarea(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + file1 := filepath.Join(tmpDir, "a.png") + file2 := filepath.Join(tmpDir, "b.png") + require.NoError(t, os.WriteFile(file1, []byte("PNG"), 0o644)) + require.NoError(t, os.WriteFile(file2, []byte("PNG"), 0o644)) + + e := newPasteTestEditor() + require.NoError(t, e.AttachFile(file1)) + require.NoError(t, e.AttachFile(file2)) + + assert.Len(t, e.attachments, 2) + assert.Contains(t, e.textarea.Value(), "@"+file1) + assert.Contains(t, e.textarea.Value(), "@"+file2) + + // Roll back the last one + e.removeLastNAttachments(1) + + assert.Len(t, e.attachments, 1) + assert.Contains(t, e.textarea.Value(), "@"+file1, "first attachment should remain") + assert.NotContains(t, e.textarea.Value(), "@"+file2, "second attachment should be removed") + + // Roll back the remaining one + e.removeLastNAttachments(1) + + assert.Empty(t, e.attachments) + assert.Empty(t, strings.TrimSpace(e.textarea.Value()), "textarea should be empty after full rollback") +} + +func TestRemoveLastNAttachments_PreservesTempAttachments(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + file := filepath.Join(tmpDir, "file.png") + require.NoError(t, os.WriteFile(file, []byte("PNG"), 0o644)) + + e := newPasteTestEditor() + + // Simulate a temp (paste) attachment + e.attachments = append(e.attachments, attachment{ + path: "/tmp/paste-1.txt", + placeholder: "@paste-1", + isTemp: true, + }) + + // Add a real file attachment + require.NoError(t, e.AttachFile(file)) + assert.Len(t, e.attachments, 2) + + // Roll back 1 — should only remove the non-temp one + e.removeLastNAttachments(1) + + assert.Len(t, e.attachments, 1) + assert.True(t, e.attachments[0].isTemp, "temp attachment should be preserved") +} + +func TestAttachFile_DuplicateRejected(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + file := filepath.Join(tmpDir, "dup.png") + require.NoError(t, os.WriteFile(file, []byte("PNG"), 0o644)) + + e := newPasteTestEditor() + require.NoError(t, e.AttachFile(file)) + require.NoError(t, e.AttachFile(file)) // duplicate + + assert.Len(t, e.attachments, 1, "duplicate should not create second attachment") +} + +func TestAttachFile_SetsCorrectLabel(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + data := make([]byte, 1024) + file := filepath.Join(tmpDir, "labeled.png") + require.NoError(t, os.WriteFile(file, data, 0o644)) + + e := newPasteTestEditor() + require.NoError(t, e.AttachFile(file)) + + require.Len(t, e.attachments, 1) + expectedLabel := fmt.Sprintf("labeled.png (%s)", units.HumanSize(float64(len(data)))) + assert.Equal(t, expectedLabel, e.attachments[0].label) +} diff --git a/pkg/tui/handlers.go b/pkg/tui/handlers.go index 941f289ea..00ddbb433 100644 --- a/pkg/tui/handlers.go +++ b/pkg/tui/handlers.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "log/slog" - "os" "strings" tea "charm.land/bubbletea/v2" @@ -417,18 +416,23 @@ func (a *appModel) handleAgentCommand(command string) (tea.Model, tea.Cmd) { // File attachment handler func (a *appModel) handleAttachFile(filePath string) (tea.Model, tea.Cmd) { - // If a file path is provided and it's an existing file, attach it directly if filePath != "" { - info, err := os.Stat(filePath) - if err == nil && !info.IsDir() { - // Insert the file reference into the editor using @filepath syntax - updated, cmd := a.chatPage.Update(messages.InsertFileRefMsg{FilePath: filePath}) - a.chatPage = updated.(chat.Page) - return a, tea.Batch(cmd, notification.SuccessCmd("File attached: "+filePath)) + updated, cmd := a.chatPage.Update(messages.InsertFileRefMsg{FilePath: filePath}) + a.chatPage = updated.(chat.Page) + if cmd != nil { + // Attachment succeeded + return a, cmd } + // Attachment failed — fall through to open the file picker with an error notification + return a, tea.Batch( + notification.ErrorCmd(fmt.Sprintf("Failed to attach %s", filePath)), + core.CmdHandler(dialog.OpenDialogMsg{ + Model: dialog.NewFilePickerDialog(filePath), + }), + ) } - // Otherwise, open the file picker dialog + // No path provided — open the file picker dialog return a, core.CmdHandler(dialog.OpenDialogMsg{ Model: dialog.NewFilePickerDialog(filePath), }) diff --git a/pkg/tui/page/chat/chat.go b/pkg/tui/page/chat/chat.go index 2700525c2..e18b447ee 100644 --- a/pkg/tui/page/chat/chat.go +++ b/pkg/tui/page/chat/chat.go @@ -503,9 +503,11 @@ func (p *chatPage) Update(msg tea.Msg) (layout.Model, tea.Cmd) { return p.handleSendMsg(msg) case msgtypes.InsertFileRefMsg: - // Attach file using editor's AttachFile method which registers the attachment - p.editor.AttachFile(msg.FilePath) - return p, nil + if err := p.editor.AttachFile(msg.FilePath); err != nil { + slog.Warn("failed to attach file", "path", msg.FilePath, "error", err) + return p, nil + } + return p, notification.SuccessCmd("File attached: " + msg.FilePath) case msgtypes.ToggleHideToolResultsMsg: // Forward to messages component to invalidate cache and trigger redraw