Bug 1052247 - Enforce that OAuth is done over HTTPS in FxAccountsOAuthClient. r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Wed, 04 Oct 2017 16:22:45 -0400
changeset 675634 7996f97ce2379b5fb664d461a53be4a3122640bc
parent 675107 69ce595533cda259186ac7469c54256839ca3762
child 734664 4fb09b804a9c2ed0e5eb97c4bec9fc2b3c738b59
push id83194
push userbmo:tchiovoloni@mozilla.com
push dateThu, 05 Oct 2017 17:12:39 +0000
reviewersmarkh
bugs1052247
milestone58.0a1
Bug 1052247 - Enforce that OAuth is done over HTTPS in FxAccountsOAuthClient. r?markh It now follows the setting of the identity.fxaccounts.allowHttp preference. MozReview-Commit-ID: 9646Xi48QMP
services/fxaccounts/FxAccounts.jsm
services/fxaccounts/FxAccountsOAuthGrantClient.jsm
services/fxaccounts/tests/xpcshell/test_accounts.js
services/fxaccounts/tests/xpcshell/test_oauth_grant_client.js
services/fxaccounts/tests/xpcshell/test_oauth_grant_client_server.js
--- a/services/fxaccounts/FxAccounts.jsm
+++ b/services/fxaccounts/FxAccounts.jsm
@@ -1286,16 +1286,17 @@ FxAccountsInternal.prototype = {
   },
 
   _rejectWhenVerified(currentState, error) {
     currentState.whenVerifiedDeferred.reject(error);
     delete currentState.whenVerifiedDeferred;
   },
 
   requiresHttps() {
+    // Also used in FxAccountsOAuthGrantClient.jsm.
     let allowHttp = Services.prefs.getBoolPref("identity.fxaccounts.allowHttp", false);
     return allowHttp !== true;
   },
 
   promiseAccountsSignUpURI() {
     return FxAccountsConfig.promiseAccountsSignUpURI();
   },
 
--- a/services/fxaccounts/FxAccountsOAuthGrantClient.jsm
+++ b/services/fxaccounts/FxAccountsOAuthGrantClient.jsm
@@ -10,21 +10,24 @@
 
 this.EXPORTED_SYMBOLS = ["FxAccountsOAuthGrantClient", "FxAccountsOAuthGrantClientError"];
 
 const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://gre/modules/FxAccountsCommon.js");
 Cu.import("resource://services-common/rest.js");
+Cu.import("resource://gre/modules/Services.jsm");
 
 Cu.importGlobalProperties(["URL"]);
 
 const AUTH_ENDPOINT = "/authorization";
 const DESTROY_ENDPOINT = "/destroy";
+// This is the same pref that's used by FxAccounts.jsm.
+const ALLOW_HTTP_PREF = "identity.fxaccounts.allowHttp";
 
 /**
  * Create a new FxAccountsOAuthClient for browser some service.
  *
  * @param {Object} options Options
  *   @param {Object} options.parameters
  *     @param {String} options.parameters.client_id
  *     OAuth id returned from client registration
@@ -40,16 +43,21 @@ this.FxAccountsOAuthGrantClient = functi
   this.parameters = options;
 
   try {
     this.serverURL = new URL(this.parameters.serverURL);
   } catch (e) {
     throw new Error("Invalid 'serverURL'");
   }
 
+  let forceHTTPS = !Services.prefs.getBoolPref(ALLOW_HTTP_PREF, false);
+  if (forceHTTPS && this.serverURL.protocol != "https:") {
+    throw new Error("'serverURL' must be HTTPS");
+  }
+
   log.debug("FxAccountsOAuthGrantClient Initialized");
 };
 
 this.FxAccountsOAuthGrantClient.prototype = {
 
   /**
    * Retrieves an OAuth access token for the signed in user
    *
--- a/services/fxaccounts/tests/xpcshell/test_accounts.js
+++ b/services/fxaccounts/tests/xpcshell/test_accounts.js
@@ -1192,17 +1192,17 @@ add_test(function test_getOAuthToken() {
   let alice = getTestUser("alice");
   alice.verified = true;
   let getTokenFromAssertionCalled = false;
 
   fxa.internal._d_signCertificate.resolve("cert1");
 
   // create a mock oauth client
   let client = new FxAccountsOAuthGrantClient({
-    serverURL: "http://example.com/v1",
+    serverURL: "https://example.com/v1",
     client_id: "abc123"
   });
   client.getTokenFromAssertion = function() {
     getTokenFromAssertionCalled = true;
     return Promise.resolve({ access_token: "token" });
   };
 
   fxa.setSignedInUser(alice).then(
@@ -1224,17 +1224,17 @@ add_test(function test_getOAuthTokenScop
   let alice = getTestUser("alice");
   alice.verified = true;
   let getTokenFromAssertionCalled = false;
 
   fxa.internal._d_signCertificate.resolve("cert1");
 
   // create a mock oauth client
   let client = new FxAccountsOAuthGrantClient({
-    serverURL: "http://example.com/v1",
+    serverURL: "https://example.com/v1",
     client_id: "abc123"
   });
   client.getTokenFromAssertion = function(assertion, scopeString) {
     equal(scopeString, "foo bar");
     getTokenFromAssertionCalled = true;
     return Promise.resolve({ access_token: "token" });
   };
 
@@ -1257,17 +1257,17 @@ add_task(async function test_getOAuthTok
   let alice = getTestUser("alice");
   alice.verified = true;
   let numTokenFromAssertionCalls = 0;
 
   fxa.internal._d_signCertificate.resolve("cert1");
 
   // create a mock oauth client
   let client = new FxAccountsOAuthGrantClient({
-    serverURL: "http://example.com/v1",
+    serverURL: "https://example.com/v1",
     client_id: "abc123"
   });
   client.getTokenFromAssertion = function() {
     numTokenFromAssertionCalls += 1;
     return Promise.resolve({ access_token: "token" });
   };
 
   await fxa.setSignedInUser(alice);
@@ -1290,17 +1290,17 @@ add_task(async function test_getOAuthTok
   let alice = getTestUser("alice");
   alice.verified = true;
   let numTokenFromAssertionCalls = 0;
 
   fxa.internal._d_signCertificate.resolve("cert1");
 
   // create a mock oauth client
   let client = new FxAccountsOAuthGrantClient({
-    serverURL: "http://example.com/v1",
+    serverURL: "https://example.com/v1",
     client_id: "abc123"
   });
   client.getTokenFromAssertion = function() {
     numTokenFromAssertionCalls += 1;
     return Promise.resolve({ access_token: "token" });
   };
 
   await fxa.setSignedInUser(alice);
@@ -1388,17 +1388,17 @@ add_test(function test_getOAuthToken_net
   let fxa = new MockFxAccounts();
   let alice = getTestUser("alice");
   alice.verified = true;
 
   fxa.internal._d_signCertificate.resolve("cert1");
 
   // create a mock oauth client
   let client = new FxAccountsOAuthGrantClient({
-    serverURL: "http://example.com/v1",
+    serverURL: "https://example.com/v1",
     client_id: "abc123"
   });
   client.getTokenFromAssertion = function() {
     return Promise.reject(new FxAccountsOAuthGrantClientError({
       error: ERROR_NETWORK,
       errno: ERRNO_NETWORK
     }));
   };
@@ -1417,17 +1417,17 @@ add_test(function test_getOAuthToken_aut
   let fxa = new MockFxAccounts();
   let alice = getTestUser("alice");
   alice.verified = true;
 
   fxa.internal._d_signCertificate.resolve("cert1");
 
   // create a mock oauth client
   let client = new FxAccountsOAuthGrantClient({
-    serverURL: "http://example.com/v1",
+    serverURL: "https://example.com/v1",
     client_id: "abc123"
   });
   client.getTokenFromAssertion = function() {
     return Promise.reject(new FxAccountsOAuthGrantClientError({
       error: ERROR_INVALID_FXA_ASSERTION,
       errno: ERRNO_INVALID_FXA_ASSERTION
     }));
   };
@@ -1446,17 +1446,17 @@ add_test(function test_getOAuthToken_unk
   let fxa = new MockFxAccounts();
   let alice = getTestUser("alice");
   alice.verified = true;
 
   fxa.internal._d_signCertificate.resolve("cert1");
 
   // create a mock oauth client
   let client = new FxAccountsOAuthGrantClient({
-    serverURL: "http://example.com/v1",
+    serverURL: "https://example.com/v1",
     client_id: "abc123"
   });
   client.getTokenFromAssertion = function() {
     return Promise.reject("boom");
   };
 
   fxa.setSignedInUser(alice).then(() => {
     fxa.getOAuthToken({ scope: "profile", client })
--- a/services/fxaccounts/tests/xpcshell/test_oauth_grant_client.js
+++ b/services/fxaccounts/tests/xpcshell/test_oauth_grant_client.js
@@ -2,17 +2,17 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 Cu.import("resource://gre/modules/FxAccountsCommon.js");
 Cu.import("resource://gre/modules/FxAccountsOAuthGrantClient.jsm");
 
 const CLIENT_OPTIONS = {
-  serverURL: "http://127.0.0.1:9010/v1",
+  serverURL: "https://127.0.0.1:9010/v1",
   client_id: "abc123"
 };
 
 const STATUS_SUCCESS = 200;
 
 /**
  * Mock request responder
  * @param {String} response
@@ -131,17 +131,17 @@ add_test(function serverErrorResponse() 
       do_check_eq(e.error, "Bad Request");
       do_check_eq(e.message, "Unauthorized");
       run_next_test();
     });
 });
 
 add_test(function networkErrorResponse() {
   let client = new FxAccountsOAuthGrantClient({
-    serverURL: "http://domain.dummy",
+    serverURL: "https://domain.dummy",
     client_id: "abc123"
   });
   Services.prefs.setBoolPref("identity.fxaccounts.skipDeviceRegistration", true);
   client.getTokenFromAssertion("assertion", "scope")
     .catch(function(e) {
       do_check_eq(e.name, "FxAccountsOAuthGrantClientError");
       do_check_eq(e.code, null);
       do_check_eq(e.errno, ERRNO_NETWORK);
@@ -200,24 +200,36 @@ add_test(function incorrectErrno() {
 
 add_test(function constructorTests() {
   validationHelper(undefined,
     "Error: Missing configuration options");
 
   validationHelper({},
     "Error: Missing 'serverURL' parameter");
 
-  validationHelper({ serverURL: "http://example.com" },
+  validationHelper({ serverURL: "https://example.com" },
+    "Error: Missing 'client_id' parameter");
+
+  validationHelper({ serverURL: "https://example.com" },
     "Error: Missing 'client_id' parameter");
 
   validationHelper({ client_id: "123ABC" },
     "Error: Missing 'serverURL' parameter");
 
-  validationHelper({ client_id: "123ABC", serverURL: "badUrl" },
-    "Error: Invalid 'serverURL'");
+  validationHelper({ client_id: "123ABC", serverURL: "http://example.com" },
+    "Error: 'serverURL' must be HTTPS");
+
+  try {
+    Services.prefs.setBoolPref("identity.fxaccounts.allowHttp", true);
+    validationHelper({ client_id: "123ABC", serverURL: "http://example.com" }, null);
+  } finally {
+    Services.prefs.clearUserPref("identity.fxaccounts.allowHttp");
+  }
+
+
 
   run_next_test();
 });
 
 add_test(function errorTests() {
   let error1 = new FxAccountsOAuthGrantClientError();
   do_check_eq(error1.name, "FxAccountsOAuthGrantClientError");
   do_check_eq(error1.code, null);
@@ -245,25 +257,44 @@ add_test(function errorTests() {
   do_check_eq(fields2.errno, statusCode);
   do_check_eq(fields2.error, "Error");
   do_check_eq(fields2.message, "Something");
 
   do_check_true(error2.toString().indexOf("Something") >= 0);
   run_next_test();
 });
 
+
+add_test(function networkErrorResponse() {
+  let client = new FxAccountsOAuthGrantClient({
+    serverURL: "https://domain.dummy",
+    client_id: "abc123"
+  });
+  Services.prefs.setBoolPref("identity.fxaccounts.skipDeviceRegistration", true);
+  client.getTokenFromAssertion("assertion", "scope")
+    .catch(function(e) {
+      do_check_eq(e.name, "FxAccountsOAuthGrantClientError");
+      do_check_eq(e.code, null);
+      do_check_eq(e.errno, ERRNO_NETWORK);
+      do_check_eq(e.error, ERROR_NETWORK);
+      run_next_test();
+    }).catch(() => {}).then(() =>
+      Services.prefs.clearUserPref("identity.fxaccounts.skipDeviceRegistration"));
+});
+
+
 /**
  * Quick way to test the "FxAccountsOAuthGrantClient" constructor.
  *
  * @param {Object} options
  *        FxAccountsOAuthGrantClient constructor options
  * @param {String} expected
- *        Expected error message
+ *        Expected error message, or null if it's expected to pass.
  * @returns {*}
  */
 function validationHelper(options, expected) {
   try {
     new FxAccountsOAuthGrantClient(options);
   } catch (e) {
     return do_check_eq(e.toString(), expected);
   }
-  throw new Error("Validation helper error");
+  return do_check_eq(expected, null);
 }
--- a/services/fxaccounts/tests/xpcshell/test_oauth_grant_client_server.js
+++ b/services/fxaccounts/tests/xpcshell/test_oauth_grant_client_server.js
@@ -2,16 +2,17 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 // A test of FxAccountsOAuthGrantClient but using a real server it can
 // hit.
 "use strict";
 
 Cu.import("resource://gre/modules/FxAccountsCommon.js");
 Cu.import("resource://gre/modules/FxAccountsOAuthGrantClient.jsm");
+Cu.import("resource://gre/modules/Services.jsm");
 
 // handlers for our server.
 var numTokenFetches;
 var activeTokens;
 
 function authorize(request, response) {
   response.setStatusLine("1.1", 200, "OK");
   let token = "token" + numTokenFetches;
@@ -46,24 +47,29 @@ function startServer() {
 
 function promiseStopServer(server) {
   return new Promise(resolve => {
     server.stop(resolve);
   });
 }
 
 add_task(async function getAndRevokeToken() {
+  Services.prefs.setBoolPref("identity.fxaccounts.allowHttp", true);
   let server = startServer();
-  let clientOptions = {
-    serverURL: "http://localhost:" + server.identity.primaryPort + "/v1",
-    client_id: "abc123",
-  }
+  try {
+    let clientOptions = {
+      serverURL: "http://localhost:" + server.identity.primaryPort + "/v1",
+      client_id: "abc123",
+    }
 
-  let client = new FxAccountsOAuthGrantClient(clientOptions);
-  let result = await client.getTokenFromAssertion("assertion", "scope");
-  equal(result.access_token, "token0");
-  equal(numTokenFetches, 1, "we hit the server to fetch a token");
-  await client.destroyToken("token0");
-  equal(activeTokens.size, 0, "We hit the server to revoke it");
-  await promiseStopServer(server);
+    let client = new FxAccountsOAuthGrantClient(clientOptions);
+    let result = await client.getTokenFromAssertion("assertion", "scope");
+    equal(result.access_token, "token0");
+    equal(numTokenFetches, 1, "we hit the server to fetch a token");
+    await client.destroyToken("token0");
+    equal(activeTokens.size, 0, "We hit the server to revoke it");
+  } finally {
+    await promiseStopServer(server);
+    Services.prefs.clearUserPref("identity.fxaccounts.allowHttp");
+  }
 });
 
 // XXX - TODO - we should probably add more tests for unexpected responses etc.