Make crossOriginObject.then undefined for promises

https://bugs.webkit.org/show_bug.cgi?id=190094 Reviewed by Darin Adler. LayoutTests/imported/w3c: Rebaseline WPT test now that more checks are passing. * web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt: Source/WebCore: Make crossOriginObject.then undefined for promises. This allows promises to work better with cross-origin WindowProxy and Location objects. Specification: - whatwg/html#3242 - whatwg/dom#536 This aligns our behavior with Blink and Gecko. No new tests, rebaselined existing test. * bindings/js/JSDOMWindowCustom.cpp: (WebCore::jsDOMWindowGetOwnPropertySlotRestrictedAccess): (WebCore::addCrossOriginWindowOwnPropertyNames): * bindings/js/JSLocationCustom.cpp: (WebCore::getOwnPropertySlotCommon): (WebCore::addCrossOriginLocationOwnPropertyNames): LayoutTests: Update existing tests to reflect behavior change. * http/tests/navigation/process-swap-window-open-expected.txt: * http/tests/navigation/process-swap-window-open.html: * http/wpt/cross-origin-window-policy/resources/utils.js: (testCrossOriginOption): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@236661 268f45cc-cd09-0410-ab3c-d52691b4dbfc
paul99 · Oct 1, 2018 · 3dd1e0f · 3dd1e0f
1 parent 2f92955
commit 3dd1e0f
Show file tree

Hide file tree

Showing 11 changed files with 120 additions and 65 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2018-10-01  Chris Dumez  <[email protected]>
+
+        Make crossOriginObject.then undefined for promises
+        https://bugs.webkit.org/show_bug.cgi?id=190094
+
+        Reviewed by Darin Adler.
+
+        Update existing tests to reflect behavior change.
+
+        * http/tests/navigation/process-swap-window-open-expected.txt:
+        * http/tests/navigation/process-swap-window-open.html:
+        * http/wpt/cross-origin-window-policy/resources/utils.js:
+        (testCrossOriginOption):
+
 2018-10-01  Alicia Boya García  <[email protected]>
 
         [MSE][GStreamer] Reset running time in PlaybackPipeline::flush()

diff --git a/LayoutTests/http/tests/navigation/process-swap-window-open-expected.txt b/LayoutTests/http/tests/navigation/process-swap-window-open-expected.txt
@@ -31,6 +31,7 @@ PASS w.postMessage is an instance of Function
 PASS w.postMessage('test', '*') did not throw exception.
 PASS w.focus() did not throw exception.
 PASS w.blur() did not throw exception.
+PASS w.then is undefined
 PASS w.location did not throw exception.
 FAIL w.location should not be null.
 PASS areArraysEqual(actual_properties, expected_property_names) is true

diff --git a/LayoutTests/http/tests/navigation/process-swap-window-open.html b/LayoutTests/http/tests/navigation/process-swap-window-open.html
@@ -39,11 +39,12 @@
     shouldNotThrow("w.postMessage('test', '*')");
     shouldNotThrow("w.focus()");
     shouldNotThrow("w.blur()");
+    shouldBe("w.then", "undefined");
 
     shouldNotThrow("w.location");
     shouldNotBe("w.location", "null");
 
-    expected_property_names = ["blur", "close", "closed", "focus", "frames", "length", "location", "opener", "parent", "postMessage", "self", "top", "window"];
+    expected_property_names = ["blur", "close", "closed", "focus", "frames", "length", "location", "opener", "parent", "postMessage", "self", "top", "window", "then"];
     actual_properties = Object.getOwnPropertyNames(w);
     shouldBeTrue("areArraysEqual(actual_properties, expected_property_names)");
 

diff --git a/LayoutTests/http/tests/security/cross-frame-access-enumeration.html b/LayoutTests/http/tests/security/cross-frame-access-enumeration.html
@@ -87,8 +87,7 @@
 
             // Window.
             whitelistedWindowIndices = ['0', '1', '2'];
