Improve error message when browser proxy cannot be found (#9385)

Co-authored-by: Blessio <[email protected]> Co-authored-by: Jonathan White <[email protected]>
keepassxreboot · Aug 6, 2023 · 1b12c95 · 1b12c95
1 parent 29726e2
commit 1b12c95
Show file tree

Hide file tree

Showing 8 changed files with 120 additions and 53 deletions.
diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts
@@ -1106,14 +1106,6 @@ Do you want to delete the entry?
         <source>Please see special instructions for browser extension use below</source>
         <translation type="unfinished"></translation>
     </message>
-    <message>
-        <source>&lt;b&gt;Error:&lt;/b&gt; The custom proxy location cannot be found!&lt;br/&gt;Browser integration WILL NOT WORK without the proxy application.</source>
-        <translation type="unfinished"></translation>
-    </message>
-    <message>
-        <source>&lt;b&gt;Warning:&lt;/b&gt; The following options can be dangerous!</source>
-        <translation type="unfinished"></translation>
-    </message>
     <message>
         <source>Executable Files</source>
         <translation type="unfinished"></translation>
@@ -1138,6 +1130,22 @@ Do you want to delete the entry?
         <source>Allow limited access to all entries in connected databases (ignores site access restrictions)</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>&lt;b&gt;Warning:&lt;/b&gt; Only adjust these settings if necessary.</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>The custom proxy location does not exist.</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>&lt;b&gt;Error:&lt;/b&gt; The custom proxy location does not exist. Correct this in the advanced settings tab.</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>&lt;b&gt;Error:&lt;/b&gt; The installed proxy executable is missing from the expected location: %1&lt;br/&gt;Please set a custom proxy location in the advanced settings or reinstall the application.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>CloneDialog</name>

diff --git a/src/browser/BrowserSettings.cpp b/src/browser/BrowserSettings.cpp
@@ -200,6 +200,11 @@ QString BrowserSettings::proxyLocation()
     return m_nativeMessageInstaller.getProxyPath();
 }
 
+QString BrowserSettings::proxyLocationAsInstalled() const
+{
+    return m_nativeMessageInstaller.getInstalledProxyPath();
+}
+
 #ifdef QT_DEBUG
 QString BrowserSettings::customExtensionId()
 {

diff --git a/src/browser/BrowserSettings.h b/src/browser/BrowserSettings.h
@@ -57,6 +57,7 @@ class BrowserSettings
     QString customProxyLocation();
     void setCustomProxyLocation(const QString& location);
     QString proxyLocation();
+    QString proxyLocationAsInstalled() const;
 #ifdef QT_DEBUG
     QString customExtensionId();
     void setCustomExtensionId(const QString& id);

diff --git a/src/browser/BrowserSettingsWidget.cpp b/src/browser/BrowserSettingsWidget.cpp
@@ -20,6 +20,7 @@
 
 #include "BrowserSettings.h"
 #include "config-keepassx.h"
+#include "gui/styles/StateColorPalette.h"
 
 #include <QFileDialog>
 
@@ -49,12 +50,9 @@ BrowserSettingsWidget::BrowserSettingsWidget(QWidget* parent)
                  snapInstructions));
     // clang-format on
 
-    m_ui->warningWidget->setCloseButtonVisible(false);
-    m_ui->warningWidget->setAutoHideTimeout(-1);
-    m_ui->warningWidget->setAnimate(false);
-
     m_ui->tabWidget->setEnabled(m_ui->enableBrowserSupport->isChecked());
     connect(m_ui->enableBrowserSupport, SIGNAL(toggled(bool)), m_ui->tabWidget, SLOT(setEnabled(bool)));
+    connect(m_ui->enableBrowserSupport, SIGNAL(toggled(bool)), SLOT(validateProxyLocation()));
 
     // Custom Browser option
 #ifdef Q_OS_WIN
