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

Unified Diff: content/browser/frame_host/navigation_controller_impl.cc

Issue 2050423002: Account for origin corner cases during AutoSubframe navigations. Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Fix unit tests Created 4 years, 6 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 | « no previous file | content/browser/frame_host/navigation_controller_impl_browsertest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index cfd1310b33122f1781d3ff4d923d0c5238d285c6..cb69349ee7b6c77a0d5061015092465c1e589fb3 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -87,6 +87,50 @@
namespace content {
namespace {
+// Determine if the current page and the destination page could belong to the
+// same origin, in the given |rfh|. This is useful in cases like in-page
+// navigations that can't be cross-origin, but may look cross-origin at first
+// glance in some cases (e.g., about:blank to a web URL, or file:// to web URL
+// when universal access is granted to files).
+//
+// To resolve these, we can use url::Origin to ensure the actual origin isn't
+// changing. However, we also fall back to GURL::GetOrigin() to let a current
+// and destination unique origin return true, since unique url::Origins don't
+// appear equal to themselves.
+//
+// TODO(creis): Clarify that this shouldn't be used to determine script access.
+bool CouldBeSameOrigin(const GURL& current_url,
+ const url::Origin& current_origin,
+ const GURL& dest_url,
+ const url::Origin& dest_origin,
+ const WebPreferences& prefs) {
+ // We don't care if web security is disabled.
+ if (!prefs.web_security_enabled)
+ return true;
+
+ // File URLs may have been granted universal access to any URL.
+ if (prefs.allow_universal_access_from_file_urls &&
+ current_origin.scheme() == url::kFileScheme)
+ return true;
+
+ // It's possible for the URLs to look different but the url::Origins to match,
+ // such as when about:blank inherits its origin from a web page.
+ if (current_origin == dest_origin)
+ return true;
+
+ // For unique origins like data: URLs, they won't appear equal to themselves.
+ //
+ // TODO(creis): Data URLs to/from about:blank, which fail url::Origin check.
+ // TODO(creis): Other unique origins that can do in-page, like data->data.
+ // TODO(creis): This is bad for sandboxed iframe URLs. Only return true if
+ // both url::Origins are unique. Test this.
+ //if (current_url.GetOrigin() == dest_url.GetOrigin())
+ if (current_origin.unique() && dest_origin.unique())
+ return true;
+
+ return false;
+}
+
// Invoked when entries have been pruned, or removed. For example, if the
// current entries are [google, digg, yahoo], with the current entry google,
// and the user types in cnet, then digg and yahoo are pruned.
@@ -1306,8 +1350,28 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
// origin. Otherwise the renderer process may be confused, leading to a
// URL spoof. We can't check the path since that may change
// (https://crbug.com/373041).
- if (GetLastCommittedEntry()->GetURL().GetOrigin() !=
- GetEntryAtIndex(entry_index)->GetURL().GetOrigin()) {
+// RenderFrameHostImpl* main_frame =
+// delegate_->GetFrameTree()->root()->current_frame_host();
+ GURL dest_top_url = GetEntryAtIndex(entry_index)->GetURL();
+ GURL current_top_url = GetLastCommittedEntry()->GetURL();
+ GURL blank_url(url::kAboutBlankURL);
+ url::Origin current_origin =
+ delegate_->GetFrameTree()->root()->current_origin();
+
+ // Allow about:blank to be the current or destination origin, beyond the
+ // other checks in CouldBeSameOrigin. We can't verify the about:blank
+ // cases any more strictly, since we don't know the destination origin.
+ // TODO(creis): If we track url::Origin on NavigationEntry, we might be
+ // able to call IsUrlInPageNavigation here instead, to share code.
+ bool could_be_same_origin =
+ dest_top_url == blank_url ||
+ // TODO(creis): This part isn't needed, due to the constructed origin.
+ //(current_top_url == blank_url &&
+ // dest_top_url.GetOrigin() == GURL(current_origin.Serialize())) ||
+ CouldBeSameOrigin(current_top_url, current_origin,
+ dest_top_url, url::Origin(dest_top_url),
+ rfh->GetRenderViewHost()->GetWebkitPreferences());
+ if (!could_be_same_origin) {
bad_message::ReceivedBadMessage(rfh->GetProcess(),
bad_message::NC_AUTO_SUBFRAME);
}
@@ -1397,7 +1461,9 @@ bool NavigationControllerImpl::IsURLInPageNavigation(
WebPreferences prefs = rfh->GetRenderViewHost()->GetWebkitPreferences();
const url::Origin& committed_origin =
rfhi->frame_tree_node()->current_origin();
- bool is_same_origin = last_committed_url.is_empty() ||
+ bool is_same_origin = CouldBeSameOrigin(last_committed_url, committed_origin,
+ url, origin, prefs);
+ /*last_committed_url.is_empty() ||
// TODO(japhet): We should only permit navigations
// originating from about:blank to be in-page if the
// about:blank is the first document that frame loaded.
@@ -1409,7 +1475,7 @@ bool NavigationControllerImpl::IsURLInPageNavigation(
committed_origin == origin ||
!prefs.web_security_enabled ||
(prefs.allow_universal_access_from_file_urls &&
- committed_origin.scheme() == url::kFileScheme);
+ committed_origin.scheme() == url::kFileScheme);*/
if (!is_same_origin && renderer_says_in_page) {
bad_message::ReceivedBadMessage(rfh->GetProcess(),
bad_message::NC_IN_PAGE_NAVIGATION);
« no previous file with comments | « no previous file | content/browser/frame_host/navigation_controller_impl_browsertest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698