Re: [tor-bugs] #5463 [BridgeDB]: BridgeDB must GPG-sign outgoing mails

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#5463: BridgeDB must GPG-sign outgoing mails
-----------------------------+----------------------------
     Reporter:  rransom      |      Owner:  isis
         Type:  enhancement  |     Status:  needs_review
     Priority:  normal       |  Milestone:
    Component:  BridgeDB     |    Version:
   Resolution:               |   Keywords:  bridgegb-email
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+----------------------------

Comment (by sysrqb):

 nice job, isis. i'm thankful that your comment `my
 fix/5463-7547-7550-8241-11475-11753-email-rewrite âbranch (which ârewrites
 the entirety of the old lib/bridgedb/EmailServer.py module)` was not
 actually a complete rewrite, otherwise this review would take a lot longer
 :)

 This review doesn't include anything more than a cursory skimming of the
 HTML template modifications or a proper review of the unit tests. Most of
 what follows are a bunch of nitpicky details that are not blockers. A few
 of them should be separate tickets, tbh, too. The only parts that I think
 should be reconsidered are the unconditional logging of email addresses. I
 really think those should be scrubbed except for when safe logging is
 disabled, unless there's a good reason?

 This only includes fix/5463-7547-7550-8241-11475-11753-email-rewrite.
 fix/5463-sign-client-email-addr and fix/5463-gpgme-homedir will
 (hopefully) follow in a few hours.


 - lib/bridgedb/Dist.py:419 - need to scrub email address
 - lib/bridgedb/Dist.py:426 - need to scrub email address
 - lib/bridgedb/Dist.py:428 - need to scrub email address
 - lib/bridgedb/Dist.py:433 - need to scrub email address

 - lib/bridgedb/bridgerequest.py:29 - s/bridges/bridges'/s
 - Bridges.isBlocked() is buggy. It returns true if any of a bridges
 ipaddr:port pair are not known to be blocked in the country. 1) When the
 filter returns true, we don't know which pair, 2) we should somehow mark
 the bridge as unhealthy and only return it if there aren't other options

 - lib/bridgedb/email/request.py - missing file header?

 - determineBridgeRequestOptions(): the presence of "get" defines an email
 as valid? I don't have a significantly better answer except that maybe
 only set it when a valid command is found? Not much is gained by this,
 though.

 - EmailBridgeRequest:withoutBlockInCountry(): Should we support multiple
 country codes per line? space and/or comma separated?

 - Should our response inform the user that we only used the last transport
 specified in their email request?

 - determineBridgeRequestOptions(): There's no harm in setting
 `request.isValid(True)` and `request.wantsKey(True)` prior to raises the
 exception, but is there a reason for doing this?

 - lib/bridgedb/email/server.py:183 - Do we want to unconditionally log the
 client's email address?
 - lib/bridgedb/email/server.py:464 - s/param/var/
 - MailMessage:getClientAddress() - Missing docstring

 - MailMessage:getRecipient() - incoming - DOCDOC, and s/param/var/
 - lib/bridgedb/email/server.py:636 - unconditionally logging client email
 address

 - While the email server is getting an upgrade, should we also bump it to
 use the ESMTPFactory? We won't see any benefit from this, but I wonder if
 there's a reason for us to continue using SMTP.

 - lib/bridgedb/email/server.py:316,328,332 - trailing white space after
 read(), seek(), tell()

 - lib/bridgedb/email/server.py:732 - The schedule was originally going to
 be used to partition the hash-rings so that bridgedb can rotate through
 the bridges over a given period, (obviously) it's not doing this right
 now. If we had higher bridge churn then this would be more useful.

 - lib/bridgedb/email/templates.py:33

 Is there a benefit to
 {{{
         while not len(command) >= 25:
 }}}
 rather than
 {{{
         while len(command) < 25:
 }}}
 ?

 - template.gettext(strings.HOWTO_TBB[1]) %
 strings.EMAIL_SPRINTF[\"HOWTO_TBB1\"] - adds an extra space between the
 words. "the %s Tor"

 - lib/bridgedb/strings.py:116 - s/the word the word/the word/ and
 s/tranlate/translate/

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5463#comment:21>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs