-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Fix sync with new native clients #4932
Conversation
This should fix syncing with the new native clients. - Filter out password history items which have `null` - Redesigned the Secure Note Type check to be more robust - Check for `null` passwords in history during import (or key rotation) This should not be an issue during the key rotation, since we do not return the invalid password history items anymore. Fixes dani-garcia#4870 Fixes dani-garcia#4921 Signed-off-by: BlackDex <black.dex@gmail.com>
2d2e72d
to
21e4e02
Compare
I had hoped this would fix an issue that I had with a vault that wouldn't sync both with the old and the new client, which I suspected was due to corrupted data - the sync would be requested, but would fail, but I tried it on my recovery machine (where I restore a backup every day) and it didn't help when I changed my image from latest to testing. The dodgy account won't sync until I delete all of the items from trash. There are one or more items in trash that prevent syncing both on old and new clients, both with :latest and :testing docker images. With around 600 items in trash, it's a relatively big haystack to find the cipher that is preventing syncing, but I'll see if i can get a chance to do so, to find out why it is causing the issue. |
@BJReplay if it also prevents syncing on an older client that is weird. But would be good to know what would be breaking this indeed. |
@BlackDex I have backups of the corrupted data that I can restore into a test environment. I am travelling for a few days, but when I get home, I will try to find the offending cipher(s), and come up with a repro. |
OK, @BlackDex I've narrowed it down to two ciphers that cause the existing Android (2024.7.1 11086) and new Beta (2024.8.1 19099) client to fail to sync. It took a while, but I restored, and deleted ciphers until I could sync, at which point I knew that the deleted ciphers were the bad ciphers preventing syncing. The new client displays a message under my vault The two suspect cyphers are (listed using BW CLI
Using the latest testing build Deleting the first cipher listed above with the null lastUsedDate in the password history allows the remaining item (with a 1/1/1970 - so near enough to null last used date to sync. At a guess those history items were imported when I transitioned from cough lastpass with null dates via an import, but interesting nonetheless. Do you want me to open a new issue or is this edge case enough (Null Dates on Password History) that you're happy to let sleeping dogs lie)? |
@BJReplay Thanks, i was able to solve this :) |
It seemed to have been possible to have `null` date values. This PR fixes this by setting the epoch start date if either the date does not exists or is not a string. This should solve sync issues with the new native mobile clients. Fixes dani-garcia#4932 (comment) Signed-off-by: BlackDex <black.dex@gmail.com>
It seemed to have been possible to have `null` date values. This PR fixes this by setting the epoch start date if either the date does not exists or is not a string. This should solve sync issues with the new native mobile clients. Fixes #4932 (comment) Signed-off-by: BlackDex <black.dex@gmail.com>
Should that fix be in the latest :testing build on docker from 8 hours ago? The sync error still exists with that build |
@BJReplay it should indeed. That should at least resolve the null time issues which i was able to replicate. If it doesn't, then something else is wrong too. |
@BlackDex OK, I will restore and examine the ciphers again tomorrow - it is possible that there were more nulls in other fields so I am thinking of working out how to review the ciphers via BW CLI look for nulls. |
@BJReplay Looking at what you posted here, it doesn't look like anything else should be causing issues. It doesn't matter which client get's the sync, all clients receive the same sync data, so the exact data from the web-vault should be fine too. |
OK, will do. I've rotated the passwords for the production accounts, so I know that they're OK, and I'll disable and re-enable the TOTP sign on, so most of the data is fine to share in any case, so I'll try to get the data to you in the next couple of days. |
@BlackDex I can now repro and will attempt to provide the information as per the steps in #4870 as requested, but can provide information to assist:
I haven't reviewed your code, but that makes sense: you won't change a cipher on the database - it is what it is, but when you encounter a corrupt one, you fix null / invalid dates with a valid default. So, when I went to redact the cipher before capturing, and saved it with a redacted email, the null dates in the password history were written back as 1/1/1970, and suddenly my clients could sync. However, until then, only the web client would successfully read that cipher. Before I repro and attempt to capture, I'll see if I can see from your commit if the changes were only on saving a cipher, rather than on retrieving a cypher as well, as, from what I can see, the nulls only get replaced on saving. |
@BlackDex I have a logcat zipped - I will get it to you. I have rolled the TOTP and passwords in the logcat so the only sensitive information in the capture are my emails. Can confirm UI and vault export and mobile clients all receive null lastUsedDate for password history until some other attribute of a cipher is changed, at which point all nulls are updated to 1/1/1970. Logcat is from a few seconds before initial log in to qa build as linked above, to the error message We were unable to process your request. Please try again later or contact us. Try again
Vault Export from Web UI:
|
But, i have fixed that already 🤔. Very strange, the null dates should not have been returned using that version of Vaultwarden. They should all be epoch 0 time. You send me the adb logs, so ill check out those and see if i can find anything there. |
It seemed to have been possible to have `null` date values. This PR fixes this by setting the epoch start date if either the date does not exists or is not a string. This should solve sync issues with the new native mobile clients. Fixes dani-garcia#4932 (comment) Signed-off-by: BlackDex <black.dex@gmail.com>
This should fix syncing with the new native clients.
null
null
passwords in history during import (or key rotation)This should not be an issue during the key rotation, since we do not return the invalid password history items anymore.
Fixes #4870
Fixes #4921