Fix merging browser keys

keepassxreboot · May 2, 2020 · ec06dee · ec06dee
1 parent 5add012
commit ec06dee
Show file tree

Hide file tree

Showing 6 changed files with 26 additions and 17 deletions.
diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp
@@ -44,11 +44,9 @@
 
 const QString BrowserService::KEEPASSXCBROWSER_NAME = QStringLiteral("KeePassXC-Browser Settings");
 const QString BrowserService::KEEPASSXCBROWSER_OLD_NAME = QStringLiteral("keepassxc-browser Settings");
-const QString BrowserService::ASSOCIATE_KEY_PREFIX = QStringLiteral("KPXC_BROWSER_");
 static const QString KEEPASSXCBROWSER_GROUP_NAME = QStringLiteral("KeePassXC-Browser Passwords");
 static int KEEPASSXCBROWSER_DEFAULT_ICON = 1;
 // These are for the settings and password conversion
-const QString BrowserService::LEGACY_ASSOCIATE_KEY_PREFIX = QStringLiteral("Public Key: ");
 static const QString KEEPASSHTTP_NAME = QStringLiteral("KeePassHttp Settings");
 static const QString KEEPASSHTTP_GROUP_NAME = QStringLiteral("KeePassHttp Passwords");
 // Extra entry related options saved in custom data
@@ -324,7 +322,7 @@ QString BrowserService::storeKey(const QString& key)
             return {};
         }
 
-        contains = db->metadata()->customData()->contains(ASSOCIATE_KEY_PREFIX + id);
+        contains = db->metadata()->customData()->contains(CustomData::BrowserKeyPrefix + id);
         if (contains) {
             dialogResult = MessageBox::warning(nullptr,
                                                tr("KeePassXC: Overwrite existing key?"),
@@ -337,7 +335,7 @@ QString BrowserService::storeKey(const QString& key)
     } while (contains && dialogResult == MessageBox::Cancel);
 
     hideWindow();
-    db->metadata()->customData()->set(ASSOCIATE_KEY_PREFIX + id, key);
+    db->metadata()->customData()->set(CustomData::BrowserKeyPrefix + id, key);
     db->metadata()->customData()->set(QString("%1_%2").arg(CustomData::Created, id),
                                       Clock::currentDateTime().toString(Qt::SystemLocaleShortDate));
     return id;
@@ -350,7 +348,7 @@ QString BrowserService::getKey(const QString& id)
         return {};
     }
 
