refactor isThirdPartyHost and block referer based on base domain #13820

diracdeltas · 2018-04-12T23:18:57Z

fix #13779
fix #11778
also removes TODO for isThirdPartyHost to handle IP addresses and adds more tests

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

Test Plan:

unit tests pass
open Brave, make sure cookie setting is block all or block 3rd party
go to docs.google.com and login
documents should appear
open devtools and go to 'network' tab
on requests to non-google.com domains like gstatic.com, the referer
header should be 'https://gstatic.com' or whatever the domain is, instead of
'https://docs.google.com...'
turn cookie setting to 'allow all'
repeat step 6. now you should see some requests to third party domains where the referer header is 'https://docs.google.com...'

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

fix #13779 fix #13779 also removes TODO for isThirdPartyHost to handle IP addresses and adds tests Test plan: 1. unit tests pass 2. open Brave, make sure cookie setting is block all or block 3rd party 3. go to docs.google.com and login 4. documents should appear 5. open devtools and go to 'network' tab 6. on a request to a non-google.com domain like gstatic.com, the referer header should be 'https://gstatic.com' or whatever the domain is, instead of 'https://docs.google.com...' 7. turn cookie setting to 'allow all' 8. repeat step 6. now the referer header should be 'https://docs.google.com...'

diracdeltas · 2018-04-12T23:27:34Z

js/lib/baseDomain.js

@@ -44,6 +47,11 @@ module.exports.getBaseDomain = function (hostname) {
    return baseDomain
  }

+  // If the hostname is a TLD, return '' for the base domain
+  if (hostname in publicSuffixes) {
+    return ''


previously for an effective TLD like co.uk, this would return co.uk as the base domain. that seems incorrect so i just set it to the empty string.

note that some eTLDs like github.io are resolvable

codecov-io · 2018-04-12T23:58:38Z

Codecov Report

Merging #13820 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #13820      +/-   ##
==========================================
+ Coverage   56.54%    56.6%   +0.06%     
==========================================
  Files         283      283              
  Lines       28798    28808      +10     
  Branches     4774     4776       +2     
==========================================
+ Hits        16284    16307      +23     
+ Misses      12514    12501      -13

Flag	Coverage Δ
#unittest	`56.6% <100%> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
js/lib/baseDomain.js	`92.98% <100%> (+22.61%)`	⬆️
app/filtering.js	`17.79% <100%> (+0.25%)`	⬆️
app/browser/isThirdPartyHost.js	`100% <100%> (+7.14%)`	⬆️

jumde · 2018-04-13T16:39:40Z

app/browser/isThirdPartyHost.js

  }
