Skip to content

removed referrer exception rule for moremorewin.net #166

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

Merged
merged 1 commit into from
Jun 25, 2018
Merged

removed referrer exception rule for moremorewin.net #166

merged 1 commit into from
Jun 25, 2018

Conversation

pes10k
Copy link
Contributor

@pes10k 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

Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

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

@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

@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 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
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.

3 participants