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

Disable on localhosts #2666

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Disable on localhosts #2666

wants to merge 4 commits into from

Conversation

ablanathtanalba
Copy link
Contributor

@ablanathtanalba ablanathtanalba commented Aug 13, 2020

This fixes #817

Instead of disabling pb entirely on localhosts, this now just removes learning capabilities when the tab origin is from a predictable localhost address.

@ghostwords
Copy link
Member

ghostwords commented Aug 13, 2020

I think it would be better to take the tack discussed in #817 (comment) and disable learning on local domains. This means Privacy Badger will still be enabled and will report domains, do widget replacement, etc.. We just don't want to ever get local domains in snitch_map, you know?

@ghostwords
Copy link
Member

ghostwords commented Aug 13, 2020

So that's for when the site you are on is a local host domain.

What about when local host domains are included as resources on a site?

  • If it's a local host site with local host resources, we should make sure Privacy Badger treats local host resources as first party on local host sites.
  • If it's a non-local site with local host resources ... This is a weird one. No need to specially handle this case at this time, I think. We can revisit later if necessary.

Edit: We currently completely ignore local host domains when it comes to modifying/blocking their requests and reporting them in the UI. I don't think we have a similar check on the learning side, however.

@ablanathtanalba
Copy link
Contributor Author

I think it would be better to take the tack discussed in #817 (comment) and disable learning on local domains. This means Privacy Badger will still be enabled and will report domains, do widget replacement, etc.. We just don't want to ever get local domains in snitch_map, you know?

Ah, this makes sense! I'll go this route, then. Next commit will go this direction 👍

Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

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

There are other learning code paths: review the places incognito.learningEnabled() gets called.

@ghostwords
Copy link
Member

Now that #2679 was merged, this should be rebased on master.

@ablanathtanalba ablanathtanalba marked this pull request as ready for review November 11, 2020 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default to disable privacy badger when domain is localhost or 127.0.0.1
2 participants