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

removed referrer exception rule for moremorewin.net #166

Merged

Conversation

@pes10k
Copy link
Contributor

pes10k commented Jun 12, 2018

We currently allow referrer headers to be set on moremorewin.net. This site seems abandoned and has a bad cert. This PR removes that domain from the exception list.

Fixes issue brave/brave-browser#327

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
…e dead and down (closes #327)
Copy link
Member

bbondy left a comment

The rule still applies to the http:// scheme though which seems to be still valid. Are you sure this is safe to remove?

@pes10k
Copy link
Contributor Author

pes10k commented Jun 15, 2018

We discussed at the privacy confab and the consensus was that it made sense to remove, since the content on the http site seems to be of little value (images of websites?), the cert on the https site is invalid, and no one had any idea why it was added in the first place or why it'd be useful to send headers to the site

@bridiver
Copy link
Collaborator

bridiver commented Jun 15, 2018

@snyderp @jonathansampson brave/browser-laptop#7407

I'm not advocating that we keep it because I don't think we should disable it for every random site out there, but just for reference

@pes10k
Copy link
Contributor Author

pes10k commented Jun 15, 2018

This site is semi-popular (https://www.alexa.com/siteinfo/popyard.com),

Do we have some criteria for what sites are worth maintaining exceptions for (ie when the privacy benefit is > the breakage cost)?

@bridiver
Copy link
Collaborator

bridiver commented Jun 15, 2018

@snyderp I don't think we have any hard criteria for it, but maybe Alexa is a good metric. @bbondy do we have a paid account so we can see estimated uniques?

@bbondy
Copy link
Member

bbondy commented Jun 15, 2018

not that I know of

@pes10k
Copy link
Contributor Author

pes10k commented Jun 15, 2018

Discussed with @bridiver on Slack, short term next steps for this might be:

  • leave PR in the queue for now, and discuss at next Privacy confab
  • figure out if we can come up for a domain-popularity threshold, and only consider exceptions for sites above that
  • If popyard.com doesn't make the cut, then just remove
  • If popyard.com does, then limit the policy to only allow moremorewin.net to receive referrers from popyard.com
@bbondy
Copy link
Member

bbondy commented Jun 20, 2018

Can you find the original things that landed for browser-laptop to see if there's context on why it was added? Then we can decide from there. I think longer term we want an enhancement to the ad-block filter syntax to dictate default shield settings, and a mode that people can use for be as protective as possible without causing web-compat issues.

@bridiver
Copy link
Collaborator

bridiver commented Jun 20, 2018

@bbondy see browser-laptop issue linked above

@bbondy
Copy link
Member

bbondy commented Jun 20, 2018

I'm ok removing but please post a new tracking issue which keeps track of past exceptions and associated issue links. That way if we create a mode later for be as protective as possible without breaking then we can revisit it.

@pes10k
Copy link
Contributor Author

pes10k commented Jun 20, 2018

Sounds good, created tracking issue here: brave/brave-browser#390

@bbondy
bbondy approved these changes Jun 25, 2018
@bbondy bbondy merged commit c5f9114 into brave:master Jun 25, 2018
NejcZdovc added a commit that referenced this pull request Dec 10, 2018
Rewards summary should now be cleared when wallet is restored
NejcZdovc pushed a commit that referenced this pull request Sep 19, 2019
NejcZdovc added a commit that referenced this pull request Sep 19, 2019
…elcomePage
NejcZdovc added a commit that referenced this pull request Sep 19, 2019
NejcZdovc added a commit that referenced this pull request Sep 19, 2019
…Rewards WelcomePage
NejcZdovc added a commit that referenced this pull request Sep 19, 2019
…Rewards WelcomePage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.