From fd7b51d35df0c0ff39aa6e87fe2ae4d53b7a8063 Mon Sep 17 00:00:00 2001
From: Gabriel Charette <gab@chromium.org>
Date: Thu, 10 Sep 2026 13:00:46 -0700
Subject: [PATCH] [Offline pages] Validate intent file digest before serving

When OfflinePageRequestHandler processes a navigation request carrying a
file_url_intent or content_url_intent reason, it serves the file
referenced by intent_url. However, ValidateFile() evaluated the
candidate OfflinePageItem's own archive rather than the file referenced
by intent_url. This caused two issues:
1. If the candidate was stored in an internal directory, the internal-
   directory fast-path assumed the target was trusted and skipped
   validation entirely.
2. If validation was skipped for intent URLs under the expectation of
   verifying at EOF, the file at intent_url was streamed over the Mojo
   data pipe before the digest was computed and compared. A digest
   mismatch was only detected after data was already delivered.

This change fixes this ordering and validation flow:
- Adds GetIntentFilePath() to resolve intent_url safely to a FilePath
  (returning an empty FilePath on invalid URLs).
- Updates ValidateFile() to open and validate the intent file's digest
  against the offline page's recorded digest before serving starts,
  regardless of where the candidate's own archive is located.
- Reuses the validated FileStream in OnTrustedOfflinePageFound() and
  seeks to 0 for serving, avoiding duplicate path resolution and open
  calls.
- Resets archive_validator_ in OnFileValidationDone() so a fresh hasher
  is allocated if advancing to another candidate on validation failure
  (previously unreachable for intents since validation was skipped).
- Removes redundant serving-time digest computation in
  DidReadForServing() as well as the at-EOF
  DidComputeActualDigestForServing() and ClearOfflinePageData() cleanup
  since validation is completed before serving begins.
- Updates the unit test fixture so that bytes_read() reflects data
  actually transferred over the data pipe on non-OK completion, and adds
  FailOnIntentFileMismatchForInternalPage to verify intent files with
  mismatched digests are not served.

R=dewittj@chromium.org
TAG=agy
CONV=9b81bd43-a335-43ec-81e4-e60832a65aed

Fixed: 520507737
Change-Id: Ife890b061d24b857b9182d34c8888edf8b58113f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8378587
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1695598}
---
diff --git a/chrome/browser/offline_pages/offline_page_request_handler.cc b/chrome/browser/offline_pages/offline_page_request_handler.cc
index 516aeb2..9fb470b 100644
--- a/chrome/browser/offline_pages/offline_page_request_handler.cc
+++ b/chrome/browser/offline_pages/offline_page_request_handler.cc
@@ -269,22 +269,7 @@
           OfflinePageRequestHandler::NetworkState::PROHIBITIVELY_SLOW_NETWORK);
 }
 
-void ClearOfflinePageData(content::WebContents::Getter web_contents_getter) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // |web_contents_getter| is passed from IO thread. We need to check if
-  // web contents is still valid.
-  content::WebContents* web_contents = web_contents_getter.Run();
-  if (!web_contents)
-    return;
-
-  // Save an cached copy of OfflinePageItem such that Tab code can get
-  // the loaded offline page immediately.
-  OfflinePageTabHelper* tab_helper =
-      OfflinePageTabHelper::FromWebContents(web_contents);
-  DCHECK(tab_helper);
-  tab_helper->ClearOfflinePage();
-}
 
 }  // namespace
 
@@ -424,27 +409,15 @@
   }
 
   // No need to open the file if it has already been opened for the validation.
+  // This is always the case when a file or content URL intent is being
+  // processed since the intent file is opened during validation.
   if (stream_) {
     DidOpenForServing(net::OK);
     return;
   }
+  DCHECK(!IsProcessingFileOrContentUrlIntent());
 
-  // If a file:// or content:// intent is being processed, open the file:// or
-  // content:// denoted in the intent instead. Otherwise, open the archive file
-  // associated with the offline page.
-  base::FilePath file_path;
-  if (IsProcessingFileUrlIntent()) {
-    bool valid = net::FileURLToFilePath(offline_header_.intent_url, &file_path);
-    DCHECK(valid);
-#if BUILDFLAG(IS_ANDROID)
-  } else if (IsProcessingContentUrlIntent()) {
-    file_path = base::FilePath(offline_header_.intent_url.spec());
-    DCHECK(file_path.IsContentUri());
-#endif  // BUILDFLAG(IS_ANDROID)
-  } else {
-    file_path = GetCurrentOfflinePage().file_path;
-  }
-  OpenFile(file_path,
+  OpenFile(GetCurrentOfflinePage().file_path,
            base::BindRepeating(&OfflinePageRequestHandler::DidOpenForServing,
                                weak_ptr_factory_.GetWeakPtr()));
 }
@@ -497,6 +470,26 @@
   return IsProcessingFileUrlIntent() || IsProcessingContentUrlIntent();
 }
 
+base::FilePath OfflinePageRequestHandler::GetIntentFilePath() const {
+  if (!IsProcessingFileOrContentUrlIntent()) {
+    return base::FilePath();
+  }
+  base::FilePath file_path;
+  if (IsProcessingFileUrlIntent()) {
+    if (!net::FileURLToFilePath(offline_header_.intent_url, &file_path)) {
+      return base::FilePath();
+    }
+#if BUILDFLAG(IS_ANDROID)
+  } else if (IsProcessingContentUrlIntent()) {
+    file_path = base::FilePath(offline_header_.intent_url.spec());
+    if (!file_path.IsContentUri()) {
+      return base::FilePath();
+    }
+#endif  // BUILDFLAG(IS_ANDROID)
+  }
+  return file_path;
+}
+
 void OfflinePageRequestHandler::OpenFile(
     const base::FilePath& file_path,
     const base::RepeatingCallback<void(int)>& callback) {
@@ -549,6 +542,23 @@
 }
 
 void OfflinePageRequestHandler::ValidateFile() {
+  // If a file or content URL intent is being processed, the file denoted in the
+  // intent will be served instead of the archive associated with the offline
+  // page, so the intent file is the one that must be validated. This is done
+  // regardless of where the offline page's own archive lives since that archive
+  // is not used.
+  if (IsProcessingFileOrContentUrlIntent()) {
+    if (GetCurrentOfflinePage().digest.empty() || GetIntentFilePath().empty()) {
+      OnFileValidationDone(FileValidationResult::FILE_VALIDATION_FAILED);
+      return;
+    }
+    OpenFile(
+        GetIntentFilePath(),
+        base::BindRepeating(&OfflinePageRequestHandler::DidOpenForValidation,
+                            weak_ptr_factory_.GetWeakPtr()));
+    return;
+  }
+
   // If the archive file is in internal directory, the offline page can be
   // deemed as trusted without going through valication.
   if (candidates_[candidate_index_].archive_is_in_internal_dir) {
@@ -563,18 +573,6 @@
     return;
   }
 
-  // If a file:// or content:// URL intent is being viewed, skip the validation.
-  // The digest for the file:// or content:// denoted in the intent was computed
-  // and used to find the offline page. However, we will not validate and read
-  // from the archive archive file assoicated with the offline page since it may
-  // not exist or even got modified. We will read from the file:// or content://
-  // denoted in the intent  and compute the digest of the read data to make sure
-  // it does not get changed.
-  if (IsProcessingFileOrContentUrlIntent()) {
-    OnFileValidationDone(FileValidationResult::FILE_VALIDATION_SUCCEEDED);
-    return;
-  }
-
   GetFileSizeForValidation();
 }
 
@@ -660,6 +658,11 @@
 
 void OfflinePageRequestHandler::OnFileValidationDone(
     FileValidationResult result) {
+  // The validator has been finalized and cannot be reused. Reset it so that a
+  // fresh instance is allocated if another digest needs to be computed (e.g.
+  // when moving to another candidate on validation failure).
+  archive_validator_ = nullptr;
+
   if (result == FileValidationResult::FILE_VALIDATION_SUCCEEDED) {
     OnTrustedOfflinePageFound();
     return;
@@ -675,7 +678,7 @@
     return;
   }
 
-  // Otherwise, no trusted offline page can be found so we fall back to the
+  // No trusted offline page can be found so we fall back to the
   // default handling.
   delegate_->FallbackToDefault();
 }
