Apply bz's comment to FeaturePolicy

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [domsecurity-backlog1] [domsecurity-active][wptsync upstream])

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Comment 1

8 months ago
Posted patch comments.patch (obsolete) — Splinter Review
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
Attachment #9013970 - Flags: review?(bzbarsky)
Assignee

Updated

8 months ago
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog1] [domsecurity-active]
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?
Attachment #9013970 - Flags: review?(bzbarsky) → review-
Assignee

Comment 3

8 months ago
> 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.
Assignee

Comment 4

8 months ago
Posted patch comments.patch (obsolete) — Splinter Review
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.
Attachment #9013970 - Attachment is obsolete: true
Attachment #9014413 - Flags: review?(bzbarsky)
> 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?
Assignee

Comment 6

8 months ago
> 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.
QA Contact: ckerschb
Assignee

Comment 7

8 months ago
Attachment #9014413 - Attachment is obsolete: true
Attachment #9014413 - Flags: review?(bzbarsky)
Attachment #9014724 - Flags: review?(bzbarsky)
QA Contact: ckerschb
Assignee

Updated

8 months ago
Blocks: 1497486
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?
Attachment #9014724 - Flags: review?(bzbarsky) → review-
Oh, and in general this is looking pretty good!  Thank you for the expanded test coverage!
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.
Attachment #9014724 - Flags: review- → review+

Comment 11

7 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/402b42c38c70
Apply bz's comments to FeaturePolicy, r=bz

Comment 12

7 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa33c65b6222
Remove FeaturePolicy assertion in getter methods, r=bz

Comment 13

7 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6fe44fb6369
Improve HTMLIFrameElement::GetFeaturePolicyDefaultOrigin, r=bz

Comment 14

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/402b42c38c70
https://hg.mozilla.org/mozilla-central/rev/fa33c65b6222
https://hg.mozilla.org/mozilla-central/rev/a6fe44fb6369
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13491 for changes under testing/web-platform/tests
Whiteboard: [domsecurity-backlog1] [domsecurity-active] → [domsecurity-backlog1] [domsecurity-active][wptsync upstream]
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)
Whiteboard: [domsecurity-backlog1] [domsecurity-active][wptsync upstream] → [domsecurity-backlog1] [domsecurity-active][wptsync upstream error]
Whiteboard: [domsecurity-backlog1] [domsecurity-active][wptsync upstream error] → [domsecurity-backlog1] [domsecurity-active][wptsync upstream]
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.