Uri: fix a few cases of insufficient escaping

2 files changed, 57 insertions, 24 deletions

diff --git a/lib/uri.cpp b/lib/uri.cpp
index 8370d83a..0e2bcb87 100644
--- a/lib/uri.cpp
+++ b/lib/uri.cpp
@@ -30,7 +30,9 @@ Uri::Uri(QByteArray primaryId, QByteArray secondaryId, QString query)
         for (const auto& p: replacePairs)
             if (primaryId[0] == p.sigil) {
                 primaryType_ = Type(p.sigil);
-                pathToBe = p.uriString + primaryId.mid(1);
+                auto safePrimaryId = primaryId.mid(1);
+                safePrimaryId.replace('/', "%2F");
+                pathToBe = p.uriString + std::move(safePrimaryId);
                 break;
             }
         if (!secondaryId.isEmpty()) {
@@ -38,11 +40,40 @@ Uri::Uri(QByteArray primaryId, QByteArray secondaryId, QString query)
                 primaryType_ = Invalid;
                 return;
             }
-            pathToBe += "/event/" + secondaryId.mid(1);
+            auto safeSecondaryId = secondaryId.mid(1);
+            safeSecondaryId.replace('/', "%2F");
+            pathToBe += "/event/" + std::move(safeSecondaryId);
         }
-        setPath(pathToBe);
+        setPath(pathToBe, QUrl::TolerantMode);
     }
-    setQuery(std::move(query));
+    if (!query.isEmpty())
+        setQuery(std::move(query));
+}
+
+static inline auto encodedPath(const QUrl& url)
+{
+    return url.path(QUrl::EncodeDelimiters | QUrl::EncodeUnicode);
+}
+
+static QString pathSegment(const QUrl& url, int which)
+{
+    return QUrl::fromPercentEncoding(
+        encodedPath(url).section('/', which, which).toUtf8());
+}
+
+static auto decodeFragmentPart(const QStringRef& part)
+{
+    return QUrl::fromPercentEncoding(part.toLatin1()).toUtf8();
+}
+
+static auto matrixToUrlRegexInit()
+{
+    // See https://matrix.org/docs/spec/appendices#matrix-to-navigation
+    const QRegularExpression MatrixToUrlRE {
+        "^/(?<main>[^:]+:[^/?]+)(/(?<sec>(\\$|%24)[^?]+))?(\\?(?<query>.+))?$"
+    };
+    Q_ASSERT(MatrixToUrlRE.isValid());
+    return MatrixToUrlRE;
 }
 
 Uri::Uri(QUrl url) : QUrl(std::move(url))
@@ -51,14 +82,13 @@ Uri::Uri(QUrl url) : QUrl(std::move(url))
     if (isEmpty())
         return; // primaryType_ == Empty
 
