Don't add space to invalid TOTP strings

* Fixes #11357
* Introduces validity parameter to TOTP generator function for future use elsewhere in the code base
* Fixes this in preview panel and TOTP dialog
* Disable actions to copy/show TOTP if the settings are invalid
* Show an error message on the TOTP setup dialog if the settings are invalid
* Show a TOTP icon with an x if the settings are invalid

COPYING

@@ -223,6 +223,7 @@ Files: share/icons/application/scalable/actions/application-exit.svg
        share/icons/application/scalable/actions/totp-copy.svg
        share/icons/application/scalable/actions/totp-copy-password.svg
        share/icons/application/scalable/actions/totp-edit.svg
+       share/icons/application/scalable/actions/totp-invalid.svg
        share/icons/application/scalable/actions/trash.svg
        share/icons/application/scalable/actions/url-copy.svg
        share/icons/application/scalable/actions/user-guide.svg

share/icons/application/scalable/actions/totp-invalid.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M14.47 15.08L11 13V7H12.5V12.25L15.58 14.08C15.17 14.36 14.79 14.7 14.47 15.08M13.08 19.92C12.72 19.97 12.37 20 12 20C7.58 20 4 16.42 4 12S7.58 4 12 4 20 7.58 20 12C20 12.37 19.97 12.72 19.92 13.08C20.61 13.18 21.25 13.4 21.84 13.72C21.94 13.16 22 12.59 22 12C22 6.5 17.5 2 12 2S2 6.5 2 12C2 17.5 6.47 22 12 22C12.59 22 13.16 21.94 13.72 21.84C13.4 21.25 13.18 20.61 13.08 19.92M21.12 15.46L19 17.59L16.88 15.47L15.47 16.88L17.59 19L15.47 21.12L16.88 22.54L19 20.41L21.12 22.54L22.54 21.12L20.41 19L22.54 16.88L21.12 15.46Z" /></svg>

share/icons/icons.qrc

@@ -92,6 +92,7 @@
         <file>application/scalable/actions/totp-copy.svg</file>
         <file>application/scalable/actions/totp-copy-password.svg</file>
         <file>application/scalable/actions/totp-edit.svg</file>
+        <file>application/scalable/actions/totp-invalid.svg</file>
         <file>application/scalable/actions/trash.svg</file>
         <file>application/scalable/actions/url-copy.svg</file>
         <file>application/scalable/actions/user-guide.svg</file>

share/translations/keepassxc_en.ts

@@ -724,6 +724,10 @@
         <source>Invalid placeholder: %1</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Entry has invalid TOTP settings</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>AutoTypeAssociationsModel</name>
@@ -9247,6 +9251,16 @@ This option is deprecated, use --set-key-file instead.</source>
         <source>Warning: the chosen wordlist is smaller than the minimum recommended size!</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Invalid Step</source>
+        <comment>TOTP</comment>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Invalid Digits</source>
+        <comment>TOTP</comment>
+        <translation type="unfinished"></translation>
+    </message>
     <message>
         <source>Fit</source>
         <translation type="unfinished"></translation>
@@ -10317,6 +10331,10 @@ Example: JBSWY3DPEHPK3PXP</source>
         <source>Are you sure you want to delete TOTP settings for this entry?</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Error: secret key is invalid</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>URLEdit</name>

src/autotype/AutoType.cpp

@@ -637,11 +637,17 @@ AutoType::parseSequence(const QString& entrySequence, const Entry* entry, QStrin
             // Platform-specific field clearing
             actions << QSharedPointer<AutoTypeClearField>::create();
         } else if (placeholder == "totp") {
+            if (entry->hasValidTotp()) {
                 // Entry totp (requires special handling)
                 QString totp = entry->totp();
                 for (const auto& ch : totp) {
                     actions << QSharedPointer<AutoTypeKey>::create(ch);
                 }
+            } else if (entry->hasTotp()) {
+                // Entry has TOTP configured but invalid settings
+                error = tr("Entry has invalid TOTP settings");
+                return {};
+            }
         } else if (placeholder.startsWith("pickchars")) {
             // Reset to the original capture to preserve case
             placeholder = match.captured(3);

src/autotype/AutoTypeSelectDialog.cpp

@@ -264,7 +264,7 @@ void AutoTypeSelectDialog::updateActionMenu(const AutoTypeMatch& match)
 
     bool hasUsername = !match.first->username().isEmpty();
     bool hasPassword = !match.first->password().isEmpty();
-    bool hasTotp = match.first->hasTotp();
+    bool hasTotp = match.first->hasValidTotp();
 
     for (auto action : m_actionMenu->actions()) {
         auto prop = action->property(MENU_FIELD_PROP_NAME);

src/browser/BrowserService.cpp

@@ -1192,7 +1192,7 @@ QJsonObject BrowserService::prepareEntry(const Entry* entry)
     res["uuid"] = entry->resolveMultiplePlaceholders(entry->uuidToHex());
     res["group"] = entry->resolveMultiplePlaceholders(entry->group()->name());
 
-    if (entry->hasTotp()) {
+    if (entry->hasValidTotp()) {
         res["totp"] = entry->totp();
     }
 

src/core/Entry.cpp

@@ -570,6 +570,12 @@ bool Entry::hasTotp() const
     return !m_data.totpSettings.isNull();
 }
 
+bool Entry::hasValidTotp() const
+{
+    auto error = Totp::checkValidSettings(m_data.totpSettings);
+    return error.isEmpty();
+}
+
 bool Entry::hasPasskey() const
 {
     return m_attributes->hasPasskey();
@@ -581,10 +587,13 @@ void Entry::removePasskey()
     removeTag(tr("Passkey"));
 }
 
-QString Entry::totp() const
+QString Entry::totp(bool* isValid) const
 {
     if (hasTotp()) {
-        return Totp::generateTotp(m_data.totpSettings);
+        return Totp::generateTotp(m_data.totpSettings, isValid);
+    }
+    if (isValid) {
+        *isValid = false;
     }
     return {};
 }

src/core/Entry.h

@@ -109,7 +109,7 @@ public:
     QString password() const;
     QString notes() const;
     QString attribute(const QString& key) const;
-    QString totp() const;
+    QString totp(bool* isValid = nullptr) const;
     QString totpSettingsString() const;
     QSharedPointer<Totp::Settings> totpSettings() const;
     Group* previousParentGroup();
@@ -126,6 +126,7 @@ public:
     void removePasskey();
 
     bool hasTotp() const;
+    bool hasValidTotp() const;
     bool isExpired() const;
     bool willExpireInDays(int days) const;
     void expireNow();

src/core/Totp.cpp

@@ -210,12 +210,33 @@ QString Totp::writeSettings(const QSharedPointer<Totp::Settings>& settings,
     }
 }
 
-QString Totp::generateTotp(const QSharedPointer<Totp::Settings>& settings, const quint64 time)
+QString Totp::checkValidSettings(const QSharedPointer<Totp::Settings>& settings)
 {
-    Q_ASSERT(!settings.isNull());
     if (settings.isNull()) {
         return QObject::tr("Invalid Settings", "TOTP");
     }
+    QVariant secret = Base32::decode(Base32::sanitizeInput(settings->key.toLatin1()));
+    if (secret.isNull()) {
+        return QObject::tr("Invalid Key", "TOTP");
+    }
+    if (settings->step == 0) {
+        return QObject::tr("Invalid Step", "TOTP");
+    }
+    if (settings->digits == 0) {
+        return QObject::tr("Invalid Digits", "TOTP");
+    }
+    return {};
+}
+
+QString Totp::generateTotp(const QSharedPointer<Totp::Settings>& settings, bool* isValid, const quint64 time)
+{
+    auto error = checkValidSettings(settings);
+    if (!error.isEmpty()) {
+        if (isValid) {
+            *isValid = false;
+        }
+        return error;
+    }
 
     const Encoder& encoder = settings->encoder;
     uint step = settings->step;
@@ -229,9 +250,6 @@ QString Totp::generateTotp(const QSharedPointer<Totp::Settings>& settings, const
     }
 
     QVariant secret = Base32::decode(Base32::sanitizeInput(settings->key.toLatin1()));
-    if (secret.isNull()) {
-        return QObject::tr("Invalid Key", "TOTP");
-    }
 
     QCryptographicHash::Algorithm cryptoHash;
     switch (settings->algorithm) {
@@ -274,6 +292,9 @@ QString Totp::generateTotp(const QSharedPointer<Totp::Settings>& settings, const
         retval[pos] = encoder.alphabet[int(password % encoder.alphabet.size())];
         password /= encoder.alphabet.size();
     }
+    if (isValid) {
+        *isValid = true;
+    }
     return retval;
 }
 

src/core/Totp.h

@@ -91,8 +91,10 @@ namespace Totp
                           const QString& title = {},
                           const QString& username = {},
                           bool forceOtp = false);
-
+    // Returns an empty string if settings are valid, otherwise an error message is supplied
-    QString generateTotp(const QSharedPointer<Totp::Settings>& settings, const quint64 time = 0ull);
+    QString checkValidSettings(const QSharedPointer<Totp::Settings>& settings);
+    QString
+    generateTotp(const QSharedPointer<Totp::Settings>& settings, bool* isValid = nullptr, const quint64 time = 0ull);
 
     bool hasCustomSettings(const QSharedPointer<Totp::Settings>& settings);
 

src/fdosecrets/objects/Item.cpp

@@ -125,7 +125,7 @@ namespace FdoSecrets
         // add some informative and readonly attributes
         attrs[ItemAttributes::UuidKey] = m_backend->uuidToHex();
         attrs[ItemAttributes::PathKey] = path();
-        if (m_backend->hasTotp()) {
+        if (m_backend->hasValidTotp()) {
             attrs[ItemAttributes::TotpKey] = m_backend->totp();
         }
         return {};

src/gui/DatabaseWidget.cpp

@@ -1527,7 +1527,7 @@ void DatabaseWidget::entryActivationSignalReceived(Entry* entry, EntryModel::Mod
         }
         break;
     case EntryModel::Totp:
-        if (entry->hasTotp()) {
+        if (entry->hasValidTotp()) {
             setClipboardTextAndMinimize(entry->totp());
         } else {
             setupTotp();
@@ -2386,7 +2386,7 @@ bool DatabaseWidget::currentEntryHasTotp()
     if (!currentEntry) {
         return false;
     }
-    return currentEntry->hasTotp();
+    return currentEntry->hasValidTotp();
 }
 
 #ifdef WITH_XC_SSHAGENT

src/gui/EntryPreviewWidget.cpp

@@ -70,8 +70,7 @@ EntryPreviewWidget::EntryPreviewWidget(QWidget* parent)
 
     m_ui->entryTotpLabel->installEventFilter(this);
 
-    connect(m_ui->entryTotpButton, SIGNAL(toggled(bool)), m_ui->entryTotpLabel, SLOT(setVisible(bool)));
+    connect(m_ui->entryTotpButton, SIGNAL(toggled(bool)), m_ui->entryTotp, SLOT(setVisible(bool)));
-    connect(m_ui->entryTotpButton, SIGNAL(toggled(bool)), m_ui->entryTotpProgress, SLOT(setVisible(bool)));
     connect(m_ui->entryCloseButton, SIGNAL(clicked()), SLOT(hide()));
     connect(m_ui->toggleUsernameButton, SIGNAL(clicked(bool)), SLOT(setUsernameVisible(bool)));
     connect(m_ui->togglePasswordButton, SIGNAL(clicked(bool)), SLOT(setPasswordVisible(bool)));
@@ -260,8 +259,7 @@ void EntryPreviewWidget::updateEntryTotp()
         m_ui->entryTotpProgress->setMaximum(m_currentEntry->totpSettings()->step);
         updateTotpLabel();
     } else {
-        m_ui->entryTotpLabel->hide();
+        m_ui->entryTotp->hide();
-        m_ui->entryTotpProgress->hide();
         m_ui->entryTotpButton->setChecked(false);
         m_ui->entryTotpLabel->clear();
         m_totpTimer.stop();
@@ -546,16 +544,23 @@ void EntryPreviewWidget::updateGroupSharingTab()
 void EntryPreviewWidget::updateTotpLabel()
 {
     if (!m_locked && m_currentEntry && m_currentEntry->hasTotp()) {
-        auto totpCode = m_currentEntry->totp();
+        bool isValid = false;
+        auto totpCode = m_currentEntry->totp(&isValid);
+        if (isValid) {
             totpCode.insert(totpCode.size() / 2, " ");
-        m_ui->entryTotpLabel->setText(totpCode);
 
             auto step = m_currentEntry->totpSettings()->step;
             auto timeleft = step - (Clock::currentSecondsSinceEpoch() % step);
             m_ui->entryTotpProgress->setValue(timeleft);
             m_ui->entryTotpProgress->update();
         } else {
-        m_ui->entryTotpLabel->clear();
+            m_totpTimer.stop();
+        }
+
+        m_ui->entryTotpProgress->setVisible(isValid);
+        m_ui->entryTotpLabel->setText(totpCode);
+    } else {
+        m_ui->entryTotp->setVisible(false);
         m_totpTimer.stop();
     }
 }

src/gui/EntryPreviewWidget.ui

@@ -110,16 +110,34 @@
           </layout>
          </item>
          <item>
-          <layout class="QVBoxLayout" name="verticalLayout">
+          <widget class="QWidget" name="entryTotp" native="true">
+           <property name="minimumSize">
+            <size>
+             <width>0</width>
+             <height>0</height>
+            </size>
+           </property>
+           <layout class="QVBoxLayout" name="verticalLayout_2">
             <property name="spacing">
              <number>0</number>
             </property>
+            <property name="leftMargin">
+             <number>0</number>
+            </property>
+            <property name="topMargin">
+             <number>0</number>
+            </property>
+            <property name="rightMargin">
+             <number>0</number>
+            </property>
+            <property name="bottomMargin">
+             <number>0</number>
+            </property>
             <item>
              <widget class="QLabel" name="entryTotpLabel">
               <property name="font">
                <font>
                 <pointsize>10</pointsize>
-               <weight>75</weight>
                 <bold>true</bold>
                </font>
               </property>
@@ -130,7 +148,7 @@
                <string notr="true">1234567</string>
               </property>
               <property name="textInteractionFlags">
-              <set>Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse</set>
+               <set>Qt::TextInteractionFlag::LinksAccessibleByMouse|Qt::TextInteractionFlag::TextSelectableByKeyboard|Qt::TextInteractionFlag::TextSelectableByMouse</set>
               </property>
              </widget>
             </item>
@@ -151,6 +169,7 @@
              </widget>
             </item>
            </layout>
+          </widget>
          </item>
          <item>
           <widget class="QToolButton" name="entryTotpButton">

src/gui/TotpDialog.cpp

@@ -39,6 +39,7 @@ TotpDialog::TotpDialog(QWidget* parent, Entry* entry)
     m_step = m_entry->totpSettings()->step;
     resetCounter();
     updateProgressBar();
+    updateSeconds();
 
     connect(&m_totpUpdateTimer, SIGNAL(timeout()), this, SLOT(updateProgressBar()));
     connect(&m_totpUpdateTimer, SIGNAL(timeout()), this, SLOT(updateSeconds()));
@@ -88,10 +89,15 @@ void TotpDialog::updateSeconds()
 
 void TotpDialog::updateTotp()
 {
-    QString totpCode = m_entry->totp();
+    bool isValid = false;
-    QString firstHalf = totpCode.left(totpCode.size() / 2);
+    QString totpCode = m_entry->totp(&isValid);
-    QString secondHalf = totpCode.mid(totpCode.size() / 2);
+    if (isValid) {
-    m_ui->totpLabel->setText(firstHalf + " " + secondHalf);
+        totpCode.insert(totpCode.size() / 2, " ");
+    }
+    m_ui->buttonBox->button(QDialogButtonBox::Ok)->setEnabled(isValid);
+    m_ui->progressBar->setVisible(isValid);
+    m_ui->timerLabel->setVisible(isValid);
+    m_ui->totpLabel->setText(totpCode);
 }
 
 void TotpDialog::resetCounter()

src/gui/TotpSetupDialog.cpp

@@ -127,5 +127,8 @@ void TotpSetupDialog::init()
                 m_ui->algorithmComboBox->setCurrentIndex(index);
             }
         }
+
+        auto error = Totp::checkValidSettings(settings);
+        m_ui->invalidKeyLabel->setVisible(!error.isEmpty());
     }
 }

src/gui/TotpSetupDialog.ui

@@ -14,6 +14,22 @@
    <string>Setup TOTP</string>
   </property>
   <layout class="QVBoxLayout" name="verticalLayout">
+   <item>
+    <widget class="QLabel" name="invalidKeyLabel">
+     <property name="font">
+      <font>
+       <weight>75</weight>
+       <bold>true</bold>
+      </font>
+     </property>
+     <property name="text">
+      <string>Error: secret key is invalid</string>
+     </property>
+     <property name="alignment">
+      <set>Qt::AlignCenter</set>
+     </property>
+    </widget>
+   </item>
    <item>
     <layout class="QHBoxLayout" name="horizontalLayout_2">
      <property name="leftMargin">
@@ -210,6 +226,7 @@
   <zorder>customSettingsGroup</zorder>
   <zorder>buttonBox</zorder>
   <zorder>groupBox</zorder>
+  <zorder>invalidKeyLabel</zorder>
  </widget>
  <tabstops>
   <tabstop>seedEdit</tabstop>

src/gui/entry/EntryModel.cpp

@@ -297,7 +297,7 @@ QVariant EntryModel::data(const QModelIndex& index, int role) const
             break;
         case Totp:
             if (entry->hasTotp()) {
-                return icons()->icon("totp");
+                return entry->hasValidTotp() ? icons()->icon("totp") : icons()->icon("totp-invalid");
             }
             break;
         case PasswordStrength:

tests/TestTotp.cpp

@@ -115,18 +115,18 @@ void TestTotp::testTotpCode()
 
     // Test 6 digit TOTP (default)
     quint64 time = 1234567890;
-    QCOMPARE(Totp::generateTotp(settings, time), QString("005924"));
+    QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("005924"));
 
     time = 1111111109;
-    QCOMPARE(Totp::generateTotp(settings, time), QString("081804"));
+    QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("081804"));
 
     // Test 8 digit TOTP (custom)
     settings->digits = 8;
     time = 1111111111;
-    QCOMPARE(Totp::generateTotp(settings, time), QString("14050471"));
+    QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("14050471"));
 
     time = 2000000000;
-    QCOMPARE(Totp::generateTotp(settings, time), QString("69279037"));
+    QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("69279037"));
 }
 
 void TestTotp::testSteamTotp()
@@ -155,9 +155,9 @@ void TestTotp::testSteamTotp()
     // Steam mobile app with a throw-away steam account. The above secret was extracted
     // from the Steam app's data for use in testing here.
     quint64 time = 1511200518;
-    QCOMPARE(Totp::generateTotp(settings, time), QString("FR8RV"));
+    QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("FR8RV"));
     time = 1511200714;
-    QCOMPARE(Totp::generateTotp(settings, time), QString("9P3VP"));
+    QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("9P3VP"));
 }
 
 void TestTotp::testEntryHistory()