[tor-bugs] #13252 [Tor Browser]: Tor Browser on OS X should not store data into the application bundle

[Tor Bug Tracker & Wiki]

#13252: Tor Browser on OS X should not store data into the application bundle
-----------------------------------+--------------------------------
 Reporter:  torosx                 |          Owner:  mcs
     Type:  defect                 |         Status:  needs_revision
 Priority:  Medium                 |      Milestone:
Component:  Tor Browser            |        Version:
 Severity:  Normal                 |     Resolution:
 Keywords:  TorBrowserTeam201603R  |  Actual Points:
Parent ID:  #6540                  |         Points:
 Reviewer:                         |        Sponsor:  None
-----------------------------------+--------------------------------
Changes (by mcs):

 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:57 arthuredelstein]:
 > Replying to [comment:55 mcs]:
 >
 > `xpcom/io/TorFileUtils.cpp`:
 >
 > {{{
 > + // When crawling up the hierarchy, components named "." do not count.
 > }}}
 > Is this a bug in Mozilla's nsILocalFile::GetParent implementation?

 You could argue that it is, but the answer may depend on your point of
 view. Kathy and I commonly encounter dots in the executable path because
 we start from the command line a lot when debugging; most people are not
 affected by this kind of issue. The code in TorFileUtils.cpp is not new
 but relocated from elsewhere.

 > When `useOSLocation`==false, you actively create the "TorBrowser-Data"
 directory:
 > {{{
 > +    rv = tbDataDir->AppendNative(tbDataLeafName);
 > +    NS_ENSURE_SUCCESS(rv, rv);
 > +    bool exists = false;
 > +    rv = tbDataDir->Exists(&exists);
 > +    if (NS_SUCCEEDED(rv) && !exists)
 > +      rv = tbDataDir->Create(nsIFile::DIRECTORY_TYPE, 0700);
 > }}}
 > but it looks like you don't create it when `useOSLocation`==true.

 You are correct. Firefox will create that directory; the only reason we
 try to create TorBrowser-Data when useOSLocation==false is to find out
 whether we can. We will add a comment to explain this.

 > On a possibly related issue, I found the following line confusing:
 > {{{
 > +    nsresult rv = NS_NewNativeLocalFile(EmptyCString(), true,
 > +                                        getter_AddRefs(tbDataDir));
 > }}}
 > I guess this is just for checking that it is possible to write to the
 parent directory where the "TorBrowser-Data" directory will be? A comment
 here might help.

 We will add a comment. That code is the same as some existing Mozilla code
 that is nearby. The call to NS_NewNativeLocalFile() gives us an nsIFile
 object whose path is soon after set to point to the correct location by
 the calls to InitWithFSRef() and AppendNative().

 > `/toolkit/xre/nsXREDirProvider.cpp`:
 >
 > {{{
 > +  // Since the TorBrowser-Data directory may be shared among different
 > +  // installations of the application, embed the app path in the update
 dir
 > +  // so that the update history is partitioned.
 > }}}
 > Is this potentially true for Linux or Windows as well? If I install `Tor
 Browser (FR)` and `Tor Browser (JA)` in the same directory, for example.
 Perhaps that's a pretty unlikely scenario, though.

 It is possible but much less likely on Linux or Windows. When we switch to
 the "side-by-side" approach on those platforms, the TorBrowser-Data
 directory will be created inside the directory that contains the Browser
 directory and startup script/Windows shortcut. Because our packaging
 already includes that top level "container" directory (which was not true
 on Mac OS), Kathy and I do not think people will install another copy of
 Tor Browser inside the same directory.

 > Apart from these issues, this patch looks OK to me.

 Thanks for your review! We will post a new patch with the comment changes
 you suggested.

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