Implement recursive resolving for placeholders by frostasm · Pull Request #1078 · keepassxreboot/keepassxc

frostasm · 2017-10-16T10:51:22Z

Implement recursive resolving for placeholders

Description

Recursive resolving for placeholders

From keepass help:

Referencing fields of other entries only works with standard fields, not with custom user strings. If you want to reference a custom user string, you need to place a redirection in a standard field of the entry with the custom string, using {S:}, and reference the standard field.

Currently KeePassXC does not support the described behavior.

Add support for {URL:Placeholders}

URL placeholders:

{URL:RMVSCM}
{URL:WITHOUTSCHEME} - analog for {URL:RMVSCM}
{URL:SCM}
{URL:SCHEME} - analog for {URL:SCM}
{URL:HOST}
{URL:PORT}
{URL:PATH}
{URL:QUERY}
{URL:FRAGMENT} - new url placeholder
{URL:USERINFO}
{URL:USERNAME}
{URL:PASSWORD}

Motivation and context

Implement behavior similar to keepass2

How has this been tested?

With unit test, TestEntry class.

Screenshots (if appropriate):

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)
✅ New feature (non-breaking change which adds functionality)

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]
✅ My change requires a change to the documentation.
✅ I have added tests to cover my changes.

frostasm · 2017-10-16T11:13:15Z

@droidmonkey @phoerious please review the changes

phoerious · 2017-10-16T11:11:47Z

src/core/Entry.cpp

+    }
+
+    QString result = str;
+    QRegExp placeholderRegEx("(\\{.*\\})", Qt::CaseInsensitive, QRegExp::RegExp2);


I don't like this regular expression. It matches zero-length placeholders and if greedy evaluation is used (it's not atm, but this may be a potential error source), it also matches placeholders containing closing braces and may produce unnecessary backtracking. You should change this to (\\{[^\\}]+).

phoerious · 2017-10-16T11:16:10Z

src/core/Entry.cpp

