Closed Bug 1390801 Opened 7 years ago Closed 6 years ago

Implement feature-policy

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: ckerschb, Assigned: baku)

References

(Blocks 4 open bugs)

Details

(Keywords: dev-doc-complete, parity-chrome, Whiteboard: [domsecurity-backlog1])

Attachments

(2 files, 16 obsolete files)

20.88 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
75.39 KB, patch
ckerschb
: review+
bzbarsky
: feedback+
Details | Diff | Splinter Review
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1][parity-chrome]
Blocks: 1442689
FWIW, we decided on a public position of "worth prototyping" in https://github.com/mozilla/standards-positions/issues/24.
Adding a WIP patch which allows to:
* parse HTTP response header and creates an object representation for featurePolicy
* allows to query whether a feature is allowed or not; that API is available through C++ and also provides the necessary WebIDL bindings so one could query |document.policy.allowsFeature("camera");|
* added testcase which verifies those parts are working

This patch does not include:
* Actually preffing off the requested feature for the context

Also there a bunch of cornercases that need to be sorted out:
* What happens if an http header ships a feature policy which contradicts with the value in the frame 'allows' attribute
* what happens if two http feature policies are sent (first one wins?)
* upper/lowercase checks
I thought the idea was that it could reuse the CSP parser? Did you look at that?
(In reply to Anne (:annevk) from comment #3)
> I thought the idea was that it could reuse the CSP parser? Did you look at
> that?

CSP Parser is a massive and complex piece of software. In contrast, a feature policy only has to do 4 string comparisions once tokenized and that's it. I think it would make sense that CSP and Feature Policy share the same tokenizer, but the actual parsing should happen separately.

I would change my mind if feature policy all of a sudden would accept wildcards like *.example.com, but I hope we don't go down that route. The way it is right now, the feature policy parser can just rely on NS_NewURI() which does all the URI parsing for us.
I guess I'm mostly concerned about the tokenizer, then. I just don't want to end up in a situation where they diverge and we're stuck with two.
Depends on: 1458504
(In reply to Anne (:annevk) from comment #5)
> I guess I'm mostly concerned about the tokenizer, then. I just don't want to
> end up in a situation where they diverge and we're stuck with two.

FWIW, I filed Bug 1458504 to factor out the tokenizer.
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [domsecurity-backlog1][parity-chrome] → [domsecurity-backlog1]
Depends on: 1483631
Assignee: nobody → amarchesini
Attached patch part 1 - FeaturePolicy parser (obsolete) — Splinter Review
Attachment #8971280 - Attachment is obsolete: true
I'm going to ask Christoph to review this code, but I would like to have a feedback from a DOM point of view:

1. is this patch correctly initialized at a nsIDocument layer? Should I move this in HTMLDocument instead?

2. The interaction with HTMLIFrameElement is correctly implemented?
Attachment #9005851 - Flags: feedback?(bzbarsky)
Attached patch part 3 - WebIDL bindings (obsolete) — Splinter Review
Here the WebIDL binding support. Am I missing some important parts (except adding more tests)?

The idea is to introduce a [FeaturePolicy='foo'] attribute to enable/disable interfaces and methods.
Attachment #9005852 - Flags: feedback?(bzbarsky)
Attachment #9005851 - Attachment description: 2_policy.patch → Part 2 - FeaturePolicy WebIDL interface
Attachment #9005850 - Flags: review?(ckerschb)
Attachment #9005851 - Flags: feedback?(ckerschb)
Comment on attachment 9005850 [details] [diff] [review]
part 1 - FeaturePolicy parser

Review of attachment 9005850 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good, but we need to iron out all these corner cases. Please look and add my suggested test cases for the parser. Happy to take another look once that has happened!

::: dom/locales/en-US/chrome/security/security.properties
@@ +101,5 @@
>  UnknownClearSiteDataValue=Clear-Site-Data header found. Unknown value “%S”.
> +
> +FeaturePolicyUnsupportedFeatureName=Unsupported Feature-Policy name “%S”.
> +# TODO: would be nice to add a link to the Feature-Policy MDN documentation here.
> +FeaturePolicyInvalidAllowValue=Unsupported Feature-Policy allow value “%S”.

Maybe we can prefix the errors like we do in CSP so that the errors in the end look like this:

Feature Policy: Skipping unsupported feature "%S".
Feature Policy: Skipping unsupported allow value "%S".

And if a feature does not contain any valid values then
Feature Policy: Skipping feature "%S" because it does not contain any valid values.

::: dom/security/featurepolicy/Feature.cpp
@@ +6,5 @@
> +
> +#include "Feature.h"
> +
> +#include "nsIURI.h"
> +

Nit: I Personally prefer if things are implemented in the .cpp file, not the .h file. For small function bodies it's fine, but probably worth implementing the actual matching function (e.g allows()) within the *.cpp

::: dom/security/featurepolicy/Feature.h
@@ +20,5 @@
> +{
> +public:
> +  explicit Feature(const nsAString& aFeatureName)
> +    : mFeatureName(aFeatureName)
> +    , mPolicy(eWhiteList)

nit: that's an argument, so please prefix with a.

@@ +46,5 @@
> +
> +  void
> +  AppendURIToWhiteList(nsIURI* aURI)
> +  {
> +    mPolicy = eWhiteList;

Does it make sense to overwrite mPolicy in this case? Shouldn't mPolicy be frozen at creation time and never be opened up? Probably it's better to add an assertion here that mPolicy is in fact eWhiteList, no?

I guess we also want tests for that case. E.g. what is the expected outcome of a policy of:
"camera aaa.com bbb.com 'none'"

@@ +66,5 @@
> +  };
> +
> +  Policy mPolicy;
> +
> +  nsTArray<nsCOMPtr<nsIURI>> mWhiteList;

mWhiteList is not used anywhere. I guess we also need an allows function something like:
bool allows(nsIURI* aURI) {
  for (all elements in whitelist) {
    if (elemet[i] == aURI) {
      return true
    }
  }
  return false
}
also capturing 'none' and 'all' of course.

::: dom/security/featurepolicy/FeaturePolicyParser.cpp
@@ +19,5 @@
> +namespace {
> +
> +void
> +ReportToConsoleUnsupportedFeature(const nsString& aFeatureName)
> +{

Probably it makes sense to move the Console util functions into FeaturePolicyUtils.h/cpp

@@ +24,5 @@
> +  const char16_t* params[] = { aFeatureName.get() };
> +
> +  nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> +                                  NS_LITERAL_CSTRING("Feature Policy"),
> +                                  nullptr, // document

please somehow pass the document so that we can log to right web console instead of the browser console. Maybe you could pass it to ParseString()?

@@ +58,5 @@
> +
> +  nsTArray<Feature> parsedFeatures;
> +
> +  for (const nsTArray<nsString>& featureTokens : tokens) {
> +    if (featureTokens.IsEmpty()) {

I guess we should warn to the console in that case.

@@ +73,5 @@
> +    // we gotta start at 1 here
> +    for (uint32_t i = 1; i < featureTokens.Length(); ++i) {
> +      const nsString& curVal = featureTokens[i];
> +      if (curVal.EqualsLiteral("'none'")) {
> +      	feature.SetAllowsNone();

I think we should clear already added URLs. Imagine a policy of "camera foo.com 'none'" then we are in a somehow incosistent state.

@@ +91,5 @@
> +      nsCOMPtr<nsIURI> uri;
> +      nsresult rv = NS_NewURI(getter_AddRefs(uri), curVal);
> +      if (NS_FAILED(rv)) {
> +        ReportToConsoleInvalidAllowValue(curVal);
> +        return false;

I think you don't want to return false here, do you?
What happens if you have a policy of "camera incorrectURI foo.com"? I guess in that case we want a policy containing at least foo.com, right?

@@ +97,5 @@
> +
> +      feature.AppendURIToWhiteList(uri);
> +    }
> +
> +    parsedFeatures.AppendElement(feature);

Since we are going to change the above we also need to update that part and check that a feature actually consists a valid value. E.g a policy of "camera invalidURL" might add a feature to parsedFeatures which potentially holds an empty whitelist. Also logging to the console in that case probably makes sense. E.g. Skipping feature "camera" because ...

::: dom/security/featurepolicy/FeaturePolicyUtils.cpp
@@ +11,5 @@
> +static const char* sSupportedFeatures[] = {
> +  "camera",
> +  "geolocation",
> +  "microphone",
> +  "vibrate",

Can we use something better than a hardcoded list here? Or asked differently how do we know that list captures all the features?

::: dom/security/featurepolicy/test/gtest/TestFeaturePolicyParser.cpp
@@ +78,5 @@
> +  ASSERT_TRUE(parsedFeatures[1].Name().Equals(NS_LITERAL_STRING("microphone")));
> +
> +  // After a 'none' we don't continue the parsing.
> +  CheckParser(NS_LITERAL_STRING("camera 'none' a b c d"), true, 1,
> +              parsedFeatures);

a b c d would be ignored by the parser anyway, because they are not valid URLs, right? So probably it's better to use valid URLs to make sure the parsing has stopped.

@@ +103,5 @@
> +
> +  // A couple of URLs but then *
> +  CheckParser(NS_LITERAL_STRING("camera http://example.com 'self' http://example.net *"), true, 1,
> +              parsedFeatures);
> +  ASSERT_TRUE(parsedFeatures[0].Name().Equals(NS_LITERAL_STRING("camera")));

Please add a test for every single feature we are supporting. For now at least the 4 we have hardcoded ultimately also using upper/lower cases variations.

In addition have a test with
* an empty set, e.g. "camera" with no valid value.
* more whitespaces
* 'self    ' with space or also not using the ending ' at all
* 'SelF' using upper and lower cases variations
* valid and invalid URIS within one policy
* 'none' 'self' and valid and invalid URLs in the same policy

and then once we get there also mochitests or wpt tests that the matching algorithm works correctly.
Attachment #9005850 - Flags: review?(ckerschb)
> Does it make sense to overwrite mPolicy in this case? Shouldn't mPolicy be
> frozen at creation time and never be opened up? Probably it's better to add
> an assertion here that mPolicy is in fact eWhiteList, no?

No, it can change. In your scenario:

> "camera aaa.com bbb.com 'none'"

Feature initially is set to eWhiteList. It stays in eWhiteList when 'aaa.com' and 'bbb.com' are added. Then, after the 'none', it becomes eNone.

> mWhiteList is not used anywhere. I guess we also need an allows function
> something like:

Following patches :) This patch is just the parser.

> Probably it makes sense to move the Console util functions into
> FeaturePolicyUtils.h/cpp

Well, they are not used elsewhere. I prefer to keep them here.

> > +  for (const nsTArray<nsString>& featureTokens : tokens) {
> > +    if (featureTokens.IsEmpty()) {
> 
> I guess we should warn to the console in that case.

No. This is totally allowed for <iframe allow="camera">. I can pass an extra parameter to know if it's for header or for attribute, but I'll do it in the next patch.


> holds an empty whitelist. Also logging to the console in that case probably
> makes sense. E.g. Skipping feature "camera" because ...

By spec, we should add an empty feature if the URL is invalid.

> Can we use something better than a hardcoded list here? Or asked differently
> how do we know that list captures all the features?

This is just for testing. The list will be increased in the following patches after the webIDL support.

> a b c d would be ignored by the parser anyway, because they are not valid
> URLs, right? So probably it's better to use valid URLs to make sure the
> parsing has stopped.

It stops because 'none' causes a 'break'. I'll change them with valid URLs.

> Please add a test for every single feature we are supporting. For now at
> least the 4 we have hardcoded ultimately also using upper/lower cases
> variations.

OK. I add this test in the following patch where I have an iterator.
Attachment #9005850 - Attachment is obsolete: true
Attachment #9007706 - Flags: review?(ckerschb)
Comment on attachment 9007706 [details] [diff] [review]
part 1 - FeaturePolicy parser

Review of attachment 9007706 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the clarifications and for incorporating my suggestions. We also need a pref so we can switch feature policy support of in case of emergency, but it's fine if that is one of the following patches.

::: dom/security/featurepolicy/FeaturePolicyUtils.cpp
@@ +12,5 @@
> +  "camera",
> +  "geolocation",
> +  "microphone",
> +  "vibrate",
> +};

That hardcoding is the only thing I still don't like, but I guess it's acceptable for now.
Attachment #9007706 - Flags: review?(ckerschb) → review+
Attachment #9005852 - Attachment is obsolete: true
Attachment #9005852 - Flags: feedback?(bzbarsky)
The WebIDL algorithms are not completely correct. Here where my implementation diverges from the spec:

1. Section 10.10.4.1: 'If the allowlist for feature in container policy matches origin, and parent’s inherited policy for feature is "Enabled", return "Enabled".'
We should check if the parent's policy for feature is 'Enabled'. Not just the inherited policy, otherwise we ignore the parent's declared policy.

2. 10.10.2: "Let origin be parent’s origin"
Actually we should use the default policy origin. For iframes we want to check their origins are allowed by the policy, and not the parent document's origin.
Attachment #9005851 - Attachment is obsolete: true
Attachment #9005851 - Flags: feedback?(ckerschb)
Attachment #9005851 - Flags: feedback?(bzbarsky)
Attachment #9008316 - Flags: review?(ckerschb)
Attachment #9008316 - Flags: feedback?(bzbarsky)
Attachment #9008317 - Flags: review?(ckerschb)
Attached patch part 5 - FeaturePolicy: MIDI (obsolete) — Splinter Review
Attachment #9008319 - Flags: review?(ckerschb)
Attachment #9008320 - Flags: review?(ckerschb)
Attached patch part 8 - FeaturePolicy: vr (obsolete) — Splinter Review
Attachment #9008321 - Flags: review?(ckerschb)
Attached patch part 6 - FeaturePolicy: payment (obsolete) — Splinter Review
Attachment #9008322 - Flags: review?(ckerschb)
Attachment #9008323 - Flags: review?(ckerschb)
Attachment #9008325 - Flags: review?(ckerschb)
Attachment #9008326 - Flags: review?(ckerschb)
Attached patch part 3 - WPTs (obsolete) — Splinter Review
The non-yet-green WPTs are:

1. autoplay - still not supported.
2. payment - I need to set a couple of pref.
3. picture-in-picture - not implemented API
4. Some cross-origin tests: I'm quite sure those are wrong. I'll ping you on IRC about them.
Attachment #9008329 - Flags: review?(ckerschb)
Attachment #9008325 - Attachment is obsolete: true
Attachment #9008325 - Flags: review?(ckerschb)
Attachment #9009210 - Flags: review?(ckerschb)
Attached patch part 8 - FeaturePolicy: vr (obsolete) — Splinter Review
Attachment #9008321 - Attachment is obsolete: true
Attachment #9008321 - Flags: review?(ckerschb)
Attachment #9009211 - Flags: review?(ckerschb)
Blocks: 1491748
Comment on attachment 9008316 [details] [diff] [review]
part 2 - FeaturePolicy implementation

Review of attachment 9008316 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Baku for taking on the work. This is the first round of review and overall this looks pretty amazing. There are a few things we should consider though. Please take a look at my comments inline; in particular we need to address the following points:

* At the top of FeaturePolicy.h we need a description of how it all works together (a) How Parsing works, b) how inheritance works, c) how overruling works, when do we call reset and why?).
* I would prefer if the pref-checking is encapsulated within FeaturePolicy.cpp entirely so that we have a clean API
* Check all the loops for 'return' VS 'continue' to make sure we consult the entire policy in all cases
* I think the origin should never be allowed to be the empty string, not entirely sure how to do that though.
* I am worried about checking sandbox flags for querying the origin, would it make sense to rely on the nodePrincipal (which would be a nullPrincipal in the sandbox case?)
* We need testcases for sandboxed iframes and data: URI iframes
* We also need testcases for inheritance, in particular where top-level denies feature but then we open it up in the iframe and vice versa

::: dom/base/nsDocument.cpp
@@ +2773,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
> +  // Initialize FeaturePolicy
> +  if (StaticPrefs::dom_security_featurePolicy_enabled()) {

nit: I think it's prefereable to only add the pref within InitFeaturePolicy() to make sure initialization of it is always guarded by the pref. Please just call InitFeaturePolicy() here.

@@ +3005,5 @@
>  
> +nsresult
> +nsIDocument::InitFeaturePolicy(nsIChannel* aChannel)
> +{
> +  MOZ_ASSERT(StaticPrefs::dom_security_featurePolicy_enabled());

instead of the MOZ_ASSERT() just return NS_OK if the pref is false.

@@ +3015,5 @@
> +
> +  nsresult rv;
> +  nsAutoString origin;
> +
> +  if (GetSandboxFlags() ^ SANDBOXED_ORIGIN) {

nit: please add a comment above what we are doing here.

@@ +3027,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +  }
> +

What happens if the branch |if (GetSandboxFlags() ^ SANDBOXED_ORIGIN)| is not entered? Then the default origin remains empty, right? Probably it's fine to keep as is, but maybe we should make sure we are not missing anything here. Could there be a potential problem in case we have two nested sandboxed iframes? Should we rather rely on the uid of the NullPrincipal instead of relying on the Sandbox flag? I guess same question holds true for data: URI iframes.

@@ +3045,5 @@
> +    }
> +  }
> +
> +  // Let's inherit the policy from the parent HTMLIFrameElement.
> +  if (parentPolicy) {

Is parentPolicy ever non null here? Looking at the code it seems it never is, because we always call |mFeaturePolicy = new FeaturePolicy(this)| right? So for all iframes we have a non null policy (even if it's not containing any feature policy values), right?

@@ +10269,5 @@
> +FeaturePolicy*
> +nsIDocument::Policy() const
> +{
> +  MOZ_ASSERT(StaticPrefs::dom_security_featurePolicy_enabled());
> +  return mFeaturePolicy;

If you really always want a non null policy, then I think it's better to incorporate that enforcment here, something like:

if (!mFeaturePolicy) {
  mFeaturePolicy = new ...;
}
return mFeaturePolicy;

If that is what we want, then I think it's safer to have that guard within ::Policy itself instead of having that enforcement sprinkled throughout the files.

::: dom/locales/en-US/chrome/security/security.properties
@@ +102,5 @@
>  
>  FeaturePolicyUnsupportedFeatureName=Feature Policy: Skipping unsupported feature name “%S”.
>  # TODO: would be nice to add a link to the Feature-Policy MDN documentation here.
> +FeaturePolicyInvalidEmptyAllowValue=Feature Policy: Skipping empty allow list for feature name “%S”.
> +# TODO: would be nice to add a link to the Feature-Policy MDN documentation here.

Please file a follow up bug within :Devtools and link to it here. For CSP, CORS and SOP we are using the category to link to MDN, we should do the same for Feature Policy.

::: dom/security/featurepolicy/FeaturePolicy.cpp
@@ +11,5 @@
> +#include "nsContentUtils.h"
> +
> +using namespace mozilla::dom;
> +
> +struct FeaturePolicy::InheritedFeature

I can't follow how InheritedFeature works. Can you please add a comment and describe. Alternatively, do we need mInheritedFeatures at all or could we potentially keep all of the inheritance stored within mFeatures?

@@ +53,5 @@
> +    if (dest->HasDeclaredFeature(featureName)) {
> +      if (dest->AllowsFeatureInternal(featureName, origin) &&
> +          src->AllowsFeatureInternal(featureName, origin)) {
> +        dest->SetInheritedAllowedFeature(featureName);
> +        return;

Should that really be a return or a break in all of the cases within this loop?

::: dom/security/featurepolicy/FeaturePolicy.h
@@ +30,5 @@
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(FeaturePolicy)
> +
> +  explicit FeaturePolicy(nsINode* aNode);
> +
> +  // A FeaturePolicy must have a default origin. This method must be called

that comment contradicts with other comments throughout the patch. 'Must have a default origin' means it can't be null, but it is null in some cases.

@@ +68,5 @@
> +
> +  nsINode*
> +  GetParentObject() const
> +  {
> +    return mParentNode;

It seems mPrentNode as well as GetParentObject() are never used, please remove.

@@ +95,5 @@
> +  // Inherits a single denied feature from the parent context.
> +  void
> +  SetInheritedDeniedFeature(const nsAString& aFeatureName);
> +
> +  // Inherits a single denied feature from the parent context.

nit: allowed not denied

::: dom/security/featurepolicy/FeaturePolicyParser.cpp
@@ +135,5 @@
> +        nsAutoString origin;
> +        rv = nsContentUtils::GetUTFOrigin(uri, origin);
> +        if (NS_WARN_IF(NS_FAILED(rv))) {
> +          ReportToConsoleInvalidAllowValue(aDocument, curVal);
> +          return false;

do you want to stop here and return or continue?

::: dom/security/featurepolicy/FeaturePolicyUtils.cpp
@@ +118,5 @@
> +          MOZ_CRASH("Unknown default value");
> +      }
> +      return true;
> +    }
> +  }

Isn't that the exact same function body as within DefaultAllowListFeature modulo the return type? Probably worth adding a helper that can be called from both functions

::: dom/security/featurepolicy/test/mochitest/test_parser.html
@@ +40,5 @@
> +  is(allowed.length, 0, "No allowlist for microphone");
> +
> +  ok(!document.policy.allowsFeature("vr"), "Vibrate is disabled for self");
> +  ok(!document.policy.allowsFeature("vr", location.origin), "Vibrate is disabled for self");
> +  ok(!document.policy.allowsFeature("vr", "http://foo.bar"), "Vibrate is disabled for foo.bar");

this is subject to change. So we probably have to update document.policy to become something else, see also:
https://github.com/WICG/feature-policy/issues/216

@@ +205,5 @@
> +  var test = tests.shift();
> +  test();
> +}
> +
> +next();

don't you need to flip the pref to run the tests? Something like pushPrefEnv()

::: dom/security/featurepolicy/test/mochitest/test_parser.html^headers^
@@ +1,1 @@
> +Feature-Policy: camera *; geolocation 'self'; microphone http://example.com http://example.org; vr 'none'

would be great to also test 'src'
Attachment #9008316 - Flags: review?(ckerschb)
> * At the top of FeaturePolicy.h we need a description of how it all works
> together (a) How Parsing works, b) how inheritance works, c) how overruling
> works, when do we call reset and why?).

OK! Writing documentation.

> * I think the origin should never be allowed to be the empty string, not
> entirely sure how to do that though.

Right. I'm testing if I can use the origin from the principal.

> * We need testcases for sandboxed iframes and data: URI iframes

We have several in WPTs.

> * We also need testcases for inheritance, in particular where top-level
> denies feature but then we open it up in the iframe and vice versa

There are a lot of them in the WPTs.

> > +struct FeaturePolicy::InheritedFeature
> 
> I can't follow how InheritedFeature works. Can you please add a comment and
> describe. Alternatively, do we need mInheritedFeatures at all or could we
> potentially keep all of the inheritance stored within mFeatures?

I prefer to have them separate because I need to check if there are declared features or just inherited ones.
Plus, Inherited features are just allowed/denied without having a list of allowed origins.

I could unify them, but it would make Feature object more complex than what it is.

I wrote some comments.
 
> It seems mPrentNode as well as GetParentObject() are never used, please
> remove.

They are needed for the WebIDL binding.

> ::: dom/security/featurepolicy/FeaturePolicyUtils.cpp
> @@ +118,5 @@
> > +          MOZ_CRASH("Unknown default value");
> > +      }
> > +      return true;
> > +    }
> > +  }
> 
> Isn't that the exact same function body as within DefaultAllowListFeature
> modulo the return type? Probably worth adding a helper that can be called
> from both functions

They are similar, but not equal...


> this is subject to change. So we probably have to update document.policy to
> become something else, see also:
> https://github.com/WICG/feature-policy/issues/216

Right. Let's see how the review process goes, and this issue goes. I can update the code before/after landing as follow up in case.

> don't you need to flip the pref to run the tests? Something like
> pushPrefEnv() 

Done in the mochitest.ini

> ::: dom/security/featurepolicy/test/mochitest/test_parser.html^headers^
> @@ +1,1 @@
> > +Feature-Policy: camera *; geolocation 'self'; microphone http://example.com http://example.org; vr 'none'
> 
> would be great to also test 'src'

'src' is not a valid entry for headers, just for attributes. We have tests in WPTs.
Attachment #9008316 - Attachment is obsolete: true
Attachment #9008316 - Flags: feedback?(bzbarsky)
Attachment #9012250 - Flags: review?(ckerschb)
Comment on attachment 9012250 [details] [diff] [review]
part 2 - FeaturePolicy implementation

Review of attachment 9012250 [details] [diff] [review]:
-----------------------------------------------------------------

Baku, thanks for incorporating my suggestions and thanks for adding the description of how it all works together within FeaturePolicy.h.

As discussed, we should file individual bugs for the hooks into FeaturePolicy. That way we can track individual work for all the features and module owners will be aware (since they will also review) that features are hooked up to feature policy. I will mark all of those patches as obsolete within this bug.

Further, as agreed, let's land the patches within this bug behind a pref. After more testing and once we have finalized things within the spec. We can flip the pref and roll this out.

Thanks - great work!

::: dom/locales/en-US/chrome/security/security.properties
@@ +101,5 @@
>  UnknownClearSiteDataValue=Clear-Site-Data header found. Unknown value “%S”.
>  
>  FeaturePolicyUnsupportedFeatureName=Feature Policy: Skipping unsupported feature name “%S”.
> +# TODO: would be nice to add a link to the Feature-Policy MDN documentation here. See bug 1449501
> +FeaturePolicyInvalidEmptyAllowValue=Feature Policy: Skipping empty allow list for feature name “%S”.

nit; maybe we should make this just:
Feature Policy: Skipping empty allow list for feature: “%S”.
Attachment #9012250 - Flags: review?(ckerschb) → review+
Attachment #9008317 - Attachment is obsolete: true
Attachment #9008317 - Flags: review?(ckerschb)
Attachment #9008319 - Attachment is obsolete: true
Attachment #9008319 - Flags: review?(ckerschb)
Attachment #9008320 - Attachment is obsolete: true
Attachment #9008320 - Flags: review?(ckerschb)
Attachment #9008322 - Attachment is obsolete: true
Attachment #9008322 - Flags: review?(ckerschb)
Attachment #9008323 - Attachment is obsolete: true
Attachment #9008323 - Flags: review?(ckerschb)
Attachment #9008326 - Attachment is obsolete: true
Attachment #9008326 - Flags: review?(ckerschb)
Attachment #9008329 - Attachment is obsolete: true
Attachment #9008329 - Flags: review?(ckerschb)
Attachment #9009210 - Attachment is obsolete: true
Attachment #9009210 - Flags: review?(ckerschb)
Attachment #9009211 - Attachment is obsolete: true
Attachment #9009211 - Flags: review?(ckerschb)
Blocks: 1483631
No longer depends on: 1483631
Blocks: 1495302
Blocks: 1495303
Blocks: 1495304
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb62db6b0343
FeaturePolicy - part 1 - HTTP header and attribute parser, r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/8edf2b229c9c
FeaturePolicy - part 2 - WebIDL + DOM integration, r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/15516085ee08
FeaturePolicy - part 3 - Enabling WPTs, r=ckerschb
Blocks: 1495358
Blocks: 1495359
Blocks: 1495362
Blocks: 1495364
Attachment #9012250 - Flags: feedback?(bzbarsky)
Blocks: 1495709
(In reply to Noemi Erli[:noemi_erli] from comment #32)
> https://hg.mozilla.org/mozilla-central/rev/8edf2b229c9c
> +FeaturePolicyInvalidEmptyAllowValue= Feature Policy: Skipping empty allow list for feature: “%S”.

Note the space at the beginning.

Additionally and from a localizer’s point of view, adding a comment about what “%S” stands for would come in handy (for each of the 3 new strings.) For instance, I’m not sure whether it stands for the name or the feature in FeaturePolicyUnsupportedFeatureName.
Depends on: 1495862
Comment on attachment 9012250 [details] [diff] [review]
part 2 - FeaturePolicy implementation

>+++ b/dom/base/nsDocument.cpp
>+nsIDocument::InitFeaturePolicy(nsIChannel* aChannel)
>+      nsCOMPtr<nsINode> node = containerWindow->GetFrameElementInternal();
>+      if (node) {
>+        HTMLIFrameElement* iframe = HTMLIFrameElement::FromNode(node);

Could use FromNodeOrNull here to  save some indentation...

>+nsIDocument::Policy() const
>+  MOZ_ASSERT(mFeaturePolicy);

So this is assuming the pref value does not change between StartDocumentLoad and the document's prototype being created.

That's probably an OK assumption, since we set up window.document quite early on...  But might be worth documenting, in case this assert ever fails.

>+++ b/dom/bindings/Bindings.conf
>+    'nativeType': 'mozilla::dom::FeaturePolicy',

Why not call the C++ thing "mozilla::dom::Policy"?  In general, we want to avoid adding new 'nativeType' annotations...

Are we pushing for the interface to be renamed to "FeaturePolicy"?

>+++ b/dom/html/HTMLIFrameElement.cpp
>+HTMLIFrameElement::Policy() const
>+  MOZ_ASSERT(StaticPrefs::dom_security_featurePolicy_enabled());

This assert  can fail; see below.

>+  return mFeaturePolicy;

The IDL says this will never return null, but in fact null can be returned here.

Specifically, consider the following sequence of events:

1) Feature policy pref is enabled.
2) Page is loaded.
3) An iframe is created, causing creation of HTMLIFrameElement.prototype with a "policy" attribute on it.
4) Feature policy pref is disabled.
5) An iframe is created.
6) .policy is accessed on the iframe from step 5.

