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.
Yeah, that leaves me pretty stumped, too.
Just a thought: It could be a very old login, from a time when it might have been possible to create strange ids, eg via import or whatever. There were also times when extensions could write logins directly.
How exactly would you handle this now? Should we monitor the error for now to see if it occurs more often?
There was a problem hiding this comment.
eg via import or whatever.
yeah! Meant to suggest this as a theory.
If literally one device, then yeah, I'd say ignore it. I don't really know how to read those stats in terms of client vs crash counts - but raw crash counts are very low, and I think the check makes sense rather than loosening here? So somehow making noise and ignoring this rather than crashing or otherwise blocking other items seems ideal? Sorry I haven't dug into the actual impl here to know if that actually makes sense
There was a problem hiding this comment.
and I think the check makes sense
Does it? This is a genuine question? 😅 I'd be easily convinced we just drop the assert - love opinions from everyone :)
There was a problem hiding this comment.
The is_valid_for_sync_server check behaves differently from the server-side Python implementation referenced in the comment above the function as well as in the Rust successor's validation regexp: the client-side validation also checks for a comma in the ID. This difference in validation actually argues in favor of removing the client-side validation entirely.
At the very least, as a minimal fix, we definitely shouldn’t panic about this. What do you think - the release is in a week, and I think we might want to include a fix for 2056116?
There was a problem hiding this comment.
So I've broken this down into two parts:
- Validate IDs during
add_with_meta(and optionally fix them via a feature) Bug 2056116 - Validate guids in add_many_with_meta #7524 - The towel: Bug 2056116 - Don't crash sync on an invalid login guid #7536
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.