Fix handling of small passphrase wordlists

* Fixes #11856
* Set the minimum recommended wordlist size to 1,296 - equal to the EFF Short List
* Issue a clear warning when using a smaller wordlist but do not prevent generation of passphrases
* Improve wording when removing custom wordlist

share/translations/keepassxc_en.ts

@@ -7117,14 +7117,6 @@ The following data is missing:
         <comment>Password quality</comment>
         <translation type="unfinished"></translation>
     </message>
-    <message>
-        <source>Confirm Delete Wordlist</source>
-        <translation type="unfinished"></translation>
-    </message>
-    <message>
-        <source>Do you really want to delete the wordlist &quot;%1&quot;?</source>
-        <translation type="unfinished"></translation>
-    </message>
     <message>
         <source>Failed to delete wordlist</source>
         <translation type="unfinished"></translation>
@@ -7178,6 +7170,18 @@ Do you want to overwrite it?</source>
         <source>Excluded characters: &quot;0&quot;, &quot;1&quot;, &quot;l&quot;, &quot;I&quot;, &quot;O&quot;, &quot;|&quot;, &quot;﹒&quot;, &quot;B&quot;, &quot;8&quot;, &quot;G&quot;, &quot;6&quot;</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Warning: the chosen wordlist is smaller than the minimum recommended size!</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Confirm Remove Wordlist</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Do you really want to remove the wordlist &quot;%1&quot;?</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>PasswordWidget</name>
@@ -9192,10 +9196,6 @@ This option is deprecated, use --set-key-file instead.</source>
         <source>Shortcut %1 conflicts with &apos;%2&apos;. Overwrite shortcut?</source>
         <translation type="unfinished"></translation>
     </message>
-    <message>
-        <source>Cannot generate valid passphrases because the wordlist is too short</source>
-        <translation type="unfinished"></translation>
-    </message>
     <message>
         <source>Encrypted files are not supported.</source>
         <translation type="unfinished"></translation>
@@ -9243,6 +9243,10 @@ This option is deprecated, use --set-key-file instead.</source>
         <source>Tags</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Warning: the chosen wordlist is smaller than the minimum recommended size!</source>
+        <translation type="unfinished"></translation>
+    </message>
     <message>
         <source>Fit</source>
         <translation type="unfinished"></translation>

src/cli/Diceware.cpp

@@ -68,11 +68,9 @@ int Diceware::execute(const QStringList& arguments)
         dicewareGenerator.setWordList(wordListFile);
     }
 
