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.

Sign up for GitHub

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

Corrected issue where blank urls resulted in spurious returns to KeepassHTTP #1031

Merged
merged 1 commit into from
Oct 3, 2017
Merged

Corrected issue where blank urls resulted in spurious returns to KeepassHTTP #1031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 18 additions & 15 deletions src/core/Entry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,28 +801,31 @@ QString Entry::resolvePlaceholder(const QString& str) const

QString Entry::resolveUrl(const QString& url) const
{
#ifdef WITH_XC_HTTP
QString newUrl = url;
if (!url.contains("://")) {
if (!url.isEmpty() && !url.contains("://")) {
// URL doesn't have a protocol, add https by default
newUrl.prepend("https://");
}
QUrl tempUrl = QUrl(newUrl);

if (tempUrl.isValid()) {
if (tempUrl.scheme() == "cmd") {
// URL is a cmd, hopefully the second argument is an URL
QStringList cmd = newUrl.split(" ");
if (cmd.size() > 1) {
return resolveUrl(cmd[1].remove("'").remove("\""));
if (newUrl.startsWith("cmd://")) {
QStringList cmdList = newUrl.split(" ");
for (int i=1; i < cmdList.size(); ++i) {
// Don't pass arguments to the resolveUrl function (they look like URL's)
if (!cmdList[i].startsWith("-") && !cmdList[i].startsWith("/")) {
return resolveUrl(cmdList[i].remove(QRegExp("'|\"")));
}
} else if (tempUrl.scheme() == "http" || tempUrl.scheme() == "https") {
// URL is nice
return tempUrl.url();
}

// No URL in this command
return QString("");
}
#else
Q_UNUSED(url);
#endif

// Validate the URL
QUrl tempUrl = QUrl(newUrl);
if (tempUrl.isValid() && (tempUrl.scheme() == "http" || tempUrl.scheme() == "https")) {
return tempUrl.url();
}

// No valid http URL's found
return QString("");
}
28 changes: 28 additions & 0 deletions tests/TestEntry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

#include "TestEntry.h"
#include "config-keepassx-tests.h"

#include <QTest>

Expand Down Expand Up @@ -130,3 +131,30 @@ void TestEntry::testClone()

delete entryOrg;
}

void TestEntry::testResolveUrl()
{
Entry* entry = new Entry();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be deleted? Or maybe make resolveUrl a static function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter in a test. Technically it should be but it really doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok made the change 😝

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QUrl may be usable, but WITH_XC_HTTP is also for those people who don't like networking functionality inside their password manager's code.

Copy link
Member Author

@droidmonkey droidmonkey Oct 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no networking code in here. QUrl is just a container.

Copy link
Member

@phoerious phoerious Oct 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I thought QUrl would also do some resolver work. But I guess I've just been doing to much Java as of late. :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was thinking the same thing when I first write that piece of code :D

QString testUrl("www.google.com");
QString testCmd("cmd://firefox " + testUrl);
QString testComplexCmd("cmd://firefox --start-now --url 'http://" + testUrl + "' --quit");
QString nonHttpUrl("ftp://google.com");
QString noUrl("random text inserted here");

// Test standard URL's
QCOMPARE(entry->resolveUrl(""), QString(""));
QCOMPARE(entry->resolveUrl(testUrl), "https://" + testUrl);
QCOMPARE(entry->resolveUrl("http://" + testUrl), "http://" + testUrl);
// Test cmd:// with no URL
QCOMPARE(entry->resolveUrl("cmd://firefox"), QString(""));
QCOMPARE(entry->resolveUrl("cmd://firefox --no-url"), QString(""));
// Test cmd:// with URL's
QCOMPARE(entry->resolveUrl(testCmd), "https://" + testUrl);
QCOMPARE(entry->resolveUrl(testComplexCmd), "http://" + testUrl);
// Test non-http URL
QCOMPARE(entry->resolveUrl(nonHttpUrl), QString(""));
// Test no URL
QCOMPARE(entry->resolveUrl(noUrl), QString(""));

delete entry;
}
1 change: 1 addition & 0 deletions tests/TestEntry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ private slots:
void testHistoryItemDeletion();
void testCopyDataFrom();
void testClone();
void testResolveUrl();
};

#endif // KEEPASSX_TESTENTRY_H