From d5e29dc45fc8ee552ab020ab33a2fb19d7ed0f73 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 3 Aug 2026 17:18:10 +0200 Subject: feat: make attachments reachable from the message pane The attachment bar had been an empty placeholder since it was written: MessageView created it and added it to the layout, and nothing ever put anything in it. MimeParser had been extracting attachments the whole time and Attachment::saveTo() already carried the path-traversal guard, so the backend needed calling rather than writing. The bar holds one "Attachments (N)..." button whatever the count. One button per file was built first and was wrong: a thread with sixteen of them made the bar as wide as the window, pushed the splitter over and left the thread list a few pixels wide. The button opens a dialog listing message number, filename and size with a Save each, and a "Save all..." when there is more than one. Save all writes into a new subdirectory named " " inside a parent the user picks, rather than dropping sixteen files loose among whatever is already there. Zipping was considered and rejected: Qt ships no zip API, so a real archive meant a new build dependency or shelling out to /usr/bin/zip at runtime, and a subdirectory answers the actual requirement. The picker names the subfolder before the user commits to a location. The subject is attacker-controlled and becomes a directory name, so attachmentFolderName() sits beside the other guards in mimeparser.cpp: it strips separators, control characters and leading dots, caps the length, and falls back to a generated name. Its test asserts that every hostile subject still resolves inside the parent directory. Two defects surfaced while using it, both silent: saveTo() overwrites an existing file, and several messages in one thread commonly attach the same filename. Saving that thread destroyed six of sixteen files while reporting all sixteen as saved. The batch path now uses saveWithoutOverwriting(), which appends " (2)" before the extension and keeps a compound extension whole. Qt::RFC2822Date rejects a Date header that carries a timezone comment, "+0200 (CEST)", which is legal per RFC 5322 and common in real mail. Qt refuses the entire string rather than ignoring the comment, so every such message lost its date prefix. Comments are stripped before parsing. Opening an attachment in its default application is deliberately not included: handing a file from a stranger to xdg-open is a different security decision from writing it where the user asked. Co-Authored-By: Claude Opus 5 --- src/mimeparser.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) (limited to 'src/mimeparser.h') diff --git a/src/mimeparser.h b/src/mimeparser.h index 64ea0ce..af7619f 100644 --- a/src/mimeparser.h +++ b/src/mimeparser.h @@ -44,8 +44,22 @@ struct Attachment /// Writes the attachment into directory. Returns the full path written, or /// an empty string on failure with *error set. + /// + /// **Overwrites an existing file of the same name.** That is right for a + /// single save the user just confirmed a location for, and wrong for + /// saving a batch: several messages in one thread commonly attach the + /// same filename. Use saveWithoutOverwriting() there. QString saveTo(const QString &directory, QString *error) const; + /// Writes the attachment into directory under a name that is not already + /// taken, appending " (2)", " (3)" and so on before the extension. + /// Returns the full path written, or an empty string on failure. + /// + /// Saving a thread's attachments with saveTo() silently destroyed files: + /// six of sixteen were lost to same-name collisions and every write still + /// reported success. + QString saveWithoutOverwriting(const QString &directory, QString *error) const; + /// True if candidatePath (need not exist) is directory itself or strictly /// beneath it, by path-boundary comparison after QDir::cleanPath on both /// sides (so ".." segments are resolved rather than compared textually). @@ -65,6 +79,21 @@ struct Attachment static bool isPathInsideDirectory(const QString &directory, const QString &candidatePath); }; +/// A directory name for a thread's saved attachments, " ". +/// +/// `rfc822Date` is a raw Date: header as ParsedMessage stores it; it is +/// reduced to "yyyy-MM-dd" when it parses and dropped when it does not. +/// +/// Both inputs are untrusted: a subject is attacker-controlled and may carry +/// path separators, "..", control characters, or nothing usable at all. The +/// result is always a single plain component, never a path, and never "." or +/// "..". Falls back to the date alone, then to a generated name, so it is +/// never empty. +/// +/// Length is capped: many filesystems limit one component to 255 bytes, and a +/// subject can be far longer than that. +QString attachmentFolderName(const QString &rfc822Date, const QString &subject); + struct ParsedMessage { bool ok = false; -- cgit v1.2.3