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

[COOP Reporting] "the environment's top-level browsing context" #4

Open
domenic opened this issue May 29, 2020 · 15 comments
Open

[COOP Reporting] "the environment's top-level browsing context" #4

domenic opened this issue May 29, 2020 · 15 comments

Comments

@domenic
Copy link
Contributor

domenic commented May 29, 2020

https://github.com/camillelamy/explainers/blob/master/coop_reporting.md#windowproxy-changes uses two phrases

WindowProxy's top level browsing context's

and

the environment's top-level browsing context

The first of these is well-defined (in that WindowProxys are 1:1 with browsing contexts and every browsing context has a TLBC). The second is less clear to me. Is it just meant to be a synonym for the first, or something else?

@domenic
Copy link
Contributor Author

domenic commented May 29, 2020

The phrase "the environment" is also used later as something to pass to the report. There, I am guessing it refers to the script URL, column number, and line number. But that usage seems pretty different from the one I quote above...

@camillelamy
Copy link
Owner

Basically, I need to look at two top-level browsign contexts. The first one is the WindowProxy's (where the access is taking place). The second one, which I tried to phrase as the environment, is the one doing the access in the first place. IIUC, the environment is where the JavaScript execution is taking place. Maybe it's clearer if I phrase this as: if the environment is related to a document, this document TLBC?

@domenic
Copy link
Contributor Author

domenic commented Jun 2, 2020

Hmm, I see. That's a bit tricky...

We have a concept for referring to that: the incumbent settings object/global object/Realm. (From which you could get a WindowProxy and then browsing context.) See whatwg/html#1430. The simplest example of this in action is that it's used to set the source property of a MessageEvent created when doing otherWindow.postMessage(...).

However, we consider that concept deprecated, and are trying to remove all usages of it from the platform, since call-site-dependent APIs are tricky to reason about. Probably MessageEvent will have to stick around, and some others, but it's still something we'd like to minimize...

Also of note, I'm currently in a discussion with @syg on the V8 team as to whether we should even support that concept when new JS features, like promises or weak references, are combined with JavaScript's built-in fn.bind(). (He says we shouldn't, since it's deprecated and requires some tricky implementation legwork. I say we should for consistency; it's weird if postMessage() gives an inaccurate source value when you combine bind() with newer JS features.)

This seems like a pretty legitimate use case, however, so I'm not sure what to do...

@camillelamy
Copy link
Owner

Well the whole point of the reporting API is to track call sites that make problematic accesses to WindowProxy, so it is by essence call site dependent :).

@domenic
Copy link
Contributor Author

domenic commented Jun 2, 2020

Yeah, I agree. So I suggest using incumbent here, in particular, "the incumbent global object's browsing context". Note that that can be null if you are running script from a disposed window, e.g. a function defined in an iframe that was removed from the document, so you'll need to handle that case.

I hope also @syg can take this into account in his decisions about whether to support incumbent going forward. Since COOP reporting seems pretty important to partners, and it sounds like it will rely on it, that seems like good input. Otherwise, probably we'll need a caveat in any developer documentation about COOP reporting that it doesn't work with bind() and promises/weak references/etc.

@syg
Copy link

syg commented Jun 2, 2020

Interesting data point, thanks for the cc. A compelling use case for incumbents beyond existing ones would certainly help sway my viewpoint. I am unfortunately rather ignorant on COOP/COEP beyond their high-level goals. What's the hazard when COOP reporting doesn't work because a bound function was passed? Did the app make itself vulnerable in a really subtle way? Error?

@camillelamy
Copy link
Owner

The goal of COOP is to isolate pages. We do this by putting windows in other browsing context group where they can't access each other. The problem is that this is potentially breaking a lot of websites, so to make it possible to deploy COOP we want to deploy a reporting API that warns developers that some of the accesses they were making to a WindowProxy don't work because of COOP. To make the API usable, we would like to point to where the access happened in the JavaScript.

If this is wrong, we risk either not detecting the access at all or sending a completely wrong information regarding the script that triggered it. That makes COOP a lot harder to deploy. And we really want people do deploy COOP to secure their websites, and to eventually gain access to more powerful APIs gated behind COOP (eg SharedArrayBuffer on Android, performance.memoryMeasurement). Our conversations with developers surfaced that having a robust reporting API is critical to getting COOP deployed.

