Open Bug 1498619 Opened 4 years ago Updated 4 years ago

FeaturePolicy should support document.domain changes


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






(Reporter: baku, Assigned: baku)


(Keywords: dev-doc-needed, Whiteboard: [domsecurity-active])


(1 file, 2 obsolete files)

We need to cover this feature with a WPT too.
Attached patch document.patch (obsolete) — Splinter Review
Attachment #9016700 - Flags: review?(bzbarsky)
Comment on attachment 9016700 [details] [diff] [review]

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?
Flags: needinfo?(amarchesini)
Priority: -- → P2
Whiteboard: [domsecurity-active]
Attached patch document.patch (obsolete) — Splinter Review
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'.
Attachment #9016700 - Attachment is obsolete: true
Attachment #9016700 - Flags: review?(bzbarsky)
Flags: needinfo?(amarchesini)
Attachment #9017548 - Flags: review?(bzbarsky)
Attached patch document.patchSplinter Review
Attachment #9017548 - Attachment is obsolete: true
Attachment #9017548 - Flags: review?(bzbarsky)
Attachment #9018154 - Flags: review?(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.  :(
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)
Sorry for the lag...

So what behavior we want is an interesting question.  Another interesting question is what behavior the spec specifies. is the relevant algorithm here, which is called from which is called from which is called from

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?
Flags: needinfo?(amarchesini)
> If we do want to make a copy, can we do so infallibly?

I filed a spec issue. The spec should say if we should clone/copy the origin or not.
Flags: needinfo?(amarchesini)
Comment on attachment 9018154 [details] [diff] [review]

Given the comments at this doesn't necessarily look like the right approach.  We need to get the spec end sorted out and tests written accordingly....
Attachment #9018154 - Flags: review?(bzbarsky) → review-
You need to log in before you can comment on or make changes to this bug.