-            whitelistedWindowPropNames = ['blur', 'close', 'closed', 'focus', 'frames', 'length', 'location', 'opener', 'parent', 'postMessage', 'self', 'top', 'window'];
-            whitelistedLocationProperties = ['href', 'replace'];
+            whitelistedWindowPropNames = ['blur', 'close', 'closed', 'focus', 'frames', 'length', 'location', 'opener', 'parent', 'postMessage', 'self', 'then', 'top', 'window'];
             whitelistedSymbols = [Symbol.toStringTag, Symbol.hasInstance, Symbol.isConcatSpreadable];
             shouldBeTrue("areArraysEqual(Object.getOwnPropertyNames(b_win).sort(), whitelistedWindowIndices.concat(whitelistedWindowPropNames).sort())");
             allWindowProps = Reflect.ownKeys(b_win);
@@ -100,7 +99,7 @@
             shouldBeTrue("areArraysEqual(symbolWindowProps, whitelistedSymbols)"); // Reflect.ownKeys should end with the cross-origin symbols for a cross-origin Window.
 
             // Location.
-            whitelistedLocationPropNames = ['href', 'replace'];
+            whitelistedLocationPropNames = ['href', 'replace', 'then'];
             allLocationProps = Reflect.ownKeys(b_win.location);
             stringLocationProps = allLocationProps.slice(0, -1 * whitelistedSymbols.length);
             symbolLocationProps = allLocationProps.slice(-1 * whitelistedSymbols.length);

diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,14 @@
+2018-10-01  Chris Dumez  <[email protected]>
+
+        Make crossOriginObject.then undefined for promises
+        https://bugs.webkit.org/show_bug.cgi?id=190094
+
+        Reviewed by Darin Adler.
+
+        Rebaseline WPT test now that more checks are passing.
+
+        * web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:
+
 2018-09-30  Walker Henderson  <[email protected]>
 
         AudioNode.connect should return passed destination node

diff --git a/...latform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt b/...latform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt
@@ -8,15 +8,15 @@ PASS [[SetPrototypeOf]] should return false
 PASS [[IsExtensible]] should return true for cross-origin objects 
 PASS [[PreventExtensions]] should throw for cross-origin objects 
 PASS [[GetOwnProperty]] - Properties on cross-origin objects should be reported |own| 
-FAIL [[GetOwnProperty]] - Property descriptors for cross-origin properties should be set up correctly Blocked a frame with origin "http://localhost:8800" from accessing a cross-origin frame. Protocols, domains, and ports must match.
+PASS [[GetOwnProperty]] - Property descriptors for cross-origin properties should be set up correctly 
 PASS [[GetOwnProperty]] - Subframe named 'then' should shadow the default 'then' value 
 PASS [[Delete]] Should throw on cross-origin objects 
 PASS [[DefineOwnProperty]] Should throw for cross-origin objects 
 PASS Can only enumerate safelisted enumerable properties 
-FAIL [[OwnPropertyKeys]] should return all properties from cross-origin objects assert_array_equals: Object.getOwnPropertyNames() gives the right answer for cross-origin Window lengths differ, expected 16 got 15
+PASS [[OwnPropertyKeys]] should return all properties from cross-origin objects 
 PASS [[OwnPropertyKeys]] should return the right symbol-named properties for cross-origin objects 
-FAIL [[OwnPropertyKeys]] should place the symbols after the property names after the subframe indices assert_equals: 'then' property should be added to the end of the string list if not there expected "then" but got "window"
-FAIL [[OwnPropertyKeys]] should not reorder where 'then' appears if it's a named subframe, nor add another copy of 'then' assert_equals: expected "then" but got "window"
+PASS [[OwnPropertyKeys]] should place the symbols after the property names after the subframe indices 
+PASS [[OwnPropertyKeys]] should not reorder where 'then' appears if it's a named subframe, nor add another copy of 'then' 
 PASS A and B jointly observe the same identity for cross-origin Window and Location 
 PASS Cross-origin functions get local Function.prototype 
 PASS Cross-origin Window accessors get local Function.prototype 

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,28 @@
+2018-10-01  Chris Dumez  <[email protected]>
+
+        Make crossOriginObject.then undefined for promises
+        https://bugs.webkit.org/show_bug.cgi?id=190094
+
+        Reviewed by Darin Adler.
+
+        Make crossOriginObject.then undefined for promises. This allows promises to work better with cross-origin WindowProxy
+        and Location objects.
+
+        Specification:
+        - https://github.com/whatwg/html/pull/3242
+        - https://github.com/whatwg/dom/issues/536
+
+        This aligns our behavior with Blink and Gecko.
+
+        No new tests, rebaselined existing test.
+
+        * bindings/js/JSDOMWindowCustom.cpp:
+        (WebCore::jsDOMWindowGetOwnPropertySlotRestrictedAccess):
+        (WebCore::addCrossOriginWindowOwnPropertyNames):
+        * bindings/js/JSLocationCustom.cpp:
+        (WebCore::getOwnPropertySlotCommon):
+        (WebCore::addCrossOriginLocationOwnPropertyNames):
+
 2018-10-01  Xan Lopez  <[email protected]>
 
         [SOUP] Fix the build for libsoup > 2.61.90

