Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

add advanced webrtc IP handling preference #13713

Merged
merged 1 commit into from
Apr 11, 2018
Merged

add advanced webrtc IP handling preference #13713

merged 1 commit into from
Apr 11, 2018

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Apr 4, 2018

fix #13668

Test Plan:

  1. go to about:preferences#advanced
  2. at the bottom, it should show a webrtc policy select menu which defaults
    to 'default'
  3. turn on fingerprinting protection to 'block all'
  4. go to https://browserleaks.com/webrtc. it should not show any IPs
  5. turn off fingerprinting protection on that page. now it should show IPs
  6. in about:preferences#advanced, set webrtc policy to 'default public
    interface only'
  7. reload https://browserleaks.com/webrtc. it should only show the
    public IP.
  8. in about:preferences#advanced, set webrtc policy to 'disable
    non-proxied UDP'
  9. reload https://browserleaks.com/webrtc. it should show no IPs.
  10. in about:preferences#advanced, set webrtc policy to 'default public
    and private interfaces'
  11. reload https://browserleaks.com/webrtc. it should show both IPs.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@diracdeltas diracdeltas requested a review from bsclifton April 4, 2018 00:56
@codecov-io
Copy link

codecov-io commented Apr 4, 2018

Codecov Report

Merging #13713 into master will decrease coverage by 0.02%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master   #13713      +/-   ##
==========================================
- Coverage   56.53%   56.51%   -0.03%     
==========================================
  Files         282      283       +1     
  Lines       28747    28756       +9     
  Branches     4764     4765       +1     
==========================================
- Hits        16253    16251       -2     
- Misses      12494    12505      +11
Flag Coverage Δ
#unittest 56.51% <66.66%> (-0.03%) ⬇️
Impacted Files Coverage Δ
js/constants/settings.js 100% <ø> (ø) ⬆️
js/constants/appConfig.js 100% <100%> (ø) ⬆️
js/constants/webrtcConstants.js 100% <100%> (ø)
app/renderer/components/preferences/advancedTab.js 48.43% <50%> (+0.89%) ⬆️
app/browser/reducers/tabsReducer.js 38.05% <62.5%> (-1.89%) ⬇️
js/state/siteSettings.js 92.71% <0%> (-0.67%) ⬇️

@diracdeltas diracdeltas self-assigned this Apr 5, 2018
@bsclifton bsclifton added this to the 0.22.x Release 3 milestone Apr 5, 2018
fix #13668

Test Plan:
1. go to about:preferences#advanced
2. at the bottom, it should show a webrtc policy select menu which defaults
   to 'default'
3. turn on fingerprinting protection to 'block all'
4. go to https://browserleaks.com/webrtc. it should not show any IPs
5. turn off fingerprinting protection on that page. now it should show IPs
6. in about:preferences#advanced, set webrtc policy to 'default public
   interface only'
7. reload https://browserleaks.com/webrtc. it should only show the
   public IP.
8. in about:preferences#advanced, set webrtc policy to 'disable
   non-proxied UDP'
9. reload https://browserleaks.com/webrtc. it should show no IPs.
10. in about:preferences#advanced, set webrtc policy to 'default public
    and private interfaces'
11. reload https://browserleaks.com/webrtc. it should show both IPs.
@bsclifton
Copy link
Member

@diracdeltas @bradleyrichter does it make sense to have this new setting on the Advanced page? Or would it make more sense to be on the Security page? Doesn't seem like an intuitive place to find it

@diracdeltas
Copy link
Member Author

@bsclifton i put it in advanced because probably 99.9% of users won't know what the options mean or care to change the default. it could go either way. it's more of a networking setting than a security setting.

@bsclifton
Copy link
Member

Clicking the What do these policies mean links me to https://cs.chromium.org/chromium/src/content/public/common/webrtc_ip_handling_policy.h

Looking at the comments in the code, I'm able to map these constants (and the comments above them) to what we have in our drop-down... but I think it might be very difficult for users to do so (unless they're also programmers).

I think it's good for a rev1 though- just something to keep in mind. Can definitely be improved in iterations 😄

@diracdeltas
Copy link
Member Author

@bsclifton noted. that's partly why i put it in advanced. :)

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes work great per test plan 😄 Code changes look great and unit tests pass and accurately cover the change

@bsclifton bsclifton merged commit 0640d1e into master Apr 11, 2018
@bsclifton bsclifton deleted the fix/13668 branch April 11, 2018 18:14
bsclifton added a commit that referenced this pull request Apr 11, 2018
add advanced webrtc IP handling preference
bsclifton added a commit that referenced this pull request Apr 11, 2018
add advanced webrtc IP handling preference
@bsclifton
Copy link
Member

master 0640d1e
0.23.x 290aeae
0.22.x efb6542

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Add an option to disable WebRTC in about:preferences
3 participants