@@ -698,6 +701,11 @@
 
   // Note that we always seek to the beginning of the file because the file may
   // have already been read for validation purpose.
+  // Seeking to 0 is safe for file and content URL intents because any intent
+  // stream was already read to EOF during intent dispatch in
+  // OfflinePageBridge::GetSizeAndComputeDigest() to match against
+  // OfflinePageModel; a non-reopenable, one-shot stream would not have
+  // survived that initial dispatch step.
   int seek_result = stream_->Seek(
       0, base::BindOnce(&OfflinePageRequestHandler::DidSeekForServing,
                         weak_ptr_factory_.GetWeakPtr()));
@@ -720,61 +728,12 @@
 void OfflinePageRequestHandler::DidReadForServing(
     scoped_refptr<net::IOBuffer> buf,
     base::expected<base::ByteSize, net::Error> result) {
-  if (!result.has_value() || !IsProcessingFileOrContentUrlIntent()) {
-    buf = nullptr;
-    // TODO(hjanuschka): Update NotifyReadRawDataComplete to accept
-    // base::expected<base::ByteSize, net::Error> directly.
-    NotifyReadRawDataComplete(!result.has_value()
-                                  ? result.error()
-                                  : base::checked_cast<int>(result->InBytes()));
-    return;
-  }
-
-  // At this point, we have a successful read &&
-  // IsProcessingFileOrContentUrlIntent() which means the read succeeds for
-  // processing the file:// or content:// URL intent. We need to compute the
-  // digest to ensure that the file:// or content:// we read is not modified
-  // since the time we received the intent, validated the data provided by
-  // file:// or content:// URL, and decided to turn it into the corresponding
-  // http/https URL and let OfflinePageRequestHandler handle it.
-  if (result->is_positive()) {
-    int bytes_read = base::checked_cast<int>(result->InBytes());
-    UpdateDigestOnBackground(
-        buf, result->InBytes(),
-        base::BindOnce(&OfflinePageRequestHandler::NotifyReadRawDataComplete,
-                       weak_ptr_factory_.GetWeakPtr(), bytes_read));
-
-  } else {
-    // When bytes read is 0, it indicates EOF. We need to finalize the hash
-    // to get the actual digest.
-    FinalizeDigestOnBackground(base::BindOnce(
-        &OfflinePageRequestHandler::DidComputeActualDigestForServing,
-        weak_ptr_factory_.GetWeakPtr(), 0));
-  }
-}
-
-void OfflinePageRequestHandler::NotifyReadRawDataComplete(int result) {
-  delegate_->NotifyReadRawDataComplete(result);
-}
-
-void OfflinePageRequestHandler::DidComputeActualDigestForServing(
-    int result,
-    const std::string& actual_digest) {
-  // If the actual digest does not match, fail the request job.
-  bool mismatch = actual_digest != GetCurrentOfflinePage().digest;
-  if (mismatch) {
-    // Note: Do not call delegate_->SetOfflinePageNavigationUIData to clear
-    // the offline bit since SetOfflinePageNavigationUIData is supposed to
-    // be called before the response is being received. Furthermore, there is
-    // no need to clear the offline bit since the error code should already
-    // indicate that the offline page is not loaded.
-    content::GetUIThreadTaskRunner({})->PostTask(
-        FROM_HERE, base::BindOnce(&ClearOfflinePageData,
-                                  delegate_->GetWebContentsGetter()));
-    result = net::ERR_FAILED;
-  }
-
-  NotifyReadRawDataComplete(result);
+  buf = nullptr;
+  // TODO(hjanuschka): Update NotifyReadRawDataComplete to accept
+  // base::expected<base::ByteSize, net::Error> directly.
+  delegate_->NotifyReadRawDataComplete(
+      result.has_value() ? base::checked_cast<int>(result->InBytes())
+                         : result.error());
 }
 
 }  // namespace offline_pages