diff --git a/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp b/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
@@ -91,10 +91,6 @@ bool jsDOMWindowGetOwnPropertySlotRestrictedAccess(JSDOMGlobalObject* thisObject
     auto& builtinNames = static_cast<JSVMClientData*>(vm.clientData)->builtinNames();
 
     // https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-)
-    if (propertyName == vm.propertyNames->toStringTagSymbol || propertyName == vm.propertyNames->hasInstanceSymbol || propertyName == vm.propertyNames->isConcatSpreadableSymbol) {
-        slot.setValue(thisObject, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontEnum, jsUndefined());
-        return true;
-    }
 
     // These are the functions we allow access to cross-origin (DoNotCheckSecurity in IDL).
     // Always provide the original function, on a fresh uncached function object.
@@ -157,13 +153,27 @@ bool jsDOMWindowGetOwnPropertySlotRestrictedAccess(JSDOMGlobalObject* thisObject
         }
     }
 
+    if (handleCommonCrossOriginProperties(thisObject, vm, propertyName, slot))
+        return true;
+
     throwSecurityError(state, scope, errorMessage);
     slot.setUndefined();
     return false;
 }
 template bool jsDOMWindowGetOwnPropertySlotRestrictedAccess<DOMWindowType::Local>(JSDOMGlobalObject*, AbstractDOMWindow&, ExecState&, PropertyName, PropertySlot&, const String&);
 template bool jsDOMWindowGetOwnPropertySlotRestrictedAccess<DOMWindowType::Remote>(JSDOMGlobalObject*, AbstractDOMWindow&, ExecState&, PropertyName, PropertySlot&, const String&);
 
+// https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-)
+bool handleCommonCrossOriginProperties(JSObject* thisObject, VM& vm, PropertyName propertyName, PropertySlot& slot)
+{
+    auto& propertyNames =  vm.propertyNames;
+    if (propertyName == propertyNames->builtinNames().thenPublicName() || propertyName == propertyNames->toStringTagSymbol || propertyName == propertyNames->hasInstanceSymbol || propertyName == propertyNames->isConcatSpreadableSymbol) {
+        slot.setValue(thisObject, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontEnum, jsUndefined());
+        return true;
+    }
+    return false;
+}
+
 // Property access sequence is:
 // (1) indexed properties,
 // (2) regular own properties,
@@ -309,29 +319,50 @@ void JSDOMWindow::heapSnapshot(JSCell* cell, HeapSnapshotBuilder& builder)
 }
 
 // https://html.spec.whatwg.org/#crossoriginproperties-(-o-)
