Tor bridges support [Client part] #13290
Conversation
github-actions
bot
added
the
potential-layer-violation-fixes
|
@diracdeltas @fmarier I guess we would need an official security review? |
yes please |
| static const base::NoDestructor<std::vector<std::string>> kSnowflakeBridges( | ||
| {"snowflake 192.0.2.3:1 2B280B23E1107BB62ABFC40DDCC8824814F80A72"}); | ||
|
|
||
| static const base::NoDestructor<std::vector<std::string>> kObfs4Briges({ |
There was a problem hiding this comment.
where are all these bridge IPs from? not sure if it's worth having the builtins at all because wouldn't they just get blocked soon?
There was a problem hiding this comment.
This is taken from the Tor browser, but I agree that this is not a very good solution, perhaps we should get a "predefined" list of bridges from a file in the component or from some network api.
There was a problem hiding this comment.
If TBB uses these then does that mean that they're not always blocked?
|
|
||
| } // namespace | ||
|
|
||
| class BraveTorTest : public InProcessBrowserTest { |
boocmp
force-pushed
the
tor_bridge
branch
3 times, most recently
from
ff4795e
to
181f4dc
Compare
boocmp
force-pushed
the
tor_bridge
branch
2 times, most recently
from
725ff82
to
fd015ba
Compare
|
@petemill ptal |
| overflow-x: scroll; | ||
| } | ||
| </style> | ||
| <div id="torBridgesControls"> |
There was a problem hiding this comment.
Can you please attach screenshots of this (and the dialog and the addition to the privacy section) to the PR description? This is for the purpose of Design (and QA) review.
There was a problem hiding this comment.
Video uploaded. (See Test plan section)
There was a problem hiding this comment.
This needs some changes for the Settings page, please see specific comments.
In general, we can have the JS class be functional - let it set a state via properties, and have that state be easy to read so that we can see certain events change certain properties (e.g. receive data from server or user -> change specific property). The UI can simply reflect the values of those properties.
| sub-label="$i18n{torUseBridgesDesc}" learn-more-url="https://tb-manual.torproject.org/en-US/bridges/" | ||
| checked="[[isUsingBridges_]]" on-change="onUseBridgesChange_"> | ||
| </settings-toggle-button> | ||
| <settings-radio-group id="brigdesGroup" no-set-pref selectable-elements="settings-collapse-radio-button" |
There was a problem hiding this comment.
Seems like we can use <cr-radio-group> here as settings-radio-group just wraps to add pref binding which isn't being used here?
| switch (this.$.brigdesGroup.selected) { | ||
| case 'useBuiltIn': | ||
| this.UpdateUseBridges_(Usage.USE_BUILT_IN) | ||
| this.OnBuiltInBridgesSelect_() |
There was a problem hiding this comment.
A handler (OnBuiltInBridgesSelect_) that is tied to a UI element (builtInBridgesType) event should probably only be called by that event, otherwise the flow can get confusing. Instead try to set a property and use observers.
This particular function (onUsageSelect_) also can be more minimal - just set the relevant properties that the user has changed from the UI, and let the rest of the class properties and observers handle side-effects.
| switch (this.useBridges_) { | ||
| case Usage.NOT_USED: | ||
| case Usage.USE_BUILT_IN: | ||
| this.$.brigdesGroup.selected = 'useBuiltIn' | ||
| break | ||
| case Usage.USE_REQUESTED: | ||
| this.$.brigdesGroup.selected = 'useRequestedBridges' | ||
| break | ||
| case Usage.USE_PROVIDED: | ||
| this.$.brigdesGroup.selected = 'useProvidedBridges' | ||
| break | ||
| } |
There was a problem hiding this comment.
Referring to DOM elements from the JS part of the component should be last result. The default pattern should be inverted where DOM attributes are bound to properties in the JS.
Looks like you should provide DOM-bindable properties for:
- the value for the brigdesGroup (aside: spelling of
brigdesbridges) - requestedBridges
- providedBridges
For example (but not limited to):
<settings-radio-group selected={{useBridgesValue_}}// property
useBridgesValue_: {
type: String,
computed: 'computeUseBridgesValue_(useBridges_)' // useBridgesValue_ observes useBridges_ changes
},
// ...
// Function
computeUseBridgesValue_() {
switch (this.useBridges_) {
case Usage.NOT_USED:
case Usage.USE_BUILT_IN:
return 'useBuiltIn'
case Usage.USE_REQUESTED:
return 'useRequestedBridges'
case Usage.USE_PROVIDED:
return 'useProvidedBridges'
}
}note that this example will be different if you want to do 2-way binding for the selected attribute. Then you won't even need to on-change handler.
| this.$.useBuiltIn.expanded = true | ||
| this.$.useRequestedBridges.expanded = true | ||
| this.$.useProvidedBridges.expanded = true |
There was a problem hiding this comment.
Can put expanded attribute directly on html element since this is constant and doesn't seem to be changed anywhere
| this.$.providedBridges.oninput = (event) => { | ||
| this.providedBridges_ = this.onProvidedBridgesChange_(event) | ||
| this.handleChanged_() | ||
| } |
There was a problem hiding this comment.
- You don't need reference to DOM element directly, you can set this event in the html
- You don't need a custom JS event for this, you can use 2-way data-binding directly to the value html attribute, e.g.
<textarea value="{{providedBridges_::input}}". Please see brave_sync_code_dialog.html (and JS) for an example of better data binding and more readable flow of data.
|
|
||
| setBridgesConfig_() { | ||
| this.loadedConfig_ = this.getCurrentConfig_() | ||
| this.$.apply.hidden = true |
browser/resources/settings/brave_privacy_page/brave_tor_subpage.js
Outdated
Show resolved
Hide resolved
boocmp
force-pushed
the
tor_bridge
branch
2 times, most recently
from
c913843
to
70d0c37
Compare
Resolves brave/brave-browser#1138
Sec review:
https://github.com/brave/security/issues/858
Desc
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
tor_tp.txt
Settings
Open Privacy & Security
'Tor Windows' should exist:
Disabling 'Private window with Tor' should gray other settings
Enabling Use Bridges should expand additional settings
NOTE: Changing the bridge settings should show 'Apply changes' button.
Built-in bridges:
Request a bridge:
Click on 'Request a New Bridge...' button.
Request dialog with captcha should be displayed.
'Renew' button should renew the captcha.
Providing wrong captcha should renew it.
Captcha is case insensitive.
Providing right captcha should close dialog and insert bridges credentials in the text area
Provide a bridge
Checking bridges:
NOTE: It's useful to open 'brave://tor-internals' and see Logs for activities.
Enable 'Private window with Tor' and launch Tor Window (Alt + Shift + N)
Check brave://components, 'Brave Tor Client Updater' should appear.
Go to settings and enable bridges
Refresh & Check brave://components, 'Brave Pluggable Transports (OS)' should appear
Check OS Task manager, new process should start
'tor-snowflake-brave' for snowflake proxies
'tor-obfs4-brave' for obfs and meek-azure proxies
Disabling bridges should stop the corresponding process and delete component from Profile.
Changing the bridge settings may change your IP address for sites.
Open f.e. https://yandex.com/internet and see on IP and region fields.
After changing the bridge settings the IP may change in a few seconds. (Need to refresh page to see it)
Provide bad bridge (f.e. obfs4 99.124.226.90:9998 8BFDDB7D7D2D4BDD4169170C818B175C6B60F799 cert=fmp+hf7s1QH6Crg+FW39P5KTxy57NCfXs+t1vBrNuYrHGebGtbRrdkKfk+pcgZhBdbrVcw iat-mode=0)
Open Tor window.
Tor connection will get stuck and you will see 'Tor connection failed' in 30 seconds.
Check the 'contact support' link opens page in regular browser (not in the Tor as it was in previous versions)
TP component ids:
windows: dnkcahhmfcanmkjhnjejoomdihffoefm
mac os : einfndjnccmoohcngmlldpmellegjjnk
linux : apfggiafobakjahnkchiecbomjgigkkn
Tor component ids:
windows: cpoalefficncklhjfpglfiplenlpccdb
mac os : cldoidikboihgcjfkhdeidbpclkineef
linux : biahpgbdmdkfgndcmfiipgcebobojjkp