keepassxc-cli show: resolve references in output by cyphar · Pull Request #1280 · keepassxreboot/keepassxc

cyphar · 2017-12-14T16:00:20Z

Previously, keepassxc-cli show would not resolve references. This
would make it quite hard to script around its output (since there's not
interface to resolve references manually either). Fix this by using
resolveMultiplePlaceholders as with all other users of ->password() and
related entry fields.

Fixes #1260
Fixes #1269
Fixes #1211
Signed-off-by: Aleksa Sarai cyphar@cyphar.com

How has this been tested?

I tested this on my own KeePassXC database on several different entries (included nested ones), and verified that entries which were references now resolved to the correct value and that non-reference values also are correctly handled.

Types of changes

✅ Breaking change (fix or feature that would cause existing functionality to change)

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]
✅ I have added tests to cover my changes.

LeakSanitiser gives errors on develop at the moment, but as far as I can tell no additional ASAN errors were added by this change.

TheZ3ro · 2017-12-14T17:12:52Z

Just a note. Notes field also need to be resolved (see below)

keepassxc/src/gui/DatabaseWidget.cpp

Line 554 in fb6636d

    
           setClipboardTextAndMinimize(currentEntry->resolveMultiplePlaceholders(currentEntry->notes()));

cyphar · 2017-12-14T20:54:12Z

Ah, I didn't know you could use references for Notes (though I guess that makes sense, since references are used everywhere).

As an aside, I have a proposal that we should be applying resolveMultiplePlaceholders by default to all entry->m_attributes->value invocations, and you have to explicitly use something like entry->rawPassword() in order to not resolve the password. At the moment resolveMultiplePlaceholders is very repetitive and error prone (as this bug shows).

chuinul · 2017-12-14T21:29:40Z

Is this working for you? The fix seems correct but from an up-to-date develop it gives me an unexpected output: "resolved" fields are empty.

When tracing, Entry::resolveReferencePlaceholderRecursive captures all information from the reference, but refEntry is not found. Am I the only one in that situation?

TheZ3ro · 2017-12-14T22:02:29Z

@cyphar I agree, but we need to make a deep refactor of the code

cyphar · 2017-12-14T23:14:11Z

@chuinul I've seen this as well. I've figured it out and will push a fix for this problem and add some tests (the bug was that uuid::toHex() is lowercase, but references can be uppercase and the default style is uppercase).

TheZ3ro · 2017-12-14T23:27:50Z

Yup, it's already tracked as #1269

cyphar · 2017-12-15T01:37:42Z

Alright, I've done quite a few fixes in this PR now.

UUID searching is now case-insensitive (in particular the searching is done by-Uuid rather than a string comparison), which fixes the current bug in develop.
keepassxc-cli show resolves references.
KeePassHTTP's Access Control dialog resolves references.
Added tests to better protect against reference resolution regressions (especially in relation to cloned entries).

Tests all pass, and I've verified that things work now.

TheZ3ro

Nice work. 👍 for testing

louib · 2017-12-16T13:42:00Z

tests/TestEntry.cpp

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


@cyphar we normally place pointers on the left, just like Group* root above.

4c4d8a5 ("Implement search for reference placeholder based on fields other than ID") changed the semantics of searching-by-reference in KeePassXC. Unforuntately it contained a bug where it implicitly became case-sensitive to UUIDs, which broke existing databases that used references (especially since the default reference format uses a different case to the UUID used while searching). The tests didn't catch this because ->toHex() preserves the case that it was provided, they have been updated to check that UUIDs are case insensitive. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

Previously, `keepassxc-cli show` would not resolve references. This would make it quite hard to script around its output (since there's not interface to resolve references manually either). Fix this by using resolveMultiplePlaceholders as with all other users of ->password() and related entry fields. Fixes: #1260 Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

The access control dialog previously would not show the "real" username or "real" title when asking for permission to give access to entries. Fix this by resolving it, as we do in many other places. Fixes: #1269 Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

This ensures that the most "intuitive" current usage of references (through the clone feature of the GUI) remains self-consistent and always produces the correct results. In addition, explicitly test that case insensitivity works as expected. These should avoid similar regressions in reference handling in the future. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

cyphar · 2017-12-17T08:27:42Z

Rebased on #1289 since it's been merged.

TheZ3ro requested review from TheZ3ro and louib December 14, 2017 17:09

This was referenced Dec 15, 2017

Cloned entries: Refs should be lower case. Breaks autotype/keepasshttp #1269

Closed

cli: show: add --attributes flag #1289

Merged

TheZ3ro approved these changes Dec 15, 2017

View reviewed changes

louib reviewed Dec 16, 2017

View reviewed changes

cyphar added 4 commits December 17, 2017 19:06

louib approved these changes Dec 17, 2017

View reviewed changes

louib merged commit 9f8943c into keepassxreboot:develop Dec 17, 2017

cyphar deleted the keepasscli-resolve-references branch December 17, 2017 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

keepassxc-cli show: resolve references in output#1280

keepassxc-cli show: resolve references in output#1280
louib merged 4 commits intokeepassxreboot:developfrom
cyphar:keepasscli-resolve-references

cyphar commented Dec 14, 2017 •

edited by TheZ3ro

Loading

Uh oh!

TheZ3ro commented Dec 14, 2017 •

edited

Loading

Uh oh!

cyphar commented Dec 14, 2017 •

edited

Loading

Uh oh!

chuinul commented Dec 14, 2017

Uh oh!

TheZ3ro commented Dec 14, 2017

Uh oh!

cyphar commented Dec 14, 2017

Uh oh!

TheZ3ro commented Dec 14, 2017

Uh oh!

cyphar commented Dec 15, 2017 •

edited

Loading

Uh oh!

TheZ3ro left a comment

Uh oh!

louib Dec 16, 2017

Uh oh!

cyphar Dec 16, 2017

Uh oh!

cyphar commented Dec 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

cyphar commented Dec 14, 2017 • edited by TheZ3ro Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How has this been tested?

Types of changes

Checklist:

Uh oh!

TheZ3ro commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chuinul commented Dec 14, 2017

Uh oh!

TheZ3ro commented Dec 14, 2017

Uh oh!

cyphar commented Dec 14, 2017

Uh oh!

TheZ3ro commented Dec 14, 2017

Uh oh!

cyphar commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheZ3ro left a comment

Choose a reason for hiding this comment

Uh oh!

louib Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

cyphar Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

cyphar commented Dec 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cyphar commented Dec 14, 2017 •

edited by TheZ3ro

Loading

TheZ3ro commented Dec 14, 2017 •

edited

Loading

cyphar commented Dec 14, 2017 •

edited

Loading

cyphar commented Dec 15, 2017 •

edited

Loading