1498619 - FeaturePolicy should support document.domain changes

Status: ASSIGNED

(Core :: DOM: Security, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

We need to cover this feature with a WPT too.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: document.patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9016700 [details] [diff] [review]
document.patch

Ah, you already landed the change to consider document.domain during security  checks, ok.

That said, I'm not clear on why we need to reset the policy at all when document.domain is set.  Why is  that, exactly?

We should also have a test something like this.  Parent, on site A:

  <iframe src="siteB" allow="fullscreen"></iframe>

and then check that fullscreen is allowed but stops being allowed if the framed page does:

  document.domain = document.domain;

(i.e. not even changing the domain, just setting the "domain is set" flag).

We should also test that if parent at site A sets document.domain = document.domain, then loads a subframe from site A, that iframe is treated as different-origin for purposes of feature policy.  And if the subframe sets   document.domain = document.domain as well, I would expect this to _not_ change that, because the document inherited policy only gets set up once, at load time, right?

Comment 3

[Andrea Marchesini [:baku]]

Attachment: document.patch

I need to duplicate the principal because, otherwise, the allowList will be updated together with the NodePrincipal()->SetDomain(foo).

Here I test:

1. iframe has access to feature X.
2. iframe doesn't have access after a document.domain = document.domain
3. it doesn't have access after a document.domain = 'foo'.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: document.patch

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm not quite following why we need the principal cloning here.  Is that basically just to handle cases when we have "self" and "src" in allowlists?  But even in the "self" case it's not clear to me why we'd need the cloning.

I really feel like I'm missing something here.  :(

Comment 6

[Andrea Marchesini [:baku]]

> I really feel like I'm missing something here.  :(

Let's assume I don't clone the principal. Here what happens:

1. a document is created and its channel has a feature-policy: fullscreen 'self'.
2. Internally, we have a Feature object, with an allowList array, containing 1 element: NodePrincipal()
3. The document does: document.domain = something; We update NodePrincipal(). Just because FeaturePolicy allowList has a reference to NodePrincipal() also that ref will have the domain set.
4. Basically, feature fullscreen will be always considered allowed, because we compare NodePrincipal() with itself.

If we want that, after document.domain = something, fullscreen stops to be allowed, we need a copy of the original principal, and not just a ref.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag...

So what behavior we want is an interesting question.  Another interesting question is what behavior the spec specifies.

https://wicg.github.io/feature-policy/#parse-policy-directive is the relevant algorithm here, which is called from https://wicg.github.io/feature-policy/#parse-header which is called from https://wicg.github.io/feature-policy/#process-response-policy which is called from https://wicg.github.io/feature-policy/#initialize-from-response

All this is just passing the document's origin through, not a copy of it.  So per the letter of the spec, as far as I can see, the thing stored in the allowlist should be the document's origin, which can then mutate when document.domain is set.

Maybe the spec means to make a copy somewhere here?

If we do want to make a copy, can we do so infallibly?

Comment 8

[Andrea Marchesini [:baku]]

> If we do want to make a copy, can we do so infallibly?

https://github.com/WICG/feature-policy/issues/253

I filed a spec issue. The spec should say if we should clone/copy the origin or not.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9018154 [details] [diff] [review]
document.patch

Given the comments at https://github.com/WICG/feature-policy/issues/253#issuecomment-441111786 this doesn't necessarily look like the right approach.  We need to get the spec end sorted out and tests written accordingly....