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

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

Merged
merged 6 commits into from
Sep 21, 2020

Conversation

szager-chromium
Copy link
Collaborator

@szager-chromium szager-chromium commented Sep 11, 2020

Closes #429


Preview | Diff

Copy link

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

index.bs Outdated
as the <a>intersection root</a>,
If the <a>intersection root</a> is not the <a>implicit root</a>,
or when calculating the <a>intersection root</a> for
a <a>target</a> which has a <a>browsing context scope origin</a>,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This disjunction doesn't seem quite right. By my reading, anything which isn't an implicit root will pass this predicate, and thus get its rectangle expanded.

I think the intention is something like

For any target which has a browsing context scope origin and is not an implicit root

?

Copy link
Collaborator Author

@szager-chromium szager-chromium Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think my language is correct. We want the union of:

  • explicit-root observers
  • implicit-root observers with a target that has a browsing context origin scope

Those are indeed the cases where the root margin should be applied. The reasoning is:

  • For explicit root observers, the target and root elements must be in the same document, so there is no security issue, and we should always apply root margin.

  • For implicit root observers, we only want to apply root margin when the document containing the target and all of its browsing context ancestors have the same origin.

@szager-chromium
Copy link
Collaborator Author

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
Copy link

domenic commented Sep 14, 2020

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
Copy link
Collaborator Author

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
Copy link

domenic commented Sep 14, 2020

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
Copy link
Collaborator Author

szager-chromium commented Sep 15, 2020

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
Copy link
Member

annevk commented Sep 15, 2020

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
Copy link

domenic commented Sep 15, 2020

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
Copy link
Member

annevk commented Sep 15, 2020

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
Copy link
Collaborator Author

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

#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
Copy link
Collaborator Author

szager-chromium commented Sep 15, 2020

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
Copy link
Member

annevk commented Sep 16, 2020

#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
Copy link
Collaborator

emilio commented Sep 16, 2020

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.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 16, 2020
An issue on the IntersectionObserver spec repo raised the issue of
the precise limitation on rootMargin and rootBounds. The original spec
language refers to the "unit of related similar-origin browsing
contexts", which suggests that cross-origin same-site observations
could use rootMargin and rootBounds. That is incorrect; same-site
cross-origin observations should *not* be able to use rootMargin and
rootBounds.

Chromium already conforms to the more restrictive behavior; this CL
is just to add a test to enforce it.

w3c/IntersectionObserver#448

Separately, the existing cross-origin-iframe.sub.html test was disabled
due to flakiness; and then a subsequent code change broke the
cross-origin limitation for applying rootMargin, so the test has been
reliably failing. This patch fixes the cross-origin check and reenables
the test.

Change-Id: Ib864f9a9433b4d323ef73a3b2b5a8f645697c07c
@szager-chromium
Copy link
Collaborator Author

szager-chromium commented Sep 16, 2020

@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
Copy link
Member

annevk commented Sep 17, 2020

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
Copy link
Collaborator Author

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

@domenic
Copy link

domenic commented Sep 17, 2020

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
Copy link
Collaborator Author

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

@szager-chromium szager-chromium merged commit 4c53ab2 into master Sep 21, 2020
@annevk
Copy link
Member

annevk commented Sep 22, 2020

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

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 7, 2021
An issue on the IntersectionObserver spec repo raised the issue of
the precise limitation on rootMargin and rootBounds. The original spec
language refers to the "unit of related similar-origin browsing
contexts", which suggests that cross-origin same-site observations
could use rootMargin and rootBounds. That is incorrect; same-site
cross-origin observations should *not* be able to use rootMargin and
rootBounds.

Chromium already conforms to the more restrictive behavior; this CL
is just to add a test to enforce it.

w3c/IntersectionObserver#448

Separately, the existing cross-origin-iframe.sub.html test was disabled
due to flakiness; and then a subsequent code change broke the
cross-origin limitation for applying rootMargin, so the test has been
reliably failing. This patch fixes the cross-origin check and reenables
the test.

Change-Id: Ib864f9a9433b4d323ef73a3b2b5a8f645697c07c
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

Successfully merging this pull request may close these issues.

rootMargin definition uses outdated HTML Standard definition
4 participants