Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655023004:
Feature policy: Add basic algorithm for supporting frame policies. (Closed)
DescriptionFeature policy: Add basic algorithm for supporting frame policies.
This allows a parsed policy from an iframe element to be used to influence
the policy inherited by the framed document.
BUG=682258
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2655023004
Cr-Commit-Position: refs/heads/master@{#451887}
Committed: https://chromium.googlesource.com/chromium/src/+/de94fd8361c6d3c0af5e4cdf757594f80340f789
Patch Set 1 #Patch Set 2 : Fix logic, add tests #
Total comments: 15
Patch Set 3 : Rebase; class names updated #Patch Set 4 : Require default-off features to be enabled in parent for frame policy to have effect #Patch Set 5 : Add frame policy to content-side code as well #
Total comments: 10
Patch Set 6 : Addressing review comments #Messages
Total messages: 37 (21 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...
Description was changed from ========== [WIP] Feature policy: Add basic algorithm for supporting frame policies. This allows a parsed policy from an iframe element to be used to influence the policy inherited by the framed document. BUG=682258 ========== to ========== Feature policy: Add basic algorithm for supporting frame policies. This allows a parsed policy from an iframe element to be used to influence the policy inherited by the framed document. BUG=682258 ==========
iclelland@chromium.org changed reviewers: + raymes@chromium.org
+r raymes, PTAL; this is the algorithm for having the iframe attributes (of any sort) influence the effective policy in the child frame. Parallel to this, lunalu@ is working on the code that transforms the various iframe attributes into the WebParsedFeaturePolicy structure that this code expects.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks Ian! I guess this will be moved into the new content/ version of the implementation? https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:166: parent->m_inheritedFeatures.get(feature)) { Hmm does this match what we had in https://docs.google.com/document/d/1saEBLE6d-qmMxao8Aw6A06oSP3yKdUG7lqMWBYlzE... I guess the difference would be that right now this will be true, even if the parent disabled the feature in itself by header policy, or if the feature is disabled by default. In both those cases, the parent page would not itself have access, so I think our reasoning was that it shouldn't be allowed to delegate through the iframe attribute either. I think here we want to return true if the parent frames policy allows the parent frames origin. Does this make sense? Could we add an extra 2 cases to the loop in createFromParentPolicy to handle this (and more closely match what we have in the doc)? Would that work? https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:825: "{\"default-self\": [\"self\"]}", m_originB.get(), &messages); Hmm - does "self" in the attribute refer to the embedder or the embedee? I feel like it should refer to the embedder since it is written in the context of the embedder. Specifically, I feel like m_originB here should be m_originA. What do you think? https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:1008: createFromParentPolicy(nullptr, m_originA); We might want to enable the feature in (1) by header policy, otherwise the feature shouldn't get delegated anyway. Is that right? I think this is the result of my comment in the previous file. https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:1067: policy2->setHeaderPolicy(FeaturePolicy::parseFeaturePolicy( Shouldn't frame (1) have to do this in order to delegate the feature through the iframe attribute?
> I guess this will be moved into the new content/ version of > the implementation? Yes, I'll be keeping it up to date as things move around -- my initial version of this was in https://codereview.chromium.org/2653623004/, but I backported it to trunk for review (and so Luna could start working off of it) https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:166: parent->m_inheritedFeatures.get(feature)) { On 2017/02/03 00:28:51, raymes wrote: > Hmm does this match what we had in > https://docs.google.com/document/d/1saEBLE6d-qmMxao8Aw6A06oSP3yKdUG7lqMWBYlzE... > > > I guess the difference would be that right now this will be true, even if the > parent disabled the feature in itself by header policy, or if the feature is > disabled by default. In both those cases, the parent page would not itself have > access, so I think our reasoning was that it shouldn't be allowed to delegate > through the iframe attribute either. > > I think here we want to return true if the parent frames policy allows the > parent frames origin. Does this make sense? > > Could we add an extra 2 cases to the loop in createFromParentPolicy to handle > this (and more closely match what we have in the doc)? Would that work? > Without this (and we can certainly debate in the doc or elsewhere whether it's correct,) you can never use an iframe attribute to enable a default-off feature for a child frame, even if you potentially could have turned it on for yourself. As soon as you introduce default-off features, developers *have* to use headers. It also means that there is no way to specify a general policy in an HTTP header like {"feature": []} turn off a feature, and then selectively turn it back on for individual frames. https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:825: "{\"default-self\": [\"self\"]}", m_originB.get(), &messages); On 2017/02/03 00:28:51, raymes wrote: > Hmm - does "self" in the attribute refer to the embedder or the embedee? I feel > like it should refer to the embedder since it is written in the context of the > embedder. Specifically, I feel like m_originB here should be m_originA. What do > you think? I recall that the way we had originally gone with this was to say that attributes like "allowfullscreen" in an iframe element would translate to {"fullscreen": [*]} which would match every origin, even if the frame contents were navigated, and 'allow="fullscreen"' would translate to {"fullscreen": ["self"]} which would be snapshotted to the origin of the src attribute. Since we do snapshot the origin at parsing time, though, we could easily switch that. I'll try it and see if it makes the spec simpler. https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:1067: policy2->setHeaderPolicy(FeaturePolicy::parseFeaturePolicy( On 2017/02/03 00:28:51, raymes wrote: > Shouldn't frame (1) have to do this in order to delegate the feature through the > iframe attribute? See the comment in the previous file -- Maybe we do want frame 1 to have to do that, but it does mean that default-off features are never going to be usable at all without being able to set HTTP headers.
Thanks! https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:166: parent->m_inheritedFeatures.get(feature)) { On 2017/02/03 16:59:54, iclelland wrote: > On 2017/02/03 00:28:51, raymes wrote: > > Hmm does this match what we had in > > > https://docs.google.com/document/d/1saEBLE6d-qmMxao8Aw6A06oSP3yKdUG7lqMWBYlzE... > > > > > > I guess the difference would be that right now this will be true, even if the > > parent disabled the feature in itself by header policy, or if the feature is > > disabled by default. In both those cases, the parent page would not itself > have > > access, so I think our reasoning was that it shouldn't be allowed to delegate > > through the iframe attribute either. > > > > I think here we want to return true if the parent frames policy allows the > > parent frames origin. Does this make sense? > > > > Could we add an extra 2 cases to the loop in createFromParentPolicy to handle > > this (and more closely match what we have in the doc)? Would that work? > > > > Without this (and we can certainly debate in the doc or elsewhere whether it's > correct,) you can never use an iframe attribute to enable a default-off feature > for a child frame, even if you potentially could have turned it on for yourself. > As soon as you introduce default-off features, developers *have* to use headers. > Hmm, that's a good point. I guess the main problem I see with it is that a site could try to lock itself down as an XSS mitigation, by saying "disable geolocation in myself". But then if it was compromised, it could simply embed a same-origin frame and then re-enable geolocation for that frame and get access. Does that make sense? (I might be missing something). > It also means that there is no way to specify a general policy in an HTTP header > like {"feature": []} turn off a feature, and then selectively turn it back on > for individual frames. That's true, but I guess they could do feature: ["self"] and then selectively re-enable. I think that fits with the security model better in the sense that the frame itself needs runtime access in order to delegate access at runtime. What do you think? https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:825: "{\"default-self\": [\"self\"]}", m_originB.get(), &messages); On 2017/02/03 16:59:54, iclelland wrote: > On 2017/02/03 00:28:51, raymes wrote: > > Hmm - does "self" in the attribute refer to the embedder or the embedee? I > feel > > like it should refer to the embedder since it is written in the context of the > > embedder. Specifically, I feel like m_originB here should be m_originA. What > do > > you think? > > I recall that the way we had originally gone with this was to say that > attributes like "allowfullscreen" in an iframe element would translate to > {"fullscreen": [*]} > which would match every origin, even if the frame contents were navigated, and > 'allow="fullscreen"' would translate to > {"fullscreen": ["self"]} > which would be snapshotted to the origin of the src attribute. > I guess I had been thinking allow="fullscreen" would translate to {"fullscreen": ["example.com"]} (where example.com was the src attribute). I think in this case it's really just a question of whichever syntax is the most clear. My personal opinion would be that "self" feels more like it's talking about the embedder in that context, but I'm open to other opinions too :) > Since we do snapshot the origin at parsing time, though, we could easily switch > that. I'll try it and see if it makes the spec simpler.
https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:166: parent->m_inheritedFeatures.get(feature)) { On 2017/02/03 18:11:28, raymes wrote: > On 2017/02/03 16:59:54, iclelland wrote: > > On 2017/02/03 00:28:51, raymes wrote: > > > Hmm does this match what we had in > > > > > > https://docs.google.com/document/d/1saEBLE6d-qmMxao8Aw6A06oSP3yKdUG7lqMWBYlzE... > > > > > > > > > I guess the difference would be that right now this will be true, even if > the > > > parent disabled the feature in itself by header policy, or if the feature is > > > disabled by default. In both those cases, the parent page would not itself > > have > > > access, so I think our reasoning was that it shouldn't be allowed to > delegate > > > through the iframe attribute either. > > > > > > I think here we want to return true if the parent frames policy allows the > > > parent frames origin. Does this make sense? > > > > > > Could we add an extra 2 cases to the loop in createFromParentPolicy to > handle > > > this (and more closely match what we have in the doc)? Would that work? > > > > > > > Without this (and we can certainly debate in the doc or elsewhere whether it's > > correct,) you can never use an iframe attribute to enable a default-off > feature > > for a child frame, even if you potentially could have turned it on for > yourself. > > As soon as you introduce default-off features, developers *have* to use > headers. > > > > Hmm, that's a good point. I guess the main problem I see with it is that a site > could try to lock itself down as an XSS mitigation, by saying "disable > geolocation in myself". But then if it was compromised, it could simply embed a > same-origin frame and then re-enable geolocation for that frame and get access. > Does that make sense? (I might be missing something). No that's a really good point. I'll update the spec, logic and tests. Maybe we should revisit the idea of using <meta> tags, so that users with HTML-only capabilities can still turn these features on. > > It also means that there is no way to specify a general policy in an HTTP > header > > like {"feature": []} turn off a feature, and then selectively turn it back on > > for individual frames. > > That's true, but I guess they could do feature: ["self"] and then selectively > re-enable. I think that fits with the security model better in the sense that > the frame itself needs runtime access in order to delegate access at runtime. > What do you think? I liked the idea that a site could say "I don't ever want to use this feature, but turn it on in this child frame" -- similarly to having a header like Feature-Policy: {"feature": ["https://some-other-site/"]} (not mentioning own origin) But without using headers. Maybe that doesn't fit the security model well, though. https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:825: "{\"default-self\": [\"self\"]}", m_originB.get(), &messages); On 2017/02/03 18:11:28, raymes wrote: > On 2017/02/03 16:59:54, iclelland wrote: > > On 2017/02/03 00:28:51, raymes wrote: > > > Hmm - does "self" in the attribute refer to the embedder or the embedee? I > > feel > > > like it should refer to the embedder since it is written in the context of > the > > > embedder. Specifically, I feel like m_originB here should be m_originA. What > > do > > > you think? > > > > I recall that the way we had originally gone with this was to say that > > attributes like "allowfullscreen" in an iframe element would translate to > > {"fullscreen": [*]} > > which would match every origin, even if the frame contents were navigated, and > > 'allow="fullscreen"' would translate to > > {"fullscreen": ["self"]} > > which would be snapshotted to the origin of the src attribute. > > > > I guess I had been thinking allow="fullscreen" would translate to {"fullscreen": > ["example.com"]} (where http://example.com was the src attribute). I think in this case > it's really just a question of whichever syntax is the most clear. My personal > opinion would be that "self" feels more like it's talking about the embedder in > that context, but I'm open to other opinions too :) We're going to translate the allow and allow* attributes directly into declarations, and the origin needs to be explicit there (there's no concept of "self" after you get out of strings). For those cases, it doesn't matter. We're not shipping the attribute that lets you specify the full policy right now, and that's the only case where a developer could actually specify "self". I'll update the tests to use explicit origins, and we can move the discussion of how we interpret the JSON text to somewhere more visible.
Description was changed from ========== Feature policy: Add basic algorithm for supporting frame policies. This allows a parsed policy from an iframe element to be used to influence the policy inherited by the framed document. BUG=682258 ========== to ========== Feature policy: Add basic algorithm for supporting frame policies. This allows a parsed policy from an iframe element to be used to influence the policy inherited by the framed document. BUG=682258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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...
Raymes, can you take a look? I've added the corresponding code for the content side; once we switch blink to using that code, I'll delete the WebKit/platform/feature-policy code from this CL, but for the moment, both implementations are being maintained in parallel. https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:166: parent->m_inheritedFeatures.get(feature)) { On 2017/02/03 19:50:04, iclelland wrote: > On 2017/02/03 18:11:28, raymes wrote: > > On 2017/02/03 16:59:54, iclelland wrote: > > > On 2017/02/03 00:28:51, raymes wrote: > > > > Hmm does this match what we had in > > > > > > > > > > https://docs.google.com/document/d/1saEBLE6d-qmMxao8Aw6A06oSP3yKdUG7lqMWBYlzE... > > > > > > > > > > > > I guess the difference would be that right now this will be true, even if > > the > > > > parent disabled the feature in itself by header policy, or if the feature > is > > > > disabled by default. In both those cases, the parent page would not itself > > > have > > > > access, so I think our reasoning was that it shouldn't be allowed to > > delegate > > > > through the iframe attribute either. > > > > > > > > I think here we want to return true if the parent frames policy allows the > > > > parent frames origin. Does this make sense? > > > > > > > > Could we add an extra 2 cases to the loop in createFromParentPolicy to > > handle > > > > this (and more closely match what we have in the doc)? Would that work? > > > > > > > > > > Without this (and we can certainly debate in the doc or elsewhere whether > it's > > > correct,) you can never use an iframe attribute to enable a default-off > > feature > > > for a child frame, even if you potentially could have turned it on for > > yourself. > > > As soon as you introduce default-off features, developers *have* to use > > headers. > > > > > > > Hmm, that's a good point. I guess the main problem I see with it is that a > site > > could try to lock itself down as an XSS mitigation, by saying "disable > > geolocation in myself". But then if it was compromised, it could simply embed > a > > same-origin frame and then re-enable geolocation for that frame and get > access. > > Does that make sense? (I might be missing something). > > No that's a really good point. I'll update the spec, logic and tests. > Maybe we should revisit the idea of using <meta> tags, so that users with > HTML-only capabilities can still turn these features on. > > > > It also means that there is no way to specify a general policy in an HTTP > > header > > > like {"feature": []} turn off a feature, and then selectively turn it back > on > > > for individual frames. > > > > That's true, but I guess they could do feature: ["self"] and then selectively > > re-enable. I think that fits with the security model better in the sense that > > the frame itself needs runtime access in order to delegate access at runtime. > > What do you think? > > I liked the idea that a site could say "I don't ever want to use this feature, > but turn it on in this child frame" -- similarly to having a header like > > Feature-Policy: {"feature": ["https://some-other-site/"]} (not mentioning own > origin) > > But without using headers. Maybe that doesn't fit the security model well, > though. Changed this to require that the feature be enabled in the embedding frame, and updated tests to validate this. https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:825: "{\"default-self\": [\"self\"]}", m_originB.get(), &messages); On 2017/02/03 19:50:04, iclelland wrote: > On 2017/02/03 18:11:28, raymes wrote: > > On 2017/02/03 16:59:54, iclelland wrote: > > > On 2017/02/03 00:28:51, raymes wrote: > > > > Hmm - does "self" in the attribute refer to the embedder or the embedee? I > > > feel > > > > like it should refer to the embedder since it is written in the context of > > the > > > > embedder. Specifically, I feel like m_originB here should be m_originA. > What > > > do > > > > you think? > > > > > > I recall that the way we had originally gone with this was to say that > > > attributes like "allowfullscreen" in an iframe element would translate to > > > {"fullscreen": [*]} > > > which would match every origin, even if the frame contents were navigated, > and > > > 'allow="fullscreen"' would translate to > > > {"fullscreen": ["self"]} > > > which would be snapshotted to the origin of the src attribute. > > > > > > > I guess I had been thinking allow="fullscreen" would translate to > {"fullscreen": > > ["example.com"]} (where http://example.com was the src attribute). I think in > this case > > it's really just a question of whichever syntax is the most clear. My personal > > opinion would be that "self" feels more like it's talking about the embedder > in > > that context, but I'm open to other opinions too :) > > We're going to translate the allow and allow* attributes directly into > declarations, and the origin needs to be explicit there (there's no concept of > "self" after you get out of strings). For those cases, it doesn't matter. > We're not shipping the attribute that lets you specify the full policy right > now, and that's the only case where a developer could actually specify "self". > I'll update the tests to use explicit origins, and we can move the discussion of > how we interpret the JSON text to somewhere more visible. Tests updated to use explicit origins for frame policy declarations. https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:1008: createFromParentPolicy(nullptr, m_originA); On 2017/02/03 00:28:51, raymes wrote: > We might want to enable the feature in (1) by header policy, otherwise the > feature shouldn't get delegated anyway. Is that right? I think this is the > result of my comment in the previous file. Done. Required by algorithm now. https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:1067: policy2->setHeaderPolicy(FeaturePolicy::parseFeaturePolicy( On 2017/02/03 16:59:54, iclelland wrote: > On 2017/02/03 00:28:51, raymes wrote: > > Shouldn't frame (1) have to do this in order to delegate the feature through > the > > iframe attribute? > > See the comment in the previous file -- Maybe we do want frame 1 to have to do > that, but it does mean that default-off features are never going to be usable at > all without being able to set HTTP headers. Changed now; header (or eventually meta) must be applied in all frames. Test updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/14 21:25:03, iclelland wrote: > Raymes, can you take a look? > > I've added the corresponding code for the content side; once we switch blink to > using that code, I'll delete the WebKit/platform/feature-policy code from this > CL, but for the moment, both implementations are being maintained in parallel. > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp > (right): > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:166: > parent->m_inheritedFeatures.get(feature)) { > On 2017/02/03 19:50:04, iclelland wrote: > > On 2017/02/03 18:11:28, raymes wrote: > > > On 2017/02/03 16:59:54, iclelland wrote: > > > > On 2017/02/03 00:28:51, raymes wrote: > > > > > Hmm does this match what we had in > > > > > > > > > > > > > > > https://docs.google.com/document/d/1saEBLE6d-qmMxao8Aw6A06oSP3yKdUG7lqMWBYlzE... > > > > > > > > > > > > > > > I guess the difference would be that right now this will be true, even > if > > > the > > > > > parent disabled the feature in itself by header policy, or if the > feature > > is > > > > > disabled by default. In both those cases, the parent page would not > itself > > > > have > > > > > access, so I think our reasoning was that it shouldn't be allowed to > > > delegate > > > > > through the iframe attribute either. > > > > > > > > > > I think here we want to return true if the parent frames policy allows > the > > > > > parent frames origin. Does this make sense? > > > > > > > > > > Could we add an extra 2 cases to the loop in createFromParentPolicy to > > > handle > > > > > this (and more closely match what we have in the doc)? Would that work? > > > > > > > > > > > > > Without this (and we can certainly debate in the doc or elsewhere whether > > it's > > > > correct,) you can never use an iframe attribute to enable a default-off > > > feature > > > > for a child frame, even if you potentially could have turned it on for > > > yourself. > > > > As soon as you introduce default-off features, developers *have* to use > > > headers. > > > > > > > > > > Hmm, that's a good point. I guess the main problem I see with it is that a > > site > > > could try to lock itself down as an XSS mitigation, by saying "disable > > > geolocation in myself". But then if it was compromised, it could simply > embed > > a > > > same-origin frame and then re-enable geolocation for that frame and get > > access. > > > Does that make sense? (I might be missing something). > > > > No that's a really good point. I'll update the spec, logic and tests. > > Maybe we should revisit the idea of using <meta> tags, so that users with > > HTML-only capabilities can still turn these features on. > > > > > > It also means that there is no way to specify a general policy in an HTTP > > > header > > > > like {"feature": []} turn off a feature, and then selectively turn it back > > on > > > > for individual frames. > > > > > > That's true, but I guess they could do feature: ["self"] and then > selectively > > > re-enable. I think that fits with the security model better in the sense > that > > > the frame itself needs runtime access in order to delegate access at > runtime. > > > What do you think? > > > > I liked the idea that a site could say "I don't ever want to use this feature, > > but turn it on in this child frame" -- similarly to having a header like > > > > Feature-Policy: {"feature": ["https://some-other-site/"]} (not mentioning own > > origin) > > > > But without using headers. Maybe that doesn't fit the security model well, > > though. > > Changed this to require that the feature be enabled in the embedding frame, and > updated tests to validate this. > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp > (right): > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:825: > "{\"default-self\": [\"self\"]}", m_originB.get(), &messages); > On 2017/02/03 19:50:04, iclelland wrote: > > On 2017/02/03 18:11:28, raymes wrote: > > > On 2017/02/03 16:59:54, iclelland wrote: > > > > On 2017/02/03 00:28:51, raymes wrote: > > > > > Hmm - does "self" in the attribute refer to the embedder or the embedee? > I > > > > feel > > > > > like it should refer to the embedder since it is written in the context > of > > > the > > > > > embedder. Specifically, I feel like m_originB here should be m_originA. > > What > > > > do > > > > > you think? > > > > > > > > I recall that the way we had originally gone with this was to say that > > > > attributes like "allowfullscreen" in an iframe element would translate to > > > > {"fullscreen": [*]} > > > > which would match every origin, even if the frame contents were navigated, > > and > > > > 'allow="fullscreen"' would translate to > > > > {"fullscreen": ["self"]} > > > > which would be snapshotted to the origin of the src attribute. > > > > > > > > > > I guess I had been thinking allow="fullscreen" would translate to > > {"fullscreen": > > > ["example.com"]} (where http://example.com was the src attribute). I think > in > > this case > > > it's really just a question of whichever syntax is the most clear. My > personal > > > opinion would be that "self" feels more like it's talking about the embedder > > in > > > that context, but I'm open to other opinions too :) > > > > We're going to translate the allow and allow* attributes directly into > > declarations, and the origin needs to be explicit there (there's no concept of > > "self" after you get out of strings). For those cases, it doesn't matter. > > We're not shipping the attribute that lets you specify the full policy right > > now, and that's the only case where a developer could actually specify "self". > > I'll update the tests to use explicit origins, and we can move the discussion > of > > how we interpret the JSON text to somewhere more visible. > > Tests updated to use explicit origins for frame policy declarations. > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:1008: > createFromParentPolicy(nullptr, m_originA); > On 2017/02/03 00:28:51, raymes wrote: > > We might want to enable the feature in (1) by header policy, otherwise the > > feature shouldn't get delegated anyway. Is that right? I think this is the > > result of my comment in the previous file. > > Done. Required by algorithm now. > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:1067: > policy2->setHeaderPolicy(FeaturePolicy::parseFeaturePolicy( > On 2017/02/03 16:59:54, iclelland wrote: > > On 2017/02/03 00:28:51, raymes wrote: > > > Shouldn't frame (1) have to do this in order to delegate the feature through > > the > > > iframe attribute? > > > > See the comment in the previous file -- Maybe we do want frame 1 to have to do > > that, but it does mean that default-off features are never going to be usable > at > > all without being able to set HTTP headers. > > Changed now; header (or eventually meta) must be applied in all frames. Test > updated. Hey Ian - sorry for the delay on this one. I'll try to get to it first thing next week. If there is time pressure, I can try to get to it sooner. Please let me know :)
On 2017/02/16 10:38:47, raymes wrote: > On 2017/02/14 21:25:03, iclelland wrote: > > Raymes, can you take a look? > > > > I've added the corresponding code for the content side; once we switch blink > to > > using that code, I'll delete the WebKit/platform/feature-policy code from this > > CL, but for the moment, both implementations are being maintained in parallel. > > > > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp > > (right): > > > > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:166: > > parent->m_inheritedFeatures.get(feature)) { > > On 2017/02/03 19:50:04, iclelland wrote: > > > On 2017/02/03 18:11:28, raymes wrote: > > > > On 2017/02/03 16:59:54, iclelland wrote: > > > > > On 2017/02/03 00:28:51, raymes wrote: > > > > > > Hmm does this match what we had in > > > > > > > > > > > > > > > > > > > > > https://docs.google.com/document/d/1saEBLE6d-qmMxao8Aw6A06oSP3yKdUG7lqMWBYlzE... > > > > > > > > > > > > > > > > > > I guess the difference would be that right now this will be true, even > > if > > > > the > > > > > > parent disabled the feature in itself by header policy, or if the > > feature > > > is > > > > > > disabled by default. In both those cases, the parent page would not > > itself > > > > > have > > > > > > access, so I think our reasoning was that it shouldn't be allowed to > > > > delegate > > > > > > through the iframe attribute either. > > > > > > > > > > > > I think here we want to return true if the parent frames policy allows > > the > > > > > > parent frames origin. Does this make sense? > > > > > > > > > > > > Could we add an extra 2 cases to the loop in createFromParentPolicy to > > > > handle > > > > > > this (and more closely match what we have in the doc)? Would that > work? > > > > > > > > > > > > > > > > Without this (and we can certainly debate in the doc or elsewhere > whether > > > it's > > > > > correct,) you can never use an iframe attribute to enable a default-off > > > > feature > > > > > for a child frame, even if you potentially could have turned it on for > > > > yourself. > > > > > As soon as you introduce default-off features, developers *have* to use > > > > headers. > > > > > > > > > > > > > Hmm, that's a good point. I guess the main problem I see with it is that a > > > site > > > > could try to lock itself down as an XSS mitigation, by saying "disable > > > > geolocation in myself". But then if it was compromised, it could simply > > embed > > > a > > > > same-origin frame and then re-enable geolocation for that frame and get > > > access. > > > > Does that make sense? (I might be missing something). > > > > > > No that's a really good point. I'll update the spec, logic and tests. > > > Maybe we should revisit the idea of using <meta> tags, so that users with > > > HTML-only capabilities can still turn these features on. > > > > > > > > It also means that there is no way to specify a general policy in an > HTTP > > > > header > > > > > like {"feature": []} turn off a feature, and then selectively turn it > back > > > on > > > > > for individual frames. > > > > > > > > That's true, but I guess they could do feature: ["self"] and then > > selectively > > > > re-enable. I think that fits with the security model better in the sense > > that > > > > the frame itself needs runtime access in order to delegate access at > > runtime. > > > > What do you think? > > > > > > I liked the idea that a site could say "I don't ever want to use this > feature, > > > but turn it on in this child frame" -- similarly to having a header like > > > > > > Feature-Policy: {"feature": ["https://some-other-site/"]} (not mentioning > own > > > origin) > > > > > > But without using headers. Maybe that doesn't fit the security model well, > > > though. > > > > Changed this to require that the feature be enabled in the embedding frame, > and > > updated tests to validate this. > > > > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp > > (right): > > > > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:825: > > "{\"default-self\": [\"self\"]}", m_originB.get(), &messages); > > On 2017/02/03 19:50:04, iclelland wrote: > > > On 2017/02/03 18:11:28, raymes wrote: > > > > On 2017/02/03 16:59:54, iclelland wrote: > > > > > On 2017/02/03 00:28:51, raymes wrote: > > > > > > Hmm - does "self" in the attribute refer to the embedder or the > embedee? > > I > > > > > feel > > > > > > like it should refer to the embedder since it is written in the > context > > of > > > > the > > > > > > embedder. Specifically, I feel like m_originB here should be > m_originA. > > > What > > > > > do > > > > > > you think? > > > > > > > > > > I recall that the way we had originally gone with this was to say that > > > > > attributes like "allowfullscreen" in an iframe element would translate > to > > > > > {"fullscreen": [*]} > > > > > which would match every origin, even if the frame contents were > navigated, > > > and > > > > > 'allow="fullscreen"' would translate to > > > > > {"fullscreen": ["self"]} > > > > > which would be snapshotted to the origin of the src attribute. > > > > > > > > > > > > > I guess I had been thinking allow="fullscreen" would translate to > > > {"fullscreen": > > > > ["example.com"]} (where http://example.com was the src attribute). I think > > in > > > this case > > > > it's really just a question of whichever syntax is the most clear. My > > personal > > > > opinion would be that "self" feels more like it's talking about the > embedder > > > in > > > > that context, but I'm open to other opinions too :) > > > > > > We're going to translate the allow and allow* attributes directly into > > > declarations, and the origin needs to be explicit there (there's no concept > of > > > "self" after you get out of strings). For those cases, it doesn't matter. > > > We're not shipping the attribute that lets you specify the full policy right > > > now, and that's the only case where a developer could actually specify > "self". > > > I'll update the tests to use explicit origins, and we can move the > discussion > > of > > > how we interpret the JSON text to somewhere more visible. > > > > Tests updated to use explicit origins for frame policy declarations. > > > > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:1008: > > createFromParentPolicy(nullptr, m_originA); > > On 2017/02/03 00:28:51, raymes wrote: > > > We might want to enable the feature in (1) by header policy, otherwise the > > > feature shouldn't get delegated anyway. Is that right? I think this is the > > > result of my comment in the previous file. > > > > Done. Required by algorithm now. > > > > > https://codereview.chromium.org/2655023004/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:1067: > > policy2->setHeaderPolicy(FeaturePolicy::parseFeaturePolicy( > > On 2017/02/03 16:59:54, iclelland wrote: > > > On 2017/02/03 00:28:51, raymes wrote: > > > > Shouldn't frame (1) have to do this in order to delegate the feature > through > > > the > > > > iframe attribute? > > > > > > See the comment in the previous file -- Maybe we do want frame 1 to have to > do > > > that, but it does mean that default-off features are never going to be > usable > > at > > > all without being able to set HTTP headers. > > > > Changed now; header (or eventually meta) must be applied in all frames. Test > > updated. > > Hey Ian - sorry for the delay on this one. I'll try to get to it first thing > next week. If there is time pressure, I can try to get to it sooner. Please let > me know :) Next week should be okay, thanks! (If we get through https://codereview.chromium.org/2651883008/ first, then about half of this CL goes away, btw)
lg - just nits https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... File content/common/feature_policy/feature_policy.cc (right): https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... content/common/feature_policy/feature_policy.cc:182: if (frame_policy) { nit: we should be consistent with brackets around single-line if's. Unfortunately the style guide says nothing, but at least within files we should try to be consistent :) https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... content/common/feature_policy/feature_policy.cc:200: if (WhitelistFromDeclaration(parsed_declaration)->Contains(origin_) && nit: I know it will be specc'd but maybe a comment explaining the reasoning here would help. e.g. // If a feature is enabled in the parent frame and the parent chooses to delegate it to a child frame // using the iframe attribute, then the feature should be enabled in the child frame. https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... File content/common/feature_policy/feature_policy.h (right): https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... content/common/feature_policy/feature_policy.h:162: const ParsedFeaturePolicyHeader* frame_policy, nit: it's a bit weird that we pass a ParsedFeaturePolicyHeader here. We might want to consider that at some point though it's a minor issue in the scheme of things. I wonder if it's better to name the variable frame_owner_policy or iframe_policy to be a bit clearer for readers? https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... File content/common/feature_policy/feature_policy_unittest.cc (right): https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:753: {{"default-self", false, {origin_b_}}}}; nit: We may want to have 2 functions to create the 2 different types of these structs to make this clearer. alexmos asked me to do this in a code review, see: https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... Since it's in 2 places now we might want to make those functions helpers somewhere. https://codereview.chromium.org/2655023004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2655023004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:193: const WebParsedFeaturePolicyHeader* framePolicy) { nit: it's a bit strange that we pass in a WebParsedFeaturePolicyHeader here now since this isn't really a header. It's only a minor naming issue, but we might want to consider it.
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...
iclelland@chromium.org changed reviewers: + pfeldman@chromium.org
Thanks, raymes! +r pfeldman, can you PTAL as OWNER? https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... File content/common/feature_policy/feature_policy.cc (right): https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... content/common/feature_policy/feature_policy.cc:182: if (frame_policy) { On 2017/02/20 02:20:17, raymes wrote: > nit: we should be consistent with brackets around single-line if's. > Unfortunately the style guide says nothing, but at least within files we should > try to be consistent :) Done. https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... content/common/feature_policy/feature_policy.cc:200: if (WhitelistFromDeclaration(parsed_declaration)->Contains(origin_) && On 2017/02/20 02:20:17, raymes wrote: > nit: I know it will be specc'd but maybe a comment explaining the reasoning here > would help. e.g. > // If a feature is enabled in the parent frame and the parent chooses to > delegate it to a child frame > // using the iframe attribute, then the feature should be enabled in the child > frame. Done. (And in blink-side implementation as well) https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... File content/common/feature_policy/feature_policy.h (right): https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... content/common/feature_policy/feature_policy.h:162: const ParsedFeaturePolicyHeader* frame_policy, On 2017/02/20 02:20:17, raymes wrote: > nit: it's a bit weird that we pass a ParsedFeaturePolicyHeader here. We might > want to consider that at some point though it's a minor issue in the scheme of > things. > > I wonder if it's better to name the variable frame_owner_policy or iframe_policy > to be a bit clearer for readers? I've started using the term 'container policy' in the spec, to avoid overloading the term 'frame' even further. I'll change it to that here for consistency. https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... File content/common/feature_policy/feature_policy_unittest.cc (right): https://codereview.chromium.org/2655023004/diff/80001/content/common/feature_... content/common/feature_policy/feature_policy_unittest.cc:753: {{"default-self", false, {origin_b_}}}}; On 2017/02/20 02:20:17, raymes wrote: > nit: We may want to have 2 functions to create the 2 different types of these > structs to make this clearer. alexmos asked me to do this in a code review, see: > https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... > > Since it's in 2 places now we might want to make those functions helpers > somewhere. That's not a bad idea. I'll follow up with that in another CL, if that's okay. https://codereview.chromium.org/2655023004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2655023004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:193: const WebParsedFeaturePolicyHeader* framePolicy) { On 2017/02/20 02:20:17, raymes wrote: > nit: it's a bit strange that we pass in a WebParsedFeaturePolicyHeader here now > since this isn't really a header. It's only a minor naming issue, but we might > want to consider it. Agreed. Will follow up as soon as the other changes settle.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by iclelland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by iclelland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487735592369680,
"parent_rev": "9d509c6bcafa280cde5094762d83656fc13606aa", "commit_rev":
"de94fd8361c6d3c0af5e4cdf757594f80340f789"}
Message was sent while issue was closed.
Description was changed from ========== Feature policy: Add basic algorithm for supporting frame policies. This allows a parsed policy from an iframe element to be used to influence the policy inherited by the framed document. BUG=682258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Feature policy: Add basic algorithm for supporting frame policies. This allows a parsed policy from an iframe element to be used to influence the policy inherited by the framed document. BUG=682258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2655023004 Cr-Commit-Position: refs/heads/master@{#451887} Committed: https://chromium.googlesource.com/chromium/src/+/de94fd8361c6d3c0af5e4cdf7575... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/de94fd8361c6d3c0af5e4cdf7575... |
