Corrected issue where blank urls resulted in spurious returns to KeepassHTTP#1031
Corrected issue where blank urls resulted in spurious returns to KeepassHTTP#1031droidmonkey merged 1 commit intorelease/2.2.2from
Conversation
|
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_HtestResolveUrl() should have more tests. This is just a proof-of-concept. |
|
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. |
e7bb6c3 to
02ddb2b
Compare
|
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. |
02ddb2b to
aae7b5a
Compare
|
|
||
| void TestEntry::testResolveUrl() | ||
| { | ||
| Entry* entry = new Entry(); |
There was a problem hiding this comment.
This should be deleted? Or maybe make resolveUrl a static function.
There was a problem hiding this comment.
It doesn't matter in a test. Technically it should be but it really doesn't matter.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There is no networking code in here. QUrl is just a container.
There was a problem hiding this comment.
True. I thought QUrl would also do some resolver work. But I guess I've just been doing to much Java as of late. :-)
There was a problem hiding this comment.
Was thinking the same thing when I first write that piece of code :D
aae7b5a to
65829b6
Compare
- Fixed entries with empty URLs being reported to KeePassHTTP clients [#1031] - Fixed YubiKey detection and enabled CLI tool for AppImage binary [#1100] - Added AppStream description [#1082] - Improved TOTP compatibility and added new Base32 implementation [#1069] - Fixed error handling when processing invalid cipher stream [#1099] - Fixed double warning display when opening a database [#1037] - Fixed unlocking databases with --pw-stdin [#1087] - Added ability to override QT_PLUGIN_PATH environment variable for AppImages [#1079] - Fixed transform seed not being regenerated when saving the database [#1068] - Fixed only one YubiKey slot being polled [#1048] - Corrected an issue with entry icons while merging [#1008] - Corrected desktop and tray icons in Snap package [#1030] - Fixed screen lock and Google fallback settings [#1029]
Description
Fixes #1017
How has this been tested?
Manually with debugger
Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]