@@ -72,10 +70,15 @@ BrowserSettingsWidget::BrowserSettingsWidget(QWidget* parent)
 
     connect(m_ui->useCustomProxy, SIGNAL(toggled(bool)), m_ui->customProxyLocation, SLOT(setEnabled(bool)));
     connect(m_ui->useCustomProxy, SIGNAL(toggled(bool)), m_ui->customProxyLocationBrowseButton, SLOT(setEnabled(bool)));
-    connect(m_ui->useCustomProxy, SIGNAL(toggled(bool)), SLOT(validateCustomProxyLocation()));
-    connect(m_ui->customProxyLocation, SIGNAL(editingFinished()), SLOT(validateCustomProxyLocation()));
+    connect(m_ui->useCustomProxy, SIGNAL(toggled(bool)), SLOT(validateProxyLocation()));
+    connect(m_ui->customProxyLocation, SIGNAL(editingFinished()), SLOT(validateProxyLocation()));
     connect(m_ui->customProxyLocationBrowseButton, SIGNAL(clicked()), this, SLOT(showProxyLocationFileDialog()));
 
+    m_ui->messageWidget->setVisible(false);
+    m_ui->messageWidget->setCloseButtonVisible(false);
+    m_ui->messageWidget->setWordWrap(true);
+    m_ui->messageWidget->setAutoHideTimeout(MessageWidget::DisableAutoHide);
+
 #ifndef Q_OS_LINUX
     m_ui->snapWarningLabel->setVisible(false);
 #endif
@@ -90,7 +93,6 @@ BrowserSettingsWidget::BrowserSettingsWidget(QWidget* parent)
     m_ui->torBrowserSupport->setHidden(true);
     m_ui->firefoxSupport->setText("Firefox and Tor Browser");
 #endif
-    m_ui->browserGlobalWarningWidget->setVisible(false);
 
 #ifndef QT_DEBUG
     m_ui->customExtensionId->setVisible(false);
@@ -154,10 +156,8 @@ void BrowserSettingsWidget::loadSettings()
     m_ui->customBrowserSupport->setVisible(false);
     m_ui->customBrowserGroupBox->setVisible(false);
     // Show notice to user
-    m_ui->browserGlobalWarningWidget->showMessage(tr("Please see special instructions for browser extension use below"),
-                                                  MessageWidget::Warning);
-    m_ui->browserGlobalWarningWidget->setCloseButtonVisible(false);
-    m_ui->browserGlobalWarningWidget->setAutoHideTimeout(-1);
+    m_ui->messageWidget->showMessage(tr("Please see special instructions for browser extension use below"),
+                                     MessageWidget::Warning);
 #endif
 #ifdef KEEPASSXC_DIST_FLATPAK
     // The sandbox makes custom proxy locations very unintuitive
@@ -184,21 +184,50 @@ void BrowserSettingsWidget::loadSettings()
 #ifdef QT_DEBUG
     m_ui->customExtensionId->setText(settings->customExtensionId());
 #endif
-
-    validateCustomProxyLocation();
+    // Validate the complete proxy location dependency - not only in case it is custom,
+    // to make trouble-shooting for both developer and user easier
+    validateProxyLocation();
 }
 
-void BrowserSettingsWidget::validateCustomProxyLocation()
+QString BrowserSettingsWidget::resolveCustomProxyLocation()
 {
-    auto path = browserSettings()->customProxyLocation();
+    auto settings = browserSettings();
+    auto proxyLocation = m_ui->customProxyLocation->text();
+    proxyLocation = settings->replaceTildeHomePath(proxyLocation);
+    return proxyLocation;
+}
 
-    if (m_ui->useCustomProxy->isChecked() && !QFile::exists(path)) {
-        m_ui->warningWidget->showMessage(tr("<b>Error:</b> The custom proxy location cannot be found!"
-                                            "<br/>Browser integration WILL NOT WORK without the proxy application."),
-                                         MessageWidget::Error);
-    } else {
-        m_ui->warningWidget->showMessage(tr("<b>Warning:</b> The following options can be dangerous!"),
-                                         MessageWidget::Warning);
+void BrowserSettingsWidget::validateProxyLocation()
+{
+    // Reset the UI
+    m_ui->messageWidget->setVisible(false);
+    m_ui->customProxyLocation->setStyleSheet("");
+    m_ui->customProxyLocation->setToolTip("");
+
+    if (m_ui->enableBrowserSupport->isChecked()) {
+        // If we are using a custom proxy location, check if it exists and display warning if not
+        if (m_ui->useCustomProxy->isChecked()) {
+            if (!QFile::exists(resolveCustomProxyLocation())) {
+                StateColorPalette statePalette;
+                auto color = statePalette.color(StateColorPalette::ColorRole::Error);
+                m_ui->customProxyLocation->setStyleSheet(QString("QLineEdit { background: %1; }").arg(color.name()));
+                m_ui->customProxyLocation->setToolTip(tr("The custom proxy location does not exist."));
+
+                m_ui->messageWidget->showMessage(tr("<b>Error:</b> The custom proxy location does not exist. Correct "
+                                                    "this in the advanced settings tab."),
+                                                 MessageWidget::Error);
+            }
+        } else {
+            // Otherwise check if the installed proxy exists
+            auto expectedProxyLocation = browserSettings()->proxyLocationAsInstalled();
+            if (!QFile::exists(expectedProxyLocation)) {
+                m_ui->messageWidget->showMessage(
+                    tr("<b>Error:</b> The installed proxy executable is missing from the expected location: %1<br/>"
+                       "Please set a custom proxy location in the advanced settings or reinstall the application.")
+                        .arg(expectedProxyLocation),
+                    MessageWidget::Error);
+            }
+        }
     }
 }
 
@@ -212,7 +241,7 @@ void BrowserSettingsWidget::saveSettings()
     settings->setMatchUrlScheme(m_ui->matchUrlScheme->isChecked());
 
     settings->setUseCustomProxy(m_ui->useCustomProxy->isChecked());
-    settings->setCustomProxyLocation(browserSettings()->replaceTildeHomePath(m_ui->customProxyLocation->text()));
+    settings->setCustomProxyLocation(resolveCustomProxyLocation());
 
     settings->setUpdateBinaryPath(m_ui->updateBinaryPath->isChecked());
     settings->setAllowGetDatabaseEntriesRequest(m_ui->allowGetDatabaseEntriesRequest->isChecked());
@@ -254,14 +283,25 @@ void BrowserSettingsWidget::showProxyLocationFileDialog()
 #else
     QString fileTypeFilter(QString("%1 (*)").arg(tr("Executable Files")));
 #endif
-    auto proxyLocation = QFileDialog::getOpenFileName(this,
-                                                      tr("Select custom proxy location"),
-                                                      QFileInfo(QCoreApplication::applicationDirPath()).filePath(),
-                                                      fileTypeFilter);
-
-    proxyLocation = browserSettings()->replaceHomePath(proxyLocation);
-    m_ui->customProxyLocation->setText(proxyLocation);
-    validateCustomProxyLocation();
+
+    auto initialFilePath = resolveCustomProxyLocation();
+    if (QFileInfo::exists(initialFilePath)) {
+        initialFilePath = QFileInfo(initialFilePath).filePath();
+    } else {
+        // ignore current status and set as it would be installed
+        initialFilePath = QFileInfo(browserSettings()->proxyLocationAsInstalled()).filePath();
+    }
+
+    QString proxyLocation =
+        QFileDialog::getOpenFileName(this, tr("Select custom proxy location"), initialFilePath, fileTypeFilter);
+
+    if (!proxyLocation.isEmpty()) {
+        proxyLocation = browserSettings()->replaceHomePath(proxyLocation);
+        m_ui->customProxyLocation->setText(proxyLocation);
+        validateProxyLocation();
+    } else {
+        // do not overwrite old proxy setting
+    }
 }
 
 void BrowserSettingsWidget::showCustomBrowserLocationFileDialog()

