Skip to content

Commit

Permalink
[HTTPS-First Mode] Prefer showing net error pages for transient errors
Browse files Browse the repository at this point in the history 
This CL exempts transient network errors (such as host resolution
failures, address unreachable errors, etc.) from immediately falling
back to HTTP and showing the HTTPS-First Mode interstitial. Instead,
this change allows the failed navigation request to go through and
show the usual network error page, which gives the user more useful
context about the error. On reloading the error page (which also
happens automatically), the upgraded navigation is continued and if it
ultimately fails will then fallback to the original HTTP fallback URL.

Bug: 1394910,1277211
Change-Id: I9204054a071d3d151a3c77187c24c785d5df6c57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4559385
Reviewed-by: Mustafa Emre Acer <[email protected]>
Commit-Queue: Chris Thompson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1149160}
  • Loading branch information
@christhompson
christhompson authored and Chromium LUCI CQ committed May 25, 2023
1 parent 4e2d529 commit 101affa
Show file tree
Hide file tree
Showing 5 changed files with 198 additions and 21 deletions.
18 changes: 15 additions & 3 deletions chrome/browser/ssl/https_only_mode_tab_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
#include "chrome/browser/ssl/https_only_mode_tab_helper.h"

#include "chrome/common/chrome_features.h"
#include "content/public/browser/navigation_entry.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/reload_type.h"

HttpsOnlyModeTabHelper::~HttpsOnlyModeTabHelper() = default;

Expand All @@ -15,9 +17,19 @@ void HttpsOnlyModeTabHelper::DidStartNavigation(
// across navigations and handles clearing them separately. Only reset if
// the HFMv2 implementation is being used to avoid interfering with HFMv1.
if (base::FeatureList::IsEnabled(features::kHttpsFirstModeV2)) {
set_fallback_url(GURL());
set_is_navigation_fallback(false);
set_is_navigation_upgraded(false);
// If the user was on an exempt net error and the tab was reloaded, only
// reset the exempt error state, but keep the upgrade state so the reload
// will result in continuing to attempt the upgraded navigation (and if it
// later fails, the fallback will be to the original fallback URL).
bool should_maintain_upgrade_state =
is_exempt_error() &&
navigation_handle->GetReloadType() != content::ReloadType::NONE;
set_is_exempt_error(false);
if (!should_maintain_upgrade_state) {
set_fallback_url(GURL());
set_is_navigation_fallback(false);
set_is_navigation_upgraded(false);
}
}
}

Expand Down
14 changes: 14 additions & 0 deletions chrome/browser/ssl/https_only_mode_tab_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ class HttpsOnlyModeTabHelper
}
void add_failed_upgrade(const GURL& url) { failed_upgrade_urls_.insert(url); }

void set_is_exempt_error(bool is_exempt_error) {
is_exempt_error_ = is_exempt_error;
}
bool is_exempt_error() { return is_exempt_error_; }

private:
explicit HttpsOnlyModeTabHelper(content::WebContents* web_contents);
friend class content::WebContentsUserData<HttpsOnlyModeTabHelper>;
Expand Down Expand Up @@ -77,6 +82,15 @@ class HttpsOnlyModeTabHelper
// hostnames will also be on the HTTP allowlist, bypassing upgrade attempts.
std::set<GURL> failed_upgrade_urls_;

// Set to true if the current navigation resulted in a net error that is
// indicative of potentially-transient network conditions (such as a hostname
// resolution failure, the network being disconnected, or an address being
// unreachable) which don't signal that the server doesn't support HTTPS. This
// is used to track whether to maintain upgrade state across reloads (such as
// the automatic net error reload) and continue the upgrade attempt
// post-reload.
bool is_exempt_error_ = false;

WEB_CONTENTS_USER_DATA_KEY_DECL();
};

