Closed Bug 1275616 Opened 5 years ago Closed 4 years ago

FxAccounts should send strings through webchannels

Categories

(Firefox :: Firefox Accounts, defect, P3)

Product:
Firefox
Component:
Firefox Accounts
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: tcsc, Assigned: stomlinson)

References

Details

The origin "https://accounts.firefox.com" was special cased to allow the fxa code to send objects through webchannels as part of bug 1238128. As part of the work for bug 1274512, eventually we'd like to remove this special case, so it should only send strings.
Blocks: 1275612
There's some context on our options in this comment from a previous bug:

  https://bugzilla.mozilla.org/show_bug.cgi?id=1238128#c15

Broadly, I think we should do this as part of a more general overhaul of how FxA web content coordinates with the browser, since there's going to be significant churn anyway.
Also FTR, there's no expectation that this bug and its brethren be resolved "soon" - ie, a naive strategy for some consumers impacted by this would be to wait until (say) Fx55 is on release then decide that all clients we care about can handle strings and unconditionally move to them. Obviously taking the opportunity to do this as part of a general overhaul as Ryan mentions would also be awesome.
Priority: -- → P3
We need to move forward on this at some point, ni? myself to ensure I revisit it.
Flags: needinfo?(rfkelly)
:stomlinson I want to put this back on your radar as well.  As a refresher of the context here: we need to stop sending objects in the body of our WebChannel messages, and start sending only strings.  For defense-in-depth security reasons.

From Comment 1 above, I think we intended to do this with the next major rev of our fx_desktop and fx_android contexts.  Are we looking at such a revision for the "handshake" work you were prototyping in Hawaii?
Flags: needinfo?(rfkelly) → needinfo?(stomlinson)
So FTR, Firefox landed initial support for this in Firefox 50. In practice, this means that the earliest we can reasonably do this is when we choose to no longer support ESR 45, as that doesn't have the support. According to the release calendar, the earliest that could be is ~June next year. To be safe, that probably means August, when ESR 52.3 will exist and there's been 2 trains without an ESR 45 release.

