From 8d9a0b8785671735bde0c41d2f430cd781dbeb14 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Wed, 11 Dec 2024 16:20:28 -0800 Subject: [PATCH 1/2] Upgrade direct feed redirect requests to HTTPS, respect strict upgrade setting --- .../direct_feed_fetcher_delegate_impl.cc | 21 +++-- .../direct_feed_fetcher_delegate_impl.h | 3 +- .../direct_feed_controller_unittest.cc | 8 +- .../brave_news/browser/direct_feed_fetcher.cc | 76 +++++++++++++------ .../brave_news/browser/direct_feed_fetcher.h | 35 ++++++--- 5 files changed, 98 insertions(+), 45 deletions(-) diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.cc b/browser/brave_news/direct_feed_fetcher_delegate_impl.cc index 22ce8cfeeaf5..25186ad5dc31 100644 --- a/browser/brave_news/direct_feed_fetcher_delegate_impl.cc +++ b/browser/brave_news/direct_feed_fetcher_delegate_impl.cc @@ -6,6 +6,7 @@ #include "brave/browser/brave_news/direct_feed_fetcher_delegate_impl.h" #include "brave/browser/brave_browser_process.h" +#include "brave/components/brave_news/browser/direct_feed_fetcher.h" #include "brave/components/brave_shields/content/browser/brave_shields_util.h" #include "content/public/browser/browser_thread.h" @@ -15,17 +16,21 @@ DirectFeedFetcherDelegateImpl::DirectFeedFetcherDelegateImpl( HostContentSettingsMap* host_content_settings_map) : host_content_settings_map_(host_content_settings_map), https_upgrade_exceptions_service_( - g_brave_browser_process->https_upgrade_exceptions_service()) {} + g_brave_browser_process->https_upgrade_exceptions_service()) { + CHECK(host_content_settings_map_); + CHECK(https_upgrade_exceptions_service_); +} DirectFeedFetcherDelegateImpl::~DirectFeedFetcherDelegateImpl() = default; -bool DirectFeedFetcherDelegateImpl::ShouldUpgradeToHttps(const GURL& url) { - DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - if (!host_content_settings_map_ || !https_upgrade_exceptions_service_) { - return true; - } - return brave_shields::ShouldUpgradeToHttps(host_content_settings_map_, url, - https_upgrade_exceptions_service_); +DirectFeedFetcher::Delegate::HTTPSUpgradeInfo +DirectFeedFetcherDelegateImpl::GetURLHTTPSUpgradeInfo(const GURL& url) { + HTTPSUpgradeInfo info; + info.should_upgrade = brave_shields::ShouldUpgradeToHttps( + host_content_settings_map_, url, https_upgrade_exceptions_service_); + info.should_force = + brave_shields::ShouldForceHttps(host_content_settings_map_, url); + return info; } base::WeakPtr diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.h b/browser/brave_news/direct_feed_fetcher_delegate_impl.h index 339dd4af5a55..b20053a3649c 100644 --- a/browser/brave_news/direct_feed_fetcher_delegate_impl.h +++ b/browser/brave_news/direct_feed_fetcher_delegate_impl.h @@ -28,7 +28,8 @@ class DirectFeedFetcherDelegateImpl : public DirectFeedFetcher::Delegate { const DirectFeedFetcherDelegateImpl&) = delete; // Must be called on UI thread - bool ShouldUpgradeToHttps(const GURL& url) override; + DirectFeedFetcher::Delegate::HTTPSUpgradeInfo GetURLHTTPSUpgradeInfo( + const GURL& url) override; base::WeakPtr AsWeakPtr() override; diff --git a/components/brave_news/browser/direct_feed_controller_unittest.cc b/components/brave_news/browser/direct_feed_controller_unittest.cc index 04f49b0651ed..aa7845caa8a6 100644 --- a/components/brave_news/browser/direct_feed_controller_unittest.cc +++ b/components/brave_news/browser/direct_feed_controller_unittest.cc @@ -86,7 +86,13 @@ class BraveNewsDirectFeedControllerTest : public testing::Test { public: ~MockDirectFeedFetcherDelegate() override = default; - bool ShouldUpgradeToHttps(const GURL& url) override { return true; } + DirectFeedFetcher::Delegate::HTTPSUpgradeInfo GetURLHTTPSUpgradeInfo( + const GURL& url) override { + HTTPSUpgradeInfo info; + info.should_upgrade = true; + info.should_force = false; + return info; + } base::WeakPtr AsWeakPtr() override { return weak_ptr_factory_.GetWeakPtr(); diff --git a/components/brave_news/browser/direct_feed_fetcher.cc b/components/brave_news/browser/direct_feed_fetcher.cc index d3e7a7c0b1b3..085498eeaaaa 100644 --- a/components/brave_news/browser/direct_feed_fetcher.cc +++ b/components/brave_news/browser/direct_feed_fetcher.cc @@ -30,6 +30,8 @@ namespace brave_news { namespace { +constexpr size_t kMaxRedirectCount = 7; + std::string GetResponseCharset(network::SimpleURLLoader* loader) { auto* response_info = loader->ResponseInfo(); if (!response_info) { @@ -154,44 +156,53 @@ DirectFeedFetcher::~DirectFeedFetcher() = default; void DirectFeedFetcher::DownloadFeed(GURL url, std::string publisher_id, DownloadFeedCallback callback) { - if (url.SchemeIs(url::kHttpScheme)) { + DownloadFeedHelper(url, url, publisher_id, 0, std::move(callback), + std::nullopt); +} + +void DirectFeedFetcher::DownloadFeedHelper( + GURL url, + GURL original_url, + std::string publisher_id, + size_t redirect_count, + DownloadFeedCallback callback, + std::optional https_upgrade_info) { + if (!https_upgrade_info && url.SchemeIs(url::kHttpScheme)) { content::GetUIThreadTaskRunner({})->PostTask( FROM_HERE, base::BindOnce( [](base::WeakPtr delegate, scoped_refptr source_task_runner, - base::OnceCallback callback, GURL url) { + base::OnceCallback)> callback, + GURL url) { if (delegate.WasInvalidated()) { return; } - bool should_upgrade = delegate->ShouldUpgradeToHttps(url); + auto upgrade_info = delegate->GetURLHTTPSUpgradeInfo(url); source_task_runner->PostTask( FROM_HERE, base::BindOnce( - [](base::OnceCallback callback, bool result) { + [](base::OnceCallback)> + callback, + Delegate::HTTPSUpgradeInfo result) { std::move(callback).Run(result); }, - std::move(callback), should_upgrade)); + std::move(callback), upgrade_info)); }, delegate_, base::SingleThreadTaskRunner::GetCurrentDefault(), base::BindOnce(&DirectFeedFetcher::DownloadFeedHelper, - weak_ptr_factory_.GetWeakPtr(), url, publisher_id, - std::move(callback)), + weak_ptr_factory_.GetWeakPtr(), url, original_url, + publisher_id, redirect_count, std::move(callback)), url)); return; } - DownloadFeedHelper(url, publisher_id, std::move(callback), false); -} - -void DirectFeedFetcher::DownloadFeedHelper(GURL url, - std::string publisher_id, - DownloadFeedCallback callback, - bool should_https_upgrade) { // Make request auto request = std::make_unique(); bool https_upgraded = false; - if (should_https_upgrade) { + if (https_upgrade_info && https_upgrade_info->should_upgrade) { GURL::Replacements replacements; replacements.SetSchemeStr(url::kHttpsScheme); url = url.ReplaceComponents(replacements); @@ -202,6 +213,7 @@ void DirectFeedFetcher::DownloadFeedHelper(GURL url, request->load_flags = net::LOAD_DO_NOT_SAVE_COOKIES; request->credentials_mode = network::mojom::CredentialsMode::kOmit; request->method = net::HttpRequestHeaders::kGetMethod; + request->redirect_mode = network::mojom::RedirectMode::kError; auto url_loader = network::SimpleURLLoader::Create( std::move(request), GetNetworkTrafficAnnotationTag()); url_loader->SetRetryOptions( @@ -216,24 +228,37 @@ void DirectFeedFetcher::DownloadFeedHelper(GURL url, // Handle response base::BindOnce(&DirectFeedFetcher::OnFeedDownloaded, weak_ptr_factory_.GetWeakPtr(), iter, std::move(callback), - url, std::move(publisher_id), https_upgraded), + url, original_url, std::move(publisher_id), + https_upgrade_info, https_upgraded, redirect_count), 5 * 1024 * 1024); } void DirectFeedFetcher::OnFeedDownloaded( SimpleURLLoaderList::iterator iter, DownloadFeedCallback callback, - GURL feed_url, + GURL url, + GURL original_url, std::string publisher_id, + std::optional https_upgrade_info, bool https_upgraded, + size_t redirect_count, std::unique_ptr response_body) { auto* loader = iter->get(); + + if (loader->NetError() == net::ERR_FAILED && loader->GetFinalURL() != url && + redirect_count < kMaxRedirectCount) { + // Redirect detected. Make another request + DownloadFeedHelper(loader->GetFinalURL(), original_url, publisher_id, + redirect_count + 1, std::move(callback), std::nullopt); + return; + } + auto response_code = -1; auto result = DirectFeedResponse(); result.charset = GetResponseCharset(loader); - result.url = feed_url; - result.final_url = loader->GetFinalURL(); + result.url = original_url; + result.final_url = url; if (loader->ResponseInfo()) { auto headers_list = loader->ResponseInfo()->headers; @@ -248,12 +273,15 @@ void DirectFeedFetcher::OnFeedDownloaded( std::string body_content = response_body ? *response_body : ""; if (response_code < 200 || response_code >= 300 || body_content.empty()) { - VLOG(1) << feed_url.spec() << " invalid response, state: " << response_code; - if (https_upgraded) { + VLOG(1) << url.spec() << " invalid response, state: " << response_code; + if (https_upgraded && https_upgrade_info && + !https_upgrade_info->should_force) { GURL::Replacements replacements; replacements.SetSchemeStr(url::kHttpScheme); - feed_url = feed_url.ReplaceComponents(replacements); - DownloadFeedHelper(feed_url, publisher_id, std::move(callback), false); + url = url.ReplaceComponents(replacements); + https_upgrade_info->should_upgrade = false; + DownloadFeedHelper(url, original_url, publisher_id, redirect_count, + std::move(callback), https_upgrade_info); return; } DirectFeedError error; @@ -264,7 +292,7 @@ void DirectFeedFetcher::OnFeedDownloaded( } ParseFeedDataOffMainThread( - feed_url, std::move(publisher_id), std::move(body_content), + url, std::move(publisher_id), std::move(body_content), base::BindOnce(&DirectFeedFetcher::OnParsedFeedData, weak_ptr_factory_.GetWeakPtr(), std::move(callback), std::move(result))); diff --git a/components/brave_news/browser/direct_feed_fetcher.h b/components/brave_news/browser/direct_feed_fetcher.h index 5b24e7620f51..f5d90045f87b 100644 --- a/components/brave_news/browser/direct_feed_fetcher.h +++ b/components/brave_news/browser/direct_feed_fetcher.h @@ -71,7 +71,12 @@ class DirectFeedFetcher { public: virtual ~Delegate() = default; - virtual bool ShouldUpgradeToHttps(const GURL& url) = 0; + struct HTTPSUpgradeInfo { + bool should_upgrade; + bool should_force; + }; + + virtual HTTPSUpgradeInfo GetURLHTTPSUpgradeInfo(const GURL& url) = 0; virtual base::WeakPtr AsWeakPtr() = 0; }; @@ -92,16 +97,24 @@ class DirectFeedFetcher { using SimpleURLLoaderList = std::list>; - void DownloadFeedHelper(GURL url, - std::string publisher_id, - DownloadFeedCallback callback, - bool should_https_upgrade); - void OnFeedDownloaded(SimpleURLLoaderList::iterator iter, - DownloadFeedCallback callback, - GURL feed_url, - std::string publisher_id, - bool https_upgraded, - const std::unique_ptr response_body); + void DownloadFeedHelper( + GURL url, + GURL original_url, + std::string publisher_id, + + size_t redirect_count, + DownloadFeedCallback callback, + std::optional https_upgrade_info); + void OnFeedDownloaded( + SimpleURLLoaderList::iterator iter, + DownloadFeedCallback callback, + GURL url, + GURL original_url, + std::string publisher_id, + std::optional https_upgrade_info, + bool https_upgraded, + size_t redirect_count, + const std::unique_ptr response_body); void OnParsedFeedData(DownloadFeedCallback callback, DirectFeedResponse result, absl::variant data); From 6dfb50fdf59ed534ce1e49f4db97aa263628583d Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Thu, 12 Dec 2024 16:21:42 -0800 Subject: [PATCH 2/2] Add News Direct feed redirect test --- browser/brave_news/BUILD.gn | 7 +- .../direct_feed_fetcher_browsertest.cc | 124 ++++++++++++++++++ .../brave_news/browser/direct_feed_fetcher.h | 3 +- 3 files changed, 131 insertions(+), 3 deletions(-) create mode 100644 browser/brave_news/direct_feed_fetcher_browsertest.cc diff --git a/browser/brave_news/BUILD.gn b/browser/brave_news/BUILD.gn index 000a87f19f88..7ac8e4779a66 100644 --- a/browser/brave_news/BUILD.gn +++ b/browser/brave_news/BUILD.gn @@ -8,14 +8,19 @@ source_set("browser_tests") { defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ] if (!is_android) { - sources = [ "brave_news_tab_helper_browsertest.cc" ] + sources = [ + "brave_news_tab_helper_browsertest.cc", + "direct_feed_fetcher_browsertest.cc", + ] deps = [ "//brave/components//brave_rewards/browser", + "//brave/components/brave_news/browser", "//brave/components/brave_news/common:common", "//brave/components/brave_news/common:mojom", "//brave/components/constants", "//chrome/browser", + "//chrome/browser:browser_process", "//chrome/browser/profiles", "//chrome/browser/profiles:profile", "//chrome/browser/ui:ui", diff --git a/browser/brave_news/direct_feed_fetcher_browsertest.cc b/browser/brave_news/direct_feed_fetcher_browsertest.cc new file mode 100644 index 000000000000..2ff577b04521 --- /dev/null +++ b/browser/brave_news/direct_feed_fetcher_browsertest.cc @@ -0,0 +1,124 @@ +// Copyright (c) 2024 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/. + +#include "brave/components/brave_news/browser/direct_feed_fetcher.h" + +#include "base/test/bind.h" +#include "chrome/browser/browser_process.h" +#include "chrome/browser/net/system_network_context_manager.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "content/public/test/browser_test.h" +#include "content/public/test/content_mock_cert_verifier.h" +#include "net/dns/mock_host_resolver.h" +#include "net/test/embedded_test_server/embedded_test_server.h" +#include "net/test/embedded_test_server/http_request.h" +#include "net/test/embedded_test_server/http_response.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace brave_news { + +namespace { +std::string GetBasicFeed() { + return R"( + + Hacker News + https://news.ycombinator.com/ + Links for the intellectually curious, ranked by readers. + + Enough with the dead butterflies (2017) + https://www.emilydamstra.com/please-enough-dead-butterflies/ + Sun, 3 Mar 2024 22:40:13 +0000 + https://news.ycombinator.com/item?id=39585207 + Comments]]> + + + )"; +} + +} // namespace + +class DirectFeedFetcherBrowserTest : public InProcessBrowserTest { + public: + DirectFeedFetcherBrowserTest() = default; + + void SetUpOnMainThread() override { + InProcessBrowserTest::SetUpOnMainThread(); + + mock_cert_verifier_.mock_cert_verifier()->set_default_result(net::OK); + host_resolver()->AddRule("*", "127.0.0.1"); + + https_server_.RegisterRequestHandler(base::BindLambdaForTesting( + [](const net::test_server::HttpRequest& request) + -> std::unique_ptr { + LOG(ERROR) << "request to https " << request.GetURL().path(); + if (request.GetURL().path() == "/feed") { + auto response = + std::make_unique(); + response->set_code(net::HTTP_OK); + response->set_content(GetBasicFeed()); + response->set_content_type("application/rss+xml"); + return response; + } else if (request.GetURL().path() == "/feed2") { + auto response = + std::make_unique(); + response->set_code(net::HTTP_MOVED_PERMANENTLY); + response->AddCustomHeader("Location", "/feed"); + return response; + } + return nullptr; + })); + fetcher_ = std::make_unique( + g_browser_process->system_network_context_manager() + ->GetSharedURLLoaderFactory(), + delegate_.AsWeakPtr()); + + ASSERT_TRUE(https_server_.Start()); + } + + protected: + class MockDelegate : public DirectFeedFetcher::Delegate { + public: + ~MockDelegate() override = default; + + DirectFeedFetcher::Delegate::HTTPSUpgradeInfo GetURLHTTPSUpgradeInfo( + const GURL& url) override { + HTTPSUpgradeInfo info; + info.should_upgrade = true; + info.should_force = false; + return info; + } + + base::WeakPtr AsWeakPtr() override { + return weak_ptr_factory_.GetWeakPtr(); + } + + private: + base::WeakPtrFactory weak_ptr_factory_{this}; + }; + + net::EmbeddedTestServer https_server_{net::EmbeddedTestServer::TYPE_HTTPS}; + content::ContentMockCertVerifier mock_cert_verifier_; + MockDelegate delegate_; + std::unique_ptr fetcher_; +}; + +IN_PROC_BROWSER_TEST_F(DirectFeedFetcherBrowserTest, RedirectToFeed) { + base::RunLoop run_loop; + GURL feed2_url = https_server_.GetURL("/feed2"); + + fetcher_->DownloadFeed( + feed2_url, "test_publisher", + base::BindLambdaForTesting([&](DirectFeedResponse response) { + const auto& result = absl::get(response.result); + ASSERT_EQ(1u, result.articles.size()); + EXPECT_EQ(feed2_url.spec(), response.url.spec()); + EXPECT_EQ("Hacker News", result.title); + run_loop.Quit(); + })); + + run_loop.Run(); +} + +} // namespace brave_news diff --git a/components/brave_news/browser/direct_feed_fetcher.h b/components/brave_news/browser/direct_feed_fetcher.h index f5d90045f87b..f8d1b436fcbf 100644 --- a/components/brave_news/browser/direct_feed_fetcher.h +++ b/components/brave_news/browser/direct_feed_fetcher.h @@ -80,7 +80,7 @@ class DirectFeedFetcher { virtual base::WeakPtr AsWeakPtr() = 0; }; - explicit DirectFeedFetcher( + DirectFeedFetcher( scoped_refptr url_loader_factory, base::WeakPtr delegate); DirectFeedFetcher(const DirectFeedFetcher&) = delete; @@ -101,7 +101,6 @@ class DirectFeedFetcher { GURL url, GURL original_url, std::string publisher_id, - size_t redirect_count, DownloadFeedCallback callback, std::optional https_upgrade_info);