[tor-bugs] #22538 [Applications/Tor Browser]: Changing circuit for page with error switches catch-all circuit instead

[Tor Bug Tracker & Wiki]

#22538: Changing circuit for page with error switches catch-all circuit instead
-------------------------------------------------+-------------------------
 Reporter:  gk                                   |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-linkability,                     |  Actual Points:
  TorBrowserTeam201904                           |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by gk):

 * status:  needs_review => needs_revision
 * keywords:  tbb-linkability, TorBrowserTeam201904R => tbb-linkability,
     TorBrowserTeam201904


Comment:

 Replying to [comment:14 acat]:
 > torbutton uses
 `gBrowser.contentPrincipal.originAttributes.firstPartyDomain` to get the
 active firstPartyDomain to change circuit. This is set to `about.ef2a7dd5
 -93bc-417f-a698-142c3116864f.mozilla` for all about:* pages, including
 about:neterror and about:certerror which are the cases here. So, if I'm
 not wrong, changing the circuit when there is a network or ssl error
 currently changes the circuit for all about:* pages (not the catch-all
 circuit).

 Yes, this behavior is new since Tor Browser 8 (we got that with
 https://bugzilla.mozilla.org/show_bug.cgi?id=1300671).

 > I did not find an obvious way to get the firstPartyDomain causing the
 error directly from gBrowser.*. Here is a patch that gets it from the url
 parameter that about:neterror and about:certerror has, which is the
 original url causing it:
 https://github.com/acatarineu/torbutton/commit/22538

 Nice idea and I think that works for now at least. Some review comments:

 1) We have been thinking about `const`ification of code as much as
 possible (see: #26184) but we are not there yet. Thus, please use `let`
 for now where you don't have fixed strings. (This means everywhere besides
 `ABOUT_URI_FIRST_PARTY_DOMAIN`).

 2) Please put the `const` at the beginning of the file using a similar
 naming schema as the other `const`s we already have.

 3)
 {{{
 +  const knownErrors = ["about:neterror", "about:certerror"];
 +  const origin = gBrowser.contentPrincipal.origin || '';
 +  if (isAboutFPD && knownErrors.some(x => origin.startsWith(x))) {
 +    try {
 }}}
 I think it's not needed to determine `origin` for every domain we are
 requesting but only for those that have the about-firstpartydomain. Thus,
 even if the code is slightly more complex please rewrite that to something
 like
 {{{
 +  if (isAboutFPD) {
 +    let knownErrors = ["about:neterror", "about:certerror"];
 +    let origin = gBrowser.contentPrincipal.origin || '';
 +    if (knownErrors.some(x => origin.startsWith(x))) {
 +      try {
 }}}

 > Another possibility could be forcing a new circuit always when there are
 network or ssl errors, although I'm not sure if that's desirable.

 Hm, there is the risk of burning a bunch of circuits that way which might
 not help if a website is poorly configured. So, I think I'd rather avoid
 that, at least for now.

 > Also not sure if this is the same as
 https://trac.torproject.org/projects/tor/ticket/25670, since I have only
 seen this behaviour either in neterror or certerror.

 I'd assume so. What would be helpful is figuring out whether we'd fix
 #22513 as well with your patch. If so, we should note this in the commit
 message and close that bug as well.

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