1955324 - sync-about-blank: Should initial about:blank be allowed to inherit system principal?

Status: NEW

(Core :: DOM: Navigation, defect, P3)

Description

[Vincent Hilla [:vhilla]]

With the work in Bug 1792685, I reviewed the remaining extension test failures. browser_ext_devtools_panels_elements_sidebar.js relates to principals, inner window replacement, and possibly the extensions code. I'll summarize my results, as I'm currently stuck.

Problem

• The test does sidebar4.setPage("sidebar.html"); and inspector.sidebar.show(sidebarIds[3]); which appends a XUL frame somewhere. Via nsGenericHTMLFrameElement::LoadSrc, we already have a mURIToLoad, which is chrome://.../webext-panels.xhtml. Our triggering principal is a system principal, same for our parent document.
• On central, nsFrameLoader::ReallyStartLoadingInternal will create the docshell, then do LoadURI. LoadURI will get to EnsureDocumentViewer and create a blank document with GetInheritedPrincipal(false) as principal.
  • At this point, our BC and parent BC both have mType=typeContent, also the new docshell has mItemType=typeContent. But the parent docshell has a chrome:// URI loaded and a system principal. Given this condition is not met as we inherit our principal from the parent, the about:blank will be loaded with system principal.
• On the contrary with the patch, a couple changes were made that lead us to load about:blank with null principal
  • We create the blank document eagerly when creating the docshell, i.e. a few steps earlier in ReallyStartLoadingInternal through MaybeCreateDocShell.
  • EnsureDocumentViewer doesn't anymore use GetInheritedPrincipal(false) but rather aOpenWindowInfo->PrincipalToInheritForAboutBlank(). This principal is determined in MaybeCreateDocShell and will never inherit a system principal to a content frame.
• So on central, we allow inheriting a system principal from the parent to a content frame, but not with this patch.
• ReallyStartLoadingInternal gets to LoadURI and an async channel load for webext-panels.xhtml starts.
• While this is ongoing, the extension code somehow reaches ParentDevToolsInspectorSidebar::onExtensionPageMount in ext-devtools-panel.js. This method starts waiting for a load event on the current window. Given the name, it seems React calls this immediately after binding the frame to the tree.
• With the patch, we now get to a problem. WouldReuseInnerWindow is false, as we load a system principal URI onto a null principal about:blank. So the async load replaces the inner window and onExtensionPageMount never completes.

Ideas

• I tried changing MaybeCreateDocShell to resemble GetInheritedPrincipal and inherit the system principal to about:blank. But the code is quite explicit that this shouldn't be done and other tests would then fail the assert !aPrincipalToInherit && mDocumentViewer->GetDocument()->NodePrincipal()->GetIsNullPrincipal() in IsAboutBlankLoadOntoInitialAboutBlank.
• WouldReuseInnerWindow could default to true for uncommitted initial documents. I could imagine this being problematic if someone attaches code to this window before it loads some system URI.
• I currently don't have many test failures left and this is the only test where I see this failure pattern. Maybe its a special case in the devtools panel code that could be changed.

Comment 1

[Vincent Hilla [:vhilla]]

I think I managed to create a minimal browser test case

const CHROME_URI = "chrome://global/content/aboutSupport.xhtml";

add_task(async function test_chrome_uri_in_new_tab() {
  // open a tab with system principal due to chrome URI
  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, CHROME_URI);
  let browser = tab.linkedBrowser;

  await SpecialPowers.spawn(browser, [CHROME_URI], async (CHROME_URI) => {
    let bc = content.browsingContext;
    let contentPrincipal = content.document.nodePrincipal;
    Assert.ok(contentPrincipal.isSystemPrincipal, "tab has system principal");
    Assert.ok(bc.isContent, "tab BC is content");

    // within a system context, add a new iframe
    let iframe = content.document.createElement("iframe");
    iframe.src = CHROME_URI;
    content.document.documentElement.appendChild(iframe);

    // iframe will start with some different principal
    let ifrBC = iframe.browsingContext;
    let aboutBlankPrincipal = iframe.contentDocument.nodePrincipal;
    Assert.ok(iframe.contentDocument.isUncommittedInitialDocument, "iframe at uncommitted initial doc");
    Assert.ok(!aboutBlankPrincipal.isSystemPrincipal, "initial about:blank doesn't have system principal");
    Assert.ok(aboutBlankPrincipal.isNullPrincipal, "initial about:blank starts out with null principal");
    Assert.ok(ifrBC.isContent, "iframe BC is content");

    // inner window will be replaced
    iframe.contentWindow.foo = "bar";
    iframe.contentWindow.addEventListener("load", () => {
      Assert.ok(false, "load event never fired on initial iframe inner window");
    });

    await new Promise(res => iframe.addEventListener("load", res));

    // after load, iframe has system principal and inner window was replaced
    let chromeDocPrincipal = iframe.contentDocument.nodePrincipal;
    Assert.ok(chromeDocPrincipal.isSystemPrincipal, "after load, iframe has system principal");
    Assert.ok(ifrBC.isContent, "iframe BC stays content");
    Assert.ok(iframe.contentWindow.foo == undefined, "iframe inner window replaced");

    iframe.remove();
  });

  BrowserTestUtils.removeTab(tab);
});

