From bug 1195569 in attachment 8712941 [details] we can see the following entries: > 1453935569159 FirefoxAccounts ERROR error POSTing /account/device: {"code":400,"errno":999,"error":"Bad Request","message":"Invalid request payload JSON format","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"} > 1453935569160 FirefoxAccounts ERROR device registration failed: {"code":400,"errno":999,"error":"Bad Request","message":"Invalid request payload JSON We should try and work out what this means.

This particular error string comes from down in the bowels of our web app framework, and it's a simple try/catch around attempting to parse the request body: try { var parsed = JSON.parse(payload.toString('utf8')); } catch (err) { return next(Boom.badRequest('Invalid request payload JSON format', err)); } So my first suspicion is that the client is somehow sending invalid JSON, maybe due to e.g. extended characters in the device name field.

Yep, I can reproduce this error log by setting my device name to "frosty the ☃", so this is probably due to mis-handling of non-ascii characters in the device registration payload.

It looks like we should have FxAccountsClient.jsm encode the JSON as utf-8 for all requests. A possible complexity is that _createSession() already utf8 encodes the email address which will probably break if we double-encode it (ie, it sounds like we should use the "raw" email address there and have _request unconditionally encode)

> have _request unconditionally encode +1, always and only encode/decode at the boundaries. *sigh*, anyone would think we were back in python land...

I hear there's a JS 3.0 in the works that will save us from this in the future ;)

FTR, we had a similar issue with the readinglist code - the code we added there can be seen at https://hg.mozilla.org/mozilla-central/file/5721360e23cd/browser/components/readinglist/ServerClient.jsm#l28 and the related tests is at https://hg.mozilla.org/mozilla-central/file/5721360e23cd/browser/components/readinglist/test/xpcshell/test_ServerClient.js#l244. I think we will find the same comments apply (ie, that we already decode the response - it's only the request that needs explicit encoding. We should try and get to this ASAP and uplift.

Adding rnewman for context from our IRC discussion, and gfritzsche as I'll ask him for review.

This patch isn't strictly necessary - it's just that without it the following patch *looks* wrong. Most references to "utf8" in Credentials.jsm are misleading, so I just nuked them. Given the result of .setup() just returns the passed-in params unchanged, I also removed them completely from the result set.

rest.js will currently JSON.stringify an object for the request body, but it makes no attempt to utf-8 encode it. All servers at the other end of these requests expect utf-8, so things go wrong. It would be possible to change this only for the request that caused this bug to be opened, but that would just leave the foot-gun in place for other future users of this module. While not strictly necessary, this patch also changes the Content-Type header of the request from "text/plain" to "application/json; charset=utf8" when an object is passed in (it remains text/plain if the caller passes a string). Note that a number of tests needed fixing for this - in particular, some of the test files are encoded as utf-8 and have non-ascii utf-8 sequences directly in the source file. These have all been replaced with Unicode literals. The request handlers in the test now often need to utf8 decode the request before checking the strings etc. All callers of this module in the tree are now in fxaccounts, so there should be no concern with compatibility for non-fxa clients (the bagheera implementation was worrying me a little, but that was removed from the tree a week ago, so \o/)

Comment on attachment 8716155 [details] [diff] [review] 0011-Bug-1243594-part-2-have-rest.js-automatically-encode.patch Oops - meant to request feedback from rfkelly for this change: (In reply to Mark Hammond [:markh] from comment #9) > While not strictly necessary, this patch also changes the Content-Type > header of the request from "text/plain" to "application/json; charset=utf8" > when an object is passed in (it remains text/plain if the caller passes a > string).

Comment on attachment 8716154 [details] [diff] [review] 0010-Bug-1243594-part-1-remove-misleading-references-to-u.patch Review of attachment 8716154 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. FTR, :markh and I spent a chunk of timing working through this code in IRC, and decided that nothing needed changing except the confusing names of the variables. All the values are being encoded (or not) as required by the protocol and API.

Comment on attachment 8716155 [details] [diff] [review] 0011-Bug-1243594-part-2-have-rest.js-automatically-encode.patch Review of attachment 8716155 [details] [diff] [review]: ----------------------------------------------------------------- Setting an explicit charset sounds good, thanks Mark. I tested it out by hand in the fxa-auth-server API and it seemed to work as intended.

Comment on attachment 8716155 [details] [diff] [review] 0011-Bug-1243594-part-2-have-rest.js-automatically-encode.patch Review of attachment 8716155 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/common/rest.js @@ +328,4 @@ > if (typeof data != "string") { > data = JSON.stringify(data); > + if (!contentType) { > + contentType = "application/json"; This changes behavior - before you would have JSON submitted as "text/plain" if no "content-type" was set. You should make sure that any such usage doesn't break server-side expectations after this patch.

> You should make sure that any such usage doesn't break server-side expectations after this patch. I expect all the server components will deal with this correctly. FWIW I built nightly with the patch applied and was able to successfully sync with my existing devices, including both upload and download of records.

https://hg.mozilla.org/integration/fx-team/rev/4f941cddcf34 https://hg.mozilla.org/integration/fx-team/rev/5688da9024b6

Comment on attachment 8716155 [details] [diff] [review] 0011-Bug-1243594-part-2-have-rest.js-automatically-encode.patch This approval request is for both patches here. Approval Request Comment [Feature/regressing bug #]: 1227527 (landed on 46) [User impact if declined]: FxA device registration will fail for users with non-ascii Sync device names, potentially preventing future device work from working as expected. [Describe test coverage new/current, TreeHerder]: New and existing tests pass. [Risks and why]: Low risk, limited to Sync/FxA [String/UUID change made/needed]: None

Backout: https://hg.mozilla.org/integration/fx-team/rev/97f2c31df5f5 https://hg.mozilla.org/integration/fx-team/rev/fb7ceaf4b009

These patches broke loop as it has its own work-around in place for the lack of utf-8 encoding. This patch removes that from loop and leaves the encoding to hawk/rest.js. https://treeherder.mozilla.org/#/jobs?repo=try&revision=694ee25f366a

Sorry, yesterday I had a day off, and I'm short on time today, I'll get to the review tomorrow at the latest.

Comment on attachment 8719651 [details] [diff] [review] 0007-Bug-1243594-part-3-leave-the-utf-8-encoding-of-the-p.patch Review of attachment 8719651 [details] [diff] [review]: ----------------------------------------------------------------- r=Standard8 with the slightly alternative change mentioned below. Sorry for the delay in getting to this. ::: browser/extensions/loop/chrome/content/modules/MozLoopService.jsm @@ -587,5 @@ > credentials = deriveHawkCredentials(sessionToken, "sessionToken", > 2 * 32, true); > } > > - if (payloadObj) { Rather than removing it, please could you change this section, so that it's wrapped in something like: if (Services.vc.compare(Services.appinfo.version, "47.0a1") < 0 && payloadObj) { (I've not tested it, but I believe that works). The add-on supports multiple versions, so if we can land this with the check already added, then I can simply import it into our github repo.

(In reply to Mark Banner (:standard8) from comment #20) > Rather than removing it, please could you change this section, so that it's > wrapped in something like: > > if (Services.vc.compare(Services.appinfo.version, "47.0a1") < 0 && > payloadObj) { > > (I've not tested it, but I believe that works). It does work, although there are a couple of complications: * It fails in xpcshell tests - appinfo.version is always == 1 there. I *could* add an explicit check that it's not == 1 (or that appinfo.os == "XPCShell" etc), but... * We are hoping to uplift these patches as FxA device registration is broken in Firefox 46 for non-ascii device names. So what I came up with was the hawk client exposes an attribute that indicates if it automatically utf-8 encodes bodies and we check that. This seems more robust and safe to uplift. What do you think?

Comment on attachment 8722272 [details] [diff] [review] 0003-Bug-1243594-part-3-leave-the-utf-8-encoding-of-the-p.patch Review of attachment 8722272 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Looks good r=Standard8. Please land as normal into the trees, I'll import it into our repo once it lands.

https://hg.mozilla.org/integration/fx-team/rev/6decc397d3d5 https://hg.mozilla.org/integration/fx-team/rev/19950e70819a https://hg.mozilla.org/integration/fx-team/rev/4e0bcdfc72de

https://hg.mozilla.org/mozilla-central/rev/6decc397d3d5 https://hg.mozilla.org/mozilla-central/rev/19950e70819a https://hg.mozilla.org/mozilla-central/rev/4e0bcdfc72de

I just landed the Hello specific parts in the Loop repo: https://github.com/mozilla/loop/commit/0bf905b2ef4a24844f6dff4b06ec33fb7bd3914a

Comment on attachment 8716154 [details] [diff] [review] 0010-Bug-1243594-part-1-remove-misleading-references-to-u.patch This uplift request is for all 3 patches Approval Request Comment [Feature/regressing bug #]: bug 1227527 [User impact if declined]: Users with non-ascii characters in their Sync device name will fail to register on every Sync startup. [Describe test coverage new/current, TreeHerder]: New tests, existing tests pass. [Risks and why]: Fairly low risk, limited to Sync and Loop. The loop changes in particular were made in a backwards compatible way. [String/UUID change made/needed]: None

Recent regression, tracking.

Comment on attachment 8716154 [details] [diff] [review] 0010-Bug-1243594-part-1-remove-misleading-references-to-u.patch UTF-8 support, recent regression, please uplift to aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/cce040dc3f15 https://hg.mozilla.org/releases/mozilla-aurora/rev/d20aa5b1d474 https://hg.mozilla.org/releases/mozilla-aurora/rev/aa06c3530cc8