+
+  if (ip.isV4Format(host1) || ip.isV4Format(host2)) {


@jumde the problem with && is that getBaseDomain thinks google.com.0.1 and 127.0.0.1 have the same base domain. so if either is an IPv4 address, we should do literal string comparison.

darkdh

I was thinking using efficient GURL::DomainIs since we can access it through muon url bindings but it behaves just like our current behavior

diracdeltas · 2018-04-13T22:24:24Z

++ for doing this in muon/chromium in the future.

riastradh-brave · 2018-04-13T23:27:25Z

Fixes #13779 twice, or is there another issue you meant to cite?

riastradh-brave · 2018-04-14T01:38:00Z

Is there a maintained upstream? Should we submit this upstream? Should we be pulling updates from it?

riastradh-brave

Seems reasonable to me. Some noncritical suggestions about comments and tests.

riastradh-brave · 2018-04-13T23:23:23Z

test/unit/app/browser/isThirdPartyhostTest.js

+  })
+  it('Handles IPv4', function () {
+    assert.ok(isThirdPartyHost('172.217.6.46', '173.217.6.46'))
+    assert.ok(!isThirdPartyHost('172.217.6.46', '172.217.6.46'))


Test equivalent decimal representations of IP addresses?

No harm if we say that 127.1 and 127.0.0.1, or 1.2.3.4 and 001.002.003.004, are distinct, but we should be explicitly intentional about it.

I think it's clear from https://github.com/brave/browser-laptop/pull/13820/files#diff-3e19062054041e338c28b2593b82e8d9R23 that this is not intended to handle IP representations that are equivalent but don't have the same string representation for now

or are you suggesting adding a test for the equivalent case returning third party so that it's explicit based on tests what the behavior should be? i can do that

^ probably best to do in a follow-up PR since it's non-critical and adding it here would dismiss the existing reviews

riastradh-brave · 2018-04-13T23:23:53Z

test/unit/app/browser/isThirdPartyhostTest.js

+    assert.ok(!isThirdPartyHost('[2001:db8:85a3::8a2e:370:7334]', '[2001:db8:85a3::8a2e:370:7334]'))
+    assert.ok(!isThirdPartyHost('2001:db8:85a3::8a2e:370:7334', '2001:db8:85a3::8a2e:370:7334'))
+    assert.ok(isThirdPartyHost('[2001:db8:85a3::8a2e:370:7334]', '[2002:db8:85a3::8a2e:370:7334]'))
+    assert.ok(isThirdPartyHost('2001:db8:85a3::8a2e:370:7334', '2002:db8:85a3::8a2e:370:7334'))


Test v4-mapped IPv6 addresses vs equivalent IPv4 addresses?

same as above

riastradh-brave · 2018-04-13T23:25:21Z

app/browser/isThirdPartyHost.js

+ * Checks if two hosts are third party. Subdomains count as first-party to the
+ * parent domain. Uses hostname (no port).
+ * @param {host1} string - First hostname to compare
+ * @param {host2} string - Second hostname to compare


Just to be clear: This is supposed to be the complement of an equivalence relation now, yes? I.e., its complement is supposed to be transitive, reflexive, and symmetric? Can this be noted in the comment?

I'm pretty sure the previous implementation's complement was also transitive, reflexive, and symmetric (at least it was supposed to be), but this refactoring makes those properties more obvious

diracdeltas · 2018-04-17T00:03:13Z

@riastradh-brave updated the issue to not fix the same issue twice, good catch

diracdeltas · 2018-04-17T00:14:05Z

Is there a maintained upstream? Should we submit this upstream? Should we be pulling updates from it?

tools/updatepsl.sh updates the public suffix list from https://publicsuffix.org/list/public_suffix_list.dat.

WRT the code itself, I originally copied it from https://github.com/EFForg/privacybadger/blob/master/src/lib/basedomain.js (because it was a much faster implementation than any of the NPM libraries I found to do the same thing). It looks like they already have an implementation of the IP checks which doesn't depend on Node, so our patch would be redundant. I could pull their fix but it seemed cleaner to use NPM's IP parsing library (which we already use elsewhere) instead of regexes.

diracdeltas · 2018-04-17T00:22:29Z

master / 0.24.x: 5a77c8b

refactor isThirdPartyHost and block referer based on base domain

bsclifton · 2018-05-02T03:50:36Z

Uplifted to 0.23.x with 21b9274

diracdeltas self-assigned this Apr 12, 2018

diracdeltas requested review from bbondy and bsclifton April 12, 2018 23:18

diracdeltas commented Apr 12, 2018

View reviewed changes

jumde reviewed Apr 13, 2018

View reviewed changes

darkdh approved these changes Apr 13, 2018

View reviewed changes

riastradh-brave approved these changes Apr 14, 2018

View reviewed changes

diracdeltas added this to the 0.24.x (Nightly Channel) milestone Apr 17, 2018

diracdeltas merged commit 5a77c8b into master Apr 17, 2018

diracdeltas deleted the fix/etld branch April 17, 2018 00:22

bsclifton mentioned this pull request Apr 26, 2018

Make Brave compatible with GDocs by default (TLD+1 for HTTP Referer header to fix Google sites) #13922

Closed

bsclifton pushed a commit that referenced this pull request May 2, 2018

Merge pull request #13820 from brave/fix/etld

21b9274

refactor isThirdPartyHost and block referer based on base domain

bsclifton modified the milestones: 0.24.x (Nightly Channel), 0.23.x (Developer Channel) May 2, 2018

diracdeltas mentioned this pull request May 2, 2018

block referer based on eTLD+1 instead of full origin #13779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor isThirdPartyHost and block referer based on base domain #13820

refactor isThirdPartyHost and block referer based on base domain #13820

diracdeltas commented Apr 12, 2018 •

edited

Loading

diracdeltas Apr 12, 2018

codecov-io commented Apr 12, 2018 •

edited

Loading

jumde Apr 13, 2018

diracdeltas Apr 13, 2018

darkdh left a comment

diracdeltas commented Apr 13, 2018

riastradh-brave commented Apr 13, 2018

riastradh-brave commented Apr 14, 2018

riastradh-brave left a comment

riastradh-brave Apr 13, 2018

diracdeltas Apr 17, 2018

diracdeltas Apr 17, 2018

diracdeltas Apr 17, 2018

riastradh-brave Apr 13, 2018

diracdeltas Apr 17, 2018

riastradh-brave Apr 13, 2018

diracdeltas Apr 17, 2018

diracdeltas commented Apr 17, 2018

diracdeltas commented Apr 17, 2018

diracdeltas commented Apr 17, 2018

bsclifton commented May 2, 2018

refactor isThirdPartyHost and block referer based on base domain #13820

refactor isThirdPartyHost and block referer based on base domain #13820

Conversation

diracdeltas commented Apr 12, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

codecov-io commented Apr 12, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

diracdeltas commented Apr 13, 2018

riastradh-brave commented Apr 13, 2018

riastradh-brave commented Apr 14, 2018

riastradh-brave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Apr 17, 2018

diracdeltas commented Apr 17, 2018

diracdeltas commented Apr 17, 2018

bsclifton commented May 2, 2018

diracdeltas commented Apr 12, 2018 •

edited

Loading

codecov-io commented Apr 12, 2018 •

edited

Loading