+
+QString Entry::resolvePlaceholderRecursive(const QString &placeholder, int maxDepth) const
+{
+    const PlaceholderType placeholderType = this->placeholderType(placeholder);


this pointer is not needed.

This is useful for debugging, have the value which is returned by the function in a separate variable. I renamed the local variable.

I meant the explicit this pointer is not needed. Just call placeholderType() without this->.

This code didn't work, variable name overrides the function name

const PlaceholderType placeholderType = placeholderType(placeholder);

Rename the variable then.

phoerious · 2017-10-16T11:19:23Z

src/core/Entry.cpp

+            const Entry* refEntry = m_group->database()->resolveEntry(referencedUuid);
+            if (refEntry) {
+                const QString wantedField = referenceRegExp->cap(wantedFieldIndex).toLower();
+                if (wantedField == "t") result = refEntry->title();


Don't put condition and body on the same line.

Use brackets on all conditional statements (styleguide).

phoerious · 2017-10-16T11:21:56Z

src/core/Entry.cpp

+    case PlaceholderType::UrlPassword:
+        return qurl.password();
+    default:
+        Q_ASSERT(false);


Use Q_ASSERT_X. Q_ASSERT(false) gives not useful error message.

phoerious · 2017-10-16T11:22:45Z

src/core/Entry.cpp

+
+Entry::PlaceholderType Entry::placeholderType(const QString &placeholder) const
+{
+    if (!placeholder.startsWith(QLatin1Char('{')) || !placeholder.endsWith(QLatin1Char('}'))) {


Can this case ever occur?

yes. In some places, the function "resolvePlaceholder" called directly.

phoerious · 2017-10-16T11:25:28Z

src/core/Entry.cpp

+        { QStringLiteral("{URL:PASSWORD}"), PlaceholderType::UrlPassword }
+    };
+
+    PlaceholderType result = placeholders.value(placeholder.toUpper(), PlaceholderType::Unknown);


Variable assignment is redundant. Directly return the result.

phoerious · 2017-10-16T11:26:05Z

tests/TestEntry.cpp

    delete entry;
 }
+
+void TestEntry::testResolveUrlPlaceholders()


+1 for tests!

phoerious · 2017-10-16T11:27:11Z

tests/TestEntry.cpp

+
+void TestEntry::testResolveUrlPlaceholders()
+{
+    Entry* entry = new Entry();


You could use a stack variable here. No need for dynamic allocation. Or at least use a QScopedPointer.

phoerious · 2017-10-16T11:31:04Z

tests/TestEntry.cpp

+
+void TestEntry::testResolveRecursivePlaceholders()
+{
+    Database* db = new Database();


TheZ3ro · 2017-10-16T13:16:33Z

tests/TestEntry.cpp

+    QString userinfo("user:pw"); // User information of the entry URL.
+    QString username("user"); // User name of the entry URL.
+    QString password("pw"); // Password of the entry URL.
+    QString fragment("fragment"); // Password of the entry URL.


The comment here is wrong, just a typo :)

frostasm · 2017-10-16T18:40:34Z

I have one question. When I make changes to correct the observations. I must use squashing or I can just push new changes with a new commit?

TheZ3ro · 2017-10-16T19:11:46Z

IMHO it's the best if you make new commits so we can see the changes easily.

We will squash when merging if needed (most of the time we don't)

frostasm · 2017-10-16T19:21:38Z

@TheZ3ro thanks for the explanation

TheZ3ro · 2017-10-16T19:27:20Z

thanks for your contributions 😉

droidmonkey · 2017-10-16T20:43:29Z

To me if its minor change thats obvious i just amend and force push. Otherwise a new commit is preferred.

frostasm · 2017-10-17T20:01:53Z

Oops, what is the minimum version of Qt should be supported?

phoerious · 2017-10-18T09:13:28Z

Until now it has been 5.2, which is required to build with Trusty. If it makes sense, we can upgrade it, though.

frostasm · 2017-10-19T18:08:34Z

I removed the usage of Q_ENUM

phoerious · 2017-10-19T18:48:58Z

tests/TestEntry.cpp

+    Database db;
+    Group* root = db.rootGroup();
+
+    Entry* entry1 = new Entry();


You used a stack variable for the test above, you can do the same here.

@phoerious can I not use stack variables? The database will still destroy all records.

You mean free store variables? Yes, you are right. I actually didn't mark it last time exactly for that reason. Forgot about it this time. All fine. Just rebase to current develop and I can merge this.

frostasm · 2017-10-19T20:36:34Z

@phoerious @droidmonkey @TheZ3ro thanks guys ;)

- 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]

phoerious requested changes Oct 16, 2017

View reviewed changes

phoerious added this to the v2.3.0 milestone Oct 16, 2017

phoerious added the new feature label Oct 16, 2017

TheZ3ro reviewed Oct 16, 2017

View reviewed changes

frostasm force-pushed the implement-recursive-resolving-for-placeholders branch from d8fce34 to 3a0c965 Compare October 17, 2017 19:55

frostasm force-pushed the implement-recursive-resolving-for-placeholders branch from 3a0c965 to 727c533 Compare October 19, 2017 18:01

phoerious reviewed Oct 19, 2017

View reviewed changes

phoerious approved these changes Oct 19, 2017

View reviewed changes

frostasm added 3 commits October 19, 2017 22:40

Add processing of the url placeholders

5e0df62

Refactor Entry::resolvePlaceholder function

e81d8be

Implement recursive resolving for placeholders

f0fcc19

frostasm force-pushed the implement-recursive-resolving-for-placeholders branch from 727c533 to f0fcc19 Compare October 19, 2017 19:41

phoerious merged commit 190d3a1 into keepassxreboot:develop Oct 19, 2017

frostasm deleted the implement-recursive-resolving-for-placeholders branch October 19, 2017 20:36

frostasm mentioned this pull request Oct 23, 2017

Nested references aren't working - only one reference layer is followed #1003

Closed

TheZ3ro mentioned this pull request Oct 28, 2017

Partial password may be leaked if it contains "{" and "}" #1137

Closed

phoerious added pr: new feature Pull request adds a new feature and removed new feature labels Nov 22, 2024

Uh oh!

Conversation

frostasm commented Oct 16, 2017

Description

Recursive resolving for placeholders

Add support for {URL:Placeholders}

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

frostasm commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frostasm commented Oct 16, 2017

Uh oh!

TheZ3ro commented Oct 16, 2017

Uh oh!

frostasm commented Oct 16, 2017

Uh oh!

TheZ3ro commented Oct 16, 2017

Uh oh!

droidmonkey commented Oct 16, 2017

Uh oh!

frostasm commented Oct 17, 2017

Uh oh!

phoerious commented Oct 18, 2017

Uh oh!

frostasm commented Oct 19, 2017

Uh oh!

phoerious Oct 16, 2017 •

edited

Loading

phoerious Oct 17, 2017 •

edited

Loading