-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Align cross-origin [[Delete]] with implementations #1728
Conversation
We need to decide whether to throw a TypeError or a DOMException SecurityError... I wasn't clear from #1726 which it should be. |
Seems nicer to match what ECMAScript would throw. Especially if we want to allow for varied implementation strategies. |
I could see either way. We chose SecurityError for [[Get]]/[[GetOwnProperty]] and for "perform a security check". It seems kind of nice to separate the security errors from the normal TypeErrors. |
Oh, hmm. I guess we should discuss all of them then. |
So [Get] / [GetOwnProperty] and [Set] throw a SecurityError as per the HTML specification, right? Shouldn't [Delete] throw a SecurityError as well? Chrome throws a SecurityError. Firefox seems to throw a generic Error but then again, it seems it throws in generic Error for [Get] / [GetOwnProperty] / [Set] as well, which is not spec-compliant. |
Yeah, I am leaning toward a SecurityError here for all cross-origin stuff. |
@@ -81694,7 +81699,7 @@ State: <OUTPUT NAME=I>1</OUTPUT> <INPUT VALUE="Increment" TYPE=BUTTON O | |||
<li><p>If <span>IsPlatformObjectSameOrigin</span>(<b>this</b>) is true, then return ? | |||
<span>OrdinaryDelete</span>(<b>this</b>, <var>P</var>).</p></li> | |||
|
|||
<li><p>Return false.</p></li> | |||
<li><p>Throw a <code>TypeError</code> exception.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not update [[Delete]] for Location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's what this line is doing, it's just the diff makes it hard to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems to update Window's [[OwnPropertyKeys]]. Window's [[Delete]] is updated earlier in this patch. However, I do not see any changes to Location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, if you look at the actual source file, line 81702 is for location. (Check out line 81705 below). GitHub has an un-expandable gap between line 79325 (for window) and line 81699 (by which time we're in location).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I guess I cannot read github diffs, sorry :)
Cross browser, it seems we have the following
|
https://bugs.webkit.org/show_bug.cgi?id=161397 Reviewed by Ryosuke Niwa. Source/WebCore: [[Delete]] should throw for cross-origin Window / Location objects: - whatwg/html#1728 Firefox and Chrome already throw. Previously, WebKit was merely ignoring the call and logging an error message. No new tests, updated existing test. * bindings/js/JSDOMWindowCustom.cpp: (WebCore::JSDOMWindow::deleteProperty): (WebCore::JSDOMWindow::deletePropertyByIndex): * bindings/js/JSLocationCustom.cpp: (WebCore::JSLocation::deleteProperty): (WebCore::JSLocation::deletePropertyByIndex): LayoutTests: Update / rebaseline existing test to reflect behavior change. * http/tests/security/cross-frame-access-delete-expected.txt: * http/tests/security/cross-frame-access-delete.html: * http/tests/security/resources/cross-frame-iframe-for-delete-test.html: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@205200 268f45cc-cd09-0410-ab3c-d52691b4dbfc
If we do that though, we should also update the places where we currently return false (that then make it throw) and throw a SecurityError instead. But all that seems uglier to me than consistently throwing a TypeError. And I believe not calling special attention to the security-sensitive failures is @othermaciej's preference, but maybe that changed. |
I agree we should consistently throw a SecurityError, not just in get/set. So:
This scheme matches Chrome/Firefox more so than currently, with the exception that Firefox is using Error instead of SecurityError. Neither of them are returning false (leading to silent sloppy mode failures) and neither of them are throwing a TypeError. I'm happy to make these updates as long as you sign off. |
Firefox and Chrome seem to agree with the specification and return null here though. |
Yeah, I think null for the prototype is probably better. |
Hmm, http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4430 gives an error in Chrome and Firefox though. |
Yes, because proto is an accessor (access to which is denied), rather than a pure MOP operation. |
Adding @yuki3, who's a more representative Blink implementer for this stuff. |
+FYI for @jeisinger. I'd support to update the spec to throw a SecurityError when deleting a property on a cross origin object. I also agree with the goal of @domenic's proposal, but I'm not sure if each statement is right or not. For example, IIRC, crossOriginObj.proto must throws while Object.getPrototypeOf(crossOriginObj) must return null. However, I agree with the resulting behaviors of @domenic's proposal, but not sure what layer is responsible to throw a SecurityError. |
@bholley pointed out that |
Trust @cdumez over me on this one, for WebKit's opinion. |
I think [[Set]] already throws a SecurityError in the specification because we call CrossOriginSet() in the cross-origin case. The first thing it does is call [[GetOwnProperty]] which will throw a SecurityError.
Those throw either way: cross-origin or not. Therefore, I don't think it is worth doing a cross-origin check just to throw a SecurityError instead of a TypeError. Seems kind of weird:
It is not like the reason this is failing is because this is cross-origin. It will fail either way so I don't think this should be a SecurityError. My personal preference would be the leave this part of the spec as is. |
OK, so per @cdumez's last comment and #1727 (comment) it sounds like we don't feel a need to be consistent and always do SecurityError here. So here's the new proposal (with only the changes from the current spec listed):
This means it will usually be SecurityError, but for things that would always fail (no matter if cross-origin or same-origin), namely [[PreventExtensions]] or [[SetPrototypeOf]], they throw a TypeError unconditionally. @annevk, I know you have a vacation coming up; do you want me to take this over? |
Yeah, failing that I can pick it up mid next week most likely. |
…ation objects https://bugs.webkit.org/show_bug.cgi?id=161397 Reviewed by Ryosuke Niwa. Source/WebCore: [[Delete]] should throw for cross-origin Window / Location objects: - whatwg/html#1728 Firefox and Chrome already throw. Previously, WebKit was merely ignoring the call and logging an error message. No new tests, updated existing test. * bindings/js/JSDOMWindowCustom.cpp: (WebCore::JSDOMWindow::deleteProperty): (WebCore::JSDOMWindow::deletePropertyByIndex): * bindings/js/JSLocationCustom.cpp: (WebCore::JSLocation::deleteProperty): (WebCore::JSLocation::deletePropertyByIndex): LayoutTests: Update / rebaseline existing test to reflect behavior change. * http/tests/security/cross-frame-access-delete-expected.txt: * http/tests/security/cross-frame-access-delete.html: * http/tests/security/resources/cross-frame-iframe-for-delete-test.html: git-svn-id: http://svn.webkit.org/repository/webkit/releases/WebKitGTK/webkit-2.14@205596 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Returning false would only cause its callers to throw in “strict mode”. Implementations however always throw. Always throwing also allows for more implementation flexibility which is somewhat preferable here due to the different cross-origin security architectures in browsers. Fixes #1726.
df75295
to
4b121e9
Compare
… a SecurityError cross-origin
CrossOriginSet has "Perform ? Call(setter, Receiver, «V»)" as step. If that throws, should that exception be masked and turned into a SecurityError too? Is that generally something we need to do? Or if the setter can be accessed we can also leak its exceptions? |
CrossOriginSet can also fail if the descriptor does not have [[Get]] or [[Set]] fields, or if the [[Set]] field is undefined. We can probably simplify the steps of CrossOriginSet by just checking that desc.[[Set]] is not undefined rather than using IsAccessorDescriptor. Thoughts? |
I think this is correct. If the setter can be accessed then it is already on the safelist.
Concretely, this is about trying to set a cross-origin safelisted field that does not have a setter (e.g. I think it would be nicer to follow ES semantics here and ignore that in sloppy mode... let's see what happens in browsers.
Nope, they uniformly deny access even in sloppy mode. So this should be a SecurityError too.
Yeah, looks right to me. So no [[Set]] => SecurityError. |
Do we need to check it is not absent and not undefined then? I guess absent != undefined here unfortunate as that is? Can fix that tomorrow. |
That's true, I think if it's an accessor descriptor then there's always both a [[Get]] and a [[Set]], but [[Set]] could be undefined. Whereas for a data descriptor both [[Get]] and [[Set]] are absent. |
While writing this commit message I realized we could rid ourselves completely of the IsDataDescriptor and IsAccessorDescriptor dependencies if we wanted to. The latter is only used in an assert that could also assert desc.[[Get]] is present. The former could be replaced by a check that desc.[[Value]] is present. (Although, can you have something where desc.[[Writeable]] is present and desc.[[Value]] is not? That would already be problemtic with the current wording.)
|
I don't see much value in minimizing the dependencies, and I think it's generally clearer using those forms than relying on the implicit knowledge that data -> [[Value]]/[[Writable]] and accessor -> [[Get]]/[[Set]]. Commit message needs a "fixes #1726", and I'm not sure why strict mode is in scare quotes. Reviewing the actual patch now... |
OK, this LGTM! |
Can you merge it? Feel free to adjust commit message per your feedback. (I'm assuming you are okay that we removed the dependency on one such ab-op here.) |
https://bugs.webkit.org/show_bug.cgi?id=161397 Reviewed by Ryosuke Niwa. Source/WebCore: [[Delete]] should throw for cross-origin Window / Location objects: - whatwg/html#1728 Firefox and Chrome already throw. Previously, WebKit was merely ignoring the call and logging an error message. No new tests, updated existing test. * bindings/js/JSDOMWindowCustom.cpp: (WebCore::JSDOMWindow::deleteProperty): (WebCore::JSDOMWindow::deletePropertyByIndex): * bindings/js/JSLocationCustom.cpp: (WebCore::JSLocation::deleteProperty): (WebCore::JSLocation::deletePropertyByIndex): LayoutTests: Update / rebaseline existing test to reflect behavior change. * http/tests/security/cross-frame-access-delete-expected.txt: * http/tests/security/cross-frame-access-delete.html: * http/tests/security/resources/cross-frame-iframe-for-delete-test.html: Canonical link: https://commits.webkit.org/179563@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@205200 268f45cc-cd09-0410-ab3c-d52691b4dbfc
…ation objects https://bugs.webkit.org/show_bug.cgi?id=161397 Reviewed by Ryosuke Niwa. Source/WebCore: [[Delete]] should throw for cross-origin Window / Location objects: - whatwg/html#1728 Firefox and Chrome already throw. Previously, WebKit was merely ignoring the call and logging an error message. No new tests, updated existing test. * bindings/js/JSDOMWindowCustom.cpp: (WebCore::JSDOMWindow::deleteProperty): (WebCore::JSDOMWindow::deletePropertyByIndex): * bindings/js/JSLocationCustom.cpp: (WebCore::JSLocation::deleteProperty): (WebCore::JSLocation::deletePropertyByIndex): LayoutTests: Update / rebaseline existing test to reflect behavior change. * http/tests/security/cross-frame-access-delete-expected.txt: * http/tests/security/cross-frame-access-delete.html: * http/tests/security/resources/cross-frame-iframe-for-delete-test.html:
Returning false would only cause its callers to throw in “strict mode”.
Implementations however always throw. Always throwing also allows for
more implementation flexibility which is somewhat preferable here due
to the different cross-origin security architectures in browsers.
Fixes #1726.