Fix CLI secret clipboard MIME hints#13384
Draft
Jesssullivan wants to merge 1 commit into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns keepassxc-cli clip with the GUI clipboard behavior by ensuring “secret” clipboard MIME hints are applied when copying sensitive data, improving integration with clipboard managers across platforms.
Changes:
- Introduces a shared
core/ClipboardMimehelper to create secretQMimeDataand expose expected secret formats for verification. - Switches GUI clipboard code and timed CLI clipboard copies to use the shared secret MIME setup (using Qt’s clipboard where appropriate).
- Extends unit tests to validate secret clipboard MIME formats, and adds a build dependency to ensure the CLI binary exists for
testcli.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/TestTools.h |
Adds a new unit test declaration for clipboard secret MIME metadata. |
tests/TestTools.cpp |
Implements testClipboardMimeData() to validate secret MIME formats/data per OS. |
tests/TestCli.cpp |
Verifies that keepassxc-cli clip produces clipboard data containing the expected secret formats. |
tests/CMakeLists.txt |
Ensures testcli depends on building keepassxc-cli. |
src/gui/Clipboard.h |
Removes macOS pasteboard converter ownership from the GUI clipboard class. |
src/gui/Clipboard.cpp |
Uses ClipboardMime::createSecretMimeData() for secret clipboard setup. |
src/core/ClipboardMime.h |
Adds new core API for creating secret clipboard MIME data and enumerating secret formats. |
src/core/ClipboardMime.cpp |
Implements platform-specific secret MIME formats (macOS/Unix/Windows) and macOS converter initialization. |
src/CMakeLists.txt |
Adds ClipboardMime.cpp to core sources and moves macOS pasteboard converter compilation into core on Apple. |
src/cli/keepassxc-cli.cpp |
Creates QGuiApplication for clip (when appropriate) so Qt clipboard APIs can be used. |
src/cli/CMakeLists.txt |
Links the CLI library against Qt6::Gui to support clipboard APIs. |
src/cli/Clip.cpp |
Uses Qt clipboard + secret MIME setup for timed copies; retains external helper behavior for unlimited copies on Unix. |
Comment on lines
360
to
362
| if(APPLE) | ||
| target_link_libraries(keepassxc_core Qt6::Gui) | ||
| target_link_libraries(keepassxc_gui "-framework Foundation -framework AppKit -framework Carbon -framework Security -framework LocalAuthentication -framework ScreenCaptureKit") |
Member
There was a problem hiding this comment.
This violates our desire to not use any GUI libraries within the core build (and by extension the CLI)
Comment on lines
+22
to
+53
| #ifdef Q_OS_MACOS | ||
| #include "gui/osutils/macutils/MacPasteboard.h" | ||
|
|
||
| #include <QPointer> | ||
| #endif | ||
|
|
||
| namespace | ||
| { | ||
| #if defined(Q_OS_MACOS) | ||
| QPointer<MacPasteboard> s_pasteboard(nullptr); | ||
|
|
||
| void initializeNativeConverters() | ||
| { | ||
| // This object lives for the whole program lifetime and we cannot delete it on exit, | ||
| // so ignore leak warnings. See https://bugreports.qt.io/browse/QTBUG-54832 | ||
| if (!s_pasteboard) { | ||
| s_pasteboard = new MacPasteboard(); | ||
| } | ||
| } | ||
| #else | ||
| void initializeNativeConverters() | ||
| { | ||
| } | ||
| #endif | ||
| } // namespace | ||
|
|
||
| namespace ClipboardMime | ||
| { | ||
| QMimeData* createSecretMimeData(const QString& text) | ||
| { | ||
| initializeNativeConverters(); | ||
|
|
Member
There was a problem hiding this comment.
This absolutely should not happen, agree
Comment on lines
+47
to
+67
| bool shouldUseGuiApplication(int argc, char** argv) | ||
| { | ||
| for (int i = 1; i < argc; ++i) { | ||
| const auto arg = QString::fromLocal8Bit(argv[i]); | ||
| if (arg == QStringLiteral("clip")) { | ||
| #if defined(Q_OS_UNIX) && !defined(Q_OS_MACOS) | ||
| const auto env = QProcessEnvironment::systemEnvironment(); | ||
| return env.contains(QStringLiteral("DISPLAY")) || env.contains(QStringLiteral("WAYLAND_DISPLAY")) | ||
| || env.contains(QStringLiteral("QT_QPA_PLATFORM")); | ||
| #else | ||
| return true; | ||
| #endif | ||
| } | ||
|
|
||
| if (!arg.startsWith(QLatin1Char('-'))) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
Member
|
In general I am going to reject this PR on the grounds it re-introduces GUI elements to the CLI (and confuses core/gui builds). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Taking a shot at #12700; going to do some more testing and thinking, as I am not totally sure this is the best approach but figured I'd share the draft.
I noticed I started on this a month or two back and had followed the issue, @idoa01's ping this morning reminded me.
Closes #12700
Notes about this:
text/x-kde-passwordManagerHintfrom Add proper text/x-kde-passwordManagerHint MIME #12696.timeout 0still uses the existing external helper so unlimited copies remain persistent after the CLI exits, which I think is the intended behavior.testautotypefailure found while validating with the full test suiteMake timed/default
keepassxc-cli clipcopies useQClipboard, so CLI copies carry the same Linux, macOS, and Windows secret clipboard hints as GUI copies. This is dove by sharing the GUI secret clipboard MIME setup through a core helper, which may or may not be smart.Tests I've run thus far:
xcrun clang-format -i src/core/ClipboardMime.cpp src/core/ClipboardMime.h src/gui/Clipboard.cpp src/gui/Clipboard.h src/cli/Clip.cpp src/cli/keepassxc-cli.cpp tests/TestCli.cpp tests/TestTools.cpp tests/TestTools.h/opt/homebrew/bin/cmake --build build --target testtools testcli keepassxc-cli/opt/homebrew/bin/ctest --test-dir build -R '^testtools$' --output-on-failure --timeout 120/opt/homebrew/bin/ctest --test-dir build -R '^testcli$' --output-on-failure --timeout 180printf 'a\n' | build/src/cli/keepassxc-cli clip tests/data/NewDatabase.kdbx '/Sample Entry' 0Also, these are passing as well.
testtools: passedtestcli: passed/Sample Entrypassword