feat: add file drag-and-drop support for images and PDFs #1658
+818
−25
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. 🔴 HIGH: Path Traversal & Symlink Attack Vulnerability The
Attack scenario: # Attacker creates symlink
ln -s /etc/passwd /tmp/secret.png
# Then pastes: /tmp/secret.png
# Extension check passes (.png), os.Stat follows symlink, file gets attachedFix: info, err := os.Lstat(line) // Use Lstat instead of Stat
if err != nil || info.IsDir() || info.Mode()&os.ModeSymlink != 0 {
return false
}
// Also add path validation:
cleanPath := filepath.Clean(line)
if strings.Contains(cleanPath, "..") {
return false // Reject path traversal attempts
} |
||
| 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 == '\\': | ||
|
Contributor
There was a problem hiding this comment. 🟡 MEDIUM: Incorrect Trailing Backslash Handling When a path ends with a single backslash, it's treated as a literal character in the filename. On Unix, backslash is an escape character, never part of filenames. This creates invalid paths. Problem:
Why this is wrong: Fix: case ch == '\\':
if i == len(s)-1 {
// Trailing backslash = malformed input, strip it
break
} else {
escaped = true
}Also update the test to expect the backslash to be stripped. |
||
| 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 "📎" | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM: TOCTOU Race Condition
Files are validated at line 1462 (
allFilesValid()callsos.Lstat()), but between that check and the actual attachment at line 1464, another process could delete/move/modify the file.Race window:
Impact: The existing error handling (line 1465) catches this gracefully, so it won't crash. However, the redundant validation at line 1462 creates an unnecessary race window.
Recommendation: Consider removing the
allFilesValid()call and rely solely onAttachFile()to validate, since it callsvalidateFilePath()internally anyway. This eliminates the redundant check and the TOCTOU window: