Bug 2056116 - Don't crash sync on logins with invalid guids#7491
Conversation
A login guid that's invalid for the sync server panicked the uploader
("record's ID is invalid").
sync15 now returns an error instead of asserting, fetch_outgoing skips such
records, add_many_with_meta validates (or regenerates, under the
fixup_invalid_guids feature) the guid, and a v6 migration repairs existing
rows.
| Ok(if row.get::<_, bool>("is_deleted")? { | ||
| let guid: Guid = row.get::<_, String>("guid")?.into(); | ||
| // A guid that is invalid for the sync server would panic the uploader | ||
| // (bug 2056116). Such a record can never have reached the server, so |
There was a problem hiding this comment.
sorry you are hitting this. I'm not sure of the entire context here, but this assertion was really more a sanity check for our code - ie, the server will accept anything as a GUID. This means that if this record somehow ended up coming from the old passwords engine, then the record may well already be on the server.
It's not clear to me how much of your strategy here relies on "Such a record can never have reached the server". Another strategy (which I'm not sure I like entirely, but which I think would work) would be to just remove the assertion. That would leave us without this safely check, but I don't recall anyone ever hitting it in the past and if it turns out this ID was being used in the past then maybe that's not such a crazy idea.
There was a problem hiding this comment.
(oops, but I also meant to say this patch looks fine to me if this is the route we should take)
There was a problem hiding this comment.
Ah, cool, it’s definitely possible that this ID is already on our servers. In that case, I’ll take the check out of here but still fix the add_with_meta. And do you think I can migrate the IDs that are already in the store (the ones that were just migrated via add_with_meta on the desktop) here?
There was a problem hiding this comment.
If the GUID hit the server, then wouldn't a mobile logins engine download it and re-upload a record with the same GUID and hit the same panic? I'm not against either solution (removing the assertion or skipping the record), but I'd like to understand what's happening better before we commit to either one of them.
There was a problem hiding this comment.
Good point - maybe it was Desktop sync only, no mobile involved?
I think this happened:
- I added
add_with_metawithout validating guid - this was used on Desktop to import logins
- now these got synced, and for one user it crashed the entire fx because this local-only login had an invalid id
Before Rust logins, this login could have been synced with the old logins sync engine, but maybe only to other Desktop's?
That's why I thought we need to fix this on three levels:
- prevent
add_with_metato add invalid ids (or fix them, guarded by a flag) - fix this login on the users desktop
- somehow harden sync to not break on a single invalid login
There was a problem hiding this comment.
I think this only makes sense if it came from desktop originally - this error should mean it was impossible for it to originate on mobile. Even then it seems odd though - "add_with_meta without validating guid" doesn't fully explain it because someone generated a guid (ie, it's never a free-form text field), and the desktop password engine has always generated "valid" GUIDs up to now.
Fixing it is also complicated in theory. It would mean you need to leave a tombstone behind so it doesn't get duplicated on other devices. And it still leaves the (improbable!) possibility it gets edited on another device without the migration implemented yet, so the tombstone is resurrected.
This all seems so edge-casey - if we can't identify how this ID might have been created legitimately, then the best thing probably is to just skip the record entirely, both reading and writing - it seems unlikely that's actual data loss. That strategy couldn't work for bookmarks though - you can't sanely just skip a single parent folder. So it seems unfortunate that the problem looks like it needs to be solved in every engine which sucks as ever engine is theoretically susceptible.
If it helps or hurts, you can almost certainly reproduce this with about:sync - it lets you edit/add records via plain JSON - that's how I verified the server doesn't reject crazy guids. Indeed, if it seems like there's exactly 1 user with this problem, someone doing silly things with about:sync is the most likely explanation I can think of.
3 participants
https://bugzilla.mozilla.org/show_bug.cgi?id=2056116
A login guid that's invalid for the sync server panicked the uploader ("record's ID is invalid").
Fixing the problem on 3 levels:
add_many_with_metaPull Request checklist
[ci full]to the PR title.