diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-03 17:18:10 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-03 17:18:10 +0200 |
| commit | abed584d02fdd89a22b37ef3da0b41f8d77f48b8 (patch) | |
| tree | 305c0e3d8af9905a06c95d8d72428e6665991721 | |
| parent | e45f68b04b5ee2400a7885d5f5b054a889061df5 (diff) | |
| download | qtmaildir-feature/attachments.tar.gz qtmaildir-feature/attachments.zip | |
feat: make attachments reachable from the message panefeature/attachments
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 "<date> <subject>" 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 <noreply@anthropic.com>
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 48 | ||||
| -rw-r--r-- | src/messageview.cpp | 192 | ||||
| -rw-r--r-- | src/messageview.h | 25 | ||||
| -rw-r--r-- | src/mimeparser.cpp | 103 | ||||
| -rw-r--r-- | src/mimeparser.h | 29 | ||||
| -rw-r--r-- | tests/test_messageview.cpp | 110 | ||||
| -rw-r--r-- | tests/test_mimeparser.cpp | 135 |
7 files changed, 639 insertions, 3 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 0e0a22f..3def99d 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -53,7 +53,7 @@ taking that too literally. | 11 | Icon, `.desktop` file, SlackBuild | packaging | M | **partly done**: icon and `.desktop` landed, SlackBuild open | | 13 | No visual feedback that an action stuck | feedback | S | **done** | | 14 | Tag column unreadable, tags need another home | presentation | M | **done** | -| 15 | Attachments are parsed but unreachable from the UI | information | M | open | +| 15 | Attachments are parsed but unreachable from the UI | information | M | **done** | | 16 | Delete on an already-deleted thread should undelete | behavior | S | open | | 17 | No completion for tags in the query bar | workflow | M | open | @@ -557,6 +557,52 @@ This is a gap in the UI only. The backend is complete and already hardened: file. A message with an attachment named `../../etc/passwd` writes inside the chosen directory under a sanitised name and nowhere else. +### Outcome (done) + +Both halves built. The paperclip column needed no new worker query, as the +plan expected: notmuch applies the `attachment` tag itself, so +`ThreadSummary::hasAttachment()` reads what is already there. + +**The bar holds ONE button, not one per file.** The plan's "one button showing +the filename and size" was built first and was wrong: a thread with sixteen +attachments made the bar as wide as the window, pushed the splitter over, and +left the thread list a few pixels wide. It is now `Attachments (N)...` opening +a dialog that lists message number, filename and size with a `Save...` each, +plus `Save all...` when there is more than one. No filename reaches the bar, +so no filename length can resize anything. + +**`Save all` writes into a new subdirectory** named `<date> <subject>`, inside +a parent the user picks. Chosen over zipping: Qt ships no zip API, so a real +`.zip` meant either a new build dependency (quazip, libzip) or shelling to +`/usr/bin/zip` at runtime, and the actual requirement was "do not drop sixteen +files loose among hundreds of others". The picker's title names the subfolder +before the user commits to a location. + +**Two defects found only by using it, both silent:** + +- **`saveTo()` overwrites, which destroyed six of sixteen files.** Several + messages in one thread commonly attach the same filename; each write landed + on the previous one and every one reported success, so the status line said + 16 while the directory held 10. `saveWithoutOverwriting()` now backs the + batch path, appending " (2)" before the extension and keeping a compound + extension whole. `saveTo()` still overwrites, which is right for a single + save the user just chose a location for. +- **`Qt::RFC2822Date` rejects a date carrying a timezone comment.** A header + ending `+0200 (CEST)` is legal per RFC 5322 and common in real mail, and Qt + refuses the whole string rather than the comment, so every such message lost + its date prefix. Comments are stripped before parsing. + +**The subject is untrusted and becomes a directory name.** +`attachmentFolderName()` lives beside the other guards in `mimeparser.cpp`, +strips separators, control characters and leading dots, caps length at 120, +and falls back to a generated name. Its test drives `../../etc`, +`/etc/passwd`, `..`, `.hidden`, a backslash and a null byte, then asserts each +result still resolves inside the parent through `isPathInsideDirectory()`. + +**Deferred, as the plan required:** opening an attachment in its default +application. That hands a stranger's file to `xdg-open` and is a separate +decision from writing it to a directory the user chose. + ## 16. Delete on an already-deleted thread should undelete **Observed:** hitting Delete twice on the same message is a natural way to diff --git a/src/messageview.cpp b/src/messageview.cpp index 9a1d3e4..4423791 100644 --- a/src/messageview.cpp +++ b/src/messageview.cpp @@ -18,16 +18,23 @@ #include "messageview.h" +#include <QApplication> #include <QDesktopServices> +#include <QDialog> +#include <QDialogButtonBox> +#include <QDir> +#include <QFileDialog> #include <QHBoxLayout> #include <QLabel> -#include <QApplication> +#include <QLocale> #include <QMouseEvent> #include <QPushButton> +#include <QStandardPaths> #include <QtNumeric> #include <QTimer> -#include <QWheelEvent> +#include <QTreeWidget> #include <QVBoxLayout> +#include <QWheelEvent> #include <QWebEnginePage> #include <QWebEngineProfile> #include <QWebEngineSettings> @@ -185,6 +192,10 @@ void MessageView::clear() m_headerLabel->clear(); m_blockedLabel->hide(); m_loadRemoteButton->hide(); + + // clear() does not go through render(), so the bar has to be emptied + // here or the previous thread's attachments stay offered. + rebuildAttachmentBar(); } void MessageView::showThread(const QList<ThreadRenderItem> &items) @@ -268,6 +279,7 @@ void MessageView::render() m_preferHtml ? HtmlBuilder::PreferHtml : HtmlBuilder::ForcePlain; setDocument(HtmlBuilder::buildThread(m_items, mode)); + rebuildAttachmentBar(); // Blocking is discovered during load, so check shortly afterwards. QTimer::singleShot(300, this, [this]() { @@ -278,6 +290,182 @@ void MessageView::render() }); } +QList<Attachment> MessageView::allAttachments() const +{ + QList<Attachment> all; + for (const ThreadRenderItem &item : m_items) + all.append(item.message.attachments); + return all; +} + +void MessageView::rebuildAttachmentBar() +{ + auto *layout = qobject_cast<QHBoxLayout *>(m_attachmentBar->layout()); + + // Rebuilt rather than updated: a thread can change under the same widget + // (toggle_html re-renders, and the next thread reuses this bar), and a + // stale button would offer a save from the message before it. + while (QLayoutItem *item = layout->takeAt(0)) { + delete item->widget(); + delete item; + } + + const int total = allAttachments().size(); + if (total == 0) { + m_attachmentBar->hide(); + return; + } + + // One button whatever the count. A button per attachment made the bar as + // wide as the window on a thread with fifteen of them, which pushed the + // splitter over and left the thread list a few pixels wide. + auto *button = new QPushButton(tr("Attachments (%1)...").arg(total), + m_attachmentBar); + button->setToolTip(tr("List the attachments in this thread")); + connect(button, &QPushButton::clicked, + this, &MessageView::showAttachmentDialog); + + layout->addWidget(button); + layout->addStretch(); + m_attachmentBar->show(); +} + +void MessageView::showAttachmentDialog() +{ + QDialog dialog(this); + dialog.setWindowTitle(tr("Attachments")); + + auto *layout = new QVBoxLayout(&dialog); + auto *list = new QTreeWidget(&dialog); + list->setColumnCount(4); + // The fourth column holds the per-row Save button and needs no label. + list->setHeaderLabels({ tr("Message"), tr("File"), tr("Size"), QString() }); + list->setRootIsDecorated(false); + list->setSelectionMode(QAbstractItemView::NoSelection); + + // A thread renders as one document, so the message number is what says + // which of them a file came from. + for (int index = 0; index < m_items.size(); ++index) { + const ParsedMessage &message = m_items.at(index).message; + for (const Attachment &attachment : message.attachments) { + auto *row = new QTreeWidgetItem(list); + row->setText(0, QString::number(index + 1)); + // safeFilename(), never the raw filename: the name in a message is + // attacker-controlled and may carry separators or "..". + row->setText(1, attachment.safeFilename()); + row->setText(2, QLocale().formattedDataSize(attachment.data.size())); + + auto *save = new QPushButton(tr("Save..."), list); + // Copied into the lambda: m_items is replaced wholesale by the + // next showThread(), so a reference would dangle. + connect(save, &QPushButton::clicked, this, + [this, attachment]() { saveAttachment(attachment); }); + list->setItemWidget(row, 3, save); + } + } + for (int column = 0; column < 3; ++column) + list->resizeColumnToContents(column); + + layout->addWidget(list); + + auto *buttons = new QDialogButtonBox(QDialogButtonBox::Close, &dialog); + // Only worth offering for more than one file: with a single attachment it + // is the same action as its own Save button, one dialog deeper. + if (allAttachments().size() > 1) { + auto *saveAll = buttons->addButton(tr("Save all..."), + QDialogButtonBox::ActionRole); + connect(saveAll, &QPushButton::clicked, this, + [this, &dialog]() { + saveAllAttachments(); + dialog.accept(); + }); + } + connect(buttons, &QDialogButtonBox::rejected, &dialog, &QDialog::reject); + layout->addWidget(buttons); + + dialog.resize(560, 320); + dialog.exec(); +} + +void MessageView::saveAllAttachments() +{ + const QList<Attachment> attachments = allAttachments(); + if (attachments.isEmpty()) + return; + + // The subfolder is stated up front rather than discovered afterwards: the + // user picks a parent, and what lands in it is one directory, not fifteen + // loose files among whatever is already there. + const QString subject = m_items.isEmpty() ? QString() + : m_items.first().message.subject; + const QString rfc822Date = m_items.isEmpty() ? QString() + : m_items.first().message.date; + const QString folder = attachmentFolderName(rfc822Date, subject); + + const QString parent = QFileDialog::getExistingDirectory( + this, + tr("Choose a folder. A subfolder \"%1\" will be created inside it.") + .arg(folder), + QStandardPaths::writableLocation(QStandardPaths::DownloadLocation)); + if (parent.isEmpty()) + return; // cancelled + + // Never overwrite an existing directory: a second save of the same thread + // gets its own folder rather than merging into the first. + QDir parentDir(parent); + QString unique = folder; + for (int suffix = 2; parentDir.exists(unique); ++suffix) + unique = tr("%1 (%2)").arg(folder).arg(suffix); + + if (!parentDir.mkpath(unique)) { + emit statusMessage(tr("Could not create %1").arg(unique)); + return; + } + const QString target = parentDir.absoluteFilePath(unique); + + int saved = 0; + QStringList failures; + for (const Attachment &attachment : attachments) { + QString error; + // Not saveTo(): several messages in a thread commonly attach the same + // filename, and overwriting silently lost six of sixteen files while + // still reporting every one as saved. + if (attachment.saveWithoutOverwriting(target, &error).isEmpty()) + failures.append(attachment.safeFilename()); + else + ++saved; + } + + if (failures.isEmpty()) { + emit statusMessage(tr("Saved %1 attachment(s) to %2") + .arg(saved).arg(target)); + } else { + emit statusMessage(tr("Saved %1 of %2 to %3; failed: %4") + .arg(saved).arg(attachments.size()) + .arg(target, failures.join(QStringLiteral(", ")))); + } +} + +void MessageView::saveAttachment(const Attachment &attachment) +{ + const QString directory = QFileDialog::getExistingDirectory( + this, tr("Save attachment to"), + QStandardPaths::writableLocation(QStandardPaths::DownloadLocation)); + if (directory.isEmpty()) + return; // cancelled + + QString error; + const QString written = attachment.saveTo(directory, &error); + if (written.isEmpty()) { + emit statusMessage(tr("Could not save attachment: %1").arg(error)); + return; + } + + // Reported, not silent: a save with no feedback is the same failure as + // acting on a thread and seeing nothing change. + emit statusMessage(tr("Saved %1").arg(written)); +} + void MessageView::toggleHtml() { const bool anyHtml = std::any_of( diff --git a/src/messageview.h b/src/messageview.h index 09e7d71..244a1ba 100644 --- a/src/messageview.h +++ b/src/messageview.h @@ -101,6 +101,31 @@ private: void updateHeader(); void setDocument(const QString &html); + /// Rebuilds the attachment bar from m_items. Called from render(), so a + /// toggle between HTML and plain text keeps the bar in step with what is + /// on screen. + /// + /// The bar holds ONE button however many attachments a thread carries. A + /// button per file resized the splitter and crushed the thread list on a + /// thread with fifteen of them. + void rebuildAttachmentBar(); + + /// The list of attachments, with a save button each and a "save all". + void showAttachmentDialog(); + + /// Saves one attachment, asking for the target directory. Writing goes + /// through Attachment::saveTo(), which is where the path-traversal guard + /// lives; the filename in a message is attacker-controlled. + void saveAttachment(const Attachment &attachment); + + /// Saves every attachment into a new subdirectory of a directory the user + /// picks, so fifteen files do not land loose among hundreds of others and + /// cannot collide with what is already there. + void saveAllAttachments(); + + /// Every attachment in the thread, in the order the messages render. + QList<Attachment> allAttachments() const; + QList<ThreadRenderItem> m_items; bool m_preferHtml = true; diff --git a/src/mimeparser.cpp b/src/mimeparser.cpp index 71cfc64..f5ef38a 100644 --- a/src/mimeparser.cpp +++ b/src/mimeparser.cpp @@ -164,6 +164,109 @@ QString Attachment::safeFilename() const return name; } +QString Attachment::saveWithoutOverwriting(const QString &directory, + QString *error) const +{ + const QString name = safeFilename(); + const QFileInfo info(name); + const QString base = info.completeBaseName(); + // Kept whole: "archive.tar.gz" must not become "archive (2).gz". + const QString suffix = info.suffix().isEmpty() + ? QString() + : QLatin1Char('.') + info.suffix(); + + const QDir dir(directory); + QString candidate = name; + for (int n = 2; dir.exists(candidate); ++n) + candidate = QStringLiteral("%1 (%2)%3").arg(base).arg(n).arg(suffix); + + // The containment check still applies: candidate is derived from + // safeFilename(), but the guarantee belongs at the write, not upstream. + const QString target = dir.absoluteFilePath(candidate); + if (!isPathInsideDirectory(directory, target)) { + if (error) { + *error = QStringLiteral("Refusing to write outside %1") + .arg(QDir::cleanPath(QDir(directory).absolutePath())); + } + return {}; + } + + QFile file(target); + if (!file.open(QIODevice::WriteOnly)) { + if (error) + *error = file.errorString(); + return {}; + } + if (file.write(data) != data.size()) { + if (error) + *error = file.errorString(); + return {}; + } + file.close(); + return target; +} + +QString attachmentFolderName(const QString &rfc822Date, const QString &subject) +{ + // The date prefix sorts chronologically in a file manager. A Date: header + // that does not parse is simply dropped rather than guessed at. + // A trailing timezone comment, "... +0200 (CEST)", is legal per RFC 5322 + // and common in the wild, but Qt::RFC2822Date rejects the whole string + // when one is present (verified on Qt 6.11). Strip comments before + // parsing, or every such message silently loses its date prefix. + QString cleaned = rfc822Date; + cleaned.remove(QRegularExpression(QStringLiteral("\\s*\\([^)]*\\)"))); + cleaned = cleaned.trimmed(); + + QString prefix; + const QDateTime parsed = QDateTime::fromString(cleaned, Qt::RFC2822Date); + if (parsed.isValid()) + prefix = parsed.toString(QStringLiteral("yyyy-MM-dd")); + + // The subject is attacker-controlled and is about to become a directory + // name. Everything that could make it more than one plain component goes: + // separators, and the control characters that can hide what a name really + // is when it is displayed. + QString name = subject.simplified(); + name.remove(QLatin1Char('/')); + name.remove(QLatin1Char('\\')); + QString stripped; + stripped.reserve(name.size()); + for (const QChar c : name) { + if (!c.isNull() && c.category() != QChar::Other_Control) + stripped.append(c); + } + // Leading dots would make a hidden directory, and a name of "." or ".." + // would escape or alias the parent; removing them handles every case. + while (stripped.startsWith(QLatin1Char('.'))) + stripped.remove(0, 1); + stripped = stripped.trimmed(); + + QString combined; + if (!prefix.isEmpty() && !stripped.isEmpty()) + combined = prefix + QLatin1Char(' ') + stripped; + else if (!prefix.isEmpty()) + combined = prefix; + else + combined = stripped; + + // A subject can be far longer than a filesystem component allows. Cut to + // a conservative 120 characters, well under the usual 255-byte limit even + // once multi-byte characters are counted as bytes. + constexpr int maxLength = 120; + if (combined.size() > maxLength) + combined = combined.left(maxLength).trimmed(); + + // Nothing usable survived: no parseable date and a subject that was empty, + // punctuation, or control characters only. + if (combined.isEmpty()) { + return QStringLiteral("attachments-%1").arg( + QUuid::createUuid().toString(QUuid::Id128).left(8)); + } + + return combined; +} + bool Attachment::isPathInsideDirectory(const QString &directory, const QString &candidatePath) { // Compare candidatePath itself, not QFileInfo(candidatePath).absolutePath() 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, "<date> <subject>". +/// +/// `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; diff --git a/tests/test_messageview.cpp b/tests/test_messageview.cpp index 4b6bc4c..278270a 100644 --- a/tests/test_messageview.cpp +++ b/tests/test_messageview.cpp @@ -16,6 +16,7 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include <QPushButton> #include <QSignalSpy> #include <QWebEngineUrlScheme> #include <QWebEngineView> @@ -38,6 +39,8 @@ private slots: void dataUrlSubResourceStillBlocked(); void zoomIsClampedToARenderableRange(); void zoomSurvivesANewDocument(); + void attachmentBarOffersEveryAttachment(); + void attachmentBarClearsBetweenThreads(); private: QWebEngineView *webViewOf(MessageView *view) const @@ -230,5 +233,112 @@ void TestMessageView::zoomSurvivesANewDocument() QCOMPARE(view.zoomFactor(), 1.5); } +/// The buttons in the attachment bar, by their label. Excludes the +/// "Load remote content" button, which lives in the same pane but is not part +/// of the bar. +static QStringList attachmentButtonLabels(MessageView *view) +{ + QStringList labels; + for (QPushButton *button : view->findChildren<QPushButton *>()) { + if (button->text() != QStringLiteral("Load remote content")) + labels.append(button->text()); + } + return labels; +} + +void TestMessageView::attachmentBarOffersEveryAttachment() +{ + // The bar existed as an empty placeholder for two releases: it was created + // and added to the layout, and nothing ever put anything in it, so + // attachments were parsed and then unreachable. + ParsedMessage first; + first.ok = true; + first.from = QStringLiteral("Sender <sender@example.org>"); + first.subject = QStringLiteral("With files"); + first.plainBody = QStringLiteral("see attached"); + first.attachments.append({ QStringLiteral("notes.txt"), + QStringLiteral("text/plain"), + QByteArray("hello") }); + + ParsedMessage second; + second.ok = true; + second.from = QStringLiteral("Other <other@example.org>"); + second.subject = QStringLiteral("Reply"); + second.plainBody = QStringLiteral("mine too"); + second.attachments.append({ QStringLiteral("../../etc/passwd"), + QStringLiteral("text/plain"), + QByteArray("root:x:0:0") }); + + ThreadRenderItem itemA; + itemA.message = first; + itemA.cidPrefix = QStringLiteral("m0"); + itemA.expanded = true; + + ThreadRenderItem itemB; + itemB.message = second; + itemB.cidPrefix = QStringLiteral("m1"); + itemB.expanded = true; + + MessageView view; + view.showThread({ itemA, itemB }); + + // ONE button whatever the count, carrying the total. A button per + // attachment made the bar as wide as the window on a thread with fifteen + // of them and pushed the splitter over, leaving the thread list unusable. + const QStringList labels = attachmentButtonLabels(&view); + QCOMPARE(labels.size(), 1); + QVERIFY2(labels.first().contains(QStringLiteral("2")), + qPrintable(QStringLiteral("expected the count in '%1'") + .arg(labels.first()))); + + // A filename never reaches the bar, so a long one cannot widen it. + QVERIFY(!labels.first().contains(QStringLiteral("notes.txt"))); + QVERIFY(!labels.first().contains(QStringLiteral("passwd"))); +} + +void TestMessageView::attachmentBarClearsBetweenThreads() +{ + ParsedMessage withFile; + withFile.ok = true; + withFile.from = QStringLiteral("Sender <sender@example.org>"); + withFile.subject = QStringLiteral("With a file"); + withFile.plainBody = QStringLiteral("attached"); + withFile.attachments.append({ QStringLiteral("report.pdf"), + QStringLiteral("application/pdf"), + QByteArray("%PDF-1.4") }); + + ThreadRenderItem carrying; + carrying.message = withFile; + carrying.cidPrefix = QStringLiteral("m0"); + carrying.expanded = true; + + MessageView view; + view.showThread({ carrying }); + QCOMPARE(attachmentButtonLabels(&view).size(), 1); + + // Moving to a thread without attachments must not leave the previous + // thread's buttons behind, still offering to save a file from a message + // that is no longer on screen. + ParsedMessage plain; + plain.ok = true; + plain.from = QStringLiteral("Sender <sender@example.org>"); + plain.subject = QStringLiteral("Nothing attached"); + plain.plainBody = QStringLiteral("just text"); + + ThreadRenderItem bare; + bare.message = plain; + bare.cidPrefix = QStringLiteral("m0"); + bare.expanded = true; + + view.showThread({ bare }); + QVERIFY(attachmentButtonLabels(&view).isEmpty()); + + view.showThread({ carrying }); + QCOMPARE(attachmentButtonLabels(&view).size(), 1); + + view.clear(); + QVERIFY(attachmentButtonLabels(&view).isEmpty()); +} + QTEST_MAIN(TestMessageView) #include "test_messageview.moc" diff --git a/tests/test_mimeparser.cpp b/tests/test_mimeparser.cpp index 74b095c..f1bbc8a 100644 --- a/tests/test_mimeparser.cpp +++ b/tests/test_mimeparser.cpp @@ -39,6 +39,9 @@ private slots: void savedAttachmentMatchesBytes(); void safeFilenameStripsPathComponents(); void pathInsideDirectoryRejectsSiblingPrefix(); + void attachmentFolderNameIsASinglePlainComponent(); + void folderNameSurvivesATimezoneComment(); + void savingABatchNeverOverwrites(); private: QString fixture(const QString &name) const @@ -243,5 +246,137 @@ void TestMimeParser::pathInsideDirectoryRejectsSiblingPrefix() QVERIFY(!Attachment::isPathInsideDirectory(base, QStringLiteral("/etc/passwd"))); } +void TestMimeParser::attachmentFolderNameIsASinglePlainComponent() +{ + const QString validDate = QStringLiteral("Thu, 7 May 2026 16:51:48 +0200"); + + // The ordinary case: date prefix so the folders sort chronologically. + QCOMPARE(attachmentFolderName(validDate, QStringLiteral("Quarterly report")), + QStringLiteral("2026-05-07 Quarterly report")); + + // A subject is attacker-controlled and is about to become a directory + // name. None of these may produce anything but one plain component. + const QStringList hostile = { + QStringLiteral("../../etc"), + QStringLiteral("/etc/passwd"), + QStringLiteral("a/b/c"), + QStringLiteral(".."), + QStringLiteral("."), + QStringLiteral(".hidden"), + QStringLiteral("with\\backslash"), + QStringLiteral("null\0byte"), + }; + for (const QString &subject : hostile) { + const QString folder = attachmentFolderName(validDate, subject); + QVERIFY2(!folder.contains(QLatin1Char('/')), + qPrintable(QStringLiteral("'%1' -> '%2'").arg(subject, folder))); + QVERIFY2(!folder.contains(QLatin1Char('\\')), + qPrintable(QStringLiteral("'%1' -> '%2'").arg(subject, folder))); + QVERIFY2(!folder.startsWith(QLatin1Char('.')), + qPrintable(QStringLiteral("'%1' -> '%2'").arg(subject, folder))); + QVERIFY2(folder != QLatin1String("..") && folder != QLatin1String("."), + qPrintable(QStringLiteral("'%1' -> '%2'").arg(subject, folder))); + QVERIFY(!folder.isEmpty()); + + // The decisive check: joining it onto a directory cannot escape. + QVERIFY2(Attachment::isPathInsideDirectory( + QStringLiteral("/tmp/parent"), + QDir(QStringLiteral("/tmp/parent")).absoluteFilePath(folder)), + qPrintable(QStringLiteral("'%1' escaped as '%2'") + .arg(subject, folder))); + } + + // An unparseable Date: is dropped rather than guessed at. + QCOMPARE(attachmentFolderName(QStringLiteral("not a date"), + QStringLiteral("Subject here")), + QStringLiteral("Subject here")); + + // Neither a usable date nor a usable subject still yields a name, since + // the caller is about to create a directory with it. + const QString generated = attachmentFolderName(QString(), QStringLiteral("///")); + QVERIFY(!generated.isEmpty()); + QVERIFY(!generated.contains(QLatin1Char('/'))); + + // A subject can be far longer than a filesystem component allows. + const QString huge = attachmentFolderName(validDate, QString(500, QLatin1Char('x'))); + QVERIFY2(huge.size() <= 120, + qPrintable(QStringLiteral("length %1").arg(huge.size()))); +} + +void TestMimeParser::folderNameSurvivesATimezoneComment() +{ + // "+0200 (CEST)" is legal per RFC 5322 and common in real mail, but + // Qt::RFC2822Date rejects the entire string when the comment is present + // (verified on Qt 6.11). Every such message silently lost its date prefix. + QCOMPARE(attachmentFolderName( + QStringLiteral("Thu, 7 May 2026 16:51:48 +0200 (CEST)"), + QStringLiteral("Report")), + QStringLiteral("2026-05-07 Report")); + + // The same date without the comment must not regress. + QCOMPARE(attachmentFolderName( + QStringLiteral("Thu, 7 May 2026 16:51:48 +0200"), + QStringLiteral("Report")), + QStringLiteral("2026-05-07 Report")); +} + +void TestMimeParser::savingABatchNeverOverwrites() +{ + // Saving a thread's attachments with saveTo() destroyed files: several + // messages in one thread commonly attach the same filename, each write + // landed on the previous one, and all of them reported success. Sixteen + // attachments produced ten files. + QTemporaryDir dir; + + Attachment first; + first.filename = QStringLiteral("questionario.pdf"); + first.data = QByteArray("first copy"); + + Attachment second; + second.filename = QStringLiteral("questionario.pdf"); + second.data = QByteArray("second copy, different bytes"); + + Attachment third; + third.filename = QStringLiteral("questionario.pdf"); + third.data = QByteArray("third"); + + QString error; + const QString pathA = first.saveWithoutOverwriting(dir.path(), &error); + const QString pathB = second.saveWithoutOverwriting(dir.path(), &error); + const QString pathC = third.saveWithoutOverwriting(dir.path(), &error); + + QVERIFY(!pathA.isEmpty()); + QVERIFY(!pathB.isEmpty()); + QVERIFY(!pathC.isEmpty()); + + // Three distinct files, and every one still holds its own bytes. + QCOMPARE(QDir(dir.path()).entryList(QDir::Files).size(), 3); + QVERIFY(pathA != pathB); + QVERIFY(pathB != pathC); + + const auto contentsOf = [](const QString &path) { + QFile file(path); + file.open(QIODevice::ReadOnly); + return file.readAll(); + }; + QCOMPARE(contentsOf(pathA), QByteArray("first copy")); + QCOMPARE(contentsOf(pathB), QByteArray("second copy, different bytes")); + QCOMPARE(contentsOf(pathC), QByteArray("third")); + + // The extension is kept whole rather than split at the first dot. + Attachment tarball; + tarball.filename = QStringLiteral("archive.tar.gz"); + tarball.data = QByteArray("one"); + Attachment tarballAgain = tarball; + tarballAgain.data = QByteArray("two"); + + QVERIFY(!tarball.saveWithoutOverwriting(dir.path(), &error).isEmpty()); + const QString second_tar = + tarballAgain.saveWithoutOverwriting(dir.path(), &error); + QVERIFY(second_tar.endsWith(QStringLiteral(".gz"))); + QVERIFY2(second_tar.contains(QStringLiteral("archive.tar")), + qPrintable(second_tar)); +} + QTEST_MAIN(TestMimeParser) #include "test_mimeparser.moc" |
