From e50070440287e4cf364ef349843f3e3abe823657 Mon Sep 17 00:00:00 2001 From: Yao Xiao Date: Fri, 26 Feb 2021 02:32:09 +0000 Subject: [PATCH] Use the navigation IsolationInfo for downloading an opened PDF What: When downloading a PDF that is already opened, use the original navigation isolation info. Why: When a subframe PDF is opened and then is manually downloaded, the network isolation key for the 2 requests are inconsistent. The navigation request is using , whereas the download request is using the . This not only has the typical performance & privacy problem, but also could give the user an unexpected cached PDF instead of the latest PDF opened -- if there are 2 PDFs opened with the same URL but different contents, and we try to save the latest one. (Note that the fundamental issue is not solvable by fixing the isolation info per https://bugs.chromium.org/p/chromium/issues/detail?id=1171524#c20, but making it download the latest opened PDF is still more desirable than making it download an old one). How: Add a |rfh| parameter in WebContents::SaveFrame(WithHeaders), to denote the original navigation targeted frame, so that we can recover the origin navigation isolation info from there. Fill in this info in the caller sites: 1. PDFWebContentsHelper::SaveUrlAs 2. RenderViewContextMenu::ExecSaveAs 3. WebContentsImpl::OnSavePage Specifically, if we have an inner-web-contents PDF, we will pass its outer-web-contents-frame; otherwise, pass the current frame host. In download_utils.cc::CreateResourceRequest, if the isolation_info is already filled in, use the given isolation info & site-for-cookies; otherwise, keep the existing behavior i.e. use as the isolation key. Bug: 1174216 Change-Id: Ibba85358a4da9efedaa5510b70f7279412685d56 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2689861 Reviewed-by: Nasko Oskov Reviewed-by: Istiaque Ahmed Reviewed-by: K. Moon Reviewed-by: Matt Menke Reviewed-by: Min Qin Commit-Queue: Yao Xiao Cr-Commit-Position: refs/heads/master@{#857983} --- .../browser/download/download_browsertest.cc | 361 ++++++++++++++++++ .../render_view_context_menu.cc | 11 +- .../internal/common/download_utils.cc | 23 +- components/download/public/common/DEPS | 1 + .../public/common/download_url_parameters.h | 13 + .../pdf/browser/pdf_web_contents_helper.cc | 6 +- .../browser/web_contents/web_contents_impl.cc | 19 +- .../browser/web_contents/web_contents_impl.h | 7 +- content/public/browser/web_contents.h | 21 +- .../public/browser/web_contents_delegate.cc | 4 +- .../public/browser/web_contents_delegate.h | 4 +- content/test/test_web_contents.cc | 3 +- content/test/test_web_contents.h | 3 +- .../mime_handler_view_guest.cc | 5 +- .../mime_handler_view_guest.h | 4 +- 15 files changed, 451 insertions(+), 34 deletions(-) diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc index 4e9826f4e11e3b..e3d2dfbf5150c8 100644 --- a/chrome/browser/download/download_browsertest.cc +++ b/chrome/browser/download/download_browsertest.cc @@ -93,6 +93,7 @@ #include "components/infobars/core/confirm_infobar_delegate.h" #include "components/infobars/core/infobar.h" #include "components/metrics/content/subprocess_metrics_provider.h" +#include "components/pdf/browser/pdf_web_contents_helper.h" #include "components/permissions/permission_request_manager.h" #include "components/prefs/pref_service.h" #include "components/reputation/core/safety_tip_test_utils.h" @@ -132,6 +133,7 @@ #include "extensions/browser/extension_system.h" #include "extensions/browser/scoped_ignore_content_verifier_for_test.h" #include "mojo/public/cpp/bindings/remote.h" +#include "net/base/features.h" #include "net/base/filename_util.h" #include "net/dns/mock_host_resolver.h" #include "net/test/embedded_test_server/embedded_test_server.h" @@ -170,6 +172,24 @@ using net::test_server::EmbeddedTestServer; namespace { +class InnerWebContentsAttachedWaiter : public content::WebContentsObserver { + public: + // Observes navigation for the specified |web_contents|. + explicit InnerWebContentsAttachedWaiter(WebContents* web_contents) + : content::WebContentsObserver(web_contents) {} + + void InnerWebContentsAttached(WebContents* inner_web_contents, + content::RenderFrameHost* render_frame_host, + bool is_full_page) override { + run_loop_.Quit(); + } + + void Wait() { run_loop_.Run(); } + + private: + base::RunLoop run_loop_{base::RunLoop::Type::kNestableTasksAllowed}; +}; + const char kDownloadTest1Path[] = "download-test1.lib"; void VerifyNewDownloadId(uint32_t expected_download_id, uint32_t download_id) { @@ -2621,6 +2641,347 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, TransientDownload) { ASSERT_FALSE(downloads[0]->IsTemporary()); } +class DownloadTestSplitCacheEnabled : public DownloadTest { + public: + DownloadTestSplitCacheEnabled() { + feature_list_.InitWithFeatures( + {net::features::kSplitCacheByNetworkIsolationKey}, {}); + } + + private: + base::test::ScopedFeatureList feature_list_; +}; + +IN_PROC_BROWSER_TEST_F(DownloadTestSplitCacheEnabled, + SaveMainFramePdfFromContextMenu_IsolationInfo) { + embedded_test_server()->ServeFilesFromDirectory(GetTestDataDirectory()); + ASSERT_TRUE(embedded_test_server()->Start()); + EnableFileChooser(true); + + net::SiteForCookies expected_site_for_cookies = + net::SiteForCookies::FromOrigin( + url::Origin::Create(embedded_test_server()->GetURL("foo.com", "/"))); + + net::IsolationInfo expected_isolation_info = net::IsolationInfo::Create( + net::IsolationInfo::RequestType::kMainFrame, + url::Origin::Create(embedded_test_server()->GetURL("foo.com", "/")), + url::Origin::Create(embedded_test_server()->GetURL("foo.com", "/")), + expected_site_for_cookies, std::set()); + + content::WebContents* web_contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + // Set up a PDF page. + GURL url = embedded_test_server()->GetURL("foo.com", "/pdf/test.pdf"); + ui_test_utils::NavigateToURL(browser(), url); + + std::vector inner_web_contents_vector = + web_contents->GetInnerWebContents(); + ASSERT_EQ(1u, inner_web_contents_vector.size()); + content::WebContents* inner_web_contents = inner_web_contents_vector.front(); + + // Wait for the page to finish loading. + if (inner_web_contents->IsLoading()) { + content::TestNavigationObserver inner_navigation_waiter(inner_web_contents); + inner_navigation_waiter.Wait(); + ASSERT_TRUE(!inner_web_contents->IsLoading()); + } + + // Stop the server. This makes sure we really are pulling from the cache for + // the download request. + ASSERT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete()); + + base::Optional trusted_params; + net::SiteForCookies site_for_cookies; + + base::RunLoop request_waiter; + URLLoaderInterceptor request_listener(base::BindLambdaForTesting( + [&](URLLoaderInterceptor::RequestParams* params) { + if (params->url_request.url == url) { + trusted_params = params->url_request.trusted_params; + site_for_cookies = params->url_request.site_for_cookies; + request_waiter.Quit(); + } + return false; + })); + + std::unique_ptr download_waiter( + CreateWaiter(browser(), 1)); + + // Simulate saving the PDF from the context menu "Save As...". + content::ContextMenuParams context_menu_params; + context_menu_params.media_type = + blink::mojom::ContextMenuDataMediaType::kPlugin; + context_menu_params.src_url = url; + context_menu_params.page_url = inner_web_contents->GetLastCommittedURL(); + TestRenderViewContextMenu menu(inner_web_contents->GetMainFrame(), + context_menu_params); + menu.Init(); + menu.ExecuteCommand(IDC_SAVE_PAGE, 0); + + request_waiter.Run(); + + EXPECT_TRUE(trusted_params.has_value()); + EXPECT_TRUE(trusted_params->isolation_info.IsEqualForTesting( + expected_isolation_info)); + EXPECT_TRUE(site_for_cookies.IsEquivalent(expected_site_for_cookies)); + + download_waiter->WaitForFinished(); + + EXPECT_EQ(1u, + download_waiter->NumDownloadsSeenInState(DownloadItem::COMPLETE)); + CheckDownloadStates(1, DownloadItem::COMPLETE); +} + +IN_PROC_BROWSER_TEST_F(DownloadTestSplitCacheEnabled, + SaveSubframePdfFromPdfUI_IsolationInfo) { + embedded_test_server()->ServeFilesFromDirectory(GetTestDataDirectory()); + ASSERT_TRUE(embedded_test_server()->Start()); + EnableFileChooser(true); + + net::SiteForCookies expected_site_for_cookies = + net::SiteForCookies::FromOrigin( + url::Origin::Create(embedded_test_server()->GetURL("foo.com", "/"))); + + net::IsolationInfo expected_isolation_info = net::IsolationInfo::Create( + net::IsolationInfo::RequestType::kSubFrame, + url::Origin::Create(embedded_test_server()->GetURL("foo.com", "/")), + url::Origin::Create(embedded_test_server()->GetURL("bar.com", "/")), + expected_site_for_cookies, std::set()); + + content::WebContents* web_contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + // Set up a page with a cross-origin iframe hosting a PDF. + GURL url = embedded_test_server()->GetURL("foo.com", "/iframe.html"); + ui_test_utils::NavigateToURL(browser(), url); + + GURL subframe_url(embedded_test_server()->GetURL("bar.com", "/pdf/test.pdf")); + + InnerWebContentsAttachedWaiter waiter(web_contents); + content::BeginNavigateIframeToURL(web_contents, + /*iframe_id=*/"test", subframe_url); + waiter.Wait(); + + std::vector inner_web_contents_vector = + web_contents->GetInnerWebContents(); + ASSERT_EQ(1u, inner_web_contents_vector.size()); + content::WebContents* inner_web_contents = inner_web_contents_vector.front(); + + // Wait for the page to finish loading. + if (inner_web_contents->IsLoading()) { + content::TestNavigationObserver inner_navigation_waiter(inner_web_contents); + inner_navigation_waiter.Wait(); + ASSERT_TRUE(!inner_web_contents->IsLoading()); + } + + // Stop the server. This makes sure we really are pulling from the cache for + // the download request. + ASSERT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete()); + + base::Optional trusted_params; + net::SiteForCookies site_for_cookies; + + base::RunLoop request_waiter; + URLLoaderInterceptor request_listener(base::BindLambdaForTesting( + [&](URLLoaderInterceptor::RequestParams* params) { + if (params->url_request.url == subframe_url) { + trusted_params = params->url_request.trusted_params; + site_for_cookies = params->url_request.site_for_cookies; + request_waiter.Quit(); + } + return false; + })); + + std::unique_ptr download_waiter( + CreateWaiter(browser(), 1)); + + // Simulate saving the PDF from the UI. + pdf::PDFWebContentsHelper* pdf_helper = + pdf::PDFWebContentsHelper::FromWebContents(inner_web_contents); + blink::mojom::ReferrerPtr referrer = blink::mojom::Referrer::New(); + referrer->url = subframe_url; + referrer->policy = + network::mojom::ReferrerPolicy::kStrictOriginWhenCrossOrigin; + static_cast(pdf_helper) + ->SaveUrlAs(subframe_url, std::move(referrer)); + + request_waiter.Run(); + + EXPECT_TRUE(trusted_params.has_value()); + EXPECT_TRUE(trusted_params->isolation_info.IsEqualForTesting( + expected_isolation_info)); + EXPECT_TRUE(site_for_cookies.IsEquivalent(expected_site_for_cookies)); + + download_waiter->WaitForFinished(); + EXPECT_EQ(1u, + download_waiter->NumDownloadsSeenInState(DownloadItem::COMPLETE)); + CheckDownloadStates(1, DownloadItem::COMPLETE); +} + +IN_PROC_BROWSER_TEST_F(DownloadTestSplitCacheEnabled, + SaveSubframeImageFromContextMenu_IsolationInfo) { + embedded_test_server()->ServeFilesFromDirectory(GetTestDataDirectory()); + ASSERT_TRUE(embedded_test_server()->Start()); + EnableFileChooser(true); + + net::SiteForCookies expected_site_for_cookies = + net::SiteForCookies::FromOrigin( + url::Origin::Create(embedded_test_server()->GetURL("foo.com", "/"))); + + net::IsolationInfo expected_isolation_info = net::IsolationInfo::Create( + net::IsolationInfo::RequestType::kSubFrame, + url::Origin::Create(embedded_test_server()->GetURL("foo.com", "/")), + url::Origin::Create(embedded_test_server()->GetURL("bar.com", "/")), + expected_site_for_cookies, std::set()); + + content::WebContents* web_contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + // Set up a page with a cross-origin iframe hosting a PDF. + GURL url = embedded_test_server()->GetURL("foo.com", "/iframe.html"); + ui_test_utils::NavigateToURL(browser(), url); + + GURL subframe_url( + embedded_test_server()->GetURL("bar.com", "/downloads/image.jpg")); + content::NavigateIframeToURL(web_contents, + /*iframe_id=*/"test", subframe_url); + + // Stop the server. This makes sure we really are pulling from the cache for + // the download request. + ASSERT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete()); + + base::Optional trusted_params; + net::SiteForCookies site_for_cookies; + + base::RunLoop request_waiter; + URLLoaderInterceptor request_listener(base::BindLambdaForTesting( + [&](URLLoaderInterceptor::RequestParams* params) { + if (params->url_request.url == subframe_url) { + trusted_params = params->url_request.trusted_params; + site_for_cookies = params->url_request.site_for_cookies; + request_waiter.Quit(); + } + return false; + })); + + std::unique_ptr download_waiter( + CreateWaiter(browser(), 1)); + + // Simulate saving the image from the context menu "Save As..." + content::ContextMenuParams context_menu_params; + context_menu_params.media_type = + blink::mojom::ContextMenuDataMediaType::kImage; + context_menu_params.src_url = subframe_url; + context_menu_params.page_url = + content::ChildFrameAt(web_contents->GetMainFrame(), 0) + ->GetLastCommittedURL(); + TestRenderViewContextMenu menu( + content::ChildFrameAt(web_contents->GetMainFrame(), 0), + context_menu_params); + menu.Init(); + menu.ExecuteCommand(IDC_CONTENT_CONTEXT_SAVEIMAGEAS, 0); + + request_waiter.Run(); + + EXPECT_TRUE(trusted_params.has_value()); + EXPECT_TRUE(trusted_params->isolation_info.IsEqualForTesting( + expected_isolation_info)); + EXPECT_TRUE(site_for_cookies.IsEquivalent(expected_site_for_cookies)); + + download_waiter->WaitForFinished(); + + EXPECT_EQ(1u, + download_waiter->NumDownloadsSeenInState(DownloadItem::COMPLETE)); +} + +IN_PROC_BROWSER_TEST_F(DownloadTestSplitCacheEnabled, + SaveSubframePdfFromContextMenu_IsolationInfo) { + embedded_test_server()->ServeFilesFromDirectory(GetTestDataDirectory()); + ASSERT_TRUE(embedded_test_server()->Start()); + EnableFileChooser(true); + + net::SiteForCookies expected_site_for_cookies = + net::SiteForCookies::FromOrigin( + url::Origin::Create(embedded_test_server()->GetURL("foo.com", "/"))); + + net::IsolationInfo expected_isolation_info = net::IsolationInfo::Create( + net::IsolationInfo::RequestType::kSubFrame, + url::Origin::Create(embedded_test_server()->GetURL("foo.com", "/")), + url::Origin::Create(embedded_test_server()->GetURL("bar.com", "/")), + expected_site_for_cookies, std::set()); + + content::WebContents* web_contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + // Set up a page with a cross-origin iframe hosting a PDF. + GURL url = embedded_test_server()->GetURL("foo.com", "/iframe.html"); + ui_test_utils::NavigateToURL(browser(), url); + + GURL subframe_url(embedded_test_server()->GetURL("bar.com", "/pdf/test.pdf")); + + InnerWebContentsAttachedWaiter waiter(web_contents); + content::BeginNavigateIframeToURL(web_contents, + /*iframe_id=*/"test", subframe_url); + waiter.Wait(); + + std::vector inner_web_contents_vector = + web_contents->GetInnerWebContents(); + ASSERT_EQ(1u, inner_web_contents_vector.size()); + content::WebContents* inner_web_contents = inner_web_contents_vector.front(); + + // Wait for the page to finish loading. + if (inner_web_contents->IsLoading()) { + content::TestNavigationObserver inner_navigation_waiter(inner_web_contents); + inner_navigation_waiter.Wait(); + ASSERT_TRUE(!inner_web_contents->IsLoading()); + } + + // Stop the server. This makes sure we really are pulling from the cache for + // the download request. + ASSERT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete()); + + base::Optional trusted_params; + net::SiteForCookies site_for_cookies; + + base::RunLoop request_waiter; + URLLoaderInterceptor request_listener(base::BindLambdaForTesting( + [&](URLLoaderInterceptor::RequestParams* params) { + if (params->url_request.url == subframe_url) { + trusted_params = params->url_request.trusted_params; + site_for_cookies = params->url_request.site_for_cookies; + request_waiter.Quit(); + } + return false; + })); + + std::unique_ptr download_waiter( + CreateWaiter(browser(), 1)); + + // Simulate saving the PDF from the context menu "Save As..." + content::ContextMenuParams context_menu_params; + context_menu_params.media_type = + blink::mojom::ContextMenuDataMediaType::kPlugin; + context_menu_params.src_url = subframe_url; + context_menu_params.page_url = inner_web_contents->GetLastCommittedURL(); + TestRenderViewContextMenu menu(inner_web_contents->GetMainFrame(), + context_menu_params); + menu.Init(); + menu.ExecuteCommand(IDC_CONTENT_CONTEXT_SAVEAVAS, 0); + + request_waiter.Run(); + + EXPECT_TRUE(trusted_params.has_value()); + EXPECT_TRUE(trusted_params->isolation_info.IsEqualForTesting( + expected_isolation_info)); + EXPECT_TRUE(site_for_cookies.IsEquivalent(expected_site_for_cookies)); + + download_waiter->WaitForFinished(); + + EXPECT_EQ(1u, + download_waiter->NumDownloadsSeenInState(DownloadItem::COMPLETE)); +} + class DownloadTestWithHistogramTester : public DownloadTest { public: void SetUp() override { diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu.cc b/chrome/browser/renderer_context_menu/render_view_context_menu.cc index 86f785a8658f23..b88a6eabbfed44 100644 --- a/chrome/browser/renderer_context_menu/render_view_context_menu.cc +++ b/chrome/browser/renderer_context_menu/render_view_context_menu.cc @@ -2994,8 +2994,15 @@ void RenderViewContextMenu::ExecSaveAs() { const GURL& url = params_.src_url; content::Referrer referrer = CreateReferrer(url, params_); std::string headers; - source_web_contents_->SaveFrameWithHeaders(url, referrer, headers, - params_.suggested_filename); + + RenderFrameHost* frame_host = + (params_.media_type == ContextMenuDataMediaType::kPlugin) + ? source_web_contents_->GetOuterWebContentsFrame() + : GetRenderFrameHost(); + if (frame_host) { + source_web_contents_->SaveFrameWithHeaders( + url, referrer, headers, params_.suggested_filename, frame_host); + } } } diff --git a/components/download/internal/common/download_utils.cc b/components/download/internal/common/download_utils.cc index 2bdaf7d7a10891..6ad1043959eb6c 100644 --- a/components/download/internal/common/download_utils.cc +++ b/components/download/internal/common/download_utils.cc @@ -270,17 +270,22 @@ std::unique_ptr CreateResourceRequest( request->request_initiator = params->initiator(); request->trusted_params = network::ResourceRequest::TrustedParams(); - // Treat downloads like top-level frame navigations to be consistent with - // cookie behavior. Also, since web-initiated downloads bypass the disk cache, - // sites can't use download timing information to tell if a cross-site URL has - // been visited before. - url::Origin origin = url::Origin::Create(params->url()); - request->trusted_params->isolation_info = net::IsolationInfo::Create( - net::IsolationInfo::RequestType::kMainFrame, origin, origin, - net::SiteForCookies::FromOrigin(origin)); + if (params->isolation_info().has_value()) { + request->trusted_params->isolation_info = params->isolation_info().value(); + request->site_for_cookies = params->isolation_info()->site_for_cookies(); + } else { + // Treat downloads like top-level frame navigations to be consistent with + // cookie behavior. Also, since web-initiated downloads bypass the disk + // cache, sites can't use download timing information to tell if a + // cross-site URL has been visited before. + url::Origin origin = url::Origin::Create(params->url()); + request->trusted_params->isolation_info = net::IsolationInfo::Create( + net::IsolationInfo::RequestType::kMainFrame, origin, origin, + net::SiteForCookies::FromOrigin(origin)); + request->site_for_cookies = net::SiteForCookies::FromUrl(params->url()); + } request->do_not_prompt_for_login = params->do_not_prompt_for_login(); - request->site_for_cookies = net::SiteForCookies::FromUrl(params->url()); request->referrer = params->referrer(); request->referrer_policy = params->referrer_policy(); request->is_main_frame = true; diff --git a/components/download/public/common/DEPS b/components/download/public/common/DEPS index b6a658afc0c9fb..e9121f4bb0857d 100644 --- a/components/download/public/common/DEPS +++ b/components/download/public/common/DEPS @@ -8,6 +8,7 @@ include_rules = [ "+net/base/net_errors.h", "+net/base/network_change_notifier.h", "+net/base/network_isolation_key.h", + "+net/base/isolation_info.h", "+net/cert/cert_status_flags.h", "+net/http/http_response_headers.h", "+net/http/http_response_info.h", diff --git a/components/download/public/common/download_url_parameters.h b/components/download/public/common/download_url_parameters.h index b91ed4081406eb..d5ed92b8a8eea0 100644 --- a/components/download/public/common/download_url_parameters.h +++ b/components/download/public/common/download_url_parameters.h @@ -18,6 +18,7 @@ #include "components/download/public/common/download_interrupt_reasons.h" #include "components/download/public/common/download_save_info.h" #include "components/download/public/common/download_source.h" +#include "net/base/isolation_info.h" #include "net/traffic_annotation/network_traffic_annotation.h" #include "net/url_request/referrer_policy.h" #include "services/network/public/cpp/resource_request_body.h" @@ -251,6 +252,14 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadUrlParameters { require_safety_checks_ = require_safety_checks; } + // Sets whether the download request will use the given isolation_info. If the + // isolation info is not set, the download will be treated as a + // top-frame navigation with respect to network-isolation-key and + // site-for-cookies. + void set_isolation_info(const net::IsolationInfo& isolation_info) { + isolation_info_ = isolation_info; + } + OnStartedCallback& callback() { return callback_; } bool content_initiated() const { return content_initiated_; } const std::string& last_modified() const { return last_modified_; } @@ -296,6 +305,9 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadUrlParameters { bool is_transient() const { return transient_; } std::string guid() const { return guid_; } bool require_safety_checks() const { return require_safety_checks_; } + const base::Optional& isolation_info() const { + return isolation_info_; + } // STATE CHANGING: All save_info_ sub-objects will be in an indeterminate // state following this call. @@ -341,6 +353,7 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadUrlParameters { DownloadSource download_source_; UploadProgressCallback upload_callback_; bool require_safety_checks_; + base::Optional isolation_info_; DISALLOW_COPY_AND_ASSIGN(DownloadUrlParameters); }; diff --git a/components/pdf/browser/pdf_web_contents_helper.cc b/components/pdf/browser/pdf_web_contents_helper.cc index 51bb84e40c0d37..d8b5674c359445 100644 --- a/components/pdf/browser/pdf_web_contents_helper.cc +++ b/components/pdf/browser/pdf_web_contents_helper.cc @@ -260,7 +260,11 @@ void PDFWebContentsHelper::HasUnsupportedFeature() { void PDFWebContentsHelper::SaveUrlAs(const GURL& url, blink::mojom::ReferrerPtr referrer) { client_->OnSaveURL(web_contents()); - web_contents()->SaveFrame(url, referrer.To()); + + if (content::RenderFrameHost* rfh = + web_contents()->GetOuterWebContentsFrame()) { + web_contents()->SaveFrame(url, referrer.To(), rfh); + } } void PDFWebContentsHelper::UpdateContentRestrictions( diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc index 588ad83114914f..7564e13733718b 100644 --- a/content/browser/web_contents/web_contents_impl.cc +++ b/content/browser/web_contents/web_contents_impl.cc @@ -4544,7 +4544,7 @@ void WebContentsImpl::OnSavePage() { if (!IsSavable()) { download::RecordSavePackageEvent( download::SAVE_PACKAGE_DOWNLOAD_ON_NON_HTML); - SaveFrame(GetLastCommittedURL(), Referrer()); + SaveFrame(GetLastCommittedURL(), Referrer(), GetMainFrame()); return; } @@ -4573,16 +4573,21 @@ bool WebContentsImpl::SavePage(const base::FilePath& main_file, return save_package_->Init(SavePackageDownloadCreatedCallback()); } -void WebContentsImpl::SaveFrame(const GURL& url, const Referrer& referrer) { +void WebContentsImpl::SaveFrame(const GURL& url, + const Referrer& referrer, + RenderFrameHost* rfh) { OPTIONAL_TRACE_EVENT0("content", "WebContentsImpl::SaveFrame"); - SaveFrameWithHeaders(url, referrer, std::string(), base::string16()); + SaveFrameWithHeaders(url, referrer, std::string(), base::string16(), rfh); } void WebContentsImpl::SaveFrameWithHeaders( const GURL& url, const Referrer& referrer, const std::string& headers, - const base::string16& suggested_filename) { + const base::string16& suggested_filename, + RenderFrameHost* rfh) { + DCHECK(rfh); + OPTIONAL_TRACE_EVENT2("content", "WebContentsImpl::SaveFrameWithHeaders", "url", base::trace_event::ValueToString(url), "headers", headers); @@ -4603,7 +4608,7 @@ void WebContentsImpl::SaveFrameWithHeaders( if (!GetLastCommittedURL().is_valid()) return; - if (delegate_ && delegate_->SaveFrame(url, referrer)) + if (delegate_ && delegate_->SaveFrame(url, referrer, rfh)) return; // TODO(nasko): This check for main frame is incorrect and should be fixed @@ -4660,6 +4665,10 @@ void WebContentsImpl::SaveFrameWithHeaders( } params->set_suggested_name(suggested_filename); params->set_download_source(download::DownloadSource::WEB_CONTENTS_API); + params->set_isolation_info( + static_cast(rfh)->ComputeIsolationInfoForNavigation( + url)); + BrowserContext::GetDownloadManager(GetBrowserContext()) ->DownloadUrl(std::move(params)); } diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h index 41ac881ee3a097..a9ae742573802c 100644 --- a/content/browser/web_contents/web_contents_impl.h +++ b/content/browser/web_contents/web_contents_impl.h @@ -469,11 +469,14 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents, bool SavePage(const base::FilePath& main_file, const base::FilePath& dir_path, SavePageType save_type) override; - void SaveFrame(const GURL& url, const Referrer& referrer) override; + void SaveFrame(const GURL& url, + const Referrer& referrer, + RenderFrameHost* rfh) override; void SaveFrameWithHeaders(const GURL& url, const Referrer& referrer, const std::string& headers, - const base::string16& suggested_filename) override; + const base::string16& suggested_filename, + RenderFrameHost* rfh) override; void GenerateMHTML(const MHTMLGenerationParams& params, base::OnceCallback callback) override; void GenerateMHTMLWithResult( diff --git a/content/public/browser/web_contents.h b/content/public/browser/web_contents.h index f1bdd0c7bad809..f7c4564d7ab04a 100644 --- a/content/public/browser/web_contents.h +++ b/content/public/browser/web_contents.h @@ -832,19 +832,24 @@ class WebContents : public PageNavigator, const base::FilePath& dir_path, SavePageType save_type) = 0; - // Saves the given frame's URL to the local filesystem. + // Saves the given frame's URL to the local filesystem. If `rfh` is provided, + // the saving is performed in its context. For example, the associated + // navigation isolation info will be used for making the network request. virtual void SaveFrame(const GURL& url, - const Referrer& referrer) = 0; + const Referrer& referrer, + RenderFrameHost* rfh) = 0; // Saves the given frame's URL to the local filesystem. The headers, if // provided, is used to make a request to the URL rather than using cache. // Format of |headers| is a new line separated list of key value pairs: - // ": \r\n: ". - virtual void SaveFrameWithHeaders( - const GURL& url, - const Referrer& referrer, - const std::string& headers, - const base::string16& suggested_filename) = 0; + // ": \r\n: ". If `rfh` is provided, the saving is + // performed in its context. For example, the associated navigation isolation + // info will be used for making the network request. + virtual void SaveFrameWithHeaders(const GURL& url, + const Referrer& referrer, + const std::string& headers, + const base::string16& suggested_filename, + RenderFrameHost* rfh) = 0; // Generate an MHTML representation of the current page conforming to the // settings provided by |params| and returning final status information via diff --git a/content/public/browser/web_contents_delegate.cc b/content/public/browser/web_contents_delegate.cc index ec7c2e25a02d94..cc9bb46ef5d1a7 100644 --- a/content/public/browser/web_contents_delegate.cc +++ b/content/public/browser/web_contents_delegate.cc @@ -274,7 +274,9 @@ bool WebContentsDelegate::GuestSaveFrame(WebContents* guest_web_contents) { return false; } -bool WebContentsDelegate::SaveFrame(const GURL& url, const Referrer& referrer) { +bool WebContentsDelegate::SaveFrame(const GURL& url, + const Referrer& referrer, + content::RenderFrameHost* rfh) { return false; } diff --git a/content/public/browser/web_contents_delegate.h b/content/public/browser/web_contents_delegate.h index 9075cda6487fb4..e752b5bd6893bc 100644 --- a/content/public/browser/web_contents_delegate.h +++ b/content/public/browser/web_contents_delegate.h @@ -575,7 +575,9 @@ class CONTENT_EXPORT WebContentsDelegate { // Called in response to a request to save a frame. If this returns true, the // default behavior is suppressed. - virtual bool SaveFrame(const GURL& url, const Referrer& referrer); + virtual bool SaveFrame(const GURL& url, + const Referrer& referrer, + content::RenderFrameHost* rfh); // Can be overridden by a delegate to return the security style of the // given |web_contents|, populating |security_style_explanations| to diff --git a/content/test/test_web_contents.cc b/content/test/test_web_contents.cc index 24598de0192346..36474dac75ba94 100644 --- a/content/test/test_web_contents.cc +++ b/content/test/test_web_contents.cc @@ -351,7 +351,8 @@ void TestWebContents::SaveFrameWithHeaders( const GURL& url, const Referrer& referrer, const std::string& headers, - const base::string16& suggested_filename) { + const base::string16& suggested_filename, + RenderFrameHost* rfh) { save_frame_headers_ = headers; suggested_filename_ = suggested_filename; } diff --git a/content/test/test_web_contents.h b/content/test/test_web_contents.h index f09b5fa6871ee5..a6f1ead879fc16 100644 --- a/content/test/test_web_contents.h +++ b/content/test/test_web_contents.h @@ -174,7 +174,8 @@ class TestWebContents : public WebContentsImpl, public WebContentsTester { void SaveFrameWithHeaders(const GURL& url, const Referrer& referrer, const std::string& headers, - const base::string16& suggested_filename) override; + const base::string16& suggested_filename, + RenderFrameHost* rfh) override; void ReattachToOuterWebContentsFrame() override {} void SetPageFrozen(bool frozen) override; diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc index 8f4c53f3f3cae0..e910810ff0de76 100644 --- a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc +++ b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc @@ -338,11 +338,12 @@ bool MimeHandlerViewGuest::GuestSaveFrame( } bool MimeHandlerViewGuest::SaveFrame(const GURL& url, - const content::Referrer& referrer) { + const content::Referrer& referrer, + content::RenderFrameHost* rfh) { if (!attached()) return false; - embedder_web_contents()->SaveFrame(stream_->original_url(), referrer); + embedder_web_contents()->SaveFrame(stream_->original_url(), referrer, rfh); return true; } diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h index d9e22ce31927da..bdba06b3d493a3 100644 --- a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h +++ b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h @@ -143,7 +143,9 @@ class MimeHandlerViewGuest content::JavaScriptDialogManager* GetJavaScriptDialogManager( content::WebContents* source) final; bool GuestSaveFrame(content::WebContents* guest_web_contents) final; - bool SaveFrame(const GURL& url, const content::Referrer& referrer) final; + bool SaveFrame(const GURL& url, + const content::Referrer& referrer, + content::RenderFrameHost* rfh) final; void OnRenderFrameHostDeleted(int process_id, int routing_id) final; void EnterFullscreenModeForTab( content::RenderFrameHost* requesting_frame,