Due to step 3, the getter for .policy will exist on that second iframe.  But the MOZ_ASSERT will fail (since the pref is disabled) and null will be returned (since in step 5 no FeaturePolicy object was created).

Maybe we're ok with this, of course.  It does mean that toggling that pref while any web pages are loaded is a footgun that can lead to crashes...

>+HTMLIFrameElement::GetFeaturePolicyDefaultOrigin(nsAString& aDefaultOrigin) const
>+    rv = NS_NewURI(getter_AddRefs(nodeURI), src,

I don't quite understand why we're doing this.  The string form of GetURIAttr tries to get an nsIURI, then serialize it to a string, with fallback in case it  couldn't get an nsIURI.  Then we're re-parsing the string here, etc.

Why can't this code just use the nsIURI version of GetURIAttr?   Please document that clearly, because otherwise there's a strong tempation to rip all this out and use that.  Or _should_ this code use that?

I also don't quite understand the naming of this function.  Is it computing https://wicg.github.io/feature-policy/#declared-origin or something else?   That thing is supposed to be an origin (i.e. nsIPrincipal), not a string.  The naming sounds like we're computing https://wicg.github.io/feature-policy/#default-origin but that is also supposed to be an origin, not a string.

Same issue with the mDefaultOrigin of FeaturePolicy, actually.

>+HTMLIFrameElement::RefreshFeaturePolicy()
>+  MOZ_ASSERT(StaticPrefs::dom_security_featurePolicy_enabled());
>+  mFeaturePolicy->ResetDeclaredPolicy();

This will crash if the iframe was created when the pref was off and then the pref got turned on and we hit this code (e.g. due to moving the iframe in the  DOM).  See above about toggling the pref while any pages are open leading to  
crashes...

>+    if (OwnerDoc()->GetSandboxFlags() ^ SANDBOXED_ORIGIN) {

This will test true in two cases:

1) SANDBOXED_ORIGIN is set _and_ at least one other sandbox flag is set.
2) SANDBOXED_ORIGIN is not set.

