[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #27609 [Applications/Tor Browser]: TBA: Evaluate Tor Onion Proxy Library
#27609: TBA: Evaluate Tor Onion Proxy Library
-------------------------------------------------+-------------------------
Reporter: sysrqb | Owner: tbb-
| team
Type: defect | Status:
| needs_revision
Priority: Very High | Milestone:
Component: Applications/Tor Browser | Version:
Severity: Normal | Resolution:
Keywords: tbb-mobile, TBA-a3, | Actual Points:
TorBrowserTeam201902 |
Parent ID: | Points:
Reviewer: | Sponsor:
| Sponsor8
-------------------------------------------------+-------------------------
Changes (by sysrqb):
* status: new => needs_revision
Comment:
Shane, nice patches. I reviewed them and have some comments. I guess some
follow-up PRs on the repo can resolve most of them.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR96
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR117
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR135
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR158
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR169
These are all public methods, should we catch the case where config is
null?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-81e0435044baa197ee1db0ad93a5a940R185
Same as above but for onionProxyContext
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-a5155bb08733d67f397556a9ada17249R110
This methods assigns the resolved torrc to the instance variable, is this
what we want? The comment doesn't say it overwrites the current value
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-187809677e9cf785e207eb5077425815R15
Nit: Is this import necessary?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-5c2594ca8e8f166cdf275dbd29be29e5R20
Is this import needed?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/2ba4a486c6849affbacb012d8d181e1d7b47527d
#diff-81e0435044baa197ee1db0ad93a5a940R331
Nit: Indentation
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f
#diff-17352e51d9de729ccd4d10b27b8e2141R144
Nit: Two spaces
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/44911f6b1f7791947652139ff8432090f3efe914
#diff-9045687b92ef42f4b5a7c35b7479228aR1
It would've been nice if each of these prototypes had a descriptive
comment
Is there a reason getListOfSupportedBridges() returns a String (instead of
a List or Vector)?
Socks5 should be the default SocksPort, should DefaultSettings reflect
this?
Should the *Port() accessors return an int instead of a string?
Starting with "DisableNetwork 1" is usually safer
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8
#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR161
Hmmmm. We shouldn't use Google DNS as the default. (as some background,
see https://medium.com/@nusenu/who-controls-tors-dns-traffic-
a74a7632e8ca). Related to the comment below, I wonder if we actually need
this at all.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8
#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR201
Nit: White space
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8
#diff-9aaca4263fb73e2f8615d49825b318f2R73
Nit: This comment isn't necessarily correct because it's based on `input`,
right?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8
#diff-9aaca4263fb73e2f8615d49825b318f2R138
`getListOfSupportedBridges()` may return null, yes?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8
#diff-9aaca4263fb73e2f8615d49825b318f2R201
Maybe TorSettings.disableNetwork() should be renamed as
TorSettings.shouldDisableNetwork()?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8
#diff-9aaca4263fb73e2f8615d49825b318f2R268
Non-exit relays shouldn't perform DNS queries - but we should confirm
this.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8
#diff-9aaca4263fb73e2f8615d49825b318f2R313
Nit: `s/yo uare/you are/`
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8
#diff-9aaca4263fb73e2f8615d49825b318f2R432
This is saying configuring an explicit SOCKS port may result in Tor auto-
selecting a different port if the configured port is already used? This
seems like it'll result in a bad user experience, don't you think?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/3637342b742e9688265b0c41b052a5c43dcd6bf6
#diff-81e0435044baa197ee1db0ad93a5a940R248
Do you prefer HALT over TERM for a particular reason?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/3637342b742e9688265b0c41b052a5c43dcd6bf6
#diff-81e0435044baa197ee1db0ad93a5a940R397
Nit: These log lines have a leading space? Was that intentional?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/3637342b742e9688265b0c41b052a5c43dcd6bf6
#diff-a8dd49b5a9ec49e701e335d9cb0cf398R20
Nit: Did you use Strings here instead an enum for a reason? Consistency
with Orbot?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/bc26ca5168ba027fbd458adea3e33f09dce9d252
#diff-81e0435044baa197ee1db0ad93a5a940R557
Nit: Is disabling and enabling network important here?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/d8071138faa63130d6eeacb088ae87b96fae385e
Nit: Does wrapping this in try/finally provide something?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/issues/64
I worry this prevents handling a failure case gracefully. Maybe it should
be documented somewhere that if `isRunning()` returns false, then the
developer should call `hasControlConnection()` and handle the situation
where it returns false (controlConnection == null).
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/27609#comment:28>
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