So is this behavior expected? That when a privileged page adds an iframe, this iframe starts out with null principal and a consecutive load will replace the inner window? I'm pretty sure this causes a problem with devtools when they attach a load listener to the initial inner window immediately after page mount.

:hsivonen

Comment 2

[Vincent Hilla [:vhilla]]

If we only care about fixing the test, onExtensionPageMount can be modified to attach a load listener to the frame instead of content window.

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to Vincent Hilla [:vhilla] from comment #2)

If we only care about fixing the test, onExtensionPageMount can be modified to attach a load listener to the frame instead of content window.

This is probably the way to go.

The comment on the code that explicitly avoids materializing an initial about:blank with the system principal refers to a test case that was authored by sefeng and reviewed by smaug. It seems intuitively bad to have a system-principal about:blank in a type: content browsing context, but sefeng, smaug, can you confirm that we indeed want to avoid that situation and, therefore, the inner window should get replaced here and, therefore, the test should tolerate the inner window replacement?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I don't know what comment you're referring to?

(I'd expect initial about:blanks in iframes always use the principal of their parent if they are in the same type docshell tree,
well, I guess sandboxed iframes might be a special case)

Comment 5

[Henri Sivonen (:hsivonen)]

I'm referring to this part in nsFrameLoader::MaybeCreateDocShell() in https://phabricator.services.mozilla.com/D155376 :

  if (mOpenWindowInfo) {
    // XUL has mOpenWindowInfo
    if (mPendingBrowsingContext->IsContent() &&
        mOpenWindowInfo->PrincipalToInheritForAboutBlank() &&
        mOpenWindowInfo->PrincipalToInheritForAboutBlank()
            ->IsSystemPrincipal()) {
      // No system principal inheritance into content XUL browser:
      // dom/tests/mochitest/general/test_nodeAdoption_chrome_boundary.xhtml
      openWindowInfo = new nsOpenWindowInfo();
      openWindowInfo->mPrincipalToInheritForAboutBlank = NullPrincipal::Create(
          mPendingBrowsingContext->OriginAttributesRef(), nullptr);
    }

For reasons that I haven't attributed to a specific test, I have also written code with the comment "Never inherit system principal to a content HTML [i]frame." in the else branch of the quoted if.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Ah, ok, so test_nodeAdoption_chrome_boundary.xhtml is a chrome test. So the top level docshell should be in chrome docshell tree and the <browser> element is in content docshell tree. So no inheritance should happen in that case.
I don't understand mPendingBrowsingContext->IsContent() check. As far as I see docshell code is doing reasonable inheritance where it checks
whether the parent docshell is the same type / in the same type of tree.
https://searchfox.org/mozilla-central/rev/3a0ca3dffd7ccf74a53066a097739f24dd8b6b10/docshell/base/nsDocShell.cpp#9668,9680

Comment 7

[Vincent Hilla [:vhilla]]

There's a similar problem in browser_extension_migration.js where we click an about:blank link in a chrome docshell. This causes us to fail

MOZ_ASSERT_IF(
      isAboutBlankLoadOntoInitialAboutBlank && inheritPrincipal,
      aLoadState->PrincipalToInherit()->Equals(GetDocument()->GetPrincipal()) ||
          (aLoadState->PrincipalToInherit()->GetIsNullPrincipal() &&
           GetDocument()->GetPrincipal()->GetIsNullPrincipal()));

as the document will have a null principal due to the code in MaybeCreateDocShell() while aLoadState->PrincipalToInherit() will be the node principal, which is a system principal.

Minimal test case:

const CHROME_URI = "chrome://global/content/aboutSupport.xhtml";

add_task(async function test_chrome_blank_link() {
  // open a tab with system principal due to chrome URI
  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, CHROME_URI);
  let browser = tab.linkedBrowser;

  const linkOpened = BrowserTestUtils.waitForNewTab(gBrowser, "about:blank");

  await SpecialPowers.spawn(browser, [], async () => {
    let bc = content.browsingContext;
    let contentPrincipal = content.document.nodePrincipal;
    Assert.ok(contentPrincipal.isSystemPrincipal, "tab has system principal");
    Assert.ok(bc.isContent, "tab BC is content");

    // within a system context, add an about:blank link
    let link = content.document.createElement("a");
    link.href = "about:blank";
    link.target = "_blank";
    content.document.documentElement.appendChild(link);
    link.click();
  });

  const blanktab = await linkOpened;

  Assert.ok(true, "did not crash from assertion failure");

  BrowserTestUtils.removeTab(blanktab);
  BrowserTestUtils.removeTab(tab);
});