-    return db->metadata()->customData()->value(ASSOCIATE_KEY_PREFIX + id);
+    return db->metadata()->customData()->value(CustomData::BrowserKeyPrefix + id);
 }
 
 QJsonArray BrowserService::findMatchingEntries(const QString& id,
@@ -635,7 +633,7 @@ QList<Entry*> BrowserService::searchEntries(const QString& url, const QString& s
     // Check if database is connected with KeePassXC-Browser
     auto databaseConnected = [&](const QSharedPointer<Database>& db) {
         for (const StringPair& keyPair : keyList) {
-            QString key = db->metadata()->customData()->value(ASSOCIATE_KEY_PREFIX + keyPair.first);
+            QString key = db->metadata()->customData()->value(CustomData::BrowserKeyPrefix + keyPair.first);
             if (!key.isEmpty() && keyPair.second == key) {
                 return true;
             }
@@ -1164,13 +1162,14 @@ int BrowserService::moveKeysToCustomData(Entry* entry, const QSharedPointer<Data
 {
     int keyCounter = 0;
     for (const auto& key : entry->attributes()->keys()) {
-        if (key.contains(LEGACY_ASSOCIATE_KEY_PREFIX)) {
+        if (key.contains(CustomData::BrowserLegacyKeyPrefix)) {
             QString publicKey = key;
-            publicKey.remove(LEGACY_ASSOCIATE_KEY_PREFIX);
+            publicKey.remove(CustomData::BrowserLegacyKeyPrefix);
 
             // Add key to database custom data
-            if (db && !db->metadata()->customData()->contains(ASSOCIATE_KEY_PREFIX + publicKey)) {
-                db->metadata()->customData()->set(ASSOCIATE_KEY_PREFIX + publicKey, entry->attributes()->value(key));
+            if (db && !db->metadata()->customData()->contains(CustomData::BrowserKeyPrefix + publicKey)) {
+                db->metadata()->customData()->set(CustomData::BrowserKeyPrefix + publicKey,
+                                                  entry->attributes()->value(key));
                 ++keyCounter;
             }
         }

diff --git a/src/browser/BrowserService.h b/src/browser/BrowserService.h
@@ -70,8 +70,6 @@ class BrowserService : public QObject
 public:
     static const QString KEEPASSXCBROWSER_NAME;
     static const QString KEEPASSXCBROWSER_OLD_NAME;
-    static const QString ASSOCIATE_KEY_PREFIX;
-    static const QString LEGACY_ASSOCIATE_KEY_PREFIX;
     static const QString OPTION_SKIP_AUTO_SUBMIT;
     static const QString OPTION_HIDE_ENTRY;
     static const QString OPTION_ONLY_HTTP_AUTH;

diff --git a/src/core/CustomData.cpp b/src/core/CustomData.cpp
@@ -22,6 +22,8 @@
 
 const QString CustomData::LastModified = QStringLiteral("_LAST_MODIFIED");
 const QString CustomData::Created = QStringLiteral("_CREATED");
+const QString CustomData::BrowserKeyPrefix = QStringLiteral("KPXC_BROWSER_");
+const QString CustomData::BrowserLegacyKeyPrefix = QStringLiteral("Public Key: ");
 
 CustomData::CustomData(QObject* parent)
     : QObject(parent)
@@ -128,6 +130,11 @@ QDateTime CustomData::getLastModified() const
     return {};
 }
 
+bool CustomData::isProtectedCustomData(const QString& key) const
+{
+    return key.startsWith(CustomData::BrowserKeyPrefix) || key.startsWith(CustomData::Created);
+}
+
 bool CustomData::operator==(const CustomData& other) const
 {
     return (m_data == other.m_data);

diff --git a/src/core/CustomData.h b/src/core/CustomData.h
@@ -43,11 +43,14 @@ class CustomData : public QObject
     int dataSize() const;
     void copyDataFrom(const CustomData* other);
     QDateTime getLastModified() const;
+    bool isProtectedCustomData(const QString& key) const;
     bool operator==(const CustomData& other) const;
     bool operator!=(const CustomData& other) const;
 
     static const QString LastModified;
     static const QString Created;
+    static const QString BrowserKeyPrefix;
+    static const QString BrowserLegacyKeyPrefix;
 
 signals:
     void customDataModified();

diff --git a/src/core/Merger.cpp b/src/core/Merger.cpp
@@ -632,7 +632,9 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context)
 
         // Check missing keys from source. Remove those from target
         for (const auto& key : targetCustomDataKeys) {
-            if (!sourceMetadata->customData()->contains(key)) {
+            // Do not remove any browser keys
+            if (!sourceMetadata->customData()->contains(key)
+                && !sourceMetadata->customData()->isProtectedCustomData(key)) {
                 auto value = targetMetadata->customData()->value(key);
                 targetMetadata->customData()->remove(key);
                 changes << tr("Removed custom data %1 [%2]").arg(key, value);

diff --git a/src/gui/dbsettings/DatabaseSettingsWidgetBrowser.cpp b/src/gui/dbsettings/DatabaseSettingsWidgetBrowser.cpp
@@ -106,7 +106,7 @@ void DatabaseSettingsWidgetBrowser::removeSelectedKey()
     if (itemSelectionModel) {
         for (const QModelIndex& index : itemSelectionModel->selectedRows(0)) {
             QString key = index.data().toString();
-            key.insert(0, BrowserService::ASSOCIATE_KEY_PREFIX);
+            key.insert(0, CustomData::BrowserKeyPrefix);
             customData()->remove(key);
         }
         updateModel();
@@ -124,9 +124,9 @@ void DatabaseSettingsWidgetBrowser::updateModel()
     m_customDataModel->setHorizontalHeaderLabels({tr("Key"), tr("Value"), tr("Created")});
 
     for (const QString& key : customData()->keys()) {
-        if (key.startsWith(BrowserService::ASSOCIATE_KEY_PREFIX)) {
+        if (key.startsWith(CustomData::BrowserKeyPrefix)) {
             QString strippedKey = key;
-            strippedKey.remove(BrowserService::ASSOCIATE_KEY_PREFIX);
+            strippedKey.remove(CustomData::BrowserKeyPrefix);
             auto created = customData()->value(QString("%1_%2").arg(CustomData::Created, strippedKey));
             m_customDataModel->appendRow(QList<QStandardItem*>()
                                          << new QStandardItem(strippedKey)
@@ -171,7 +171,7 @@ void DatabaseSettingsWidgetBrowser::removeSharedEncryptionKeys()
 
     QStringList keysToRemove;
     for (const QString& key : m_db->metadata()->customData()->keys()) {
-        if (key.startsWith(BrowserService::ASSOCIATE_KEY_PREFIX)) {
+        if (key.startsWith(CustomData::BrowserKeyPrefix)) {
             keysToRemove << key;
         }
     }