Narigo / keepass-diff

A CLI-tool to diff Keepass (.kdbx) files. Useful, if syncing with Dropbox or NextCloud and getting multiple files due to conflicts.
https://keepass-diff.narigo.dev/
MIT License
306 stars 28 forks source link

Crashes after entering passwords - panic on unwrap() call #18

Closed NuclearFej closed 3 years ago

NuclearFej commented 4 years ago

Hi -

When I run keepass-diff, it takes the arguments for the paths to each file just fine, but after the passwords are entered interactively, it crashes with the following error:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src\libcore\option.rs:378:21

I'm running Windows. First try was in WSL, installed from cargo which was version 0.3.0. Then I downloaded and built version 1.0.0 myself, again in WSL, same problem. Then I switched to PowerShell and built it again, same version 1.0.0, same error.

Your program looks really nice, thanks for making it! Doesn't work for me though, I'm curious if anyone else is experiencing this bug.

I don't know Rust but if you don't want to fix this I could learn & take a look at it myself.

Narigo commented 4 years ago

I would love to have help here. I don't have a Windows box to develop, so I'm merely hoping that it works right now... I'd like to have a better test suite that works not only on Mac/Linux, so if you have ideas how to do that... :)

Is it not working for your specific files or can you create a non-private reproducer, so I can try to see if I get the same error and debug it?

Freekers commented 4 years ago

Regarding not having a Windows box to develop; have a look at VirtualBox, which allows you to run Windows virtual machines on Mac/Linux :)

Narigo commented 4 years ago

Thanks for the hint @Freekers, I have used VirtualBox before and I should have a Windows license on my old laptop somewhere. The thing is: I don't really want to set everything up on my newer device, learn how WSL works, how PowerShell works (and what the difference to WSL might be), how to set up Rust properly and see if I can reproduce the issue. It would take me more than an afternoon to basically get to that point and then I need to check how to fix it at all.

From the initial posting, I suspect it could be multiple things:

So from the initial posting, I wouldn't mind if @NuclearFej tries to look into it and I can give pointers when necessary :) We all might learn from it ;)

NuclearFej commented 4 years ago

It's caused by this bug. https://github.com/sseemayer/keepass-rs/issues/16

Narigo commented 4 years ago

Thanks for the investigation @NuclearFej ! Do you have a test file for me with an empty title? Maybe we can at least have a fix for that. It looks like the KeepassX software on Mac OS always returns a title (the empty string), so keepass-rs seems to return Some("") for me instead of panicking. While looking into this, I found another issue: If you have the same key multiple times in a group, only the last entry will show up.

NuclearFej commented 4 years ago

I am no longer able to reproduce the bug. Neither my old database nor a newly created one with empty titles causes the bug to occur. I tested under both Windows and WSL 2 (Debian), building fresh from this repo each time.

Perhaps KeePass was updated to match KeePassX's behavior. In any case, I'm sorry I wasn't able to be of more help.

(I'm a bit curious, though, why and when keepass-rs returns None instead of an empty string - since XML doesn't really have null, just empty tags - this is what you get when you export an entry with an empty title: `

Title
<Value></Value>

` ...just a tag with nothing in it, like you'd expect.)

Narigo commented 4 years ago

I haven't looked into the specification of the KeePass XML, but maybe there is no <Key>Title</Key> present at all in such a case and some clients may export it like that, while others always create <Key>Title</Key>?

NuclearFej commented 3 years ago

Upstream was patched. Not sure if it's necessary, I complied with the newer version 0.4.8 and it works for me but so did the old version.

Narigo commented 3 years ago

This should be included in the latest release now. @NuclearFej if you have a chance to confirm, that would be great :) I'll close this issue now and will reopen if the issue persists.