feat: add file drag-and-drop support for images and PDFs

[ambigois]

• 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
• 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 (10+ test cases)
• Works with all major terminal emulators
• 100% backward compatible

Assisted-By: cagent

[github-actions]

Security & Bug Review

I've identified several critical security vulnerabilities and bugs in the new file drag-and-drop code:

🔴 Critical Security Issues

1. Path Traversal & Symlink Vulnerabilities

• The code accepts arbitrary file paths without validation
• os.Stat() is used instead of os.Lstat(), allowing symlink attacks
• Attackers could access sensitive files like /etc/passwd via symlinks

2. TOCTOU Race Condition

• Files are validated, then attached without atomic operations
• Files deleted between validation and attachment fail silently

Recommendations

For Security Issues:

• Use os.Lstat() to detect symlinks and reject them
• Validate paths with filepath.Clean() and restrict to safe directories
• Consider using filepath.EvalSymlinks() and checking the resolved path

For Race Condition:

• Add error return values to AttachFile() and addFileAttachment()
• Log failures and notify users which files couldn't be attached
• Consider opening file handles during validation to prevent TOCTOU

For Trailing Backslash:

• Strip trailing backslashes or reject as malformed input
• Update test expectations to reflect correct behavior

Please address the HIGH severity security issues before merging.

[github-actions]

🔴 HIGH: Path Traversal & Symlink Attack Vulnerability

The attemptStatAll() function calls os.Stat() on user-provided paths without validation. This creates two critical security issues:

1. Path Traversal: Attackers can paste paths like ../../etc/passwd or /etc/shadow to probe file existence
2. Symlink Attack: os.Stat() follows symlinks, allowing attackers to create malicious.png -> /etc/passwd and bypass extension validation

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 attached

Fix:

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
}

[github-actions]

🟡 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:

• Input: /path/to/file.png\
• Output: /path/to/file.png\ (backslash included)
• This file doesn't exist, causing attachment failures

Why this is wrong:
A trailing backslash in Unix shell output indicates a malformed/incomplete escape sequence. The test at line 75-79 validates this incorrect behavior.

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.

[rumpl]

/review

[docker-agent]

Security Review Summary

⚠️ Found 1 HIGH severity security vulnerability and 3 MEDIUM severity issues

Critical Issues:

1. Path traversal vulnerability in validateFilePath() - The security check for ".." happens AFTER filepath.Clean() has already resolved them, making it completely ineffective

Medium Issues:

2. Incorrect error handling - Failed file attachments cause file picker to open instead of showing error notification
3. TOCTOU race condition - Files validated twice with window between checks
4. Size limit off-by-one - Files exactly 5MB are accepted when limit appears to be meant as hard cap