diff --git a/chrome/browser/offline_pages/offline_page_request_handler.h b/chrome/browser/offline_pages/offline_page_request_handler.h
index 9226498..9c7c8d11 100644
--- a/chrome/browser/offline_pages/offline_page_request_handler.h
+++ b/chrome/browser/offline_pages/offline_page_request_handler.h
@@ -175,6 +175,7 @@
   bool IsProcessingFileUrlIntent() const;
   bool IsProcessingContentUrlIntent() const;
   bool IsProcessingFileOrContentUrlIntent() const;
+  base::FilePath GetIntentFilePath() const;
 
   void OnTrustedOfflinePageFound();
   void VisitTrustedOfflinePage();
@@ -203,9 +204,6 @@
   void DidSeekForServing(base::expected<int64_t, net::Error> result);
   void DidReadForServing(scoped_refptr<net::IOBuffer> buf,
                          base::expected<base::ByteSize, net::Error> result);
-  void NotifyReadRawDataComplete(int result);
-  void DidComputeActualDigestForServing(int result,
-                                        const std::string& actual_digest);
 
   GURL url_;
   raw_ptr<Delegate> delegate_;
diff --git a/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc b/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc
index 2f2f0041..67237500 100644
--- a/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc
+++ b/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc
@@ -761,10 +761,6 @@
 }
 
 void OfflinePageURLLoaderBuilder::OnComplete() {
-  if (client_->completion_status().error_code != net::OK) {
-    mime_type_.clear();
-    body_.clear();
-  }
   ReadCompleted(
       ResponseInfo(client_->completion_status().error_code, mime_type_, body_));
   // Clear intermediate data in preparation for next potential page loading.
@@ -1366,10 +1362,7 @@
   EXPECT_EQ(net::ERR_FAILED, request_status());
   EXPECT_NE("multipart/related", mime_type());
   EXPECT_EQ(0, bytes_read());
-  // Note that the offline bit is not cleared on purpose due to the fact that
-  // other flag, like request status, should already indicate that the offline
-  // page fails to load.
-  EXPECT_TRUE(is_offline_page_set_in_navigation_data());
+  EXPECT_FALSE(is_offline_page_set_in_navigation_data());
   EXPECT_FALSE(offline_page_tab_helper()->GetOfflinePageForTest());
 }
 
@@ -1404,10 +1397,42 @@
   EXPECT_EQ(net::ERR_FAILED, request_status());
   EXPECT_NE("multipart/related", mime_type());
   EXPECT_EQ(0, bytes_read());
-  // Note that the offline bit is not cleared on purpose due to the fact that
-  // other flag, like request status, should already indicate that the offline
-  // page fails to load.
-  EXPECT_TRUE(is_offline_page_set_in_navigation_data());
+  EXPECT_FALSE(is_offline_page_set_in_navigation_data());
+  EXPECT_FALSE(offline_page_tab_helper()->GetOfflinePageForTest());
+}
+
+TEST_F(OfflinePageRequestHandlerTest, FailOnIntentFileMismatchForInternalPage) {
+  SimulateHasNetworkConnectivity(true);
+
+  std::string expected_data(MakeContentOfSize(2 * 1024));
+  ArchiveValidator archive_validator;
+  archive_validator.Update(expected_data);
+  std::string expected_digest = archive_validator.Finish();
+  int expected_size = expected_data.length();
+
+  // Save an offline page whose archive lives in the internal directory.
+  const GURL test_url(kTestUrl);
+  int64_t offline_id =
+      SaveInternalPage(test_url, GURL(), base::FilePath(kFilename1),
+                       expected_size, expected_digest);
+
+  // Create a file with unrelated content. The path to this file will be fed
+  // into "intent_url" of extra headers.
+  std::string unrelated_data(MakeContentOfSize(3 * 1024));
+  base::FilePath unrelated_file_path = CreateFileWithContent(unrelated_data);
+
+  // Load an URL with custom header that contains "intent_url" pointing to the
+  // unrelated file. Expect the request fails without serving any data.
+  net::HttpRequestHeaders extra_headers;
+  extra_headers.AddHeaderFromString(UseOfflinePageHeaderForIntent(
+      OfflinePageHeader::Reason::FILE_URL_INTENT, offline_id,
+      net::FilePathToFileURL(unrelated_file_path)));
+  LoadPageWithHeaders(test_url, extra_headers);
+
+  EXPECT_EQ(net::ERR_FAILED, request_status());
+  EXPECT_NE("multipart/related", mime_type());
+  EXPECT_EQ(0, bytes_read());
+  EXPECT_FALSE(is_offline_page_set_in_navigation_data());
   EXPECT_FALSE(offline_page_tab_helper()->GetOfflinePageForTest());
 }
 

