fix: enforce !-free cidPrefix invariant at both concatenation sites

The cid: namespacing scheme (cid:<prefix>!<id>) is only unambiguous
because the prefix half is guaranteed free of '!': the first '!' in the
result is always the separator, so an attacker-controlled Content-ID
containing '!' only extends the id half rather than colliding with a
different prefix. That invariant previously existed only as a comment.

Add Q_ASSERT_X at both independent call sites that perform this
concatenation (CidSchemeHandler::namespacedKey and
HtmlBuilder::namespaceCids) so a future prefix generator that violates it
traps in debug builds, per Task 5's precedent of not letting one unit's
correctness depend silently on another's future behaviour. Since
Q_ASSERT compiles out in release, pin the property that actually matters
release builds too test: distinct (prefix, id) pairs across a documented
"m<index>" prefix set and hostile Content-IDs (containing '!', percent-
encoded '!', empty, leading/trailing '!') never collide, and the key
always splits at its first '!' back to the exact original prefix.

4 files changed, 98 insertions, 1 deletions

diff --git a/src/cidschemehandler.h b/src/cidschemehandler.h
index 56e6ea1..1231fc6 100644
--- a/src/cidschemehandler.h
+++ b/src/cidschemehandler.h
@@ -20,8 +20,27 @@ public:
     void setParts(const QHash<QString, InlinePart> &parts) { m_parts = parts; }
 
     /// Builds the namespaced key HtmlBuilder's rewritten URLs will request.
+    ///
+    /// The '!' separator disambiguates a hostile Content-ID from the prefix
+    /// only because prefix is guaranteed '!'-free: the FIRST '!' in the
+    /// result is always the separator, so an attacker-controlled contentId
+    /// containing '!' (even several) cannot make one message's key collide
+    /// with another's, it only extends the id half after that first '!'.
+    /// This is asserted here rather than merely documented, since two call
+    /// sites (this one and HtmlBuilder::namespaceCids) perform the same
+    /// concatenation independently and neither should trust the other to
+    /// have checked it. Q_ASSERT is compiled out in release builds; the
+    /// property that matters there (distinct pairs never collide, and the
+    /// key always splits at its first '!' back to the original prefix) is
+    /// pinned by a test instead, since it holds unconditionally regardless
+    /// of whether this assertion fires.
     static QString namespacedKey(const QString &prefix, const QString &contentId)
-    { return prefix + QLatin1Char('!') + contentId; }
+    {
+        Q_ASSERT_X(!prefix.contains(QLatin1Char('!')), "CidSchemeHandler::namespacedKey",
+                   "cidPrefix must never contain '!': it is the separator, and a "
+                   "prefix containing one would make the split ambiguous");
+        return prefix + QLatin1Char('!') + contentId;
+    }
 
     void requestStarted(QWebEngineUrlRequestJob *job) override;
 
diff --git a/src/htmlbuilder.cpp b/src/htmlbuilder.cpp
index 1830383..89501ea 100644
--- a/src/htmlbuilder.cpp
+++ b/src/htmlbuilder.cpp
@@ -56,6 +56,19 @@ QString HtmlBuilder::namespaceCids(const QString &html, const QString &prefix)
     if (prefix.isEmpty())
         return html;
 
+    // The '!' separator is only unambiguous if prefix itself never contains
+    // one: the FIRST '!' in "cid:<prefix>!<id>" is always taken as the
+    // separator, so a hostile Content-ID containing '!' only extends the id
+    // half, never collides with a different prefix. This concatenation is
+    // performed independently in two places (here and
+    // CidSchemeHandler::namespacedKey); neither trusts the other to have
+    // checked it, so both assert it directly. Q_ASSERT is compiled out in
+    // release builds — the property that matters there is pinned by
+    // TestHtmlBuilder::namespacedKeyRejectsPrefixContainingSeparator instead.
+    Q_ASSERT_X(!prefix.contains(QLatin1Char('!')), "HtmlBuilder::namespaceCids",
+               "cidPrefix must never contain '!': it is the separator, and a "
+               "prefix containing one would make the split ambiguous");
+
     // This runs on the sender's raw, unescaped HTML markup (not on text that
     // has been through toHtmlEscaped()), so no double-escaping happens here;
     // it is purely a URL rewrite over the existing markup.
diff --git a/src/htmlbuilder.h b/src/htmlbuilder.h
index ac24e28..ff62c69 100644
--- a/src/htmlbuilder.h
+++ b/src/htmlbuilder.h
@@ -16,6 +16,15 @@ struct ThreadRenderItem
     /// use the same Content-ID (cid:logo@example.org), which would collide in
     /// a single document, so every reference is rewritten to
     /// cid:<prefix>!<id>.