The path traversal vulnerability (issue #1) should be addressed before merge as it could allow access to arbitrary files outside intended boundaries.

---

All issues flagged are in lines added by this PR (+ lines only).

[docker-agent]

🔴 HIGH SEVERITY: Path Traversal Vulnerability

The security check for ".." happens AFTER filepath.Clean() has already resolved them:

clean := filepath.Clean(path)  // Resolves ".." to actual paths
if strings.Contains(clean, "..") {  // Will NEVER find ".." here!
    return nil, os.ErrPermission
}

Example exploit:

• Input: /tmp/app/../../../etc/passwd
• After filepath.Clean(): /etc/passwd
• Check strings.Contains("/etc/passwd", "..")? → FALSE ✓ (bypassed!)
• Result: Access granted to /etc/passwd

Fix: Check the ORIGINAL path for ".." before calling filepath.Clean(), or use a different approach like checking if the cleaned path starts with an approved base directory.

// Check BEFORE cleaning
if strings.Contains(path, "..") {
    return nil, os.ErrPermission
}
clean := filepath.Clean(path)

[docker-agent]

🟡 MEDIUM: Incorrect Error Handling

When file attachment fails, returning nil for the command causes the file picker dialog to open again in handlers.go::handleAttachFile() (line 424 checks if cmd != nil).

Current UX:

1. User drags file into editor
2. Attachment fails (e.g., file too large)
3. File picker dialog opens again ❌ (confusing)

Expected UX:

1. User drags file into editor
2. Attachment fails
3. Error notification shows explaining why ✓

Fix: Return an error notification instead of nil:

if err := p.editor.AttachFile(msg.FilePath); err != nil {
    return p, notification.ErrorCmd(fmt.Sprintf("Failed to attach file: %v", err))
}

[docker-agent]

🟡 MEDIUM: TOCTOU Race Condition

Files are validated at line 1462 (allFilesValid() calls os.Lstat()), but between that check and the actual attachment at line 1464, another process could delete/move/modify the file.

Race window:

Time 1: allFilesValid() checks file exists ✓
Time 2: [another process deletes file]
Time 3: AttachFile() tries to use file ✗

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 on AttachFile() to validate, since it calls validateFilePath() internally anyway. This eliminates the redundant check and the TOCTOU window:

filePaths := ParsePastedFiles(content)
if len(filePaths) > 0 {
    for _, path := range filePaths {
        if err := e.AttachFile(path); err != nil {
            slog.Warn("failed to attach dropped file", "path", path, "error", err)
            return false  // Failed to parse as files, treat as text
        }
    }
    return true
}

[docker-agent]

🟡 MEDIUM: Size Check Off-by-One

The check uses > instead of >=, allowing files exactly 5MB to be attached:

if info.Size() > 5*1024*1024 {  // Only rejects > 5MB, not >= 5MB

Result:

• 5,242,879 bytes → ❌ rejected
• 5,242,880 bytes (exactly 5MB) → ✓ accepted
• 5,242,881 bytes → ❌ rejected

The log message "file too large for attachment" and lack of boundary tests suggest the intent was a hard 5MB limit (reject files 5MB or larger).

Recommendation: Use >= for a cleaner boundary:

if info.Size() >= 5*1024*1024 {
    slog.Warn("file too large for attachment", "path", path, "size", info.Size())
    return false
}

Also consider adding a test case for this boundary condition.

[ambigois]

/review

[docker-agent]

Code Review Summary

Found 2 bugs in the changed code that should be addressed:

High Severity Issues

1. Rollback leaves orphaned placeholder text - When partial file attachment fails, the rollback removes attachments from the list but leaves @filename text in the textarea

Medium Severity Issues

1. Error handling prevents fallback file picker - When attachment fails, the file picker dialog doesn't open as a fallback despite the comment indicating it should

Both issues are in newly added code from this PR.

[docker-agent]

BUG: Rollback leaves orphaned placeholder text in textarea

When handlePaste attempts to attach multiple files but one fails (e.g., unsupported type or size limit), this rollback only removes attachments from the e.attachments list. However, AttachFile() also inserts placeholder text like @/path/to/file.png into the textarea before adding to the attachments list.

The problem: removeLastNAttachments() only removes from the attachments slice, NOT from the textarea. This leaves orphaned placeholder text that will be sent as literal text in the message instead of being replaced with file content.

Example scenario:

1. User pastes [valid.png (1MB), toolarge.png (6MB)]
2. valid.png attaches successfully → @/path/to/valid.png inserted into textarea, added to attachments
3. toolarge.png fails (size > 5MB)
4. removeLastNAttachments(1) removes valid.png from attachments list
5. Result: Textarea still contains @/path/to/valid.png but no corresponding attachment

Fix: removeLastNAttachments() should also remove the placeholder text from the textarea, or AttachFile() should not insert text until after all validations pass.

[docker-agent]

BUG: Error handling prevents fallback file picker from opening

The code checks if cmd != nil to decide whether to open the file picker dialog. However, both success AND error cases from chatPage.Update() return non-nil commands:

• Success: returns notification.SuccessCmd(...)
• Error: returns notification.ErrorCmd(...)

This means when attachment fails, the function returns the error notification but does NOT open the file picker dialog, contradicting the comment on line 427: "No path provided or attachment failed — open the file picker dialog"

Current behavior:

• Attachment succeeds → shows success notification, no picker (correct ✓)
• Attachment fails → shows error notification, no picker (incorrect ✗)
• No path provided → opens picker (correct ✓)

Expected behavior per comment:

• Attachment fails → shows error notification AND opens picker as fallback

Fix: Change the logic to distinguish between success and error, or use a different approach like checking the cmd type or adding an explicit success flag.

[ambigois]

/review

[docker-agent]

Review Summary

I've reviewed the file drag-and-drop implementation and found no critical bugs in the added code. The implementation is well-designed with:

✅ Proper path traversal protection (conservative .. substring check)
✅ Symlink rejection for security
✅ File size limits enforced (5MB)
✅ All-or-nothing attachment behavior with rollback on partial failure
✅ Comprehensive test coverage for edge cases
✅ Support for multiple terminal formats (Unix, Windows Terminal)

The code is defensive, handles edge cases properly, and the test suite validates the expected behavior. Great work!