diff --git a/src/browser/BrowserSettingsWidget.h b/src/browser/BrowserSettingsWidget.h
@@ -32,18 +32,20 @@ class BrowserSettingsWidget : public QWidget
 
 public:
     explicit BrowserSettingsWidget(QWidget* parent = nullptr);
-    ~BrowserSettingsWidget();
+    ~BrowserSettingsWidget() override;
 
 public slots:
     void loadSettings();
     void saveSettings();
 
 private slots:
     void showProxyLocationFileDialog();
-    void validateCustomProxyLocation();
+    void validateProxyLocation();
     void showCustomBrowserLocationFileDialog();
 
 private:
+    QString resolveCustomProxyLocation();
+
     QScopedPointer<Ui::BrowserSettingsWidget> m_ui;
 };
 

diff --git a/src/browser/BrowserSettingsWidget.ui b/src/browser/BrowserSettingsWidget.ui
@@ -27,7 +27,7 @@
     <number>0</number>
    </property>
    <item>
-    <widget class="MessageWidget" name="browserGlobalWarningWidget" native="true"/>
+    <widget class="MessageWidget" name="messageWidget" native="true"/>
    </item>
    <item>
     <widget class="QCheckBox" name="enableBrowserSupport">
@@ -267,12 +267,15 @@
       </attribute>
       <layout class="QVBoxLayout" name="verticalLayout_6">
        <item>
-        <widget class="MessageWidget" name="warningWidget" native="true">
-         <property name="sizePolicy">
-          <sizepolicy hsizetype="Preferred" vsizetype="Preferred">
-           <horstretch>0</horstretch>
-           <verstretch>0</verstretch>
-          </sizepolicy>
+        <widget class="MessageWidget" name="advancedMessageWidget" native="true">
+         <property name="text" stdset="0">
+          <string>&lt;b&gt;Warning:&lt;/b&gt; Only adjust these settings if necessary.</string>
+         </property>
+         <property name="closeButtonVisible" stdset="0">
+          <bool>false</bool>
+         </property>
+         <property name="messageType" stdset="0">
+          <number>2</number>
          </property>
         </widget>
        </item>

diff --git a/src/browser/NativeMessageInstaller.cpp b/src/browser/NativeMessageInstaller.cpp
@@ -272,17 +272,24 @@ QString constructFlatpakPath()
 #endif
 
 /**
- * Gets the path to keepassxc-proxy binary
- *
- * @param location Custom proxy path
- * @return path Path to keepassxc-proxy
+ * Returns the effective proxy path used to build the native messaging JSON script
  */
 QString NativeMessageInstaller::getProxyPath() const
 {
+    QString result;
     if (browserSettings()->useCustomProxy()) {
-        return browserSettings()->customProxyLocation();
+        result = browserSettings()->customProxyLocation();
+    } else {
+        result = getInstalledProxyPath();
     }
+    return result;
+}
 
+/**
+ * Returns the original proxy path at the time of installation
+ */
+QString NativeMessageInstaller::getInstalledProxyPath() const
+{
     QString path;
 #if defined(KEEPASSXC_DIST_APPIMAGE)
     path = QProcessEnvironment::systemEnvironment().value("APPIMAGE");

diff --git a/src/browser/NativeMessageInstaller.h b/src/browser/NativeMessageInstaller.h
@@ -33,6 +33,7 @@ class NativeMessageInstaller
     bool isBrowserEnabled(BrowserShared::SupportedBrowsers browser);
 
     QString getProxyPath() const;
+    QString getInstalledProxyPath() const;
     void updateBinaryPaths();
 
 private: