Merge #698(kitsune): Make linkifyUrl() stricter on email addresses

quotient-im · Aug 16, 2023 · f56a39b · f56a39b
2 parents 4cbe830 + ba0b547
commit f56a39b
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 41 deletions.
diff --git a/Quotient/util.cpp b/Quotient/util.cpp
@@ -32,10 +32,8 @@ void Quotient::linkifyUrls(QString& htmlEscapedText)
         QStringLiteral(
             R"(\b((www\.(?!\.)(?!(\w|\.|-)+@)|(https?|ftp):(//)?\w|(magnet|matrix):)(&(?![lg]t;)|[^&\s<>'"])+(&(?![lg]t;)|[^&!,.\s<>'"\]):])))"),
         RegExpOptions);
-    // email address:
-    // [word chars, dots or dashes]@[word chars, dots or dashes].[word chars]
     static const QRegularExpression EmailAddressRegExp(
-        QStringLiteral(R"(\b(mailto:)?((\w|\.|-)+@(\w|\.|-)+\.\w+\b))"),
+        QStringLiteral(R"((^|[][[:space:](){}`'";<>])(mailto:)?((\w|[!#$%&'*+=^_‘{|}~.-])+@(\w|\.|-)+\.\w+\b))"),
         RegExpOptions);
     // An interim liberal implementation of
     // https://matrix.org/docs/spec/appendices.html#identifier-grammar
@@ -49,12 +47,10 @@ void Quotient::linkifyUrls(QString& htmlEscapedText)
     // NOTE: htmlEscapedText is already HTML-escaped! No literal <,>,&,"
 
     htmlEscapedText.replace(EmailAddressRegExp,
-                            QStringLiteral(R"(<a href="mailto:\2">\1\2</a>)"));
-    htmlEscapedText.replace(FullUrlRegExp,
-                            QStringLiteral(R"(<a href="\1">\1</a>)"));
-    htmlEscapedText.replace(
-        MxIdRegExp,
-        QStringLiteral(R"(\1<a href="https://matrix.to/#/\2">\2</a>)"));
+                            R"(\1<a href='mailto:\3'>\2\3</a>)"_ls);
+    htmlEscapedText.replace(FullUrlRegExp, R"(<a href='\1'>\1</a>)"_ls);
+    htmlEscapedText.replace(MxIdRegExp,
+                            R"(\1<a href='https://matrix.to/#/\2'>\2</a>)"_ls);
 }
 
 QString Quotient::sanitized(const QString& plainText)

diff --git a/autotests/utiltests.cpp b/autotests/utiltests.cpp
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: LGPL-2.1-or-later
 
 #include <Quotient/omittable.h>
+#include <Quotient/util.h>
 
 #include <QtTest/QtTest>
 
@@ -38,8 +39,53 @@ static_assert(std::is_same_v<Omittable<bool>,
 class TestUtils : public QObject {
     Q_OBJECT
 private Q_SLOTS:
-    // TODO
+    void testLinkifyUrl();
+
+private:
+    void testLinkified(QString original, const QString& expected,
+                       const int sourceLine) const
+    {
+        linkifyUrls(original);
+        QTest::qCompare(original, expected, "", "", __FILE__, sourceLine);
+    }
 };
 
+#ifndef Q_MOC_RUN // moc chokes on the code below, doesn't really need it
+void TestUtils::testLinkifyUrl()
+{
+    // Pending rewrite once std::source_location() works everywhere
+#    define T(Original, Expected) testLinkified(Original, Expected, __LINE__)
+
+    T("https://www.matrix.org"_ls,
+      "<a href='https://www.matrix.org'>https://www.matrix.org</a>"_ls);
+//    T("www.matrix.org"_ls, // Doesn't work yet
+//      "<a href='https://www.matrix.org'>www.matrix.org</a>"_ls);
+    T("smb://some/file"_ls, "smb://some/file"_ls); // Disallowed scheme
+    T("https:/something"_ls, "https:/something"_ls); // Malformed URL
+    T("https://matrix.to/#/!roomid:example.org"_ls,
+      "<a href='https://matrix.to/#/!roomid:example.org'>https://matrix.to/#/!roomid:example.org</a>"_ls);
+    T("https://matrix.to/#/@user_id:example.org"_ls,
+      "<a href='https://matrix.to/#/@user_id:example.org'>https://matrix.to/#/@user_id:example.org</a>"_ls);
+    T("https://matrix.to/#/#roomalias:example.org"_ls,
+      "<a href='https://matrix.to/#/#roomalias:example.org'>https://matrix.to/#/#roomalias:example.org</a>"_ls);
+    T("https://matrix.to/#/##ircroomalias:example.org"_ls,
+      "<a href='https://matrix.to/#/##ircroomalias:example.org'>https://matrix.to/#/##ircroomalias:example.org</a>"_ls);
+    T("me@example.org"_ls,
+      "<a href='mailto:me@example.org'>me@example.org</a>"_ls);
+    T("mailto:me@example.org"_ls,
+      "<a href='mailto:me@example.org'>mailto:me@example.org</a>"_ls);
+    T("www.example.com?email@example.com"_ls,
+      "<a href='www.example.com?email@example.com'>www.example.com?email@example.com</a>"_ls);
+    T("!room_id:example.org"_ls,
+      "<a href='https://matrix.to/#/!room_id:example.org'>!room_id:example.org</a>"_ls);
+    T("@user_id:example.org"_ls,
+      "<a href='https://matrix.to/#/@user_id:example.org'>@user_id:example.org</a>"_ls);
+    T("#room_alias:example.org"_ls,
+      "<a href='https://matrix.to/#/#room_alias:example.org'>#room_alias:example.org</a>"_ls);
+
+#    undef T
+}
+#endif
+
 QTEST_APPLESS_MAIN(TestUtils)
 #include "utiltests.moc"
diff --git a/quotest/quotest.cpp b/quotest/quotest.cpp
@@ -111,7 +111,6 @@ private slots:
     TEST_DECL(addAndRemoveTag)
     TEST_DECL(markDirectChat)
     TEST_DECL(visitResources)
-    TEST_DECL(prettyPrintTests)
     // Add more tests above here
 
 public:
@@ -896,36 +895,6 @@ bool checkPrettyPrint(
     return result;
 }
 
-TEST_IMPL(prettyPrintTests)
-{
-    const bool prettyPrintTestResult = checkPrettyPrint(
-        { { "https://www.matrix.org",
-            R"(<a href="https://www.matrix.org">https://www.matrix.org</a>)" },
-//          { "www.matrix.org", // Doesn't work yet
-//             R"(<a href="https://www.matrix.org">www.matrix.org</a>)" },
-          { "smb://somewhere/file", "smb://somewhere/file" }, // Disallowed scheme
-          { "https:/something", "https:/something" }, // Malformed URL
-          { "https://matrix.to/#/!roomid:example.org",
-            R"(<a href="https://matrix.to/#/!roomid:example.org">https://matrix.to/#/!roomid:example.org</a>)" },
-          { "https://matrix.to/#/@user_id:example.org",
-            R"(<a href="https://matrix.to/#/@user_id:example.org">https://matrix.to/#/@user_id:example.org</a>)" },
-          { "https://matrix.to/#/#roomalias:example.org",
-            R"(<a href="https://matrix.to/#/#roomalias:example.org">https://matrix.to/#/#roomalias:example.org</a>)" },
-          { "https://matrix.to/#/##ircroomalias:example.org",
-            R"(<a href="https://matrix.to/#/##ircroomalias:example.org">https://matrix.to/#/##ircroomalias:example.org</a>)" },
-          { "me@example.org",
-            R"(<a href="mailto:me@example.org">me@example.org</a>)" },
-          { "mailto:me@example.org",
-            R"(<a href="mailto:me@example.org">mailto:me@example.org</a>)" },
-          { "!room_id:example.org",
-            R"(<a href="https://matrix.to/#/!room_id:example.org">!room_id:example.org</a>)" },
-          { "@user_id:example.org",
-            R"(<a href="https://matrix.to/#/@user_id:example.org">@user_id:example.org</a>)" },
-          { "#room_alias:example.org",
-            R"(<a href="https://matrix.to/#/#room_alias:example.org">#room_alias:example.org</a>)" } });
-    FINISH_TEST(prettyPrintTestResult);
-}
-
 void TestManager::conclude()
 {
     // Clean up the room (best effort)