Skip to content
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

Should deleting a cross-origin Window property throw? #1726

Closed
cdumez opened this issue Aug 29, 2016 · 11 comments
Closed

Should deleting a cross-origin Window property throw? #1726

cdumez opened this issue Aug 29, 2016 · 11 comments
Assignees

Comments

@cdumez
Copy link

cdumez commented Aug 29, 2016

Should deleting a cross-origin Window property throw?

Firefox and Chrome throw but it I cannot find the part of the specification that says we should:

https://html.spec.whatwg.org/#windowproxy-delete

7.4.9 [[Delete]] ( P )

If P is an array index property name, return false.

Let W be the value of the [[Window]] internal slot of this.

If IsPlatformObjectSameOrigin(W) is true, then return ? OrdinaryDelete(W, P).

Return false.

@cdumez
Copy link
Author

cdumez commented Aug 29, 2016

@annevk @domenic

@domenic
Copy link
Member

domenic commented Aug 29, 2016

I believe how this works is that ES defines its internal methods to return booleans, and then various consumers of them (e.g. the delete operator or Reflect.deleteProperty) will consume those booleans and sometimes throw. In particular, per https://tc39.github.io/ecma262/#sec-delete-operator-runtime-semantics-evaluation step 5.d, an error is thrown in strict mode, but (per 5.e) false is returned in sloppy mode.

@cdumez
Copy link
Author

cdumez commented Aug 29, 2016

Note that http://w3c-test.org/html/browsers/origin/cross-origin-objects/cross-origin-objects.sub.html seems to expect us to throw. It does not seem to be in strict mode. Firefox is passing.

@domenic
Copy link
Member

domenic commented Aug 29, 2016

/cc @bzbarsky. Is throwing for cross-origin delete in sloppy mode expected or desirable?

We could make it throw, by throwing a TypeError instead of returning false. That seems a bit unexpected from a JS developer's perspective, but it wouldn't be the worst thing.

@cdumez
Copy link
Author

cdumez commented Aug 29, 2016

Someone should double check but it seems Chrome throws a SecurityError.

@bzbarsky
Copy link
Contributor

@bholley knows this stuff better than I do. But afaict in Firefox any operation on a property on a cross-origin window first asks a general "can I operate on this property?" question and that will throw for all properties that are not in the whitelisted set.

I don't know that I have a strong opinion about whether to throw in this situation or not.

@bzbarsky
Copy link
Contributor

Oh, and I guess in Gecko the question is actually "can I perform verb X on this property?" where the possible verbs are get, set, call, enumerate, get property descriptor. delete uses the "set" verb at the moment.

Fundamentally, if foo.bar = undefined throws (even in non-strict mode), then it feels like delete foo.bar should throw too.

@bholley
Copy link

bholley commented Aug 30, 2016

The basic idea when we hashed this out was that we would generally err on the side of throwing, since it gives engines maximum implementational flexibility (and diminishes web-compat constraints), and we're not really trying to make these objects friendly to operate on.

I don't really remember the specifics of |delete|, but it doesn't surprise me that we decided it should throw.

The implementation in Firefox is here: http://searchfox.org/mozilla-central/rev/0dd403224a5acb0702bdbf7ff405067f5d29c239/js/xpconnect/wrappers/FilteringWrapper.cpp#235

@annevk
Copy link
Member

annevk commented Aug 30, 2016

https://tc39.github.io/ecma262/#sec-invariants-of-the-essential-internal-methods says [[Delete]] must return a Boolean, but I think that section also allows exceptions although it's not explicitly stated. [[Delete]] can certainly throw already anyway.

So I guess we should change this if implementations already throw. And we should probably test indexed properties before doing that. It seems the test does not cover them.

@annevk
Copy link
Member

annevk commented Aug 30, 2016

If we change this we should also change https://html.spec.whatwg.org/multipage/browsers.html#location-delete.

@annevk
Copy link
Member

annevk commented Aug 30, 2016

Update for the test: web-platform-tests/wpt#3610. I'll make a PR for HTML.

@annevk annevk self-assigned this Aug 30, 2016
annevk added a commit that referenced this issue Aug 30, 2016
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.
annevk added a commit that referenced this issue Sep 13, 2016
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.
domenic pushed a commit that referenced this issue Sep 14, 2016
Returning false for [[Delete]] (on Window and Location objects) would
only cause its callers to throw in strict mode. Implementations
however always throw.

We also decided to throw "SecurityError" for [[DefineOwnProperty]]
and [[Set]] (the latter through CrossOriginSet). We did not do this
for all internal methods: only those where throwing was unique to
their cross-origin behavior.

Fixes #1726.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Returning false for [[Delete]] (on Window and Location objects) would
only cause its callers to throw in strict mode. Implementations
however always throw.

We also decided to throw "SecurityError" for [[DefineOwnProperty]]
and [[Set]] (the latter through CrossOriginSet). We did not do this
for all internal methods: only those where throwing was unique to
their cross-origin behavior.

Fixes whatwg#1726.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants