Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate WebRTC IP leak protection #2783

Merged
merged 7 commits into from
Jul 14, 2021
Merged

Conversation

ghostwords
Copy link
Member

@ghostwords ghostwords commented Jul 8, 2021

Implements the first part of #2782 (comment).

Since we reuse #2782 as the "learn more" page, need to add a short and clear summary of what's happening and what you could install instead to the top of that page prior to release.

Deprecation notice:

Screenshot from 2021-07-08 17-15-33

@ablanathtanalba
Copy link
Contributor

ablanathtanalba commented Jul 12, 2021

Removing code + settings is my favorite kind of PR 😄
Got a few questions and observations though:

If the 'x' icon to close the overlay message enough to acknowledge the user's having seen the message, do we need the big "I understand" button? Or since the "I understand" button is clearer about what's going on, could we remove the 'x' button? I'm not sure which is better, but I don't think having the two there, including the 'x' button potentially bringing the overlay message back to them when they've already seen it, is the best route.

I'm also wondering if there actually needs to be infrastructure put in place for future deprecated settings. Could this be a temporal solution that after a few releases or so, we eventually remove from the codebase entirely? A couple thoughts for either option:

  • if we do need the infrastructure for future deprecated settings, there is probably a better way to set it up, like a set of tuples with their key names and callbacks (I didn't go too far down this thought path, it might actually take more work to reformat how other settings are propped up so they could be instrumented this way too)
  • if we don't need the setup for future deprecations, it reduces the amount of code needed for this PR but it does add a bit of knowledge creep for outside contributors (we'd have to remember this happened and then refer back to it down the road)

EDIT: I re-tested with Arabic RTL locale on Firefox and it looks good! For posterity:
Screen Shot 2021-07-13 at 12 11 45 PM

@ablanathtanalba
Copy link
Contributor

Also just from a visual standpoint, I played around with formatting it a little bit so that it's not as claustrophobic in the overlay. What do you think:
Screen Shot 2021-07-12 at 3 11 32 PM

It's just this:

.deprecated_setting {
    font-size: 13px;
    color: #555;
}

#webrtc-deprecation-div {
    margin-top: 40px;
}

@ghostwords
Copy link
Member Author

Good call, I removed the "X" (and made the "I understand" button less padded).

I don't understand what you mean by "infrastructure for future deprecated settings". When it's time to remove the WebRTC setting entirely, we'll remove most of the additions in this PR. There is a function called initDeprecations(), which does sound generic, but it's necessary to initialize a couple of private settings (to keep track of whether this is a legacy WebRTC setting user and whether we should show the WebRTC prompt or not). Where do you see extra overhead? (How would you reduce the amount of code needed for this PR?)

I couldn't reproduce the Arabic locale issue.

Copy link
Contributor

@ablanathtanalba ablanathtanalba left a comment

Choose a reason for hiding this comment

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

I understand now, I thought that the initDeprecations() section was also for laying groundwork for future deprecated settings like this. Makes sense to me now 👍

Also the visual changes look good, with the removed x button and the less padded consent button. Definitely less crowded, and to the point.

@ghostwords ghostwords merged commit 52908a5 into master Jul 14, 2021
@ghostwords ghostwords deleted the deprecate-webrtc-protection branch July 14, 2021 21:54
@sickcodes
Copy link

@ghostwords why was this essential protection feature removed?

@ghostwords
Copy link
Member Author

Hi @sickcodes. Please see #2782 for the discussion. The short answer is that this setting doesn't actually do anything useful for most people at this point while confusing users as to what it actually does and breaking websites. Let me know if you have any questions.

@sickcodes
Copy link

Thanks for the reply @ghostwords, the phrase, "doesn't do anything useful for most users," is akin to the, "I have nothing to hide argument."

This also doesn't exclude protection from any unknown 0days in the WebRTC protocol.

Breaking websites is the definition of Privacy Badger; it blocks tracking.

Do you mind reverting this PR added back for the protection of non-most users, at the expense of inconvenience for another subset of users?

@ghostwords
Copy link
Member Author

ghostwords commented Nov 12, 2021

the phrase, "doesn't do anything useful for most users," is akin to the, "I have nothing to hide argument."

I don't know what you mean. Did you read my explanations in #2782? Did you see that uBlock Origin is also removing this setting?

Privacy Badger is meant to be an install-and-forget kind of tool. Breaking websites is an unfortunate side effect of increased privacy that we continually work to minimize. If a setting offers a good tradeoff between privacy and convenience, we will try to add it and enable it by default for all Privacy Badger users. This WebRTC setting does not offer a good tradeoff.

If you'd like to disable WebRTC in your browser, you can look for and install a separate extension that does provide this setting. Let me know if I can help with this.

@ghostwords
Copy link
Member Author

ghostwords commented Nov 12, 2021

To clarify further, Privacy Badger is not the right tool for advanced/power users who want a checkbox for every browser feature, with no regard for the privacy/convenience tradeoff. For example, disabling JavaScript will also improve your privacy. Privacy Badger will not provide this checkbox because Privacy Badger is not that kind of tool.

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

Successfully merging this pull request may close these issues.

3 participants