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

Which origins to use in postMessage? #1542

Closed
domenic opened this issue Jul 12, 2016 · 5 comments
Closed

Which origins to use in postMessage? #1542

domenic opened this issue Jul 12, 2016 · 5 comments

Comments

@domenic
Copy link
Member

domenic commented Jul 12, 2016

This is part of #1431 and #1430.

postMessage current uses, per spec:

  • the entry settings object is used for an origin check if targetOrigin is "/"
  • the incumbent settings object to determine the origin and source attributes of the resulting MessageEvent.

I'm having a hard time with black-box testing of these cases, so I'm trying to fall back to code inspection.

Code inspection of Blink reveals that Blink uses the current settings object for both of these: https://chromium.googlesource.com/chromium/src/+/540e575dc43e718821bc4ac682735a9631e33c1f/third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp#176

WebKit uses "callerDOMWindow": https://github.com/WebKit/webkit/blob/152410da081c1a638a3c2b8c0f55b5e726fffd15/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp#L552. I'm not sure what this is supposed to be exactly.

Gecko appears to follow the current spec and use both incumbent and entry: http://searchfox.org/mozilla-central/rev/c44d0b1673fef5e0e2e19fa82d6780a74c186151/dom/base/nsGlobalWindow.cpp#8255, although in an offline email @bzbarsky said

I don't see why entry settings is any better than current settings here, really, assuming the current settings bits get sorted out in such a way that we can use current settings for the source origin here.


With this info in hand I'd say the simplest thing to do here is to converge on current for both. @bzbarsky, do you agree? When I quoted you above you were only referring to the "/" thing, but having them be inconsistent seems bad...

This feels like a classic case of enough browsers disagreeing so that we get to just pick the behavior we think is most reasonable.

@bzbarsky
Copy link
Contributor

If Bobby is fine with using current for the origin (that is, if the various document.domain and cross-origin-objects issues have been resolved to make that do some approximation of the right thing), then that sounds fine.

It's not clear to me to what extent UAs implement the currently-specced cross-origin-objects setup, and use of current here would definitely depend on that setup being faithfully enough implemented; otherwise you would get security bugs. Might be worth calling that out in the spec.

@bzbarsky
Copy link
Contributor

//cc @bholley

@bholley
Copy link

bholley commented Jul 19, 2016

Using current for source is pretty bizarre, no? It would mean that same-origin postMessage would always see the source as the receiver of the message, rather than the sender.

If Blink really does set source = current that's strong evidence of web-compatibility, but it seems pretty surprising and we might as well avoid it if we can. I wouldn't keep the incumbent concept around just for this, but if we need it for referrer anyway, I think we should use incumbent here.

@domenic
Copy link
Member Author

domenic commented Jul 19, 2016

Ah, interesting. This probably works in Chrome because Chrome's window properties creation is currently broken; it creates realm-appropriate representations for all windows, instead of only creating realm-appropriate representations for cross-origin windows. I wonder what it uses to determine the "caller" in that case, and if it matches incumbent. (My guess is that it does except in edge cases.)

I can do some more black-box testing (since source should actually be pretty easy to test), but unless I find surprising results I guess it makes sense to stick with incumbent here.

You're OK with switching to current for the "targetOrigin is is /" case? Or maybe we should switch to incumbent there; I imagine that's (perhaps approximately) what WebKit is doing with its callerDOMWindow, and in effect what Chrome is doing with its inappropriate extra realm-appropriate wrappers. In general both Chrome and WebKit kind of assume there's a single window which gets passed along through the algorithms and is used for all the checks, and I can't blame them; that does seem like a more reasonable model.

@bholley
Copy link

bholley commented Jul 19, 2016

If we're using incumbent for source we might as well use it for everything else here, so that the implementation can just get one thing and pass it around, which reduces the chances of bugs.

We should definitely write WPT tests for this stuff to preserve the blackbox testing work that's happening. :-)

domenic added a commit that referenced this issue Jul 19, 2016
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.
domenic added a commit that referenced this issue Jul 19, 2016
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
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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants