Window allows defining own indexed properties
Reported by
sandle...@gmail.com,
Feb 23 2017
|
||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36 Steps to reproduce the problem: 1. Enter any website: http://example.com/ 2. Open developer console and run the following line: window[0] = 1; 3. Now check for the existence of the above Object.getOwnPropertyNames(window).indexOf("0") != -1 // returns true What is the expected behavior? The value should not be set What went wrong? Window should not allow setting indexed properties, as defined in the latest HTML specification at the following URL: https://html.spec.whatwg.org/multipage/browsers.html#windowproxy-defineownproperty in step 2 sub-step 1. Did this work before? N/A Chrome version: 55.0.2883.87 Channel: n/a OS Version: Linux michx 4.9.9-1-ARCH #1 SMP PREEMPT Thu Feb 9 19:07:09 CET 2017 x86_64 GNU/Linux Flash Version: Shockwave Flash 24.0 r0 Side note - from my tests, Mozilla Firefox is already up to spec on this matter, the correct behavior can be observed in their stable release.
,
Feb 28 2017
,
Mar 1 2017
Tested this issue on Ubuntu 14.04 with latest chrome dev #58.0.3025.5, with the steps mentioned in the comment #0 and observed that after step 3 it is giving an output as "true" where as in Firefox it has output as "false", considered this as expected output Able to reproduce this issue on Windows 10, Ubuntu 14.04 and Mac 10.12.3 Issue is broken in M-48 Bisect Info: =========== Good build : 48.0.2528.0, Revision Range- 352486 Bad build : 48.0.2529.0, Revision Range- 352715 After executing the per-revision-bisect script, i got the following CL's between good and bad build versions ============================================ https://chromium.googlesource.com/chromium/src/+log/088764aeb99e93add4fb3450b9b2bdf256632060..48ce0ef9537a41150fa0d59edd47c318661aeee4 unable to find the Culprit CL which as caused this issue, requesting dev team to look into it and assign it to the concern owner. Thank You...
,
Mar 1 2017
Thanks for researching into this issue. After you said the issue is not reproducible in build 48.0.2528.0, I've downloaded it and dis some testing From what I understand, there just seems to be a another issue in that build, preventing getOwnPropertyNames from returning the indexed properties. If you perform an easier check, you can see the browser still allows setting indexed properties on the Window object: In Chromium build : window[0] // Returns undefined, as nothing is done yet window[0] = 1 window[0] == 1 // Returns true Just to be sure I've performed the same test on my stable Firefox version (51.0.1), and the result was false (it doesn't allow setting the indexed properties) Two screenshots are attached, one for the Chromium 48.0.2528.0 build, and one for the Firefox test. Regards, Michael.
,
Mar 10 2017
,
Mar 14 2017
,
Mar 14 2017
@tkent - see comment 4 - it does not seem to be a regression (at least not since Chrome 48).
,
Apr 11 2017
Removing regression label and downgrading to P3. Sathya, do you know if this is something that should be happening on the V8 side (IIUC JSGlobalProxy is the exotic object that corresponds to the web's WindowProxy), or whether there's a hook Blink should be using to get this behaviour (AFAIK we don't normally have the hooks to customize [[DefineOwnProperty]], aside from security checks).
,
Apr 11 2017
+yukishiino@ for WindowProxy bindings issues and +jochen@ the specific API question.
,
Apr 12 2017
I think that we can hide the issue on Blink side.
,
Apr 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c105f1574d48e229385fbef9293aca66fc087f9b commit c105f1574d48e229385fbef9293aca66fc087f9b Author: yukishiino <yukishiino@chromium.org> Date: Mon Apr 17 09:28:49 2017 v8binding: Implements an alternative of WindowProxy.[[DefineOwnProperty]]. Implements the following part of the spec with using indexed interceptor. https://html.spec.whatwg.org/C/browsers.html#windowproxy-defineownproperty step 2 - 1. If P is an array index property name, return false. The spec's actual effect is mostly that author script cannot define a property with an array index property name on a window object. With this CL, the following ES code has no effect. window[0] = 42; Object.defineProperty(window, 0, {value: 42}); BUG= 695385 Review-Url: https://codereview.chromium.org/2816743002 Cr-Commit-Position: refs/heads/master@{#464912} [delete] https://crrev.com/782c68372ded66d885ebd848bace62d639998dfc/third_party/WebKit/LayoutTests/external/wpt/html/browsers/the-window-object/window-indexed-properties-expected.txt [modify] https://crrev.com/c105f1574d48e229385fbef9293aca66fc087f9b/third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-delete-expected.txt [modify] https://crrev.com/c105f1574d48e229385fbef9293aca66fc087f9b/third_party/WebKit/LayoutTests/http/tests/security/resources/cross-frame-iframe-for-delete-test.html [modify] https://crrev.com/c105f1574d48e229385fbef9293aca66fc087f9b/third_party/WebKit/Source/core/frame/DOMWindow.cpp [modify] https://crrev.com/c105f1574d48e229385fbef9293aca66fc087f9b/third_party/WebKit/Source/core/frame/DOMWindow.h [modify] https://crrev.com/c105f1574d48e229385fbef9293aca66fc087f9b/third_party/WebKit/Source/core/frame/Window.idl
,
Apr 18 2017
,
Apr 18 2017
@yukishiino Hi, Sorry to bug again,
first of all, thanks for your time and prompt fix!
I've compiled the revision with your fix, and made some tests, and it's weird, it seems the fix is not fully air-tight for some reason, the following test fails for the fix:
> Object.defineProperty(window, "1", {get: function() { return 123 }});
> window[1] == 123
> true
Sorry I didn't supply this test before, I didn't think of it back then.
I looked at the code fixing the issue, but I'm not really sure why this still works, but I'm barely familiar with the Chromium source, so you probably have a better idea
,
Apr 18 2017
Yeah, setters don't make the object exotic in the same way, which is why I wasn't sure this could be done without V8-side help. (Admittedly, the differences are getting increasingly esoteric, but the bug isn't precisely fixed.)
,
Apr 18 2017
jochen@, I'd like to have your opinions here. For data properties, the indexed interceptor is called as expected regardless of a way to define a property (assignment or Object.defineProperty). But for accessor properties, the indexed interceptor is not called. I understand that it doesn't make much sense to call the indexed interceptor with an accessor property. However, is there any chance to deny such an accessor property definition? I don't fully understand, but, https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-defineownproperty-p-desc seems saying that we should deny defining an accessor property for Integer Indexed exotic object. Is this related? FWIW, I can define an accessor property with integer index key on a built-in Array. Is this expected?
,
Apr 18 2017
does defining an IndexedPropertyDefinerCallback work? https://cs.chromium.org/chromium/src/v8/include/v8.h?rcl=87d4ee8cfb0bdc2b30aab32c8bbdc00e65c16397&l=5147
,
Apr 18 2017
It might have bugs, but in general it should work. At least to the extend of these tests: https://cs.chromium.org/chromium/src/v8/test/cctest/test-api-interceptors.cc?type=cs&q=PropertyDefinerCallbackIndexed&l=1714
,
Apr 19 2017
Good to know. I'll use that handler.
,
Jul 2 2017
Hi @yukishiino, do you have a rough estimate on when you will have time to finish this issue? Thanks, Mich.
,
Jul 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/58ab4a971b06dec13e4edf9de8382ca6847f6190 commit 58ab4a971b06dec13e4edf9de8382ca6847f6190 Author: yukishiino <yukishiino@chromium.org> Date: Fri Jul 07 09:59:51 2017 v8binding: Don't allow author script to define indexed accessor prop. Author script is not allowed to define an accessor property with an array index property name if the object supports indexed properties. This CL disallows it. BUG= 695385 Review-Url: https://codereview.chromium.org/2832923003 Cr-Commit-Position: refs/heads/master@{#484871} [add] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object-expected.txt [add] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/LayoutTests/external/wpt/WebIDL/ecmascript-binding/legacy-platform-object.html [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/LayoutTests/external/wpt/dom/collections/HTMLCollection-supported-property-indices-expected.txt [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Document-getElementsByTagName-expected.txt [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Element-getElementsByTagName-expected.txt [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/LayoutTests/external/wpt/html/browsers/the-window-object/window-indexed-properties-expected.txt [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/LayoutTests/external/wpt/html/browsers/the-window-object/window-indexed-properties-strict-expected.txt [delete] https://crrev.com/af5eed337751ca4f227f80a722c3c374dcf7973e/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/media-elements/interfaces/TextTrackCueList/getter-expected.txt [delete] https://crrev.com/af5eed337751ca4f227f80a722c3c374dcf7973e/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/media-elements/interfaces/TextTrackList/getter-expected.txt [delete] https://crrev.com/af5eed337751ca4f227f80a722c3c374dcf7973e/third_party/WebKit/LayoutTests/media/track/opera/interfaces/TextTrackCueList/getter-expected.txt [delete] https://crrev.com/af5eed337751ca4f227f80a722c3c374dcf7973e/third_party/WebKit/LayoutTests/media/track/opera/interfaces/TextTrackList/getter-expected.txt [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/templates/interface.h.tmpl [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexed.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexed.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedGlobal.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedGlobal.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedPrimaryGlobal.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedPrimaryGlobal.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface3.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface3.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperations.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperationsNotEnumerable.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperationsNotEnumerable.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/core/frame/DOMWindow.cpp [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/core/frame/DOMWindow.h [modify] https://crrev.com/58ab4a971b06dec13e4edf9de8382ca6847f6190/third_party/WebKit/Source/core/frame/Window.idl
,
Jul 11 2017
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by schenney@chromium.org
, Feb 23 2017