[tor-bugs] #12872 [BridgeDB]: Know within which country a bridge is located
Tor Bug Tracker & Wiki
blackhole at torproject.org
Wed Jan 7 22:20:34 UTC 2015
#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
More information about the tor-bugs
mailing list