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

Minimize usage of the entry concept #1431

Open
26 of 31 tasks
domenic opened this issue Jun 15, 2016 · 9 comments
Open
26 of 31 tasks

Minimize usage of the entry concept #1431

domenic opened this issue Jun 15, 2016 · 9 comments

Comments

@domenic
Copy link
Member

domenic commented Jun 15, 2016

This is a tracking bug, both for HTML and for the wider web ecosystem, to see if we can minimize the number of places that use the entry global/settings object/realm. Originally this was https://www.w3.org/Bugs/Public/show_bug.cgi?id=27203.

Here is our HTML checklist:

Fetch:

Service Worker:

XHR (whatwg/xhr#98):

Notifications (whatwg/notifications#86):

If you have other specs that use the entry concept, comment here and I will update the OP.

@domenic domenic self-assigned this Jun 15, 2016
domenic added a commit that referenced this issue Jun 29, 2016
As shown by https://settings-object-worker-client-abibrisfdk.now.sh, all
browsers use the current settings object for the fetch client, i.e. the
"outside settings", instead of the incumbent settings object. (Except we
do not know the result in Edge, since they don't seem to send a Referer
header for us to inspect.) Part of #1430.

As shown by
https://settings-object-worker-client-wellwosyqf.now.sh/entry/entry.html,
all browsers also use the current settings object for URL resolution,
instead of the entry settings object. Part of #1431.
domenic added a commit that referenced this issue Jun 30, 2016
Tested by https://url-parsing-entry-iaqyojravh.now.sh/entry/entry.html;
all browsers use relevant, instead of entry, at least for URL parsing.
Further testing is needed for the origin checks.

Part of #1431.
@domenic
Copy link
Member Author

domenic commented Jun 30, 2016

I've investigated most of the "easy" ones. Many of the rest have to do with origin checks.

@bzbarsky @bholley @annevk, would you be able to help check my logic on the origin check things? window.frameElement is a representative and simple example. It says

If container's node document's origin is not same origin-domain with the entry settings object's origin, then return null and abort these steps.

My usual trick of setting up something a test similar to https://html.spec.whatwg.org/multipage/webappapis.html#realms-settings-objects-global-objects does not work because I can't call frames[0].hello() cross-origin.

My tentative conclusion is that in cross-origin situations all four must always be same-origin, and thus interchangeable for origin checks.

  • When testing something not in the whitelist of cross-origin-accessible properties (for example, window.frameElement or document.open or ImageBitmap), entry, incumbent, current, and relevant must all be same-origin. Entry and incumbent because otherwise you can't call incumbent's frames[0].hello() from entry. Incumbent and current since otherwise incumbent couldn't access current.frameElement or current.document.open or current.ImageBitmap. And current and relevant since otherwise "perform a security check" will immediately fail.
  • When testing something on the whitelist (postMessage, location stuff), entry must be same-origin with incumbent because otherwise you can't call incumbent's frames[0].hello(). Incumbent and current since anything on the whitelist creates an origin-appropriate representation. And current and relevant since otherwise "perform a security check" will immediately fail.

Does this seem correct? I'm not so sure on the incumbent ~ entry correspondence, but the other correspondences seem right.

If that's correct we can just replace them all with current or relevant, which is nice.

@bholley
Copy link

bholley commented Jul 1, 2016

In general a lot of these differences are observable in the non-revoking document.domain case. However, since Gecko revokes on document.domain, it is unlikely that much/anything on the Web would depend on the behavior there.

annevk pushed a commit that referenced this issue Jul 6, 2016
As shown by https://settings-object-worker-client-abibrisfdk.now.sh, all
browsers use the current settings object for the fetch client, i.e. the
"outside settings", instead of the incumbent settings object. (Except we
do not know the result in Edge, since they don't seem to send a Referer
header for us to inspect.) Part of #1430.

As shown by
https://settings-object-worker-client-wellwosyqf.now.sh/entry/entry.html,
all browsers also use the current settings object for URL resolution,
instead of the entry settings object. Part of #1431.
annevk pushed a commit that referenced this issue Jul 6, 2016
Tested by https://url-parsing-entry-iaqyojravh.now.sh/entry/entry.html;
all browsers use relevant, instead of entry, at least for URL parsing.
Further testing is needed for the origin checks.

Part of #1431.
@bzbarsky
Copy link
Contributor

bzbarsky commented Jul 7, 2016

My tentative conclusion is that in cross-origin situations all four must always be same-origin

No, they must be same-effective-script-origin, generally (ignoring cross-origin-accessible stuff like window.postMessage). That's not the same as same-origin. And as Bobby says, they may not even be same-effective-script-origin in the revoking document.domain case.

@domenic
Copy link
Member Author

domenic commented Jul 7, 2016

Hmm. Would anyone be able to help me by outlining how to make a test for, say, window.frameElement that distinguishes these different cases? I guess we'd want to create a setup where incumbent, entry, relevant, and current are all different-origin (but are same-effective-script-origin), and I'm not sure how to do that.

@bzbarsky
Copy link
Contributor

bzbarsky commented Jul 7, 2016

You serve some of your stuff from foo.bar.com and some from baz.bar.com and set document.domain in both to "bar.com".

@domenic
Copy link
Member Author

domenic commented Jul 8, 2016

OK. Can someone check that I set up this test right? https://frame-element-entry-feolpgggwv.now.sh/, with source at https://github.com/domenic/browser-quirk-tests/tree/master/frame-element with a subfolder for each origin.

I believe what this shows is that frameElement does in fact depend on the entry settings object. I have made the current, relevant, and incumbent pages all document.domain themselves to have now.sh as their origin's domain. I have left entry to have an origin of https://frame-element-entry-feolpgggwv.now.sh/. Clicking on the test button then produces a security error, presumably according to the spec's

If container's node document's origin is not same origin-domain with the entry settings object's origin, then return null and abort these steps.

In this case container's node document should be that of the incumbent global.

(This occurs in Firefox, Edge, and Chrome.)

This is a bit surprising to me, since previously @bzbarsky indicated that he thought that frameElement was probably wrong in using entry, and I wasn't able to find a trace of entry usage in Chrome's implementation. (In fact, what I was able to find seems at least to suggest it's checking current, not entry. But V8 has known issues with creating cross-frame property descriptors...)

So it seems quite possible I've made a mistake in my setup. Could anyone take some time to confirm my findings? I can't figure out any way that this security error would consistently occur, given that everything except entry is same origin-domain, unless entry were involved in the security check.

@bzbarsky
Copy link
Contributor

bzbarsky commented Jul 8, 2016

Clicking on the test button then produces a security error

You're seeing a security error because when the test case does frames[0].test (note we're just doing the Get() so far, not even the call) we end up in https://html.spec.whatwg.org/#windowproxy-getownproperty which tests false in step 3 because the thing inside the iframe set document.domain but the thing outside did not. We then go to https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-) and "test" is not in the list at https://html.spec.whatwg.org/#crossoriginproperties-(-o-) so we return undefined. Back in https://html.spec.whatwg.org/#windowproxy-getownproperty we have undefined in step 5, go on to step 6, "test" is not a child browsing context name, we go to step 7 and throw a SecurityError. The "test" function is never called, nor is the frameElement getter.

The actual exception message in the SecurityError does make this somewhat clearer in Firefox than in Chrome:

Permission denied to access property "test"

domenic added a commit to domenic/browser-quirk-tests that referenced this issue Jul 12, 2016
domenic added a commit that referenced this issue Jul 12, 2016
Since this is a same origin-domain check, we can use any settings
object, as they are all same origin-domain. Part of #1431.
domenic added a commit that referenced this issue Jul 12, 2016
This updates the origin check in pushState/replaceState to use the
origin of the document of the relevant History object, instead of that
of the entry settings object. This more correctly matches 2/3 open
source browsers:

- https://chromium.googlesource.com/chromium/src/+/c21f0b11ac83ea970d0eaf6a0b223d48a32a4b32/third_party/WebKit/Source/core/frame/History.cpp#234
- https://github.com/WebKit/webkit/blob/0ee7b606dbf35d9688c15b19b1a83ec1ff242cd7/Source/WebCore/page/History.cpp#L150

(Gecko does no such security check). It also helps with #1431.

While there, cleaned up some redundant steps and tightened wording.
domenic added a commit that referenced this issue Jul 12, 2016
domenic added a commit that referenced this issue Jul 13, 2016
As discussed in #1540, this check does not give any additional
protections over those already provided by CORS, which these days fonts
are subject to.

Fixes #1540. Helps with #1431.
domenic added a commit that referenced this issue Jul 14, 2016
As discussed in #1540, this check does not give any additional
protections over those already provided by CORS, which these days fonts
are subject to.

Fixes #1540. Helps with #1431.
domenic added a commit that referenced this issue Jul 18, 2016
Since this is a same origin-domain check, we can use any settings
object, as they are all same origin-domain. Part of #1431.
domenic added a commit that referenced this issue Dec 5, 2016
Fixes #2116, wherein it is documented that this restriction is not
supported in any existing engines and attempting to do so in WebKit
proved not web-compatible.

Also helps with #1431.
domenic added a commit that referenced this issue Dec 6, 2016
Fixes #2116, wherein it is documented that this restriction is not
supported in any existing engines and attempting to do so in WebKit
proved not web-compatible.

Also helps with #1431.
littledan added a commit to littledan/html that referenced this issue Dec 10, 2018
Hopefully this will help discourage people from using them.

Also remove "entry global object" as it seemed to be unused.

See whatwg#1430/whatwg#1431
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
As shown by https://settings-object-worker-client-abibrisfdk.now.sh, all
browsers use the current settings object for the fetch client, i.e. the
"outside settings", instead of the incumbent settings object. (Except we
do not know the result in Edge, since they don't seem to send a Referer
header for us to inspect.) Part of whatwg#1430.

As shown by
https://settings-object-worker-client-wellwosyqf.now.sh/entry/entry.html,
all browsers also use the current settings object for URL resolution,
instead of the entry settings object. Part of whatwg#1431.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Tested by https://url-parsing-entry-iaqyojravh.now.sh/entry/entry.html;
all browsers use relevant, instead of entry, at least for URL parsing.
Further testing is needed for the origin checks.

Part of whatwg#1431.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
As discussed in whatwg#1540, this check does not give any additional
protections over those already provided by CORS, which these days fonts
are subject to.

Fixes whatwg#1540. Helps with whatwg#1431.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Since this is a same origin-domain check, we can use any settings
object, as they are all same origin-domain. Part of whatwg#1431.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
This updates the origin check in pushState/replaceState to use the
origin of the document of the relevant History object, instead of that
of the entry settings object. This more correctly matches 2/3 open
source browsers:

- https://chromium.googlesource.com/chromium/src/+/c21f0b11ac83ea970d0eaf6a0b223d48a32a4b32/third_party/WebKit/Source/core/frame/History.cpp#234
- https://github.com/WebKit/webkit/blob/0ee7b606dbf35d9688c15b19b1a83ec1ff242cd7/Source/WebCore/page/History.cpp#L150

(Gecko does no such security check). It also helps with whatwg#1431.

While there, cleaned up some redundant steps and tightened wording.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Previously one of the origin checks was performed with the entry
settings object, while the origin and source attributes of the resulting
MessageEvent were derived from the incumbent settings object. At least
WebKit and Blink appear to use the same global for both, and it makes
sense to align the checks on the same global.

The difference is only observable in test cases that fiddle with
document.domain, as entry and incumbent are always same origin-domain
(but, in document.domain cases, not always same origin).

Fixes whatwg#1542. Helps whatwg#1431 but hurts whatwg#1430.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Fixes whatwg#2116, wherein it is documented that this restriction is not
supported in any existing engines and attempting to do so in WebKit
proved not web-compatible.

Also helps with whatwg#1431.
domenic added a commit that referenced this issue May 5, 2021
This matches implementations, and helps with #1431.

Also, slightly modernize the algorithm.
domenic added a commit that referenced this issue May 5, 2021
That is, check the type of response directly, instead of synthesizing an opaque origin for opaque responses and then comparing that to the entry settings object's origin.

This helps with #1431 by removing various uses of the entry concept, and closes #2761 by removing the origin concept for image and media elements entirely, since it is now unused.
@domenic
Copy link
Member Author

domenic commented May 5, 2021

With #6655 and #6656 we almost have the spec matching the cases I see in Chromium codesearch, i.e. Location, document.open()/document.write(), and window.open().

What remains is the spec uses the entry settings object for hyperlink downloading, which needs to be rebased on top of navigation in general.

domenic added a commit that referenced this issue May 6, 2021
That is, check the type of response directly, instead of synthesizing an opaque origin for opaque responses and then comparing that to the entry settings object's origin.

Closes #2813. Helps with #1431 by removing various uses of the entry concept, and closes #2761 by removing the origin concept for image and media elements entirely, since it is now unused.
domenic added a commit that referenced this issue May 6, 2021
This matches implementations, and helps with #1431.

Also, slightly modernize the algorithm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants