[tor-bugs] #16620 [Tor Browser]: Transform window.name handling into Firefox patch

[Tor Bug Tracker & Wiki]

#16620: Transform window.name handling into Firefox patch
-------------------------------------------------+-------------------------
 Reporter:  mikeperry                            |          Owner:  mcs
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_information
Component:  Tor Browser                          |      Milestone:
 Severity:  Normal                               |        Version:
 Keywords:  tbb-torbutton-conversion,            |     Resolution:
  TorBrowserTeam201510                           |  Actual Points:
Parent ID:                                       |         Points:
  Sponsor:  SponsorU                             |
-------------------------------------------------+-------------------------
Changes (by brade):

 * status:  needs_review => needs_information
 * keywords:  tbb-torbutton-conversion, TorBrowserTeam201510R => tbb-
     torbutton-conversion, TorBrowserTeam201510


Comment:

 Replying to [comment:5 arthuredelstein]:
 > (Sorry for the delay in reviewing.)
 >
 > I built and tested the C++ patch and it seems to be working as intended.

 Thanks, but unfortunately Mark and I discovered that our patch has some
 problems.  Specifically, window.name is being cleared when a new
 tab/window is opened. This means that, effectively, you cannot set
 window.name via a call to window.open(). We discovered this while working
 on automated tests and will revise the patch to include some special-case
 handling for about:blank (which the JS code we are replacing has).  Sorry
 for the premature review request.

 > would it be possible to use
 > {{{
 > +    nsCOMPtr<nsIDocShellTreeItem> item(this);
 > +    nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(item));
 > }}}
 > ?

 No, QI is not the same things as get_Interface.  While do_QueryInterface()
 returns a variant of the same object, do_GetInterface() typically returns
 a completely different object.  The implementations of those two methods
 are different for docShell objects.

 > As an experiment, I browsed to https://www.torproject.org, opened the
 page's JS console and entered `window.name = "test"`. Then I navigated to
 https://trac.torproject.org. I noticed that `window.name` was reset to an
 empty string. This behavior is different from our isolation policy for
 caches, DOM storage, favicons, etc, where we isolate by base domain. Might
 we want to use ThirdPartyUtil::GetBaseDomain instead of
 CheckSameOriginURI, so that www.torproject.org and trac.torproject.org are
 allowed to share data via `window.name`?

 That is a good question.  Mark and I tried to create a patch that Mozilla
 might accept.  In https://bugzilla.mozilla.org/show_bug.cgi?id=444222,
 there is some support for the "always clear window.name when switching
 origins" approach that our patch implements.  Of course other opinions are
 expressed there as well.

 Replying to [comment:6 gk]:
 > Not sure what "navigated" in this context means.  Are you saying the
 patch behaves differently than specified in 4.5.12 of our design document?

 Not to answer for Arthur, but I think the new behavior is slightly
 different.  The TB design doc says "we reset the window.name property of
 tabs in Torbutton every time we encounter a blank Referer."  Our patch
 clears it every time the security context changes, which means whenever
 the origin changes.  This means that our new patch is even more aggressive
 about clearing window.name.  According to the following comment, this is
 OK:
 https://bugzilla.mozilla.org/show_bug.cgi?id=444222#c71
 But there is a chance that we will introduce compatibility issues with
 some web sites who use (abuse?) window.name to pass info across domains.
 Opinions?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16620#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online