Expand Down
115 changes: 115 additions & 0 deletions chrome/browser/ssl/https_upgrades_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
#include "net/test/embedded_test_server/http_request.h"
#include "net/test/embedded_test_server/request_handler_util.h"
#include "net/test/test_data_directory.h"
#include "services/network/public/cpp/url_loader_completion_status.h"
#include "services/network/public/mojom/network_context.mojom.h"
#include "ui/base/l10n/l10n_util.h"
#include "url/url_constants.h"
Expand Down Expand Up @@ -891,6 +892,120 @@ IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest,
histograms()->ExpectBucketCount(kEventHistogram, Event::kUpgradeNetError, 1);
}

// If the upgraded HTTPS URL is not available due to a potentially-transient
// exempted net error (here a DNS resolution error), show the regular net error
// page instead of the HTTPS-First Mode interstitial. If the network conditions
// change such that the network error no longer triggers, reloading the tab
// should continue the upgraded navigation, which will fail and trigger fallback
// to HTTP. (Regression test for crbug.com/1277211.)
IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest,
ExemptNetErrorOnUpgrade_ShouldNotFallback) {
// This test is only interesting when HTTPS-First Mode is enabled.
if (!IsHttpsFirstModePrefEnabled()) {
return;
}

GURL http_url = http_server()->GetURL("bad-https.com", "/simple.html");
GURL https_url = https_server()->GetURL("bad-https.com", "/simple.html");
auto* contents = browser()->tab_strip_model()->GetActiveWebContents();

{
// Set up an interceptor that will return ERR_NAME_NOT_RESOLVED. Navigating
// to the HTTP URL should get upgraded to HTTPS, but fail with a net error
// page on the HTTPS URL.
auto dns_failure_interceptor =
std::make_unique<content::URLLoaderInterceptor>(base::BindRepeating(
[](content::URLLoaderInterceptor::RequestParams* params) {
params->client->OnComplete(network::URLLoaderCompletionStatus(
net::ERR_NAME_NOT_RESOLVED));
return true;
}));
EXPECT_FALSE(content::NavigateToURL(contents, http_url));
EXPECT_EQ(https_url, contents->GetLastCommittedURL());
EXPECT_FALSE(chrome_browser_interstitials::IsShowingInterstitial(contents));

// Reload the tab. The net error should still be showing as the navigation
// still results in ERR_NAME_NOT_RESOLVED.
content::TestNavigationObserver nav_observer(contents, 1);
contents->GetController().Reload(content::ReloadType::NORMAL,
/*check_for_repost=*/false);
nav_observer.Wait();
EXPECT_EQ(https_url, contents->GetLastCommittedURL());
EXPECT_FALSE(chrome_browser_interstitials::IsShowingInterstitial(contents));
}

// Interceptor is now out of scope and no longer applies. Reload the tab and
// the upgraded navigation should continue, fail due to the bad HTTPS on the
// server, and fall back to HTTP.
content::TestNavigationObserver nav_observer(contents, 1);
contents->GetController().Reload(content::ReloadType::NORMAL,
/*check_for_repost=*/false);
nav_observer.Wait();

if (IsHttpsFirstModePrefEnabled()) {
ASSERT_TRUE(
chrome_browser_interstitials::IsShowingHttpsFirstModeInterstitial(
contents));
ProceedThroughInterstitial(contents);
}

// Should now be on the HTTP URL and it should be allowlisted.
EXPECT_EQ(http_url, contents->GetLastCommittedURL());
Profile* profile = Profile::FromBrowserContext(contents->GetBrowserContext());
content::SSLHostStateDelegate* state = profile->GetSSLHostStateDelegate();
EXPECT_TRUE(state->IsHttpAllowedForHost(
http_url.host(), contents->GetPrimaryMainFrame()->GetStoragePartition()));
}

