Fix multiple TOTP issues

* Fix #9847 - don't provide TOTP values if settings are blank or completely wrong * Fix #6838 - don't reset the ui when creating a new entry and applying TOTP to it * Move totp source into the core folder
keepassxreboot · Jan 6, 2024 · 9f3b4dc · 9f3b4dc
1 parent 5d64292
commit 9f3b4dc
Show file tree

Hide file tree

Showing 16 changed files with 52 additions and 17 deletions.
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
@@ -59,6 +59,7 @@ set(keepassx_SOURCES
         core/TimeDelta.cpp
         core/TimeInfo.cpp
         core/Tools.cpp
+        core/Totp.cpp
         core/Translator.cpp
         core/UrlTools.cpp
         cli/Utils.cpp
@@ -194,8 +195,7 @@ set(keepassx_SOURCES
         streams/qtiocompressor.cpp
         streams/StoreDataStream.cpp
         streams/SymmetricCipherStream.cpp
-        quickunlock/QuickUnlockInterface.cpp
-        totp/totp.cpp)
+        quickunlock/QuickUnlockInterface.cpp)
 if(APPLE)
     set(keepassx_SOURCES
             ${keepassx_SOURCES}

diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp
@@ -24,7 +24,7 @@
 #include "core/Metadata.h"
 #include "core/PasswordHealth.h"
 #include "core/Tools.h"
-#include "totp/totp.h"
+#include "core/Totp.h"
 
 #include <QDir>
 #include <QRegularExpression>
@@ -566,7 +566,7 @@ void Entry::setTotp(QSharedPointer<Totp::Settings> settings)
     m_attributes->remove(Totp::ATTRIBUTE_SEED);
     m_attributes->remove(Totp::ATTRIBUTE_SETTINGS);
 
-    if (settings->key.isEmpty()) {
+    if (!settings || settings->key.isEmpty()) {
         m_data.totpSettings.reset();
     } else {
         m_data.totpSettings = std::move(settings);

diff --git a/src/totp/totp.cpp → src/core/Totp.cpp b/src/totp/totp.cpp → src/core/Totp.cpp
@@ -16,7 +16,7 @@
  *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "totp.h"
+#include "Totp.h"
 
 #include "core/Base32.h"
 #include "core/Clock.h"
@@ -59,6 +59,11 @@ static QString getNameForHashType(const Totp::Algorithm hashType)
 
 QSharedPointer<Totp::Settings> Totp::parseSettings(const QString& rawSettings, const QString& key)
 {
+    // Early out if both strings are empty
+    if (rawSettings.isEmpty() && key.isEmpty()) {
+        return {};
+    }
+
     // Create default settings
     auto settings = createSettings(key, DEFAULT_DIGITS, DEFAULT_STEP);
 
@@ -96,6 +101,11 @@ QSharedPointer<Totp::Settings> Totp::parseSettings(const QString& rawSettings, c
                 settings->algorithm = getHashTypeByName(query.queryItemValue("otpHashMode"));
             }
         } else {
+            if (settings->key.isEmpty()) {
+                // Legacy format cannot work with an empty key
+                return {};
+            }
+
             // Parse semi-colon separated values ([step];[digits|S])
             settings->format = StorageFormat::LEGACY;
             auto vars = rawSettings.split(";");

diff --git a/src/totp/totp.h → src/core/Totp.h b/src/totp/totp.h → src/core/Totp.h
diff --git a/src/format/OpVaultReaderSections.cpp b/src/format/OpVaultReaderSections.cpp
@@ -18,7 +18,7 @@
 #include "OpVaultReader.h"
 
 #include "core/Entry.h"
-#include "totp/totp.h"
+#include "core/Totp.h"
 
 #include <QDebug>
 #include <QJsonArray>

diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp
@@ -497,6 +497,10 @@ void DatabaseWidget::setupTotp()
 
     auto setupTotpDialog = new TotpSetupDialog(this, currentEntry);
     connect(setupTotpDialog, SIGNAL(totpUpdated()), SIGNAL(entrySelectionChanged()));
+    if (currentWidget() == m_editEntryWidget) {
+        // Entry is being edited, tell it when we are finished updating TOTP
+        connect(setupTotpDialog, SIGNAL(totpUpdated()), m_editEntryWidget, SLOT(updateTotp()));
+    }
     connect(this, &DatabaseWidget::databaseLockRequested, setupTotpDialog, &TotpSetupDialog::close);
     setupTotpDialog->open();
 }

diff --git a/src/gui/EntryPreviewWidget.cpp b/src/gui/EntryPreviewWidget.cpp
@@ -21,10 +21,10 @@
 
 #include "Application.h"
 #include "core/Config.h"
+#include "core/Totp.h"
 #include "gui/Clipboard.h"
 #include "gui/Font.h"
 #include "gui/Icons.h"
-#include "totp/totp.h"
 #if defined(WITH_XC_KEESHARE)
 #include "keeshare/KeeShare.h"
 #include "keeshare/KeeShareSettings.h"

diff --git a/src/gui/TotpDialog.cpp b/src/gui/TotpDialog.cpp
@@ -20,9 +20,9 @@
 #include "ui_TotpDialog.h"
 
 #include "core/Clock.h"
+#include "core/Totp.h"
 #include "gui/Clipboard.h"
 #include "gui/MainWindow.h"
-#include "totp/totp.h"
 
 #include <QPushButton>
 #include <QShortcut>

diff --git a/src/gui/TotpExportSettingsDialog.cpp b/src/gui/TotpExportSettingsDialog.cpp
@@ -17,11 +17,11 @@
 
 #include "TotpExportSettingsDialog.h"
 
+#include "core/Totp.h"
 #include "gui/Clipboard.h"
 #include "gui/MainWindow.h"
 #include "gui/SquareSvgWidget.h"
 #include "qrcode/QrCode.h"
-#include "totp/totp.h"
 
 #include <QBoxLayout>
 #include <QBuffer>

diff --git a/src/gui/TotpSetupDialog.cpp b/src/gui/TotpSetupDialog.cpp
@@ -19,8 +19,8 @@
 #include "ui_TotpSetupDialog.h"
 
 #include "core/Base32.h"
+#include "core/Totp.h"
 #include "gui/MessageBox.h"
-#include "totp/totp.h"
 
 TotpSetupDialog::TotpSetupDialog(QWidget* parent, Entry* entry)
     : QDialog(parent)

diff --git a/src/gui/csvImport/CsvImportWidget.cpp b/src/gui/csvImport/CsvImportWidget.cpp
@@ -22,9 +22,9 @@
 #include <QStringListModel>
 
 #include "core/Clock.h"
+#include "core/Totp.h"
 #include "format/KeePass2Writer.h"
 #include "gui/MessageBox.h"
-#include "totp/totp.h"
 
 // I wanted to make the CSV import GUI future-proof, so if one day you need a new field,
 // all you have to do is add a field to m_columnHeader, and the GUI will follow:
@@ -206,7 +206,7 @@ void CsvImportWidget::writeDatabase()
         auto otpString = m_parserModel->data(m_parserModel->index(r, 6));
         if (otpString.isValid() && !otpString.toString().isEmpty()) {
             auto totp = Totp::parseSettings(otpString.toString());
-            if (totp->key.isEmpty()) {
+            if (!totp || totp->key.isEmpty()) {
                 // Bare secret, use default TOTP settings
                 totp = Totp::parseSettings({}, otpString.toString());
             }

diff --git a/src/gui/entry/EditEntryWidget.cpp b/src/gui/entry/EditEntryWidget.cpp
@@ -115,6 +115,7 @@ EditEntryWidget::EditEntryWidget(QWidget* parent)
     m_entryModifiedTimer.setSingleShot(true);
     m_entryModifiedTimer.setInterval(0);
     connect(&m_entryModifiedTimer, &QTimer::timeout, this, [this] {
+        // TODO: Upon refactor of this widget, this needs to merge unsaved changes in the UI
         if (isVisible() && m_entry) {
             setForms(m_entry);
         }
@@ -711,6 +712,13 @@ void EditEntryWidget::toKeeAgentSettings(KeeAgentSettings& settings) const
     settings.setSaveAttachmentToTempFile(m_sshAgentSettings.saveAttachmentToTempFile());
 }
 
+void EditEntryWidget::updateTotp()
+{
+    if (m_entry) {
+        m_attributesModel->setEntryAttributes(m_entry->attributes());
+    }
+}
+
 void EditEntryWidget::browsePrivateKey()
 {
     auto fileName = fileDialog()->getOpenFileName(this, tr("Select private key"), FileDialog::getLastDir("sshagent"));
@@ -867,15 +875,15 @@ void EditEntryWidget::loadEntry(Entry* entry,
     m_create = create;
     m_history = history;
 
-    connect(m_entry, &Entry::modified, this, [this] { m_entryModifiedTimer.start(); });
-
     if (history) {
         setHeadline(QString("%1 \u2022 %2").arg(parentName, tr("Entry history")));
     } else {
         if (create) {
             setHeadline(QString("%1 \u2022 %2").arg(parentName, tr("Add entry")));
         } else {
             setHeadline(QString("%1 \u2022 %2 \u2022 %3").arg(parentName, entry->title(), tr("Edit entry")));
+            // Reload entry details if changed outside of the edit dialog
+            connect(m_entry, &Entry::modified, this, [this] { m_entryModifiedTimer.start(); });
         }
     }
 

diff --git a/src/gui/entry/EditEntryWidget.h b/src/gui/entry/EditEntryWidget.h
@@ -118,6 +118,7 @@ private slots:
     void updateSSHAgentAttachment();
     void updateSSHAgentAttachments();
     void updateSSHAgentKeyInfo();
+    void updateTotp();
     void browsePrivateKey();
     void addKeyToAgent();
     void removeKeyFromAgent();

diff --git a/tests/TestCsvExporter.cpp b/tests/TestCsvExporter.cpp
@@ -22,9 +22,9 @@
 #include <QTest>
 
 #include "core/Group.h"
+#include "core/Totp.h"
 #include "crypto/Crypto.h"
 #include "format/CsvExporter.h"
-#include "totp/totp.h"
 
 QTEST_GUILESS_MAIN(TestCsvExporter)
 

diff --git a/tests/TestOpVaultReader.cpp b/tests/TestOpVaultReader.cpp
@@ -20,9 +20,9 @@
 #include "config-keepassx-tests.h"
 #include "core/Group.h"
 #include "core/Metadata.h"
+#include "core/Totp.h"
 #include "crypto/Crypto.h"
 #include "format/OpVaultReader.h"
-#include "totp/totp.h"
 
 #include <QJsonObject>
 #include <QList>

diff --git a/tests/TestTotp.cpp b/tests/TestTotp.cpp
@@ -19,8 +19,8 @@
 #include "TestTotp.h"
 
 #include "core/Entry.h"
+#include "core/Totp.h"
 #include "crypto/Crypto.h"
-#include "totp/totp.h"
 
 #include <QTest>
 
@@ -97,6 +97,14 @@ void TestTotp::testParseSecret()
     QCOMPARE(settings->digits, 6u);
     QCOMPARE(settings->step, 30u);
     QCOMPARE(settings->algorithm, Totp::Algorithm::Sha1);
+
+    // Blank settings (expected failure)
+    settings = Totp::parseSettings("", "");
+    QVERIFY(settings.isNull());
+
+    // TOTP Settings with blank secret (expected failure)
+    settings = Totp::parseSettings("30;8", "");
+    QVERIFY(settings.isNull());
 }
 
 void TestTotp::testTotpCode()
@@ -164,4 +172,8 @@ void TestTotp::testEntryHistory()
     entry.setTotp(settings);
     QCOMPARE(entry.historyItems().size(), 2);
     QCOMPARE(entry.totpSettings()->key, QString("foo"));
+    // Nullptr Settings (expected reset of TOTP)
+    entry.setTotp(nullptr);
+    QVERIFY(!entry.hasTotp());
+    QCOMPARE(entry.historyItems().size(), 3);
 }