Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ask user before allowing kdbx files as key files #3625

Merged
merged 1 commit into from
Oct 20, 2019
Merged

Ask user before allowing kdbx files as key files #3625

merged 1 commit into from
Oct 20, 2019

Conversation

qw3ry
Copy link
Contributor

@qw3ry qw3ry commented Oct 15, 2019 •
edited by droidmonkey
Loading

Type of change

  • ✅ New feature (non-breaking change which adds functionality)

Description and Context

When adding a keyfile, the selected file is checked:

  • if it is a *.kdbx file, the user is warned that the file is not appropriate, but can ignore the warning. Maybe some users decided to give their keyfiles a kbdx extension and want to continue with this in the future.
  • if it is the current database file, an error is shown. The database would be corrupted if it was used.

Strictly speaking, the change could be considered to be a breaking change. The user cannot pick the database as its own keyfile anymore. But practically this would corrupt the database anyway, so no ones "workflow" is breaking

This change might need new translations (I'm not sure how they work with Qt. Tested with English local only).

In issue #3611 @phoerious and @droidmonkey kind of referenced this feature

Screenshots

Sorry for the missing window headers, my screenshot tool failed to grab them. They are (in both cases): "Database file as key file"
image

Testing strategy

Only manual test. This is a GUI change, there is not much to test here.

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@droidmonkey
Copy link
Member

This could be more generic in that any file chosen that is not a *.key file should provide a warning about how a key file cannot change or will invalidate your database access. Great start

@phoerious
Copy link
Member

I don't think we should enforce any specific file extension. People want to use their jpegs and stuff as key files.

@qw3ry
Copy link
Contributor Author

qw3ry commented Oct 15, 2019

@phoerious Thats what I thought as well. The file picker does have a *.key filter as pre-selection already, that should suffice (?)

@droidmonkey
Copy link
Member

My comment still stands, if there user actively chooses to go "off filter" they should be aware of the consequences of that choice. A prompt may not be entirely appropriate, but a warning banner in the master key tool could work well.

@qw3ry
Copy link
Contributor Author

qw3ry commented Oct 16, 2019

@droidmonkey not sure how to implement this in a reasonable way:

  • The key-edit dialog seems to be MessageBox-centered at first sight. Is there a MessageWidget anywhere near, that I can easily access?
  • The MainWindow does have displayGlobalMessage that targets a MessageWidget, but the method is not static, so I'd need to carry an object pointer through several levels of constructors - or "hope" that the parent pointer in QWidget is of correct type.
  • I could add another message widget inside the settings, but I'm not sure if thats a good idea either.

@droidmonkey
Copy link
Member

MessageWidget is overkill for this. A simple label with a note in it is perfectly fine. It could even be static just reminding users not to pick their database file or any file that is prone to changing. Prefer a freshly generated key file.

@droidmonkey droidmonkey changed the title Reject kbdx files as keyfiles Reject kdbx files as keyfiles Oct 18, 2019
@droidmonkey droidmonkey force-pushed the avoid-databases-as-keyfiles branch from c91a393 to e04abd3 Compare October 18, 2019 02:30
@droidmonkey
Copy link
Member

I made the necessary changes

@droidmonkey droidmonkey requested a review from phoerious October 18, 2019 02:30
@droidmonkey droidmonkey added the ux label Oct 18, 2019
@droidmonkey droidmonkey added this to the v2.5.0 milestone Oct 18, 2019
@qw3ry @droidmonkey
Add check for database files when selecting a key file
12ab351 
Reject own database file as the key file. Prompt for other kdbx files as key files.

Also add a static warning message to the key file selection dialog
@droidmonkey droidmonkey force-pushed the avoid-databases-as-keyfiles branch from e04abd3 to 12ab351 Compare October 18, 2019 21:07
@droidmonkey droidmonkey requested a review from phoerious October 18, 2019 21:08
@droidmonkey droidmonkey changed the title Reject kdbx files as keyfiles Ask user before allowing kdbx files as key files Oct 18, 2019
@droidmonkey droidmonkey merged commit a1e12c1 into keepassxreboot:develop Oct 20, 2019
@qw3ry qw3ry deleted the avoid-databases-as-keyfiles branch October 22, 2019 08:13
phoerious added a commit that referenced this pull request Oct 26, 2019
@phoerious
Release 2.5.0
1ab8a9f 
Added

- Add 'Paper Backup' aka 'Export to HTML file' to the 'Database' menu [[#3277](#3277)]
- Add statistics panel with information about the database (number of entries, number of unique passwords, etc.) to the Database Settings dialog [[#2034](#2034)]
- Add offline user manual accessible via the 'Help' menu [[#3274](#3274)]
- Add support for importing 1Password OpVault files [[#2292](#2292)]
- Implement Freedesktop.org secret storage DBus protocol so that KeePassXC can be used as a vault service by libsecret [[#2726](#2726)]
- Add support for OnlyKey as an alternative to YubiKeys (requires yubikey-personalization >= 1.20.0) [[#3352](#3352)]
- Add group sorting feature [[#3282](#3282)]
- Add feature to download favicons for all entries at once [[#3169](#3169)]
- Add word case option to passphrase generator [[#3172](#3172)]
- Add support for RFC6238-compliant TOTP hashes [[#2972](#2972)]
- Add UNIX man page for main program [[#3665](#3665)]
- Add 'Monospaced font' option to the notes field [[#3321](#3321)]
- Add support for key files in auto open [[#3504](#3504)]
- Add search field for filtering entries in Auto-Type dialog [[#2955](#2955)]
- Complete usernames based on known usernames from other entries [[#3300](#3300)]
- Parse hyperlinks in the notes field of the entry preview pane [[#3596](#3596)]
- Allow abbreviation of field names in entry search [[#3440](#3440)]
- Allow setting group icons recursively [[#3273](#3273)]
- Add copy context menu for username and password in Auto-Type dialog [[#3038](#3038)]
- Drop to background after copying a password to the clipboard [[#3253](#3253)]
- Add 'Lock databases' entry to tray icon menu [[#2896](#2896)]
- Add option to minimize window after unlocking [[#3439](#3439)]
- Add option to minimize window after opening a URL [[#3302](#3302)]
- Request accessibility permissions for Auto-Type on macOS [[#3624](#3624)]
- Browser: Add initial support for multiple URLs [[#3558](#3558)]
- Browser: Add entry-specific browser integration settings [[#3444](#3444)]
- CLI: Add offline HIBP checker (requires a downloaded HIBP dump) [[#2707](#2707)]
- CLI: Add 'flatten' option to the 'ls' command [[#3276](#3276)]
- CLI: Add password generation options to `Add` and `Edit` commands [[#3275](#3275)]
- CLI: Add XML import [[#3572](#3572)]
- CLI: Add CSV export to the 'export' command [[#3278](#3278)]
- CLI: Add `-y --yubikey` option for YubiKey [[#3416](#3416)]
- CLI: Add `--dry-run` option for merging databases [[#3254](#3254)]
- CLI: Add group commands (mv, mkdir and rmdir) [[#3313](#3313)].
- CLI: Add interactive shell mode command `open` [[#3224](#3224)]

Changed

- Redesign database unlock dialog [ [#3287](#3287)]
- Rework the entry preview panel [ [#3306](#3306)]
- Move notes to General tab on Group Preview Panel [[#3336](#3336)]
- Enable entry actions when editing an entry and cleanup entry context menu  [[#3641](#3641)]
- Improve detection of external database changes  [[#2389](#2389)]
- Warn if user is trying to use a KDBX file as a key file [[#3625](#3625)]
- Add option to disable KeePassHTTP settings migrations prompt [[#3349](#3349), [#3344](#3344)]
- Re-enabled Wayland support (no Auto-Type yet) [[#3520](#3520), [#3341](#3341)]
- Add icon to 'Toggle Window' action in tray icon menu [[3244](#3244)]
- Merge custom data between databases only when necessary [[#3475](#3475)]
- Improve various file-handling related issues when picking files using the system's file dialog [[#3473](#3473)]
- Add 'New Entry' context menu when no entries are selected [[#3671](#3671)]
- Reduce default Argon2 settings from 128 MiB and one thread per CPU core to 64 MiB and two threads to account for lower-spec mobile hardware [ [#3672](#3672)]
- Browser: Remove unused 'Remember' checkbox for HTTP Basic Auth [[#3371](#3371)]
- Browser: Show database name when pairing with a new browser [[#3638](#3638)]
- Browser: Show URL in allow access dialog [[#3639](#3639)]
- CLI: The password length option `-l` for the CLI commands `Add` and `Edit` is now `-L` [[#3275](#3275)]
- CLI: The `-u` shorthand for the `--upper` password generation option has been renamed to `-U` [[#3275](#3275)]
- CLI: Rename command `extract` to `export`. [[#3277](#3277)]

Fixed

- Improve accessibility for assistive technologies [[#3409](#3409)]
- Correctly unlock all databases if `--pw-stdin` is provided [[#2916](#2916)]
- Fix password generator issues with special characters [[#3303](#3303)]
- Fix KeePassXC interrupting shutdown procedure [[#3666](#3666)]
- Fix password visibility toggle button state on unlock dialog [[#3312](#3312)]
- Fix potential data loss if database is reloaded while user is editing an entry [[#3656](#3656)]
- Fix hard-coded background color in search help popup [[#3001](#3001)]
- Fix font choice for password preview [[#3425](#3425)]
- Fix handling of read-only files when autosave is enabled [[#3408](#3408)]
- Handle symlinks correctly when atomic saves are disabled [[#3463](#3463)]
- Enable HighDPI icon scaling on Linux [[#3332](#3332)]
- Make Auto-Type on macOS more robust and remove old Carbon API calls [[#3634](#3634), [[#3347](#3347))]
- Hide Share tab if KeePassXC is compiled without KeeShare support and other minor KeeShare improvements [[#3654](#3654), [[#3291](#3291), [#3029](#3029), [#3031](#3031), [#3236](#3236)]
- Correctly bring window to the front when clicking tray icon on macOS [[#3576](#3576)]
- Correct application shortcut created by MSI Installer on Windows [[#3296](#3296)]
- Fix crash when removing custom data [[#3508](#3508)]
- Fix placeholder resolution in URLs [[#3281](#3281)]
- Fix various inconsistencies and platform-dependent compilation bugs [[#3664](#3664), [#3662](#3662), [#3660](#3660), [#3655](#3655), [#3649](#3649), [#3417](#3417), [#3357](#3357), [#3319](#3319), [#3318](#3318), [#3304](#3304)]
- Browser: Fix potential leaking of entries through the browser integration API if multiple databases are opened [[#3480](#3480)]
- Browser: Fix password entropy calculation [[#3107](#3107)]
- Browser: Fix Windows registry settings for portable installation [[#3603](#3603)]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droidmonkey droidmonkey droidmonkey approved these changes

@phoerious phoerious phoerious approved these changes

Assignees
No one assigned
Labels
ux
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@qw3ry @droidmonkey @phoerious