Skip to content

Commit

Permalink
Enable mixed image autoupgrades by default
Browse files Browse the repository at this point in the history
Bug: 1042877
Change-Id: Ib4ba9d336bde09e82c17594dcd105448b2ff8916
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2454853
Commit-Queue: Carlos IL <carlosil@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Auto-Submit: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818153}
  • Loading branch information
carlosjoan91 authored and Commit Bot committed Oct 16, 2020
1 parent 175070a commit 7e60647
Show file tree
Hide file tree
Showing 14 changed files with 62 additions and 60 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1838,14 +1838,11 @@ private boolean getUseStricMixedContentCheckingLocked() {

@CalledByNative
private boolean getAllowMixedContentAutoupgradesLocked() {
assert Thread.holdsLock(mAwSettingsLock);
// We only allow mixed content autoupgrades (upgrading HTTP subresources to HTTPS in HTTPS
// sites) when the mixed content mode is set to MIXED_CONTENT_COMPATIBILITY, which keeps it
// in line with the behavior in Chrome. With MIXED_CONTENT_ALWAYS_ALLOW, we disable
// autoupgrades since the developer is explicitly allowing mixed content, whereas with
// MIXED_CONTENT_NEVER_ALLOW, there is no need to autoupgrade since the content will be
// blocked.
return mMixedContentMode == WebSettings.MIXED_CONTENT_COMPATIBILITY_MODE;
// TODO(crbug.com/1139424): Mixed content autoupgrades are temporarily disabled completely
// on WebView. This should remain as is for MIXED_CONTENT_ALWAYS_ALLOW and
// MIXED_CONTENT_NEVER_ALLOW, but should be controlled via a variations flag for
// MIXED_CONTENT_COMPATIBILITY_MODE.
return false;
}

public boolean getOffscreenPreRaster() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,9 @@ IN_PROC_BROWSER_TEST_F(
ConversionRegistrationBrowserTest,
RegisterWithDifferentUrlTypes_ConversionReceivedOrIgnored) {
const char kSecureHost[] = "a.test";
// TODO(crbug.com/1137113): Should include a test where an insecure request is
// blocked from conversion registration if it is made on a secure page. Note
// that this can't work for image requests due to image auto-upgrade.
struct {
std::string page_host;
std::string redirect_host;
Expand All @@ -327,8 +330,6 @@ IN_PROC_BROWSER_TEST_F(
{kSecureHost /* page_host */, kSecureHost /* redirect_host */,
true /* conversion_expected */},
{"insecure.com" /* page_host */, kSecureHost /* redirect_host */,
false /* conversion_expected */},
{kSecureHost /* page_host */, "insecure.com" /* redirect_host */,
false /* conversion_expected */}};

for (const auto& test_case : kTestCases) {
Expand Down
25 changes: 23 additions & 2 deletions content/browser/site_per_process_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6978,6 +6978,22 @@ IN_PROC_BROWSER_TEST_P(SitePerProcessBrowserTest,
EvalJs(foo_root, "self.origin;"));
}

// A subclass of SitePerProcessIgnoreCertErrorsBrowsertest that disables mixed
// content autoupgrades.
// TODO(carlosil): Since the flag will be cleaned up eventually, this should be
// changed to proper plumbing that adds the relevant urls to the allowlist.
class SitePerProcessIgnoreCertErrorsAllowMixedContentBrowserTest
: public SitePerProcessIgnoreCertErrorsBrowserTest {
public:
SitePerProcessIgnoreCertErrorsAllowMixedContentBrowserTest() {
feature_list.InitAndDisableFeature(
blink::features::kMixedContentAutoupgrade);
}

private:
base::test::ScopedFeatureList feature_list;
};

// Tests that the WebContents is notified when passive mixed content is
// displayed in an OOPIF. The test ignores cert errors so that an HTTPS
// iframe can be loaded from a site other than localhost (the
Expand All @@ -6988,8 +7004,9 @@ IN_PROC_BROWSER_TEST_P(SitePerProcessBrowserTest,
#else
#define MAYBE_PassiveMixedContentInIframe PassiveMixedContentInIframe
#endif
IN_PROC_BROWSER_TEST_P(SitePerProcessIgnoreCertErrorsBrowserTest,
MAYBE_PassiveMixedContentInIframe) {
IN_PROC_BROWSER_TEST_P(
SitePerProcessIgnoreCertErrorsAllowMixedContentBrowserTest,
MAYBE_PassiveMixedContentInIframe) {
net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTPS);
https_server.ServeFilesFromSourceDirectory(GetTestDataFilePath());
SetupCrossSiteRedirector(&https_server);
Expand Down Expand Up @@ -15965,6 +15982,10 @@ INSTANTIATE_TEST_SUITE_P(All,
INSTANTIATE_TEST_SUITE_P(All,
SitePerProcessIgnoreCertErrorsBrowserTest,
testing::ValuesIn(RenderDocumentFeatureLevelValues()));
INSTANTIATE_TEST_SUITE_P(
All,
SitePerProcessIgnoreCertErrorsAllowMixedContentBrowserTest,
testing::ValuesIn(RenderDocumentFeatureLevelValues()));
INSTANTIATE_TEST_SUITE_P(All,
SitePerProcessProgrammaticScrollTest,
testing::ValuesIn(RenderDocumentFeatureLevelValues()));
Expand Down
21 changes: 0 additions & 21 deletions testing/variations/fieldtrial_testing_config.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,27 +41,6 @@
]
}
],
"AllPassiveMixedContentAutoupgrade": [
{
"platforms": [
"android",
"chromeos",
"linux",
"mac"
],
"experiments": [
{
"name": "Enabled_10",
"params": {
"mode": "all-passive"
},
"enable_features": [
"AutoupgradeMixedContent"
]
}
]
}
],
"AmpBackgroundTab": [
{
"platforms": [
Expand Down
12 changes: 1 addition & 11 deletions third_party/blink/renderer/core/loader/mixed_content_checker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -771,17 +771,7 @@ bool MixedContentChecker::ShouldAutoupgrade(
return false;
}

auto autoupgrade_mode = base::GetFieldTrialParamValueByFeature(
blink::features::kMixedContentAutoupgrade,
blink::features::kMixedContentAutoupgradeModeParamName);

if (autoupgrade_mode ==
blink::features::kMixedContentAutoupgradeModeAllPassive) {
return true;
}

// Otherwise we default to excluding images.
return type != mojom::RequestContextType::IMAGE;
return true;
}

void MixedContentChecker::CheckMixedPrivatePublic(
Expand Down
4 changes: 0 additions & 4 deletions third_party/blink/web_tests/NeverFixTests
Original file line number Diff line number Diff line change
Expand Up @@ -1415,10 +1415,6 @@ crbug.com/869492 virtual/threaded/external/wpt/feature-policy/experimental-featu
crbug.com/869492 virtual/threaded/external/wpt/feature-policy/experimental-features/lazyload/lazyload-enabled-image-tentative.sub.html [ Skip ]
crbug.com/869492 virtual/threaded/external/wpt/feature-policy/experimental-features/lazyload/lazyload-image-loading-lazy-tentative.sub.html [ Skip ]

# Tests that only work when the mixed content autoupgrade feature is enabled.
# Covered in virtual test suite.
http/tests/mixed-autoupgrade/* [ Skip ]

# Not expected to pass in default configuration, only virtual test suite.
crbug.com/830901 fast/webgl/video-metadata/texImage-video-last-uploaded-metadata.html [ Skip ]
virtual/webgl-extra-video-texture-metadata/fast/webgl/video-metadata/texImage-video-last-uploaded-metadata.html [ Pass ]
Expand Down
29 changes: 25 additions & 4 deletions third_party/blink/web_tests/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -5990,10 +5990,6 @@ crbug.com/1025274 external/wpt/mixed-content/gen/top.meta/unset/audio-tag.https.
crbug.com/1025274 external/wpt/mixed-content/gen/top.meta/unset/video-tag.https.html [ Failure ]
crbug.com/1025274 http/tests/security/mixedContent/insecure-audio-video-in-main-frame.html [ Failure ]

#Mixed content autoupgrade tests that can be re-enabled once image autoupgrades launch
crbug.com/1042877 virtual/autoupgrade-optionally-blockable-mixed-content/http/tests/mixed-autoupgrade/optionally/image-upgrade-console-message.https.html [ Failure ]
crbug.com/1042877 virtual/autoupgrade-optionally-blockable-mixed-content/http/tests/mixed-autoupgrade/optionally/image-upgrade.https.html [ Failure ]

# DevTools roll
crbug.com/1052111 http/tests/devtools/a11y-axe-core/audits-start-view-a11y-test.js [ Skip ]

Expand Down Expand Up @@ -6433,6 +6429,31 @@ crbug.com/1131252 virtual/threaded/external/wpt/css/css-transforms/animation/tra

crbug.com/1136163 [ Linux ] external/wpt/pointerevents/pointerlock/pointerevent_movementxy_with_pointerlock.html [ Failure ]

# Mixed content autoupgrades make these tests not applicable, since they check for mixed content images, the tests can be removed when cleaning up pre autoupgrades mixed content code.
crbug.com/1042877 external/wpt/fetch/cross-origin-resource-policy/scheme-restriction.https.window.html [ Failure ]
crbug.com/1042877 external/wpt/mixed-content/gen/top.meta/unset/img-tag.https.html [ Failure ]
crbug.com/1042877 external/wpt/mixed-content/imageset.https.sub.html [ Failure ]
crbug.com/1042877 external/wpt/upgrade-insecure-requests/gen/iframe-blank-inherit.meta/unset/img-tag.https.html [ Failure ]
crbug.com/1042877 external/wpt/upgrade-insecure-requests/gen/srcdoc-inherit.meta/unset/img-tag.https.html [ Failure ]
crbug.com/1042877 external/wpt/upgrade-insecure-requests/gen/top.meta/unset/img-tag.https.html [ Failure ]
crbug.com/1042877 virtual/omt-worker-fetch/external/wpt/fetch/cross-origin-resource-policy/scheme-restriction.https.window.html [ Failure ]
crbug.com/1042877 virtual/omt-worker-fetch/external/wpt/upgrade-insecure-requests/gen/iframe-blank-inherit.meta/unset/img-tag.https.html [ Failure ]
crbug.com/1042877 virtual/omt-worker-fetch/external/wpt/upgrade-insecure-requests/gen/srcdoc-inherit.meta/unset/img-tag.https.html [ Failure ]
crbug.com/1042877 virtual/omt-worker-fetch/external/wpt/upgrade-insecure-requests/gen/top.meta/unset/img-tag.https.html [ Failure ]
crbug.com/1042877 http/tests/security/mixedContent/insecure-css-image-with-reload.html [ Failure ]
crbug.com/1042877 http/tests/security/mixedContent/insecure-image-in-iframe.html [ Failure ]
crbug.com/1042877 http/tests/security/mixedContent/insecure-image-in-main-frame-allowed.html [ Failure ]
crbug.com/1042877 http/tests/security/mixedContent/insecure-image-in-main-frame-blocked.html [ Failure ]
crbug.com/1042877 http/tests/security/mixedContent/insecure-image-in-main-frame.html [ Failure ]
crbug.com/1042877 http/tests/security/mixedContent/preload-insecure-image-in-main-frame-blocked.html [ Failure ]
crbug.com/1042877 http/tests/security/mixedContent/strict-mode-image-blocked.https.html [ Timeout ]
crbug.com/1042877 http/tests/security/mixedContent/strict-mode-image-in-frame-blocked.https.html [ Failure ]
crbug.com/1042877 http/tests/security/mixedContent/strict-mode-image-reportonly.https.php [ Failure ]
crbug.com/1042877 http/tests/security/mixedContent/strict-mode-via-pref-image-blocked.https.html [ Failure ]

# Mixed content autoupgrades cause test to fail because test relied on http subresources to test a different origin, needs to be changed to not rely on HTTP URLs.
crbug.com/1042877 http/tests/security/img-crossorigin-redirect-credentials.https.html [ Failure ]

# Sheriff 2020-09-30
crbug.com/1133821 virtual/off-main-thread-css-paint/external/wpt/css/css-paint-api/one-custom-property-animation.https.html [ Pass Failure ]
crbug.com/1133821 virtual/off-main-thread-css-paint/external/wpt/css/css-paint-api/two-custom-property-animation.https.html [ Pass Failure ]
Expand Down
5 changes: 0 additions & 5 deletions third_party/blink/web_tests/VirtualTestSuites
Original file line number Diff line number Diff line change
Expand Up @@ -427,11 +427,6 @@
"args": ["--enable-blink-features=AutomaticLazyImageLoading",
"--disable-blink-features=RestrictAutomaticLazyImageLoadingToDataSaver"]
},
{
"prefix" : "autoupgrade-optionally-blockable-mixed-content",
"bases": ["http/tests/mixed-autoupgrade/optionally"],
"args": ["--enable-features=AutoupgradeMixedContent"]
},
{
"prefix": "webgl-extra-video-texture-metadata",
"bases": ["fast/webgl/video-metadata"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Inspector issue: {
requestId : <string>
url : http://devtools.test:8000/inspector-protocol/resources/img.png
}
resolutionStatus : MixedContentWarning
resolutionStatus : MixedContentAutomaticallyUpgraded
resourceType : Image
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Tests that willSendRequest contains the correct mixed content status for passive mixed content.
Network agent enabled
Mixed content type of https://example.test:8443/inspector-protocol/resources/passive-mixed-content-iframe.html: none
Mixed content type of http://example.test:8000/resources/square.png: optionally-blockable
Mixed content type of https://example.test:8000/resources/square.png: none

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CONSOLE WARNING: Mixed Content: The page at 'https://127.0.0.1:8443/security/referrer-policy-attribute-img-no-referrer-when-downgrade.html' was loaded over HTTPS, but requested an insecure element 'http://127.0.0.1:8000/security/resources/green-if-no-referrer.php'. This request was automatically upgraded to HTTPS, For more information see https://blog.chromium.org/2019/10/no-more-mixed-messages-about-https.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CONSOLE WARNING: Mixed Content: The page at 'https://127.0.0.1:8443/security/resources/referrer-policy-conflicting-policies.html' was loaded over HTTPS, but requested an insecure element 'http://127.0.0.1:8080/security/resources/green-if-no-referrer.php'. This request was automatically upgraded to HTTPS, For more information see https://blog.chromium.org/2019/10/no-more-mixed-messages-about-https.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CONSOLE WARNING: Mixed Content: The page at 'https://127.0.0.1:8443/security/resources/referrer-policy-conflicting-policies-image-set.html' was loaded over HTTPS, but requested an insecure element 'http://127.0.0.1:8080/security/resources/green-if-no-referrer.php'. This request was automatically upgraded to HTTPS, For more information see https://blog.chromium.org/2019/10/no-more-mixed-messages-about-https.html

This file was deleted.

0 comments on commit 7e60647

Please sign in to comment.