Bug 1321299. Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined. r=bholley, a=gchang
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 02 Dec 2016 15:24:20 -0500
changeset 452518 ddcf602cb70753ecbd965a67ad50fcd41b55639e
parent 452517 181d381af5dfff2cafcd816be6167a1897d963a0
child 452519 b54862bc31222c532db94f38e19c990f7806fa18
push id39418
push userbmo:twointofive@gmail.com
push dateWed, 21 Dec 2016 20:59:30 +0000
reviewersbholley, gchang
bugs1321299
milestone51.0
Bug 1321299. Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined. r=bholley, a=gchang
js/xpconnect/wrappers/AccessCheck.cpp
js/xpconnect/wrappers/FilteringWrapper.cpp
js/xpconnect/wrappers/FilteringWrapper.h
testing/web-platform/tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.sub.html
--- a/js/xpconnect/wrappers/AccessCheck.cpp
+++ b/js/xpconnect/wrappers/AccessCheck.cpp
@@ -6,16 +6,17 @@
 
 #include "AccessCheck.h"
 
 #include "nsJSPrincipals.h"
 #include "nsGlobalWindow.h"
 
 #include "XPCWrapper.h"
 #include "XrayWrapper.h"
+#include "FilteringWrapper.h"
 
 #include "jsfriendapi.h"
 #include "mozilla/dom/BindingUtils.h"
 #include "mozilla/dom/LocationBinding.h"
 #include "mozilla/dom/WindowBinding.h"
 #include "mozilla/jsipc/CrossProcessObjectWrappers.h"
 #include "nsIDOMWindowCollection.h"
 #include "nsJSUtils.h"
