1496034 - Apply bz's comment to FeaturePolicy

Status: RESOLVED FIXED

Description

[Andrea Marchesini [:baku]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1390801#c34

Comment 1

[Andrea Marchesini [:baku]]

Attachment: comments.patch

Here the main changes:

1. nsIPrincipal instead of nsString for origin
2. mFeaturePolicy is always created in HTMLIFrameElement in order to avoid pref changes. We do something similar in nsIDocument.
3. mFeaturePolicy is traversed/unlinked by HTMLIFrameElement and not its parent class.
4. NullPrincipal() is used for opaque origin, instead of an empty string.
5. WhiteList renamed AllowList to match the spec syntax.
6. comments

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 9013970 [details] [diff] [review]
comments.patch

>+++ b/dom/base/nsDocument.cpp

The documentation I asked for in nsIDocument::Policy is not there.

>+++ b/dom/html/HTMLIFrameElement.cpp
>+  // We always need a featurePolicy, also if not exposed.

"even if not exposed"

>+  mFeaturePolicy = new FeaturePolicy(this);

This is going to refcount us in the constructor... probably OK, I guess, since we have no subclasses.  

>+HTMLIFrameElement::GetFeaturePolicyDefaultOrigin(nsIPrincipal** aPrincipal) const

This function still needs documentation explaining what it's trying to compute, in spec terms.  Not having that makes it hard to review, esp in edge cases.  For  example, why do we want to create a separate nullprincipal in the sandboxed case instead of still using our document's principal?  Totally unclear.  Needs either spec references or documentation.

>+  if (!GetURIAttr(nsGkAtoms::src, nullptr, getter_AddRefs(nodeURI))) {

We should just document that if false is returned nodeURI will be null.   Also that nodeURI can be null even if true is returned.

>+  nsresult rv = nsContentUtils::GetUTFOrigin(nodeURI, origin);

"origin" is unused after this.   Why is this call here at all?  Please document or remove.

I'm a little unclear on how this stuff works for various edge cases of iframes, both in the spec and in our impl.  Some obvious questions:

1)  What happens with an <iframe srcdoc> that uses 'self' in its policy?
2)  What happens with <iframe src="data:...">, where the frame doc will have a nullprincipal but we will end up with a codebase principal, I think, from GetFeaturePolicyDefaultOrigin?  Will 'self' work?  Should it, per spec?

> HTMLIFrameElement::RefreshFeaturePolicy()
>     if (OwnerDoc()->GetSandboxFlags() ^ SANDBOXED_ORIGIN) {

This still looks wrong to me, and if correct still needs a clear explanation for why it's done this way.

>+++ b/dom/html/nsGenericHTMLFrameElement.cpp

OK, so we no longer CC mFeaturePolicy from this class, but the member and getter  still live on this class?   Shouldn't those be on HTMLIFrameElement too?

>+++ b/dom/security/featurepolicy/Feature.cpp
>+    if (BasePrincipal::Cast(principal)->FastEquals(aPrincipal)) {

Spec has a "same origin-domain" check, no?   Or is this not the check that the spec is doing?

>+      return BasePrincipal::Cast(mDefaultOrigin)->FastEquals(aOrigin);

Here too?

>+++ b/dom/security/featurepolicy/FeaturePolicyUtils.h
>+    // Feature allowed of the document's origin.

Not quite sure what this means.  Maybe "Feature allowed for documents that are same-origin with this one" or something?

>+  // Iterator.

Sure, but what do the arguments to the callback mean, for example?

It worries me that switching from strings to principals did not affect any test results.   The obvious conclusion is that test coverage is poor.  Could we add some tests that get fixed by these changes, please?

Comment 3

[Andrea Marchesini [:baku]]

> 1)  What happens with an <iframe srcdoc> that uses 'self' in its policy?

In this case, 'src' attribute is not set and the parent document's origin will be used.
I'll add a test for this.

> 2)  What happens with <iframe src="data:...">, where the frame doc will have
> a nullprincipal but we will end up with a codebase principal, I think, from
> GetFeaturePolicyDefaultOrigin?  Will 'self' work?  Should it, per spec?

There are 3 FeaturePolicy objects involved here:

1. the parent document's policy - origin: document's origin.
2. the iframe policy - origin: data URL's origin.
3. the iframe document policy - origin: nullprincipal. only '*' features should be allowed here.

in my interpretation of the spec, data: URL should be able to allow only features marked as '*'.
I'll add a test here as well.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: comments.patch

I wrote some WPTs to check sandboxed, srcdoc, data: URL iframes.
They seem to behave fine with this code, if I read the spec correctly.

