From ec9f379cae9a0de20d4c6ac0264b56d65481fd79 Mon Sep 17 00:00:00 2001 From: Arthur Edelstein Date: Wed, 30 Aug 2023 16:42:55 -0700 Subject: [PATCH] Upgrade URLs typed with http: (#19822) --- .../https_upgrade_browsertest.cc | 68 +++++++++++++------ .../renderer_host/chrome_navigation_ui_data.h | 19 ++++++ .../browser/ssl/https_upgrades_interceptor.cc | 6 ++ test/filters/browser_tests.filter | 4 +- 4 files changed, 75 insertions(+), 22 deletions(-) create mode 100644 chromium_src/chrome/browser/renderer_host/chrome_navigation_ui_data.h diff --git a/browser/brave_shields/https_upgrade_browsertest.cc b/browser/brave_shields/https_upgrade_browsertest.cc index 7438441df12..444a0d7efdd 100644 --- a/browser/brave_shields/https_upgrade_browsertest.cc +++ b/browser/brave_shields/https_upgrade_browsertest.cc @@ -30,7 +30,9 @@ #if BUILDFLAG(IS_ANDROID) #include "chrome/test/base/android/android_browser_test.h" #else +#include "chrome/browser/ui/browser_navigator_params.h" #include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/ui_test_utils.h" #endif using brave_shields::ControlType; @@ -45,6 +47,7 @@ struct TestCase { const char* domain; const char* path; ControlType control_type; + bool type_url; PageResult expected_result; }; @@ -53,24 +56,36 @@ constexpr char kSimple[] = "/simple.html"; constexpr char kNonexistent[] = "/nonexistent.html"; constexpr TestCase kTestCases[] = { - {false, "insecure1.com", kSimple, ControlType::ALLOW, PageResult::kHttp}, - {false, "insecure2.com", kSimple, ControlType::BLOCK_THIRD_PARTY, + {false, "insecure1.com", kSimple, ControlType::ALLOW, false, PageResult::kHttp}, - {false, "insecure3.com", kSimple, ControlType::BLOCK, - PageResult::kInterstitial}, - {false, "broken1.com", kNonexistent, ControlType::ALLOW, PageResult::kHttp}, - {false, "broken2.com", kNonexistent, ControlType::BLOCK_THIRD_PARTY, + {false, "insecure2.com", kSimple, ControlType::BLOCK_THIRD_PARTY, false, PageResult::kHttp}, - {false, "broken3.com", kNonexistent, ControlType::BLOCK, + {false, "insecure3.com", kSimple, ControlType::BLOCK, false, PageResult::kInterstitial}, - {false, "upgradable1.com", kSimple, ControlType::ALLOW, PageResult::kHttp}, - {false, "upgradable2.com", kSimple, ControlType::BLOCK_THIRD_PARTY, + {false, "broken1.com", kNonexistent, ControlType::ALLOW, false, + PageResult::kHttp}, + {false, "broken2.com", kNonexistent, ControlType::BLOCK_THIRD_PARTY, false, + PageResult::kHttp}, + {false, "broken3.com", kNonexistent, ControlType::BLOCK, false, + PageResult::kInterstitial}, + {false, "upgradable1.com", kSimple, ControlType::ALLOW, false, + PageResult::kHttp}, + {false, "upgradable2.com", kSimple, ControlType::BLOCK_THIRD_PARTY, false, PageResult::kHttps}, - {false, "upgradable3.com", kSimple, ControlType::BLOCK, PageResult::kHttps}, - {true, "secure1.com", kSimple, ControlType::ALLOW, PageResult::kHttps}, - {true, "secure2.com", kSimple, ControlType::BLOCK_THIRD_PARTY, + {false, "upgradable3.com", kSimple, ControlType::BLOCK, false, PageResult::kHttps}, - {true, "secure3.com", kSimple, ControlType::BLOCK, PageResult::kHttps}}; + {false, "upgradable1.com", kSimple, ControlType::ALLOW, true, + PageResult::kHttp}, + {false, "upgradable2.com", kSimple, ControlType::BLOCK_THIRD_PARTY, true, + PageResult::kHttps}, + {false, "upgradable3.com", kSimple, ControlType::BLOCK, true, + PageResult::kHttps}, + {true, "secure1.com", kSimple, ControlType::ALLOW, false, + PageResult::kHttps}, + {true, "secure2.com", kSimple, ControlType::BLOCK_THIRD_PARTY, false, + PageResult::kHttps}, + {true, "secure3.com", kSimple, ControlType::BLOCK, false, + PageResult::kHttps}}; base::FilePath GetTestDataDir() { return base::FilePath(FILE_PATH_LITERAL("net/data/url_request_unittest")); @@ -135,14 +150,24 @@ class HttpsUpgradeBrowserTest : public PlatformBrowserTest { mock_cert_verifier_.TearDownInProcessBrowserTestFixture(); } - void AttemptToNavigateToURL(const GURL& url) { + void AttemptNavigation(const GURL& url, bool url_typed_with_http_scheme) { +#if BUILDFLAG(IS_ANDROID) + // Chromium Android does not appear to skip upgrading URLs typed + // with an http scheme, so we don't need a special setting: content::NavigateToURLBlockUntilNavigationsComplete(Contents(), url, 1, true); +#else + NavigateParams params(chrome_test_utils::GetProfile(this), url, + ui::PAGE_TRANSITION_TYPED); + params.url_typed_with_http_scheme = url_typed_with_http_scheme; + ui_test_utils::NavigateToURL(¶ms); +#endif } GURL RunTestCaseNavigation(bool shields_enabled, bool global_setting, - const TestCase& test_case) { + const TestCase& test_case, + bool type_url) { SCOPED_TRACE(testing::Message() << "global_setting: " << global_setting << ", " << "test_case.init_secure: " << test_case.init_secure << ", " @@ -161,7 +186,7 @@ class HttpsUpgradeBrowserTest : public PlatformBrowserTest { // Run navigation twice to ensure that the behavior doesn't // change after first run. for (int i = 0; i < 2; ++i) { - AttemptToNavigateToURL(initial_url); + AttemptNavigation(initial_url, type_url); } return initial_url; } @@ -201,7 +226,8 @@ class HttpsUpgradeBrowserTest_FlagDisabled : public HttpsUpgradeBrowserTest { IN_PROC_BROWSER_TEST_F(HttpsUpgradeBrowserTest, CheckUpgrades) { for (bool global_setting : {true, false}) { for (const TestCase& test_case : kTestCases) { - RunTestCaseNavigation(true, global_setting, test_case); + RunTestCaseNavigation(true, global_setting, test_case, + test_case.type_url); bool interstitial_showing = chrome_browser_interstitials::IsShowingInterstitial(Contents()); if (test_case.expected_result == PageResult::kInterstitial) { @@ -221,8 +247,8 @@ IN_PROC_BROWSER_TEST_F(HttpsUpgradeBrowserTest, CheckUpgrades) { IN_PROC_BROWSER_TEST_F(HttpsUpgradeBrowserTest, CheckUpgradesWithShieldsDown) { for (bool global_setting : {true, false}) { for (const TestCase& test_case : kTestCases) { - const GURL initial_url = - RunTestCaseNavigation(false, global_setting, test_case); + const GURL initial_url = RunTestCaseNavigation( + false, global_setting, test_case, test_case.type_url); // Shields down means no URLs change and no interstitials shown. EXPECT_EQ(initial_url, Contents()->GetLastCommittedURL()); bool interstitial_showing = @@ -235,8 +261,8 @@ IN_PROC_BROWSER_TEST_F(HttpsUpgradeBrowserTest, CheckUpgradesWithShieldsDown) { IN_PROC_BROWSER_TEST_F(HttpsUpgradeBrowserTest_FlagDisabled, CheckUpgrades) { for (bool global_setting : {true, false}) { for (const TestCase& test_case : kTestCases) { - const GURL initial_url = - RunTestCaseNavigation(true, global_setting, test_case); + const GURL initial_url = RunTestCaseNavigation( + true, global_setting, test_case, test_case.type_url); // Disabled flag means no URLs change and no interstitials shown. EXPECT_EQ(initial_url, Contents()->GetLastCommittedURL()); bool interstitial_showing = diff --git a/chromium_src/chrome/browser/renderer_host/chrome_navigation_ui_data.h b/chromium_src/chrome/browser/renderer_host/chrome_navigation_ui_data.h new file mode 100644 index 00000000000..04ce7ea2eff --- /dev/null +++ b/chromium_src/chrome/browser/renderer_host/chrome_navigation_ui_data.h @@ -0,0 +1,19 @@ +/* Copyright (c) 2023 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_RENDERER_HOST_CHROME_NAVIGATION_UI_DATA_H_ +#define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_RENDERER_HOST_CHROME_NAVIGATION_UI_DATA_H_ + +#define url_is_typed_with_http_scheme() \ + return_false() const { \ + return false; \ + } \ + bool url_is_typed_with_http_scheme() + +#include "src/chrome/browser/renderer_host/chrome_navigation_ui_data.h" // IWYU pragma: export + +#undef url_is_typed_with_http_scheme + +#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_RENDERER_HOST_CHROME_NAVIGATION_UI_DATA_H_ diff --git a/chromium_src/chrome/browser/ssl/https_upgrades_interceptor.cc b/chromium_src/chrome/browser/ssl/https_upgrades_interceptor.cc index 396ad25c0f3..78899bd0f1b 100644 --- a/chromium_src/chrome/browser/ssl/https_upgrades_interceptor.cc +++ b/chromium_src/chrome/browser/ssl/https_upgrades_interceptor.cc @@ -11,6 +11,9 @@ #include "net/base/features.h" #include "net/base/url_util.h" +// Prevent double-defining macro +#include "chrome/browser/renderer_host/chrome_navigation_ui_data.h" + #define MaybeCreateLoader(...) \ MaybeCreateLoader(__VA_ARGS__) { \ if (brave_shields::IsHttpsByDefaultFeatureEnabled()) { \ @@ -57,9 +60,12 @@ #define IsLocalhost(URL) IsLocalhostOrOnion(URL) +#define url_is_typed_with_http_scheme() return_false() + #include "src/chrome/browser/ssl/https_upgrades_interceptor.cc" #undef MaybeCreateLoader #undef MaybeCreateLoaderForResponse #undef IsEnabled #undef IsLocalhost +#undef url_is_typed_with_http_scheme diff --git a/test/filters/browser_tests.filter b/test/filters/browser_tests.filter index 09328633fc7..3aa6769b9c4 100644 --- a/test/filters/browser_tests.filter +++ b/test/filters/browser_tests.filter @@ -662,9 +662,11 @@ -HttpsUpgradesBrowserTest.SlowHttps_ShouldInterstitial/* -HttpsUpgradesBrowserTest.URLsAutocompletedWithHttpSchemeAreUpgraded/* -HttpsUpgradesBrowserTest.URLsTypedWithHttpSchemeNoUpgrades/* --HttpsUpgradesBrowserTest.UrlWithHttpScheme_ShouldUpgrade/* +-HttpsUpgradesBrowserTest.URLsTypedWithHttpSchemeNoUpgradesAllowlist/HttpsFirstModeWithSiteEngagementAndHttpsUpgrades +-HttpsUpgradesBrowserTest.URLsTypedWithHttpSchemeNoUpgradesAllowlist/HttpsUpgradesOnly -HttpsUpgradesBrowserTest.UrlWithHttpScheme_BrokenSSL_ShouldInterstitial/* -HttpsUpgradesBrowserTest.UrlWithHttpScheme_BrokenSSL_ShouldInterstitial_SiteEngagement/* +-HttpsUpgradesBrowserTest.UrlWithHttpScheme_ShouldUpgrade/* -HttpsUpgradesBrowserTest.UrlWithHttpsScheme_ShouldLoad/* # These tests fail because we change the Learn More link in the HTTPS First