913199 - Use HTTPS for FxAccounts server URL

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Zachary Carter [:zaach]]

Attachment: use_https.patch

Now that certs are deployed on the Firefox Accounts jelly server we can switch to using HTTPS.

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 800374 [details] [diff] [review]
use_https.patch

Review of attachment 800374 [details] [diff] [review]:
-----------------------------------------------------------------

Trivial r+ for the code change, but a bigger question arises: do you ever want to speak non-HTTPS for FxA? If so, you need to validate the URL accordingly. Please make and document that decision, and if you decide you do want to restrict the protocol, update the patch and request re-review.

There might be additional sec concerns around hijacking this pref, so I encourage you to raise that during sec review or sooner.

Comment 2

[Zachary Carter [:zaach]]

For our deploy(In reply to Richard Newman [:rnewman] from comment #1)
> Comment on attachment 800374 [details] [diff] [review]
> use_https.patch
> 
> Review of attachment 800374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Trivial r+ for the code change, but a bigger question arises: do you ever
> want to speak non-HTTPS for FxA? If so, you need to validate the URL
> accordingly. Please make and document that decision, and if you decide you
> do want to restrict the protocol, update the patch and request re-review.
> 
> There might be additional sec concerns around hijacking this pref, so I
> encourage you to raise that during sec review or sooner.

For our deployments? We'd only speak to it using HTTPS. Someone testing a local instance might use non-HTTPS. If someone were to run their own FxA+Sync servers (if that will be an option), maybe non-HTTPS.

Comment 3

[Zachary Carter [:zaach]]

Attachment: use_https-2.patch

We're requiring HTTPS now. I've added a comment to the pref and a check.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 810217 [details] [diff] [review]
use_https-2.patch

Review of attachment 810217 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +51,5 @@
>  
>    _getAccountsURI: function () {
> +    let url = Services.urlFormatter.formatURLPref("firefox.accounts.remoteUrl");
> +    if (!/^https:/.test(url)) {
> +      throw new Error("Firefox Accounts server must use HTTPS");

You call this method in two places:

  * Initing the wrapper
  * When injecting data.

Neither of these places are expecting it to throw -- for example, if this throws the load of aboutaccounts.js will fail! -- so I tentatively assert that this patch isn't complete.

And ideally you'll have a test for the behavior when there's an invalid value in the pref... living documentation.

Comment 5

[Zachary Carter [:zaach]]

Attachment: use_https-3.patch

Thanks rnewman. This patch refactors things a bit to favor testability. 

(do_check_throws should really be defined by the test runner!)

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 810805 [details] [diff] [review]
use_https-3.patch

Review of attachment 810805 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits.

I'll file a bug to lift do_check_throws to head.js.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +13,5 @@
>    //dump("FXA: " + msg + "\n");
>  };
>  
> +function error(msg) {
> +  dump("FXA Error: " + msg + "\n");

"Firefox Account" where it might appear in logs that mere mortals see?

And let's use console.log(), so at least this error is visible to users who aren't running Firefox from a command line.

@@ +27,5 @@
> +
> +    try {
> +      iframe.src = fxAccounts.getAccountsURI();
> +    } catch (e) {
> +      error(e.message);

error("Couldn't init Firefox Account wrapper: " + e.message);

@@ +80,5 @@
>    },
>  
>    injectData: function (type, content) {
> +    try {
> +      let authUrl = fxAccounts.getAccountsURI();

More minimal change:

  let authUrl;
  try {
    authUrl = fxAccounts.getAccountsURI();
  } catch (e) {
    error("Couldn't inject data: " + e.message);
    return;
  }
  ...

Comment 7

[Zachary Carter [:zaach]]

https://hg.mozilla.org/projects/elm/rev/e48bc467fc21

Comment 8

[James Bonacci [:jbonacci]]

OK great. When this appears in ELM, give me a ping.
QA will want to test this.

Comment 9

[James Bonacci [:jbonacci]]

Also, I am going to assume this is desktop only...

Comment 10

[James Bonacci [:jbonacci]]

This is on my list now for testing.
See also: https://github.com/mozilla/firefox-account-bridge/issues/48

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e48bc467fc21

Comment 12

[James Bonacci [:jbonacci]]

Cleaning up Resolved/Fixed bugs from December's first release.
Verified that we now have a working first-release of FxA to Desktop/Android Nightly.
Re-open as needed.