+    ///
+    /// Requirement on whatever generates this value: it must never contain
+    /// '!'. The separator that makes cid:<prefix>!<id> unambiguous is the
+    /// FIRST '!' in the namespaced string; that only holds if the prefix
+    /// half is guaranteed free of the character, since the id half is
+    /// attacker-controlled and may legitimately contain '!' itself. The
+    /// documented "m<index>" form (e.g. "m0", "m1") satisfies this. Enforced
+    /// with Q_ASSERT at both places that perform this concatenation
+    /// (HtmlBuilder::namespaceCids and CidSchemeHandler::namespacedKey).
     QString cidPrefix;
 };
 
diff --git a/tests/test_htmlbuilder.cpp b/tests/test_htmlbuilder.cpp
index 38699f4..86a40cd 100644
--- a/tests/test_htmlbuilder.cpp
+++ b/tests/test_htmlbuilder.cpp
@@ -1,4 +1,6 @@
+#include <QSet>
 #include <QtTest>
+#include "cidschemehandler.h"
 #include "htmlbuilder.h"
 
 class TestHtmlBuilder : public QObject
@@ -21,6 +23,7 @@ private slots:
     void namespacesCidInStyleBlock();
     void namespacesMultipleCidRefsOnOneLine();
     void namespacesWhitespaceAroundEquals();
+    void namespacedKeyRejectsPrefixContainingSeparator();
 };
 
 void TestHtmlBuilder::escapesPlainText()
@@ -202,5 +205,58 @@ void TestHtmlBuilder::namespacesWhitespaceAroundEquals()
     QVERIFY(html.contains(QStringLiteral("cid:m0!logo@example.org")));
 }
 
+void TestHtmlBuilder::namespacedKeyRejectsPrefixContainingSeparator()
+{
+    // The property that actually matters, and holds even in release builds
+    // where Q_ASSERT is compiled out: distinct (prefix, id) pairs, drawn from
+    // the documented "m<index>" generator form plus hostile Content-IDs
+    // (including ones containing '!', a percent-encoded '!', and several
+    // '!'s), always produce distinct keys, and the key always splits at its
+    // FIRST '!' back to exactly the original prefix. That only holds because
+    // prefixes generated in the "m<index>" form are themselves '!'-free; the
+    // attacker only ever controls the id half, which sits after the first
+    // (and only guaranteed-separator) '!'.
+    const QStringList prefixes = { QStringLiteral("m0"), QStringLiteral("m1"),
+                                    QStringLiteral("m2"), QStringLiteral("m10") };
+    const QStringList hostileIds = {
+        QStringLiteral("logo@example.org"),
+        QStringLiteral("a!b@x"),
+        QStringLiteral("a!b!c@x"),
+        QStringLiteral("%21encoded@x"),
+        QStringLiteral(""),
+        QStringLiteral("!leading@x"),
+        QStringLiteral("trailing!@x"),
+    };
+
+    QSet<QString> seenKeys;
+    for (const QString &prefix : prefixes) {
+        for (const QString &id : hostileIds) {
+            const QString key = CidSchemeHandler::namespacedKey(prefix, id);
+
+            // Distinctness: no other (prefix, id) pair already produced this
+            // exact key.
+            QVERIFY2(!seenKeys.contains(key),
+                     qPrintable(QStringLiteral("collision for key '%1'").arg(key)));
+            seenKeys.insert(key);
+
+            // Splitting at the FIRST '!' always recovers the original
+            // prefix, regardless of how many '!' the hostile id contributes.
+            const qsizetype sep = key.indexOf(QLatin1Char('!'));
+            QVERIFY(sep != -1);
+            QCOMPARE(key.left(sep), prefix);
+            QCOMPARE(key.mid(sep + 1), id);
+        }
+    }
+
+    // Same property, exercised through HtmlBuilder::namespaceCids, which
+    // performs the identical concatenation independently: the resulting URL
+    // must contain the CidSchemeHandler-computed key verbatim.
+    const QString html = HtmlBuilder::namespaceCids(
+        QStringLiteral("<img src=\"cid:a!b@x\">"), QStringLiteral("m0"));
+    const QString expectedKey = CidSchemeHandler::namespacedKey(
+        QStringLiteral("m0"), QStringLiteral("a!b@x"));
+    QVERIFY(html.contains(QStringLiteral("cid:%1").arg(expectedKey)));
+}
+
 QTEST_MAIN(TestHtmlBuilder)
 #include "test_htmlbuilder.moc"