keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
20.83k stars 1.44k forks source link

Clipboard clearing not working when using Gnome on Wayland unless the window is focussed #4498

Closed MartinJM closed 2 years ago

MartinJM commented 4 years ago

Expected Behavior

After the timeout for the clipboard clearing has expired, I expect the clipboard to be cleared - regardless of whether or not the KeepassXC window is focussed.

Current Behavior

If you don't have the KeepassXC window focussed when it attempts to clear the clipboard, it fails to do so. It does so without giving a warning, and focussing the window later doesn't clear the clipboard either. Having the window focussed when the timeout ends does clear the clipboard (as expected).

Possible Solution

I don't know if there's a fix available, as I don't know anything about Wayland really.

If there is no fix available, maybe it's possible to give the user a warning message (or notification) that he has to focus the window to clear the clipboard, and then do so when focus is given to the KeepassXC window?

Steps to Reproduce

  1. Install Fedora 31 (Workstation Edition)
  2. Login using the default GNOME, which uses Wayland
  3. Install KeepassXC using dnf (this will give you version 2.5.3)
  4. Open a password database
  5. Make sure the clear clipboard setting is enabled
  6. Copy a password
  7. Focus another window
  8. Wait for the clear clipboard timeout to pass
  9. Paste the password (this shouldn't be possible)

Context

Because clearing the clipboard fails silently, and there is no indicator to show if the timeout has passed yet, there is always a bit of uncertainty if the clipboard is cleared or not. This is not a feeling I like to have around a password manager.

When starting GNOME on X11, the clipboard is cleared properly, hence I think it's a problem with the clearing on Wayland.

keepassxc-cli clip does work as expected under Wayland, also without the terminal focussed.

Debug Info

KeePassXC - Version 2.5.3 Revision: f8c962b

Qt 5.13.2 Debugging mode is disabled.

Operating system: Fedora 31 (Workstation Edition) CPU architecture: x86_64 Kernel: linux 5.5.10-200.fc31.x86_64

Enabled extensions:

Cryptographic libraries: libgcrypt 1.8.5

phoerious commented 4 years ago

We are requesting the clipboard to be cleared. I know that there are sometimes problems with Gnome, but those are beyond our control, unfortunately.

MartinJM commented 4 years ago

Fair enough. It may be good to document this so users who use Gnome with Wayland know not to expect the clipboard to be cleared if the window isn't focussed.

Would it still be possible to get some functionality that clears the password when the window is focussed, or when a button is pressed? Or is manually putting something else on your clipboard (using Ctrl+c) just as effective?

droidmonkey commented 4 years ago

This was supposed to be fixed in #4165

This smells a lot like a Wayland bug or xwayland bug to be more exact.

MartinJM commented 4 years ago

Since it works on X11 I think it's indeed something in Wayland. I have looked at where I can report that, but I haven't found the proper place yet. Do you maybe have a better idea of what the proper place would be?

I also tested the older version that is in the repositories of Fedora (KeepassXC version 2.4.3), and it also fails to clear the clipboard after the timeout.

Maybe it has something to do with this comment: https://github.com/keepassxreboot/keepassxc/issues/3704#issuecomment-561112570 ? Is there any way I can test that?

I also tried running it with WAYLAND_DEBUG=1, where I can see that it tries to empty the clipboard, but after that I can still paste the password that should have been cleared. It doesn't report any errors or warnings. If a full dump would help I can provide that after making a new database.

Edit: I have made a full dump, using WAYLAND_DEBUG=1 keepassxc 2> keepassxc_debug_stderr.txt . The output can be found here: https://gist.github.com/MartinJM/f28155701186c769321b5f732f921d6f The copy seems to happen around line 1190, clearing around 1416.

Edit2:

The actual data transfer happens when the receiving client sends a wl_data_offer.receive request. This request takes a mime type and a file descriptor as arguments. This request will generate a wl_data_source.send event on the sending client with the same arguments, and the latter client is expected to write its data to the given file descriptor using the chosen mime type.

In the dump from my first edit, there are only 2 data_source.sends (both wl_data_source@30.send, line 1199, and line 1416). The first one makes sense to me, the second not so much. It seems to happen before the new data source has been created (which is the datasource I'd expect to be empty).

To me this sounds like it's indeed something that has to be fixed in Qt or xWayland. I'm still not sure where I'd have to report that, so I'm open for suggestions.

droidmonkey commented 4 years ago

It's definitely not a Qt bug since it only happens when the window does not have focus. Also the fact that you copy data to the clipboard in the first place. This leads be to be 100% sure it is on the [x] Wayland side.

phoerious commented 4 years ago

The CLI uses xclip, so it's not surprising that it works. Wayland has a lot of bugs like this due to its new confinement model. It's also the reason why we cannot implement Auto-Type for it unless somebody builds a stable and standardised API for assistive technologies. I know there are some onscreen keyboards for Wayland, but none of that is even remotely ready for productive use. Same goes for trivial things such as checking whether capslock is enabled or not.

pp3345 commented 4 years ago

I've found the same issue and thought it's a mutter bug as it works fine in Sway and reported it there: https://gitlab.gnome.org/GNOME/mutter/-/issues/1241

As it turns out, this does seem to be an issue on KPXC's side as it destroys the data source and creates a new one which is prohibited for unfocussed clients as it seems. The proper way, according to the comments in GitLab, would be to call wl_data_device.set_selection(nil).

MartinJM commented 4 years ago

Thanks for your message, I hadn't looked into it anymore. Since I did have some extra time today, I decided to test what you said, and changed the code as follows:

diff --git a/src/gui/Clipboard.cpp b/src/gui/Clipboard.cpp
index ae5d8290..39a52f46 100644
--- a/src/gui/Clipboard.cpp
+++ b/src/gui/Clipboard.cpp
@@ -94,7 +94,15 @@ void Clipboard::clearClipboard()     if (m_lastCopied == clipboard->text(QClipboard::Clipboard)
         || m_lastCopied == clipboard->text(QClipboard::Selection)) {
+        printf("Clearing clipboard\n");
+        clipboard->clear(QClipboard::Clipboard);
+        printf("2\n");
+        if (clipboard->supportsSelection()) {
+            clipboard->clear(QClipboard::Selection);
+        }
+        printf("3\n");
         setText("", false);
+        printf("4\n");
     }

When running with WAYLAND_DEBUG=1 shows setting the clipboard as follows:

[4062464.551]  -> wl_data_device_manager@10.create_data_source(new id wl_data_source@42)
[4062464.557]  -> wl_data_source@42.offer("text/plain")
[4062464.560]  -> wl_data_source@42.offer("x-kde-passwordManagerHint")
[4062464.562]  -> wl_data_source@42.offer("text/plain;charset=utf-8")
[4062464.566]  -> wl_data_device@13.set_selection(wl_data_source@42, 3679)

And clearing the clipboard as:

Clearing clipboard
[4072795.690]  -> wl_data_device@13.set_selection(nil, 3679)
[4072795.697]  -> wl_data_source@42.destroy()
2
3
[4072795.711]  -> wl_data_device_manager@10.create_data_source(new id wl_data_source@43)
[4072795.716]  -> wl_data_source@43.offer("text/plain")
[4072795.719]  -> wl_data_source@43.offer("x-kde-passwordManagerHint")
[4072795.722]  -> wl_data_source@43.offer("text/plain;charset=utf-8")
[4072795.727]  -> wl_data_device@13.set_selection(wl_data_source@43, 3679)
4

However, after this, the password can still be copied to other windows. So setting the data device selection to nil doesn't fix this issue when using Gnome (I also tried it without the setText("", false);, which didn't fix it either).

Edit: In the meantime I have upgraded to Fedora 32, and the data in this post was when running Mutter 3.36.1 and Qt 5.13.2.

bugaevc commented 4 years ago

Setting the selection to nil does work. Trying to do this while unfocused is what doesn't.

MartinJM commented 4 years ago

Yes, you're right, I've changed it to "doesn't fix this issue" instead of "doesn't work".

droidmonkey commented 4 years ago

This is most definitely not our bug to fix. This is either a problem in Wayland, a problem in Qt, or both.

malo44 commented 4 years ago

I have the same problem with Keepassxc 2.4.3 in Ubuntu 19.04. I dont run Wayland

echo $XDG_SESSION_TYPE = x11

droidmonkey commented 4 years ago

Different bug, fixed in more recent versions of KeePassXC.

renatobellotti commented 3 years ago

The bug still exists in Fedora 33:

After copying a password, the clipboard is never cleaned. The mono port of Keepass works...

phoerious commented 3 years ago

Is Mono even using Wayland at all?

renatobellotti commented 3 years ago

I don't know, but it runs on my system...

issueonkeepassxc commented 2 years ago

Using keepassXc on fedora 36 i am still esperimenting this issue. Any tips?

jakedane commented 1 year ago

To anybody running into the same issue, because it's still not fixed in mutter, I solved it by adding a call to wl-copy --clear when the clipboard should be cleared and rebuilt the package with that.

Here, in the Clipboard::clearCopiedText() method in the file src/gui/Clipboard.cpp:

void Clipboard::clearCopiedText()
{
…
    if (m_lastCopied == clipboard->text(QClipboard::Clipboard)
        || m_lastCopied == clipboard->text(QClipboard::Selection)) {
        setText("", false);
        /* force a clipboard clean on Wayland */
        QProcess::startDetached("/usr/bin/wl-copy", {"--clear"});
    }
…
}

And add #include <QProcess> at the start of the file. Now it clears the clipboard also when the window doesn't have focus.

janvojt commented 1 month ago

I still have this issue with Wayland and KeePassXC 2.7.9.

The wl-copy -c trick does the job with clearing the clipboard. However, the password does appear in copyq after clearing. I tried simulate clearing the password manually outside KeePass by running wl-copy -c. It clears the clipboard, but its last contents do not appear in copyq. Therefore, I am suspecting either clipboard->text() or the failing clipboard->clear() causes copyq to pick up clipboard contents, see below the current clearing logic:

void Clipboard::clearCopiedText()
{
    m_timer->stop();
    emit updateCountdown(-1, "");

    auto* clipboard = QApplication::clipboard();
    if (!clipboard) {
        qWarning("Unable to access the clipboard.");
        return;
    }

    if (m_lastCopied == clipboard->text(QClipboard::Clipboard)
        || m_lastCopied == clipboard->text(QClipboard::Selection)) {
        clipboard->clear(QClipboard::Clipboard);
        clipboard->clear(QClipboard::Selection);
#ifdef Q_OS_UNIX
        // Gnome Wayland doesn't let apps modify the clipboard when not in focus, so force clear
        if (QProcessEnvironment::systemEnvironment().contains("WAYLAND_DISPLAY")) {
            QProcess::startDetached("wl-copy", {"-c"});
        }
#endif
    }

    m_lastCopied.clear();
}

If I am correct, probably the solution for Wayland would be to not compare the current clipboard contents and just delete the clipboard. Obviously, this might be annoying if user already copied something else and it gets cleared, but I think it is still better solution than just exposing passwords in copyq. I mean something like the below:

void Clipboard::clearCopiedText()
{
    m_timer->stop();
    emit updateCountdown(-1, "");

#ifdef Q_OS_UNIX
    // Gnome Wayland doesn't let apps modify the clipboard when not in focus, so force clear
    if (QProcessEnvironment::systemEnvironment().contains("WAYLAND_DISPLAY")) {
        QProcess::startDetached("wl-copy", {"-c"});
        m_lastCopied.clear();
        return;
    }
#endif

    auto* clipboard = QApplication::clipboard();
    if (!clipboard) {
        qWarning("Unable to access the clipboard.");
        return;
    }

    if (m_lastCopied == clipboard->text(QClipboard::Clipboard)
        || m_lastCopied == clipboard->text(QClipboard::Selection)) {
        clipboard->clear(QClipboard::Clipboard);
        clipboard->clear(QClipboard::Selection);
    }

    m_lastCopied.clear();
}
droidmonkey commented 1 month ago

What is copyq? Seems to need a bug report about not respecting the password hint flag in some situations.

janvojt commented 1 month ago

copyq is clipboard manager. It is respecting the password hint. It ignores the password when copied from KeePass.

When password is copied, there are 3 formats in the clipboard:

$ wl-paste -l
text/plain;charset=utf-8
x-kde-passwordManagerHint
text/plain

The problem is, when KeePass clears the password after 10 seconds, copyq then stores the clipboard contents, because it only sees the text/plain format. My guess would be that while clearing the x-kde-passwordManagerHint format is cleared first, text/plain second, but copyq intercepts it in a moment where I have only plaintext format in the clipboard, therefore stores it in the clipboard history.

If I have time I will try to compile KeePass with some timeouts in between the clearing calls and test when exactly is the password stored.

droidmonkey commented 1 month ago

Seems like odd behavior for copyq and still indicates a bug. The current value shouldn't be stored even though the passwordhint flag might be temporarily vacated. It's not like the current value all of a sudden doesn't become a password. That only happens when a new value is applied without the flag.