keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
20.17k stars 1.42k forks source link

Improve Entry placeholder resolution #10846

Closed c4rlo closed 1 month ago

c4rlo commented 1 month ago

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:

Also fixes #1741

Testing strategy

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

Type of change

droidmonkey commented 1 month ago

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. https://github.com/keepassxreboot/keepassxc/issues/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 commented 1 month ago

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 commented 1 month ago

My latest commit here should fix #1741.