Clicking on a Giphly embed in Twitter loads about:blank

VERIFIED FIXED in Firefox 49

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: kbrosnan, Assigned: bzbarsky)

Tracking

({regression})

49 Branch
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox48 unaffected, firefox49blocking verified, firefox50- verified, firefox51+ verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

* Open https://twitter.com/mmmandel/status/773528085803077632
* Click once to load the embed
* Click again on the image (not the text below the image)

Expected results: New tab with giphly page loaded

Actual results: about:blank

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=49f41e6097076c35c92082cb26
8ea63b9ea5ab03&tochange=58d84f830e72fece44a4838ca47781a3b754aea1

bz wrote all the patches in the regression range, ni to comment. Tentatively blocking on bug 1190641
Flags: needinfo?(bzbarsky)
Version: Trunk → 49 Branch
Simple testcase:

  <iframe srcdoc="<a href='http://mozilla.org' target='_blank'>Link to mozilla.org</a>"
          sandbox="allow-popups allow-popups-to-escape-sandbox allow-same-origin
                   allow-scripts">
  </iframe>

The allow-popups-to-escape-sandbox bit is key.  Without that things work.  Looking into why this is failing, but chances are it's because the spec is broken and targeted anchors are not affected by bug 960545.  See also bug 1190641 comment 20 and <https://github.com/whatwg/html/issues/1218>....

I strongly believe we should fix this before shipping 49 (e.g. by disabling bug 1190641 there; still looking into what the simplest possible fix is).
Based on testing Chrome, it looks to me like they simply disable the check for the one permitted sandbox navigator when "allow-popups-to-escape-sandbox" is set or something.  For example, this testcase:

  <a href="http://example.com" target="foo">Click me first</a>
  <iframe srcdoc="<a href='http://mozilla.org' target='foo'>Click me second. Did I navigate?</a>"
          sandbox="allow-popups-to-escape-sandbox">
  </iframe>

performs the navigation.  But switching the target in the iframe to '_top' blocks the navigation in Chrome...

Looking into their source now to see if I can figure out exactly what they do.
We may need to block 49 on this issue. bz will comment further...
OK.  So I checked the Chrome source and I was right.  They simply don't enforce https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate step 3 if A doesn't have the "sandbox propagates to auxiliary browsing contexts" flag.

At this point, I think we have four options for Firefox 49:

1) Implement the Chrome behavior.  This, I believe, opens up a hole
   in sandboxing; the testcase in comment 3 shows the sandboxed page
   navigating a page that the parent opened...
2) Ship our existing behavior.  This breaks pages, clearly.
3) Unship support for allow-popups-to-escape-sandbox.  This puts us at no worse
   a place than 48, but this feature is somewhat widely anticipated because it 
   allows ads to be sandboxed that otherwise can't be sandboxed at all.  In
   terms of other browser support, Safari (shipping; webkit nightly is different)
   doesn't apply the sandbox to popups at all.
