1243594 - rest.js doesn't utf-8 encode payload causing 400 errors during device registration

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 1

[Ryan Kelly [:rfkelly]]

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.

Comment 2

[Ryan Kelly [:rfkelly]]

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.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

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)

Comment 4

[Ryan Kelly [:rfkelly]]

> have _request unconditionally encode

+1, always and only encode/decode at the boundaries.

*sigh*, anyone would think we were back in python land...

Comment 5

[Mark Hammond [:markh] [:mhammond]]

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

Comment 6

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

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

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0010-Bug-1243594-part-1-remove-misleading-references-to-u.patch

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.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0011-Bug-1243594-part-2-have-rest.js-automatically-encode.patch

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 10

[Mark Hammond [:markh] [:mhammond]]

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 11

[Ryan Kelly [:rfkelly]]

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 12

[Ryan Kelly [:rfkelly]]

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 13

[Georg Fritzsche [:gfritzsche]]

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.

Comment 14

[Ryan Kelly [:rfkelly]]

> 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.

Comment 15

[Pulsebot]

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

Comment 16

[Mark Hammond [:markh] [:mhammond]]

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

Comment 17

[Pulsebot]

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

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0007-Bug-1243594-part-3-leave-the-utf-8-encoding-of-the-p.patch

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

Comment 19

[Mark Banner (:standard8)]

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 20

[Mark Banner (:standard8)]

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.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1243594-part-3-leave-the-utf-8-encoding-of-the-p.patch

(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 22

[Mark Banner (:standard8)]

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.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2bff4542410

Comment 24

[Pulsebot]

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

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 26

[Mark Banner (:standard8)]

I just landed the Hello specific parts in the Loop repo:

https://github.com/mozilla/loop/commit/0bf905b2ef4a24844f6dff4b06ec33fb7bd3914a

Comment 27

[Mark Hammond [:markh] [:mhammond]]

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

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Recent regression, tracking.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 30

[Carsten Book [:Tomcat]]

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