[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:  TorBrowserTeam201603  |  Actual Points:
Parent ID:  #6540                 |         Points:
 Reviewer:                        |        Sponsor:  None
----------------------------------+--------------------------------
Changes (by arthuredelstein):

 * sponsor:   => None


Comment:

 I read through each of the three tor-browser patches carefully and I
 didn't find any problems, although I am pretty unfamiliar with this part
 of the codebase so I may not understand all of the ramifications. Very
 nice work! One question I have for patch
 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e:
 {{{
 --- a/toolkit/mozapps/extensions/AddonManager.jsm
 +++ b/toolkit/mozapps/extensions/AddonManager.jsm
 @@ -42,6 +42,11 @@ const PREF_MATCH_OS_LOCALE            =
 "intl.locale.matchOS";
  const PREF_SELECTED_LOCALE            = "general.useragent.locale";
  const UNKNOWN_XPCOM_ABI               = "unknownABI";

 +#ifdef TOR_BROWSER_VERSION
 +#expand const TOR_BROWSER_VERSION = __TOR_BROWSER_VERSION__;
 +const PREF_EM_LAST_TORBROWSER_VERSION =
 "extensions.lastTorBrowserVersion";
 +#endif
 +
  const UPDATE_REQUEST_VERSION          = 2;
  const CATEGORY_UPDATE_PARAMS          = "extension-update-params";

 @@ -866,6 +871,30 @@ var AddonManagerInternal = {
          this.validateBlocklist();
        }

 +#ifdef TOR_BROWSER_VERSION
 +      // To ensure that extension override prefs are reinstalled into the
 +      // user's profile after each update, set appChanged = true if the
 +      // Mozilla app version has not changed but the Tor Browser version
 +      // has changed.
 +      let tbChanged = undefined;
 +      try {
 +        tbChanged = TOR_BROWSER_VERSION !=
 +
 Services.prefs.getCharPref(PREF_EM_LAST_TORBROWSER_VERSION);
 +      }
 +      catch (e) { }
 +      if (tbChanged !== false) {
 +        // Because PREF_EM_LAST_TORBROWSER_VERSION was not present in
 older
 +        // versions of Tor Browser, an app change is indicated when
 tbChanged
 +        // is undefined or true.
 +        if (appChanged === false) {
 +          appChanged = true;
 +        }
 +
 +        Services.prefs.setCharPref(PREF_EM_LAST_TORBROWSER_VERSION,
 +                                   TOR_BROWSER_VERSION);
 +      }
 +#endif

 }}}

 Is it true here that the TOR_BROWSER_VERSION preprocessor directive
 already contains quotation marks? I noticed in some other places in a
 previous patch Kathy and Mark used

 {{{
 +#ifdef TOR_BROWSER_VERSION
 +# Add double-quotes back on (stripped by JarMaker.py).
 +#expand const TOR_BROWSER_VERSION = "__TOR_BROWSER_VERSION__";
 +#endif
 }}}
 but perhaps the quotes are not stripped on AddonManager.jsm?

 One other observation: in b03f511d38631243fec0e6c5427d9a50e602a762 it
 would be nice to have a doc comment at the head of the
 `migrateOneTorBrowserDataDir` function describing what it does. I also
 have difficulty parsing the comment inside that function:
 {{{
 +    // The destination directory exists, which is expected in the case of
 +    // migration of the browser profile. Set the new directory aside and
 set
 +    // tmpDir to point to the new, temporary location.
 }}}
 Could you make it explicit why an existing destination directory is
 expected in migrating the browser profile? And, should the next sentence
 be something like `Set the old directory aside and set tmpDir to point to
 its new temporary location`? (Sorry if I'm misunderstanding.)

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