Comment 5

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> In this case, 'src' attribute is not set

Why?  Someone can write:   <iframe srcdoc="foo" src="bar"> and that will ignore src and render the srcdoc.  What does the spec say to do with feature policy in this case?  What does our implementation do?

We should test both cases, with and without src, here.  And we should test dynamic changes (e.g. srcdoc being set dynamically on an iframe with existing loaded src and src being set dynamically on  an iframe with existing loaded srcdoc; the latter case should be a complete no-op).

> in my interpretation of the spec, data: URL should be able to allow only features marked as '*'.

OK, but is that reasonable behavior?

I guess blob: will work ok because we can in fact create a useful codebase principal from a blob url?

Comment 6

[Andrea Marchesini [:baku]]

> Why?  Someone can write:   <iframe srcdoc="foo" src="bar"> and that will
> ignore src and render the srcdoc.  What does the spec say to do with feature
> policy in this case?  What does our implementation do?

The spec says nothing about srcdoc. I filed a spec bug: https://github.com/WICG/feature-policy/issues/223

> We should test both cases, with and without src, here.  And we should test
> dynamic changes (e.g. srcdoc being set dynamically on an iframe with
> existing loaded src and src being set dynamically on  an iframe with
> existing loaded srcdoc; the latter case should be a complete no-op).

Ok.

> > in my interpretation of the spec, data: URL should be able to allow only features marked as '*'.
> 
> OK, but is that reasonable behavior?

data: URLs are considered cross-origin in general. I think this is the right behavior.

> I guess blob: will work ok because we can in fact create a useful codebase
> principal from a blob url?

Right.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: comments.patch

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 9014724 [details] [diff] [review]
comments.patch

>+++ b/dom/base/nsDocument.cpp
> nsIDocument::Policy() const
> {
>   MOZ_ASSERT(StaticPrefs::dom_security_featurePolicy_enabled());

Note that this assert will fail if the pref is changed after initial document setup.   I don't think we should be asserting this.

>+++ b/dom/html/HTMLIFrameElement.cpp
>+HTMLIFrameElement::GetFeaturePolicyDefaultOrigin(nsIPrincipal** aPrincipal) const

The docs don't match the implemenentation in the srcdoc case.

The function now always returns NS_OK.  Why do we need the nsresult return type at all?  Why not return already_AddRefed<nsIPrincipal>?  You could then take early returns in special cases like srcdoc.

Do we have tests that would catch the behavior change from us no longer checking sandboxing  flags here?

> data: URLs are considered cross-origin in general. I think this is the right behavior.

Sure, but why shouldn't 'self' be the nonce origin for the data: case?  Anyway, this seems like a spec issue that we should just file.

Do we have a test for us  no longer checking sandboxing flags here?

>+++ b/dom/security/featurepolicy/Feature.cpp
>+Feature::AllowListContains(nsIPrincipal* aPrincipal) const
>+    if (BasePrincipal::Cast(principal)->Subsumes(aPrincipal,
>+                                                 BasePrincipal::DontConsiderDocumentDomain)) {

The spec _does_ consider document.domain, doesn't it?  Again, there should be tests for this stuff...

>+++ b/dom/security/featurepolicy/FeaturePolicy.cpp
> FeaturePolicy::AllowsFeatureInternal(const nsAString& aFeatureName,
>+      return BasePrincipal::Cast(mDefaultOrigin)->Subsumes(aOrigin,
>+                                                           BasePrincipal::DontConsiderDocumentDomain);

Again, I think the spec considers document.domain...  Again, tests?

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Oh, and in general this is looking pretty good!  Thank you for the expanded test coverage!

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 9014724 [details] [diff] [review]
comments.patch

Just talked about this with Andrea on IRC.  We agreed to get this landed with all but the document.domain comments addressed and file followups for the document.domain bits and adding more test coverage and so forth.

r+ with the other bits addressed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/402b42c38c70
Apply bz's comments to FeaturePolicy, r=bz

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa33c65b6222
Remove FeaturePolicy assertion in getter methods, r=bz

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6fe44fb6369
Improve HTMLIFrameElement::GetFeaturePolicyDefaultOrigin, r=bz

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/402b42c38c70
https://hg.mozilla.org/mozilla-central/rev/fa33c65b6222
https://hg.mozilla.org/mozilla-central/rev/a6fe44fb6369

Comment 15

[Web Platform Test Sync Bot]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13491 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13491
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/440777378?utm_source=github_status&utm_medium=notification)

Comment 17

[Web Platform Test Sync Bot]

Upstream PR merged