-    if (!dicewareGenerator.isValid()) {
+    // Show a warning if the wordlist is smaller than the recommended size
-        // We already validated the word count input so if the generator is invalid, it
+    if (!dicewareGenerator.isWordListValid()) {
-        // must be because the word list is too small.
+        err << QObject::tr("Warning: the chosen wordlist is smaller than the minimum recommended size!") << Qt::endl;
-        err << QObject::tr("Cannot generate valid passphrases because the wordlist is too short") << Qt::endl;
-        return EXIT_FAILURE;
     }
 
     QString password = dicewareGenerator.generatePassphrase();

src/core/PassphraseGenerator.cpp

@@ -99,7 +99,7 @@ void PassphraseGenerator::setWordList(const QString& path)
 
     m_wordlist = wordset.toList();
 
-    if (m_wordlist.size() < m_minimum_wordlist_length) {
+    if (!isWordListValid()) {
         qWarning("Wordlist is less than minimum acceptable size: %s", qPrintable(path));
     }
 }
@@ -117,8 +117,7 @@ void PassphraseGenerator::setWordSeparator(const QString& separator)
 
 QString PassphraseGenerator::generatePassphrase() const
 {
-    // In case there was an error loading the wordlist
+    if (m_wordlist.isEmpty()) {
-    if (!isValid() || m_wordlist.empty()) {
         return {};
     }
 
@@ -149,7 +148,7 @@ QString PassphraseGenerator::generatePassphrase() const
     return words.join(m_separator);
 }
 
-bool PassphraseGenerator::isValid() const
+bool PassphraseGenerator::isWordListValid() const
 {
-    return m_wordCount > 0 && m_wordlist.size() >= m_minimum_wordlist_length;
+    return m_wordlist.size() >= m_minWordListSize;
 }

src/core/PassphraseGenerator.h

@@ -40,7 +40,7 @@ public:
     void setWordCase(PassphraseWordCase wordCase);
     void setDefaultWordList();
     void setWordSeparator(const QString& separator);
-    bool isValid() const;
+    bool isWordListValid() const;
 
     QString generatePassphrase() const;
 
@@ -50,7 +50,7 @@ public:
 
 private:
     int m_wordCount;
-    int m_minimum_wordlist_length = 4000;
+    int m_minWordListSize = 1296;
     PassphraseWordCase m_wordCase;
     QString m_separator;
     QList<QString> m_wordlist;

src/gui/PasswordGeneratorWidget.cpp

@@ -65,7 +65,7 @@ PasswordGeneratorWidget::PasswordGeneratorWidget(QWidget* parent)
     connect(m_ui->buttonApply, SIGNAL(clicked()), SLOT(applyPassword()));
     connect(m_ui->buttonCopy, SIGNAL(clicked()), SLOT(copyPassword()));
     connect(m_ui->buttonGenerate, SIGNAL(clicked()), SLOT(regeneratePassword()));
-    connect(m_ui->buttonDeleteWordList, SIGNAL(clicked()), SLOT(deleteWordList()));
+    connect(m_ui->buttonDeleteWordList, SIGNAL(clicked()), SLOT(removeCustomWordList()));
     connect(m_ui->buttonAddWordList, SIGNAL(clicked()), SLOT(addWordList()));
     connect(m_ui->buttonClose, SIGNAL(clicked()), SIGNAL(closed()));
 
@@ -115,6 +115,11 @@ PasswordGeneratorWidget::PasswordGeneratorWidget(QWidget* parent)
         m_ui->comboBoxWordList->addItem(fileName, path.absolutePath() + QDir::separator() + fileName);
     }
 
+    // Set color of wordlist warning
+    StateColorPalette statePalette;
+    auto color = statePalette.color(StateColorPalette::ColorRole::False);
+    m_ui->labelWordListWarning->setStyleSheet(QString("QLabel { color: %1; }").arg(color.name()));
+
     loadSettings();
 }
 
@@ -257,11 +262,9 @@ void PasswordGeneratorWidget::regeneratePassword()
             m_ui->editNewPassword->setText(m_passwordGenerator->generatePassword());
         }
     } else {
-        if (m_dicewareGenerator->isValid()) {
         m_ui->editNewPassword->setText(m_dicewareGenerator->generatePassphrase());
     }
 }
-}
 
 void PasswordGeneratorWidget::updateButtonsEnabled(const QString& password)
 {
@@ -379,34 +382,29 @@ bool PasswordGeneratorWidget::isPasswordGenerated() const
     return m_passwordGenerated;
 }
 
