Add new Base32 implementation #1069
Conversation
|
I've rebased. |
src/core/Base32.cpp
Outdated
| } | ||
|
|
||
| // add pad characters | ||
| while (o < encodedData.size()) |
src/core/Base32.cpp
Outdated
| quint64 quantum = 0; | ||
| int nQuantumBytes = 5; | ||
|
|
||
| for (int n = 0; n < 8; n++) { |
There was a problem hiding this comment.
Ok, will change it.
src/core/Base32.cpp
Outdated
| quint64 mask = quint64(0xFF) << offset; | ||
| for (int n = offset; n >= 0; n -= 8) { | ||
| char c = static_cast<char>((quantum & mask) >> n); | ||
| data[o++] = c; |
There was a problem hiding this comment.
I would sleep better if you added a few range checking assertions here and for encodedData inside the encode() method when doing something like this with long-lived counters. QByteArray implicitly extends the array if you assign an element beyond the array's range. So we wouldn't get a buffer overflow, but probably wouldn't notice either.
There was a problem hiding this comment.
Good catch! This helped me find a bug in Base32::decode() affecting how the number of bytes of decoded data was being calculated, even though it didn't truncate the result, it was causing the array to be resized unnecessarily. I will be submitting the changes soon.
src/core/Optional.h
Outdated
| * by all the main compiler toolchains. | ||
| */ | ||
|
|
||
| template <typename T> class Optional |
There was a problem hiding this comment.
Do we really need our own Optional implementation? Wouldn't the same be possible with QVariant?
There was a problem hiding this comment.
I think we can make this change to reduce the code base size, as the variant type can serve as an optional.
There was a problem hiding this comment.
Just use QVariant::isNull() or QVariant::isValid().
There was a problem hiding this comment.
I'm almost done making this change, I decided for QVariant::isNull; as I understand thatQVariant::isValid is used for detecting when a type not defined for the variant (i.e. not registered with Q_DECLARE_METATYPE) is used.
src/totp/totp.cpp
Outdated
| | ((hmac[offset + 1] & 0xff) << 16) | ||
| | ((hmac[offset + 2] & 0xff) << 8) | ||
| | (hmac[offset + 3] & 0xff); | ||
| int binary = ((hmac[offset] & 0x7f) << 24) | ((hmac[offset + 1] & 0xff) << 16) | ((hmac[offset + 2] & 0xff) << 8) | |
There was a problem hiding this comment.
I liked the previous formatting better. It's hard to read on a single line.
There was a problem hiding this comment.
I agree with you, this was the result of running clang-format; I'll change it.
The bug that was fixed, was affecting how the number of bytes of decoded data was calculated and thus, even though it didn't truncate the result, it was causing the array to be resized unnecessarily.
|
@phoerious I've finished making the changes solicited, please review them. |
|
Nice work. Thanks! |
- 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
Provides a port of the new Base32 implementation, plus fixes to make it compatible with Google Authenticator's implementation, that was previously introduced in PRs #984 and #1001.
Motivation and context
See issue #1022.
How has this been tested?
Manually on Linux.
Screenshots (if appropriate):
N/A.
Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]