From f82ebbf9c62d0899168557553d1dde0348850a90 Mon Sep 17 00:00:00 2001
From: Alexey Rusakov <Kitsune-Ral@users.sf.net>
Date: Sat, 21 Dec 2024 22:26:51 +0100
Subject: [PATCH] AccountSettings: escape forward- and backslashes in MXIDs

Fixes #842.
---
 Quotient/settings.cpp      | 22 +++++++++++++++++++--
 Quotient/settings.h        |  8 +++++++-
 autotests/CMakeLists.txt   |  1 +
 autotests/testsettings.cpp | 40 ++++++++++++++++++++++++++++++++++++++
 quotest/quotest.cpp        |  1 +
 5 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 autotests/testsettings.cpp

diff --git a/Quotient/settings.cpp b/Quotient/settings.cpp
index 40e547db0..4a4ce0e01 100644
--- a/Quotient/settings.cpp
+++ b/Quotient/settings.cpp
@@ -3,6 +3,8 @@
 
 #include "settings.h"
 
+#include "ranges_extras.h"
+
 #include <QtCore/QUrl>
 
 using namespace Quotient;
@@ -17,6 +19,20 @@ void Settings::setLegacyNames(const QString& organizationName,
     legacyApplicationName = applicationName;
 }
 
+QString Settings::escapedForSettings(QString key)
+{
+    key.replace(u'/', u"%2F"_s);
+    key.replace(u'\\', u"%5C"_s);
+    return key;
+}
+
+QString Settings::unescapedFromSettings(QString key)
+{
+    key.replace(u"%2F"_s, u"/"_s);
+    key.replace(u"%5C"_s, u"\\"_s);
+    return key;
+}
+
 Settings::Settings(QObject* parent) : QSettings(parent)
 {}
 
@@ -57,6 +73,8 @@ QStringList Settings::childGroups() const
     for (const auto& g: legacyGroups)
         if (!groups.contains(g))
             groups.push_back(g);
+    if (group() == u"Accounts")
+        std::ranges::for_each(groups, [](QString& g) { g = unescapedFromSettings(g); }); // See #842
     return groups;
 }
 
@@ -78,7 +96,7 @@ QStringList SettingsGroup::childGroups() const
 {
     const_cast<SettingsGroup*>(this)->beginGroup(groupPath);
     const_cast<QSettings&>(legacySettings).beginGroup(groupPath);
-    QStringList l = Settings::childGroups();
+    auto l = Settings::childGroups();
     const_cast<SettingsGroup*>(this)->endGroup();
     const_cast<QSettings&>(legacySettings).endGroup();
     return l;
@@ -88,7 +106,7 @@ void SettingsGroup::remove(const QString& key)
 {
     QString fullKey { groupPath };
     if (!key.isEmpty())
-        fullKey += u'/' + key;
+        fullKey += u'/' + escapedForSettings(key);
     Settings::remove(fullKey);
 }
 
diff --git a/Quotient/settings.h b/Quotient/settings.h
index 3b7c95305..b35e06a47 100644
--- a/Quotient/settings.h
+++ b/Quotient/settings.h
@@ -69,6 +69,12 @@ class QUOTIENT_API Settings : public QSettings {
     Q_INVOKABLE bool contains(const QString& key) const;
     Q_INVOKABLE QStringList childGroups() const;
 
+    //! Escape forward- and backslashes in keys because QSettings doesn't (see #842)
+    static QString escapedForSettings(QString key);
+
+    //! Unescape `\` and `/` in keys stored with escapedForSettings()
+    static QString unescapedFromSettings(QString key);
+
 private:
     static QString legacyOrganizationName;
     static QString legacyApplicationName;
@@ -128,7 +134,7 @@ class QUOTIENT_API AccountSettings : public SettingsGroup {
                    WRITE setEncryptionAccountPickle)
 public:
     explicit AccountSettings(const QString& accountId, QObject* parent = nullptr)
-        : SettingsGroup("Accounts/"_L1 + accountId, parent)
+        : SettingsGroup("Accounts/"_L1 + escapedForSettings(accountId), parent)
     {}
 
     QString userId() const;
diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt
index a8023a351..28540d2c1 100644
--- a/autotests/CMakeLists.txt
+++ b/autotests/CMakeLists.txt
@@ -29,4 +29,5 @@ quotient_add_test(NAME testcryptoutils)
 quotient_add_test(NAME testkeyverification)
 quotient_add_test(NAME testcrosssigning)
 quotient_add_test(NAME testkeyimport)
+quotient_add_test(NAME testsettings)
 quotient_add_test(NAME testthread)
diff --git a/autotests/testsettings.cpp b/autotests/testsettings.cpp
new file mode 100644
index 000000000..0fd77b95d
--- /dev/null
+++ b/autotests/testsettings.cpp
@@ -0,0 +1,40 @@
+// SPDX-FileCopyrightText: The Quotient Project Contributors
+// SPDX-License-Identifier: LGPL-3.0-or-later
+
+#include <QTest>
+
+#include <Quotient/settings.h>
+
+class TestSettings : public QObject {
+    Q_OBJECT
+private slots:
+    void accountSettings();
+};
+
+using namespace Qt::Literals;
+using Quotient::Settings, Quotient::SettingsGroup, Quotient::AccountSettings;
+
+void TestSettings::accountSettings()
+{
+    static const auto AccountsGroupName = u"Accounts"_s;
+
+    QSettings qSettings{};
+    qSettings.remove(AccountsGroupName);
+    const auto mxId = u"@user/with\\slashes:example.org"_s; // Test #842
+    const auto escapedMxId = Settings::escapedForSettings(mxId);
+    AccountSettings(mxId).setHomeserver(QUrl(u"https://example.org"_s));
+    qSettings.sync();
+    qSettings.beginGroup(AccountsGroupName);
+    // NB: QSettings::contains() doesn't work on groups, only on leaf keys; hence childGroups below
+    auto childGroups = qSettings.childGroups();
+    QVERIFY(childGroups.contains(escapedMxId));
+    QVERIFY(SettingsGroup(AccountsGroupName).childGroups().contains(mxId));
+    SettingsGroup(AccountsGroupName).remove(mxId);
+    qSettings.sync();
+    childGroups = qSettings.childGroups();
+    QVERIFY(!childGroups.contains(escapedMxId));
+    qSettings.endGroup();
+}
+
+QTEST_GUILESS_MAIN(TestSettings)
+#include "testsettings.moc"
diff --git a/quotest/quotest.cpp b/quotest/quotest.cpp
index df43638fe..cec6b4315 100644
--- a/quotest/quotest.cpp
+++ b/quotest/quotest.cpp
@@ -3,6 +3,7 @@
 
 #include <Quotient/connection.h>
 #include <Quotient/room.h>
+#include <Quotient/settings.h>
 #include <Quotient/thread.h>
 #include <Quotient/user.h>
 #include <Quotient/uriresolver.h>