// Test that if one site redirects to a non-existent site, that we show the
// regular net error page instead of the HTTPS-First Mode interstitial.
// (Regression test for crbug.com/1277211.)
IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest,
RedirectToNonexistentSite_ShouldNotInterstitial) {
// This test is only interesting when HTTPS-First Mode is enabled.
if (!IsHttpsFirstModePrefEnabled()) {
return;
}

std::string nonexistent_domain = "nonexistentsite.com";
GURL nonexistent_http_url =
http_server()->GetURL(nonexistent_domain, "/simple.html");
GURL nonexistent_https_url =
https_server()->GetURL(nonexistent_domain, "/simple.html");
std::string www_redirect_path =
base::StrCat({"/server-redirect?", nonexistent_http_url.spec()});
GURL redirecting_https_url =
https_server()->GetURL("foo.com", www_redirect_path);
GURL redirecting_http_url =
http_server()->GetURL("foo.com", www_redirect_path);

auto* contents = browser()->tab_strip_model()->GetActiveWebContents();

// Set up an interceptor that will return ERR_NAME_NOT_RESOLVED for
// nonexistentsite.com.
auto dns_failure_interceptor =
std::make_unique<content::URLLoaderInterceptor>(
base::BindLambdaForTesting(
[nonexistent_domain](
content::URLLoaderInterceptor::RequestParams* params) {
if (params->url_request.url.host() == nonexistent_domain) {
params->client->OnComplete(network::URLLoaderCompletionStatus(
net::ERR_NAME_NOT_RESOLVED));
return true;
}
return false;
}));

// Navigating to the HTTP URL should get upgraded to HTTPS, but fail with a
// net error page on the HTTPS URL.
EXPECT_FALSE(content::NavigateToURL(contents, redirecting_http_url));
EXPECT_FALSE(
chrome_browser_interstitials::IsShowingHttpsFirstModeInterstitial(
contents));
EXPECT_EQ(url::kHttpsScheme, contents->GetLastCommittedURL().scheme());
EXPECT_EQ(nonexistent_domain, contents->GetLastCommittedURL().host());
}

// Navigations in subframes should not get upgraded by HTTPS-Only Mode. They
// should be blocked as mixed content.
IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest,
Expand Down
65 changes: 47 additions & 18 deletions chrome/browser/ssl/https_upgrades_interceptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,20 @@ bool DoesInsecureContentSettingDisableUpgrading(const GURL& url,
#endif
}

// Check for net errors that should not result in an HTTPS-First Mode
// interstitial. These cover cases where the error is most likely related to the
// local network conditions or a transient error, where it is more useful to
// show the user a network error page than the HTTPS-First Mode interstitial.
// (Or, in the case of HTTPS Upgrades, we don't want to fallback and allowlist
// the hostname yet -- instead we want to show the network error page and then
// retry the HTTPS upgrade again later.)
bool IsHttpsFirstModeExemptedError(int error) {
return net::IsHostnameResolutionError(error) ||
error == net::ERR_NETWORK_CHANGED ||
error == net::ERR_INTERNET_DISCONNECTED ||
error == net::ERR_ADDRESS_UNREACHABLE;
}

} // namespace

using RequestHandler = HttpsUpgradesInterceptor::RequestHandler;
Expand Down Expand Up @@ -227,6 +241,23 @@ void HttpsUpgradesInterceptor::MaybeCreateLoader(
tab_helper = HttpsOnlyModeTabHelper::FromWebContents(web_contents);
}

StatefulSSLHostStateDelegate* state =
static_cast<StatefulSSLHostStateDelegate*>(
profile->GetSSLHostStateDelegate());
auto* storage_partition =
web_contents->GetPrimaryMainFrame()->GetStoragePartition();

// Set up the interstitial state before checking any exclusions to upgrades,
// as some may depend on this being configured.
interstitial_state_ = std::make_unique<
security_interstitials::https_only_mode::HttpInterstitialState>();
interstitial_state_->enabled_by_pref = http_interstitial_enabled_by_pref_;
// StatefulSSLHostStateDelegate can be null during tests.
if (state && state->IsHttpsEnforcedForHost(
tentative_resource_request.url.host(), storage_partition)) {
interstitial_state_->enabled_by_engagement_heuristic = true;
}

