Merge pull request #1627 from warner/escape

improve parameter-escaping

resources/static/dialog/controllers/actions.js

@@ -56,20 +56,6 @@ BrowserID.Modules.Actions = (function() {
       if(data.ready) _.defer(data.ready);
     },
 
-    /**
-     * Show an error message
-     * @method doError
-     * @param {string} [template] - template to use, if not given, use "error"
-     * @param {object} [info] - info to send to template
-     */
-    doError: function(template, info) {
-      if(!info) {
-        info = template;
-        template = "error";
-      }
-      this.renderError(template, info);
-    },
-
     doCancel: function() {
       if(onsuccess) onsuccess(null);
     },

resources/static/dialog/controllers/dialog.js

@@ -13,6 +13,7 @@ BrowserID.Modules.Dialog = (function() {
       errors = bid.Errors,
       dom = bid.DOM,
       win = window,
+      startExternalDependencies = true,
       channel,
       sc;
 
@@ -83,9 +84,12 @@ BrowserID.Modules.Dialog = (function() {
 
   function fixupURL(origin, url) {
     var u;
-    if (/^http/.test(url)) u = URLParse(url);
+    if (typeof(url) !== "string")
+      throw "urls must be strings: (" + url + ")";
+    if (/^http(s)?:\/\//.test(url)) u = URLParse(url);
     else if (/^\//.test(url)) u = URLParse(origin + url);
     else throw "relative urls not allowed: (" + url + ")";
+    // encodeURI limits our return value to [a-z0-9:/?%], excluding <script>
     return encodeURI(u.validate().normalize().toString());
   }
 
@@ -97,8 +101,23 @@ BrowserID.Modules.Dialog = (function() {
 
       win = options.window || window;
 
+      // startExternalDependencies is used in unit testing and can only be set
+      // by the creator/starter of this module.  If startExternalDependencies
+      // is set to false, the channel, state machine, and actions controller
+      // are not started.  These dependencies can interfere with the ability to
+      // unit test this module because they can throw exceptions and show error
+      // messages.
+      startExternalDependencies = true;
+      if (typeof options.startExternalDependencies === "boolean") {
+        startExternalDependencies = options.startExternalDependencies;
+      }
+
       sc.start.call(self, options);
-      startChannel.call(self);
+
+      if (startExternalDependencies) {
+        startChannel.call(self);
+      }
+
       options.ready && _.defer(options.ready);
     },
 
@@ -111,32 +130,51 @@ BrowserID.Modules.Dialog = (function() {
       return this.get(origin_url, {}, success, error);
     },
 
-    get: function(origin_url, params, success, error) {
+    get: function(origin_url, paramsFromRP, success, error) {
       var self=this,
           hash = win.location.hash;
 
       setOrigin(origin_url);
 
-      var actions = startActions.call(self, success, error);
-      startStateMachine.call(self, actions);
 
-      params = params || {};
+      if (startExternalDependencies) {
+        var actions = startActions.call(self, success, error);
+        startStateMachine.call(self, actions);
+      }
+
+      // Security Note: paramsFromRP is the output of a JSON.parse on an
+      // RP-controlled string. Most of these fields are expected to be simple
+      // printable strings (hostnames, usernames, and URLs), but we cannot
+      // rely upon the RP to do that. In particular we must guard against
+      // these strings containing <script> tags. We will populate a new
+      // object ("params") with suitably type-checked properties.
+      var params = {};
       params.hostname = user.getHostname();
 
       // verify params
-      if (params.tosURL && params.privacyURL) {
-        try {
-          params.tosURL = fixupURL(origin_url, params.tosURL);
-          params.privacyURL = fixupURL(origin_url, params.privacyURL);
-        } catch(e) {
-          return self.renderError("error", {
-            action: {
-              title: "error in " + origin_url,
-              message: "improper usage of API: " + e
-            }
-          });
+      try {
+        if (paramsFromRP.requiredEmail) {
+          if (!bid.verifyEmail(paramsFromRP.requiredEmail))
+            throw "invalid requiredEmail: (" + paramsFromRP.requiredEmail + ")";
+          params.requiredEmail = paramsFromRP.requiredEmail;
+        }
+        if (paramsFromRP.tosURL && paramsFromRP.privacyURL) {
+          params.tosURL = fixupURL(origin_url, paramsFromRP.tosURL);
+          params.privacyURL = fixupURL(origin_url, paramsFromRP.privacyURL);
         }
+      } catch(e) {
+        // note: renderError accepts HTML and cheerfully injects it into a
+        // frame with a powerful origin. So convert 'e' first.
+        self.renderError("error", {
+          action: {
+            title: "error in " + _.escape(origin_url),
+            message: "improper usage of API: " + _.escape(e)
+          }
+        });
+
+        return e;
       }
+      // after this point, "params" can be relied upon to contain safe data
 
       // XXX Perhaps put this into the state machine.
       self.bind(win, "unload", onWindowUnload);

resources/static/dialog/resources/state.js

@@ -46,11 +46,7 @@ BrowserID.State = (function() {
       self.tosURL = info.tosURL;
       requiredEmail = info.requiredEmail;
 
-      if ((typeof(requiredEmail) !== "undefined") && (!bid.verifyEmail(requiredEmail))) {
-        // Invalid format
-        startAction("doError", "invalid_required_email", {email: requiredEmail});
-      }
-      else if (info.email && info.type === "primary") {
+      if (info.email && info.type === "primary") {
         primaryVerificationInfo = info;
         redirectToState("primary_user", info);
       }