But why is that the right test?  At the very least, this needs a very clear comment explaining what's really being tested here.  I suspect this is actually a typo...

Note that in nsIDocument::InitFeaturePolicy we didn't consider sandbox flags when determining the "origin" arg to SetDeclaredPolicy.  Here we _are_ considering them.  Why the inconsistency?  Again, should be documented, both places.  Assuming it's not a bug.

>+  mFeaturePolicy->InheritPolicy(OwnerDoc()->Policy());

This can pass null to InheritPolicy if the pref got turned on after initial document load, right?  Looks like another crash opportunity.

>+++ b/dom/html/nsGenericHTMLFrameElement.h
>+  // Used by <iframe> only.

Then why is it living on the superclass, not  on HTMLIFrameElement?

>+++ b/dom/security/featurepolicy/Feature.cpp
>+using namespace mozilla::dom;

Why is this better than putting the while file in the mozilla::dom namespace?

>+Feature::Allows(const nsAString& aOrigin) const
>+    if (whiteListedOrigin.Equals(aOrigin)) {

Is this intending to implement https://wicg.github.io/feature-policy/#matches or something else?  If it is, then it should be operating on principals and doing  same origin-domain checks, no?

I didn't audit the details of FeaturePolicy::InheritPolicy; I hope there are  really good tests for that stuff.  If not, I can go back and read through it carefully...

>+++ b/dom/security/featurepolicy/FeaturePolicy.h
>+  AllowsFeatureInternal(const nsAString& aFeatureName,
>+                        const nsAString& aOrigin) const;

This needs to be documented.

>+  HasInheritedDeniedFeature(const nsAString& aFeatureName) const;
>+  HasDeclaredFeature(const nsAString& aFeatureName) const;

These need to be documented.

I did not audit FeaturePolicyParser::ParseString.  Let me know if I should.

>+++ b/dom/security/featurepolicy/FeaturePolicyUtils.h

The new functions here need documentation.  Do they match some spec algorithms?  If not, what do they do?

>+++ b/dom/webidl/HTMLIFrameElement.webidl
>+  [CEReactions, SetterThrows, Pure]
>+           attribute DOMString allow;

I don't see this in <https://wicg.github.io/feature-policy/#policy>.  Looks to me like it comes from https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-iframe-element and we should put it in the relevant part of this 
file.

We should also condition it on the pref, possibly...
Attachment #9012250 - Flags: feedback?(bzbarsky) → feedback+
Flags: needinfo?(amarchesini)
> Are we pushing for the interface to be renamed to "FeaturePolicy"?

yes, there is a spec issue about the renaming.
I'm going to apply all your comments in a separate bug.
Flags: needinfo?(amarchesini)
Blocks: 1496034
Blocks: 1497034
Blocks: 1497141
No longer depends on: 1495862
See Also: → 1495862
Blocks: 1497486
There are basic Feature-Policy docs on MDN now.

Guide pages    
https://developer.mozilla.org/en-US/docs/Web/HTTP/Feature_Policy
https://developer.mozilla.org/en-US/docs/Web/HTTP/Feature_Policy/Using_Feature_Policy

Reference pages
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-allow
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers#Security
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/fullscreen
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/geolocation
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/microphone
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/payment
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/vr
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/midi
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/autoplay
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/camera
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/encrypted-media
(we can document more directives once they become implemented more broadly)


We can update the compat data for Firefox once there is a bug that actually enables this outside of Nightly (please add "dev-doc-needed" to that bug). Meanwhile it is listed as an experimental Nightly feature: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features#Security (last row in the table)

:baku, please let me know if this looks good to you.
Flags: needinfo?(amarchesini)
Thanks! It looks great!
I have these comments:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Feature_Policy/Using_Feature_Policy

"This is the default value in order to be backwards compatible."
This is not true. We have 'self' for microphone, camera and midi. Soon fullscreen and payment.

"This is equivalent to:
<iframe src="https://example.com..." allow="fullscreen *"></iframe>"
no. it's equivalent to fullscreen 'src'.

"Disabling a feature in a policy is a one-way toggle. Once a feature is disabled, it cannot be re-enabled by any frame or descendant."
Unfortunately this is not true. See: https://github.com/WICG/feature-policy/issues/233

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy and the other URLs.

"This is the default value in order to be backwards compatible."
See before
Flags: needinfo?(amarchesini)
> "This is the default value in order to be backwards compatible."
This is not true. We have 'self' for microphone, camera and midi. Soon fullscreen and payment.

Ah, then I misunderstood bug 1496037.
Thanks for your review!

I've updated the defaults to 'self' on all pages, changed the iframe example to |allow="fullscreen 'src'"|, and removed the sentence about the one-way toggle.
Depends on: 1514296

How to I use it? MDN examples are so bad, I don't understand how to set it. 👎👎👎

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: