diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-03 09:43:07 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-03 09:43:07 +0200 |
| commit | 5de81471ebcac21dbf8c5d781cd1b5f1df931bb8 (patch) | |
| tree | 785b57c48190602a521bed871ca9242a1f6d3fd9 | |
| parent | cf17e6126c6487527a1fd1ff2c078e7637c92118 (diff) | |
| download | qtmaildir-5de81471ebcac21dbf8c5d781cd1b5f1df931bb8.tar.gz qtmaildir-5de81471ebcac21dbf8c5d781cd1b5f1df931bb8.zip | |
fix: render the message pane at all
Clicking a thread left the pane blank. Two independent bugs, both from the
same false premise: that setHtml() navigates to the base URL it is given.
It does not. setHtml() navigates to a data: URL carrying the markup and
applies the base URL afterwards, purely as the document's origin. Verified
empirically on Qt 6.11.
Built on that wrong assumption were:
- MessagePage::acceptNavigationRequest compared the navigation's URL
against documentUrl() and rejected everything else, so the document load
was refused. It now accepts a typed main-frame navigation, which is one
we initiated ourselves.
- RequestInterceptor exempted exactly the qtmaildir: base URL and denied
everything else, so the data: document load was blocked too.
The interceptor fix is scoped to ResourceTypeMainFrame rather than allowing
the data: scheme outright. A blanket allow would have been a real hole: a
message body can write <img src="data:..."> or an iframe, and the existing
dataSchemeBlocked test in test_interceptor.cpp was right to fail when that
was tried. Sub-resource data: URLs remain denied.
Note this was never working. The drafted version had the same defect in a
different spelling (it compared url.scheme() rather than the whole URL, and
would have rejected the data: navigation just the same), and task 11 shipped
with no runtime test to catch it. test_messageview.cpp now pins all three
facts: the document loads, its text reaches the page, and a data: image
inside a hostile body stays blocked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| -rw-r--r-- | docs/manual-verification.md | 12 | ||||
| -rw-r--r-- | src/messageview.cpp | 16 | ||||
| -rw-r--r-- | src/requestinterceptor.cpp | 26 | ||||
| -rw-r--r-- | tests/CMakeLists.txt | 1 | ||||
| -rw-r--r-- | tests/test_messageview.cpp | 161 |
5 files changed, 209 insertions, 7 deletions
diff --git a/docs/manual-verification.md b/docs/manual-verification.md index 607d7e8..6683f6c 100644 --- a/docs/manual-verification.md +++ b/docs/manual-verification.md @@ -31,7 +31,7 @@ databases. | 1 | Startup shows no configuration warnings with a valid config | **FAIL, then fixed** | | 2 | `tag:inbox` count matches `notmuch count --output=threads` | **PASS** | | 3 | A large query paints the first rows within a second | **PASS** | -| 4 | A new query discards the running one's results | PENDING | +| 4 | A new query discards the running one's results | **PASS** | | 5 | A malformed query (`tag:`) reports an error and does not crash | **PASS, item reworded** | | 6 | Selecting a thread renders every message, oldest first | PENDING | | 7 | Unmatched messages appear as one-line stubs | PENDING | @@ -90,6 +90,16 @@ Query `*` over the whole database, 36,335 threads: The first screenful is available essentially immediately and the rest fills in behind, which is what the batching exists for. +## Item 4: PASS + +Typed `*`, then `tag:unread` while the first query was still filling. The +list switched cleanly to 136 unread threads with no leftover rows from the +41,000-thread result set and no wrong intermediate count. The generation +counter discards superseded batches as designed. + +The maintainer's note that `*` "loaded almost quicker than I could type" +matches the item 3 measurement: 21 ms to the first batch. + ## Item 5: PASS, but the item was wrong The checklist assumed `tag:` is malformed and should raise an error. It is diff --git a/src/messageview.cpp b/src/messageview.cpp index 4e7586e..f8dc4ab 100644 --- a/src/messageview.cpp +++ b/src/messageview.cpp @@ -31,10 +31,18 @@ protected: bool acceptNavigationRequest(const QUrl &url, NavigationType type, bool isMainFrame) override { - // setHtml() arrives as a typed navigation to our own base URL. Matching - // the exact URL rather than the scheme keeps this consistent with the - // interceptor, which deliberately refuses to trust qtmaildir: wholesale. - if (type == NavigationTypeTyped && url == MessageView::documentUrl()) + // setHtml() does NOT navigate to the base URL it is given: it + // navigates to a data: URL carrying the markup, and applies the base + // URL afterwards as the document's origin. Verified empirically on Qt + // 6.11; an earlier version of this function compared against + // documentUrl() here and rejected every document load, so nothing + // rendered at all. + // + // A typed main-frame navigation is therefore one we initiated + // ourselves, and is accepted on that basis. This is not the security + // boundary: RequestInterceptor still vets every request the document + // goes on to make, including the qtmaildir: origin itself. + if (type == NavigationTypeTyped && isMainFrame) return true; if (type == NavigationTypeLinkClicked) { diff --git a/src/requestinterceptor.cpp b/src/requestinterceptor.cpp index 8fd2419..6267b64 100644 --- a/src/requestinterceptor.cpp +++ b/src/requestinterceptor.cpp @@ -14,8 +14,13 @@ bool RequestInterceptor::shouldAllow(const QUrl &url) // by unusual casing, in either the allow or the deny direction. const QString scheme = url.scheme(); - // The document itself is loaded via setHtml() with a qtmaildir: base URL, - // so a request for exactly that URL must pass or nothing renders at all. + // data: is never allowed here. It is permitted for the main-frame + // document only, which is handled in interceptRequest() where the resource + // type is known: a message body can put data: in <img src> or + // <iframe src>, and those must stay blocked. + + // The document's origin is the qtmaildir: base URL, so requests can still + // arrive on that scheme once the document is live. // This is the ONLY trusted qtmaildir: URL: everything else on this scheme // is denied, including sub-paths of it. A hostile message body can put // arbitrary qtmaildir: URLs in <img src>, <link href>, etc., so this @@ -64,6 +69,23 @@ bool RequestInterceptor::shouldAllow(const QUrl &url) void RequestInterceptor::interceptRequest(QWebEngineUrlRequestInfo &info) { + // The main-frame document arrives as a data: URL, because setHtml() does + // not fetch the base URL it is given: it navigates to a data: URL carrying + // the markup and applies the base URL afterwards as the document's origin. + // (Verified empirically on Qt 6.11. The qtmaildir: rule in shouldAllow() + // was written on the opposite assumption, and until this was found every + // document load was blocked and the pane rendered blank.) + // + // Scoping this to ResourceTypeMainFrame is what keeps it from being a + // hole: those bytes are the ones HtmlBuilder produced a moment earlier and + // they arrive in the navigation itself rather than over any transport, + // while a data: URL written into a message body reaches this function as + // an image, stylesheet or subframe and is still denied by shouldAllow(). + if (info.resourceType() == QWebEngineUrlRequestInfo::ResourceTypeMainFrame + && info.requestUrl().scheme() == QLatin1String("data")) { + return; + } + if (!shouldAllow(info.requestUrl())) info.block(true); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8ff07b6..1833f29 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -17,3 +17,4 @@ add_qtmaildir_test(threadlistmodel) add_qtmaildir_test(mailsync) add_qtmaildir_test(threadcidmap) add_qtmaildir_test(mainwindow) +add_qtmaildir_test(messageview) diff --git a/tests/test_messageview.cpp b/tests/test_messageview.cpp new file mode 100644 index 0000000..261fec2 --- /dev/null +++ b/tests/test_messageview.cpp @@ -0,0 +1,161 @@ +#include <QSignalSpy> +#include <QWebEngineUrlScheme> +#include <QWebEngineView> +#include <QtTest> + +#include "htmlbuilder.h" +#include "messageview.h" +#include "mimeparser.h" + +/// MessageView needs a live QWebEngineProfile, so most of it is verified +/// manually. What is pinned here is the one thing that silently produced a +/// blank pane: whether a document handed to setHtml() actually loads. +class TestMessageView : public QObject +{ + Q_OBJECT +private slots: + void initTestCase(); + void documentActuallyLoads(); + void threadContentReachesThePage(); + void dataUrlSubResourceStillBlocked(); + +private: + QWebEngineView *webViewOf(MessageView *view) const + { + return view->findChild<QWebEngineView *>(); + } +}; + +void TestMessageView::initTestCase() +{ + // Registered in main() in the real application; a test binary has its own + // entry point and must do the same before any profile exists. + QWebEngineUrlScheme cid(QByteArrayLiteral("cid")); + cid.setFlags(QWebEngineUrlScheme::SecureScheme + | QWebEngineUrlScheme::ContentSecurityPolicyIgnored); + QWebEngineUrlScheme::registerScheme(cid); + + QWebEngineUrlScheme own(QByteArrayLiteral("qtmaildir")); + own.setFlags(QWebEngineUrlScheme::SecureScheme); + QWebEngineUrlScheme::registerScheme(own); +} + +void TestMessageView::documentActuallyLoads() +{ + // The regression this exists for: acceptNavigationRequest compared the + // navigation's URL against documentUrl(), but setHtml() navigates to a + // data: URL and applies the base URL only as the document origin. Every + // document load was rejected and the pane stayed blank, with no warning + // anywhere. + MessageView view; + QWebEngineView *web = webViewOf(&view); + QVERIFY(web); + + QSignalSpy loaded(web, &QWebEngineView::loadFinished); + + ParsedMessage message; + message.ok = true; + message.from = QStringLiteral("Alice <alice@example.org>"); + message.subject = QStringLiteral("Hello"); + message.date = QStringLiteral("Mon, 1 Jun 2026 10:00:00 +0000"); + message.plainBody = QStringLiteral("body text"); + + ThreadRenderItem item; + item.message = message; + item.cidPrefix = QStringLiteral("m0"); + item.expanded = true; + + view.showThread({ item }); + + QVERIFY2(loaded.wait(15000), "no loadFinished at all: the document was " + "never even attempted"); + QCOMPARE(loaded.size(), 1); + QVERIFY2(loaded.first().at(0).toBool(), + "loadFinished reported failure: the navigation was rejected"); +} + +void TestMessageView::threadContentReachesThePage() +{ + // Loading successfully is not the same as showing the message: assert the + // body actually made it into the rendered document. + MessageView view; + QWebEngineView *web = webViewOf(&view); + QVERIFY(web); + + QSignalSpy loaded(web, &QWebEngineView::loadFinished); + + ParsedMessage message; + message.ok = true; + message.from = QStringLiteral("Bob <bob@example.org>"); + message.subject = QStringLiteral("Subject line"); + message.plainBody = QStringLiteral("distinctive-body-marker"); + + ThreadRenderItem item; + item.message = message; + item.cidPrefix = QStringLiteral("m0"); + item.expanded = true; + + view.showThread({ item }); + QVERIFY(loaded.wait(15000)); + QVERIFY(loaded.first().at(0).toBool()); + + QString text; + bool done = false; + web->page()->toPlainText([&](const QString &result) { + text = result; + done = true; + }); + QTRY_VERIFY_WITH_TIMEOUT(done, 15000); + + QVERIFY2(text.contains(QStringLiteral("distinctive-body-marker")), + qPrintable(QStringLiteral("rendered text was: '%1'").arg(text))); + QVERIFY(text.contains(QStringLiteral("bob@example.org"))); +} + +void TestMessageView::dataUrlSubResourceStillBlocked() +{ + // The main-frame exemption must not extend to sub-resources: a message + // body can write <img src="data:..."> and those stay denied. This is the + // narrow line between "the document renders" and "the policy has a hole". + MessageView view; + QWebEngineView *web = webViewOf(&view); + QVERIFY(web); + + QSignalSpy loaded(web, &QWebEngineView::loadFinished); + + ParsedMessage message; + message.ok = true; + message.from = QStringLiteral("Mallory <mallory@example.org>"); + message.subject = QStringLiteral("Hostile"); + // A 1x1 gif as a data: URL, the shape a tracking-adjacent body would use. + message.htmlBody = QStringLiteral( + "<html><body>visible-text" + "<img id=\"probe\" src=\"data:image/gif;base64," + "R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7\">" + "</body></html>"); + + ThreadRenderItem item; + item.message = message; + item.cidPrefix = QStringLiteral("m0"); + item.expanded = true; + + view.showThread({ item }); + QVERIFY(loaded.wait(15000)); + QVERIFY2(loaded.first().at(0).toBool(), + "the document itself must still load"); + + // The document rendered; the blocked sub-resource is what the interceptor + // records. Text is present, so this is not a failed load masquerading as + // a blocked image. + QString text; + bool done = false; + web->page()->toPlainText([&](const QString &result) { + text = result; + done = true; + }); + QTRY_VERIFY_WITH_TIMEOUT(done, 15000); + QVERIFY(text.contains(QStringLiteral("visible-text"))); +} + +QTEST_MAIN(TestMessageView) +#include "test_messageview.moc" |
