Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

content/common/feature_policy/feature_policy.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/common/feature_policy/feature_policy.h"
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 
 namespace content {
 
 namespace {
 
-// Given a string name, return the matching feature struct, or nullptr if it is
-// not the name of a policy-controlled feature.
-blink::WebFeaturePolicyFeature FeatureForName(
-    const std::string& feature_name,
-    const FeaturePolicy::FeatureList& features) {
-  for (const auto& feature_mapping : features) {
-    if (feature_name == feature_mapping.second->feature_name)
-      return feature_mapping.first;
-  }
-  return blink::WebFeaturePolicyFeature::NotFound;
-}
-
-// Definitions of all features controlled by Feature Policy should appear here.
-const FeaturePolicy::Feature kDocumentCookie{
-    "cookie", FeaturePolicy::FeatureDefault::EnableForAll};
-const FeaturePolicy::Feature kDocumentDomain{
-    "domain", FeaturePolicy::FeatureDefault::EnableForAll};
-const FeaturePolicy::Feature kDocumentWrite{
-    "docwrite", FeaturePolicy::FeatureDefault::EnableForAll};
-const FeaturePolicy::Feature kFullscreenFeature{
-    "fullscreen", FeaturePolicy::FeatureDefault::EnableForSelf};
-const FeaturePolicy::Feature kGeolocationFeature{
-    "geolocation", FeaturePolicy::FeatureDefault::EnableForSelf};
-const FeaturePolicy::Feature kMidiFeature{
-    "midi", FeaturePolicy::FeatureDefault::EnableForAll};
-const FeaturePolicy::Feature kNotificationsFeature{
-    "notifications", FeaturePolicy::FeatureDefault::EnableForAll};
-const FeaturePolicy::Feature kPaymentFeature{
-    "payment", FeaturePolicy::FeatureDefault::EnableForSelf};
-const FeaturePolicy::Feature kPushFeature{
-    "push", FeaturePolicy::FeatureDefault::EnableForAll};
-const FeaturePolicy::Feature kSyncScript{
-    "sync-script", FeaturePolicy::FeatureDefault::EnableForAll};
-const FeaturePolicy::Feature kSyncXHR{
-    "sync-xhr", FeaturePolicy::FeatureDefault::EnableForAll};
-const FeaturePolicy::Feature kUsermedia{
-    "usermedia", FeaturePolicy::FeatureDefault::EnableForAll};
-const FeaturePolicy::Feature kVibrateFeature{
-    "vibrate", FeaturePolicy::FeatureDefault::EnableForSelf};
-const FeaturePolicy::Feature kWebRTC{
-    "webrtc", FeaturePolicy::FeatureDefault::EnableForAll};
-
 // Extracts a Whitelist from a ParsedFeaturePolicyDeclaration.
 std::unique_ptr<FeaturePolicy::Whitelist> WhitelistFromDeclaration(
     const ParsedFeaturePolicyDeclaration& parsed_declaration) {
   std::unique_ptr<FeaturePolicy::Whitelist> result =
       base::WrapUnique(new FeaturePolicy::Whitelist());
   if (parsed_declaration.matches_all_origins)
     result->AddAll();
   for (const auto& origin : parsed_declaration.origins)
     result->Add(origin);
   return result;
 }
 
 }  // namespace
 
 ParsedFeaturePolicyDeclaration::ParsedFeaturePolicyDeclaration()
     : matches_all_origins(false) {}
 
 ParsedFeaturePolicyDeclaration::ParsedFeaturePolicyDeclaration(
-    std::string feature_name,
+    blink::WebFeaturePolicyFeature feature,
     bool matches_all_origins,
     std::vector<url::Origin> origins)
-    : feature_name(feature_name),
+    : feature(feature),
       matches_all_origins(matches_all_origins),
       origins(origins) {}
 
 ParsedFeaturePolicyDeclaration::ParsedFeaturePolicyDeclaration(
     const ParsedFeaturePolicyDeclaration& rhs) = default;
 
 ParsedFeaturePolicyDeclaration::~ParsedFeaturePolicyDeclaration() {}
 
 FeaturePolicy::Whitelist::Whitelist() : matches_all_origins_(false) {}
 
@@ skipping 50 matching lines @@
 
 bool FeaturePolicy::IsFeatureEnabled(
     blink::WebFeaturePolicyFeature feature) const {
   return IsFeatureEnabledForOrigin(feature, origin_);
 }
 
 bool FeaturePolicy::IsFeatureEnabledForOrigin(
     blink::WebFeaturePolicyFeature feature,
     const url::Origin& origin) const {
   DCHECK(base::ContainsKey(feature_list_, feature));
-  const FeaturePolicy::Feature* feature_definition = feature_list_.at(feature);
+  const FeaturePolicy::FeatureDefault default_policy =
+      feature_list_.at(feature);
   DCHECK(base::ContainsKey(inherited_policies_, feature));
   if (!inherited_policies_.at(feature))
     return false;
   auto whitelist = whitelists_.find(feature);
   if (whitelist != whitelists_.end())
     return whitelist->second->Contains(origin);
-  if (feature_definition->default_policy ==
-      FeaturePolicy::FeatureDefault::EnableForAll) {
+  if (default_policy == FeaturePolicy::FeatureDefault::EnableForAll) {
     return true;
   }
-  if (feature_definition->default_policy ==
-      FeaturePolicy::FeatureDefault::EnableForSelf) {
+  if (default_policy == FeaturePolicy::FeatureDefault::EnableForSelf) {
     // TODO(iclelland): Remove the pointer equality check once it is possible to
     // compare opaque origins successfully against themselves.
     // https://crbug.com/690520
     return (&origin_ == &origin) || origin_.IsSameOriginWith(origin);
   }
   return false;
 }
 
 void FeaturePolicy::SetHeaderPolicy(
     const ParsedFeaturePolicyHeader& parsed_header) {
   DCHECK(whitelists_.empty());
   for (const ParsedFeaturePolicyDeclaration& parsed_declaration :
        parsed_header) {
-    blink::WebFeaturePolicyFeature feature =
-        FeatureForName(parsed_declaration.feature_name, feature_list_);
-    if (feature == blink::WebFeaturePolicyFeature::NotFound)
-      continue;
+    blink::WebFeaturePolicyFeature feature = parsed_declaration.feature;
+    DCHECK(feature != blink::WebFeaturePolicyFeature::NotFound);

[raymes, 2017/03/09 02:52:00]

DCHECK_NE(...)

But could this happen in the browser process if there was a compromised
renderer? I wonder if we should still continue here?

[lunalu1, 2017/03/09 16:05:44]

Please correct me if I am wrong, to get a ParsedFeaturePolicyDeclaration,
ParseFeaturePolicy must have been called, where NotFound feature are dropped.
Could you please specify exactly what case where a NotFound feature is
re-introduced?

[raymes, 2017/03/12 23:43:39]

ParsedFeaturePolicyHeader is a member of FrameReplicationState (see
https://cs.chromium.org/chromium/src/content/common/frame_replication_state.h...)

It gets passed from the renderer to the browser process and then back to other
renderers. If a renderer is compromised, it could lie about its
ParsedFeaturePolicyHeader and introduce something different which is where this
could become an unexpected value.

Actually though, it turns out that we already check this with enum traits:
https://cs.chromium.org/chromium/src/content/common/frame_messages.h?rcl=a054...

So I think what you have is safe :) You can probably even remove this DHECK.

[lunalu1, 2017/03/13 15:23:30]

Lemme keep DCHECK here for now til most part of feature policy is implemented
and thoroughly tested. 

     whitelists_[feature] = WhitelistFromDeclaration(parsed_declaration);
   }
 }
 
 FeaturePolicy::FeaturePolicy(url::Origin origin,
                              const FeatureList& feature_list)
     : origin_(origin), feature_list_(feature_list) {}
 
 FeaturePolicy::FeaturePolicy(url::Origin origin)
     : origin_(origin), feature_list_(GetDefaultFeatureList()) {}
@@ skipping 23 matching lines @@
 
 void FeaturePolicy::AddContainerPolicy(
     const ParsedFeaturePolicyHeader& container_policy,
     const FeaturePolicy* parent_policy) {
   DCHECK(parent_policy);
   for (const ParsedFeaturePolicyDeclaration& parsed_declaration :
        container_policy) {
     // If a feature is enabled in the parent frame, and the parent chooses to
     // delegate it to the child frame, using the iframe attribute, then the
     // feature should be enabled in the child frame.
-    blink::WebFeaturePolicyFeature feature =
-        FeatureForName(parsed_declaration.feature_name, feature_list_);
+    blink::WebFeaturePolicyFeature feature = parsed_declaration.feature;
     if (feature == blink::WebFeaturePolicyFeature::NotFound)
       continue;
     if (WhitelistFromDeclaration(parsed_declaration)->Contains(origin_) &&
         parent_policy->IsFeatureEnabled(feature)) {
       inherited_policies_[feature] = true;
     } else {
       inherited_policies_[feature] = false;
     }
   }
 }
 
 // static
 const FeaturePolicy::FeatureList& FeaturePolicy::GetDefaultFeatureList() {
-  CR_DEFINE_STATIC_LOCAL(
-      FeatureList, default_feature_list,
-      ({{blink::WebFeaturePolicyFeature::DocumentCookie, &kDocumentCookie},
-        {blink::WebFeaturePolicyFeature::DocumentDomain, &kDocumentDomain},
-        {blink::WebFeaturePolicyFeature::DocumentWrite, &kDocumentWrite},
-        {blink::WebFeaturePolicyFeature::Fullscreen, &kFullscreenFeature},
-        {blink::WebFeaturePolicyFeature::Geolocation, &kGeolocationFeature},
-        {blink::WebFeaturePolicyFeature::MidiFeature, &kMidiFeature},
-        {blink::WebFeaturePolicyFeature::Notifications, &kNotificationsFeature},
-        {blink::WebFeaturePolicyFeature::Payment, &kPaymentFeature},
-        {blink::WebFeaturePolicyFeature::Push, &kPushFeature},
-        {blink::WebFeaturePolicyFeature::SyncScript, &kSyncScript},
-        {blink::WebFeaturePolicyFeature::SyncXHR, &kSyncXHR},
-        {blink::WebFeaturePolicyFeature::Usermedia, &kUsermedia},
-        {blink::WebFeaturePolicyFeature::Vibrate, &kVibrateFeature},
-        {blink::WebFeaturePolicyFeature::WebRTC, &kWebRTC}}));
+  CR_DEFINE_STATIC_LOCAL(FeatureList, default_feature_list,
+                         ({{blink::WebFeaturePolicyFeature::DocumentCookie,
+                            FeaturePolicy::FeatureDefault::EnableForAll},
+                           {blink::WebFeaturePolicyFeature::DocumentDomain,
+                            FeaturePolicy::FeatureDefault::EnableForAll},
+                           {blink::WebFeaturePolicyFeature::DocumentWrite,
+                            FeaturePolicy::FeatureDefault::EnableForAll},
+                           {blink::WebFeaturePolicyFeature::Fullscreen,
+                            FeaturePolicy::FeatureDefault::EnableForSelf},
+                           {blink::WebFeaturePolicyFeature::Geolocation,
+                            FeaturePolicy::FeatureDefault::EnableForSelf},
+                           {blink::WebFeaturePolicyFeature::MidiFeature,
+                            FeaturePolicy::FeatureDefault::EnableForAll},
+                           {blink::WebFeaturePolicyFeature::Notifications,
+                            FeaturePolicy::FeatureDefault::EnableForAll},
+                           {blink::WebFeaturePolicyFeature::Payment,
+                            FeaturePolicy::FeatureDefault::EnableForSelf},
+                           {blink::WebFeaturePolicyFeature::Push,
+                            FeaturePolicy::FeatureDefault::EnableForAll},
+                           {blink::WebFeaturePolicyFeature::SyncScript,
+                            FeaturePolicy::FeatureDefault::EnableForAll},
+                           {blink::WebFeaturePolicyFeature::SyncXHR,
+                            FeaturePolicy::FeatureDefault::EnableForAll},
+                           {blink::WebFeaturePolicyFeature::Usermedia,
+                            FeaturePolicy::FeatureDefault::EnableForAll},
+                           {blink::WebFeaturePolicyFeature::Vibrate,
+                            FeaturePolicy::FeatureDefault::EnableForSelf},
+                           {blink::WebFeaturePolicyFeature::WebRTC,
+                            FeaturePolicy::FeatureDefault::EnableForAll}}));

[raymes, 2017/03/09 02:52:00]

Orhtogonal to this CL: we should think about removing some of these at some
point. Some of them are out of date, like WebRTC and Usermedia. Push and
Notifications ar ealso not needed anymore.

[lunalu1, 2017/03/09 16:05:44]

Yes. Definitely. After I land in the code to add container policy. I will remove
/ update the feature list. 
So a separate CL would make it more clear what we are trying to do. WDYT? 

[raymes, 2017/03/12 23:43:39]

Sounds great :)

   return default_feature_list;
 }
 
 }  // namespace content