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

Improve Portability Check #10760

Merged
merged 1 commit into from
May 27, 2024
Merged

Conversation

OnSlbWXt
Copy link

Screenshots

Fixes #10755
The check checked against keepassxc.ini being in the application directory, while the .portable file was actually being responsible for determining if the application was installed in portable mode.
In addition this fixes KeePassXCPortable on portableapps.com, as the check returned "not-portable" there, when it should have responsed "portable".
image

Testing strategy

No Test was conducted as it was only a filename change.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

Comment on lines 211 to 212
// If portable settings file exists save the JSON scripts to the application folder
if (QFile::exists(QCoreApplication::applicationDirPath() + QStringLiteral("/keepassxc.ini"))) {
if (QFile::exists(QCoreApplication::applicationDirPath() + QStringLiteral("/.portable"))) {

Choose a reason for hiding this comment

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

May be best to move this to an isPortable() function, especially if that check is done in more than one place.
Also, the comment still says "portable settings file". Should probably say "portable marker", or just "In portable mode".

Copy link
Member

Choose a reason for hiding this comment

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

Turn this into a function of Config: https://github.com/keepassxreboot/keepassxc/blob/develop/src%2Fcore%2FConfig.cpp#L522

Then use it there and here.

Copy link
Author

Choose a reason for hiding this comment

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

would do that, but for that I am not versed enough in cpp to safely and securely code it. I don't even a way to compile keepassxc (or any cpp project), so testing is also out of the window.
Throw me a base C-Project (or Java, or PHP) and I could do more, but for cpp I am unfortunately only able to do small fixes like this, which don't really change the structure of the code

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to point out that changing the behaviour of this code may result in unexpected failures for people who rely on it.

Copy link
Author

Choose a reason for hiding this comment

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

would do that, but for that I am not versed enough in cpp to safely and securely code it. I don't even a way to compile keepassxc (or any cpp project), so testing is also out of the window. Throw me a base C-Project (or Java, or PHP) and I could do more, but for cpp I am unfortunately only able to do small fixes like this, which don't really change the structure of the code

I attempted it in a local repository, but:

  • Config: portablePath would need to become a member-attribute or method getDefaultConfigs() would need an additional parameter, or we would duplicate code, leaving us open for something similar to happen in the future
  • Config: alternatively: set m_isPortable (from isPortable) inside getDefaultConfigs(), but having to call the function in the parameterized Constructor aswell, even tough it serves no other purpose
  • Config: can't put it into init, as it is called after getDefaultConfigs(), which requires said configs.

I am absolutely not comfortable in making that change myself.

@OnSlbWXt
Copy link
Author

OnSlbWXt commented May 15, 2024

I pushed how I would solve #10760 (comment)
but can not test the changes myself.

and apprently I am mixing static and non static fields.

src/core/Config.h Outdated Show resolved Hide resolved
@droidmonkey droidmonkey added this to the v2.7.9 milestone May 22, 2024
@droidmonkey
Copy link
Member

I fixed everything

@droidmonkey droidmonkey changed the title Fix 10755 Portability Check Improve Portability Check May 27, 2024
@droidmonkey droidmonkey merged commit 1fd8923 into keepassxreboot:develop May 27, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't save native messaging script for Browser Plugin
4 participants