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.

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

Make fill form shortcuts optional #46

Merged
merged 6 commits into from
Nov 27, 2023

Conversation

stefanb2
Copy link
Contributor

@stefanb2 stefanb2 commented Nov 18, 2023

Fill form shortcuts are the only thing that depends on class Keypress, which on Linux is dependent on X11. When the program is running in a Wayland session, then those shortcuts do not work, i.e. they are useless, but still a X11 client connection is created.

This PR makes all code related to those short cuts optional and the user can disable them with a build configuration option. I.e. she can create an executable that will be a pure Wayland client in a Wayland session.

Fixes #32

@bkueng bkueng mentioned this pull request Nov 21, 2023
Copy link
Owner

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

I'm fine with this for now but in the longer run I want to either remove it completely, or implement a wayland-compatible solution.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
Provide an editor configuration that reflects the current conventions
encountered in the source files.
Wrap all code related to ShortcutAction::FillForm(PasswordOnly) and
class Keypress with

   #ifndef DISABLE_FILL_FORM_SHORTCUTS
   ...
   #endif

Fixes bkueng#32
@stefanb2
Copy link
Contributor Author

IMHO it should be removed, because after pondering a bit about it I feel it is a security risk to randomly try to select another window on the desktop and then insert secret information.

I still feel that ydotool would be a possible solution, provided it would be available as properly integrated desktop/systemd user service. But as I personally see it as a security risk I couldn't convince myself to spend the time on such an implementation.

User can add -DDISABLE_FILL_FORM_SHORTCUTS=1 to CMake configure command
line to disable fill form shortcuts in the build.

- set compile definition DISABLE_FILL_FORM_SHORTCUTS
- add message if shortcuts are enabled or not
- move add_subdirectory() calls towards the end of the file so that they
  can rely on find_package()-generated flags.
- wrap everything related to class Keypress implementation with

   if (NOT DISABLE_FILL_FORM_SHORTCUTS)
       ...
   endif()

- move X11 libraries linking to conditional branch under src/

Fixes bkueng#32
Fedora 40 will switch to KDE Plasma 6 and no longer offer a X11
session, i.e. Qt programs will definitely run on Wayland. Therefore we
should remove all X11 dependencies as the fill form shortcuts do not
work on Wayland anyway...

- factor out X11 packages to environment variables
- do not install X11 packages when flag is set
- add -DDISABLE_FILL_FORM_SHORTCUTS=1 to configure step when flag is set
@stefanb2 stefanb2 force-pushed the topic-make-x11-dependency-optional branch from b84596d to 81df8bb Compare November 22, 2023 10:27
@bkueng
Copy link
Owner

bkueng commented Nov 24, 2023

IMHO it should be removed, because after pondering a bit about it I feel it is a security risk to randomly try to select another window on the desktop and then insert secret information.

I agree it demands a bit more from the user. But how (if at all) do you see it as more of a security risk than going through the clipboard?

@stefanb2
Copy link
Contributor Author

I agree it demands a bit more from the user. But how (if at all) do you see it as more of a security risk than going through the clipboard?

In the sense that CTRL-TAB selects the wrong window, f.ex. the chat window instead of the browser window with the password dialog as the user intended. Of course that maybe just me and my usual sausage fingers :-)

Copy link
Owner

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

In the sense that CTRL-TAB selects the wrong window, f.ex. the chat window instead of the browser window with the password dialog as the user intended. Of course that maybe just me and my usual sausage fingers :-)

I'm pretty sure you could handle it 😆

@bkueng bkueng merged commit 6284c1f into bkueng:main Nov 27, 2023
16 checks passed
@stefanb2 stefanb2 deleted the topic-make-x11-dependency-optional branch November 28, 2023 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] get rid of X11 dependency
2 participants