// Exclude HTTPS URLs.
if (tentative_resource_request.url.SchemeIs(url::kHttpsScheme)) {
RecordNavigationRequestSecurityLevel(
Expand All @@ -252,21 +283,6 @@ void HttpsUpgradesInterceptor::MaybeCreateLoader(
return;
}

StatefulSSLHostStateDelegate* state =
static_cast<StatefulSSLHostStateDelegate*>(
profile->GetSSLHostStateDelegate());
auto* storage_partition =
web_contents->GetPrimaryMainFrame()->GetStoragePartition();

interstitial_state_ = std::make_unique<
security_interstitials::https_only_mode::HttpInterstitialState>();
interstitial_state_->enabled_by_pref = http_interstitial_enabled_by_pref_;
// StatefulSSLHostStateDelegate can be null during tests.
if (state && state->IsHttpsEnforcedForHost(
tentative_resource_request.url.host(), storage_partition)) {
interstitial_state_->enabled_by_engagement_heuristic = true;
}

// Don't exclude local-network requests (for now) but record metrics for them.
if (net::IsHostnameNonUnique(tentative_resource_request.url.host())) {
RecordNavigationRequestSecurityLevel(
Expand Down Expand Up @@ -361,8 +377,7 @@ void HttpsUpgradesInterceptor::MaybeCreateLoaderOnHstsQueryCompleted(
// Then check whether the host has been allowlisted by the user (or by a
// previous upgrade attempt failing).
// TODO(crbug.com/1394910): Distinguish HTTPS-First Mode and HTTPS-Upgrades
// allowlist entries, and ensure that HTTPS-Upgrades allowlist entries don't
// downgrade Page Info.
// allowlist entries.
// TODO(crbug.com/1394910): Move this to a helper function `IsAllowlisted()`,
// especially once this gets more complicated for HFM vs. Upgrades.
StatefulSSLHostStateDelegate* state =
Expand Down Expand Up @@ -506,7 +521,6 @@ bool HttpsUpgradesInterceptor::MaybeCreateLoaderForResponse(
bool* skip_other_interceptors,
bool* will_return_unsafe_redirect) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

// When an upgraded navigation fails, this method creates a loader to trigger
// the fallback to HTTP.
//
Expand Down Expand Up @@ -542,6 +556,21 @@ bool HttpsUpgradesInterceptor::MaybeCreateLoaderForResponse(
return false;
}

// If the navigation resulted in an exempted transient network error, don't
// intercept the failure and allow the normal net error page to trigger. Set
// the exempt-error flag so if the net error page is reloaded we can try
// continuing with the upgrade (and fallback to the original URL if the
// transient network error goes away and the navigation fails).
// Currently this is only done if the HTTPS-First Mode interstitial is
// enabled, because otherwise these would cause the interstitial to be shown
// which is confusing. HTTPS-Upgrades will silently fall back to HTTP for
// these errors.
if (IsInterstitialEnabled(*interstitial_state_) &&
IsHttpsFirstModeExemptedError(status.error_code)) {
tab_helper->set_is_exempt_error(true);
return false;
}

Profile* profile =
Profile::FromBrowserContext(web_contents->GetBrowserContext());
StatefulSSLHostStateDelegate* state =
Expand Down
7 changes: 7 additions & 0 deletions chrome/browser/ssl/https_upgrades_navigation_throttle.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,13 @@ HttpsUpgradesNavigationThrottle::WillStartRequest() {
// heuristic.
}

// TODO(crbug.com/1448371): There are some cases where the navigation may
// "restart", such as if we encounter an exempted transient network error on
// the upgraded HTTPS URL, show a net error page, and then reload the tab. In
// these cases the navigation will proceed with the upgrade/fallback logic,
// but the navigation timeout will no longer be set. Currently, re-starting
// the timer here would trigger a DCHECK in NavigationRequest.

// Navigation is HTTPS or an initial HTTP navigation (which will get
// upgraded by the interceptor). Fallback HTTP navigations are handled in
// WillRedirectRequest().
Expand Down

0 comments on commit 101affa

Please sign in to comment.