Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve duplicate URL warning #9635

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/browser/BrowserService.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions src/browser/BrowserService.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
8 changes: 8 additions & 0 deletions src/gui/entry/EditEntryWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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()
Expand Down
1 change: 1 addition & 0 deletions src/gui/entry/EditEntryWidget.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ private slots:
void removeCurrentURL();
void editCurrentURL();
void updateCurrentURL();
void entryURLEdited(const QString& url);
#endif

private:
Expand Down
7 changes: 4 additions & 3 deletions src/gui/entry/EntryURLModel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ void EntryURLModel::setEntryAttributes(EntryAttributes* entryAttributes)

endResetModel();
}
#include <QDebug>

QVariant EntryURLModel::data(const QModelIndex& index, int role) const
{
if (!index.isValid()) {
Expand All @@ -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);
Expand Down
16 changes: 16 additions & 0 deletions tests/TestBrowser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
}
1 change: 1 addition & 0 deletions tests/TestBrowser.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ private slots:
void testValidURLs();
void testBestMatchingCredentials();
void testBestMatchingWithAdditionalURLs();
void testIsUrlIdentical();

private:
QList<Entry*> createEntries(QStringList& urls, Group* root) const;
Expand Down