camillelamy / explainers

11 stars 5 forks source link

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

Open domenic opened 4 years ago

domenic commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 https://github.com/whatwg/html/issues/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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

camillelamy commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

After walking through the spec for a bit (and documenting my findings in https://github.com/whatwg/html/pull/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 commented 4 years ago

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

camillelamy commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.