4) Ship some other fix (e.g. setting the one permitted sandboxed navigator
   even though we're escaping the sandbox).

#1 is probably most web-compatible, but as I said I believe it introduces a security hole and poisons the well in terms of being able to remove that security hole, since both Chrome and we would be shipping it.

#2 is not something I really want to do, honestly.  Especially because I expect people to use this sandbox flag more and more, _especially_ once we announce we're shipping it in 49.  I expect we would see a ton of breakage if we did this.

#3 is probably safest in some sense, but runs the risk of some pages being broken anyway, if they use the flag to get the behavior they want in Chrome and get it automatically in Safari.  This is a pretty theoretical breakage, though; we have no concrete examples yet.

#4 runs the risk of us shipping something that still leads to _some_ pages somewhere being broken and then having to change the behavior again if/when the spec actually gets sorted out to not suck.  Of course it's possible that #4 _is_ what will end up getting standardized.  It seems like a sane solution to me, but I haven't dug through all the edge cases here.

Olli, do you have any thoughts on those options?  Bob, likewise?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Flags: needinfo?(bobowen.code)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8789075 [details] [diff] [review]
A viable option 4 implementation.  Fixes the testcase, at least

>@@ -1048,26 +1048,27 @@ nsWindowWatcher::OpenWindowInternal(mozI
>   // better have a window to use by this point
>   if (!newDocShellItem) {
>     return rv;
>   }
> 
>   nsCOMPtr<nsIDocShell> newDocShell(do_QueryInterface(newDocShellItem));
>   NS_ENSURE_TRUE(newDocShell, NS_ERROR_UNEXPECTED);
> 
>+  if (parentWindow) {
>+    newDocShell->SetOnePermittedSandboxedNavigator(
>+      parentWindow->GetDocShell());
>+  }

so we set OnePermittedSandboxedNavigator always, even if opener wasn't sandboxed at all. I guess that doesn't matter since the value is used only if opener is sandboxed.
Flags: needinfo?(bugs)
Attachment #8789075 - Flags: review?(bugs) → review+
> so we set OnePermittedSandboxedNavigator always, even if opener wasn't sandboxed at all.

Oh, hmm.  That's not great, just on general "follow the spec" grounds.  How about I only set it if activeDocSandboxFlags is not 0?
Flags: needinfo?(bugs)
I'm going to commit this to inbound with the change from comment 8, so I can ask for branch approvals.
Comment on attachment 8789075 [details] [diff] [review]
A viable option 4 implementation.  Fixes the testcase, at least

Approval Request Comment
[Feature/regressing bug #]: Bug 1190641
[User impact if declined]: At least some links from twitter will be broken.
[Describe test coverage new/current, TreeHerder]: We now have web platform tests
   for this case.  We already had them for the window.open() cases.
[Risks and why]: See comment 5.  We're taking approach #4 from there.  I believe
   the risks are fairly low, but not entirely negligible.
[String/UUID change made/needed]: None.
Attachment #8789075 - Flags: approval-mozilla-beta?
Attachment #8789075 - Flags: approval-mozilla-aurora?
sorry boris had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=35521319&repo=mozilla-inbound
Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df7cb65bee39
Set the one allowed sandbox navigator even when popups are allowed to escape the sandbox, so the new popup can be navigated by the sandboxed document.  r=smaug
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeaebaf00e95
Backed out changeset df7cb65bee39 for wpt test failures
note comment #12 is just after the backout information since there was a pulsebot problem, so the changeset is backed out and not relanded or so
(In reply to Boris Zbarsky [:bz] from comment #6)
> Created attachment 8789075 [details] [diff] [review]
> A viable option 4 implementation.  Fixes the testcase, at least

I think option 4, is the correct solution, given how most of the spec is written ... even though it's wrong on this one point.

I wish I'd thought through the implications further when I realised what I mentioned on irc (bug 1190641 comment 20) ... sorry.

I suppose one other option would be to allow the initial navigation, but not subsequent ones, in the sandbox escape case.
However, the genie is already out of the bottle, so I'm not sure that would make much difference and it would probably make the spec even more complicated.
Flags: needinfo?(bobowen.code)
(In reply to Boris Zbarsky [:bz] from comment #8)
> Oh, hmm.  That's not great, just on general "follow the spec" grounds.  How
> about I only set it if activeDocSandboxFlags is not 0?
Sounds good.
Flags: needinfo?(bugs)
(In reply to Bob Owen (:bobowen) from comment #16)
> (In reply to Carsten Book [:Tomcat] from comment #11)
> > sorry boris had to back this out for test failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=35521319&repo=mozilla-
> > inbound
> 
> Looks like this might just be missing the rename to helper-1 here:
> https://hg.mozilla.org/integration/mozilla-inbound/file/df7cb65bee39/testing/
> web-platform/tests/html/semantics/embedded-content/the-iframe-element/
> iframe_sandbox_popups_nonescaping-1.html#l15

Confirmed this and as we want to get this landed ASAP, I'll push the amended patch.
Flags: needinfo?(bzbarsky)
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f847d402cd2a
Set the one allowed sandbox navigator even when popups are allowed to escape the sandbox, so the new popup can be navigated by the sandboxed document.  r=smaug
Tracking 51+ for this regression.
Comment on attachment 8789075 [details] [diff] [review]
A viable option 4 implementation.  Fixes the testcase, at least

Once this lands on m-c let's uplift to m-a, m-b, and m-r.
Attachment #8789075 - Flags: approval-mozilla-release+
Attachment #8789075 - Flags: approval-mozilla-beta?
Attachment #8789075 - Flags: approval-mozilla-beta+
Attachment #8789075 - Flags: approval-mozilla-aurora?
Attachment #8789075 - Flags: approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> Comment on attachment 8789075 [details] [diff] [review]
> A viable option 4 implementation.  Fixes the testcase, at least
> 
> Once this lands on m-c let's uplift to m-a, m-b, and m-r.

This will need a rebase ... same rebase for m-a/m-b I think and more to rebase for m-r.

It includes the web platform test manifest, which I'm not familiar with, so I'll leave this for bz.
Flags: needinfo?(bzbarsky)
Marking as a blocker. I am not sure when we will build rc3 but we will certainly wait on the rebase.
> Looks like this might just be missing the rename to helper-1 here:

Indeed.  I thought I had tested all four of the files before pushing, but clearly not.  :(  Thank you for fixing this up.  I'll do the rebases.
OK, I think I know what's going on here.  If the script in the helper-2 iframe runs before the iframe has an nsIFrame (and hence a presshell/prescontext), the click() will be ignored because nsGenericHTMLElement::CheckHandleEventForAnchorsPreconditions will return false.

I can probably replace the click test with one that does a window.open() and then a location.href set.  But it'd be good to test both codepaths...  I'll just do the click off the parent page's onload, by which point we really ought to have a prescontext in the child.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1614e7c150b9
Set the one allowed sandbox navigator even when popups are allowed to escape the sandbox, so the new popup can be navigated by the sandboxed document.  r=smaug
Attachment #8789499 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/1614e7c150b9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Michelle, can your team have a look at testing the new fix, now that it's landed in m-c and aurora? Thank you very much!
Flags: needinfo?(mfunches)
QA Update: Beta, Aurora, Nightly have all been verified for the builds listed here. All is working as expected. about:blank no longer displays. GIPHY is successfully displayed.
Flags: needinfo?(mfunches) → needinfo?(lhenry)
Fantastic, thank you Michelle!
Flags: needinfo?(lhenry)
As part of the 49.0-build3 sign off, we've also managed to confirm that this bug is fixed on 49. Testing covered Windows 7 x86, Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.

Updating status flags based on Comment 35 and our own work.
This has already been fixed, no need to track it now.
You need to log in before you can comment on or make changes to this bug.