@@ -172,16 +173,22 @@ AccessCheck::isCrossOriginAccessPermitte
                isCrossOriginAccessPermitted(cx, wrapper, id, Wrapper::SET);
     }
 
     RootedObject obj(cx, js::UncheckedUnwrap(wrapper, /* stopAtWindowProxy = */ false));
     CrossOriginObjectType type = IdentifyCrossOriginObject(obj);
     if (JSID_IS_STRING(id)) {
         if (IsPermitted(type, JSID_TO_FLAT_STRING(id), act == Wrapper::SET))
             return true;
+    } else if (type != CrossOriginOpaque &&
+               IsCrossOriginWhitelistedSymbol(cx, id)) {
+        // We always allow access to @@toStringTag, @@hasInstance, and
+        // @@isConcatSpreadable.  But then we nerf them to be a value descriptor
+        // with value undefined in CrossOriginXrayWrapper.
+        return true;
     }
 
     if (act != Wrapper::GET)
         return false;
 
     // Check for frame IDs. If we're resolving named frames, make sure to only
     // resolve ones that don't shadow native properties. See bug 860494.
     if (type == CrossOriginWindow) {
--- a/js/xpconnect/wrappers/FilteringWrapper.cpp
+++ b/js/xpconnect/wrappers/FilteringWrapper.cpp
@@ -11,16 +11,39 @@
 
 #include "jsapi.h"
 
 using namespace JS;
 using namespace js;
 
 namespace xpc {
 
+static JS::SymbolCode sCrossOriginWhitelistedSymbolCodes[] = {
+    JS::SymbolCode::toStringTag,
+    JS::SymbolCode::hasInstance,
+    JS::SymbolCode::isConcatSpreadable
+};
+
+bool
+IsCrossOriginWhitelistedSymbol(JSContext* cx, JS::HandleId id)
+{
+    if (!JSID_IS_SYMBOL(id)) {
+        return false;
+    }
+
+    JS::Symbol* symbol = JSID_TO_SYMBOL(id);
+    for (auto code : sCrossOriginWhitelistedSymbolCodes) {
+        if (symbol == JS::GetWellKnownSymbol(cx, code)) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 template <typename Policy>
 static bool
 Filter(JSContext* cx, HandleObject wrapper, AutoIdVector& props)
 {
     size_t w = 0;
     RootedId id(cx);
     for (size_t n = 0; n < props.length(); ++n) {
         id = props[n];
@@ -183,26 +206,41 @@ bool
 CrossOriginXrayWrapper::getPropertyDescriptor(JSContext* cx,
                                               JS::Handle<JSObject*> wrapper,
                                               JS::Handle<jsid> id,
                                               JS::MutableHandle<PropertyDescriptor> desc) const
 {
     if (!SecurityXrayDOM::getPropertyDescriptor(cx, wrapper, id, desc))
         return false;
     if (desc.object()) {
+        // Cross-origin DOM objects do not have symbol-named properties apart
+        // from the ones we add ourselves here.
+        MOZ_ASSERT(!JSID_IS_SYMBOL(id),
+                   "What's this symbol-named property that appeared on a "
+                   "Window or Location instance?");
+
         // All properties on cross-origin DOM objects are |own|.
         desc.object().set(wrapper);
 
         // All properties on cross-origin DOM objects are non-enumerable and
         // "configurable". Any value attributes are read-only.
         desc.attributesRef() &= ~JSPROP_ENUMERATE;
         desc.attributesRef() &= ~JSPROP_PERMANENT;
         if (!desc.getter() && !desc.setter())
             desc.attributesRef() |= JSPROP_READONLY;
+    } else if (IsCrossOriginWhitelistedSymbol(cx, id)) {
+        // Spec says to return PropertyDescriptor {
+        //   [[Value]]: undefined, [[Writable]]: false, [[Enumerable]]: false,
+        //   [[Configurable]]: true
+        // }.
+        //
+        desc.setDataDescriptor(JS::UndefinedHandleValue, JSPROP_READONLY);
+        desc.object().set(wrapper);
     }
+
     return true;
 }
 
 bool
 CrossOriginXrayWrapper::getOwnPropertyDescriptor(JSContext* cx,
                                                  JS::Handle<JSObject*> wrapper,
                                                  JS::Handle<jsid> id,
                                                  JS::MutableHandle<PropertyDescriptor> desc) const
@@ -213,17 +251,37 @@ CrossOriginXrayWrapper::getOwnPropertyDe
 
 bool
 CrossOriginXrayWrapper::ownPropertyKeys(JSContext* cx, JS::Handle<JSObject*> wrapper,
                                         JS::AutoIdVector& props) const
 {
     // All properties on cross-origin objects are supposed |own|, despite what
     // the underlying native object may report. Override the inherited trap to
     // avoid passing JSITER_OWNONLY as a flag.
-    return SecurityXrayDOM::getPropertyKeys(cx, wrapper, JSITER_HIDDEN, props);
+    if (!SecurityXrayDOM::getPropertyKeys(cx, wrapper, JSITER_HIDDEN, props)) {
+        return false;
+    }
+
+    // Now add the three symbol-named props cross-origin objects have.
+#ifdef DEBUG
+    for (size_t n = 0; n < props.length(); ++n) {
+        MOZ_ASSERT(!JSID_IS_SYMBOL(props[n]),
+                   "Unexpected existing symbol-name prop");
+    }
+#endif
+    if (!props.reserve(props.length() +
+                       ArrayLength(sCrossOriginWhitelistedSymbolCodes))) {
+        return false;
+    }
+
+    for (auto code : sCrossOriginWhitelistedSymbolCodes) {
+        props.infallibleAppend(SYMBOL_TO_JSID(JS::GetWellKnownSymbol(cx, code)));
+    }
+
+    return true;
 }
 
 bool
 CrossOriginXrayWrapper::defineProperty(JSContext* cx, JS::Handle<JSObject*> wrapper,
                                        JS::Handle<jsid> id,
                                        JS::Handle<PropertyDescriptor> desc,
                                        JS::ObjectOpResult& result) const
 {
--- a/js/xpconnect/wrappers/FilteringWrapper.h
+++ b/js/xpconnect/wrappers/FilteringWrapper.h
@@ -79,11 +79,15 @@ class CrossOriginXrayWrapper : public Se
     virtual bool delete_(JSContext* cx, JS::Handle<JSObject*> wrapper,
                          JS::Handle<jsid> id, JS::ObjectOpResult& result) const override;
 
     virtual bool getPropertyDescriptor(JSContext* cx, JS::Handle<JSObject*> wrapper,
                                        JS::Handle<jsid> id,
                                        JS::MutableHandle<JS::PropertyDescriptor> desc) const override;
 };
 
+// Check whether the given jsid is a symbol whose value can be gotten
+// cross-origin.  Cross-origin gets always return undefined as the value.
+bool IsCrossOriginWhitelistedSymbol(JSContext* cx, JS::HandleId id);
+
 } // namespace xpc
 
 #endif /* __FilteringWrapper_h__ */
--- a/testing/web-platform/tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.sub.html
+++ b/testing/web-platform/tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.sub.html
@@ -61,26 +61,30 @@ addTest(function() {
 }, "Basic sanity-checking");
 
 /*
  * Whitelist behavior.
  *
  * Also tests for [[GetOwnProperty]] and [[HasOwnProperty]] behavior.
  */
 
-var whitelistedWindowProps = ['location', 'postMessage', 'window', 'frames', 'self', 'top', 'parent',
-                              'opener', 'closed', 'close', 'blur', 'focus', 'length'];
+var whitelistedWindowPropNames = ['location', 'postMessage', 'window', 'frames', 'self', 'top', 'parent',
+                                  'opener', 'closed', 'close', 'blur', 'focus', 'length'];
+var whitelistedSymbols = [Symbol.isConcatSpreadable, Symbol.toStringTag,
+                          Symbol.hasInstance];
+var whitelistedWindowProps = whitelistedWindowPropNames.concat(whitelistedSymbols);
+
 addTest(function() {
   for (var prop in window) {
     if (whitelistedWindowProps.indexOf(prop) != -1) {
       C[prop]; // Shouldn't throw.
       Object.getOwnPropertyDescriptor(C, prop); // Shouldn't throw.
-      assert_true(Object.prototype.hasOwnProperty.call(C, prop), "hasOwnProperty for " + prop);
+      assert_true(Object.prototype.hasOwnProperty.call(C, prop), "hasOwnProperty for " + String(prop));
     } else {
-      assert_throws(null, function() { C[prop]; }, "Should throw when accessing " + prop + " on Window");
+      assert_throws(null, function() { C[prop]; }, "Should throw when accessing " + String(prop) + " on Window");
       assert_throws(null, function() { Object.getOwnPropertyDescriptor(C, prop); },
                     "Should throw when accessing property descriptor for " + prop + " on Window");
       assert_throws(null, function() { Object.prototype.hasOwnProperty.call(C, prop); },
                     "Should throw when invoking hasOwnProperty for " + prop + " on Window");
     }
     if (prop != 'location')
       assert_throws(null, function() { C[prop] = undefined; }, "Should throw when writing to " + prop + " on Window");
   }
@@ -160,34 +164,47 @@ addTest(function() {
 addTest(function() {
   assert_true(isObject(Object.getOwnPropertyDescriptor(C, 'close')), "C.close is |own|");
   assert_true(isObject(Object.getOwnPropertyDescriptor(C, 'top')), "C.top is |own|");
   assert_true(isObject(Object.getOwnPropertyDescriptor(C.location, 'href')), "C.location.href is |own|");
   assert_true(isObject(Object.getOwnPropertyDescriptor(C.location, 'replace')), "C.location.replace is |own|");
 }, "[[GetOwnProperty]] - Properties on cross-origin objects should be reported |own|");
 
 function checkPropertyDescriptor(desc, propName, expectWritable) {
+  var isSymbol = (typeof(propName) == "symbol");
+  propName = String(propName);
   assert_true(isObject(desc), "property descriptor for " + propName + " should exist");
   assert_equals(desc.enumerable, false, "property descriptor for " + propName + " should be non-enumerable");
   assert_equals(desc.configurable, true, "property descriptor for " + propName + " should be configurable");
+  if (isSymbol) {
+    assert_true("value" in desc,
+                "property descriptor for " + propName + " should be a value descriptor");
+    assert_equals(desc.value, undefined,
+                  "symbol-named cross-origin visible prop " + propName +
+                  " should come back as undefined");
+  }
   if ('value' in desc)
     assert_equals(desc.writable, expectWritable, "property descriptor for " + propName + " should have writable: " + expectWritable);
   else
     assert_equals(typeof desc.set != 'undefined', expectWritable,
                   "property descriptor for " + propName + " should " + (expectWritable ? "" : "not ") + "have setter");
 }
 
 addTest(function() {
   whitelistedWindowProps.forEach(function(prop) {
     var desc = Object.getOwnPropertyDescriptor(C, prop);
     checkPropertyDescriptor(desc, prop, prop == 'location');
   });
   checkPropertyDescriptor(Object.getOwnPropertyDescriptor(C.location, 'replace'), 'replace', false);
   checkPropertyDescriptor(Object.getOwnPropertyDescriptor(C.location, 'href'), 'href', true);
   assert_equals(typeof Object.getOwnPropertyDescriptor(C.location, 'href').get, 'undefined', "Cross-origin location should have no href getter");
+  whitelistedSymbols.forEach(function(prop) {
+    var desc = Object.getOwnPropertyDescriptor(C.location, prop);
+    checkPropertyDescriptor(desc, prop, false);
+  });
 }, "[[GetOwnProperty]] - Property descriptors for cross-origin properties should be set up correctly");
 
 /*
  * [[Delete]]
  */
 addTest(function() {
   assert_throws(null, function() { delete C.location; }, "Can't delete cross-origin property");
   assert_throws(null, function() { delete C.parent; }, "Can't delete cross-origin property");
@@ -232,22 +249,45 @@ addTest(function() {
     assert_unreached("Shouldn't have been able to enumerate " + prop + " on cross-origin Location");
 }, "[[Enumerate]] should return an empty iterator");
 
 /*
  * [[OwnPropertyKeys]]
  */
 
 addTest(function() {
-  assert_array_equals(whitelistedWindowProps.sort(), Object.getOwnPropertyNames(C).sort(),
+  assert_array_equals(whitelistedWindowPropNames.sort(),
+                      Object.getOwnPropertyNames(C).sort(),
                       "Object.getOwnPropertyNames() gives the right answer for cross-origin Window");
   assert_array_equals(Object.getOwnPropertyNames(C.location).sort(), ['href', 'replace'],
                       "Object.getOwnPropertyNames() gives the right answer for cross-origin Location");
 }, "[[OwnPropertyKeys]] should return all properties from cross-origin objects");
 
+// Compare two arrays that need to have the same elements but may be in
+// different orders.  The problem is that there's no good way to sort arrays
+// containing Symbols.
+function assert_arrays_equal_up_to_order(arr1, arr2, desc) {
+  for (let item of arr1) {
+    assert_in_array(item, arr2, desc);
+  }
+
+  for (let item of arr2) {
+    assert_in_array(item, arr1, desc);
+  }
+}
+
+addTest(function() {
+  assert_arrays_equal_up_to_order(Object.getOwnPropertySymbols(C),
+                                  whitelistedSymbols,
+    "Object.getOwnPropertySymbols() should return the three symbol-named properties that are exposed on a cross-origin Window");
+  assert_arrays_equal_up_to_order(Object.getOwnPropertySymbols(C.location),
+                                  whitelistedSymbols,
+    "Object.getOwnPropertySymbols() should return the three symbol-named properties that are exposed on a cross-origin Location");
+}, "[[OwnPropertyKeys]] should return the right symbol-named properties for cross-origin objects");
+
 addTest(function() {
   assert_true(B.eval('parent.C') === C, "A and B observe the same identity for C's Window");
   assert_true(B.eval('parent.C.location') === C.location, "A and B observe the same identity for C's Location");
 }, "A and B jointly observe the same identity for cross-origin Window and Location");
 
 function checkFunction(f, proto) {
   var name = f.name || '<missing name>';
   assert_equals(typeof f, 'function', name + " is a function");
@@ -301,16 +341,21 @@ addTest(function() {
   var set_href_B = B.eval('Object.getOwnPropertyDescriptor(parent.C.location, "href").set');
   assert_true(set_self_href != set_href_A, 'different Location accessors per-incumbent script settings object');
   assert_true(set_href_A != set_href_B, 'different Location accessors per-incumbent script settings object');
   checkFunction(set_self_href, Function.prototype);
   checkFunction(set_href_A, Function.prototype);
   checkFunction(set_href_B, B.Function.prototype);
 }, "Same-origin observers get different accessors for cross-origin Location");
 
+addTest(function() {
+  assert_equals({}.toString.call(C), "[object Object]");
+  assert_equals({}.toString.call(C.location), "[object Object]");
+}, "{}.toString.call() does the right thing on cross-origin objects");
+
 function doDocumentDomainTest(cb) {
   window.addEventListener('message', function onmessage(evt) {
     window.removeEventListener('message', onmessage);
     test(function() {
       var results = evt.data;
       assert_true(results.length > 0, 'Need results');
       results.forEach(function(r) { assert_true(r.pass, r.message); });
     }, "Cross-origin object identity preserved across document.domain");