diff --git a/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc b/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc
index 2f2f0041..67237500 100644
--- a/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc
+++ b/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc
@@ -761,10 +761,6 @@
 }
 
 void OfflinePageURLLoaderBuilder::OnComplete() {
-  if (client_->completion_status().error_code != net::OK) {
-    mime_type_.clear();
-    body_.clear();
-  }
   ReadCompleted(
       ResponseInfo(client_->completion_status().error_code, mime_type_, body_));
   // Clear intermediate data in preparation for next potential page loading.
@@ -1366,10 +1362,7 @@
   EXPECT_EQ(net::ERR_FAILED, request_status());
   EXPECT_NE("multipart/related", mime_type());
   EXPECT_EQ(0, bytes_read());
-  // Note that the offline bit is not cleared on purpose due to the fact that
-  // other flag, like request status, should already indicate that the offline
-  // page fails to load.
-  EXPECT_TRUE(is_offline_page_set_in_navigation_data());
+  EXPECT_FALSE(is_offline_page_set_in_navigation_data());
   EXPECT_FALSE(offline_page_tab_helper()->GetOfflinePageForTest());
 }
 
@@ -1404,10 +1397,42 @@
   EXPECT_EQ(net::ERR_FAILED, request_status());
   EXPECT_NE("multipart/related", mime_type());
   EXPECT_EQ(0, bytes_read());
-  // Note that the offline bit is not cleared on purpose due to the fact that
-  // other flag, like request status, should already indicate that the offline
-  // page fails to load.
-  EXPECT_TRUE(is_offline_page_set_in_navigation_data());
+  EXPECT_FALSE(is_offline_page_set_in_navigation_data());
+  EXPECT_FALSE(offline_page_tab_helper()->GetOfflinePageForTest());
+}
+
+TEST_F(OfflinePageRequestHandlerTest, FailOnIntentFileMismatchForInternalPage) {
+  SimulateHasNetworkConnectivity(true);
+
+  std::string expected_data(MakeContentOfSize(2 * 1024));
+  ArchiveValidator archive_validator;
+  archive_validator.Update(expected_data);
+  std::string expected_digest = archive_validator.Finish();
+  int expected_size = expected_data.length();
+
+  // Save an offline page whose archive lives in the internal directory.
+  const GURL test_url(kTestUrl);
+  int64_t offline_id =
+      SaveInternalPage(test_url, GURL(), base::FilePath(kFilename1),
+                       expected_size, expected_digest);
+
+  // Create a file with unrelated content. The path to this file will be fed
+  // into "intent_url" of extra headers.
+  std::string unrelated_data(MakeContentOfSize(3 * 1024));
+  base::FilePath unrelated_file_path = CreateFileWithContent(unrelated_data);
+
+  // Load an URL with custom header that contains "intent_url" pointing to the
+  // unrelated file. Expect the request fails without serving any data.
+  net::HttpRequestHeaders extra_headers;
+  extra_headers.AddHeaderFromString(UseOfflinePageHeaderForIntent(
+      OfflinePageHeader::Reason::FILE_URL_INTENT, offline_id,
+      net::FilePathToFileURL(unrelated_file_path)));
+  LoadPageWithHeaders(test_url, extra_headers);
+
+  EXPECT_EQ(net::ERR_FAILED, request_status());
+  EXPECT_NE("multipart/related", mime_type());
+  EXPECT_EQ(0, bytes_read());
+  EXPECT_FALSE(is_offline_page_set_in_navigation_data());
   EXPECT_FALSE(offline_page_tab_helper()->GetOfflinePageForTest());
 }