Fix download of MHTML document.
Trivial fix and regressions tests. To be cherry-picked into M89 beta.
A page mustn't mix MHTML documents with regular documents. It would be
a security issue. The browser process is in charge of blocking those.
When moving the check outside of blink, a regressions has been
introduced: navigation that produce a download shouldn't be blocked.
R=antoniosartori@chromium.org
CC=ericlaw@microsoft.com,dcheng@chromium.org
(cherry picked from commit 00f90c402bb81c16be5bd825ec11783e8b51731e)
Bug: 1171765,1170484
Change-Id: I090076743b9ef8077a74bef7b2a75b54da8af307
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2659061
Reviewed-by: Eric Lawrence [MSFT] <ericlaw@microsoft.com>
Reviewed-by: Antonio Sartori <antoniosartori@chromium.org>
Commit-Queue: Eric Lawrence [MSFT] <ericlaw@microsoft.com>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#848740}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2661415
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/4324@{#2075}
Cr-Branched-From: c73b5a651d37a6c4d0b8e3262cc4015a5579c6c8-refs/heads/master@{#827102}
diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc
index a673d5c2..10ed255 100644
--- a/content/browser/download/download_browsertest.cc
+++ b/content/browser/download/download_browsertest.cc
@@ -67,6 +67,7 @@
#include "content/test/test_content_browser_client.h"
#include "net/base/filename_util.h"
#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/controllable_http_response.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"
@@ -4693,27 +4694,19 @@
// Test fixture for loading MHTML.
class MhtmlLoadingTest : public DownloadContentTest {
protected:
- void SetUpOnMainThread() override {
- DownloadContentTest::SetUpOnMainThread();
-
- // Allows loading the MHTML, instead of downloading it.
- new_client_.set_allowed_rendering_mhtml_over_http(true);
- old_client_ = SetBrowserClientForTesting(&new_client_);
+ // Return an URL for loading a local test file.
+ GURL GetFileURL(const base::FilePath::CharType* file_path) {
+ base::FilePath path;
+ CHECK(base::PathService::Get(base::DIR_SOURCE_ROOT, &path));
+ path = path.Append(GetTestDataFilePath());
+ path = path.Append(file_path);
+ return GURL(FILE_PATH_LITERAL("file:") + path.value());
}
-
- void TearDownOnMainThread() override {
- SetBrowserClientForTesting(old_client_);
- DownloadContentTest::TearDownOnMainThread();
- }
-
- private:
- DownloadTestContentBrowserClient new_client_;
- ContentBrowserClient* old_client_;
};
-IN_PROC_BROWSER_TEST_F(MhtmlLoadingTest, AllowRenderMultipartRelatedPage) {
- // .mhtml file is mapped to "multipart/related" by the test server.
- GURL url = embedded_test_server()->GetURL("/download/hello.mhtml");
+IN_PROC_BROWSER_TEST_F(MhtmlLoadingTest,
+ AllowRenderMultipartRelatedPageFromFile) {
+ GURL url = GetFileURL(FILE_PATH_LITERAL("download/hello.mhtml"));
auto observer = std::make_unique<content::TestNavigationObserver>(url);
observer->WatchExistingWebContents();
observer->StartWatchingNewWebContents();
@@ -4723,9 +4716,8 @@
observer->WaitForNavigationFinished();
}
-IN_PROC_BROWSER_TEST_F(MhtmlLoadingTest, AllowRenderMessageRfc822Page) {
- // .mht file is mapped to "message/rfc822" by the test server.
- GURL url = embedded_test_server()->GetURL("/download/test.mht");
+IN_PROC_BROWSER_TEST_F(MhtmlLoadingTest, AllowRenderMessageRfc822PageFromFile) {
+ GURL url = GetFileURL(FILE_PATH_LITERAL("download/test.mht"));
auto observer = std::make_unique<content::TestNavigationObserver>(url);
observer->WatchExistingWebContents();
observer->StartWatchingNewWebContents();
@@ -4735,4 +4727,72 @@
observer->WaitForNavigationFinished();
}
+IN_PROC_BROWSER_TEST_F(MhtmlLoadingTest,
+ DisallowRenderMultipartRelatedPageFromHTTP) {
+ net::EmbeddedTestServer server;
+ net::test_server::ControllableHttpResponse response(&server, "/");
+ EXPECT_TRUE(server.Start());
+ std::unique_ptr<DownloadTestObserver> observer(CreateWaiter(shell(), 1));
+
+ GURL url = server.GetURL(kOrigin, "/");
+
+ shell()->LoadURL(url);
+
+ response.WaitForRequest();
+ response.Send(net::HTTP_OK, "multipart/related");
+ response.Done();
+
+ observer->WaitForFinished();
+ EXPECT_EQ(
+ 1u, observer->NumDownloadsSeenInState(download::DownloadItem::COMPLETE));
+}
+
+IN_PROC_BROWSER_TEST_F(MhtmlLoadingTest,
+ DisallowRenderMessageRfc822PageFromHTTP) {
+ net::EmbeddedTestServer server;
+ net::test_server::ControllableHttpResponse response(&server, "/");
+ EXPECT_TRUE(server.Start());
+ std::unique_ptr<DownloadTestObserver> observer(CreateWaiter(shell(), 1));
+
+ GURL url = server.GetURL(kOrigin, "/");
+
+ shell()->LoadURL(url);
+
+ response.WaitForRequest();
+ response.Send(net::HTTP_OK, "message/rfc822");
+ response.Done();
+
+ observer->WaitForFinished();
+ EXPECT_EQ(
+ 1u, observer->NumDownloadsSeenInState(download::DownloadItem::COMPLETE));
+}
+
+// Regression test for https://crbug.com/1171765
+IN_PROC_BROWSER_TEST_F(MhtmlLoadingTest, DisallowRenderMessageRfc822Iframe) {
+ net::EmbeddedTestServer server;
+ net::test_server::ControllableHttpResponse main_response(&server, "/main");
+ net::test_server::ControllableHttpResponse sub_response(&server, "/sub");
+ EXPECT_TRUE(server.Start());
+
+ std::unique_ptr<DownloadTestObserver> observer(CreateWaiter(shell(), 1));
+
+ GURL main_url = server.GetURL(kOrigin, "/main");
+ GURL sub_url = server.GetURL(kOrigin, "/sub");
+
+ shell()->LoadURL(main_url);
+
+ main_response.WaitForRequest();
+ main_response.Send(net::HTTP_OK, "text/html",
+ "<iframe src='./sub'></iframe>");
+ main_response.Done();
+
+ sub_response.WaitForRequest();
+ sub_response.Send(net::HTTP_OK, "message/rfc822");
+ sub_response.Done();
+
+ observer->WaitForFinished();
+ EXPECT_EQ(
+ 1u, observer->NumDownloadsSeenInState(download::DownloadItem::COMPLETE));
+}
+
} // namespace content
diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
index 02896ba..b469e32 100644
--- a/content/browser/renderer_host/navigation_request.cc
+++ b/content/browser/renderer_host/navigation_request.cc
@@ -2267,7 +2267,7 @@
// MHTML document can't be framed into non-MHTML document (and vice versa).
// The full page must load from the MHTML archive or none of it.
- if (is_mhtml_archive && !IsInMainFrame()) {
+ if (is_mhtml_archive && !IsInMainFrame() && response_should_be_rendered_) {
OnRequestFailedInternal(
network::URLLoaderCompletionStatus(net::ERR_BLOCKED_BY_RESPONSE),
false /* skip_throttles */, base::nullopt /* error_page_contnet */,
@@ -2285,6 +2285,8 @@
response_head_.get(), url::Origin::Create(common_params_->url),
common_params_->url, common_params_->referrer->url);
if (coop_requires_blocking) {
+ // TODO(https://crbug.com/1172169): Investigate what must be done in case of
+ // a download.
OnRequestFailedInternal(
network::URLLoaderCompletionStatus(*coop_requires_blocking),
false /* skip_throttles */, base::nullopt /* error_page_content */,
@@ -2297,6 +2299,8 @@
const base::Optional<network::mojom::BlockedByResponseReason>
coep_requires_blocking = EnforceCOEP();
if (coep_requires_blocking) {
+ // TODO(https://crbug.com/1172169): Investigate what must be done in case of
+ // a download.
OnRequestFailedInternal(
network::URLLoaderCompletionStatus(*coep_requires_blocking),
false /* skip_throttles */, base::nullopt /* error_page_content */,
@@ -2349,6 +2353,8 @@
coep_reporter->QueueNavigationReport(redirect_chain_[0],
/*report_only=*/false);
}
+ // TODO(https://crbug.com/1172169): Investigate what must be done in
+ // case of a download.
OnRequestFailedInternal(network::URLLoaderCompletionStatus(
network::mojom::BlockedByResponseReason::
kCoepFrameResourceNeedsCoepHeader),