Bug 1517044 - Don't allow nsIWritablePropertyBag calls to overwrite nsIUpdate and nsIUpdatePatch attributes. r=mhowell
authorRobert Strong <robert.bugzilla@gmail.com>
Thu, 10 Jan 2019 15:18:14 -0800
changeset 510476 df91f6e79753b73b9bd8deb54d25eb8b4da65629
parent 510475 e7ad1e4500a9387142de84ea9235264530d36e1b
child 510477 0d7e7cd19c7f34a26eb018995ace70a5be750b12
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmhowell
bugs1517044
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1517044 - Don't allow nsIWritablePropertyBag calls to overwrite nsIUpdate and nsIUpdatePatch attributes. r=mhowell
toolkit/mozapps/update/nsUpdateService.js
toolkit/mozapps/update/tests/unit_aus_update/updateManagerXML.js
--- a/toolkit/mozapps/update/nsUpdateService.js
+++ b/toolkit/mozapps/update/nsUpdateService.js
@@ -1228,23 +1228,31 @@ function UpdatePatch(patch) {
         break;
       case "finalURL":
       case "state":
       case "type":
       case "URL":
         this[attr.name] = attr.value;
         break;
       default:
-        // Set nsIPropertyBag properties that were read from the xml file.
-        this.setProperty(attr.name, attr.value);
+        if (!this._attrNames.includes(attr.name)) {
+          // Set nsIPropertyBag properties that were read from the xml file.
+          this.setProperty(attr.name, attr.value);
+        }
         break;
     }
   }
 }
 UpdatePatch.prototype = {
+  // nsIUpdatePatch attribute names used to prevent nsIWritablePropertyBag from
+  // over writing nsIUpdatePatch attributes.
+  _attrNames: [
+    "errorCode", "finalURL", "selected", "size", "state", "type", "URL",
+  ],
+
   /**
    * See nsIUpdateService.idl
    */
   serialize: function UpdatePatch_serialize(updates) {
     var patch = updates.createElementNS(URI_UPDATE_NS, "patch");
     patch.setAttribute("size", this.size);
     patch.setAttribute("type", this.type);
     patch.setAttribute("URL", this.URL);
@@ -1261,34 +1269,46 @@ UpdatePatch.prototype = {
     if (this.selected) {
       patch.setAttribute("selected", this.selected);
     }
     if (this.state != STATE_NONE) {
       patch.setAttribute("state", this.state);
     }
 
     for (let [name, value] of Object.entries(this._properties)) {
-      if (value.present) {
+      if (value.present && !this._attrNames.includes(name)) {
         patch.setAttribute(name, value.data);
       }
     }
     return patch;
   },
 
   /**
    * See nsIWritablePropertyBag.idl
    */
   setProperty: function UpdatePatch_setProperty(name, value) {
+    if (this._attrNames.includes(name)) {
+      throw Components.Exception(
+        "Illegal value '" + name + "' (attribute exists on nsIUpdatePatch) " +
+        "when calling method: [nsIWritablePropertyBag::setProperty]",
+        Cr.NS_ERROR_ILLEGAL_VALUE);
+    }
     this._properties[name] = { data: value, present: true };
   },
 
   /**
    * See nsIWritablePropertyBag.idl
    */
   deleteProperty: function UpdatePatch_deleteProperty(name) {
+    if (this._attrNames.includes(name)) {
+      throw Components.Exception(
+        "Illegal value '" + name + "' (attribute exists on nsIUpdatePatch) " +
+        "when calling method: [nsIWritablePropertyBag::deleteProperty]",
+        Cr.NS_ERROR_ILLEGAL_VALUE);
+    }
     if (name in this._properties) {
       this._properties[name].present = false;
     } else {
       throw Cr.NS_ERROR_FAILURE;
     }
   },
 
   /**
@@ -1303,17 +1323,17 @@ UpdatePatch.prototype = {
 
   * enumerate() {
     // An nsISupportsInterfacePointer is used so creating an array using
     // Array.from will retain the QueryInterface for nsIProperty.
     let ip = Cc["@mozilla.org/supports-interface-pointer;1"].
              createInstance(Ci.nsISupportsInterfacePointer);
     let qi = ChromeUtils.generateQI([Ci.nsIProperty]);
     for (let [name, value] of Object.entries(this._properties)) {
-      if (value.present) {
+      if (value.present && !this._attrNames.includes(name)) {
         // The nsIPropertyBag enumerator returns a nsISimpleEnumerator whose
         // elements are nsIProperty objects. Calling QueryInterface for
         // nsIProperty on the object doesn't return to the caller an object that
         // is already queried to nsIProperty but do it just in case it is fixed
         // at some point.
         ip.data = {name, value: value.data, QueryInterface: qi};
         yield ip.data.QueryInterface(Ci.nsIProperty);
       }
@@ -1322,16 +1342,22 @@ UpdatePatch.prototype = {
 
   /**
    * See nsIPropertyBag.idl
    *
    * Note: returns null instead of throwing when the property doesn't exist to
    *       simplify code and to silence warnings in debug builds.
    */
   getProperty: function UpdatePatch_getProperty(name) {
+    if (this._attrNames.includes(name)) {
+      throw Components.Exception(
+        "Illegal value '" + name + "' (attribute exists on nsIUpdatePatch) " +
+        "when calling method: [nsIWritablePropertyBag::getProperty]",
+        Cr.NS_ERROR_ILLEGAL_VALUE);
+    }
     if (name in this._properties && this._properties[name].present) {
       return this._properties[name].data;
     }
     return null;
   },
 
   QueryInterface: ChromeUtils.generateQI([Ci.nsIUpdatePatch,
                                           Ci.nsIPropertyBag,
@@ -1426,18 +1452,20 @@ function Update(update) {
         case "name":
         case "previousAppVersion":
         case "serviceURL":
         case "statusText":
         case "type":
           this[attr.name] = attr.value;
           break;
         default:
-          // Set nsIPropertyBag properties that were read from the xml file.
-          this.setProperty(attr.name, attr.value);
+          if (!this._attrNames.includes(attr.name)) {
+            // Set nsIPropertyBag properties that were read from the xml file.
+            this.setProperty(attr.name, attr.value);
+          }
           break;
       }
     }
   }
 
   if (!this.previousAppVersion) {
     this.previousAppVersion = Services.appinfo.version;
   }
@@ -1465,16 +1493,25 @@ function Update(update) {
     // "<App Name> <Update App Version>"
     let brandBundle = Services.strings.createBundle(URI_BRAND_PROPERTIES);
     let appName = brandBundle.GetStringFromName("brandShortName");
     this.name = gUpdateBundle.formatStringFromName("updateName",
                                                    [appName, this.displayVersion], 2);
   }
 }
 Update.prototype = {
+  // nsIUpdate attribute names used to prevent nsIWritablePropertyBag from over
+  // writing nsIUpdate attributes.
+  _attrNames: [
+    "appVersion", "buildID", "channel", "detailsURL", "displayVersion",
+    "elevationFailure", "errorCode", "installDate", "isCompleteUpdate", "name",
+    "previousAppVersion", "promptWaitTime", "serviceURL", "state", "statusText",
+    "type", "unsupported",
+  ],
+
   /**
    * See nsIUpdateService.idl
    */
   getPatchAt: function Update_getPatchAt(index) {
     return this._patches[index];
   },
 
   /**
@@ -1558,40 +1595,52 @@ Update.prototype = {
     if (this.unsupported) {
       update.setAttribute("unsupported", this.unsupported);
     }
     if (this.elevationFailure) {
       update.setAttribute("elevationFailure", this.elevationFailure);
     }
 
     for (let [name, value] of Object.entries(this._properties)) {
-      if (value.present) {
+      if (value.present && !this._attrNames.includes(name)) {
         update.setAttribute(name, value.data);
       }
     }
 
     for (let i = 0; i < this.patchCount; ++i) {
       update.appendChild(this.getPatchAt(i).serialize(updates));
     }
 
     updates.documentElement.appendChild(update);
     return update;
   },
 
   /**
    * See nsIWritablePropertyBag.idl
    */
   setProperty: function Update_setProperty(name, value) {
+    if (this._attrNames.includes(name)) {
+      throw Components.Exception(
+        "Illegal value '" + name + "' (attribute exists on nsIUpdate) " +
+        "when calling method: [nsIWritablePropertyBag::setProperty]",
+        Cr.NS_ERROR_ILLEGAL_VALUE);
+    }
     this._properties[name] = { data: value, present: true };
   },
 
   /**
    * See nsIWritablePropertyBag.idl
    */
   deleteProperty: function Update_deleteProperty(name) {
+    if (this._attrNames.includes(name)) {
+      throw Components.Exception(
+        "Illegal value '" + name + "' (attribute exists on nsIUpdate) " +
+        "when calling method: [nsIWritablePropertyBag::deleteProperty]",
+        Cr.NS_ERROR_ILLEGAL_VALUE);
+    }
     if (name in this._properties) {
       this._properties[name].present = false;
     } else {
       throw Cr.NS_ERROR_FAILURE;
     }
   },
 
   /**
@@ -1606,17 +1655,17 @@ Update.prototype = {
 
   * enumerate() {
     // An nsISupportsInterfacePointer is used so creating an array using
     // Array.from will retain the QueryInterface for nsIProperty.
     let ip = Cc["@mozilla.org/supports-interface-pointer;1"].
              createInstance(Ci.nsISupportsInterfacePointer);
     let qi = ChromeUtils.generateQI([Ci.nsIProperty]);
     for (let [name, value] of Object.entries(this._properties)) {
-      if (value.present) {
+      if (value.present && !this._attrNames.includes(name)) {
         // The nsIPropertyBag enumerator returns a nsISimpleEnumerator whose
         // elements are nsIProperty objects. Calling QueryInterface for
         // nsIProperty on the object doesn't return to the caller an object that
         // is already queried to nsIProperty but do it just in case it is fixed
         // at some point.
         ip.data = {name, value: value.data, QueryInterface: qi};
         yield ip.data.QueryInterface(Ci.nsIProperty);
       }
@@ -1624,16 +1673,22 @@ Update.prototype = {
   },
 
   /**
    * See nsIPropertyBag.idl
    * Note: returns null instead of throwing when the property doesn't exist to
    *       simplify code and to silence warnings in debug builds.
    */
   getProperty: function Update_getProperty(name) {
+    if (this._attrNames.includes(name)) {
+      throw Components.Exception(
+        "Illegal value '" + name + "' (attribute exists on nsIUpdate) " +
+        "when calling method: [nsIWritablePropertyBag::getProperty]",
+        Cr.NS_ERROR_ILLEGAL_VALUE);
+    }
     if (name in this._properties && this._properties[name].present) {
       return this._properties[name].data;
     }
     return null;
   },
 
   QueryInterface: ChromeUtils.generateQI([Ci.nsIUpdate,
                                           Ci.nsIPropertyBag,
--- a/toolkit/mozapps/update/tests/unit_aus_update/updateManagerXML.js
+++ b/toolkit/mozapps/update/tests/unit_aus_update/updateManagerXML.js
@@ -256,10 +256,61 @@ function run_test() {
                "the first property name" + MSG_SHOULD_EQUAL);
   Assert.equal(results[0].value, "custom3 patch value",
                "the first property value" + MSG_SHOULD_EQUAL);
   Assert.equal(results[1].name, "custom4_attr",
                "the second property name" + MSG_SHOULD_EQUAL);
   Assert.equal(results[1].value, "custom4 patch value",
                "the second property value" + MSG_SHOULD_EQUAL);
 
+  let attrNames = [
+    "appVersion", "buildID", "channel", "detailsURL", "displayVersion",
+    "elevationFailure", "errorCode", "installDate", "isCompleteUpdate", "name",
+    "previousAppVersion", "promptWaitTime", "serviceURL", "state", "statusText",
+    "type", "unsupported",
+  ];
+  checkIllegalProperties(update, attrNames);
+
+  attrNames = [
+    "errorCode", "finalURL", "selected", "size", "state", "type", "URL",
+  ];
+  checkIllegalProperties(patch, attrNames);
+
   executeSoon(doTestFinish);
 }
+
+function checkIllegalProperties(object, propertyNames) {
+  let objectName =
+    object instanceof Ci.nsIUpdate ? "nsIUpdate" : "nsIUpdatePatch";
+  propertyNames.forEach(function(name) {
+    // Check that calling getProperty, setProperty, and deleteProperty on an
+    // nsIUpdate attribute throws NS_ERROR_ILLEGAL_VALUE
+    let result = 0;
+    try {
+      object.getProperty(name);
+    } catch (e) {
+      result = e.result;
+    }
+    Assert.equal(result, Cr.NS_ERROR_ILLEGAL_VALUE,
+                 "calling getProperty using an " + objectName + " attribute " +
+                 "name should throw NS_ERROR_ILLEGAL_VALUE");
+
+    result = 0;
+    try {
+      object.setProperty(name, "value");
+    } catch (e) {
+      result = e.result;
+    }
+    Assert.equal(result, Cr.NS_ERROR_ILLEGAL_VALUE,
+                 "calling setProperty using an " + objectName + " attribute " +
+                 "name should throw NS_ERROR_ILLEGAL_VALUE");
+
+    result = 0;
+    try {
+      object.deleteProperty(name);
+    } catch (e) {
+      result = e.result;
+    }
+    Assert.equal(result, Cr.NS_ERROR_ILLEGAL_VALUE,
+                 "calling deleteProperty using an " + objectName + " attribute " +
+                 "name should throw NS_ERROR_ILLEGAL_VALUE");
+  });
+}