aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md48
-rw-r--r--src/messageview.cpp192
-rw-r--r--src/messageview.h25
-rw-r--r--src/mimeparser.cpp103
-rw-r--r--src/mimeparser.h29
-rw-r--r--tests/test_messageview.cpp110
-rw-r--r--tests/test_mimeparser.cpp135
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"