Improve duplicate URL warning (#9635)

Co-authored-by: varjolintu <sami.vanttinen@protonmail.com>
keepassxreboot · Aug 14, 2023 · 139153d · 139153d
1 parent eee25a1
commit 139153d
Show file tree

Hide file tree

Showing 7 changed files with 58 additions and 3 deletions.
diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp
@@ -544,6 +544,33 @@ bool BrowserService::isPasswordGeneratorRequested() const
     return m_passwordGeneratorRequested;
 }
 
+// Returns true if URLs are identical. Paths with "/" are removed during comparison.
+// URLs without scheme reverts to https.
+// Special handling is needed because QUrl::matches() with QUrl::StripTrailingSlash does not strip "/" paths.
+bool BrowserService::isUrlIdentical(const QString& first, const QString& second) const
+{
+    auto trimUrl = [](QString url) {
+        url = url.trimmed();
+        if (url.endsWith("/")) {
+            url.remove(url.length() - 1, 1);
+        }
+
+        return url;
+    };
+
+    if (first.isEmpty() || second.isEmpty()) {
+        return false;
+    }
+
+    const auto firstUrl = trimUrl(first);
+    const auto secondUrl = trimUrl(second);
+    if (firstUrl == secondUrl) {
+        return true;
+    }
+
+    return QUrl(firstUrl).matches(QUrl(secondUrl), QUrl::StripTrailingSlash);
+}
+
 QString BrowserService::storeKey(const QString& key)
 {
     auto db = getDatabase();

diff --git a/src/browser/BrowserService.h b/src/browser/BrowserService.h
@@ -82,6 +82,7 @@ class BrowserService : public QObject
     QString getCurrentTotp(const QString& uuid);
     void showPasswordGenerator(const KeyPairMessage& keyPairMessage);
     bool isPasswordGeneratorRequested() const;
+    bool isUrlIdentical(const QString& first, const QString& second) const;
 
     void addEntry(const EntryParameters& entryParameters,
                   const QString& group,

diff --git a/src/gui/entry/EditEntryWidget.cpp b/src/gui/entry/EditEntryWidget.cpp
@@ -160,6 +160,9 @@ void EditEntryWidget::setupMain()
     connect(m_mainUi->fetchFaviconButton, SIGNAL(clicked()), m_iconsWidget, SLOT(downloadFavicon()));
     connect(m_mainUi->urlEdit, SIGNAL(textChanged(QString)), m_iconsWidget, SLOT(setUrl(QString)));
     m_mainUi->urlEdit->enableVerifyMode();
+#endif
+#ifdef WITH_XC_BROWSER
+    connect(m_mainUi->urlEdit, SIGNAL(textChanged(QString)), this, SLOT(entryURLEdited(const QString&)));
 #endif
     connect(m_mainUi->expireCheck, &QCheckBox::toggled, [&](bool enabled) {
         m_mainUi->expireDatePicker->setEnabled(enabled);
@@ -398,6 +401,11 @@ void EditEntryWidget::updateCurrentURL()
         m_browserUi->removeURLButton->setEnabled(false);
     }
 }
+
+void EditEntryWidget::entryURLEdited(const QString& url)
+{
+    m_additionalURLsDataModel->setEntryUrl(url);
+}
 #endif
 
 void EditEntryWidget::setupProperties()

diff --git a/src/gui/entry/EditEntryWidget.h b/src/gui/entry/EditEntryWidget.h
@@ -132,6 +132,7 @@ private slots:
     void removeCurrentURL();
     void editCurrentURL();
     void updateCurrentURL();
+    void entryURLEdited(const QString& url);
 #endif
 
 private:

diff --git a/src/gui/entry/EntryURLModel.cpp b/src/gui/entry/EntryURLModel.cpp
@@ -54,7 +54,7 @@ void EntryURLModel::setEntryAttributes(EntryAttributes* entryAttributes)
 
     endResetModel();
 }
-#include <QDebug>
+
 QVariant EntryURLModel::data(const QModelIndex& index, int role) const
 {
     if (!index.isValid()) {
@@ -70,10 +70,11 @@ QVariant EntryURLModel::data(const QModelIndex& index, int role) const
     const auto urlValid = Tools::checkUrlValid(value);
 
     // Check for duplicate URLs in the attribute list. Excludes the current key/value from the comparison.
-    auto customAttributeKeys = m_entryAttributes->customKeys();
+    auto customAttributeKeys = m_entryAttributes->customKeys().filter(BrowserService::ADDITIONAL_URL);
     customAttributeKeys.removeOne(key);
-    const auto duplicateUrl = m_entryAttributes->values(customAttributeKeys).contains(value) || value == m_entryUrl;
 
+    const auto duplicateUrl = m_entryAttributes->values(customAttributeKeys).contains(value)
+                              || browserService()->isUrlIdentical(value, m_entryUrl);
     if (role == Qt::BackgroundRole && (!urlValid || duplicateUrl)) {
         StateColorPalette statePalette;
         return statePalette.color(StateColorPalette::ColorRole::Error);

diff --git a/tests/TestBrowser.cpp b/tests/TestBrowser.cpp
@@ -741,3 +741,19 @@ void TestBrowser::testBestMatchingWithAdditionalURLs()
     QCOMPARE(sorted.length(), 1);
     QCOMPARE(sorted[0]->url(), urls[0]);
 }
+
+void TestBrowser::testIsUrlIdentical()
+{
+    QVERIFY(browserService()->isUrlIdentical("https://example.com", "https://example.com"));
+    QVERIFY(browserService()->isUrlIdentical("https://example.com", "  https://example.com  "));
+    QVERIFY(!browserService()->isUrlIdentical("https://example.com", "https://example2.com"));
+    QVERIFY(!browserService()->isUrlIdentical("https://example.com/", "https://example.com/#login"));
+    QVERIFY(browserService()->isUrlIdentical("https://example.com", "https://example.com/"));
+    QVERIFY(browserService()->isUrlIdentical("https://example.com/", "https://example.com"));
+    QVERIFY(browserService()->isUrlIdentical("https://example.com/  ", "  https://example.com"));
+    QVERIFY(!browserService()->isUrlIdentical("https://example.com/", "  example.com"));
+    QVERIFY(browserService()->isUrlIdentical("https://example.com/path/to/nowhere",
+                                             "https://example.com/path/to/nowhere/"));
+    QVERIFY(!browserService()->isUrlIdentical("https://example.com/", "://example.com/"));
+    QVERIFY(browserService()->isUrlIdentical("ftp://127.0.0.1/", "ftp://127.0.0.1"));
+}
diff --git a/tests/TestBrowser.h b/tests/TestBrowser.h
@@ -52,6 +52,7 @@ private slots:
     void testValidURLs();
     void testBestMatchingCredentials();
     void testBestMatchingWithAdditionalURLs();
+    void testIsUrlIdentical();
 
 private:
     QList<Entry*> createEntries(QStringList& urls, Group* root) const;