Merge pull request #26989 from brave/news-direct-feed-https

Upgrade direct feed redirect requests to HTTPS, respect strict upgrade setting

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",

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"(<rss version="2.0">
+    <channel>
+      <title>Hacker News</title>
+      <link>https://news.ycombinator.com/</link>
+      <description>Links for the intellectually curious, ranked by readers.</description>
+      <item>
+        <title>Enough with the dead butterflies (2017)</title>
+        <link>https://www.emilydamstra.com/please-enough-dead-butterflies/</link>
+        <pubDate>Sun, 3 Mar 2024 22:40:13 +0000</pubDate>
+        <comments>https://news.ycombinator.com/item?id=39585207</comments>
+        <description><![CDATA[<a href="https://news.ycombinator.com/item?id=39585207">Comments</a>]]></description>
+      </item>
+    </channel>
+  </rss>)";
+}
+
+}  // 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<net::test_server::HttpResponse> {
+          LOG(ERROR) << "request to https " << request.GetURL().path();
+          if (request.GetURL().path() == "/feed") {
+            auto response =
+                std::make_unique<net::test_server::BasicHttpResponse>();
+            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<net::test_server::BasicHttpResponse>();
+            response->set_code(net::HTTP_MOVED_PERMANENTLY);
+            response->AddCustomHeader("Location", "/feed");
+            return response;
+          }
+          return nullptr;
+        }));
+    fetcher_ = std::make_unique<DirectFeedFetcher>(
+        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<DirectFeedFetcher::Delegate> AsWeakPtr() override {
+      return weak_ptr_factory_.GetWeakPtr();
+    }
+
+   private:
+    base::WeakPtrFactory<MockDelegate> weak_ptr_factory_{this};
+  };
+
+  net::EmbeddedTestServer https_server_{net::EmbeddedTestServer::TYPE_HTTPS};
+  content::ContentMockCertVerifier mock_cert_verifier_;
+  MockDelegate delegate_;
+  std::unique_ptr<DirectFeedFetcher> 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<DirectFeedResult>(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

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<DirectFeedFetcher::Delegate>

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<DirectFeedFetcher::Delegate> AsWeakPtr() override;
 

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<DirectFeedFetcher::Delegate> AsWeakPtr() override {
       return weak_ptr_factory_.GetWeakPtr();

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<Delegate::HTTPSUpgradeInfo> https_upgrade_info) {
+  if (!https_upgrade_info && url.SchemeIs(url::kHttpScheme)) {
     content::GetUIThreadTaskRunner({})->PostTask(
         FROM_HERE,
         base::BindOnce(
             [](base::WeakPtr<Delegate> delegate,
                scoped_refptr<base::SingleThreadTaskRunner> source_task_runner,
-               base::OnceCallback<void(bool)> callback, GURL url) {
+               base::OnceCallback<void(
+                   std::optional<Delegate::HTTPSUpgradeInfo>)> 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<void(bool)> callback, bool result) {
+                      [](base::OnceCallback<void(
+                             std::optional<Delegate::HTTPSUpgradeInfo>)>
+                             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<network::ResourceRequest>();
   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<Delegate::HTTPSUpgradeInfo> https_upgrade_info,
     bool https_upgraded,
+    size_t redirect_count,
     std::unique_ptr<std::string> 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)));