Add a session migration for uphold fingerprinting #14178

bsclifton · 2018-05-18T22:40:59Z

Fixes #14152
Fixes #14204

Lays down foundation for #10488

Auditors: @diracdeltas, @darkdh

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:

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

bsclifton · 2018-05-18T22:43:30Z

app/migrations/20180518_uphold.js

+  let migrationNeeded = false
+
+  try {
+    migrationNeeded = compareVersions(data.lastAppVersion, '0.22.714') !== 1


compareVersions will return -1 if less than 0.22.714 and 0 for 0.22.714. It can only be 1 if greater than 0.22.714 (and in that case, the migration won't run)

diracdeltas · 2018-05-19T00:17:01Z

@bsclifton doesn't #14158 already fix the issue for most users? is this PR just so that users who set some non-default value for FP get set to allowAllFingerprinting?

i'm not sure the latter is desirable

bsclifton · 2018-05-20T23:03:55Z

~~@diracdeltas good question-~~

@srirambv can you please verify that our last 0.22.x Release 4 works UNLESS you have a site setting already set? I agree with Yan- I don't think it would be a good decision to overwrite existing settings

~~1. ex: have profile with live released version- you can visit uphold.com, etc but don't change site settings~~
~~2. Copy that same profile to our latest release (BETA)~~
~~3. Verify that exception for fingerprint is added~~

~~Maybe I mis-understood comment in #14152 (comment). The previous code SHOULD have covered all the cases we wanted~~

bsclifton · 2018-05-21T05:53:36Z

I re-reviewed the code- the default site import only runs once actually:

browser-laptop/app/sessionStore.js

Lines 968 to 979 in ea409ce

    
           if (!data.defaultSiteSettingsListImported) { 
        
             for (var i = 0; i < defaultSiteSettingsList.length; ++i) { 
        
               let setting = defaultSiteSettingsList[i] 
        
               if (!data.siteSettings[setting.pattern]) { 
        
                 data.siteSettings[setting.pattern] = {} 
        
               } 
        
               let targetSetting = data.siteSettings[setting.pattern] 
        
               if (!targetSetting.hasOwnProperty[setting.name]) { 
        
                 targetSetting[setting.name] = setting.value 
        
               } 
        
             } 
        
             data.defaultSiteSettingsListImported = true

@diracdeltas this code is needed... but we shouldn't overwrite the setting if it exists. Let me tweak the code a bit

@diracdeltas

Fixes #14152 Lays down foundation for #10488 Auditors: @diracdeltas, @darkdh

bsclifton · 2018-05-21T06:12:19Z

Code updated! Will only set a value if one isn't set... but will run once for any existing users upgrading from 0.22.714 (or earlier)

diracdeltas · 2018-05-21T23:54:52Z

app/migrations/20180518_uphold.js

+      data.siteSettings[pattern] = {}
+    }
+    let targetSetting = data.siteSettings[pattern]
+    if (targetSetting.fingerprintingProtection == null) {


noting that this will still override the siteSetting if the global setting is set to 'blockAllFingerprinting'.

Fixed with 321b208 😄 👍

bsclifton · 2018-05-22T05:43:25Z

New commit added fixing #14204 - I forgot to include ? in the hostPattern 🤦‍♂️

diracdeltas · 2018-05-22T05:49:27Z

app/migrations/20180518_uphold.js

+  // don't apply if:
+  // - user chooses to block all fingerprinting (global setting)
+  // - user is not upgrading from 0.22.714 or earlier
+  if (data.fingerprintingProtectionAll || !data.lastAppVersion) {


i think you want to check data.fingerprintingProtectionAll && data.fingerprintingProtectionAll.enabled?

After missing this, I fixed this and added some unit tests! 😄 Should cover most of the cases

@diracdeltas

… place Fixes #14204 Auditors: @diracdeltas

diracdeltas

lgtm thanks!

Add a session migration for uphold fingerprinting

NejcZdovc · 2018-05-22T11:28:32Z

master ec60238
0.23 7e6a5cc
0.22 66d75eb

bsclifton added this to the 0.22.x Release 4 (Beta channel) milestone May 18, 2018

bsclifton self-assigned this May 18, 2018

bsclifton requested review from diracdeltas and darkdh May 18, 2018 22:40

bsclifton commented May 18, 2018

View reviewed changes

bsclifton requested a review from srirambv May 18, 2018 22:51

Add a session migration for uphold fingerprinting

1bfb422

Fixes #14152 Lays down foundation for #10488 Auditors: @diracdeltas, @darkdh

diracdeltas reviewed May 21, 2018

View reviewed changes

diracdeltas reviewed May 22, 2018

View reviewed changes

Fix hostPattern and don't apply migration if user has global block in…

2eb1138

… place Fixes #14204 Auditors: @diracdeltas

diracdeltas approved these changes May 22, 2018

View reviewed changes

NejcZdovc merged commit ec60238 into brave:master May 22, 2018

NejcZdovc added a commit that referenced this pull request May 22, 2018

Merge pull request #14178 from bsclifton/session-migration-cleanup

7e6a5cc

Add a session migration for uphold fingerprinting

NejcZdovc added a commit that referenced this pull request May 22, 2018

Merge pull request #14178 from bsclifton/session-migration-cleanup

66d75eb

Add a session migration for uphold fingerprinting

bsclifton deleted the session-migration-cleanup branch May 22, 2018 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a session migration for uphold fingerprinting #14178

Add a session migration for uphold fingerprinting #14178

bsclifton commented May 18, 2018 •

edited by NejcZdovc

Loading

bsclifton May 18, 2018 •

edited

Loading

diracdeltas commented May 19, 2018

bsclifton commented May 20, 2018 •

edited

Loading

bsclifton commented May 21, 2018

bsclifton commented May 21, 2018

diracdeltas May 21, 2018

bsclifton May 22, 2018

bsclifton commented May 22, 2018

diracdeltas May 22, 2018 •

edited

Loading

bsclifton May 22, 2018

diracdeltas left a comment

NejcZdovc commented May 22, 2018

Add a session migration for uphold fingerprinting #14178

Add a session migration for uphold fingerprinting #14178

Conversation

bsclifton commented May 18, 2018 • edited by NejcZdovc Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

bsclifton May 18, 2018 • edited Loading

Choose a reason for hiding this comment

diracdeltas commented May 19, 2018

bsclifton commented May 20, 2018 • edited Loading

bsclifton commented May 21, 2018

bsclifton commented May 21, 2018

diracdeltas May 21, 2018

Choose a reason for hiding this comment

bsclifton May 22, 2018

Choose a reason for hiding this comment

bsclifton commented May 22, 2018

diracdeltas May 22, 2018 • edited Loading

Choose a reason for hiding this comment

bsclifton May 22, 2018

Choose a reason for hiding this comment

diracdeltas left a comment

Choose a reason for hiding this comment

NejcZdovc commented May 22, 2018

bsclifton commented May 18, 2018 •

edited by NejcZdovc

Loading

bsclifton May 18, 2018 •

edited

Loading

bsclifton commented May 20, 2018 •

edited

Loading

diracdeltas May 22, 2018 •

edited

Loading