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.

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

Android app fails to sync due to malformed data - all other platforms work #1006

Closed
Lockszmith-GH mannequin opened this issue Jul 9, 2020 · 13 comments
Closed

Android app fails to sync due to malformed data - all other platforms work #1006

Lockszmith-GH mannequin opened this issue Jul 9, 2020 · 13 comments
Assignees

Comments

@Lockszmith-GH
Copy link
Mannequin

Lockszmith-GH mannequin commented Jul 9, 2020

Describe the Bug

Web vault, command line, Firefox addon, Chrome extension and Windows application all function properly.
Android app refuses to sync.
Login is granted, but sync never succeeds.

If synced in the past, all data remains accessible, edits are synced back to server.
However new items and edits from other sources are not synced.

Steps To Reproduce

Not sure about reproduction, as I'm not 100% sure how the data crept in.
However, using the debugger, I found 3 items that broke the CipherData constructor.

In my case specifically: response.Login was null and the LoginData constructor threw the exception.

I replace SyncCiphersAsync with the following code in order to identify the culprits:

        private async Task SyncCiphersAsync(string userId, List<CipherResponse> response)
        {
            System.Diagnostics.Trace.WriteLine("SyncCiphersAsync");
            var badBunch = response.Where(c => null == c.Login && null == c.Card && null == c.Identity && null == c.SecureNote).ToDictionary(
                    c => c.Id,
                    c => {
                        try
                        {
                            var test = new CipherData(c, userId);
                        }
                        catch (Exception e)
                        {
                            System.Diagnostics.Trace.WriteLine(
                                    new StringBuilder()
                                    .AppendLine("ERROR SyncCiphersAsync: new CipherData threw an exception!")
                                    .Append("Id: ").AppendLine(c.Id)
                                    .Append("Name: ").AppendLine(c.Name)
                                    .AppendLine().Append("Exception: ").AppendLine(e.Message)
                                    .AppendLine("StackTrace:").AppendLine(e.StackTrace)
                                    .ToString()
                                );
                            try
                            {
                            }
                            catch { }
                        }
                        return c;
                    }
                );
            var ciphers = response.Where(c => null != c.Login || null != c.Card || null != c.Identity || null != c.SecureNote ).ToDictionary(
                    c => c.Id,
                    c => new CipherData(c, userId)
                );

            await _cipherService.ReplaceAsync(ciphers);
        }

this helped me pin point the 'broken' item ids, then using the bw get item <id> cli tool I identified the items, and after reviewing removed them.

Expected Result

Sync should never fail, especially if it doesn't fail on any other platform.
At the very least, if an item breaks sync, an item id should be logged somewhere (server or client side).

A possible (and very simple) solution would be to just engulf the switch in the CipherData constructor with try...catch, as the 'default' case is OK with assigning nothing.

Actual Result

Sync just isn't completing, no error, no info.

Environment

  • Device: Android, any - tested on VM, OnePlus 3, OnePlus 6, Samsung S8+
  • Operating system: docker on linux (installed via docker scripts a long time ago)
  • Build Version: 2.5.0 (3093), although it's been this way for a while.
  • Is this a Beta release? N

Additional Context

There are quite a few Sync related issues here, some are related to bitwarden_rs, this is not the case for me.

As always, you probably have the best password vault solution in the world right now, mainly because of the open-source nature of it.
Thank for maintaining it, and making it a reality.

@kspearrin kspearrin mannequin assigned cscharf Jul 9, 2020
@Lockszmith-GH
Copy link
Mannequin Author

Lockszmith-GH mannequin commented Jul 9, 2020

