Fix resolving reference in entry field #1486

TheZ3ro · 2018-02-14T23:36:39Z

Description

This is a 2.3.0 hotfix

Reference resolving in 2.2.4 is broken, you can't resolve a reference if it's in a main field (title, username, etc)

This also prevent recursive loop when resolving references (for example inserting {TITLE} in the title field) and fixes a bug in the EditEntry widget not saving AutotypeWindowAssociations.

The code for reference resolving is very repetitive a refactor need to take place in next version.

How has this been tested?

Manually

Types of changes

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

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]

louib · 2018-02-15T00:12:17Z

This also prevent recursive loop when resolving references (for example inserting {TITLE} in the title field)

@TheZ3ro can you add a unit test for that?

TheZ3ro · 2018-02-15T10:21:44Z

Yes I can, but first I need to find out with the test that are already implemented didn't catch this regression since 2.2.4

TheZ3ro · 2018-02-15T22:58:59Z

I've added test case, but now they are failing in tests/TestEntry.cpp line 135 and 144 for #1485

phoerious · 2018-02-16T17:39:39Z

src/core/Entry.cpp

@@ -778,17 +778,30 @@ QString Entry::resolvePlaceholderRecursive(const QString& placeholder, int maxDe
    case PlaceholderType::Unknown:
        return placeholder;
    case PlaceholderType::Title:
-        return title();
+        if (placeholderType(title()) == typeOfPlaceholder) {


Took me a while to understand what you are doing here. You seem to be short cutting infinitely recursing self-references. I would prefer you use an explicit value on the right-hand side instead of typeOfPlaceholder. Makes the whole thing a lot easier to read.

Also: maxDepth isn't used at all. You need to check that to cancel the recursion and return the current value as-is when the counter hits 0 (then you also don't necessarily need the shortcut above, but you may keep it anyway, just make it easier to understand).
The same is true for resolveReferencePlaceholderRecursive() BTW.

I know about maxDepth, it was a 5 minute fix to avoid delaying the beta 2.3.0 release.

Like I said

The code for reference resolving is very repetitive, a refactor need to take place in next version.

Just check if maxDepth is 0 and then return the value unchanged. Much easier than everything else in this PR.

phoerious · 2018-02-16T17:45:30Z

tests/TestEntry.cpp

@@ -126,6 +126,24 @@ void TestEntry::testClone()
    QCOMPARE(entryCloneHistory->timeInfo().creationTime(), entryOrg->timeInfo().creationTime());
    delete entryCloneHistory;

+    Entry* entryCloneUserRef = entryOrg->clone(Entry::CloneUserAsRef);


QScopedPointer

All the Entry variables declared in this file need to be refactored as well, like the comment above: Should I do it or should we ship it like this and fix in the next version?

Just do it. It's basically search and replace.

phoerious

Looks good to me now. Only need to find out why the tests are failing (crashing).

TheZ3ro · 2018-02-17T11:57:00Z

@phoerious

I've added test case, but now they are failing in tests/TestEntry.cpp line 135 and 144 for #1485

…ry association bug

Improve test suite to use smart pointers where possible

phoerious · 2018-02-17T15:49:06Z

The problem was that you were trying to resolve references to entries which weren't members of the same database (or no database at all to be precise), so it crashed on dereferencing a nullptr.
Also you should use QScopedPointer only on entries which aren't reparented by setGroup().

I fixed those issues and cleaned up the commit history. Have a look.

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

TheZ3ro added this to the v2.3.0 milestone Feb 14, 2018

TheZ3ro changed the title ~~Resolve reference in entry field~~ Fix resolving reference in entry field Feb 14, 2018

TheZ3ro added bug high priority 🚨 labels Feb 14, 2018

TheZ3ro requested review from a team and weslly February 14, 2018 23:37

TheZ3ro force-pushed the hotfix/reference-in-field branch from ba11bf4 to 09ea6d4 Compare February 14, 2018 23:39

weslly approved these changes Feb 14, 2018

View reviewed changes

phoerious requested changes Feb 16, 2018

View reviewed changes

phoerious approved these changes Feb 17, 2018

View reviewed changes

louib approved these changes Feb 17, 2018

View reviewed changes

TheZ3ro and others added 3 commits February 17, 2018 16:36

Resolve reference in entry field, prevent recorsive loop, fix EditEnt…

34f2635

…ry association bug

Check maxDepth on placeholder resolution to avoid infinite recursion

0bad87d

Add test case for reference resolution in cloned entries

53db77e

Improve test suite to use smart pointers where possible

phoerious force-pushed the hotfix/reference-in-field branch from b805246 to 53db77e Compare February 17, 2018 15:46

phoerious merged commit 7713a7b into release/2.3.0 Feb 17, 2018

phoerious deleted the hotfix/reference-in-field branch February 17, 2018 17:04

phoerious mentioned this pull request Feb 26, 2018

Error when recovering an entry with references from history #1485

Closed

phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix resolving reference in entry field #1486

Fix resolving reference in entry field #1486

TheZ3ro commented Feb 14, 2018 •

edited

Loading

louib commented Feb 15, 2018 •

edited

Loading

TheZ3ro commented Feb 15, 2018

TheZ3ro commented Feb 15, 2018

phoerious Feb 16, 2018 •

edited

Loading

TheZ3ro Feb 16, 2018

phoerious Feb 16, 2018 •

edited

Loading

phoerious Feb 16, 2018

TheZ3ro Feb 16, 2018

phoerious Feb 16, 2018

phoerious left a comment

TheZ3ro commented Feb 17, 2018

phoerious commented Feb 17, 2018 •

edited

Loading

Fix resolving reference in entry field #1486

Fix resolving reference in entry field #1486

Conversation

TheZ3ro commented Feb 14, 2018 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

louib commented Feb 15, 2018 • edited Loading

TheZ3ro commented Feb 15, 2018

TheZ3ro commented Feb 15, 2018

phoerious Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

TheZ3ro Feb 16, 2018

Choose a reason for hiding this comment

phoerious Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

phoerious Feb 16, 2018

Choose a reason for hiding this comment

TheZ3ro Feb 16, 2018

Choose a reason for hiding this comment

phoerious Feb 16, 2018

Choose a reason for hiding this comment

phoerious left a comment

Choose a reason for hiding this comment

TheZ3ro commented Feb 17, 2018

phoerious commented Feb 17, 2018 • edited Loading

TheZ3ro commented Feb 14, 2018 •

edited

Loading

louib commented Feb 15, 2018 •

edited

Loading

phoerious Feb 16, 2018 •

edited

Loading

phoerious Feb 16, 2018 •

edited

Loading

phoerious commented Feb 17, 2018 •

edited

Loading