-static void addCrossOriginWindowPropertyNames(ExecState& state, PropertyNameArray& propertyNames)
+template <CrossOriginObject objectType>
+static void addCrossOriginPropertyNames(VM& vm, PropertyNameArray& propertyNames)
+{
+    switch (objectType) {
+    case CrossOriginObject::Location: {
+        static const Identifier* const properties[] = { &vm.propertyNames->href, &vm.propertyNames->replace };
+        for (auto* property : properties)
+            propertyNames.add(*property);
+        break;
+    }
+    case CrossOriginObject::Window: {
+        auto& builtinNames = static_cast<JSVMClientData*>(vm.clientData)->builtinNames();
+        static const Identifier* const properties[] = {
+            &builtinNames.blurPublicName(), &builtinNames.closePublicName(), &builtinNames.closedPublicName(),
+            &builtinNames.focusPublicName(), &builtinNames.framesPublicName(), &vm.propertyNames->length,
+            &builtinNames.locationPublicName(), &builtinNames.openerPublicName(), &builtinNames.parentPublicName(),
+            &builtinNames.postMessagePublicName(), &builtinNames.selfPublicName(), &builtinNames.topPublicName(),
+            &builtinNames.windowPublicName()
+        };
+
+        for (auto* property : properties)
+            propertyNames.add(*property);
+        break;
+    }
+    }
+}
+
+// https://html.spec.whatwg.org/#crossoriginownpropertykeys-(-o-)
+template <CrossOriginObject objectType>
+void addCrossOriginOwnPropertyNames(JSC::ExecState& state, JSC::PropertyNameArray& propertyNames)
 {
     auto& vm = state.vm();
+    addCrossOriginPropertyNames<objectType>(vm, propertyNames);
 
     static const Identifier* const properties[] = {
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().blurPublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().closePublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().closedPublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().focusPublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().framesPublicName(),
-        &vm.propertyNames->length,
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().locationPublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().openerPublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().parentPublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().postMessagePublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().selfPublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().topPublicName(),
-        &static_cast<JSVMClientData*>(vm.clientData)->builtinNames().windowPublicName()
+        &vm.propertyNames->builtinNames().thenPublicName(), &vm.propertyNames->toStringTagSymbol, &vm.propertyNames->hasInstanceSymbol, &vm.propertyNames->isConcatSpreadableSymbol
     };
 
     for (auto* property : properties)
         propertyNames.add(*property);
+
 }
+template void addCrossOriginOwnPropertyNames<CrossOriginObject::Window>(JSC::ExecState&, JSC::PropertyNameArray&);
+template void addCrossOriginOwnPropertyNames<CrossOriginObject::Location>(JSC::ExecState&, JSC::PropertyNameArray&);
 
 static void addScopedChildrenIndexes(ExecState& state, DOMWindow& window, PropertyNameArray& propertyNames)
 {
@@ -348,17 +379,6 @@ static void addScopedChildrenIndexes(ExecState& state, DOMWindow& window, Proper
         propertyNames.add(Identifier::from(&state, i));
 }
 
-// https://html.spec.whatwg.org/#crossoriginownpropertykeys-(-o-)
-void addCrossOriginWindowOwnPropertyNames(ExecState& state, PropertyNameArray& propertyNames)
-{
-    addCrossOriginWindowPropertyNames(state, propertyNames);
-
-    auto& vm = state.vm();
-    propertyNames.add(vm.propertyNames->toStringTagSymbol);
-    propertyNames.add(vm.propertyNames->hasInstanceSymbol);
-    propertyNames.add(vm.propertyNames->isConcatSpreadableSymbol);
-}
-
 // https://html.spec.whatwg.org/#windowproxy-ownpropertykeys
 void JSDOMWindow::getOwnPropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNames, EnumerationMode mode)
 {
@@ -368,7 +388,7 @@ void JSDOMWindow::getOwnPropertyNames(JSObject* object, ExecState* exec, Propert
 
     if (!BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped(), DoNotReportSecurityError)) {
         if (mode.includeDontEnumProperties())
-            addCrossOriginWindowOwnPropertyNames(*exec, propertyNames);
+            addCrossOriginOwnPropertyNames<CrossOriginObject::Window>(*exec, propertyNames);
         return;
     }
     Base::getOwnPropertyNames(thisObject, exec, propertyNames, mode);

diff --git a/Source/WebCore/bindings/js/JSDOMWindowCustom.h b/Source/WebCore/bindings/js/JSDOMWindowCustom.h
@@ -40,6 +40,10 @@ enum class DOMWindowType { Local, Remote };
 template <DOMWindowType windowType>
 bool jsDOMWindowGetOwnPropertySlotRestrictedAccess(JSDOMGlobalObject*, AbstractDOMWindow&, JSC::ExecState&, JSC::PropertyName, JSC::PropertySlot&, const String&);
 
-void addCrossOriginWindowOwnPropertyNames(JSC::ExecState&, JSC::PropertyNameArray&);
+enum class CrossOriginObject { Window, Location };
+
+template <CrossOriginObject objectType>
+void addCrossOriginOwnPropertyNames(JSC::ExecState&, JSC::PropertyNameArray&);
+bool handleCommonCrossOriginProperties(JSC::JSObject* thisObject, JSC::VM&, JSC::PropertyName, JSC::PropertySlot&);
 
 } // namespace WebCore
diff --git a/Source/WebCore/bindings/js/JSLocationCustom.cpp b/Source/WebCore/bindings/js/JSLocationCustom.cpp
@@ -26,6 +26,7 @@
 #include "JSDOMBinding.h"
 #include "JSDOMBindingSecurity.h"
 #include "JSDOMExceptionHandling.h"
+#include "JSDOMWindowCustom.h"
 #include "RuntimeApplicationChecks.h"
 #include <JavaScriptCore/JSFunction.h>
 #include <JavaScriptCore/Lookup.h>
@@ -54,10 +55,6 @@ static bool getOwnPropertySlotCommon(JSLocation& thisObject, ExecState& state, P
         return false;
 
     // https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-)
-    if (propertyName == vm.propertyNames->toStringTagSymbol || propertyName == vm.propertyNames->hasInstanceSymbol || propertyName == vm.propertyNames->isConcatSpreadableSymbol) {
-        slot.setValue(&thisObject, PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum, jsUndefined());
-        return true;
-    }
 
     // We only allow access to Location.replace() cross origin.
     if (propertyName == vm.propertyNames->replace) {
@@ -74,6 +71,9 @@ static bool getOwnPropertySlotCommon(JSLocation& thisObject, ExecState& state, P
         return true;
     }
 
+    if (handleCommonCrossOriginProperties(&thisObject, vm, propertyName, slot))
+        return true;
+
     throwSecurityError(state, scope, message);
     slot.setUndefined();
     return true;
@@ -163,32 +163,12 @@ bool JSLocation::deletePropertyByIndex(JSCell* cell, ExecState* exec, unsigned p
     return Base::deletePropertyByIndex(thisObject, exec, propertyName);
 }
 
-// https://html.spec.whatwg.org/#crossoriginproperties-(-o-)
-static void addCrossOriginLocationPropertyNames(ExecState& state, PropertyNameArray& propertyNames)
-{
-    VM& vm = state.vm();
-    static const Identifier* const properties[] = { &vm.propertyNames->href, &vm.propertyNames->replace };
-    for (auto* property : properties)
-        propertyNames.add(*property);
-}
-
-// https://html.spec.whatwg.org/#crossoriginownpropertykeys-(-o-)
-static void addCrossOriginLocationOwnPropertyNames(ExecState& state, PropertyNameArray& propertyNames)
-{
-    VM& vm = state.vm();
-    addCrossOriginLocationPropertyNames(state, propertyNames);
-
-    propertyNames.add(vm.propertyNames->toStringTagSymbol);
-    propertyNames.add(vm.propertyNames->hasInstanceSymbol);
-    propertyNames.add(vm.propertyNames->isConcatSpreadableSymbol);
-}
-
 void JSLocation::getOwnPropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNames, EnumerationMode mode)
 {
     JSLocation* thisObject = jsCast<JSLocation*>(object);
     if (!BindingSecurity::shouldAllowAccessToFrame(exec, thisObject->wrapped().frame(), DoNotReportSecurityError)) {
         if (mode.includeDontEnumProperties())
-            addCrossOriginLocationOwnPropertyNames(*exec, propertyNames);
+            addCrossOriginOwnPropertyNames<CrossOriginObject::Location>(*exec, propertyNames);
         return;
     }
     Base::getOwnPropertyNames(thisObject, exec, propertyNames, mode);

diff --git a/Source/WebCore/bindings/js/JSRemoteDOMWindowCustom.cpp b/Source/WebCore/bindings/js/JSRemoteDOMWindowCustom.cpp
@@ -102,7 +102,7 @@ void JSRemoteDOMWindow::getOwnPropertyNames(JSObject*, ExecState* exec, Property
     // FIXME: Add scoped children indexes.
 
     if (mode.includeDontEnumProperties())
-        addCrossOriginWindowOwnPropertyNames(*exec, propertyNames);
+        addCrossOriginOwnPropertyNames<CrossOriginObject::Window>(*exec, propertyNames);
 }
 
 bool JSRemoteDOMWindow::defineOwnProperty(JSC::JSObject*, JSC::ExecState* state, JSC::PropertyName, const JSC::PropertyDescriptor&, bool)