So yeah, some time away, and hopefully well after Shane lands his current work ;)
(In reply to Ryan Kelly [:rfkelly] from comment #4)
> :stomlinson I want to put this back on your radar as well.  As a refresher
> of the context here: we need to stop sending objects in the body of our
> WebChannel messages, and start sending only strings.  For defense-in-depth
> security reasons.
> 
> From Comment 1 above, I think we intended to do this with the next major rev
> of our fx_desktop and fx_android contexts.  Are we looking at such a
> revision for the "handshake" work you were prototyping in Hawaii?


I didn't have plans to piggyback this onto the other work, though we could in
theory.

If we could just stop writing new features and creating bugs, I could get back
and finish this.
Flags: needinfo?(stomlinson)
> So FTR, Firefox landed initial support for this in Firefox 50.

It's not entirely obvious from the comments above, but the behaviour of the browser after v50 is:

* If you send a string, it will be JSON decoded into an object
* If you send a non-string, you'll get an error, unless you're on a special allowlist that includes FxA

So IIUC, what Mark is proposing is that we want for anything < 50 to be unsupported, then we just switch to sending strings instead of objects within our existing auth brokers.  Browsers >= 50 will handle this transparently.

This seems OK to me.  I guess if we wanted to be done sooner, we could UA-sniff for >= 50 and just ship it now.  But we try not to do that too much...
(In reply to Ryan Kelly [:rfkelly] from comment #7)
> So IIUC, what Mark is proposing is that we want for anything < 50 to be
> unsupported, then we just switch to sending strings instead of objects
> within our existing auth brokers.  Browsers >= 50 will handle this
> transparently.

Yep, thanks for clarifying!
(In reply to Ryan Kelly [:rfkelly] from comment #7)
> > So FTR, Firefox landed initial support for this in Firefox 50.
> 
> So IIUC, what Mark is proposing is that we want for anything < 50 to be
> unsupported, then we just switch to sending strings instead of objects
> within our existing auth brokers.  Browsers >= 50 will handle this
> transparently.
> 
> This seems OK to me.  I guess if we wanted to be done sooner, we could
> UA-sniff for >= 50 and just ship it now.  But we try not to do that too
> much...


I just did a DataDog graph to see the distribution of Fx Desktop versions used to sign up. I count the number of submits on the signup screen as an approximation of people trying to sign up. The graph is at [1].

For those without DataDog access, the synopsis, converted to percentages, and sorted by popularity:

Version	% of total
50	89.4
49	3.1
47	1.8
45	1.2
48	1.2
51	1.0
43	0.9
44	0.5
46	0.4
41	0.4

~ 9.6% of new Desktop users come from Fx < 50.

10% of a large number is a large number, causing a 10% drop in signups is likely to cause some consternation. UA sniffing and sending strings >= Fx 50 seems entirely reasonable so we can protect users we can protect as quickly as possible. 

:markh, has Fennec received the same WebChannel update to expect strings? If not, the content server has to keep sending Objects over WebChannels until Fennec is updated, so I don't mind maintaining both capabilities in the content server.

[1] - https://app.datadoghq.com/dash/54226/fxa-content---login?live=true&page=0&is_auto=false&from_ts=1482849213420&to_ts=1483454013420&tile_size=m&fullscreen=172731044
Flags: needinfo?(markh)
Assignee: nobody → stomlinson
Opened on the content server at https://github.com/mozilla/fxa-content-server/issues/4577, assigned to myself.
I have a tentative content-server fix at https://github.com/mozilla/fxa-content-server/pull/4579, just need :markh's thoughts on whether Fennec also expects a stringified WebChannel `detail` property.
As mentioned in the PR, Fennec uses the exact same code, and best I can tell, the exact same version numbering (eg, bug 1287884 is listed as tracking version 50 even though it is a Fennec only bug)
Flags: needinfo?(markh)
The accounts-side work for this has shipped, browsers >= 50 should now receive stringified webchannel messages rather than raw JSON objects.  I guess the next step here is to see if we can remove the client code that falls back to using the raw object?
(In reply to Ryan Kelly [:rfkelly] from comment #13)
> The accounts-side work for this has shipped, browsers >= 50 should now
> receive stringified webchannel messages rather than raw JSON objects.  I
> guess the next step here is to see if we can remove the client code that
> falls back to using the raw object?

I think we can test this by modifying the |webchannel.allowObject.urlWhitelist| preference, and assuming that works, the patch here would just be to change that pref's default value.
(In reply to Mark Hammond [:markh] from comment #14)
> I think we can test this by modifying the
> |webchannel.allowObject.urlWhitelist| preference, and assuming that works,
> the patch here would just be to change that pref's default value.

But TBH, changing that pref would really only be checking a string is *actually* sent - the code that looks at that pref is only hit when the channel message is *not* a string.
(In reply to Mark Hammond [:markh] from comment #14)
> the patch here would just be to change that pref's default value.

Actually that's not complete - services/fxaccounts/FxAccountsConfig.jsm also has code to adjust that pref based on the auto-config endpoint we hit - all references to that pref in that file should also be nuked.
:markh - I'm removing myself from "Assigned to". I thought this would only have a content-server portion and have too many balls in the air to complete the browser portion.
Assignee: stomlinson → nobody
See Also: → 1346072
Yeah, let's not conflate the multiple issues here - FXA no longer sends string, which is what this bug describes, so marking it as fixed. I opened bug 1346072 for the client work.
Assignee: nobody → stomlinson
No longer blocks: 1275612
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
(In reply to Mark Hammond [:markh] from comment #18)
> FXA no longer sends string

Doh - that should say: FxA no longer sends objects and instead sends strings.
Product: Core → Firefox
You need to log in before you can comment on or make changes to this bug.