Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2023)

Unified Diff: content/renderer/history_controller.cc

Issue 1848813005: Fix HistoryEntry corruption when commit isn't for provisional entry (try #2). (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Fix test nits Created 4 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « content/browser/frame_host/navigation_controller_impl_browsertest.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: content/renderer/history_controller.cc
diff --git a/content/renderer/history_controller.cc b/content/renderer/history_controller.cc
index c8553ff2e277ecf1fadec475ebd0fdaa588380d2..5de3172d79d57301de45d093d4d6f45d28865a78 100644
--- a/content/renderer/history_controller.cc
+++ b/content/renderer/history_controller.cc
@@ -189,7 +189,36 @@ void HistoryController::UpdateForCommit(RenderFrameImpl* frame,
case blink::WebBackForwardCommit:
if (!provisional_entry_)
return;
- current_entry_.reset(provisional_entry_.release());
+
+ // If the current entry is null, this must be a main frame commit.
+ DCHECK(current_entry_ || frame->IsMainFrame());
+
+ // Commit the provisional entry, but only if it is a plausible transition.
+ // Do not commit it if the navigation is in a subframe and the provisional
+ // entry's main frame item does not match the current entry's main frame,
+ // which can happen if multiple forward navigations occur. In that case,
+ // committing the provisional entry would corrupt it, leading to a URL
+ // spoof. See https://crbug.com/597322. (Note that the race in this bug
+ // does not affect main frame navigations, only navigations in subframes.)
+ //
+ // Note that we cannot compare the provisional entry against |item|, since
+ // |item| may have redirected to a different URL and ISN. We also cannot
+ // compare against the main frame's URL, since that may have changed due
+ // to a replaceState. (Even origin can change on replaceState in certain
+ // modes.)
+ //
+ // It would be safe to additionally check the ISNs of all parent frames
+ // (and not just the root), but that is less critical because it won't
+ // lead to a URL spoof.
+ if (frame->IsMainFrame() ||
+ current_entry_->root().itemSequenceNumber() ==
+ provisional_entry_->root().itemSequenceNumber()) {
+ current_entry_.reset(provisional_entry_.release());
+ }
+
+ // We're guaranteed to have a current entry now.
+ DCHECK(current_entry_);
+
if (HistoryEntry::HistoryNode* node =
current_entry_->GetHistoryNodeForFrame(frame)) {
node->set_item(item);
« no previous file with comments | « content/browser/frame_host/navigation_controller_impl_browsertest.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698