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.

Sign up for GitHub

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

Jump to bottom

Fix updating reference passwords from KeePassXC-Browser #2218

Merged
merged 2 commits into from
Jan 21, 2019
Merged

Fix updating reference passwords from KeePassXC-Browser #2218

merged 2 commits into from
Jan 21, 2019

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Aug 22, 2018 •
edited
Loading

Fix updating reference passwords from KeePassXC-Browser.

Description

If an entry has a password field as a reference from another entry, updating credentials from KeePassXC-Browser overrides the reference with the new password.

This change detects the reference and updates the password to the original entry instead.

Motivation and context

This solves situations where the original entry doesn't have any URL set, but the clone(s) where the password is used as a reference does.

Fixes keepassxreboot/keepassxc-browser#271.

How has this been tested?

Manually. A test case has been also modified.

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

@varjolintu varjolintu added this to the v2.3.4 milestone Aug 22, 2018
@varjolintu varjolintu requested a review from a team August 22, 2018 08:13
@varjolintu varjolintu added the bug label Aug 22, 2018
@varjolintu varjolintu mentioned this pull request Aug 22, 2018
Closed
@varjolintu varjolintu changed the title Allow updating reference passwords from KeePassXC-Browser Fix updating reference passwords from KeePassXC-Browser Aug 22, 2018
droidmonkey
droidmonkey requested changes Aug 22, 2018
View reviewed changes
Copy link
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not check if the username is also a reference. This function can likely be made generic to update any chosen field and check for references appropriately.

Does this handle recursive references?

@droidmonkey droidmonkey modified the milestones: v2.3.4, v2.4.0 Aug 22, 2018
@droidmonkey
Copy link
Member

Please rebase onto develop after 2.3.4 is merged in.

@droidmonkey droidmonkey force-pushed the release/2.3.4 branch 2 times, most recently from 4264ed6 to d6cae74 Compare August 22, 2018 15:23
@varjolintu
Copy link
Member Author

The username is not overwritten when updating credentials. But you are correct that this might not work with recursive references.

PR is closed..?

@varjolintu varjolintu changed the base branch from release/2.3.4 to develop August 22, 2018 17:12
@varjolintu varjolintu reopened this Aug 22, 2018
@varjolintu
Copy link
Member Author

varjolintu commented Aug 22, 2018 •
edited
Loading

Todo:

  • Handle recursive references
  • Prevent updating the username to the original entry
  • Rebase

@varjolintu
Copy link
Member Author

@droidmonkey Necessary changes done.

@droidmonkey
Copy link
Member

I'll run it through some tests and see if i can still break it

@droidmonkey droidmonkey merged commit 0da9efd into keepassxreboot:develop Jan 21, 2019
@varjolintu varjolintu deleted the reference_update_fix branch January 22, 2019 05:23
droidmonkey added a commit that referenced this pull request Mar 19, 2019
@droidmonkey
Release 2.4.0
c51752d 
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droidmonkey droidmonkey droidmonkey approved these changes

Assignees
No one assigned
Labels
bug feature: Browser
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.

Improve support for field references
2 participants
@varjolintu @droidmonkey