Until the client is patched (if that's what devs decide), here is a quick way to get rid of troublesome entries.

Using the bw cli on Windows and using PowerShell the code below will help identify potential problem items, and help remove them.
'Bad' items are deemed any items you have that does not have values at all for it's username and passwords.

Before removal, the code below will preserve an export of all of your data first, and a list of the removed items before it actually removes them.

Use with extreme caution

  • Final NOTE: in order to actually allow the removal, you will need to uncomment the deletion command (remove the # from the beginning of the line)
# saving bw vault, and removing invalid entries

$pw = (Get-Credential -Message "bw password" -UserName 'nevermind').GetNetworkCredential().password

bw export "$pw" --format json --output .\bw_1.json ## The only way to export Unicode chars properly
(bw list organizations | convertfrom-json) | % { Write-Host "`nExporting org: $($_.id) $($_.name)..."; bw export "$pw" --format json --organizationid $_.id --output ./bw_$($_.id) }

$vault = bw list items | convertfrom-json
$bad = $vault | Where-Object { $_.type -eq 1 -and $_.login.password -eq $null -and $_.login.passwordRevisionDate -eq $null -and $_.login.totp -eq $null -and $_.login.username -eq $null -and $_.login.uris -eq $null } | Select-Object -Property organizationId,Id | Group-Object -Property organizationid
$bad | Export-Clixml -Path ./bad_BW.clixml

# delete bad entries, the line is commented to prevent accidental deletion.
# $bad.group.id | % { bw delete item "$_" --permanent}

bw sync

Remove-Variable pw # this will 'forget' the password you entered.

@addisonbeck
Copy link
Contributor

Thanks for all the information here!

In regards to this:

'Bad' items are deemed any items you have that does not have values at all for it's username and passwords.

Is that what the bad items you removed looked like? I'm not able to recreate this scenario just by having a login with a blank username & password. Was there anything else irregular about these items? Was their 'Name' field also empty?

@Lockszmith-GH
Copy link
Mannequin Author

Lockszmith-GH mannequin commented Jul 10, 2020

The 'bad items' from the cli was a larger set then those I found in the Android debugger.
It's hard to pin point what exactly got malformed, but for some client gets a null for the login object (in the Android app it is typed as LoginData class) which contained Username, Password, Totp, PasswordRevisionDate and list of Uris.
The PowerShell code just identifies were all of these values are blank.

In practice, it is possible for all to be empty, and the entry still be valid for sync with Android, but in my particular case, instead of an empty object, the value returned was invalid and the object was completely missing.

I have cleared the culprits from my Db, so can't validate anymore right now.
But the solution I proposed (putting a try-catch around the switch block) will at least cause Android client to function exactly like the rest of the platform clients.

@Lockszmith-GH
Copy link
Mannequin Author

Lockszmith-GH mannequin commented Jul 10, 2020

I can create a pull request, I just want to get developer's general approval on the concept of the solution.

@addisonbeck
Copy link
Contributor

Sure, go ahead! I was hoping to figure out how this happened in the first place & maybe reproduce so we can try and stop this kind of malformed data from making it far enough to need to get catched on sync, but it looks like that might not be feasible, and your solution is worth implementing regardless for any similar issues.

@kspearrin
Copy link
Mannequin

kspearrin mannequin commented Jul 10, 2020

@Lockszmith Are you using bitwarden_rs?

@Lockszmith-GH
Copy link
Mannequin Author

Lockszmith-GH mannequin commented Jul 11, 2020

no, although I did see this happen with bitwarden_rs (which a friend is running) and on my own bitwarden-vanilla self-hosted server.
In both vaults the sync broke at the same location.
The solution above fixed it for both.

I'll prep the PR.

kspearrin mannequin pushed a commit that referenced this issue Jul 11, 2020
@cscharf
Copy link
Mannequin

cscharf mannequin commented Jul 13, 2020

Merged PR adds corrective behavior to the crash, keeping this open to try to reproduce the cause of that bad data in the first place and take corrective action at the source.

@cscharf
Copy link
Mannequin

cscharf mannequin commented Jul 13, 2020

@Lockszmith , can you specify how those entries were originally added to your vault? Did you use the import tool, the CLI, browser extension, or web vault to add them (or another client)?

@cscharf
Copy link
Mannequin

cscharf mannequin commented Jul 23, 2020

Closing issue for now, would love to get more information however on at least the source of the bad data, whether or not it was imported, from where, etc.

@cscharf cscharf mannequin closed this as completed Jul 23, 2020
@Lockszmith-GH
Copy link
Mannequin Author

Lockszmith-GH mannequin commented Jul 23, 2020

@cscharf

@Lockszmith , can you specify how those entries were originally added to your vault? Did you use the import tool, the CLI, browser extension, or web vault to add them (or another client)?

Sorry somehow missed your questions earlier - I believe it was via import, but unsure, because I noticed the sync failing weeks after the fact

@cscharf
Copy link
Mannequin

cscharf mannequin commented Jul 23, 2020

No worries and thanks for the additional info... Do you remember what source you imported from (another pw manager, csv, etc.)?

@Lockszmith-GH
Copy link
Mannequin Author

Lockszmith-GH mannequin commented Jul 23, 2020

My first import (a couple of years ago) was LastPass - don't think I had issues there, since then only exports from web vault.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant