|
|
Created:
5 years, 2 months ago by estark Modified:
5 years, 2 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, gavinp+loader_chromium.org, asvitkine+watch_chromium.org, blink-reviews, kinuko+watch, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStrictly block blockable mixed subresources in subframes
This CL effectively stops showing the mixed content shield when a
subframe loads a blockable mixed resource. It's a step on the way to
removing the mixed content shield entirely.
Intent to deprecate:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/1yWQaXmkXpA/BDV1-dC9su4J
BUG=536925
Committed: https://crrev.com/7ab4d1e27a89e5cff91386e13c3b8736f6b15749
Cr-Commit-Position: refs/heads/master@{#354838}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : add a layout test #Patch Set 4 : fix content settings browser test #
Total comments: 2
Patch Set 5 : rebase #
Messages
Total messages: 34 (9 generated)
estark@chromium.org changed reviewers: + mkwst@chromium.org
mkwst, are you still on board with removing the mixed content shield when a subframe tries to load blockable mixed content? (See BUG for context.) If so, two questions for you: 1.) Do you think this should be controlled by a setting or a runtime enabled feature or neither? Right now it's neither. 2.) I completely, miserably failed at writing a test for this. (I blame jetlag.) I couldn't figure out how to construct a LocalFrame with a parent in its frame tree to pass in to shouldBlockFetch() in a unit test. Any tips?
ping :)
On 2015/10/07 at 17:07:39, estark wrote: > mkwst, are you still on board with removing the mixed content shield when a subframe tries to load blockable mixed content? (See BUG for context.) If so, two questions for you: I am! > 1.) Do you think this should be controlled by a setting or a runtime enabled feature or neither? Right now it's neither. If we want to be totally safe, we'd add a setting, and toggle it based on a Finch experiment. Given that we've already gotten approval to drop the shield entirely (https://groups.google.com/a/chromium.org/d/msg/blink-dev/1yWQaXmkXpA/BDV1-dC9...), and I'm only holding off on that because of Flash, dropping the shield inside subframes doesn't seem like something that would require safety. If folks explode in Canary/Dev, we can just revert the patch. Easy. > 2.) I completely, miserably failed at writing a test for this. (I blame jetlag.) I couldn't figure out how to construct a LocalFrame with a parent in its frame tree to pass in to shouldBlockFetch() in a unit test. Any tips? LayoutTest seems like the easiest thing to do here: look at `LayoutTests/http/tests/security/mixedContent/strict-mode-image-in-frame-blocked.https.html` for an example of how that might look.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1392993002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1392993002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1392993002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1392993002/20001
On 2015/10/09 15:17:36, Mike West (slow) wrote: > On 2015/10/07 at 17:07:39, estark wrote: > > mkwst, are you still on board with removing the mixed content shield when a > subframe tries to load blockable mixed content? (See BUG for context.) If so, > two questions for you: > > I am! > > > 1.) Do you think this should be controlled by a setting or a runtime enabled > feature or neither? Right now it's neither. > > If we want to be totally safe, we'd add a setting, and toggle it based on a > Finch experiment. Given that we've already gotten approval to drop the shield > entirely > (https://groups.google.com/a/chromium.org/d/msg/blink-dev/1yWQaXmkXpA/BDV1-dC9...), > and I'm only holding off on that because of Flash, dropping the shield inside > subframes doesn't seem like something that would require safety. If folks > explode in Canary/Dev, we can just revert the patch. Easy. Cool, sgtm. > > > 2.) I completely, miserably failed at writing a test for this. (I blame > jetlag.) I couldn't figure out how to construct a LocalFrame with a parent in > its frame tree to pass in to shouldBlockFetch() in a unit test. Any tips? > > LayoutTest seems like the easiest thing to do here: look at > `LayoutTests/http/tests/security/mixedContent/strict-mode-image-in-frame-blocked.https.html` > for an example of how that might look. Aha, now I see that there's a way to control the embedder's mixed content settings from a layout test, which is what I was missing. Done, thanks.
Fixed a failing test. Another thought: looking at this again, I wonder if we should be doing this a different way and actually having chrome/ decide whether or not to strictly block subresources in iframes. That is, right now I have Blink saying "Oh, it's an active subresource in an iframe, I'll just block it outright and not even ask the embedder." Instead, we could have Blink say "Oh, it's an active subresource in an iframe, I'll tell the embedder that when I send the notification about mixed content running." Then chrome/ can decide not to show the shield based on the fact that it's an active subresource in an iframe. The advantage of that approach is that it wouldn't force our new behavior on other embedders. The disadvantage is that it seems kind of messy to plumb a boolean is-active-subresource-inside-iframe all the way down to chrome/ to decide whether or not to show the shield based on it. What do you think?
On 2015/10/09 at 18:12:37, estark wrote: > Fixed a failing test. > > Another thought: looking at this again, I wonder if we should be doing this a different way and actually having chrome/ decide whether or not to strictly block subresources in iframes. That is, right now I have Blink saying "Oh, it's an active subresource in an iframe, I'll just block it outright and not even ask the embedder." Instead, we could have Blink say "Oh, it's an active subresource in an iframe, I'll tell the embedder that when I send the notification about mixed content running." Then chrome/ can decide not to show the shield based on the fact that it's an active subresource in an iframe. > > The advantage of that approach is that it wouldn't force our new behavior on other embedders. The disadvantage is that it seems kind of messy to plumb a boolean is-active-subresource-inside-iframe all the way down to chrome/ to decide whether or not to show the shield based on it. What do you think? Generally, I think it's good to give the embedder options. In this particular case, I don't think these are options we want to provide. :) If/when we/you move MIX up and out of Blink, we might be able to do this kind of work more cleanly in the embedder, so that the embedder can make all the relevant decisions about a particular request. For the moment, I think simply shutting down the message to the embedder is the right thing to do. Thanks for adding the test. Patch #4 LGTM.
estark@chromium.org changed reviewers: + felt@chromium.org, mpearson@chromium.org
Sounds good, thanks Mike. felt: can you please review content_setting_bubble_model_browsertest.cc? mpearson: can you please review histograms.xml?
https://codereview.chromium.org/1392993002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right): https://codereview.chromium.org/1392993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:337: // content is allowed. Why is that decision being codified here? Why not ask the embedder, passing in frame status?
https://codereview.chromium.org/1392993002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right): https://codereview.chromium.org/1392993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:337: // content is allowed. On 2015/10/12 05:52:02, felt wrote: > Why is that decision being codified here? Why not ask the embedder, passing in > frame status? I just saw that you already mentioned this point, and your reason was to avoid plumbing through the boolean (and not thinking any embedders will allow it anyway). But IMO it seems really odd (layering violation?) to have half the policy decided in one place and the other half at another layer.
On 2015/10/12 05:54:07, felt wrote: > https://codereview.chromium.org/1392993002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right): > > https://codereview.chromium.org/1392993002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:337: // content is > allowed. > On 2015/10/12 05:52:02, felt wrote: > > Why is that decision being codified here? Why not ask the embedder, passing in > > frame status? > > I just saw that you already mentioned this point, and your reason was to avoid > plumbing through the boolean (and not thinking any embedders will allow it > anyway). > > But IMO it seems really odd (layering violation?) to have half the policy > decided in one place and the other half at another layer. Hrm, I keep going back and forth... I see what you're saying, felt, and agree that it's odd to introduce a particular case where the embedder doesn't get to make the decision, but on the other hand, Blink already kind of reserves the right to not ask the embedder sometimes (e.g. when CSP says to strictly block mixed content). So the fact that the Blink <-> embedder contract already doesn't guarantee that the embedder always gets to decide, plus the fact that we'd be going out of our way to give the embedder flexibility that wouldn't actually be used, plus the fact that all active mixed content is going to be blocked strictly soon anyway, makes me lean towards just blocking it in Blink and leaving the embedder out of it.
histograms.xml lgtm for what it's worth
the browsertest change lgtm
On 2015/10/12 11:05:19, estark wrote: > On 2015/10/12 05:54:07, felt wrote: > > > https://codereview.chromium.org/1392993002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right): > > > > > https://codereview.chromium.org/1392993002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:337: // content > is > > allowed. > > On 2015/10/12 05:52:02, felt wrote: > > > Why is that decision being codified here? Why not ask the embedder, passing > in > > > frame status? > > > > I just saw that you already mentioned this point, and your reason was to avoid > > plumbing through the boolean (and not thinking any embedders will allow it > > anyway). > > > > But IMO it seems really odd (layering violation?) to have half the policy > > decided in one place and the other half at another layer. > > Hrm, I keep going back and forth... I see what you're saying, felt, and agree > that it's odd to introduce a particular case where the embedder doesn't get to > make the decision, but on the other hand, Blink already kind of reserves the > right to not ask the embedder sometimes (e.g. when CSP says to strictly block > mixed content). So the fact that the Blink <-> embedder contract already doesn't > guarantee that the embedder always gets to decide, plus the fact that we'd be > going out of our way to give the embedder flexibility that wouldn't actually be > used, plus the fact that all active mixed content is going to be blocked > strictly soon anyway, makes me lean towards just blocking it in Blink and > leaving the embedder out of it. Are there other embedders that might break? (Like any of the fancy schmancy Chrome-derivative projects.)
On 2015/10/14 at 00:45:20, felt wrote: > > Hrm, I keep going back and forth... I see what you're saying, felt, and agree > > that it's odd to introduce a particular case where the embedder doesn't get to > > make the decision, but on the other hand, Blink already kind of reserves the > > right to not ask the embedder sometimes (e.g. when CSP says to strictly block > > mixed content). So the fact that the Blink <-> embedder contract already doesn't > > guarantee that the embedder always gets to decide, plus the fact that we'd be > > going out of our way to give the embedder flexibility that wouldn't actually be > > used, plus the fact that all active mixed content is going to be blocked > > strictly soon anyway, makes me lean towards just blocking it in Blink and > > leaving the embedder out of it. > > Are there other embedders that might break? (Like any of the fancy schmancy Chrome-derivative projects.) "break" in the sense that they wouldn't load insecure resources from nested frames? Yes. All of them, including Chrome. I think the claim here is that that behavior isn't "broken", but, in fact, desirable. :) I agree with your layering concerns in a broad sense; the embedder should make policy decisions, and Blink should abide. That said, this is a policy decision that we're not going to make: we'd basically be piping a lot of information up to the embedder in order for it to hard-code `return false;`. I'm not nearly concerned enough about layering to ask Emily to do that work without a use case for maybe sometimes saying `return true;`. At the moment, I don't see that use case, so I don't think the extra work is justified. Put another way, we don't ask the embedder whether or not we should block requests based on CSP; that's just the way the web works. Just because we're making a decision doesn't mean that we need to double check outside of Blink. :)
On 2015/10/14 07:36:51, Mike West (slow) wrote: > On 2015/10/14 at 00:45:20, felt wrote: > > > Hrm, I keep going back and forth... I see what you're saying, felt, and > agree > > > that it's odd to introduce a particular case where the embedder doesn't get > to > > > make the decision, but on the other hand, Blink already kind of reserves the > > > right to not ask the embedder sometimes (e.g. when CSP says to strictly > block > > > mixed content). So the fact that the Blink <-> embedder contract already > doesn't > > > guarantee that the embedder always gets to decide, plus the fact that we'd > be > > > going out of our way to give the embedder flexibility that wouldn't actually > be > > > used, plus the fact that all active mixed content is going to be blocked > > > strictly soon anyway, makes me lean towards just blocking it in Blink and > > > leaving the embedder out of it. > > > > Are there other embedders that might break? (Like any of the fancy schmancy > Chrome-derivative projects.) > > "break" in the sense that they wouldn't load insecure resources from nested > frames? Yes. All of them, including Chrome. I think the claim here is that that > behavior isn't "broken", but, in fact, desirable. :) > > I agree with your layering concerns in a broad sense; the embedder should make > policy decisions, and Blink should abide. That said, this is a policy decision > that we're not going to make: we'd basically be piping a lot of information up > to the embedder in order for it to hard-code `return false;`. I'm not nearly > concerned enough about layering to ask Emily to do that work without a use case > for maybe sometimes saying `return true;`. At the moment, I don't see that use > case, so I don't think the extra work is justified. That's what I am asking. Have you checked whether there any Blink embedders that currently rely on mixed content working, either for subframes or otherwise? Just checking this doesn't break Chromecast or something else like that. > > Put another way, we don't ask the embedder whether or not we should block > requests based on CSP; that's just the way the web works. Just because we're > making a decision doesn't mean that we need to double check outside of Blink. :)
On 2015/10/14 at 07:44:35, felt wrote: > That's what I am asking. Have you checked whether there any Blink embedders that currently rely on mixed content working, either for subframes or otherwise? Just checking this doesn't break Chromecast or something else like that. I would dearly love to break Chromecast, because their handling of mixed content is terrible. My vague understanding was they they were setting the "always allow" flag, which this change continues to respect. See `chromecast/browser/cast_content_browser_client.cc:256`: https://blink.lc/chromium/tree/chromecast/browser/cast_content_browser_client...
On 2015/10/14 at 08:20:43, Mike West (slow) wrote: > On 2015/10/14 at 07:44:35, felt wrote: > > That's what I am asking. Have you checked whether there any Blink embedders that currently rely on mixed content working, either for subframes or otherwise? Just checking this doesn't break Chromecast or something else like that. > > I would dearly love to break Chromecast, because their handling of mixed content is terrible. My vague understanding was they they were setting the "always allow" flag, which this change continues to respect. See `chromecast/browser/cast_content_browser_client.cc:256`: https://blink.lc/chromium/tree/chromecast/browser/cast_content_browser_client... I think we're early enough in M48 that we can land this and deal with fallout. As long as the flags continue to work the way they're supposed to, I think we'll be fine. :)
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1392993002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1392993002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, felt@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1392993002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1392993002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1392993002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7ab4d1e27a89e5cff91386e13c3b8736f6b15749 Cr-Commit-Position: refs/heads/master@{#354838} |