Comment 8

[Vincent Hilla [:vhilla]]

I can fix browser_extension_migration.js by changing assertSupportLink to set link.href to a data: URI. But maybe we should decide on what principal about:blank loaded from a system page should have, and document this decision through a more explicit test.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

The initial about:blank loaded in an iframe should get principal from its (same type) parent.
The type of the docshell tree (content or chrome) shouldn't really matter.

Comment 10

[Vincent Hilla [:vhilla]]

I'm unsure about the exact condition to here:

nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();
nsCOMPtr<nsIPrincipal> partitionedPrincipal = doc->PartitionedPrincipal();
if ((mPendingBrowsingContext->IsContent() || XRE_IsContentProcess()) &&
    (!principal || principal->IsSystemPrincipal())) {
  // Never inherit system principal to a content HTML [i]frame.
  principal = NullPrincipal::Create(mPendingBrowsingContext->OriginAttributesRef(), nullptr);
  partitionedPrincipal = principal;
}

As mentioned in #c0, GetInheritedPrincipal() that does "reasonable inheritance" (#c6) won't be used anymore for docshell initialization. We could replace above code with a call to GetInheritedPrincipal for the newly created docshell that is pending initialization, i.e.

nsCOMPtr<nsIPrincipal> principal = docShell->GetInheritedPrincipal(false);
nsCOMPtr<nsIPrincipal> partitionedPrincipal = docShell->GetInheritedPrincipal(false, true);
if (!principal) {
  principal = NullPrincipal::Create(mPendingBrowsingContext->OriginAttributesRef(), nullptr);
  partitionedPrincipal = principal;
}

but whenever I make changes to this code, I hit another assert introduced by this patch: !aPrincipalToInherit && mDocumentViewer->GetDocument()->NodePrincipal()->GetIsNullPrincipal() in nsDocShell::IsAboutBlankLoadOntoInitialAboutBlank. I assume this means I inherited a principal to the initial about:blank that the consecutive sync navigation to about:blank doesn't want. But as we don't replace the document, we want the right principal from the start.

Comment 11

[Vincent Hilla [:vhilla]]

This condition is quite similar to nsDocShellLoadState::SetupInheritingPrincipal, which is called from nsDocShell::LoadURI. So even if we change this condition, for the iframe case at least, SetupInheritingPrincipal would still clear the mPrincipalToInherit so that we run into the assertion anyway.

Though for the second test case were a link is clicked, we never call SetupInheritingPrincipal, rather OnLinkClick calls SetPrincipalToInherit. The initial document is still created from MaybeCreateDocShell, so we go through above condition and create the document with null principal, despite the link click causing us to have a system about:blank as navigation destination. For this case at least, tweaking the condition in MaybeCreateDocShell should suffice. But MaybeCreateDocShell doesn't create the principal themselves, rather mOpenWindowInfo is set as null principal in nsWindowWatcher::OpenWindowInternal. This code again is quite explicit about not inheriting system principals to the initial about:blank.

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #6)

I don't understand mPendingBrowsingContext->IsContent() check. As far as I see docshell code is doing reasonable inheritance where it checks
whether the parent docshell is the same type / in the same type of tree.

Chances are that when I interpreted whatever test failure I was seeing, I interpreted it as "no system principal in BC of type content" instead of interpreting the failure as "no inheritance across BC type boundaries".

Comment 13

[Vincent Hilla [:vhilla]]

I discussed the issue further with :hsivonen.

• This comment in OpenWindowInternal was likely the reason why he chose to not inherit system principals to about:blank in MaybeCreateDocShell.
• We think that relaxing system principal inheritance would introduce additional risks to the patch. System principal inheritance to the initial about:blank is not web-observable and hopefully not necessary. The tests can be made to pass regardless, so I'm hopeful there are no issues in the Firefox frontend.

My current understanding is that changes would be needed in MaybeCreateDocShell, SetupInheritingPrincipal and OpenWindowInternal.

So I favor leaving this bug as a potential follow-up and not address the issue in the current changeset.