Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2254533002:
[FeaturePolicy] Initial implementation of Feature Policy (Closed)
|
Created:
4 years, 4 months ago by iclelland Modified:
4 years, 1 month ago CC:
chromium-reviews, tyoshino+watch_chromium.org, mlamouri+watch-blink_chromium.org, sof, eae+blinkwatch, dcheng, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, gavinp+loader_chromium.org, blink-reviews, Nate Chapin, loading-reviews_chromium.org, rwlbuis, alexmos Base URL:
https://chromium.googlesource.com/chromium/src.git@fp-flag Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[FeaturePolicy] Initial implementation of Feature Policy
This CL adds to each frame (local or remote) a FeaturePolicy object, which can be used to control the exposure and behaviour of web platform features. A frame's policy is computed from the policy declared in the "Feature-Policy" HTTP header, it's parent frame's policy, and the default policy for each feature.
See https://wicg.github.io/feature-policy/ for the specification in development.
Three features from the specification are implemented with this CL: cookie, domain, and docwrite.
(This behaviour is all conditional on the runtime flag for Feature Policy)
Future CLs are going to implement:
- <iframe> attribute support
- Layout tests
- Parser fuzzing
- Control over additional features
BUG=623682
Committed: https://crrev.com/7d15b7fc471b33e2d52a45876cb8323a4fb0e780
Cr-Commit-Position: refs/heads/master@{#429623}
Patch Set 1 #Patch Set 2 : Rebase; Fix policy combination and inheritance rules. #Patch Set 3 : Rebase #Patch Set 4 : Rewrite for new whitelist-only algorithm; add tests #Patch Set 5 : Remove overaggressive bool transform #
Total comments: 17
Patch Set 6 : Rebase #Patch Set 7 : Fixes; nested by-default feature policy inheritance #Patch Set 8 : Rebase, format #Patch Set 9 : Remove unneeded includes #Patch Set 10 : More closely follow the new algorithms for policy inheritance and resolution. #Patch Set 11 : Simplify processing; rebase #Patch Set 12 : Rebsae #Patch Set 13 : Clean up; add remaining policy-controlled features #
Total comments: 51
Patch Set 14 : Addressing review comments #Patch Set 15 : Conform to JFV spec for header format #
Total comments: 25
Patch Set 16 : Addressing review comments from PS#13 and #15 #
Total comments: 10
Patch Set 17 : Addressing comments from PS#16 #
Total comments: 3
Patch Set 18 : Don't leak FeaturePolicy objects #
Total comments: 12
Patch Set 19 : Addressing feedback from PS#18 #
Total comments: 6
Patch Set 20 : Rebase #Patch Set 21 : Addressing nits #Dependent Patchsets: Messages
Total messages: 104 (69 generated)
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [WIP][FeaturePolicy] Initial implementation of Feature Policy BUG=623682 ========== to ========== [WIP][FeaturePolicy] Initial implementation of Feature Policy BUG=623682 ==========
iclelland@chromium.org changed reviewers: + igrigorik@chromium.org, jyasskin@chromium.org, ojan@chromium.org, raymes@chromium.org
Mailing to interested parties as FYI
The diagrams in the test cases are super-helpful. Great stuff :) This doesn't handle <meta> and attribute policy, I assume those will be separate CLs?
Ian, thanks so much for getting this going! https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:58: bool FeaturePolicy::Whitelist::targets(const SecurityOrigin* origin) const nit: would "contains" be slightly clearer? https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:66: })); (optionally) the long-hand way isn't much longer for (const auto& target : origins) { if (target->isSameSchemeHostPortAndSuborigin(origin)) return true } return false; https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:85: void FeaturePolicy::addPolicyFromString(const String& policy) I think it's a little unclear what it means to "add" a policy. Will this ever need to be called multiple times (it doesn't seem to in this CL)? Should we instead just pass in the current frame's policy string on construction in the function above? That would avoid this function being abused. But perhaps there is a good reason for this :) If we do need it perhaps we can do something to clarify it. https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:92: } Hmm, what about the case where the parent sets "docwrite: a.com" and then the iframe a.com doesn't specify a policy. I think in that case we want the policy for a.com to resolve to * (the default policy) but I think this code will cause it to resolve to a.com for the subframe? (I might be wrong so please let me know if I'm missing something :). I actually think it might be useful to have a concept of a "resolved" policy for a frame. It's similar to a feature policy but there is a well defined whitelist for every feature (no features missing) which has taken into account header policy, meta policy and all the parent policies for that frame. Given a resolved policy of a parent frame and a {header policy, meta policy, embedding information} of the current frame, we should be able to produce a new resolved policy for the current frame. The distinction might help to avoid confusion between the structure representing what a developer specified in a header tag/meta tag and the internal structure representing what will actually be enforced. This is a mouthful and might be too complicated but let me know your thoughts :) https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:182: return whitelists; I haven't reviewed the parsing yet :) https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:65: // +-------------+ +1 thanks for these awesome diagrams!
lunalu@chromium.org changed reviewers: + lunalu@chromium.org
https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:529: if (!isLoadingMainFrame()) { Personal preference: FeaturePolicy::createFromParentPolicy(!isLoadingMainFrame() ? m_frame->client()->parent()->getFeaturePolicy() : nullptr, m_frame->securityContext()->getSecurityOrigin());
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:58: bool FeaturePolicy::Whitelist::targets(const SecurityOrigin* origin) const On 2016/09/19 09:19:05, raymes wrote: > nit: would "contains" be slightly clearer? Probably; I was using the nomenclature from the original spec design. Changed here because it makes sense. https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:66: })); On 2016/09/19 09:19:04, raymes wrote: > (optionally) the long-hand way isn't much longer > for (const auto& target : origins) > { > if (target->isSameSchemeHostPortAndSuborigin(origin)) > return true > } > return false; Thanks, that's clearer. (I always have such high hopes for lambdas, but they never work out :) ) https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:85: void FeaturePolicy::addPolicyFromString(const String& policy) On 2016/09/19 09:19:04, raymes wrote: > I think it's a little unclear what it means to "add" a policy. Will this ever > need to be called multiple times (it doesn't seem to in this CL)? Should we > instead just pass in the current frame's policy string on construction in the > function above? That would avoid this function being abused. But perhaps there > is a good reason for this :) If we do need it perhaps we can do something to > clarify it. I wouldn't want to pass in the current frame's string, since we'd have to serialize it from the current policy (which might not be the same as the string in the header or meta tag, depending on the parent frame policy) The intention was to initialize a policy with the parent's policy, and then to use this method to add the declarations from the HTTP header string, then use it again with the <meta> tag content, if present. Each invocation would further restrict the current policy. The meta tag parsing isn't present yet, but I will add some unit tests to ensure that this method works as intended. https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:92: } On 2016/09/19 09:19:05, raymes wrote: > Hmm, what about the case where the parent sets "docwrite: a.com" and then the > iframe a.com doesn't specify a policy. I think in that case we want the policy > for a.com to resolve to * (the default policy) but I think this code will cause > it to resolve to a.com for the subframe? (I might be wrong so please let me know > if I'm missing something :). No, I think you're right, and I didn't catch that case. I've added a step in |createFromParentPolicy| that checks to see whether the feature should be enabled by default in nested contexts. In that case, enabling it for a.com in the parent means that unless a.com says otherwise, it gets "*". This isn't the case for policies like vibrate, which aren't enabled in nested contexts, and so a.com needs to explicitly enable it's embeddees, and so on down the chain. I added two test cases for it as well. > > I actually think it might be useful to have a concept of a "resolved" policy for > a frame. It's similar to a feature policy but there is a well defined whitelist > for every feature (no features missing) which has taken into account header > policy, meta policy and all the parent policies for that frame. Given a resolved > policy of a parent frame and a {header policy, meta policy, embedding > information} of the current frame, we should be able to produce a new resolved > policy for the current frame. The distinction might help to avoid confusion > between the structure representing what a developer specified in a header > tag/meta tag and the internal structure representing what will actually be > enforced. This is a mouthful and might be too complicated but let me know your > thoughts :) That's an interesting idea; the current FeaturePolicy is sort-of that resolved policy, or at least it's intended to be. The biggest difference, I think, is that any features which haven't been mentioned by any policy aren't explicitly represented; we use the defaults in that case. I'll give that some more thought, though. It might help clarify some of the behaviour around inheritance and policy layering. https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:182: return whitelists; On 2016/09/19 09:19:05, raymes wrote: > I haven't reviewed the parsing yet :) NP; I haven't written a fuzzer for it yet either :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:92: } Hmm, I'm still not sure I feel comfortable with this approach - it's hard to see that it matches the algorithm in https://docs.google.com/document/d/1saEBLE6d-qmMxao8Aw6A06oSP3yKdUG7lqMWBYlzE.... I realize that the algorithm assumes we have all the information at once, which we might not in practice. Maybe we can set up a meeting to chat next week?
https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:92: } On 2016/09/29 07:17:00, raymes wrote: > Hmm, I'm still not sure I feel comfortable with this approach - it's hard to see > that it matches the algorithm in > https://docs.google.com/document/d/1saEBLE6d-qmMxao8Aw6A06oSP3yKdUG7lqMWBYlzE.... > I realize that the algorithm assumes we have all the information at once, which > we might not in practice. Maybe we can set up a meeting to chat next week? Sounds like a good idea. I'll see if I can find some time on your calendar.
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
raymes, can you PTAL again? The |createFromParentPolicy| and |isFeatureEnabledForOrigin| methods follow pretty closely the algorithms we converged on (except that they don't incorporate <meta> tags or <iframe> attributes yet). https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:529: if (!isLoadingMainFrame()) { On 2016/09/20 14:55:49, loonybear wrote: > Personal preference: > FeaturePolicy::createFromParentPolicy(!isLoadingMainFrame() ? > m_frame->client()->parent()->getFeaturePolicy() : nullptr, > m_frame->securityContext()->getSecurityOrigin()); Done (except that I omitted the "!" and swapped the two sides.) https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:58: bool FeaturePolicy::Whitelist::targets(const SecurityOrigin* origin) const On 2016/09/27 15:06:27, iclelland wrote: > On 2016/09/19 09:19:05, raymes wrote: > > nit: would "contains" be slightly clearer? > > Probably; I was using the nomenclature from the original spec design. Changed > here because it makes sense. Done. https://codereview.chromium.org/2254533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:182: return whitelists; On 2016/09/27 15:06:28, iclelland wrote: > On 2016/09/19 09:19:05, raymes wrote: > > I haven't reviewed the parsing yet :) > > NP; I haven't written a fuzzer for it yet either :) https://codereview.chromium.org/2420013004 FYI
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.h (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.h:115: m_featurePolicy = newPolicy; What's the plan for remote frames? I guess we'd need to sync their feature policy too? Can a feature policy change after a new Document is loaded in this frame? https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:58: m_origins.append(origin); Nit: std::move() https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:61: bool FeaturePolicy::Whitelist::contains(const SecurityOrigin* origin) const { Nit: const ref if we never expect this to be null. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:90: DEFINE_STATIC_LOCAL( No need to use a Vector here; range-based for loops work over plain old arrays: const FeaturePolicy kFeatureRegistry[] = { {"cookie", 32, kEnableFeatureForAllOrigins}, {"domain", 34, kEnableFeatureForAllOrigins}, ... }; for (const FeaturePolicyFeature& feature : kFeatureRegistry) { ... } https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:151: FeaturePolicy::FeaturePolicy(PassRefPtr<SecurityOrigin> currentOrigin) Nit: PassRefPtr is on the way out. Just use RefPtr here and std::move() into m_origin.
https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.h (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.h:115: m_featurePolicy = newPolicy; On 2016/10/14 20:33:16, dcheng wrote: > What's the plan for remote frames? I guess we'd need to sync their feature > policy too? Yes, we'll need to make sure that the corresponding localframe has an identical policy to the remote frame. > Can a feature policy change after a new Document is loaded in this frame? Changing the policy shouldn't be possible, I think. Detaching the frame from its parent shouldn't affect its allowed feature set, and re-attaching elsewhere should trigger a content reload (re-evaluating the policy in the new location). Similarly, if the policy has been affected by attributes on the iframe (not implemented in this CL), then changing those should also trigger a reload, generating a new policy. Do you mean to suggest that a setter is unneeded? (If so, I think I can agree) https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:90: DEFINE_STATIC_LOCAL( On 2016/10/14 20:33:17, dcheng wrote: > No need to use a Vector here; range-based for loops work over plain old arrays: > > const FeaturePolicy kFeatureRegistry[] = { > {"cookie", 32, kEnableFeatureForAllOrigins}, > {"domain", 34, kEnableFeatureForAllOrigins}, > ... > }; > > for (const FeaturePolicyFeature& feature : kFeatureRegistry) { > ... > } It's mostly a vector so that it can be appended to in tests (see the constructor for FeaturePolicyTest) -- there may be a better way, though. I'll see if I can improve on that. Thanks.
Thanks Ian, I feel much better about this! :) I haven't re-reviewed the tests yet but I looked at the parsing code. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:89: // If there is no frame, or if feature policy is disabled, use defaults nit: "." at end of sentence https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:98: // Otherwise, check policy nit: same here. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:99: return featurePolicy->isFeatureEnabledForOrigin( nit: couldn't this just be isFeatureEnabled? https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:116: void FeaturePolicy::addPolicyFromString(const String& policy) { I don't like the "addPolicyFromString" name/approach because I feel it's too general and so prone to be misused. Could we instead just make this specific and say "setHeaderPolicy" or "enforceHeaderPolicy"? We can address how to factor in meta policy as a next step. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:118: return; Can we have a check: DCHECK(m_declaredWhitelists.empty()) for now? I think it's good to verify that this is only called once (I realize that would change with meta, but we can address that separately :) https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:120: if (isFeatureEnabled(whitelist.key)) { I think this check is complex and hard to verify correctness because under the hood it's looking at the current declared whitelist, but then ends up changing it. I think it's not needed for now since we don't have meta policy yet and it's clearer to see what's going on if we leave it out. That way the declaredWhitelists matches what was specified in the header policy. If the feature isn't enabled because of the inherited policy, it will be enforced correctly in isFeatureEnabledForOrigin. We can address adding in the meta policy separately (actually I think it's better to define how to combine header/meta policy into a "declared policy" in the algorithms doc first). https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:121: m_declaredWhitelists.set(whitelist.key, whitelist.value); nit: Could we just call this m_headerWhitelists for now? When we add the logic for meta we can generalize? https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:128: const SecurityOrigin* origin) const { It would be nice to link to an algorithm here but I don't think we have a canonical link yet :) https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:129: if (m_inheritedFeatures.contains(feature)) { I think the behavior if a feature isn't in the map is unclear and so more bug prone. Can we just initialize all the features in the constructor? If there is no parent frame then we would initialize all the features to true for the top level initialized policy. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:159: FeaturePolicy::parse(const String& policy) { I think parsing code tends to fit better not as a member function. Could we instead just make this a standalone function and pass the origin of the policy owner in as a parameter? https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:173: continue; // Array element is not an object; skip Hmm, should we just give up at each of these failure points? I'm open to other alternatives though. I think we should probably display a console error in each case if possible though - should we return an error string from the function? https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:214: for (const auto& inheritedFeature : m_inheritedFeatures) { nit, optional: should we indent the string here for easy reading? https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:221: for (const auto& whitelist : m_declaredWhitelists) { nit: same here https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:26: // Equivalent to ["self"]. The feature is enabled for top-level frames, but nit: I think "top-level" is a bit misleading, because it specifically means the main frame. How about "the current frame". https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:30: // Equivalent to ["*"]. The feature is enabled by default for all frames, but all frames->for the current frame and all child frames ? https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:32: // reenabled by any of that frame's children. nit: these 2 lines are a little confusing. I think the first line is sufficient and the algorithm can be studied to determine the behavior when defaults are overridden. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:38: // given feature (declared below.) nit: declared below). https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:78: // Returns whether or not the given feature is enabled for the policy's nit: for the origin of the frame that owns the policy.
https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.h (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.h:115: m_featurePolicy = newPolicy; On 2016/10/15 01:55:34, iclelland wrote: > On 2016/10/14 20:33:16, dcheng wrote: > > What's the plan for remote frames? I guess we'd need to sync their feature > > policy too? > > Yes, we'll need to make sure that the corresponding localframe has an identical > policy to the remote frame. > > > Can a feature policy change after a new Document is loaded in this frame? > Changing the policy shouldn't be possible, I think. Well, a LocalFrame can be used for multiple Document loads. Each of those Documents could have a different FeaturePolicy, right? > > Detaching the frame from its parent shouldn't affect its allowed feature set, > and re-attaching elsewhere should trigger a content reload (re-evaluating the > policy in the new location). > > Similarly, if the policy has been affected by attributes on the iframe (not > implemented in this CL), then changing those should also trigger a reload, > generating a new policy. Today in Blink, changes to iframe attributes don't affect the subframe itself until it's reloaded in some way. https://crbug.com/642603 is one interop bug that's been filed as a result of this. > > Do you mean to suggest that a setter is unneeded? (If so, I think I can agree)
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:89: // If there is no frame, or if feature policy is disabled, use defaults On 2016/10/18 02:42:30, raymes wrote: > nit: "." at end of sentence Done. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:98: // Otherwise, check policy On 2016/10/18 02:42:30, raymes wrote: > nit: same here. Done. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:99: return featurePolicy->isFeatureEnabledForOrigin( On 2016/10/18 02:42:30, raymes wrote: > nit: couldn't this just be isFeatureEnabled? Probably; we shouldn't ever be using a policy for a different origin than the frame's origin here. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.h (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.h:115: m_featurePolicy = newPolicy; On 2016/10/18 04:11:18, dcheng wrote: > On 2016/10/15 01:55:34, iclelland wrote: > > On 2016/10/14 20:33:16, dcheng wrote: > > > What's the plan for remote frames? I guess we'd need to sync their feature > > > policy too? > > > > Yes, we'll need to make sure that the corresponding localframe has an > identical > > policy to the remote frame. > > > > > Can a feature policy change after a new Document is loaded in this frame? > > Changing the policy shouldn't be possible, I think. > > Well, a LocalFrame can be used for multiple Document loads. Each of those > Documents could have a different FeaturePolicy, right? Yes, I misread. After a document is loaded into the frame, the policy should never change *until* another document is loaded into the same frame. Each of those documents would have a different policy. > > > > > Detaching the frame from its parent shouldn't affect its allowed feature set, > > and re-attaching elsewhere should trigger a content reload (re-evaluating the > > policy in the new location). > > > > Similarly, if the policy has been affected by attributes on the iframe (not > > implemented in this CL), then changing those should also trigger a reload, > > generating a new policy. > > Today in Blink, changes to iframe attributes don't affect the subframe itself > until it's reloaded in some way. https://crbug.com/642603 is one interop bug > that's been filed as a result of this. Perhaps we should follow the lead of the 'sandbox' attribute, and claim that it only takes effect on navigation. That's an issue for the spec discussion, though; I'll raise it there. > > > > > Do you mean to suggest that a setter is unneeded? (If so, I think I can agree) > https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:58: m_origins.append(origin); On 2016/10/14 20:33:17, dcheng wrote: > Nit: std::move() Done. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:61: bool FeaturePolicy::Whitelist::contains(const SecurityOrigin* origin) const { On 2016/10/14 20:33:17, dcheng wrote: > Nit: const ref if we never expect this to be null. Done. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:116: void FeaturePolicy::addPolicyFromString(const String& policy) { On 2016/10/18 02:42:31, raymes wrote: > I don't like the "addPolicyFromString" name/approach because I feel it's too > general and so prone to be misused. Could we instead just make this specific and > say "setHeaderPolicy" or "enforceHeaderPolicy"? We can address how to factor in > meta policy as a next step. Done. Changed to setHeaderPolicy. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:118: return; On 2016/10/18 02:42:31, raymes wrote: > Can we have a check: > DCHECK(m_declaredWhitelists.empty()) for now? I think it's good to verify that > this is only called once (I realize that would change with meta, but we can > address that separately :) Done. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:121: m_declaredWhitelists.set(whitelist.key, whitelist.value); On 2016/10/18 02:42:30, raymes wrote: > nit: Could we just call this m_headerWhitelists for now? When we add the logic > for meta we can generalize? Done. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:128: const SecurityOrigin* origin) const { On 2016/10/18 02:42:31, raymes wrote: > It would be nice to link to an algorithm here but I don't think we have a > canonical link yet :) Acknowledged. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:129: if (m_inheritedFeatures.contains(feature)) { On 2016/10/18 02:42:31, raymes wrote: > I think the behavior if a feature isn't in the map is unclear and so more bug > prone. Can we just initialize all the features in the constructor? If there is > no parent frame then we would initialize all the features to true for the top > level initialized policy. Done. That matches the current text in the explainer as well. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:151: FeaturePolicy::FeaturePolicy(PassRefPtr<SecurityOrigin> currentOrigin) On 2016/10/14 20:33:17, dcheng wrote: > Nit: PassRefPtr is on the way out. Just use RefPtr here and std::move() into > m_origin. Done. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:159: FeaturePolicy::parse(const String& policy) { On 2016/10/18 02:42:31, raymes wrote: > I think parsing code tends to fit better not as a member function. Could we > instead just make this a standalone function and pass the origin of the policy > owner in as a parameter? Done. Renamed and moved to the anonymous namespace. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:173: continue; // Array element is not an object; skip On 2016/10/18 02:42:31, raymes wrote: > Hmm, should we just give up at each of these failure points? I'm open to other > alternatives though. I think we should probably display a console error in each > case if possible though - should we return an error string from the function? We can't fail at "Feature not recognized", as that would break compatibility with future iterations of the spec, and not allow other browsers to experiment with policies on features we don't support. Similarly, I'm not sure that we should fail because we don't recognize an origin; there may be legitimate reasons for another browser in a different environment to use hostnames that we don't currently understand, and I'd like to just pass over them, knowing that Chrome code will never need to test against them. Happy to fail on structural problems, though -- if you don't use JSON, or don't use the right JSON structure, then something is badly wrong with the policy, and maybe we shouldn't allow it at all. (With a console error if possible). (I still need to refactor this to return an error in that case, then) https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:214: for (const auto& inheritedFeature : m_inheritedFeatures) { On 2016/10/18 02:42:30, raymes wrote: > nit, optional: should we indent the string here for easy reading? Done. This code is currently just so I can log the state of the policy objects; I don't know if it will survive into the final product. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:221: for (const auto& whitelist : m_declaredWhitelists) { On 2016/10/18 02:42:31, raymes wrote: > nit: same here Done. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:26: // Equivalent to ["self"]. The feature is enabled for top-level frames, but On 2016/10/18 02:42:31, raymes wrote: > nit: I think "top-level" is a bit misleading, because it specifically means the > main frame. How about "the current frame". I think that's the point, though -- it's not true that it's "granted for the current frame if neither the current frame nor any ancestor frames have declared an explicit policy", which is how that would read. If the default is "self", then it's available by default at the top level, but must be explicitly delegated. The "self" keyword is evaluated once, in the main frame, where it is expanded to that frame's origin. After that, the usual inheritance rules apply. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:38: // given feature (declared below.) On 2016/10/18 02:42:31, raymes wrote: > nit: declared below). Done. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:78: // Returns whether or not the given feature is enabled for the policy's On 2016/10/18 02:42:31, raymes wrote: > nit: for the origin of the frame that owns the policy. Done.
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks great! Mostly just nits now. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.h (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.h:115: m_featurePolicy = newPolicy; On 2016/10/19 12:51:55, iclelland wrote: > On 2016/10/18 04:11:18, dcheng wrote: > > On 2016/10/15 01:55:34, iclelland wrote: > > > On 2016/10/14 20:33:16, dcheng wrote: > > > > What's the plan for remote frames? I guess we'd need to sync their feature > > > > policy too? > > > > > > Yes, we'll need to make sure that the corresponding localframe has an > > identical > > > policy to the remote frame. > > > > > > > Can a feature policy change after a new Document is loaded in this frame? > > > Changing the policy shouldn't be possible, I think. > > > > Well, a LocalFrame can be used for multiple Document loads. Each of those > > Documents could have a different FeaturePolicy, right? > > Yes, I misread. After a document is loaded into the frame, the policy should > never change *until* another document is loaded into the same frame. Each of > those documents would have a different policy. > > > > > > > > Detaching the frame from its parent shouldn't affect its allowed feature > set, > > > and re-attaching elsewhere should trigger a content reload (re-evaluating > the > > > policy in the new location). > > > > > > Similarly, if the policy has been affected by attributes on the iframe (not > > > implemented in this CL), then changing those should also trigger a reload, > > > generating a new policy. > > > > Today in Blink, changes to iframe attributes don't affect the subframe itself > > until it's reloaded in some way. https://crbug.com/642603 is one interop bug > > that's been filed as a result of this. > > Perhaps we should follow the lead of the 'sandbox' attribute, and claim that it > only takes effect on navigation. That's an issue for the spec discussion, > though; I'll raise it there. > > > > > > > > > Do you mean to suggest that a setter is unneeded? (If so, I think I can > agree) > > > +1 for following sandbox attribute :) https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:173: continue; // Array element is not an object; skip On 2016/10/19 12:51:55, iclelland wrote: > On 2016/10/18 02:42:31, raymes wrote: > > Hmm, should we just give up at each of these failure points? I'm open to other > > alternatives though. I think we should probably display a console error in > each > > case if possible though - should we return an error string from the function? > > We can't fail at "Feature not recognized", as that would break compatibility > with future iterations of the spec, and not allow other browsers to experiment > with policies on features we don't support. > Similarly, I'm not sure that we should fail because we don't recognize an > origin; there may be legitimate reasons for another browser in a different > environment to use hostnames that we don't currently understand, and I'd like to > just pass over them, knowing that Chrome code will never need to test against > them. > > Happy to fail on structural problems, though -- if you don't use JSON, or don't > use the right JSON structure, then something is badly wrong with the policy, and > maybe we shouldn't allow it at all. (With a console error if possible). > > (I still need to refactor this to return an error in that case, then) That all sounds good to me :) I'm happy if you want to do this in a followup CL. Perhaps add a TODO (or file a bug)? https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:26: // Equivalent to ["self"]. The feature is enabled for top-level frames, but I guess my main concern with the wording was that it only says something about how the default affects the top-level frame. But defaults don't only have an impact on the top-level frame, right? Don't they impact all frames? How do we express the impact "self" would have on an arbitrarily nested frame that relied on a default as opposed to specifying a whitelist? I think I was taking the perspective of a developer who is thinking about their page that might be arbitrarily embedded. They would ask the question "how will this default affect my page if I don't override it?". That led to my suggestion: "The feature will be enabled for the current frame, but not for any child frames". Of course this depends on the current frame having access to the feature in the first place (which I omitted :) https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:177: m_headerWhitelists.set(whitelist.key, whitelist.value); Could we just set m_headerWhitelists = parseFeaturePolicyString(...); here? (I might be missing something :) https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:184: if (m_inheritedFeatures.contains(feature)) { Can we remove this now (or make it a DCHECK?) https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:13: #define ORIGIN_C "https://example.org/" nit: can these just be const char*? I haven't seen this style much but it may be a blink thing? https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:41: featureRegistry.append(&kOffByDeaultFeature); nit (optional): should we just make all of these invented features, defined in this file? https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:48: RefPtr<SecurityOrigin> originA = SecurityOrigin::createFromString(ORIGIN_A); nit: should these be prefixed with m_ (I don't know blink style well, so I may be wrong :)? https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:58: EXPECT_TRUE(invalidPolicy); Hmm, what is this testing? https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:86: // top-level-only features should be disabled. nit: I think this comment is slightly out of date? The top-level only feature is enabled because the nested frame is the same origin
https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:88: const Frame* frame) { Make this a LocalFrame. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.h (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.h:114: void setFeaturePolicy(FeaturePolicy* newPolicy) { For replicating this so it'll work for OOPIF, look at the plumbing around https://cs.chromium.org/chromium/src/content/renderer/render_frame_proxy.cc?r... (I'm not entirely sure what serialization format we should use for IPC here though.) https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:41: std::unique_ptr<JSONArray> items = JSONArray::cast(std::move(policyJSON)); The input is already a JSONArray, so this cast is unnecessary. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:21: enum FeaturePolicyFeatureDefault { Nit: prefer enum classes in new code https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:71: class Whitelist final : public GarbageCollectedFinalized<Whitelist> { I think this doesn't need to be GCed (nor FeaturePolicy itself) https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:37: FeaturePolicy::getFeatureRegistry(); Doesn't this copy the feature registry? So I don't think any of the changes here are actually affecting the policies. Since the unit test is just testing createFromParentPolicy, maybe it'd be easier to just have a const default registry, and let createFromParentPolicy take in a pointer to the registry to use. Then tests can pass in whatever custom registry they like, and we can avoid shooting ourselves in the foot from code that accidentally mutates the feature policy outside of tests.
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:26: // Equivalent to ["self"]. The feature is enabled for top-level frames, but On 2016/10/19 23:47:36, raymes wrote: > I guess my main concern with the wording was that it only says something about > how the default affects the top-level frame. But defaults don't only have an > impact on the top-level frame, right? Don't they impact all frames? How do we > express the impact "self" would have on an arbitrarily nested frame that relied > on a default as opposed to specifying a whitelist? > > I think I was taking the perspective of a developer who is thinking about their > page that might be arbitrarily embedded. They would ask the question "how will > this default affect my page if I don't override it?". That led to my suggestion: > "The feature will be enabled for the current frame, but not for any child > frames". Of course this depends on the current frame having access to the > feature in the first place (which I omitted :) The wording is wrong, now that I've given it some more thought; I was considering only cross-origin frame inclusion. The actual effect of default:self is that the feature is enabled by default at the top-level, but that does not persist across origin boundaries. Every time you include a cross-origin frame, you need to explicitly grant the new frame access to the feature. I've updated the wording, and added a new test case for that as well. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:30: // Equivalent to ["*"]. The feature is enabled by default for all frames, but On 2016/10/18 02:42:31, raymes wrote: > all frames->for the current frame and all child frames ? I don't think "the current frame" means anything when you're declaring the default policy for a feature, though. If you set the default to ["*"], you're enabling the feature by default for all frames. https://codereview.chromium.org/2254533002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:32: // reenabled by any of that frame's children. On 2016/10/18 02:42:31, raymes wrote: > nit: these 2 lines are a little confusing. I think the first line is sufficient > and the algorithm can be studied to determine the behavior when defaults are > overridden. Done. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:88: const Frame* frame) { On 2016/10/20 17:41:44, dcheng wrote: > Make this a LocalFrame. Done. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:41: std::unique_ptr<JSONArray> items = JSONArray::cast(std::move(policyJSON)); On 2016/10/20 17:41:44, dcheng wrote: > The input is already a JSONArray, so this cast is unnecessary. Done. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:177: m_headerWhitelists.set(whitelist.key, whitelist.value); On 2016/10/19 23:47:36, raymes wrote: > Could we just set > m_headerWhitelists = parseFeaturePolicyString(...); > here? > > (I might be missing something :) I think we can -- this used to be guarded, feature-by-feature, by "if(featureEnabled(whitelist.key))", so that additional policy strings (like meta tags) couldn't re-enable features. Simpler now, thanks :) https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:184: if (m_inheritedFeatures.contains(feature)) { On 2016/10/19 23:47:36, raymes wrote: > Can we remove this now (or make it a DCHECK?) I'll make it a dcheck -- it'll just guard against anyone trying to test a feature struct that isn't in the registry. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:21: enum FeaturePolicyFeatureDefault { On 2016/10/20 17:41:44, dcheng wrote: > Nit: prefer enum classes in new code Done. Renamed values to avoid unnecessary 'k' as well. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:71: class Whitelist final : public GarbageCollectedFinalized<Whitelist> { On 2016/10/20 17:41:44, dcheng wrote: > I think this doesn't need to be GCed (nor FeaturePolicy itself) Done; Removed oilpan. I think the pointer management still makes sense, but please take a look. I ended up adding it as a unique_ptr in Frame.h, which doesn't seem as clean as Member<T>, but works. An alternative is to make it a member var, and either implement copy/assign or add functions to reset the state of the policy, so that it can be updated in FrameLoader. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:13: #define ORIGIN_C "https://example.org/" On 2016/10/19 23:47:37, raymes wrote: > nit: can these just be const char*? I haven't seen this style much but it may be > a blink thing? I'm using the macros so that we can do easy string substitution to build the test policies. In tests, I create a policy string like: "{\"vibrate\": [\"" ORIGIN_A "\"]}" and the preprocessor's string concatenation takes care of building the correct string. We couldn't do that with char*. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:37: FeaturePolicy::getFeatureRegistry(); On 2016/10/20 17:41:44, dcheng wrote: > Doesn't this copy the feature registry? So I don't think any of the changes here > are actually affecting the policies. > > Since the unit test is just testing createFromParentPolicy, maybe it'd be easier > to just have a const default registry, and let createFromParentPolicy take in a > pointer to the registry to use. Then tests can pass in whatever custom registry > they like, and we can avoid shooting ourselves in the foot from code that > accidentally mutates the feature policy outside of tests. I had fixed this this afternoon, but you're right that a pointer to a default registry would work. I've changed it to use const references everywhere, with a default built in to the FeaturePolicy class (so that call sites don't have to know the correct list of features, but the tests can still override). PTAL to see if that is satisfactory, thanks! https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:41: featureRegistry.append(&kOffByDeaultFeature); On 2016/10/19 23:47:37, raymes wrote: > nit (optional): should we just make all of these invented features, defined in > this file? Done. I thought that using the real features would make the test cases more intuitively understandable, but this is more resilient against features being removed from the policy spec. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:48: RefPtr<SecurityOrigin> originA = SecurityOrigin::createFromString(ORIGIN_A); On 2016/10/19 23:47:37, raymes wrote: > nit: should these be prefixed with m_ (I don't know blink style well, so I may > be wrong :)? Done. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:58: EXPECT_TRUE(invalidPolicy); On 2016/10/19 23:47:37, raymes wrote: > Hmm, what is this testing? Ahh, I had implemented that test yesterday, but didn't upload it. Done now. https://codereview.chromium.org/2254533002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:86: // top-level-only features should be disabled. On 2016/10/19 23:47:37, raymes wrote: > nit: I think this comment is slightly out of date? The top-level only feature is > enabled because the nested frame is the same origin Thanks, fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:54: continue; // Whitelist is not an array; skip nit: I think this comment is redundant now https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:29: // child frames. Preface: Since this is just comments I don't feel too concerned and won't spend more of your time on this, but some suggestions below :) That's a good point that there isn't really any concept of a "current frame" when defining a default for a feature. It still feels a bit like the recursive case is missing though, e.g. what effect will the default policy of on the child frames if it takes effect (this comment only says the impact on the top level frame). I guess to be very specific (and verbose) about the impact a default policy for FEATURE will have on a given FRAME is, 1) If FEATURE is enabled for FRAME by its parents, and 2) If the default policy for FEATURE isn't overridden by a header policy in FRAME Then the default policy will take effect for FRAME, otherwise the default policy won't have any effect. If the default policy takes effect, then: -If the default policy is [] then FEATURE won't be enabled for FRAME or any of its children -If the default policy is ["self"] then FEATURE will be enabled for FRAME but not its children -If the default policy is ["*"] then FEATURE will be enabled for FRAME and all of its children. https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:39: struct FeaturePolicyFeature { nit (optional): if we move the enum above and this class into the public section of FeaturePolicy it could make the naming simpler. Then we would just have: FeaturePolicy::FeatureDefault and FeaturePolicy::Feature Additionally/alternatively, FeatureDefault could be defined in Feature, so that it would be: FeaturePolicy::Feature::Default. https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:93: FeatureList& features); Is this just for testing? It might be better to make it private and add a comment mentioning it? https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:451: EXPECT_TRUE(policy2->isFeatureEnabled(&kDefaultOffFeature)); Hmm, the code here doesn't seem to match what's in the diagram. (2) sets a header policy of "self", same with (4). Without that, I think this line should evaluate to FALSE as well - that is, (2) falls back to the default policy which is to have the feature disabled in itself, even though the parent allows the feature to be enabled there.
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:54: continue; // Whitelist is not an array; skip On 2016/10/23 23:31:05, raymes wrote: > nit: I think this comment is redundant now Yes, it is :) https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:29: // child frames. On 2016/10/23 23:31:05, raymes wrote: > Preface: Since this is just comments I don't feel too concerned and won't spend > more of your time on this, but some suggestions below :) > > That's a good point that there isn't really any concept of a "current frame" > when defining a default for a feature. It still feels a bit like the recursive > case is missing though, e.g. what effect will the default policy of on the child > frames if it takes effect (this comment only says the impact on the top level > frame). > > I guess to be very specific (and verbose) about the impact a default policy for > FEATURE will have on a given FRAME is, > 1) If FEATURE is enabled for FRAME by its parents, and > 2) If the default policy for FEATURE isn't overridden by a header policy in > FRAME > Then the default policy will take effect for FRAME, otherwise the default policy > won't have any effect. > > If the default policy takes effect, then: > -If the default policy is [] then FEATURE won't be enabled for FRAME or any of > its children > -If the default policy is ["self"] then FEATURE will be enabled for FRAME but > not its children > -If the default policy is ["*"] then FEATURE will be enabled for FRAME and all > of its children. Thanks, I'll incorporate that -- I think it may make sense to move much of this into explanatory comments for the FeaturePolicy class at this point -- especially with the name scoping you suggest below. https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:39: struct FeaturePolicyFeature { On 2016/10/23 23:31:05, raymes wrote: > nit (optional): if we move the enum above and this class into the public section > of FeaturePolicy it could make the naming simpler. Then we would just have: > FeaturePolicy::FeatureDefault and > FeaturePolicy::Feature > > Additionally/alternatively, FeatureDefault could be defined in Feature, so that > it would be: > FeaturePolicy::Feature::Default. Done. https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:93: FeatureList& features); On 2016/10/23 23:31:05, raymes wrote: > Is this just for testing? It might be better to make it private and add a > comment mentioning it? Yes, this interface is just for testing, although it is also used internally by the intentionally-public createFromParentPolicy(parent, origin) method. I'll make it private and friend the test class. https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2254533002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:451: EXPECT_TRUE(policy2->isFeatureEnabled(&kDefaultOffFeature)); On 2016/10/23 23:31:05, raymes wrote: > Hmm, the code here doesn't seem to match what's in the diagram. (2) sets a > header policy of "self", same with (4). Without that, I think this line should > evaluate to FALSE as well - that is, (2) falls back to the default policy which > is to have the feature disabled in itself, even though the parent allows the > feature to be enabled there. Thanks, you're right -- I forgot to update the diagram there to match that.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm! Thanks! https://codereview.chromium.org/2254533002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:52: // feature is available when no policy has been declared, ans determines how the nit: and https://codereview.chromium.org/2254533002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:55: // If the default policy is in effect for a frame, then it controls how the nit: double space https://codereview.chromium.org/2254533002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:66: // FeaturePolicy::DefaultPolicy for details) I find this section a bit confusing, because inheritance makes me think that the whitelists are actually inherited. Perhaps the key points are: -Frames can control what features are enabled in themselves or in child frames -If a feature is disabled in a child frame, it cannot be re-enabled by that child frame or any of its descendants. -If no policy is explicitly specified for a frame, the default policy is used (although this might be redundant with the above).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Would you help me understand your plan about the relationship between OriginTrial and FeaturePolicy in terms of V8 bindings?
On 2016/10/24 08:51:19, haraken wrote: > Would you help me understand your plan about the relationship between > OriginTrial and FeaturePolicy in terms of V8 bindings? The long term plan is to unify them -- they are both going to be conditional features; not included in the template, but added as soon as we determine that they are required on an interface. In my mind, they are essentially the same thing, except that one has always been part of the platform, and now needs to be selectively removed, while the other is for new features which are *not* present on most pages. In both cases, though, they will be added only as necessary. https://docs.google.com/document/d/1PHkCEyfUKwhoJEiRwcio9JWU5xQ1OuvIRb8enSbfY... is still the plan that I'm following; looking to converge on that this quarter or Q1. Immediately, I'm actually planning on removing the Feature Policy control over document.cookie, document.domain, and document.write/writeln, until we have consensus on how those features should *actually* be disabled in practice. V1 of Feature Policy will likely only ship with support for enabling features which are currently disabled cross-origin, or are easily disabled (through permissions). This CL should actually just be about parsing the policy objects themselves, and querying them from ConditionalFeatures.cpp.
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
On 2016/10/24 14:23:12, iclelland wrote: > On 2016/10/24 08:51:19, haraken wrote: > > Would you help me understand your plan about the relationship between > > OriginTrial and FeaturePolicy in terms of V8 bindings? > > The long term plan is to unify them -- they are both going to be conditional > features; not included in the template, but added as soon as we determine that > they are required on an interface. > > In my mind, they are essentially the same thing, except that one has always been > part of the platform, and now needs to be selectively removed, while the other > is for new features which are *not* present on most pages. In both cases, > though, they will be added only as necessary. > > https://docs.google.com/document/d/1PHkCEyfUKwhoJEiRwcio9JWU5xQ1OuvIRb8enSbfY... > is still the plan that I'm following; looking to converge on that this quarter > or Q1. > > Immediately, I'm actually planning on removing the Feature Policy control over > document.cookie, document.domain, and document.write/writeln, until we have > consensus on how those features should *actually* be disabled in practice. V1 of > Feature Policy will likely only ship with support for enabling features which > are currently disabled cross-origin, or are easily disabled (through > permissions). This CL should actually just be about parsing the policy objects > themselves, and querying them from ConditionalFeatures.cpp. Thanks, the plan sounds good to me. I just want to confirm but do you want to associate FeaturePolicy with Frame instead of ExecutionContext? I'm curious because 1) workers don't have frames and 2) SecurityContext/SecurityOrigin is associated with ExecutionContext.
On 2016/10/24 18:30:50, haraken wrote: > On 2016/10/24 14:23:12, iclelland wrote: > > On 2016/10/24 08:51:19, haraken wrote: > > > Would you help me understand your plan about the relationship between > > > OriginTrial and FeaturePolicy in terms of V8 bindings? > > > > The long term plan is to unify them -- they are both going to be conditional > > features; not included in the template, but added as soon as we determine that > > they are required on an interface. > > > > In my mind, they are essentially the same thing, except that one has always > been > > part of the platform, and now needs to be selectively removed, while the other > > is for new features which are *not* present on most pages. In both cases, > > though, they will be added only as necessary. > > > > > https://docs.google.com/document/d/1PHkCEyfUKwhoJEiRwcio9JWU5xQ1OuvIRb8enSbfY... > > is still the plan that I'm following; looking to converge on that this quarter > > or Q1. > > > > Immediately, I'm actually planning on removing the Feature Policy control over > > document.cookie, document.domain, and document.write/writeln, until we have > > consensus on how those features should *actually* be disabled in practice. V1 > of > > Feature Policy will likely only ship with support for enabling features which > > are currently disabled cross-origin, or are easily disabled (through > > permissions). This CL should actually just be about parsing the policy objects > > themselves, and querying them from ConditionalFeatures.cpp. > > Thanks, the plan sounds good to me. > > I just want to confirm but do you want to associate FeaturePolicy with Frame > instead of ExecutionContext? I'm curious because 1) workers don't have frames > and 2) SecurityContext/SecurityOrigin is associated with ExecutionContext. Yes, the plan is to associate them with frames. The idea behind FeaturePolicy is to restrict the set of available features in a frame, and in all of the frames which it includes -- not every frame has an ExecutionContext (OOPIF, for instance), but the policy still needs to be defined so that if another frame is included which *does* have an ExecutionContext, then it is inherited properly. There are no (current) plans to implement FeaturePolicy for workers, but that is something to consider. (I expect that dedicated workers should be simple enough to support, but I don't know how shared workers would be guaranteed to inherit their policy correctly, especially when they could be instantiated by scripts running at multiple points in the frame tree) For #2, the security context I'm using -- from Frame.getSecurityContext() -- can either be a Document, (which is an ExecutionContext) or a RemoteSecurityContext (which is not).
On 2016/10/24 18:47:29, iclelland wrote: > On 2016/10/24 18:30:50, haraken wrote: > > On 2016/10/24 14:23:12, iclelland wrote: > > > On 2016/10/24 08:51:19, haraken wrote: > > > > Would you help me understand your plan about the relationship between > > > > OriginTrial and FeaturePolicy in terms of V8 bindings? > > > > > > The long term plan is to unify them -- they are both going to be conditional > > > features; not included in the template, but added as soon as we determine > that > > > they are required on an interface. > > > > > > In my mind, they are essentially the same thing, except that one has always > > been > > > part of the platform, and now needs to be selectively removed, while the > other > > > is for new features which are *not* present on most pages. In both cases, > > > though, they will be added only as necessary. > > > > > > > > > https://docs.google.com/document/d/1PHkCEyfUKwhoJEiRwcio9JWU5xQ1OuvIRb8enSbfY... > > > is still the plan that I'm following; looking to converge on that this > quarter > > > or Q1. > > > > > > Immediately, I'm actually planning on removing the Feature Policy control > over > > > document.cookie, document.domain, and document.write/writeln, until we have > > > consensus on how those features should *actually* be disabled in practice. > V1 > > of > > > Feature Policy will likely only ship with support for enabling features > which > > > are currently disabled cross-origin, or are easily disabled (through > > > permissions). This CL should actually just be about parsing the policy > objects > > > themselves, and querying them from ConditionalFeatures.cpp. > > > > Thanks, the plan sounds good to me. > > > > I just want to confirm but do you want to associate FeaturePolicy with Frame > > instead of ExecutionContext? I'm curious because 1) workers don't have frames > > and 2) SecurityContext/SecurityOrigin is associated with ExecutionContext. > > Yes, the plan is to associate them with frames. The idea behind FeaturePolicy is > to restrict the set of available features in a frame, and in all of the frames > which it includes -- not every frame has an ExecutionContext (OOPIF, for > instance), but the policy still needs to be defined so that if another frame is > included which *does* have an ExecutionContext, then it is inherited properly. > > There are no (current) plans to implement FeaturePolicy for workers, but that is > something to consider. (I expect that dedicated workers should be simple enough > to support, but I don't know how shared workers would be guaranteed to inherit > their policy correctly, especially when they could be instantiated by scripts > running at multiple points in the frame tree) > > For #2, the security context I'm using -- from Frame.getSecurityContext() -- can > either be a Document, (which is an ExecutionContext) or a RemoteSecurityContext > (which is not). Thanks for the clarification, makes sense! bindings/ LGTM.
Also, do you have an idea of how this data will be serialized so we could send it between processes for OOPIF? https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:45: frame = toDocument(executionContext)->executingFrame(); This is probably a dumb question, but what is executingFrame()? Why is it important to use that instead of the document's frame? https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:36: parseFeaturePolicyString(std::unique_ptr<JSONArray> policyItems, parseFeaturePolicyFromJson? (Also, I wonder if we should be fuzzing this?) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:190: return createFromParentPolicy(parent, currentOrigin, getDefaultFeatureList()); Nit: std::move(currentOrigin) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:206: const FeaturePolicy::Feature* feature, Nit: I'd suggest passing this by ref, since it's not allowed to be null here. Similar for isFeatureEnabled(...) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:117: const char* featureName; Nit: const char* const is slightly preferred (all the features are defined as consts, but if for some reason it wasn't, then featureName would actually be mutable) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:889: return std::unique_ptr<JSONArray>(nullptr); // Header is not valid JSON Nit: return nullptr works.
> Also, do you have an idea of how this data will be serialized so we could send it between processes for OOPIF? I sent an email to discuss more now that we've got the algorithm straightened out :) On Tue, 25 Oct 2016 at 06:16 <dcheng@chromium.org> wrote: Also, do you have an idea of how this data will be serialized so we could send it between processes for OOPIF? https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:45: frame = toDocument(executionContext)->executingFrame(); This is probably a dumb question, but what is executingFrame()? Why is it important to use that instead of the document's frame? https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:36: parseFeaturePolicyString(std::unique_ptr<JSONArray> policyItems, parseFeaturePolicyFromJson? (Also, I wonder if we should be fuzzing this?) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:190: return createFromParentPolicy(parent, currentOrigin, getDefaultFeatureList()); Nit: std::move(currentOrigin) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:206: const FeaturePolicy::Feature* feature, Nit: I'd suggest passing this by ref, since it's not allowed to be null here. Similar for isFeatureEnabled(...) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:117: const char* featureName; Nit: const char* const is slightly preferred (all the features are defined as consts, but if for some reason it wasn't, then featureName would actually be mutable) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:889: return std::unique_ptr<JSONArray>(nullptr); // Header is not valid JSON Nit: return nullptr works. https://codereview.chromium.org/2254533002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Also, do you have an idea of how this data will be serialized so we could send it between processes for OOPIF? I sent an email to discuss more now that we've got the algorithm straightened out :) On Tue, 25 Oct 2016 at 06:16 <dcheng@chromium.org> wrote: Also, do you have an idea of how this data will be serialized so we could send it between processes for OOPIF? https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:45: frame = toDocument(executionContext)->executingFrame(); This is probably a dumb question, but what is executingFrame()? Why is it important to use that instead of the document's frame? https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:36: parseFeaturePolicyString(std::unique_ptr<JSONArray> policyItems, parseFeaturePolicyFromJson? (Also, I wonder if we should be fuzzing this?) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:190: return createFromParentPolicy(parent, currentOrigin, getDefaultFeatureList()); Nit: std::move(currentOrigin) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:206: const FeaturePolicy::Feature* feature, Nit: I'd suggest passing this by ref, since it's not allowed to be null here. Similar for isFeatureEnabled(...) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:117: const char* featureName; Nit: const char* const is slightly preferred (all the features are defined as consts, but if for some reason it wasn't, then featureName would actually be mutable) https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:889: return std::unique_ptr<JSONArray>(nullptr); // Header is not valid JSON Nit: return nullptr works. https://codereview.chromium.org/2254533002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:45: frame = toDocument(executionContext)->executingFrame(); On 2016/10/24 19:16:45, dcheng wrote: > This is probably a dumb question, but what is executingFrame()? Why is it > important to use that instead of the document's frame? executingFrame() can also return the frame of the main document if the executing document is an HTML import. We haven't actually specified what happens in that case, but it seemed like the right frame to use; thinking that imports should have the same policy as the document they are being imported into. https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:36: parseFeaturePolicyString(std::unique_ptr<JSONArray> policyItems, On 2016/10/24 19:16:45, dcheng wrote: > parseFeaturePolicyFromJson? Changed. > (Also, I wonder if we should be fuzzing this?) Yes :) It needs to be updated now, but https://codereview.chromium.org/2420013004/ https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:190: return createFromParentPolicy(parent, currentOrigin, getDefaultFeatureList()); On 2016/10/24 19:16:45, dcheng wrote: > Nit: std::move(currentOrigin) Done. https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:206: const FeaturePolicy::Feature* feature, On 2016/10/24 19:16:45, dcheng wrote: > Nit: I'd suggest passing this by ref, since it's not allowed to be null here. > Similar for isFeatureEnabled(...) Done. https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:117: const char* featureName; On 2016/10/24 19:16:45, dcheng wrote: > Nit: const char* const is slightly preferred (all the features are defined as > consts, but if for some reason it wasn't, then featureName would actually be > mutable) Makes sense, thanks - done. https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2254533002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:889: return std::unique_ptr<JSONArray>(nullptr); // Header is not valid JSON On 2016/10/24 19:16:45, dcheng wrote: > Nit: return nullptr works. Nice, thanks. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/24 19:16:45, dcheng wrote: > Also, do you have an idea of how this data will be serialized so we could send > it between processes for OOPIF? > Most of the state can be serialized as the header data itself. (It's also JSON, so we could read all of the headers, if there are multiple, combine them, and then stringify *that* array if necessary.) Once we add support for iframe attributes (enable="feature" and disable="feature",) then I think we will need to serialize those as well, so that the frame knows how it was embedded if its parent is OOP. dcheng, is this CL blocked on support for OOPIF, or is it okay to land the functionality that we I have implemented so far and iterate?
> dcheng, is this CL blocked on support for OOPIF, or is it okay to land the > functionality that we I have implemented so far and iterate? Assuming things work out as we discussed for OOPIF and we don't have to change FeaturePolicy.h/cpp too much I think we should land this and iterate. But it may be worth testing that assumption a bit by delving into the code :)
LGTM, I think it's fine to land it and implement OOPIF in followups. https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.cpp:330: m_featurePolicy(nullptr) { Nit: this can just be default initialized. https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:888: if (!headerValue) Nit: this early return isn't necessary (JSONArray::cast handles this case as well) https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.h (right): https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.h:167: const String& header); Nit: newline after this
https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.cpp:330: m_featurePolicy(nullptr) { On 2016/11/03 05:54:43, dcheng wrote: > Nit: this can just be default initialized. Done. https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:888: if (!headerValue) On 2016/11/03 05:54:43, dcheng wrote: > Nit: this early return isn't necessary (JSONArray::cast handles this case as > well) Done, thanks. https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.h (right): https://codereview.chromium.org/2254533002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.h:167: const String& header); On 2016/11/03 05:54:43, dcheng wrote: > Nit: newline after this Thanks, done. (Really wish git cl format would handle the vertical whitespace as well)
Description was changed from ========== [WIP][FeaturePolicy] Initial implementation of Feature Policy BUG=623682 ========== to ========== [FeaturePolicy] Initial implementation of Feature Policy This CL adds to each frame (local or remote) a FeaturePolicy object, which can be used to control the exposure and behaviour of web platform features. A frame's policy is computed from the policy declared in the "Feature-Policy" HTTP header, it's parent frame's policy, and the default policy for each feature. See https://wicg.github.io/feature-policy/ for the specification in development. Three features from the specification are implemented with this CL: cookie, domain, and docwrite. (This behaviour is all conditional on the runtime flag for Feature Policy) Future CLs are going to implement: - <iframe> attribute support - Layout tests - Parser fuzzing - Control over additional features BUG=623682 ==========
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, haraken@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2254533002/#ps400001 (title: "Addressing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [FeaturePolicy] Initial implementation of Feature Policy This CL adds to each frame (local or remote) a FeaturePolicy object, which can be used to control the exposure and behaviour of web platform features. A frame's policy is computed from the policy declared in the "Feature-Policy" HTTP header, it's parent frame's policy, and the default policy for each feature. See https://wicg.github.io/feature-policy/ for the specification in development. Three features from the specification are implemented with this CL: cookie, domain, and docwrite. (This behaviour is all conditional on the runtime flag for Feature Policy) Future CLs are going to implement: - <iframe> attribute support - Layout tests - Parser fuzzing - Control over additional features BUG=623682 ========== to ========== [FeaturePolicy] Initial implementation of Feature Policy This CL adds to each frame (local or remote) a FeaturePolicy object, which can be used to control the exposure and behaviour of web platform features. A frame's policy is computed from the policy declared in the "Feature-Policy" HTTP header, it's parent frame's policy, and the default policy for each feature. See https://wicg.github.io/feature-policy/ for the specification in development. Three features from the specification are implemented with this CL: cookie, domain, and docwrite. (This behaviour is all conditional on the runtime flag for Feature Policy) Future CLs are going to implement: - <iframe> attribute support - Layout tests - Parser fuzzing - Control over additional features BUG=623682 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== [FeaturePolicy] Initial implementation of Feature Policy This CL adds to each frame (local or remote) a FeaturePolicy object, which can be used to control the exposure and behaviour of web platform features. A frame's policy is computed from the policy declared in the "Feature-Policy" HTTP header, it's parent frame's policy, and the default policy for each feature. See https://wicg.github.io/feature-policy/ for the specification in development. Three features from the specification are implemented with this CL: cookie, domain, and docwrite. (This behaviour is all conditional on the runtime flag for Feature Policy) Future CLs are going to implement: - <iframe> attribute support - Layout tests - Parser fuzzing - Control over additional features BUG=623682 ========== to ========== [FeaturePolicy] Initial implementation of Feature Policy This CL adds to each frame (local or remote) a FeaturePolicy object, which can be used to control the exposure and behaviour of web platform features. A frame's policy is computed from the policy declared in the "Feature-Policy" HTTP header, it's parent frame's policy, and the default policy for each feature. See https://wicg.github.io/feature-policy/ for the specification in development. Three features from the specification are implemented with this CL: cookie, domain, and docwrite. (This behaviour is all conditional on the runtime flag for Feature Policy) Future CLs are going to implement: - <iframe> attribute support - Layout tests - Parser fuzzing - Control over additional features BUG=623682 Committed: https://crrev.com/7d15b7fc471b33e2d52a45876cb8323a4fb0e780 Cr-Commit-Position: refs/heads/master@{#429623} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/7d15b7fc471b33e2d52a45876cb8323a4fb0e780 Cr-Commit-Position: refs/heads/master@{#429623} |