-void PasswordGeneratorWidget::deleteWordList()
+void PasswordGeneratorWidget::removeCustomWordList()
 {
     if (m_ui->comboBoxWordList->currentIndex() < m_firstCustomWordlistIndex) {
         return;
     }
 
-    QFile file(m_ui->comboBoxWordList->currentData().toString());
+    auto wordlist = m_ui->comboBoxWordList->currentText();
-    if (!file.exists()) {
-        return;
-    }
-
     auto result = MessageBox::question(this,
-                                       tr("Confirm Delete Wordlist"),
+                                       tr("Confirm Remove Wordlist"),
-                                       tr("Do you really want to delete the wordlist \"%1\"?").arg(file.fileName()),
+                                       tr("Do you really want to remove the wordlist \"%1\"?").arg(wordlist),
-                                       MessageBox::Delete | MessageBox::Cancel,
+                                       MessageBox::Remove | MessageBox::Cancel,
                                        MessageBox::Cancel);
-    if (result != MessageBox::Delete) {
-        return;
-    }
 
-    if (!file.remove()) {
+    if (result == MessageBox::Remove) {
+        QFile file(m_ui->comboBoxWordList->currentData().toString());
+        if (file.exists() && !file.remove()) {
             MessageBox::critical(this, tr("Failed to delete wordlist"), file.errorString());
-        return;
         }
 
         m_ui->comboBoxWordList->removeItem(m_ui->comboBoxWordList->currentIndex());
         updateGenerator();
     }
+}
 
 void PasswordGeneratorWidget::addWordList()
 {
@@ -589,11 +587,7 @@ void PasswordGeneratorWidget::updateGenerator()
         }
         m_passwordGenerator->setFlags(flags);
 
-        if (m_passwordGenerator->isValid()) {
+        m_ui->buttonGenerate->setEnabled(m_passwordGenerator->isValid());
-            m_ui->buttonGenerate->setEnabled(true);
-        } else {
-            m_ui->buttonGenerate->setEnabled(false);
-        }
     } else {
         m_dicewareGenerator->setWordCase(
             static_cast<PassphraseGenerator::PassphraseWordCase>(m_ui->wordCaseComboBox->currentData().toInt()));
@@ -610,11 +604,8 @@ void PasswordGeneratorWidget::updateGenerator()
 
         m_dicewareGenerator->setWordSeparator(m_ui->editWordSeparator->text());
 
-        if (m_dicewareGenerator->isValid()) {
+        m_ui->labelWordListWarning->setVisible(!m_dicewareGenerator->isWordListValid());
         m_ui->buttonGenerate->setEnabled(true);
-        } else {
-            m_ui->buttonGenerate->setEnabled(false);
-        }
     }
 
     regeneratePassword();

src/gui/PasswordGeneratorWidget.h

@@ -67,7 +67,7 @@ public slots:
     void applyPassword();
     void copyPassword();
     void setPasswordVisible(bool visible);
-    void deleteWordList();
+    void removeCustomWordList();
     void addWordList();
 
 protected:

src/gui/PasswordGeneratorWidget.ui

