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

Remove Carbon from Mac Auto-Type #3347

Merged
merged 4 commits into from
Jul 3, 2019
Merged

Remove Carbon from Mac Auto-Type #3347

merged 4 commits into from
Jul 3, 2019

Conversation

droidmonkey
Copy link
Member

Type of change

  • ✅ Refactor (significant modification to existing code)

Description and Context

Testing strategy

Tested on Mojave

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]
  • ✅ My change requires a change to the documentation, and I have updated it accordingly.
  • ✅ I have added tests to cover my changes.

@droidmonkey droidmonkey added this to the v2.5.0 milestone Jul 1, 2019
@droidmonkey droidmonkey force-pushed the refactor/remove-carbon branch from 4d667cf to 1fd6ace Compare July 1, 2019 13:38
phoerious
phoerious requested changes Jul 1, 2019
View reviewed changes
src/autotype/mac/AutoTypeMac.cpp Outdated Show resolved Hide resolved
src/autotype/mac/AutoTypeMac.cpp Outdated Show resolved Hide resolved
src/autotype/mac/AutoTypeMac.cpp Outdated Show resolved Hide resolved
src/autotype/mac/AutoTypeMac.cpp Outdated Show resolved Hide resolved
src/autotype/mac/AutoTypeMac.cpp Outdated Show resolved Hide resolved
src/autotype/mac/AutoTypeMac.cpp Outdated Show resolved Hide resolved
src/gui/macutils/AppKitImpl.mm Outdated Show resolved Hide resolved
src/gui/macutils/AppKitImpl.mm Outdated Show resolved Hide resolved
@droidmonkey
Copy link
Member Author

@phoerious nice review, you got me!

@droidmonkey
Copy link
Member Author

changes made

weslly
weslly approved these changes Jul 2, 2019
View reviewed changes
Copy link
Contributor

@weslly weslly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine for me, but some issues with the keyboard layout still persist (though they aren't in the scope of this PR)

@droidmonkey droidmonkey requested a review from phoerious July 3, 2019 18:14
@droidmonkey droidmonkey merged commit ce1f19c into develop Jul 3, 2019
@droidmonkey droidmonkey deleted the refactor/remove-carbon branch July 3, 2019 18:43
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)]
@georgesnow
Copy link

I get the impression based on the lack of response to this issue: #3393

The NSEvent replacement is staying and not reverting back to the Carbon API (which is not deprecated)
As I stated on the other issue KeePassXC is viewing all key presses when using the NSEvent API, which would appropriate for an app like TextExpander, but not for hotkeys.

https://developer.apple.com/documentation/appkit/nsevent?language=objc

"The NSEvent object contains pertinent information about each event, such as where the cursor was located or which character was typed...global event monitor that allows you to monitor events in other applications,"

@droidmonkey
Copy link
Member Author

I understand your frustrations but we made a change for a reason. No additional security requirements are necessary to support NSEvent versus Carbon. I can consider a reversion of this PR, but it won't happen soon.

@georgesnow
Copy link

@droidmonkey understood.

Accessibility is needed for Autotype to work in general, but the inherent difference is KPXC is registering to view all keystrokes under that security permission. The Carbon API doesn't require the to register to view all keystrokes. It is registering the hotkey with the OS. My point here is if the app doesn't need that level of access and there is alternative it seems best to use the least amount of access.

And comes down to two parts:
1-Security permissions <=this is the same regardless for NSEvent and Carbon to use GAutotype/Autotype
2-How the hotkey event is being handled <=this is a coding difference as stated above

https://www.macworld.com/article/3311982/the-difference-between-accessibility-and-full-disk-access.html

Apple requires explicit permission, because it’s just these kinds of features that can be leveraged and abused by simpler malware that doesn’t dig deeply into exploiting the system, but could, for instance, try to capture your keystrokes.

@parodymshifter
Copy link

On Mojave 10.14.6 the change to autotype hotkey handling is dropping the 1st character of passwords in Terminal windows. It seems to work correctly in a text editor window. The breakage of the function in Terminal windows makes the hotkey feature nearly useless for me.

@georgesnow
Copy link

This is Most likely because the Nsevent api doesn’t allow (and isn’t) interrupting or blocking the event from passing. it only observes it. Carbon API stop it’s from posting to other applications

@droidmonkey
Copy link
Member Author

Please open an issue about this

droidmonkey added a commit to droidmonkey/keepassxc that referenced this pull request Nov 4, 2019
@droidmonkey
Revert "Remove Carbon from Mac Auto-Type (keepassxreboot#3347)"
607e35b 
This reverts commit ce1f19c.
@droidmonkey droidmonkey mentioned this pull request Nov 5, 2019
Merged
droidmonkey added a commit that referenced this pull request Nov 9, 2019
@droidmonkey
Revert "Remove Carbon from Mac Auto-Type (#3347)"
4a19172 
This reverts commit ce1f19c.
phoerious pushed a commit that referenced this pull request Nov 9, 2019
@droidmonkey @phoerious
Revert "Remove Carbon from Mac Auto-Type (#3347)"
440331d 
This reverts commit ce1f19c.
@yan12125 yan12125 mentioned this pull request Nov 28, 2019
Merged
droidmonkey pushed a commit that referenced this pull request Dec 21, 2019
@droidmonkey
Partial fix for building on Mac OS X < 10.12
794cc1e 
This commit reverts #3357.

The previous PR is for the new symbol NSEventMaskKeyDown, which is
introduced in #3347. In #3794, #3347 is reverted, so the workaround
in #3357 is no longer needed. Furthermore, it causes build failures
on 10.11 (#3932) as the header file for type NSEventMask is removed
in #3794, too.

Note that this is not a complete fix. A complete patch can be found
at [1]. In MacPorts, the OS version for building a package is the same
as the OS that installs it, so #ifdef can be used to replace @available.
The latter language feature is not available until Xcode 9.

With the patch mentioned in the previous paragraph, KeePassXC 2.5.1
can be built on Mac OS X 10.9 or newer.

Ref: #2899

[1] https://github.com/macports/macports-ports/blob/de1bb703ad19c258ad27eb903cf510dc1930107c/security/KeePassXC/files/patch-old-mac.diff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rockihack rockihack rockihack left review comments

@varjolintu varjolintu varjolintu approved these changes

@phoerious phoerious phoerious approved these changes

@weslly weslly weslly approved these changes

Assignees
No one assigned
Labels
feature: Auto-Type platform: macOS pr: refactoring Pull request that refactors code
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.

Replace Carbon HotKey registration with NSEvent API
7 participants
@droidmonkey @georgesnow @parodymshifter @weslly @phoerious @rockihack @varjolintu