Closed Bug 1243594 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Firefox Accounts, defect, P1)

46 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox45 --- unaffected
firefox46 + fixed
firefox47 + fixed

People

(Reporter: markh, Assigned: markh)

References

Details

(Keywords: regression, Whiteboard: [fxa-waffle-ignore])

Attachments

(3 files, 1 obsolete file)

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.
Component: Server: Firefox Accounts → FxAccounts
Flags: firefox-backlog+
Product: Cloud Services → Core
Priority: -- → P1
Adding rnewman for context from our IRC discussion, and gfritzsche as I'll ask him for review.
Assignee: nobody → markh
Summary: 400 errors during device registration → rest.js doesn't utf-8 encode payload causing 400 errors during device registration
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.
Attachment #8716154 - Flags: review?(rfkelly)
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/)
Attachment #8716155 - Flags: review?(gfritzsche)
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).
Attachment #8716155 - Flags: feedback?(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.
Attachment #8716154 - Flags: review?(rfkelly) → review+
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.
Attachment #8716155 - Flags: feedback?(rfkelly) → feedback+
Whiteboard: [fxa-waffle-ignore]
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.
Attachment #8716155 - Flags: review?(gfritzsche) → review+
> 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 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
Attachment #8716155 - Flags: approval-mozilla-aurora?
Attachment #8716155 - Flags: approval-mozilla-aurora?
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
Attachment #8719651 - Flags: review?(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 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.
Attachment #8719651 - Flags: review?(standard8) → review+
(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?
Attachment #8719651 - Attachment is obsolete: true
Attachment #8722272 - Flags: review?(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.
Attachment #8722272 - Flags: review?(standard8) → review+
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
Attachment #8716154 - Flags: approval-mozilla-aurora?
Recent regression, tracking.
Keywords: regression
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.
Attachment #8716154 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1227527
Version: unspecified → 46 Branch
Product: Core → Firefox
Target Milestone: mozilla47 → Firefox 47
You need to log in before you can comment on or make changes to this bug.