[tbb-bugs] #19757 [Applications/Tor Browser]: Make a menu to add onion and auth-cookie to TB

[Tor Bug Tracker & Wiki]

#19757: Make a menu to add onion and auth-cookie to TB
-------------------------------------------------+-------------------------
 Reporter:  mrphs                                |          Owner:  brade
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ux-team, tbb-usability, tor-hs,      |  Actual Points:  6.2
  TorBrowserTeam202001R                          |
Parent ID:  #30000                               |         Points:  8
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor27-must
-------------------------------------------------+-------------------------

Comment (by mcs):

 Replying to [comment:25 pospeselr]:
 > This feels a bit fishy to me. We're saving off a list of indices and
 then individually removing them from both {{{this._keyInfoList}}} and
 {{{this._tree}}}. This happens in an async context, so wouldn't it be
 possible for the indices to become invalidated if keys were to be added in
 the middle of a large batch of deletions?

 Thanks for your review! The only other code that modifies or sets
 `this._keyInfoList` is `_loadSavedKeys`, which is only called one time
 (from `_init()`). That means that the code we wrote should not be fragile
 in practice.

 > I *think* a better way to do this would be to first create a list of the
 hsAdresses we want to remove,  do our synchronous work updating
 {{{this._keyInfoList}}} and {{{this._tree}}}, then call
 {{{onionAuthRemove}}} with all of our hsAddresses (assuming they can't all
 be batched together into one call to tor).

 We can only remove one key at a time from using the Tor control protocol,
 so the problem I see with your suggested approach is that we would be
 removing rows from the tree before we know whether tor successfully
 removed them. If you still think what we have now is too fragile, how
 about this instead: we can change the code inside `_deleteOneKey()` to not
 assume that the index stays the same during the
 `aTorController.onionAuthRemove()` call. If that async call returns
 without throwing we know that tor removed the key, and then we would use
 the `.hsAddress` field to re-find the row that needs to be removed from
 `this._keyInfoList`.

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