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

Issue 625349 link

Starred by 3 users
Status: Fixed
Owner:
creis@chromium.org
Closed: Aug 2016
Cc:
█ mkwst@chromium.org
japhet@chromium.org
creis@chromium.org
alex...@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

insecure-iframe-in-main-frame.html is failing with the new navigation path

Project Member Reported by alex...@chromium.org, Jul 2 2016 
http/tests/security/mixedContent/insecure-iframe-in-main-frame.html fails when run in site isolation modes (e.g., --site-per-process), producing the following output diff:
 ============== Back Forward List ==============
 curr->  https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-frame.html
+            http://example.test:8080/security/mixedContent/resources/boring.html (in frame "<!--framePath //<!--frame0-->-->")
 ===============================================

The test enables running mixed content (via WebKitAllowRunningInsecureContent preference) and then sets up an HTTP page, which opens an HTTPS popup, which adds the insecure HTTP iframe boring.html.  The above diff is for the history dump of that popup.

Because running mixed content is enabled, boring.html does actually load correctly on both the old and new navigation paths, but strangely, it doesn't show up on the history dump with the old path, but it does show up with the new path.

The reason appears to be that mixed content changes the commit type for the iframe to an inert commit in FrameLoader::receivedFirstData():
    
    else if (historyCommitType == InitialCommitInChildFrame && MixedContentChecker::isMixedContent(m_frame->tree().top()->securityContext()->getSecurityOrigin(), m_documentLoader->url()))
        historyCommitType = HistoryInertCommit;

which causes HistoryController::UpdateForCommit not to put the commit into the PageState on the old path.  It does, however, show up in the PageState that's produced in on the new path. (Note that the  layout test history dumps here are produced by BlinkTestController::OnCaptureSessionHistory in the browser process).

To confirm this, I added an HTTPS iframe to the HTTPS popup's main page (i.e., no mixed content), and that iframe correctly shows up in history dumps both with --site-per-process and without.

Since the behavior of the test is actually correct (boring.html does commit in both modes), we might just want to tweak test expectations and check that the iframe has loaded without the confusing history dumps, since it seems like boring.html should really show up on the output in both modes.

Comment 1 by sabineb@chromium.org, Jul 7 2016

Components: Blink>SecurityFeature

Comment 2 by creis@chromium.org, Jul 21 2016

Cc: japhet@chromium.org mkwst@chromium.org
Components: UI>Browser>Navigation
Looking more closely, I don't think we can just change the expectations here.  The receivedFirstData line referenced above comes from https://codereview.chromium.org/278133002, which was the fix for  issue 348952 .  It looks like the new navigation path is regressing that bug, which causes the frame to be restored without marking it as mixed content.

Nate, was the intent of that fix to force the frame to be loaded from scratch if it failed a mixed content check?  It seems like there might be some issues with that.  Why is the fix limited to the initial commit?  What happens if you navigate the frame to another HTTP page and then restore?  (Will we forget about the navigation, or will we incorrectly restore it without warning?)  I'll try to put together a test page.

Comment 3 by creis@chromium.org, Aug 1 2016

Components: Blink>Loader
Owner: creis@chromium.org
Status: Started (was: Available)
I've confirmed that the fix for  issue 348952  is problematic, both in default mode and the new UseSubframeNavigationEntries mode.  We'll want to remove it and find another way to fix that bug.

--

Problem 1: The original fix breaks PageState on the mixed content iframe, so we lose things like scroll position and form data when going back.
Repro steps:
1) Visit https://csreis.github.io/tests/cross-site-iframe-mixed-content.html
2) Click the shield in the omnibox to allow unsafe scripts.
3) Resize the iframe to height=100 in DevTools, and scroll down.
4) Click "simple.html" in the iframe.
5) Go back.
We should have gone back in the iframe and scrolled to the bottom.  Instead, we reload the main frame and lose any information about scroll state, form data, etc.

--

Problem 2: The original fix is incomplete and doesn't address the case where mixed content is not the initial frame load (in both default Chrome and --isolate-extensions).
Repro steps:
1) Visit https://csreis.github.io/tests/cross-site-iframe-mixed-content-initially-blank.html
2) Click "Go HTTP," which is blocked.
3) Click the shield in the omnibox to allow unsafe scripts.
4) Click "Go HTTP," which turns omnibox red.
5) Visit http://www.chromium.org.
6) Go back.
We end up loading the HTTP iframe and the omnibox is neither red nor green.

This was the original problem from  issue 348952 , and it basically matches what --isolate-extensions does in the simpler (initial frame load) case:
1) Visit https://csreis.github.io/tests/cross-site-iframe-mixed-content.html
2) Click the shield in the omnibox to allow unsafe scripts.
3) Visit http://www.chromium.org.
4) Go back.

Comment 4 by creis@chromium.org, Aug 1 2016

Labels: Proj-IsolateExtensions-BlockingLaunch
This seems to be related to MixedContentChecker's behavior for the first time we visit an HTTP iframe in an HTTPS page, compared to how it behaves when we go back/forward.  We're getting to MixedContentChecker::shouldBlockFetch in both cases.

Looks like the iframe is treated as WebMixedContent::ContextType::Blockable on the first visit, and WebMixedContent::ContextType::ShouldBeBlockable when we leave and come back.  In the ShouldBeBlockable case, the choice boils down to whether we're in strict mode (which we're not), so we allow the iframe to load.

The contextType appears to come from WebMixedContent::contextTypeFromRequestContext.  It seems to usually be 18 (RequestContextLocation), which get treated as Blockable.  In the back/forward case, it's 17 (RequestContextInternal), which is ShouldBeBlockable.

mkwst@ or alexmos@: Is there a way to make this case Blockable, per issue 388650?  That would fix this bug, as well as all the issues listed in comment 3 with the original fix.  (If we can't do it for all RequestContextInternal, is there a way to distinguish this particular case?  I'm not familiar with request context types.)

I ask because this is blocking us from switching to the new UseSubframeNavigationEntries() path needed for OOPIFs.  :)

Comment 6 by creis@chromium.org, Aug 2 2016

Status: Fixed (was: Started)
►

Sign in to add a comment