[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #12872 [BridgeDB]: Know within which country a bridge is located

#12872: Know within which country a bridge is located
--------------------------+---------------------------------
     Reporter:  sysrqb    |      Owner:  isis
         Type:  defect    |     Status:  accepted
     Priority:  blocker   |  Milestone:
    Component:  BridgeDB  |    Version:
   Resolution:            |   Keywords:  bridgedb-dist, easy
Actual Points:            |  Parent ID:
       Points:            |
--------------------------+---------------------------------

Comment (by isis):

 Replying to [comment:5 pagea]:
 > Review & constructive criticism on the patch is welcome; this patch is
 my first contribution to this project and I fully expect to have to make
 some changes to make it suitable for merging.

 Thanks a lot! For future reference, I reviewed pagea's patch, reporting
 potential issues as I found them on IRC. Here's the relevant things I
 pointed out on IRC:

  * In L62 and L60 in geo.py, you probably want to double-check that
 `geoip` and `geoipv6` are not None, otherwise L65 will produce an
 `AttributeError`.
  * If `ip` in L65 in geo.py is an `ipaddr.IPAddress`, then I believe that
 you'll need to pass `str(ip)`, rather than the actual class as in
 `db.country_code_by_addr(ip)`, so use `db.country_code_by_addr(str(ip))`
 insteadâ or actually `ip.compressed` or `ip.exploded` is probably safer.
 (I don't remember if `ip.__str__() == ip.compressed`.)
  * In geo.py, `logSafely()` is imported from `bridgedb.safelogging`, but
 not used.

 Just for your future information, if you're logging an IP address, or an
 email address (or anything which looks sufficiently like one), and it is
 surrounded by whitespace characters, then BridgeDB's logger will
 automatically sanitise it if SAFELOGGING is enabled, without you needing
 to call logSafely().

  * In L672 in HTTPServer.py, you might want to check that `ip is not
 None`, or an empty `str`, or whatever other things which
 `twisted.web.http.Request.getClientIP()` might decide to return to you.
 You can also use `bridgedb.parse.addr.isIPAddress(ip, compressed=False)`
 as a way to ensure that the IP address is valid and that it's definitely
 an instance of `ipaddr.IPAddress`.
  * Tiny whitespace nitpicks: don't remove the second newline from L801 in
 HTTPServer.py, and put four more spaces before the word "reported" on L123
 in HTTPServer.py.

 But overall it looks excellent :D

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12872#comment:6>
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