CrossSiteResourceHandler: cancel request if there's no RFH to do
a policy check against.
Also included is a new test which demonstrates the issue. This was based
on CrossSiteTransferTest, which I did some cleanup on.
BUG=538784,268640
TEST=content_browsertests, with and without --enable-browser-side-navigation
Review URL: https://codereview.chromium.org/1384113002
Cr-Commit-Position: refs/heads/master@{#352413}
diff --git a/content/browser/cross_site_transfer_browsertest.cc b/content/browser/cross_site_transfer_browsertest.cc
index 0ce755c..c9d58161 100644
--- a/content/browser/cross_site_transfer_browsertest.cc
+++ b/content/browser/cross_site_transfer_browsertest.cc
@@ -64,11 +64,10 @@
BrowserThread::IO, FROM_HERE,
base::Bind(
&TrackingResourceDispatcherHostDelegate::SetTrackedURLOnIOThread,
- base::Unretained(this),
- tracked_url));
+ base::Unretained(this), tracked_url, run_loop_->QuitClosure()));
}
- // Waits until the tracked URL has been requests, and the request for it has
+ // Waits until the tracked URL has been requested, and the request for it has
// been destroyed.
bool WaitForTrackedURLAndGetCompleted() {
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -107,10 +106,12 @@
DISALLOW_COPY_AND_ASSIGN(TrackingThrottle);
};
- void SetTrackedURLOnIOThread(const GURL& tracked_url) {
+ void SetTrackedURLOnIOThread(const GURL& tracked_url,
+ const base::Closure& run_loop_quit_closure) {
CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
throttle_created_ = false;
tracked_url_ = tracked_url;
+ run_loop_quit_closure_ = run_loop_quit_closure;
}
void OnTrackedRequestDestroyed(bool completed) {
@@ -118,20 +119,20 @@
tracked_request_completed_ = completed;
tracked_url_ = GURL();
- BrowserThread::PostTask(
- BrowserThread::UI, FROM_HERE, run_loop_->QuitClosure());
+ BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+ run_loop_quit_closure_);
}
// These live on the IO thread.
GURL tracked_url_;
bool throttle_created_;
+ base::Closure run_loop_quit_closure_;
- // This is created and destroyed on the UI thread, but stopped on the IO
- // thread.
+ // This lives on the UI thread.
scoped_ptr<base::RunLoop> run_loop_;
- // Set on the IO thread while |run_loop_| is non-NULL, read on the UI thread
- // after deleting run_loop_.
+ // Set on the IO thread while |run_loop_| is non-nullptr, read on the UI
+ // thread after deleting run_loop_.
bool tracked_request_completed_;
DISALLOW_COPY_AND_ASSIGN(TrackingResourceDispatcherHostDelegate);
@@ -148,7 +149,7 @@
bool is_transfer =
(params.transferred_global_request_id != GlobalRequestID());
if (is_transfer)
- return NULL;
+ return nullptr;
NavigationController::LoadURLParams load_url_params(params.url);
load_url_params.referrer = params.referrer;
load_url_params.frame_tree_node_id = params.frame_tree_node_id;
@@ -167,16 +168,14 @@
class CrossSiteTransferTest : public ContentBrowserTest {
public:
- CrossSiteTransferTest() : old_delegate_(NULL) {
- }
+ CrossSiteTransferTest() : old_delegate_(nullptr) {}
// ContentBrowserTest implementation:
void SetUpOnMainThread() override {
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
- base::Bind(
- &CrossSiteTransferTest::InjectResourceDisptcherHostDelegate,
- base::Unretained(this)));
+ base::Bind(&CrossSiteTransferTest::InjectResourceDispatcherHostDelegate,
+ base::Unretained(this)));
host_resolver()->AddRule("*", "127.0.0.1");
ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
content::SetupCrossSiteRedirector(embedded_test_server());
@@ -211,7 +210,7 @@
IsolateAllSitesForTesting(command_line);
}
- void InjectResourceDisptcherHostDelegate() {
+ void InjectResourceDispatcherHostDelegate() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
old_delegate_ = ResourceDispatcherHostImpl::Get()->delegate();
ResourceDispatcherHostImpl::Get()->SetDelegate(&tracking_delegate_);
@@ -220,7 +219,7 @@
void RestoreResourceDisptcherHostDelegate() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
ResourceDispatcherHostImpl::Get()->SetDelegate(old_delegate_);
- old_delegate_ = NULL;
+ old_delegate_ = nullptr;
}
TrackingResourceDispatcherHostDelegate& tracking_delegate() {
@@ -253,7 +252,7 @@
// Navigate to a starting URL, so there is a history entry to replace.
GURL url1 = embedded_test_server()->GetURL("/site_isolation/blank.html?1");
- NavigateToURL(shell(), url1);
+ EXPECT_TRUE(NavigateToURL(shell(), url1));
// Force all future navigations to transfer. Note that this includes same-site
// navigiations which may cause double process swaps (via OpenURL and then via
@@ -272,7 +271,7 @@
NavigateToURLContentInitiated(shell(), url2, true, true);
// There should be one history entry. url2 should have replaced url1.
- EXPECT_TRUE(controller.GetPendingEntry() == NULL);
+ EXPECT_TRUE(controller.GetPendingEntry() == nullptr);
EXPECT_EQ(1, controller.GetEntryCount());
EXPECT_EQ(0, controller.GetCurrentEntryIndex());
EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL());
@@ -291,7 +290,7 @@
// There should be two history entries. url2 should have replaced url1. url2
// should not have replaced url3.
- EXPECT_TRUE(controller.GetPendingEntry() == NULL);
+ EXPECT_TRUE(controller.GetPendingEntry() == nullptr);
EXPECT_EQ(2, controller.GetEntryCount());
EXPECT_EQ(1, controller.GetCurrentEntryIndex());
EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL());
@@ -312,7 +311,7 @@
// Navigate to a starting URL, so there is a history entry to replace.
GURL url = embedded_test_server()->GetURL("/site_isolation/blank.html?1");
- NavigateToURL(shell(), url);
+ EXPECT_TRUE(NavigateToURL(shell(), url));
// Force all future navigations to transfer. Note that this includes same-site
// navigiations which may cause double process swaps (via OpenURL and then via
@@ -326,7 +325,7 @@
NavigateToURLContentInitiated(shell(), url2, true, true);
// There should be one history entry. url2 should have replaced url1.
- EXPECT_TRUE(controller.GetPendingEntry() == NULL);
+ EXPECT_TRUE(controller.GetPendingEntry() == nullptr);
EXPECT_EQ(1, controller.GetEntryCount());
EXPECT_EQ(0, controller.GetCurrentEntryIndex());
EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL());
@@ -337,7 +336,7 @@
// There should be two history entries. url2 should have replaced url1. url2
// should not have replaced url3.
- EXPECT_TRUE(controller.GetPendingEntry() == NULL);
+ EXPECT_TRUE(controller.GetPendingEntry() == nullptr);
EXPECT_EQ(2, controller.GetEntryCount());
EXPECT_EQ(1, controller.GetCurrentEntryIndex());
EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL());
@@ -353,7 +352,7 @@
// Navigate to a starting URL, so there is a history entry to replace.
GURL url1 = embedded_test_server()->GetURL("/site_isolation/blank.html?1");
- NavigateToURL(shell(), url1);
+ EXPECT_TRUE(NavigateToURL(shell(), url1));
// Navigate to a page on A.com which redirects to B.com with entry
// replacement. This will switch processes via OpenURL twice. First to A.com,
@@ -368,7 +367,7 @@
NavigateToURLContentInitiated(shell(), url2a, true, true);
// There should be one history entry. url2b should have replaced url1.
- EXPECT_TRUE(controller.GetPendingEntry() == NULL);
+ EXPECT_TRUE(controller.GetPendingEntry() == nullptr);
EXPECT_EQ(1, controller.GetEntryCount());
EXPECT_EQ(0, controller.GetCurrentEntryIndex());
EXPECT_EQ(url2b, controller.GetEntryAtIndex(0)->GetURL());
@@ -382,7 +381,7 @@
// There should be two history entries. url2b should have replaced url1. url2b
// should not have replaced url3b.
- EXPECT_TRUE(controller.GetPendingEntry() == NULL);
+ EXPECT_TRUE(controller.GetPendingEntry() == nullptr);
EXPECT_EQ(2, controller.GetEntryCount());
EXPECT_EQ(1, controller.GetCurrentEntryIndex());
EXPECT_EQ(url2b, controller.GetEntryAtIndex(0)->GetURL());
@@ -397,7 +396,7 @@
// Navigate to a starting URL, so there is a history entry to replace.
GURL url1 = embedded_test_server()->GetURL("/site_isolation/blank.html?1");
- NavigateToURL(shell(), url1);
+ EXPECT_TRUE(NavigateToURL(shell(), url1));
// Force all future navigations to transfer.
ShellContentBrowserClient::SetSwapProcessesForRedirect(true);
diff --git a/content/browser/loader/cross_site_resource_handler.cc b/content/browser/loader/cross_site_resource_handler.cc
index 7ffb65974..b05ac5d 100644
--- a/content/browser/loader/cross_site_resource_handler.cc
+++ b/content/browser/loader/cross_site_resource_handler.cc
@@ -86,16 +86,18 @@
}
}
-// Returns whether a transfer is needed by doing a check on the UI thread.
-bool CheckNavigationPolicyOnUI(GURL real_url,
- int process_id,
- int render_frame_id) {
- CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
- RenderFrameHostImpl* rfh =
- RenderFrameHostImpl::FromID(process_id, render_frame_id);
- if (!rfh)
- return false;
-
+// Determines whether a navigation to |dest_url| may be completed using an
+// existing RenderFrameHost, or whether transferring to a new RenderFrameHost
+// backed by a different render process is required. This is a security policy
+// check determined by the current site isolation mode, and must be done
+// before the resource at |dest_url| is delivered to |rfh|.
+//
+// When this function returns true for a subframe, an out-of-process iframe
+// must be created.
+//
+// TODO(nick): Move this function to RFHM.
+bool IsRendererTransferNeededForNavigation(RenderFrameHostImpl* rfh,
+ const GURL& dest_url) {
// A transfer is not needed if the current SiteInstance doesn't yet have a
// site. This is the case for tests that use NavigateToURL.
if (!rfh->GetSiteInstance()->HasSite())
@@ -108,14 +110,14 @@
return false;
GURL effective_url = SiteInstanceImpl::GetEffectiveURL(
- rfh->GetSiteInstance()->GetBrowserContext(), real_url);
+ rfh->GetSiteInstance()->GetBrowserContext(), dest_url);
// TODO(nasko, nick): These following --site-per-process checks are
// overly simplistic. Update them to match all the cases
// considered by RenderFrameHostManager::DetermineSiteInstanceForURL.
if (SiteInstance::IsSameWebSite(rfh->GetSiteInstance()->GetBrowserContext(),
rfh->GetSiteInstance()->GetSiteURL(),
- real_url)) {
+ dest_url)) {
return false; // The same site, no transition needed.
}
@@ -125,6 +127,25 @@
SiteIsolationPolicy::DoesSiteRequireDedicatedProcess(effective_url);
}
+// Returns whether a transfer is needed by doing a check on the UI thread.
+CrossSiteResourceHandler::NavigationDecision
+CheckNavigationPolicyOnUI(GURL real_url, int process_id, int render_frame_id) {
+ CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
+ RenderFrameHostImpl* rfh =
+ RenderFrameHostImpl::FromID(process_id, render_frame_id);
+
+ // Without a valid RFH against which to check, we must cancel the request,
+ // to prevent the resource at |url| from being delivered to a potentially
+ // unsuitable renderer process.
+ if (!rfh)
+ return CrossSiteResourceHandler::NavigationDecision::CANCEL_REQUEST;
+
+ if (IsRendererTransferNeededForNavigation(rfh, real_url))
+ return CrossSiteResourceHandler::NavigationDecision::TRANSFER_REQUIRED;
+ else
+ return CrossSiteResourceHandler::NavigationDecision::USE_EXISTING_RENDERER;
+}
+
} // namespace
CrossSiteResourceHandler::CrossSiteResourceHandler(
@@ -236,11 +257,18 @@
return next_handler_->OnResponseStarted(response, defer);
}
-void CrossSiteResourceHandler::ResumeOrTransfer(bool is_transfer) {
- if (is_transfer) {
- StartCrossSiteTransition(response_.get());
- } else {
- ResumeResponse();
+void CrossSiteResourceHandler::ResumeOrTransfer(NavigationDecision decision) {
+ switch (decision) {
+ case NavigationDecision::CANCEL_REQUEST:
+ // TODO(nick): What kind of cleanup do we need here?
+ controller()->Cancel();
+ break;
+ case NavigationDecision::USE_EXISTING_RENDERER:
+ ResumeResponse();
+ break;
+ case NavigationDecision::TRANSFER_REQUIRED:
+ StartCrossSiteTransition(response_.get());
+ break;
}
}
diff --git a/content/browser/loader/cross_site_resource_handler.h b/content/browser/loader/cross_site_resource_handler.h
index b73a5a1..db1dc84 100644
--- a/content/browser/loader/cross_site_resource_handler.h
+++ b/content/browser/loader/cross_site_resource_handler.h
@@ -26,6 +26,12 @@
// and not a download.
class CrossSiteResourceHandler : public LayeredResourceHandler {
public:
+ enum class NavigationDecision {
+ TRANSFER_REQUIRED,
+ USE_EXISTING_RENDERER,
+ CANCEL_REQUEST
+ };
+
CrossSiteResourceHandler(scoped_ptr<ResourceHandler> next_handler,
net::URLRequest* request);
~CrossSiteResourceHandler() override;
@@ -63,7 +69,7 @@
bool OnNormalResponseStarted(ResourceResponse* response,
bool* defer);
- void ResumeOrTransfer(bool is_transfer);
+ void ResumeOrTransfer(NavigationDecision decision);
void ResumeIfDeferred();
// Called when about to defer a request. Sets |did_defer_| and logs the
diff --git a/content/browser/loader/cross_site_resource_handler_browsertest.cc b/content/browser/loader/cross_site_resource_handler_browsertest.cc
new file mode 100644
index 0000000..4d66c1a
--- /dev/null
+++ b/content/browser/loader/cross_site_resource_handler_browsertest.cc
@@ -0,0 +1,277 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/common/frame_messages.h"
+#include "content/public/browser/resource_dispatcher_host.h"
+#include "content/public/browser/resource_dispatcher_host_delegate.h"
+#include "content/public/browser/resource_throttle.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/test_utils.h"
+#include "content/shell/browser/shell.h"
+#include "content/shell/browser/shell_resource_dispatcher_host_delegate.h"
+#include "ipc/ipc_security_test_util.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/url_request/url_request.h"
+
+namespace content {
+
+namespace {
+
+// A ResourceDispatchHostDelegate that uses ResourceThrottles to pause a
+// targeted request temporarily, to run a chunk of test code.
+class TestResourceDispatcherHostDelegate
+ : public ShellResourceDispatcherHostDelegate {
+ public:
+ using RequestDeferredHook = base::Callback<void(const base::Closure& resume)>;
+ TestResourceDispatcherHostDelegate() : throttle_created_(false) {}
+
+ void RequestBeginning(net::URLRequest* request,
+ ResourceContext* resource_context,
+ AppCacheService* appcache_service,
+ ResourceType resource_type,
+ ScopedVector<ResourceThrottle>* throttles) override {
+ CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+ ShellResourceDispatcherHostDelegate::RequestBeginning(
+ request, resource_context, appcache_service, resource_type, throttles);
+
+ // If this is a request for the tracked URL, add a throttle to track it.
+ if (request->url() == tracked_url_) {
+ // Expect only a single request for the tracked url.
+ ASSERT_FALSE(throttle_created_);
+ throttle_created_ = true;
+
+ throttles->push_back(
+ new CallbackRunningResourceThrottle(request, this, run_on_start_));
+ }
+ }
+
+ // Starts tracking a URL. The request for previously tracked URL, if any,
+ // must have been made and deleted before calling this function.
+ void SetTrackedURL(const GURL& tracked_url,
+ const RequestDeferredHook& run_on_start) {
+ CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ // Should not currently be tracking any URL.
+ ASSERT_FALSE(run_loop_);
+
+ // Create a RunLoop that will be stopped once the request for the tracked
+ // URL has been destroyed, to allow tracking the URL while also waiting for
+ // other events.
+ run_loop_.reset(new base::RunLoop());
+
+ BrowserThread::PostTask(
+ BrowserThread::IO, FROM_HERE,
+ base::Bind(&TestResourceDispatcherHostDelegate::SetTrackedURLOnIOThread,
+ base::Unretained(this), tracked_url, run_on_start,
+ run_loop_->QuitClosure()));
+ }
+
+ // Waits until the tracked URL has been requested, and the request for it has
+ // been destroyed.
+ bool WaitForTrackedURLAndGetCompleted() {
+ CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ run_loop_->Run();
+ run_loop_.reset();
+ return tracked_request_completed_;
+ }
+
+ private:
+ // A ResourceThrottle which defers the request at WillStartRequest time until
+ // a test-supplied callback completes. Notifies |tracker| when the request is
+ // destroyed.
+ class CallbackRunningResourceThrottle : public ResourceThrottle {
+ public:
+ CallbackRunningResourceThrottle(net::URLRequest* request,
+ TestResourceDispatcherHostDelegate* tracker,
+ const RequestDeferredHook& run_on_start)
+ : request_(request),
+ tracker_(tracker),
+ run_on_start_(run_on_start),
+ weak_factory_(this) {}
+
+ void WillStartRequest(bool* defer) override {
+ *defer = true;
+ base::Closure resume_request_on_io_thread = base::Bind(
+ base::IgnoreResult(&BrowserThread::PostTask), BrowserThread::IO,
+ FROM_HERE, base::Bind(&CallbackRunningResourceThrottle::Resume,
+ weak_factory_.GetWeakPtr()));
+ BrowserThread::PostTask(
+ BrowserThread::UI, FROM_HERE,
+ base::Bind(run_on_start_, resume_request_on_io_thread));
+ }
+
+ ~CallbackRunningResourceThrottle() override {
+ // If the request is deleted without being cancelled, its status will
+ // indicate it succeeded, so have to check if the request is still pending
+ // as well.
+ tracker_->OnTrackedRequestDestroyed(!request_->is_pending() &&
+ request_->status().is_success());
+ }
+
+ // ResourceThrottle implementation:
+ const char* GetNameForLogging() const override {
+ return "CallbackRunningResourceThrottle";
+ }
+
+ private:
+ void Resume() { controller()->Resume(); }
+ net::URLRequest* request_;
+ TestResourceDispatcherHostDelegate* tracker_;
+ RequestDeferredHook run_on_start_;
+ base::WeakPtrFactory<CallbackRunningResourceThrottle> weak_factory_;
+
+ DISALLOW_COPY_AND_ASSIGN(CallbackRunningResourceThrottle);
+ };
+
+ void SetTrackedURLOnIOThread(const GURL& tracked_url,
+ const RequestDeferredHook& run_on_start,
+ const base::Closure& run_loop_quit_closure) {
+ CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+ throttle_created_ = false;
+ tracked_url_ = tracked_url;
+ run_on_start_ = run_on_start;
+ run_loop_quit_closure_ = run_loop_quit_closure;
+ }
+
+ void OnTrackedRequestDestroyed(bool completed) {
+ CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+ tracked_request_completed_ = completed;
+ tracked_url_ = GURL();
+ run_on_start_ = RequestDeferredHook();
+
+ BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+ run_loop_quit_closure_);
+ }
+
+ // These live on the IO thread.
+ GURL tracked_url_;
+ bool throttle_created_;
+ base::Closure run_loop_quit_closure_;
+ RequestDeferredHook run_on_start_;
+
+ // This lives on the UI thread.
+ scoped_ptr<base::RunLoop> run_loop_;
+
+ // Set on the IO thread while |run_loop_| is non-nullptr, read on the UI
+ // thread after deleting run_loop_.
+ bool tracked_request_completed_;
+
+ DISALLOW_COPY_AND_ASSIGN(TestResourceDispatcherHostDelegate);
+};
+
+class CrossSiteResourceHandlerTest : public ContentBrowserTest {
+ public:
+ CrossSiteResourceHandlerTest() : old_delegate_(nullptr) {}
+
+ // ContentBrowserTest implementation:
+ void SetUpOnMainThread() override {
+ BrowserThread::PostTask(
+ BrowserThread::IO, FROM_HERE,
+ base::Bind(
+ &CrossSiteResourceHandlerTest::InjectResourceDispatcherHostDelegate,
+ base::Unretained(this)));
+ host_resolver()->AddRule("*", "127.0.0.1");
+ ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+ content::SetupCrossSiteRedirector(embedded_test_server());
+ }
+
+ void TearDownOnMainThread() override {
+ BrowserThread::PostTask(
+ BrowserThread::IO, FROM_HERE,
+ base::Bind(&CrossSiteResourceHandlerTest::
+ RestoreResourceDispatcherHostDelegate,
+ base::Unretained(this)));
+ }
+
+ protected:
+ void SetUpCommandLine(base::CommandLine* command_line) override {
+ IsolateAllSitesForTesting(command_line);
+ }
+
+ void InjectResourceDispatcherHostDelegate() {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ old_delegate_ = ResourceDispatcherHostImpl::Get()->delegate();
+ ResourceDispatcherHostImpl::Get()->SetDelegate(&tracking_delegate_);
+ }
+
+ void RestoreResourceDispatcherHostDelegate() {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ ResourceDispatcherHostImpl::Get()->SetDelegate(old_delegate_);
+ old_delegate_ = nullptr;
+ }
+
+ TestResourceDispatcherHostDelegate& tracking_delegate() {
+ return tracking_delegate_;
+ }
+
+ private:
+ TestResourceDispatcherHostDelegate tracking_delegate_;
+ ResourceDispatcherHostDelegate* old_delegate_;
+};
+
+void SimulateMaliciousFrameDetachOnUIThread(int render_process_id,
+ int frame_routing_id,
+ const base::Closure& done_cb) {
+ RenderFrameHostImpl* rfh =
+ RenderFrameHostImpl::FromID(render_process_id, frame_routing_id);
+ CHECK(rfh);
+
+ // Inject a frame detach message. An attacker-controlled renderer could do
+ // this without also cancelling the pending navigation (as blink would, if you
+ // removed the iframe from the document via js).
+ rfh->OnMessageReceived(FrameHostMsg_Detach(frame_routing_id));
+ done_cb.Run();
+}
+
+} // namespace
+
+// Regression test for https://crbug.com/538784 -- ensures that one can't
+// sidestep CrossSiteResourceHandler by detaching a frame mid-request.
+IN_PROC_BROWSER_TEST_F(CrossSiteResourceHandlerTest,
+ NoDeliveryToDetachedFrame) {
+ GURL attacker_page = embedded_test_server()->GetURL(
+ "evil.com", "/cross_site_iframe_factory.html?evil(evil)");
+ EXPECT_TRUE(NavigateToURL(shell(), attacker_page));
+
+ FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+ ->GetFrameTree()
+ ->root();
+
+ RenderFrameHost* child_frame = root->child_at(0)->current_frame_host();
+
+ // Attacker initiates a navigation to a cross-site document. Under --site-per-
+ // process, these bytes must not be sent to the attacker process.
+ GURL target_resource =
+ embedded_test_server()->GetURL("a.com", "/title1.html");
+
+ // We add a testing hook to simulate the attacker-controlled process sending
+ // FrameHostMsg_Detach before the http response arrives. At the time this test
+ // was written, the resource request had a lifetime separate from the RFH,
+ tracking_delegate().SetTrackedURL(
+ target_resource, base::Bind(&SimulateMaliciousFrameDetachOnUIThread,
+ child_frame->GetProcess()->GetID(),
+ child_frame->GetRoutingID()));
+ EXPECT_TRUE(ExecuteScript(
+ shell()->web_contents()->GetMainFrame(),
+ base::StringPrintf("document.getElementById('child-0').src='%s'",
+ target_resource.spec().c_str())));
+
+ // Wait for the scenario to play out. If this returns false, it means the
+ // request did not succeed, which is good in this case.
+ EXPECT_FALSE(tracking_delegate().WaitForTrackedURLAndGetCompleted())
+ << "Request should have been cancelled before reaching the renderer.";
+}
+
+} // namespace content
diff --git a/content/content_tests.gypi b/content/content_tests.gypi
index e374b9ec..b3155e8 100644
--- a/content/content_tests.gypi
+++ b/content/content_tests.gypi
@@ -224,6 +224,7 @@
'browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc',
'browser/indexed_db/mock_browsertest_indexed_db_class_factory.h',
'browser/loader/resource_dispatcher_host_browsertest.cc',
+ 'browser/loader/cross_site_resource_handler_browsertest.cc',
'browser/loader/async_resource_handler_browsertest.cc',
'browser/manifest/manifest_browsertest.cc',
'browser/media/android/media_session_browsertest.cc',