@mikewest - you were interested in his subject.

@annevk
Copy link

annevk commented Jul 6, 2020

Why can this not use current global object similar to IsPlatformObjectSameOrigin?

@camillelamy
Copy link
Owner

Does the current global object give you access to both the top level origin of the page and the origin of the document in which the JavaScript is executing? We do need both to exclude scripts executed by cross-origin iframes from generating reports.

@annevk
Copy link

annevk commented Jul 10, 2020

If you use the current settings object, yes. There are some cases where that is different from the incumbent, but those cases would also be handled differently by IsPlatformObjectSameOrigin which I'd consider an equivalent check.

@domenic
Copy link
Contributor Author

domenic commented Jul 10, 2020

After walking through the spec for a bit (and documenting my findings in whatwg/html#5724), I agree with @annevk that the current settings object works here. Normally it doesn't correspond to the notion of "caller" in a useful way, but if you're intercepting things as early as [[Get]]/[[Set]], then it does.

Incumbent might provide a slightly-more-useful notion of caller than current in edge cases. E.g. I believe that if you did something like

const f = coopedWindow.alert.bind(coopedWindow, "blah");

f();

then incumbent would be the caller of f, whereas current would be coopedWindow. So the report would be a bit inaccurate. But that's OK for an edge case.

(I think this situation can occur if coopedWindow is cross-origin but same-site and uses document.domain to make coopedWindow.alert accessible to the caller. In this case you would need to necessarily not be using COEP, since them in combination would create cross-origin isolation and disable document.domain. So... rather edge-casey, indeed.)

@camillelamy
Copy link
Owner

I have updated the explainer to use the current global object rather than the incumbent one.

@camillelamy
Copy link
Owner

Update on this, @ArthurSonzogni has been working on the implementation on the Chrome side, and we wonder if we shouldn't use the incumbent global object instead of the current one, as it seems we would using the wrong object depending on the way the setter/getter is called. For example, we were considering the following case:

w1 = window;
w2 = window[0]; // A child window
w3 = window; // Another window
F2 = w2.postMessage; // w2's function
F1 = function(w, F) { F.call(w, "message"); }
F1(w3, F2); // F2.call(w3, "message")

In this case, IIUC, the current global object is w2's and the incumbent one is w1's. Since the whole thing is called from w1, in this case we want a report for w1 and not w2.

@ArthurSonzogni
Copy link
Contributor

Update on this, @ArthurSonzogni has been working on the implementation on the Chrome side, and we wonder if we shouldn't use the incumbent global object instead of the current one, as it seems we would using the wrong object

Relevant discussion we had:
https://chromium-review.googlesource.com/c/chromium/src/+/2323250/2/third_party/blink/renderer/core/frame/dom_window.cc#453

@domenic
Copy link
Contributor Author

domenic commented Jul 29, 2020

I'm not sure I quite understand. In the line

const F2 = w2.postMessage;

this will perform a [[Get]]. At the time the [[Get]] algorithm is running, the current global object will be w1. The current global object only changes once the algorithm for postMessage() starts running, i.e. several steps into the F2.call(w3, "message") line, after we've left the [[Get]] section of the spec and once we've gone into the window post message steps part.

The reason this doesn't work in the Chromium CL might be because of the hacky implementation of cross-origin properties in Blink. Using incumbent in Blink might be a reasonable workaround, with a TODO to clean that up once cross-origin properties are cleaned up.

But, I am not sure, since you tried https://chromium-review.googlesource.com/c/chromium/src/+/2081633 and didn't see a difference. That means either:

My guess from some initial code inspection is that it's latter case. In what I see currently is particular ReportCoopAccess is called from LocationAttributeGet and OpenerAttributeSet, which do not correspond to the general [[Get]] hook, but instead correspond to the location attribute getter steps and the opener attribute getter steps. Inside those parts of the spec, current would indeed not work. But I can't be sure in this diagnosis, because your example is about w2.postMessage, and I don't see where in the Chromium codebase that would be impacted; I only see code for location and opener.

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

No branches or pull requests

5 participants