Corrected issue where blank urls resulted in spurious returns to KeepassHTTP

[droidmonkey]

Description

Fixes #1017

How has this been tested?

Manually with debugger

Types of changes

• ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

• ✅ I have read the CONTRIBUTING document. [REQUIRED]
• ✅ My code follows the code style of this project. [REQUIRED]
• ✅ All new and existing tests passed. [REQUIRED]
• ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

[yan12125]

How about adding a test case? For example:

diff --git a/tests/TestEntry.cpp b/tests/TestEntry.cpp
index 4d34cf31..fe37ca22 100644
--- a/tests/TestEntry.cpp
+++ b/tests/TestEntry.cpp
@@ -19,6 +19,7 @@
 
 #include <QTest>
 
+#include "config-keepassx-tests.h"
 #include "core/Entry.h"
 #include "crypto/Crypto.h"
 
@@ -130,3 +131,12 @@ void TestEntry::testClone()
 
     delete entryOrg;
 }
+
+void TestEntry::testResolveUrl()
+{
+#ifdef WITH_XC_HTTP
+    Entry* entry = new Entry();
+    QCOMPARE(entry->resolveUrl(""), QString(""));
+    delete entry;
+#endif
+}
diff --git a/tests/TestEntry.h b/tests/TestEntry.h
index 0c97c0b9..3f6d20ee 100644
--- a/tests/TestEntry.h
+++ b/tests/TestEntry.h
@@ -31,6 +31,7 @@ private slots:
     void testHistoryItemDeletion();
     void testCopyDataFrom();
     void testClone();
+    void testResolveUrl();
 };
 
 #endif // KEEPASSX_TESTENTRY_H

testResolveUrl() should have more tests. This is just a proof-of-concept.

[droidmonkey]

After reading the function a couple of times I don't like how the non-HTTP case returns an empty string. Shouldn't we just pass-through the input string? Seems like the behavior of this function is very complex and sometimes undefined.

[droidmonkey]

OK, so after doing a little test-driven-development I found that the resolveUrl function was completely broken in a variety of cases. I corrected those and wrote many test cases for it to be run against. I also found that the WITH_XC_HTTP guards were completely unnecessary as QUrl is usable regardless of the HTTP presence.

[yan12125]

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

[droidmonkey]

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

[droidmonkey]

ok made the change 😝

[phoerious]

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.

[droidmonkey]

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

[phoerious]

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

[TheZ3ro]

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

[TheZ3ro]

👍 for tests