application-services
application-services copied to clipboard
mozilla
Ensure incoming logins records have their fields "fixed up" before being applied
When there's an incoming record from sync, the fields should be "fixed up" before they are applied locally - currently they are just applied as-is.
The mirror should probably have the unfixed version - ie, the mirror should stay a faithful mirror of what's on the server.
For example, if an incoming record has a unicode (ie, non-punycode) version of a URL, it appears as though the record would end up without the punycode version, which is bad.
┆Issue is synchronized with this Jira Task ┆epic: a-s storage components (backlog)
Flagging this as a potential good-first-bug, if we can provide a few more details.
Is this still a good first bug? Are there any more details here? (I'm confused about what you mean by "mirror")
The "mirror" is a dedicated table which "mirrors" what we know is on the server - so when we sync, we first grab records from the server and stick it in this "mirror" - we then use the mirror to work out what needs to be done to the "local" table. Some comments here might be helpful.
The intent is to make sure we perform the same validation as we do for local changes to logins - in the example above, any URLs with extended characters are converted to "punycode" versions of the URL when we receive from them the app - but not when we get them from the server. In theory the server should never have them, but this is an additional line of defense. Ditto for some of the other fields. A critical difference though is that this code should never outright reject something, but instead use a fixed up version.
Because the exact set of validations hasn't been defined and therefore some further analysis is necessary, maybe "good-first-issue" isn't appropriate - certainly it's not suitable as someone's first ever Rust patch, but possibly is reasonable for a motivated person as their first app-services patch.
possibly is reasonable for a motivated person as their first app-services patch.
oops, to be clear, it's possibly a good-first-bug for a motivated person if we just to the punycode thing - eg, this test is checking local logins - a new test would be to modify one of the sync tests and pretend we are seeing an incoming version of the smiley-face domain, and making sure we apply the punycode version of it to the local db. This bug implies that we are probably applying the smiley-face version, which is bad.
Hi, I started working on this and think I've managed to setup a test that that tests a specific version of the issue here.
I've also added a simple prototype fix specifically for the origin case here
Note: I have little experience with rust so I'm sure I'm doing something wrong but wondering if I'm on the right track.
Moved to bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1865158
Change performed by the Move to Bugzilla add-on.