summaryrefslogtreecommitdiffstats
path: root/tests
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-03 17:18:10 +0200
committerDanilo M. <danix@danix.xyz>2026-08-03 17:18:10 +0200
commitabed584d02fdd89a22b37ef3da0b41f8d77f48b8 (patch)
tree305c0e3d8af9905a06c95d8d72428e6665991721 /tests
parente45f68b04b5ee2400a7885d5f5b054a889061df5 (diff)
downloadqtmaildir-abed584d02fdd89a22b37ef3da0b41f8d77f48b8.tar.gz
qtmaildir-abed584d02fdd89a22b37ef3da0b41f8d77f48b8.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>
Diffstat (limited to 'tests')
-rw-r--r--tests/test_messageview.cpp110
-rw-r--r--tests/test_mimeparser.cpp135
2 files changed, 245 insertions, 0 deletions
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"