Open Bug 1577903 Opened 5 years ago Updated 2 years ago

<a href="javascript:alert(1);" target="_blank">Run JS in _blank new tab</a> fails

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

Tracking Status
firefox70 --- affected

People

(Reporter: asa, Unassigned)

Details

Since bug 1503681, the testcase at https://webdbg.com/test/window/jstarget.htm Run JS in _blank new tab fails.

Flags: needinfo?(bzbarsky)

So a testcase that was broken even before bug 1503681 is:

<a href="javascript:alert(1)" rel="noopener" target="_blank">One</a>

I'll check on Tuesday to see whether that's an issue in the UI or backend.

OK, so the proximate cause is that in nsJSThunk::EvaluateScript we have:

  • owner == nullptr
  • loadInfo->GetForceInheritPrincipal() == true
  • loadInfo->mPrincipalToInherit is a nullprincipal.

so principal ends up being that nullprincipal (which is wrong). But objectPrincipal is a different nullprincipal (also wrong), so evaluation is not allowed.

Why is that happening? Well, for the incorrect inherited principal what happens is that in nsDocShell::InternalLoad we have the principal of the web page as the triggering principal in the nsDocShellLoadState, but a nullprincipal as the "principal to inherit". That happens because our load flags have nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL. That doesn't seem to be correct in this specific case, and I am pretty sure it's being added by the UI.

For the incorrect principal of the page, that's coming from nsDocShell::CreateAboutBlankContentViewer being passed a nullprincipal when nsDocShell::CreateContentViewerForActor calls it. That happens because the principal ContentChild::RecvConstructBrowser gets inside aWindowInit is a nullprincipal. That's coming from ContentParent::CreateBrowser, which afaict always passes a nullprincipal down. In the same-process case we would then fix things up by calling SetInitialPrincipalToSubject before doing the load in the new window, but in the cross-process case that obviously doesn't happen.

Seems like we'd need to solve both of those issues for this to work.

Flags: needinfo?(nika)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bzbarsky)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

That happens because our load flags have nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL. That doesn't seem to be correct in this specific case, and I am pretty sure it's being added by the UI.

The child process is told what to do by the parent via a WebNavigation:LoadURI message. It has support for loaduri flags but the message doesn't include this flag.

The parent stack comes through loadOneTab (which has an allowInheritPrincipal option which would send the right loaduri flag to the child, which is not used) from nsIBrowserDOMWindow::openURIInFrame. We probably need to add a flag on nsIBrowserDOMWindow, and make the browser.js implementation use it to pass the right option into loadOneTab, and make whatever native code is invoking openURIInFrame pass it when necessary. Or something.

Ugh.

x-ref bug 1485961.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

For the incorrect principal of the page, that's coming from nsDocShell::CreateAboutBlankContentViewer being passed a nullprincipal when nsDocShell::CreateContentViewerForActor calls it. That happens because the principal ContentChild::RecvConstructBrowser gets inside aWindowInit is a nullprincipal. That's coming from ContentParent::CreateBrowser, which afaict always passes a nullprincipal down. In the same-process case we would then fix things up by calling SetInitialPrincipalToSubject before doing the load in the new window, but in the cross-process case that obviously doesn't happen.

In general we don't inherit the principal for the initial about:blank document when doing a cross origin load, and I think this is the only case where that is observably the wrong thing to do. When we do a noopener load, we make a point of requesting a new process, in order to try to distribute browser load across processes.

In a post-fission world, this could be fixed by not special-casing noopener loads in the same way, and always using the in-process codepath, just without opener set. The load in the new browsing context would then potentially trigger a process switch into a new process if necessary, which wouldn't happen for javascript: loads. For non-fission codepaths, we might want to do additional work to request a process switch on the first navigation in the new Browsing Context.

Flags: needinfo?(nika)
Priority: -- → P2
Severity: normal normal → S3 S3
You need to log in before you can comment on or make changes to this bug.