From 139153d9a32ef9a10089ad2cda2ff972fe22eac2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sami=20V=C3=A4nttinen?= Date: Mon, 14 Aug 2023 05:18:24 +0300 Subject: [PATCH] Improve duplicate URL warning (#9635) Co-authored-by: varjolintu --- src/browser/BrowserService.cpp | 27 +++++++++++++++++++++++++++ src/browser/BrowserService.h | 1 + src/gui/entry/EditEntryWidget.cpp | 8 ++++++++ src/gui/entry/EditEntryWidget.h | 1 + src/gui/entry/EntryURLModel.cpp | 7 ++++--- tests/TestBrowser.cpp | 16 ++++++++++++++++ tests/TestBrowser.h | 1 + 7 files changed, 58 insertions(+), 3 deletions(-) diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index 9f7d29c26..b412409a5 100644 --- 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 index 9acdfb558..46bffef01 100644 --- a/src/browser/BrowserService.h +++ b/src/browser/BrowserService.h @@ -82,6 +82,7 @@ public: 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 index 6e3a606b2..1a71db445 100644 --- 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 index 582cdfbbc..27be1d339 100644 --- 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 index d43c8cb16..55d8dd51c 100644 --- a/src/gui/entry/EntryURLModel.cpp +++ b/src/gui/entry/EntryURLModel.cpp @@ -54,7 +54,7 @@ void EntryURLModel::setEntryAttributes(EntryAttributes* entryAttributes) endResetModel(); } -#include + 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 index c7d8bcb92..d7345537d 100644 --- 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 index 670188cc9..ed8146b57 100644 --- a/tests/TestBrowser.h +++ b/tests/TestBrowser.h @@ -52,6 +52,7 @@ private slots: void testValidURLs(); void testBestMatchingCredentials(); void testBestMatchingWithAdditionalURLs(); + void testIsUrlIdentical(); private: QList createEntries(QStringList& urls, Group* root) const;