Add cookieException for google drive #9879

darkdh · 2017-07-05T21:27:36Z

fixes #9810

Auditors: @diracdeltas, @bsclifton, @bbondy

Test Plan:

Go to google drive
Make sure block 3rd party cookies
You should be able to download file
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.

Test Plan:

Reviewer Checklist:

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

diracdeltas · 2017-07-06T00:22:48Z

I would prefer if this allowed specific cookies or cookie domains instead of all 3p cookies on Facebook and Google Drive. I use both FB and Google Drive with 3p cookies blocked (the default right now) and have never had a problem, so I don't want Brave to automatically change the setting to 'allow'.

diracdeltas · 2017-07-06T21:41:50Z

app/filtering.js

@@ -46,9 +47,6 @@ let initializedPartitions = {}
 const transparent1pxGif = 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7'
 const pdfjsOrigin = `chrome-extension://${config.PDFJSExtensionId}`

-// Third party domains that require a valid referer to work
-const refererExceptions = ['use.typekit.net', 'cloud.typography.com', 'www.moremorewin.net']


diracdeltas · 2017-07-06T21:45:19Z

app/filtering.js

@@ -273,9 +271,15 @@ function registerForBeforeSendHeaders (session, partition) {

      if (cookieSetting === 'blockAllCookies' ||
        isThirdPartyHost(parsedFirstPartyUrl.hostname, parsedTargetUrl.hostname)) {
+        let hasCookieException = false
+        cookieExceptions.forEach((exceptionPair) => {


i think this would be more efficient if cookieExceptions were a map like {firstPartyOrigin: [origins]} instead of a list like [[firstPartyOrigin, origin],...]. but this seems ok for now

this method is HUGE 😛 I'd love to see this (the cookieSetting logic) moved into it's own method so we can easily unit test it. If you don't mind, maybe I can try a rev1 for the test?

@diracdeltas

fixes #9810 Auditors: @diracdeltas, @bsclifton, @bbondy Test Plan: 1. Go to google drive 2. Make sure block 3rd party cookies 3. You should be able to download file

@darkdh

Auditors: @darkdh, @diracdeltas Test Plan: `npm run unittest -- --grep="filtering unit tests"`

bsclifton

@darkdh @diracdeltas I rebased this, pulled the logic out to a method, and then added some unit tests 😄 I was going to give this as feedback after reviewing, but had some free cycles. Let me know what you think!

The unit tests I added help add this coverage (and I think also help prevent regressions):

Statement coverage from 15.63% to 19.05%
Branch coverage from 0% to 7.3%
Function coverage from 0% to 4.17%
Line coverage from 15.63% to 19.05%

bsclifton · 2017-07-07T05:04:24Z

app/filtering.js

@@ -273,9 +271,15 @@ function registerForBeforeSendHeaders (session, partition) {

      if (cookieSetting === 'blockAllCookies' ||
        isThirdPartyHost(parsedFirstPartyUrl.hostname, parsedTargetUrl.hostname)) {
+        let hasCookieException = false
+        cookieExceptions.forEach((exceptionPair) => {


this method is HUGE 😛 I'd love to see this (the cookieSetting logic) moved into it's own method so we can easily unit test it. If you don't mind, maybe I can try a rev1 for the test?

darkdh · 2017-07-07T19:55:55Z

test/unit/app/filteringTest.js

+          assert(cookieException)
+
+          const url = cookieException[0]
+          const firstPartyUrl = cookieException[1]


�The firstPartyUrl should be cookieException[0]. I will make it right in the commit changing the cookie exception to map

@diracdeltas

Auditors: @diracdeltas, @bsclifton

diracdeltas · 2017-07-07T22:09:34Z

app/filtering.js

@@ -220,6 +218,38 @@ function registerForBeforeRedirect (session, partition) {
  })
 }

+module.exports.considerRequestExceptions = (requestHeaders, url, firstPartyUrl, isPrivate) => {


maybe rename this to something more descriptive like applyCookieSetting

diracdeltas · 2017-07-07T22:14:25Z

app/filtering.js

+          firstPartyOrigin !== pdfjsOrigin && !hasCookieException) {
+        requestHeaders['Cookie'] = undefined
+      }
+      if (cookieSetting !== 'blockAllCookies' &&


i think the cookieSetting !== 'blockAllCookies' part is a bug in the original code and should be omitted. the referer should be blocked when the cookieSetting is 'blockAllCookies'.

diracdeltas · 2017-07-07T22:16:48Z

js/data/siteHacks.js

-  ['https://inbox.google.com', 'https://hangouts.google.com'],
-  ['https://mail.google.com', 'https://hangouts.google.com']
-]
+module.exports.cookieExceptions = {


this will probably need to be a map of domains to list, like 'https://inbox.google.com': [...] because we may need multiple cookie exception origins for the same top-level origin

diracdeltas · 2017-07-07T22:19:23Z

thanks for adding tests!

diracdeltas · 2017-07-07T22:58:38Z

this doesn't seem to fix the cookie issue on drive.google.com for me. i don't see any requests to https://doc-0g-3g-docs.googleusercontent.com, only https://lh*.googleusercontent.com

darkdh · 2017-07-08T00:06:43Z

@diracdeltas I downloaded .pdf, .html, .txt, .mp4 files from google drive and it will send request to doc-*-3g-doc.googleusercontent.com
I think we need wildcard on this

If you blocked [*.]googleusercontent.com on Chrome, and doing the steps listed above, you can see the download failed just same as we see in Brave

diracdeltas · 2017-07-13T22:43:17Z

the wildcard fixes the issue for me 👍

Add cookieException for google drive

luixxiul · 2017-09-09T15:08:21Z

@darkdh should this fix #8601 as well?

darkdh · 2017-09-09T21:07:08Z

@luixxiul, no they are different. We are missing something on #8601 because you can see other non-chromium based browsers don't have the option(ex. Firefox).

darkdh self-assigned this Jul 5, 2017

darkdh requested review from bbondy and bsclifton July 5, 2017 21:27

darkdh mentioned this pull request Jul 5, 2017

Facebook Messenger Error -- Cannot Send Image/File #698

Closed

bsclifton requested a review from diracdeltas July 6, 2017 04:59

darkdh force-pushed the default_unblock_3rd_cookies branch from ac891d8 to dc208c1 Compare July 6, 2017 19:26

darkdh changed the title ~~Default disable 3rd party cookies for these sites~~ Add exception of 3rd party cookies for these sites Jul 6, 2017

darkdh added the PR/work-in-progress ⚒ label Jul 6, 2017

darkdh removed request for bbondy, bsclifton and diracdeltas July 6, 2017 20:01

darkdh force-pushed the default_unblock_3rd_cookies branch from dc208c1 to 3e1bc32 Compare July 6, 2017 20:24

darkdh changed the title ~~Add exception of 3rd party cookies for these sites~~ Add cookieException for google drive Jul 6, 2017

darkdh removed the PR/work-in-progress ⚒ label Jul 6, 2017

darkdh requested review from diracdeltas, bbondy and bsclifton July 6, 2017 20:25

diracdeltas reviewed Jul 6, 2017

View reviewed changes

bsclifton requested a review from diracdeltas July 7, 2017 04:37

darkdh and others added 2 commits July 6, 2017 21:42

Add cookieException for google drive

d4165c9

fixes #9810 Auditors: @diracdeltas, @bsclifton, @bbondy Test Plan: 1. Go to google drive 2. Make sure block 3rd party cookies 3. You should be able to download file

Broke the cookie/referer check out to a function + wrote unit tests :)

695f0f8

Auditors: @darkdh, @diracdeltas Test Plan: `npm run unittest -- --grep="filtering unit tests"`

bsclifton force-pushed the default_unblock_3rd_cookies branch from 3e1bc32 to 695f0f8 Compare July 7, 2017 06:07

bsclifton approved these changes Jul 7, 2017

View reviewed changes

bsclifton added this to the 0.19.x (Developer Channel) milestone Jul 7, 2017

bsclifton added QA/test-plan-specified release-notes/include webcompat labels Jul 7, 2017

darkdh commented Jul 7, 2017

View reviewed changes

Change cookieExceptions to map

8b624df

Auditors: @diracdeltas, @bsclifton

diracdeltas reviewed Jul 7, 2017

View reviewed changes

Using wildcard pattern

b794367

bsclifton requested review from diracdeltas and bsclifton July 25, 2017 16:18

bsclifton approved these changes Jul 25, 2017

View reviewed changes

bsclifton merged commit edf2d34 into master Jul 25, 2017

bsclifton deleted the default_unblock_3rd_cookies branch July 25, 2017 16:45

bsclifton added a commit that referenced this pull request Jul 25, 2017

Merge pull request #9879 from brave/default_unblock_3rd_cookies

430025e

Add cookieException for google drive

luixxiul mentioned this pull request Jul 25, 2017

Fix lint errors introduced with #9879 #10128

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cookieException for google drive #9879

Add cookieException for google drive #9879

darkdh commented Jul 5, 2017 •

edited by bsclifton

Loading

diracdeltas commented Jul 6, 2017

diracdeltas Jul 6, 2017

diracdeltas Jul 6, 2017 •

edited

Loading

bsclifton Jul 7, 2017

bsclifton left a comment •

edited

Loading

bsclifton Jul 7, 2017

darkdh Jul 7, 2017

diracdeltas Jul 7, 2017 •

edited

Loading

diracdeltas Jul 7, 2017 •

edited

Loading

diracdeltas Jul 7, 2017 •

edited

Loading

diracdeltas commented Jul 7, 2017

diracdeltas commented Jul 7, 2017 •

edited

Loading

darkdh commented Jul 8, 2017

diracdeltas commented Jul 13, 2017

luixxiul commented Sep 9, 2017

darkdh commented Sep 9, 2017

Add cookieException for google drive #9879

Add cookieException for google drive #9879

Conversation

darkdh commented Jul 5, 2017 • edited by bsclifton Loading

diracdeltas commented Jul 6, 2017

diracdeltas Jul 6, 2017

Choose a reason for hiding this comment

diracdeltas Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

bsclifton Jul 7, 2017

Choose a reason for hiding this comment

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

bsclifton Jul 7, 2017

Choose a reason for hiding this comment

darkdh Jul 7, 2017

Choose a reason for hiding this comment

diracdeltas Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

diracdeltas Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

diracdeltas Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

diracdeltas commented Jul 7, 2017

diracdeltas commented Jul 7, 2017 • edited Loading

darkdh commented Jul 8, 2017

diracdeltas commented Jul 13, 2017

luixxiul commented Sep 9, 2017

darkdh commented Sep 9, 2017

darkdh commented Jul 5, 2017 •

edited by bsclifton

Loading

diracdeltas Jul 6, 2017 •

edited

Loading

bsclifton left a comment •

edited

Loading

diracdeltas Jul 7, 2017 •

edited

Loading

diracdeltas Jul 7, 2017 •

edited

Loading

diracdeltas Jul 7, 2017 •

edited

Loading

diracdeltas commented Jul 7, 2017 •

edited

Loading