szager-chromium commented 3 years ago

Closes #429

szager-chromium commented 3 years ago

Disclaimer: I don't remember seeing "browsing context scope origin" before today, so I might be misunderstanding. @annevk could potentially help us out.

But, I don't think this is right. This basically turns off IntersectionObserver in cross-origin iframes, I think, since nothing in a cross-origin iframe will have a browsing context scope origin. If I understand correctly.

I think you want to use "relevant agent" instead. That will allow cross-origin same-site relations in cases where they could potentially synchronously script each other using document.domain, but that seems OK... if you could synchronously script the other frame then I don't think we'd want to block IntersectionObserver from working.

I don't think relevant agent is right. This requirement has nothing to do with scriptability, which is the purpose of "relevant realm." This is a security restriction to prevent cross-origin iframes from probing geometry information about their ancestors. For security-related features, I think origin is the right term. To quote the spec:

"Origins are the fundamental currency of the web's security model. Two actors in the web platform that share an origin are assumed to trust each other and to have the same authority. Actors with differing origins are considered potentially hostile versus each other, and are isolated from each other to varying degrees."

That pretty well describes the purpose of the restrictions on root margin and root bounds.

And to be clear: this change doesn't disable IntersectionObserver for cross-origin iframes. IntersectionObserver will still work, but with these limitations:

Any rootMargin set in the IntersectionObserver constructor will be ignored.
IntersectionObserverEntry will not have its rootBounds field populated.

domenic commented 3 years ago

Sure, origin is the ideal security boundary. However, you can synchronously inspect all the rectangles of the other realm, if you're in the same agent. (Or just do new otherWindow.IntersectionObserver(...).) So it's what we've been using for cases like this.

this change doesn't disable IntersectionObserver for cross-origin iframes

OK; I didn't understand the full implication here. Either way, the change to origin (and in particular checking the ancestor chain's origins) is very different from the restrictions that the unit-of-related-same-origin-browsing-contexts version imposed. Is this intentionally meant to be a normative change to add more restrictions, or is it meant to just modernize the terminology?

szager-chromium commented 3 years ago

Either way, the change to origin (and in particular checking the ancestor chain's origins) is very different from the restrictions that the unit-of-related-same-origin-browsing-contexts version imposed. Is this intentionally meant to be a normative change to add more restrictions, or is it meant to just modernize the terminology?

This is just meant to modernize the terminology. I'm pretty sure that all of the browsers already conform to the new language; there are web platform tests that verify this behavior for cross-origin iframes.

domenic commented 3 years ago

Hmm. Well, the modern version of URSOBC is agent, so if that's the case then I'd suggest using agent.

Otherwise, it'd be good to point to the web platform tests for the normative differences here, e.g.:

Testing cross-origin same-site cases (unrestricted with URSOBC, restricted with this PR)
Testing origin A embeds origin B embeds origin A cases (unrestricted with URSOBC, restricted with this PR)

szager-chromium commented 3 years ago

Hmm. Well, the modern version of URSOBC is agent, so if that's the case then I'd suggest using agent.

Otherwise, it'd be good to point to the web platform tests for the normative differences here, e.g.:

Testing cross-origin same-site cases (unrestricted with URSOBC, restricted with this PR)

Testing origin A embeds origin B embeds origin A cases (unrestricted with URSOBC, restricted with this PR)

I don't see that A-B-A would be unrestricted with URSOBC. The old spec says:

"The transitive closure of all the browsing contexts that are directly reachable browsing contexts forms a unit of related browsing contexts."

It then goes on to define URSOBC as a URBC with the property that all member contexts have similar origin. By that definition, the two A frames would not be in the same URSOBC.

I uploaded a new test to enforce the same-site cross-origin behavior:

https://chromium-review.googlesource.com/c/chromium/src/+/2411229

annevk commented 3 years ago

Those As are directly reachable though... Is target always an element? I guess this relates to the third party discussion and you basically want the cookie-definition of third party, except scoped to origins?

cc @emilio

domenic commented 3 years ago

I just don't understand why this spec needs such a special definition, unlike every other JS API on the web platform that uses agents.

annevk commented 3 years ago

Well, given the use case of ensuring you are not being spoofed it makes sense to not treat A2 in A1 -> B -> A2 as same-origin with A1 as that might make things rather easy for B.

szager-chromium commented 3 years ago

I only just remembered that the A-B-A situation came up previously:

https://github.com/w3c/IntersectionObserver/issues/409

The test mentioned in that bug currently fails on chromium and passes on firefox and webkit:

https://wpt.fyi/results/intersection-observer/same-origin-grand-child-iframe.sub.html?label=experimental&label=master&aligned

That being the case, we should probably consider it from first principles and decide what makes sense, without worrying about changing behavior; one way or another, behavior is going to change.

szager-chromium commented 3 years ago

Thinking about this a bit more, I suppose it would make sense to codify the firefox/webkit behavior (i.e., allow rootMargin and rootBounds for A-B-A). The purpose of restricting these options is to prevent probing the geometry of a cross-origin parent frame. But it's pointless trying to prevent the leaf A frame from probing geometry of the B frame, because the top-level A frame knows everything already.

What do y'all think?

annevk commented 3 years ago

161 has prior discussion on exactly this. I discussed bz's question there with @emilio and he could not come up with a scenario where it would leak information about any intermediate frames, be it A-B-A or A-B-C-A.

What #161 also has discussion is on what check needs to be made here. Same origin or same origin-domain. And there should also be a same site test written as Firefox appears to do a schemeless same site check in its code, which is not what we want here long term.

emilio commented 3 years ago

I'm trying to understand what is what we're trying to prevent if we don't want a regular origin/origin-domain check. So for the A1-B-C-A2 case, allowing IntersectionObservers from A2 to observe the implicit root bounds is never problematic, I think, right? They can already be queried by script.

If so, the question is what information having a rootMargin gives A2 that it wouldn't otherwise have. I'm not sure that's much either, given the margin is relative to A1's viewport... So it allows A2 to know its relative position against A1's viewport, but B/C are still totally opaque otherwise. I don't think that's problematic (knowing your position relative to the viewport is kinda the point of IntersectionObserver), but I could be missing something.

Tangentially, I think at least Firefox already exposes the position of A2 relative to A1 via GeometryUtils, so if that's something that we shouldn't be exposing that's another bug to fix.

szager-chromium commented 3 years ago

@annevk From #161, it sounds like you're in favor of origin-domain, yes? "Browsing context scope origin" refers to "origin" rather than "origin-domain", so it's not quite right; I guess the spec language would need to be changed to something along these lines:

If observer is an implict root observer and target's browsing context is same origin-domain with the top-level browsing context, then apply rootMargin.

Does that sound right to you?

annevk commented 3 years ago

A browsing context doesn't have an origin (or authority). I think you want target's relevant settings object's origin is ... with target's relevant settings object's top-level origin. And yeah, same origin-domain might make sense given it's about script access. Would be good to add tests for document.domain either way.

szager-chromium commented 3 years ago

OK, I updated this patch based on the discussion thus far. I think it reads much better, now; please take a look.

domenic commented 3 years ago

This looks pretty good to me, although I'd suggest the names "same/cross-origin-domain target" to help draw attention to the exact check being used.

Thanks to @annevk and @emilio for getting involved; I don't have much context here to know the right behavior; I just try to help phrase things :).

szager-chromium commented 3 years ago

@annevk @domenic If you approve of the latest changes, please go ahead and approve the pull request; thanks.

annevk commented 3 years ago

Did you end up writing tests to ensure implementations align on this?

w3c / IntersectionObserver

Remove references to unit of related similar-origin browsing contexts #448

161 has prior discussion on exactly this. I discussed bz's question there with @emilio and he could not come up with a scenario where it would leak information about any intermediate frames, be it A-B-A or A-B-C-A.