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
domenic opened this Issue Jun 15, 2016 · 8 comments

Comments

4 participants
@domenic
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

Use current instead of incumbent + entry in worker constructors
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

Use relevant settings object for pushState/replaceState URL parsing
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

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 30, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@bholley

bholley Jul 1, 2016

Collaborator

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.

Collaborator

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 added a commit that referenced this issue Jul 6, 2016

Use current instead of incumbent + entry in worker constructors
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 added a commit that referenced this issue Jul 6, 2016

Use relevant settings object for pushState/replaceState URL parsing
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

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jul 7, 2016

Collaborator

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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 7, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jul 7, 2016

Collaborator

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".

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 8, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jul 8, 2016

Collaborator

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"

Collaborator

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

Use "current settings object" in the frameElement origin check
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

Use the associated document for pushState/replaceState's origin check
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

Use relevant settings object in protocol handlers
The test at https://url-parsing-entry-gjqursujea.now.sh/entry/entry.html
reveals that Firefox at least uses relevant for URL parsing. Boris says
that Firefox does something nonsensical for the origin check and that he
would prefer relevant or current.

Code inspection reveals that Blink and WebKit use relevant for both URL
parsing and origin checking:

- https://chromium.googlesource.com/chromium/src/+/ee94bde91c72a046bec15436d56cfe32bb0e524c/third_party/WebKit/Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp#71
- https://github.com/WebKit/webkit/blob/83624b838d4fa81df77060c51b09587169f8ff19/Source/WebCore/Modules/navigatorcontentutils/NavigatorContentUtils.cpp#L109

Edge does not support these methods.

Helps with #1431.

domenic added a commit that referenced this issue Jul 13, 2016

Remove unnecessary and unimplemented canvas tainting when painting text
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

Remove unnecessary and unimplemented canvas tainting when painting text
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

Use "current settings object" in the frameElement origin check
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 18, 2016

Use the associated document for pushState/replaceState's origin check
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 18, 2016

Use relevant settings object in protocol handlers
The test at https://url-parsing-entry-gjqursujea.now.sh/entry/entry.html
reveals that Firefox at least uses relevant for URL parsing. Boris says
that Firefox does something nonsensical for the origin check and that he
would prefer relevant or current.

Code inspection reveals that Blink and WebKit use relevant for both URL
parsing and origin checking:

- https://chromium.googlesource.com/chromium/src/+/ee94bde91c72a046bec15436d56cfe32bb0e524c/third_party/WebKit/Source/modules/navigatorcontentutils/NavigatorContentUtils.cpp#71
- https://github.com/WebKit/webkit/blob/83624b838d4fa81df77060c51b09587169f8ff19/Source/WebCore/Modules/navigatorcontentutils/NavigatorContentUtils.cpp#L109

Edge does not support these methods.

Helps with #1431.

domenic added a commit that referenced this issue Jul 19, 2016

Use only the incumbent global in postMessage
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 #1542. Helps #1431 but hurts #1430.

domenic added a commit that referenced this issue Jul 20, 2016

Use only the incumbent global in postMessage
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 #1542. Helps #1431 but hurts #1430.

domenic added a commit to domenic/ServiceWorker that referenced this issue Aug 26, 2016

Parse URLs relative to the relevant settings object, not entry
Fixes #922. Web platform tests at web-platform-tests/wpt#3449. Part of the overall effort in whatwg/html#1431.

In the process, fixes some ambiguity about how to parse URLs in the Link header, as previous it parsed relative to the entry settings object, which did not exist during this algorithm.

domenic added a commit to domenic/ServiceWorker that referenced this issue Sep 6, 2016

Parse URLs relative to the relevant settings object, not entry
Fixes #922. Web platform tests at web-platform-tests/wpt#3449. Part of the overall effort in whatwg/html#1431.

In the process, fixes some ambiguity about how to parse URLs in the Link header, as previous it parsed relative to the entry settings object, which did not exist during this algorithm.
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk
Member

annevk commented Nov 22, 2016

https://notifications.spec.whatwg.org/ uses entry a ton too for which I filed whatwg/notifications#86.

domenic added a commit that referenced this issue Dec 5, 2016

Allow <a>/<area> with download="" to not require user activation
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

Allow <a>/<area> with download="" to not require user activation
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment