New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 870815 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Display an error page instead of "data:," for failed XFO checks

Project Member Reported by alex...@chromium.org, Aug 3

Issue description

Currently, when a navigation is blocked by X-Frame-Options, the frame loads a blank page with a unique origin, via the "data:," URL.  This behavior was introduced in r401664 and then kept by the PlzNavigate XFO implementation in https://codereview.chromium.org/2488743003/diff/500001/content/renderer/render_frame_impl.cc.  

While this was saner than the previous behavior of committing about:blank, we should try to clean this up and commit a real error page, which would result in a better user experience and cleaner code.  I think we wanted to do this all along but were blocked on weird error page behavior, which could for example stick an error in a Chrome Web Store subframe into the parent's process, resulting in renderer kills.  More context in issues  622385 ,  588314 , 560511,  555418 , and  689733 .  Now that we have PlzNavigate (with old navigation paths removed) and error page isolation ( issue 838161 ), we should revisit this.  Error page isolation is currently only enabled for main frames, and so this might be blocked on also enabling it on subframes.

Same probably applies to CSP frame-ancestors, which is still enforced in the renderer (see issue 759184) and loads "data:," in DocumentLoader::CancelLoadAfterCSPDenied (https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/loader/document_loader.cc?l=581&rcl=e08a84a069d2f5f440e5b84e8903dd5515647dd7).

 
Owner: arthurso...@chromium.org
Status: Started (was: Untriaged)
I started a change for X-Frame-Options. I seems to works.
https://chromium-review.googlesource.com/c/chromium/src/+/1163247

I checked X-Frame-Options with the ChromeWebStore. It works too (See video)
I still need to understand why it now work and adds some tests.
out-37.ogv
2.0 MB View Download
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/144eff60bb0d1b1aa9c587afa7e4fb1d989c9621

commit 144eff60bb0d1b1aa9c587afa7e4fb1d989c9621
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Aug 13 10:37:15 2018

Display error page for X-Frame-Options.

Currently, when a navigation is blocked by X-Frame-Options, the frame
loads a blank page with an unique origin via the "data:," URL.

Committing the real error page was not possible at that time. Mainly
because of the interaction with the ChromeWebStore. See  bug 622385 .

This CL removes the code preventing the renderer process to display the
error page.

Bug: 870815
Change-Id: I8d94f832791de019779679a7414371b6422e32fe
Reviewed-on: https://chromium-review.googlesource.com/1163247
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582543}
[modify] https://crrev.com/144eff60bb0d1b1aa9c587afa7e4fb1d989c9621/chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
[modify] https://crrev.com/144eff60bb0d1b1aa9c587afa7e4fb1d989c9621/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/144eff60bb0d1b1aa9c587afa7e4fb1d989c9621/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/144eff60bb0d1b1aa9c587afa7e4fb1d989c9621/content/renderer/render_frame_impl.cc

Cc: -nasko@chromium.org
Owner: ----
Status: Available (was: Started)
This task is fixed for the X-Frame-Options case. There are still some work to do for CSP: frame-ancestors.

It looks like we need to hook into DocumentLoader::CancelLoadAfterCSPDenied() and try to do something equivalent of RenderFrameImpl::LoadNavigationErrorPage()

I don't know if it is possible. The alternative is to move frame-ancestors checks to the browser process, which will fix this for free.

Started -> Available: I am no more actively working on it.
Cc: nasko@chromium.org
Cc: mfrederick@google.com lghall@google.com
 Issue 41251  has been merged into this issue.

Sign in to add a comment