@@ -769,7 +769,113 @@ QProgressBar::chunk {
       <layout class="QGridLayout" name="gridLayout_2">
        <item row="0" column="0">
         <layout class="QGridLayout" name="gridLayout_3">
+         <item row="1" column="1" alignment="Qt::AlignRight">
+          <widget class="QLabel" name="labelWordList">
+           <property name="sizePolicy">
+            <sizepolicy hsizetype="Minimum" vsizetype="Preferred">
+             <horstretch>0</horstretch>
+             <verstretch>0</verstretch>
+            </sizepolicy>
+           </property>
+           <property name="text">
+            <string>Wordlist:</string>
+           </property>
+          </widget>
+         </item>
+         <item row="3" column="2">
+          <layout class="QHBoxLayout" name="horizontalLayout_9">
+           <item>
+            <widget class="QLineEdit" name="editWordSeparator">
+             <property name="sizePolicy">
+              <sizepolicy hsizetype="Minimum" vsizetype="Fixed">
+               <horstretch>0</horstretch>
+               <verstretch>0</verstretch>
+              </sizepolicy>
+             </property>
+             <property name="minimumSize">
+              <size>
+               <width>20</width>
+               <height>0</height>
+              </size>
+             </property>
+             <property name="text">
+              <string/>
+             </property>
+            </widget>
+           </item>
+           <item>
+            <spacer name="horizontalSpacer_7">
+             <property name="orientation">
+              <enum>Qt::Horizontal</enum>
+             </property>
+             <property name="sizeHint" stdset="0">
+              <size>
+               <width>40</width>
+               <height>20</height>
+              </size>
+             </property>
+            </spacer>
+           </item>
+          </layout>
+         </item>
+         <item row="2" column="1" alignment="Qt::AlignRight">
+          <widget class="QLabel" name="labelWordCount">
+           <property name="text">
+            <string>Word Count:</string>
+           </property>
+           <property name="buddy">
+            <cstring>spinBoxLength</cstring>
+           </property>
+          </widget>
+         </item>
+         <item row="4" column="1" alignment="Qt::AlignRight">
+          <widget class="QLabel" name="wordCaseLabel">
+           <property name="text">
+            <string>Word Case:</string>
+           </property>
+          </widget>
+         </item>
          <item row="1" column="2">
+          <layout class="QHBoxLayout" name="horizontalLayout_10">
+           <item>
+            <widget class="QComboBox" name="comboBoxWordList">
+             <property name="sizePolicy">
+              <sizepolicy hsizetype="Expanding" vsizetype="Fixed">
+               <horstretch>0</horstretch>
+               <verstretch>0</verstretch>
+              </sizepolicy>
+             </property>
+            </widget>
+           </item>
+           <item>
+            <widget class="QPushButton" name="buttonDeleteWordList">
+             <property name="focusPolicy">
+              <enum>Qt::TabFocus</enum>
+             </property>
+             <property name="toolTip">
+              <string>Delete selected wordlist</string>
+             </property>
+             <property name="accessibleDescription">
+              <string>Delete selected wordlist</string>
+             </property>
+            </widget>
+           </item>
+           <item>
+            <widget class="QPushButton" name="buttonAddWordList">
+             <property name="focusPolicy">
+              <enum>Qt::TabFocus</enum>
+             </property>
+             <property name="toolTip">
+              <string>Add custom wordlist</string>
+             </property>
+             <property name="accessibleDescription">
+              <string>Add custom wordlist</string>
+             </property>
+            </widget>
+           </item>
+          </layout>
+         </item>
+         <item row="2" column="2">
           <layout class="QHBoxLayout" name="horizontalLayout_3">
            <property name="sizeConstraint">
             <enum>QLayout::SetMinimumSize</enum>
@@ -814,61 +920,7 @@ QProgressBar::chunk {
            </item>
           </layout>
          </item>
-         <item row="3" column="1" alignment="Qt::AlignRight">
+         <item row="4" column="2">
-          <widget class="QLabel" name="wordCaseLabel">
-           <property name="text">
-            <string>Word Case:</string>
-           </property>
-          </widget>
-         </item>
-         <item row="2" column="1" alignment="Qt::AlignRight">
-          <widget class="QLabel" name="labelWordSeparator">
-           <property name="text">
-            <string>Word Separator:</string>
-           </property>
-          </widget>
-         </item>
-         <item row="0" column="2">
-          <layout class="QHBoxLayout" name="horizontalLayout_10">
-           <item>
-            <widget class="QComboBox" name="comboBoxWordList">
-             <property name="sizePolicy">
-              <sizepolicy hsizetype="Expanding" vsizetype="Fixed">
-               <horstretch>0</horstretch>
-               <verstretch>0</verstretch>
-              </sizepolicy>
-             </property>
-            </widget>
-           </item>
-           <item>
-            <widget class="QPushButton" name="buttonDeleteWordList">
-             <property name="focusPolicy">
-              <enum>Qt::TabFocus</enum>
-             </property>
-             <property name="toolTip">
-              <string>Delete selected wordlist</string>
-             </property>
-             <property name="accessibleDescription">
-              <string>Delete selected wordlist</string>
-             </property>
-            </widget>
-           </item>
-           <item>
-            <widget class="QPushButton" name="buttonAddWordList">
-             <property name="focusPolicy">
-              <enum>Qt::TabFocus</enum>
-             </property>
-             <property name="toolTip">
-              <string>Add custom wordlist</string>
-             </property>
-             <property name="accessibleDescription">
-              <string>Add custom wordlist</string>
-             </property>
-            </widget>
-           </item>
-          </layout>
-         </item>
-         <item row="3" column="2">
           <layout class="QHBoxLayout" name="horizontalLayout_6">
            <item>
             <widget class="QComboBox" name="wordCaseComboBox"/>
@@ -888,62 +940,23 @@ QProgressBar::chunk {
            </item>
           </layout>
          </item>
-         <item row="1" column="1" alignment="Qt::AlignRight">
+         <item row="3" column="1" alignment="Qt::AlignRight">
-          <widget class="QLabel" name="labelWordCount">
+          <widget class="QLabel" name="labelWordSeparator">
            <property name="text">
-            <string>Word Count:</string>
+            <string>Word Separator:</string>
-           </property>
-           <property name="buddy">
-            <cstring>spinBoxLength</cstring>
            </property>
           </widget>
          </item>
-         <item row="2" column="2">
+         <item row="0" column="2">
-          <layout class="QHBoxLayout" name="horizontalLayout_9">
+          <widget class="QLabel" name="labelWordListWarning">
-           <item>
+           <property name="font">
-            <widget class="QLineEdit" name="editWordSeparator">
+            <font>
-             <property name="sizePolicy">
+             <weight>75</weight>
-              <sizepolicy hsizetype="Minimum" vsizetype="Fixed">
+             <bold>true</bold>
-               <horstretch>0</horstretch>
+            </font>
-               <verstretch>0</verstretch>
-              </sizepolicy>
-             </property>
-             <property name="minimumSize">
-              <size>
-               <width>20</width>
-               <height>0</height>
-              </size>
            </property>
            <property name="text">
-              <string/>
+            <string>Warning: the chosen wordlist is smaller than the minimum recommended size!</string>
-             </property>
-            </widget>
-           </item>
-           <item>
-            <spacer name="horizontalSpacer_7">
-             <property name="orientation">
-              <enum>Qt::Horizontal</enum>
-             </property>
-             <property name="sizeHint" stdset="0">
-              <size>
-               <width>40</width>
-               <height>20</height>
-              </size>
-             </property>
-            </spacer>
-           </item>
-          </layout>
-         </item>
-         <item row="0" column="1" alignment="Qt::AlignRight">
-          <widget class="QLabel" name="labelWordList">
-           <property name="sizePolicy">
-            <sizepolicy hsizetype="Minimum" vsizetype="Preferred">
-             <horstretch>0</horstretch>
-             <verstretch>0</verstretch>
-            </sizepolicy>
-           </property>
-           <property name="text">
-            <string>Wordlist:</string>
            </property>
           </widget>
          </item>

tests/TestCli.cpp

@@ -1089,8 +1089,9 @@ void TestCli::testDiceware()
     }
     smallWordFile.close();
 
+    // Ensure a warning is shown if the wordlist is too short
     execCmd(dicewareCmd, {"diceware", "-W", "11", "-w", smallWordFile.fileName()});
-    QCOMPARE(m_stderr->readLine(), QByteArray("Cannot generate valid passphrases because the wordlist is too short\n"));
+    QVERIFY(m_stderr->readLine().length() > 0);
 }
 
 void TestCli::testEdit()

tests/TestPassphraseGenerator.cpp

@@ -34,7 +34,7 @@ void TestPassphraseGenerator::testWordCase()
 {
     PassphraseGenerator generator;
     generator.setWordSeparator(" ");
-    QVERIFY(generator.isValid());
+    QVERIFY(generator.isWordListValid());
 
     QString passphrase;
     passphrase = generator.generatePassphrase();
@@ -57,14 +57,14 @@ void TestPassphraseGenerator::testWordCase()
 void TestPassphraseGenerator::testUniqueEntriesInWordlist()
 {
     PassphraseGenerator generator;
-    // set the limit down, so we don;t have to do a very large file
+    // set the limit down, so we don't have to do a very large file
-    generator.m_minimum_wordlist_length = 4;
+    generator.m_minWordListSize = 4;
 
     // link to bad wordlist
     QString path = QString(KEEPASSX_TEST_DATA_DIR).append("/wordlists/bad_wordlist_with_duplicate_entries.wordlist");
 
-    // setting will work, it creates the warning however, and isValid will fail
+    // setting will work, it creates the warning however, and isWordListValid will fail
     generator.setWordList(path);
     // so this fails
-    QVERIFY(!generator.isValid());
+    QVERIFY(!generator.isWordListValid());
 }