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 Entry placeholder resolution #10846

Merged
merged 4 commits into from
Jun 16, 2024

Conversation

c4rlo
Copy link
Contributor

@c4rlo c4rlo commented Jun 2, 2024

After resolving placeholders in entry attributes (e.g. {URL}), previously the code would do it all over again if anything had changed, multiple times up to the recursion limit. This would have the effect of applying a much greater recursion limit, which is confusing and unnecessary, and probably undesired.

Also, optimise and improve resolution of multiple placeholders by (a) being consistent about which placeholder substring to replace (i.e. the one found by the regex, as opposed to possibly one just introduced through a previous substitution), and (b) building the replacement string from scratch rather than doing successive string replacements.

Together, the above two changes are enough to fix #7158 in my testing.

While I was at it, I made some further improvements to the entry-related code:

  • Placeholder resolution: remove unnecessary special casing for self-referential placeholders (these are taken care of by existing recursion depth limit).
  • Entry::size(): when computing tag size, use the same delimiter set as in other places in the code.
  • Factor tag delimiter set regex out into global constant.
  • Move some constants from being public static data members of class Entry to being local to Entry.cpp (in an anonymous namespace).
  • Migrate some QRegEx instances to QRegularExpression, the modern alternative.
  • Miscellaneous minor code cleanups.

Also fixes #1741

Testing strategy

No new functionality. Existing tests look sufficiently comprehensive, and they continue to pass.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)
  • ✅ Refactor (significant modification to existing code)

@c4rlo c4rlo force-pushed the entry-placeholders-fix branch from 4453524 to 2c682fa Compare June 2, 2024 11:41
@droidmonkey
Copy link
Member

droidmonkey commented Jun 2, 2024

Oh awesome, I have been meaning to clean this code for years.... its a mess.

Can you also look into the "you've reached maximum depth of replacement" that gets thrown for some special sequences.
#1741

There are some other long standing issues: https://github.com/keepassxreboot/keepassxc/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3A%22feature%3A+Reference%2FPlaceholders%22

One thing I really want to move towards, this is another PR, is to swap the functions of Entry. Basically if you call Entry::Password for example, it will return the fully baked result with references resolved. Calling Entry::rawPassword or forcing to Entry::getAttribute(Entry::PASSWORD) would return the raw value. You also remove all the setter functions and force use of Entry::setAttribute(...). This wouldn't affect writing to databases since this is handled at the attribute level already. This moves us closer to a proper data model.

@droidmonkey
Copy link
Member

droidmonkey commented Jun 2, 2024

Just checked and the fixes here do not fix the findings in #1741

Setting an entry with a password of sdss{fdfdf}sdd is enough to trigger the issue. Just show the password in the preview panel.

@c4rlo c4rlo force-pushed the entry-placeholders-fix branch from 2c682fa to 8afff73 Compare June 2, 2024 16:21
@c4rlo
Copy link
Contributor Author

c4rlo commented Jun 2, 2024

My latest commit here should fix #1741.

@droidmonkey droidmonkey added this to the v2.7.9 milestone Jun 13, 2024
c4rlo added 3 commits June 16, 2024 08:05
After resolving placeholders, previously the code would do it all over
again if anything had changed, multiple times up to the recursion limit.
This would have the effect of applying a much greater recursion limit,
which is confusing and unnecessary, and probably undesired.
- Entry::size(): when computing tag size, use same delimiter set as in
  other places in the code
- Factor tag delimiter set regex out into global constant
- Placeholder resolution: remove unnecessary special casing for
  self-referential placeholders (these are taken care of by existing
  recursion depth limit)
- Placeholder resolution: less wasteful string building loop
- Move some constants from being public static data members of Entry to
  being local to Entry.cpp (in anonymous namespace)
- Migrate some QRegEx instances to QRegularExpression, the modern
  alternative
- Miscellanous minor code cleanups
When encountering a {brace-enclosed} substring, the placeholder
resolution logic would previously keep recursing until it hit the
recursion depth limit (currently 10). This would lead to "Maximum depth
of replacement has been reached" messages, and was also wasting CPU
cycles.

Fixes keepassxreboot#1741.
@droidmonkey droidmonkey force-pushed the entry-placeholders-fix branch from 8afff73 to 07e1faa Compare June 16, 2024 12:41
@droidmonkey droidmonkey merged commit da8874d into keepassxreboot:develop Jun 16, 2024
11 checks passed
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Jun 16, 2024
droidmonkey added a commit that referenced this pull request Jun 16, 2024
* Entry placeholder resolution: don't overdo it

After resolving placeholders, previously the code would do it all over again if anything had changed, multiple times up to the recursion limit. This would have the effect of applying a much greater recursion limit, which is confusing and unnecessary, and probably undesired.

* Entry tweaks and minor refactoring

- Entry::size(): when computing tag size, use same delimiter set as in other places in the code
- Factor tag delimiter set regex out into global constant
- Placeholder resolution: remove unnecessary special casing for self-referential placeholders (these are taken care of by existing recursion depth limit)
- Placeholder resolution: less wasteful string building loop
- Move some constants from being public static data members of Entry to being local to Entry.cpp (in anonymous namespace)
- Migrate some QRegEx instances to QRegularExpression, the modern alternative
- Miscellanous minor code cleanups

* Entry: fix hitting recursion limit with {braces}

When encountering a {brace-enclosed} substring, the placeholder resolution logic would previously keep recursing until it hit the recursion depth limit (currently 10). This would lead to "Maximum depth of replacement has been reached" messages, and was also wasting CPU cycles.

Fixes #1741

---------

Co-authored-by: Jonathan White <[email protected]>
pull bot pushed a commit to tigerwill90/keepassxc that referenced this pull request Jun 17, 2024
* Entry placeholder resolution: don't overdo it

After resolving placeholders, previously the code would do it all over again if anything had changed, multiple times up to the recursion limit. This would have the effect of applying a much greater recursion limit, which is confusing and unnecessary, and probably undesired.

* Entry tweaks and minor refactoring

- Entry::size(): when computing tag size, use same delimiter set as in other places in the code
- Factor tag delimiter set regex out into global constant
- Placeholder resolution: remove unnecessary special casing for self-referential placeholders (these are taken care of by existing recursion depth limit)
- Placeholder resolution: less wasteful string building loop
- Move some constants from being public static data members of Entry to being local to Entry.cpp (in anonymous namespace)
- Migrate some QRegEx instances to QRegularExpression, the modern alternative
- Miscellanous minor code cleanups

* Entry: fix hitting recursion limit with {braces}

When encountering a {brace-enclosed} substring, the placeholder resolution logic would previously keep recursing until it hit the recursion depth limit (currently 10). This would lead to "Maximum depth of replacement has been reached" messages, and was also wasting CPU cycles.

Fixes keepassxreboot#1741

---------

Co-authored-by: Jonathan White <[email protected]>
libf-de pushed a commit to libf-de/keepassxc-secretservice-dbus that referenced this pull request Jun 20, 2024
Release 2.7.9

