1714180 - Follow-up to SmartBlock phase 2 to improve opt-in isolation

Status: VERIFIED FIXED

(Core :: Privacy: Anti-Tracking, defect)

Description

[Thomas Wisniewski [:twisniewski]]

Right now, when a user opts in for a given shim on one tab, a logic flaw in the code will later allow all shims on subsequently-opened tabs to the same domain. (That is, opting in for just Facebook on a site like Patreon will cause other visits to Patreon to allow not just Facebook, but other trackers which should be shimmed).

While we're here, we should improve the messages sent to the web console to affirm when a given shim is allowed on a new tab due to having been opted-in earlier on that same site. Right now, those messages only appear on the specific visit where the user opts in.

Comment 1

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1714180 - fix a logic flaw in SmartBlock which makes it more permissive than necessary; r?denschub

The bug in question is that if a user opts into allowing one tracker on a given
site for the session, new visits to that site will allow others trackers as
well, not just the expected one.

While we're here, we also improve the web console messages to show the same
"allowing X due to opt in" message on new visits as well, not just the original
one where the user initially opted-in for that shim.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53a738aac3a3
fix a logic flaw in SmartBlock which makes it more permissive than necessary; r=denschub,webcompat-reviewers

Comment 3

[Thomas Wisniewski [:twisniewski]]

Comment on attachment 9224796 [details]
Bug 1714180 - fix a logic flaw in SmartBlock which makes it more permissive than necessary; r?denschub

Beta/Release Uplift Approval Request

• User impact if declined: SmartBlock will be more permissive than necessary with which trackers it allows when users opt to bypass specific protections
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1) open a new private browsing tab.

2. visit https://www.patreon.com/login
3. open the web console developer tool.
4. confirm that these warnings appear:
   a) Google Analytics is being shimmed by Firefox.
   b) Facebook SDK is being shimmed by Firefox.
5. click on the "Continue with Facebook" button in the Patreon tab.
6. confirm that the devtools console has added this message as the popup appears:
   • Facebook SDK is allowed on https://www.patreon.com for this browsing session due to user opt-in.
7. close the Facebook authentication popup.
8. open a second private browsing tab and again visit https://www.patreon.com/login
9. open the web console developer tool.
10. confirm that these warnings appear:
    a) Google Analytics is being shimmed by Firefox.
    b) Facebook SDK is allowed on https://www.patreon.com for this browsing session due to user opt-in.

If the fix is not working, then likely one or both of 10a and 10b will be missing, or both will say they are allowed due to user opt-in.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low risk, as this change only affects SmartBlock, and not in a way which should make it more permissive rather than less.

QE testing is optional, as these STR are being added to our QE test-plan for SmartBlock releases (including of PI-1033 in JIRA for Fx90). A local build has been tested with these STR, and behaves as expected.

• String changes made/needed: none

Comment 4

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/53a738aac3a3

Comment 5

[Julien Cristau [:jcristau]]

Can you clarify what the leave-open is for?

Comment 6

[Thomas Wisniewski [:twisniewski]]

I wasn't sure if we need leave-open for the beta uplift request. If it's not required for that, we can remove it.

Comment 7

[Julien Cristau [:jcristau]]

It's much preferred to let the bug get resolved as the fix hits central, then we can track status properly.

Comment 8

[Simona Badau, Desktop QA]

Verified as fixed using the latest Nightly 91.0a1 on macOS Big Sur 11.4, Ubuntu 20.04, and Windows 10 x64. Verified using the STR from Comment 3 - both the warnings 10a and 10b are properly displayed.

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9224796 [details]
Bug 1714180 - fix a logic flaw in SmartBlock which makes it more permissive than necessary; r?denschub

approved for 90.0b5

Comment 10

[Julien Cristau [:jcristau]]

The extension version bump doesn't apply cleanly on beta because bug 1713635 is only on nightly. What version should I use?

Comment 11

[Thomas Wisniewski [:twisniewski]]

Ah, sorry about that. If it would be easy to do so, we could uplift bug 1713635 first as well? That might save us headaches with the number getting out of sync later. It's also a similarly low-risk patch, and we've tested with it active on nightly already. If that's a good option, I can make an official request to uplift on that bug right away?

Otherwise, using the next minor version number compared to what's on beta should be fine (if it's at 23.3.0, we could go for 23.4.0, for instance). As long as the number goes up for this patch and the one in bug 1714407 (or both land together with one version bump), we should be fine.

Comment 12

[Julien Cristau [:jcristau]]

If bug 1713635 can land independently of changes on the AC side then uplifting that as well seems reasonable.

Comment 13

[Thomas Wisniewski [:twisniewski]]

Yes, we landed the same change to AC independently in https://github.com/mozilla-mobile/android-components/pull/10390

So if that's alright, then we should be alright doing so.

That said, I just noticed that bug 1713635 has a perf-regression filed against it (which doesn't make any sense to me). So if we'd like to avoid uplifting it for now because of that, and just version-bump this one independently, I'll understand :)

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2ee062634bc5

Comment 15

[Julien Cristau [:jcristau]]

I ended up skipping 1713635 and bumping the version from 23.0.0 to 23.1.0, uplifting this and bug 1714407. If we end up uplifting 1713635 after all, we can bump to 23.3.0 to get things back in sync.

Comment 16

[Thomas Wisniewski [:twisniewski]]

Thanks!

Comment 17

[Simona Badau, Desktop QA]

Verified as fixed using the latest Firefox 90 beta 5 on macOS Big Sur 11.4, Ubuntu 20.04, and Windows 10 x64.