Ensure that HistoryController's current entry is updated on inert commits.
BUG=480201
TEST=See bug for repro steps.
Review URL: https://codereview.chromium.org/1112823005
Cr-Commit-Position: refs/heads/master@{#327806}
diff --git a/content/browser/bad_message.h b/content/browser/bad_message.h
index fe89e1c..8084d8b 100644
--- a/content/browser/bad_message.h
+++ b/content/browser/bad_message.h
@@ -39,6 +39,7 @@
RWHVM_UNEXPECTED_FRAME_TYPE,
RFPH_DETACH,
DFH_BAD_EMBEDDER_MESSAGE,
+ NC_AUTO_SUBFRAME,
// Please add new elements here. The naming convention is abbreviated class
// name (e.g. RenderFrameHost becomes RFH) plus a unique description of the
// reason.
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 17f0007..f2850dc 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -1316,6 +1316,13 @@
// Update the current navigation entry in case we're going back/forward.
if (entry_index != last_committed_entry_index_) {
+ // Make sure that a subframe commit isn't changing the main frame URL.
+ // Otherwise the renderer process may be confused, leading to a URL spoof.
+ if (GetLastCommittedEntry()->GetURL() !=
+ GetEntryAtIndex(entry_index)->GetURL()) {
+ bad_message::ReceivedBadMessage(rfh->GetProcess(),
+ bad_message::NC_AUTO_SUBFRAME);
+ }
last_committed_entry_index_ = entry_index;
DiscardNonCommittedEntriesInternal();
return true;
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index 7a89b26c..57bbf7c 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -1070,4 +1070,104 @@
ResourceDispatcherHost::Get()->SetDelegate(nullptr);
}
+// Ensure the renderer process does not get confused about the current entry
+// due to subframes and replaced entries. See https://crbug.com/480201.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+ PreventSpoofFromSubframeAndReplace) {
+ // Start at an initial URL.
+ GURL url1(embedded_test_server()->GetURL(
+ "/navigation_controller/simple_page_1.html"));
+ NavigateToURL(shell(), url1);
+
+ // Now go to a page with a real iframe.
+ GURL url2(embedded_test_server()->GetURL(
+ "/navigation_controller/page_with_data_iframe.html"));
+ NavigateToURL(shell(), url2);
+
+ // It is safe to obtain the root frame tree node here, as it doesn't change.
+ FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+ ->GetFrameTree()
+ ->root();
+ ASSERT_EQ(1U, root->child_count());
+ ASSERT_NE(nullptr, root->child_at(0));
+
+ {
+ // Navigate in the iframe.
+ FrameNavigateParamsCapturer capturer(root->child_at(0));
+ GURL frame_url(embedded_test_server()->GetURL(
+ "/navigation_controller/simple_page_2.html"));
+ NavigateFrameToURL(root->child_at(0), frame_url);
+ capturer.Wait();
+ EXPECT_EQ(NAVIGATION_TYPE_NEW_SUBFRAME, capturer.details().type);
+ }
+
+ {
+ // Go back in the iframe.
+ TestNavigationObserver back_load_observer(shell()->web_contents());
+ shell()->web_contents()->GetController().GoBack();
+ back_load_observer.Wait();
+ }
+
+ {
+ // Go forward in the iframe.
+ TestNavigationObserver forward_load_observer(shell()->web_contents());
+ shell()->web_contents()->GetController().GoForward();
+ forward_load_observer.Wait();
+ }
+
+ GURL url3(embedded_test_server()->GetURL(
+ "/navigation_controller/page_with_iframe.html"));
+ {
+ // location.replace() to cause an inert commit.
+ TestNavigationObserver replace_load_observer(shell()->web_contents());
+ std::string script = "location.replace('" + url3.spec() + "')";
+ EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+ replace_load_observer.Wait();
+ }
+
+ {
+ // Go back to url2.
+ TestNavigationObserver back_load_observer(shell()->web_contents());
+ shell()->web_contents()->GetController().GoBack();
+ back_load_observer.Wait();
+
+ // Make sure the URL is correct for both the entry and the main frame, and
+ // that the process hasn't been killed for showing a spoof.
+ EXPECT_TRUE(root->current_frame_host()->IsRenderFrameLive());
+ EXPECT_EQ(url2, shell()->web_contents()->GetLastCommittedURL());
+ EXPECT_EQ(url2, root->current_url());
+ }
+
+ {
+ // Go back to reset main frame entirely.
+ TestNavigationObserver back_load_observer(shell()->web_contents());
+ shell()->web_contents()->GetController().GoBack();
+ back_load_observer.Wait();
+ EXPECT_EQ(url1, shell()->web_contents()->GetLastCommittedURL());
+ EXPECT_EQ(url1, root->current_url());
+ }
+
+ {
+ // Go forward.
+ TestNavigationObserver back_load_observer(shell()->web_contents());
+ shell()->web_contents()->GetController().GoForward();
+ back_load_observer.Wait();
+ EXPECT_EQ(url2, shell()->web_contents()->GetLastCommittedURL());
+ EXPECT_EQ(url2, root->current_url());
+ }
+
+ {
+ // Go forward to the replaced URL.
+ TestNavigationObserver forward_load_observer(shell()->web_contents());
+ shell()->web_contents()->GetController().GoForward();
+ forward_load_observer.Wait();
+
+ // Make sure the URL is correct for both the entry and the main frame, and
+ // that the process hasn't been killed for showing a spoof.
+ EXPECT_TRUE(root->current_frame_host()->IsRenderFrameLive());
+ EXPECT_EQ(url3, shell()->web_contents()->GetLastCommittedURL());
+ EXPECT_EQ(url3, root->current_url());
+ }
+}
+
} // namespace content
diff --git a/content/renderer/history_controller.cc b/content/renderer/history_controller.cc
index 35f072a..f2d3d96 100644
--- a/content/renderer/history_controller.cc
+++ b/content/renderer/history_controller.cc
@@ -161,14 +161,30 @@
const WebHistoryItem& item,
WebHistoryCommitType commit_type,
bool navigation_within_page) {
- if (commit_type == blink::WebBackForwardCommit) {
- if (!provisional_entry_)
- return;
- current_entry_.reset(provisional_entry_.release());
- } else if (commit_type == blink::WebStandardCommit) {
- CreateNewBackForwardItem(frame, item, navigation_within_page);
- } else if (commit_type == blink::WebInitialCommitInChildFrame) {
- UpdateForInitialLoadInChildFrame(frame, item);
+ switch (commit_type) {
+ case blink::WebBackForwardCommit:
+ if (!provisional_entry_)
+ return;
+ current_entry_.reset(provisional_entry_.release());
+ break;
+ case blink::WebStandardCommit:
+ CreateNewBackForwardItem(frame, item, navigation_within_page);
+ break;
+ case blink::WebInitialCommitInChildFrame:
+ UpdateForInitialLoadInChildFrame(frame, item);
+ break;
+ case blink::WebHistoryInertCommit:
+ // Even for inert commits (e.g., location.replace, client redirects), make
+ // sure the current entry gets updated, if there is one.
+ if (current_entry_) {
+ if (HistoryEntry::HistoryNode* node =
+ current_entry_->GetHistoryNodeForFrame(frame)) {
+ node->set_item(item);
+ }
+ }
+ break;
+ default:
+ NOTREACHED() << "Invalid commit type: " << commit_type;
}
}
diff --git a/content/test/data/navigation_controller/page_with_data_iframe.html b/content/test/data/navigation_controller/page_with_data_iframe.html
new file mode 100644
index 0000000..f91f8e0
--- /dev/null
+++ b/content/test/data/navigation_controller/page_with_data_iframe.html
@@ -0,0 +1,7 @@
+<html>
+<head></head>
+<body>
+ <p>This page has a data URL iframe.
+ <p><iframe src="data:text/html,Subframe"></iframe>
+</body>
+</html>