* Passkeys: Ability to easily remove a passkey from an entry [keepassxreboot#10777]
* Snap: Use new desktop portal for native messaging integration [keepassxreboot#10906]

* Improve entry placeholder/reference feature [keepassxreboot#10846]
* Improve CSV importing when title field isn't specified [keepassxreboot#10843]
* Improve encrypted Bitwarden importing [keepassxreboot#10800]
* Improve database settings UX [keepassxreboot#10821]
* Improve handling of clipboard actions from entry preview [keepassxreboot#10810]
* Improve group/entry view resize behavior and set sensible defaults [keepassxreboot#10641]
* Passkeys: Fix incorrect username fill [keepassxreboot#10874]
* Passkeys: Return additional data to the extension [keepassxreboot#10857]
* Fix password clear timer inconsistency on unlock view [keepassxreboot#10708]
* Fix portability check [keepassxreboot#10760]
* Fix page overflow on HTML exports [keepassxreboot#10735]
* Fix broken builds when using system provided zxcvbn [keepassxreboot#10717]
* Fix copy password button when text is selected [keepassxreboot#10853]
* Fix tab ordering on application settings pages [keepassxreboot#10907]
* SSH Agent: Fix broken decrypt button [keepassxreboot#10638]
* Windows: Fix ALT Auto-Type modifier [keepassxreboot#10795]
* Windows: Fix wrong DACL memory size allocation [keepassxreboot#10712]
* macOS: Fix monospace font sizing [keepassxreboot#10739]
* Flatpak: Fix configuration settings off-by-one error [keepassxreboot#10688]
* BSD: Fix compiling with libusb implementation [keepassxreboot#10736]

# -----BEGIN PGP SIGNATURE-----
#
# iQEzBAABCAAdFiEENIkEDB8MPuq41ValRA/GXy4MbgEFAmZzTogACgkQRA/GXy4M
# bgHahggAg+hzMTiM0uDaw5yfxhv6GEfQQBPHMhX3JDyHEC+i7Pq6OjlxQkdUrRdu
# f4w74od5jSul0Al/ehu9L2eZwNPMnU87FWDn16o1btYHsG9n24v5S0DuQoLXUjde
# Y9nJNKeRNoWAlVKWbUG2YGvy9hF9YbtrFaiBksaQ+g3w8Xz82PzLY0VaUu4Xa/LO
# RXAhryJC+8T3T479dXpHxJcUmEWkoY4bqj1i6R8tEK5Kz9y1c0kqzqwWysKMj+rD
# WxTb2V4y9s57pO35zt9yxMLg66xx9bdcQHbSULa2vZNMFd9qdqk8WJmWFle112yG
# UCBXv2ZIjd3lghPt0IrD+WKcuL85Aw==
# =rbfs
# -----END PGP SIGNATURE-----
# gpg: directory '/home/runner/.gnupg' created
# gpg: keybox '/home/runner/.gnupg/pubring.kbx' created
# gpg: Signature made Wed Jun 19 21:32:56 2024 UTC
# gpg:                using RSA key 3489040C1F0C3EEAB8D556A5440FC65F2E0C6E01
# gpg: Can't check signature: No public key
Perlover added a commit to Perlover/keepassxc that referenced this pull request Jul 24, 2024
Release 2.7.9

* Passkeys: Ability to easily remove a passkey from an entry [keepassxreboot#10777]
* Snap: Use new desktop portal for native messaging integration [keepassxreboot#10906]

* Improve entry placeholder/reference feature [keepassxreboot#10846]
* Improve CSV importing when title field isn't specified [keepassxreboot#10843]
* Improve encrypted Bitwarden importing [keepassxreboot#10800]
* Improve database settings UX [keepassxreboot#10821]
* Improve handling of clipboard actions from entry preview [keepassxreboot#10810]
* Improve group/entry view resize behavior and set sensible defaults [keepassxreboot#10641]
* Passkeys: Fix incorrect username fill [keepassxreboot#10874]
* Passkeys: Return additional data to the extension [keepassxreboot#10857]
* Fix password clear timer inconsistency on unlock view [keepassxreboot#10708]
* Fix portability check [keepassxreboot#10760]
* Fix page overflow on HTML exports [keepassxreboot#10735]
* Fix broken builds when using system provided zxcvbn [keepassxreboot#10717]
* Fix copy password button when text is selected [keepassxreboot#10853]
* Fix tab ordering on application settings pages [keepassxreboot#10907]
* SSH Agent: Fix broken decrypt button [keepassxreboot#10638]
* Windows: Fix ALT Auto-Type modifier [keepassxreboot#10795]
* Windows: Fix wrong DACL memory size allocation [keepassxreboot#10712]
* macOS: Fix monospace font sizing [keepassxreboot#10739]
* Flatpak: Fix configuration settings off-by-one error [keepassxreboot#10688]
* BSD: Fix compiling with libusb implementation [keepassxreboot#10736]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup feature: Reference/Placeholders pr: backported Pull request backported to previous release
Projects
None yet
2 participants