-    if (!QUrl::isValid()) { // MatrixUri::isValid() checks primaryType_
-        primaryType_ = Invalid;
+    primaryType_ = Invalid;
+    if (!QUrl::isValid()) // MatrixUri::isValid() checks primaryType_
         return;
-    }
 
     if (scheme() == "matrix") {
         // Check sanity as per https://github.com/matrix-org/matrix-doc/pull/2312
-        const auto& urlPath = path();
+        const auto& urlPath = encodedPath(*this);
         const auto& splitPath = urlPath.splitRef('/');
         switch (splitPath.size()) {
         case 2:
@@ -83,17 +113,15 @@ Uri::Uri(QUrl url) : QUrl(std::move(url))
 
     primaryType_ = NonMatrix; // Default, unless overridden by the code below
     if (scheme() == "https" && authority() == "matrix.to") {
-        // See https://matrix.org/docs/spec/appendices#matrix-to-navigation
-        static const QRegularExpression MatrixToUrlRE {
-            R"(^/(?<main>[^/?]+)(/(?<sec>[^?]+))?(\?(?<query>.+))?$)"
-        };
+        static const auto MatrixToUrlRE = matrixToUrlRegexInit();
         // matrix.to accepts both literal sigils (as well as & and ? used in
         // its "query" substitute) and their %-encoded forms;
         // so force QUrl to decode everything.
-        auto f = fragment(QUrl::FullyDecoded);
+        auto f = fragment(QUrl::EncodeUnicode);
         if (auto&& m = MatrixToUrlRE.match(f); m.hasMatch())
-            *this = Uri { m.captured("main").toUtf8(),
-                          m.captured("sec").toUtf8(), m.captured("query") };
+            *this = Uri { decodeFragmentPart(m.capturedRef("main")),
+                          decodeFragmentPart(m.capturedRef("sec")),
+                          decodeFragmentPart(m.capturedRef("query")) };
     }
 }
 
@@ -119,7 +147,7 @@ Uri::Type Uri::type() const { return primaryType_; }
 
 Uri::SecondaryType Uri::secondaryType() const
 {
-    return path().section('/', 2, 2) == "event" ? EventId : NoSecondaryId;
+    return pathSegment(*this, 2) == "event" ? EventId : NoSecondaryId;
 }
 
 QUrl Uri::toUrl(UriForm form) const
@@ -148,13 +176,13 @@ QString Uri::primaryId() const
     if (primaryType_ == Empty || primaryType_ == Invalid)
         return {};
 
-    const auto& idStem = path().section('/', 1, 1);
+    const auto& idStem = pathSegment(*this, 1);
     return idStem.isEmpty() ? idStem : primaryType_ + idStem;
 }
 
 QString Uri::secondaryId() const
 {
-    const auto& idStem = path().section('/', 3);
+    const auto& idStem = pathSegment(*this, 3);
     return idStem.isEmpty() ? idStem : secondaryType() + idStem;
 }
 
diff --git a/tests/quotest.cpp b/tests/quotest.cpp
index 44a17baf..19d5eec0 100644
--- a/tests/quotest.cpp
+++ b/tests/quotest.cpp
@@ -730,14 +730,19 @@ TEST_IMPL(visitResources)
         "matrix:room/" + roomAlias.mid(1) + "/event/" + eventId.mid(1),
         "https://matrix.to/#/" + roomId + '/' + eventId
     };
-    // The following URIs are not supposed to be actually joined (and even
-    // exist, as yet) - only to be syntactically correct
+    // Check that reserved characters are correctly processed.
     static const auto& joinRoomAlias =
-        QStringLiteral("unjoined:example.org"); // # will be added below
+        QStringLiteral("##/?.@\"unjoined:example.org");
+    static const auto& encodedRoomAliasNoSigil =
+        QUrl::toPercentEncoding(joinRoomAlias.mid(1), ":");
     static const QString joinQuery { "?action=join" };
+    // These URIs are not supposed to be actually joined (and even exist,
+    // as yet) - only to be syntactically correct
     static const QStringList joinByAliasUris {
-        "matrix:room/" + joinRoomAlias + joinQuery,
-        "https://matrix.to/#/%23"/*`#`*/ + joinRoomAlias + joinQuery
+        Uri(joinRoomAlias.toUtf8(), {}, joinQuery.mid(1)).toDisplayString(),
+        "matrix:room/" + encodedRoomAliasNoSigil + joinQuery,
+        "https://matrix.to/#/%23"/*`#`*/ + encodedRoomAliasNoSigil + joinQuery,
+        "https://matrix.to/#/%23" + joinRoomAlias.mid(1) /* unencoded */ + joinQuery
     };
     static const auto& joinRoomId = QStringLiteral("!anyid:example.org");
     static const QStringList viaServers { "matrix.org", "example.org" };
@@ -758,7 +763,7 @@ TEST_IMPL(visitResources)
         || testResourceResolver(eventUris, &UriDispatcher::roomAction,
                                 room(), { eventId })
         || testResourceResolver(joinByAliasUris, &UriDispatcher::joinAction,
-                                connection(), { '#' + joinRoomAlias })
+                                connection(), { joinRoomAlias })
         || testResourceResolver(joinByIdUris, &UriDispatcher::joinAction,
                                 connection(), { joinRoomId, viaServers }))
         return true;