1168658 - Enable tracking protection in b2g

Status: RESOLVED FIXED

Description

[[:fabrice] Fabrice Desré]

Created attachment 8610928 [details] [diff] [review]
wip

François, I'm not sure to have all the pieces together here.

On desktop when loading http://ericschmidt.com/ I see that blocking works because the console logs "The resource at "https://platform.twitter.com/widgets.js" was blocked because tracking protection is enabled." and the twitter handle is unstyled. However on b2g we still load everything.

Any idea what I'm missing?

Comment 1

[[:fabrice] Fabrice Desré]

I updated https://people.mozilla.org/~fdesre/ to serve as a test page as well.

Comment 2

[François Marier [:francois]]

Comment on attachment 8610928 [details] [diff] [review]
wip

Review of attachment 8610928 [details] [diff] [review]:
-----------------------------------------------------------------

Here's another test page: https://people.mozilla.org/~fmarier/tracking-test/

Since tracking protection relies on the Safe Browsing code, are you sure that works on B2G yet? This page should be blocked if you have malware protection turned on: http://itisatrap.org/firefox/its-an-attack.html

Also, until bug 1157081 is fixed, either malware or phishing protection must be turned on in the prefs, you can't just have tracking protection on. I can see you've done that correctly in your patch.

Comment 3

[[:fabrice] Fabrice Desré]

Thanks François. Malware protection works fine with this patch, and using your test page I get a "You have successfully enabled tracking protection. Please continue!" message...

So I'm really not sure why the blocking of twitter js code doesn't work on b2g - I made sure to start with a fresh profile and no http cache. Are we using different lists depending on the UA or platform?

Comment 4

[François Marier [:francois]]

The list is the same on all platform. It comes from:

  https://services.disconnect.me/disconnect-plaintext.json

but we remove these entries:

  https://github.com/mozilla-services/shavar-list-exceptions/blob/master/allow_list

Twitter.com is on our allow list so it shouldn't be blocked. Perhaps what you're seeing on desktop is due to a stale list entry (we currently have a bug on our server implementation which means you may not have received the removal). Have you tried a fresh profile on desktop?

I don't get a shield icon on http://ericschmidt.com/ so it seems like it's working fine on B2G :)

Comment 5

[[:fabrice] Fabrice Desré]

You're right, my default profile has a stale list. A fresh desktop profile and the b2g one are matching!

Comment 6

[[:fabrice] Fabrice Desré]

Created attachment 8613674 [details] [diff] [review]
tracking-protection.patch

Comment 7

[François Marier [:francois]]

Comment on attachment 8613674 [details] [diff] [review]
tracking-protection.patch

Review of attachment 8613674 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ +356,2 @@
>  
> +pref("browser.safebrowsing.debug", true);

I think we should leave this off by default.

@@ +403,5 @@
>  // URL for checking the reason for a malware warning.
>  pref("browser.safebrowsing.malware.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=");
> +
> +// Tracking protection
> +pref("privacy.trackingprotection.enabled", true);

If it's on, it means TP is always on for every page load, Private Browsing mode or otherwise.

While I think that's a good idea on B2G, I think you need a product person to decide what the default should be for this option.

As a first step, perhaps we should add an option in the developer options to toggle this ON and OFF?

@@ +404,5 @@
>  pref("browser.safebrowsing.malware.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=");
> +
> +// Tracking protection
> +pref("privacy.trackingprotection.enabled", true);
> +pref("privacy.trackingprotection.pbmode.enabled", true);

This one is unnecessary if the previous one is true. `pbmode.enabled` means that in Private Browsing, TP is always on.

Comment 8

[[:fabrice] Fabrice Desré]

Created attachment 8617591 [details] [diff] [review]
tracking-protection.patch v2

Addressed review comments. I've let it enabled by default, and we'll decide if we want to change that when doing the UI work in bug 1154605.

Comment 9

[François Marier [:francois]]

Comment on attachment 8617591 [details] [diff] [review]
tracking-protection.patch v2

Review of attachment 8617591 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ +356,3 @@
>  
>  // Prevent loading of pages identified as malware
> +pref("browser.safebrowsing.malware.enabled", true);

BTW, once bug 1157081 has landed we won't need to have these turned ON for tracking protection to work.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/3c8a737a5f8a

Comment 11

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/3c8a737a5f8a

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/bfd82015df48 for causing frequent Gij(10) failures in homescreen_navigation_test.js and edges_gesture_test.js like https://treeherder.mozilla.org/logviewer.html#?job_id=2135010&repo=b2g-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=2134285&repo=b2g-inbound

Comment 14

[[:fabrice] Fabrice Desré]

Created attachment 8624973 [details] [review]
gaia tests patch

I need this gaia prefs to not get weird intermittent reflow errors in Gij(10). Green try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcb249781bfa

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8624973 [details] [review]
gaia tests patch

Very strange that you get those reflow errors, but this looks fine to me. Thanks!

Comment 16

[[:fabrice] Fabrice Desré]

Gaia part:
https://github.com/mozilla-b2g/gaia/commit/311c4e59936a407e64509f54fecb440d8a78e3c8

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/6c1f673f4087

Comment 18

[François Marier [:francois]]

Fabrice, now that Bug 1157081 is fixed, it would be a good idea to flip the Safe Browsing prefs off by default (and test that TP still works) because Safe Browsing is way more data-hungry than TP.

Comment 19

[J. Ryan Stinnett [:jryans]]

Setting ni? to ensure comment 18 is not missed.

Comment 20

[Martijn Wargers (zombie)]

This is why  test_cost_control_data_alert_mobile.py in Gaia UI test is failing. This is sure going to give me a nice bill from my provider while trying to find the regression range for that bug.

Is safe browsing supposed to be on while on a mobile data connection?

Comment 21

[[:fabrice] Fabrice Desré]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #20)
> This is why  test_cost_control_data_alert_mobile.py in Gaia UI test is
> failing. This is sure going to give me a nice bill from my provider while
> trying to find the regression range for that bug.
> 
> Is safe browsing supposed to be on while on a mobile data connection?

Martijn,
You can unset the pref like I did for a couple of other gaia tests (look at the PR).

Comment 22

[François Marier [:francois]]

We should also unset it globally like I mentioned in comment 18.

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to François Marier [:francois] from comment #22)
> We should also unset it globally like I mentioned in comment 18.

I'll be happy to review ;)

Comment 24

[Martijn Wargers (zombie)]

(In reply to Fabrice Desré [:fabrice] from comment #21)
> Martijn,
> You can unset the pref like I did for a couple of other gaia tests (look at
> the PR).

Yes, I know. Question is more, would it be wise to download MBs of data while the user is on a mobile data connection? I don't think so.

Comment 25

[Martijn Wargers (zombie)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #24)
> Yes, I know. Question is more, would it be wise to download MBs of data
> while the user is on a mobile data connection? I don't think so.

Fabrice, I can take care of bug 1177553, but would you consider this a valid bug? I think b2g needs to be careful with data when using mobile data, no?

Comment 26

[[:fabrice] Fabrice Desré]

Yes, I'm surprised that we download so much. We need to investigate with a real profile, ie. not one that is reset all the time to run tests.

Comment 27

[Martijn Wargers (zombie)]

(In reply to Fabrice Desré [:fabrice] from comment #26)
> Yes, I'm surprised that we download so much. We need to investigate with a
> real profile, ie. not one that is reset all the time to run tests.

I filed bug 1179770 with a logcat.

Comment 28

[Martijn Wargers (zombie)]

So safe browsing is disabled again in bug 1180503. Why was it enabled in the first place, then?

Comment 29

[François Marier [:francois]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #28)
> So safe browsing is disabled again in bug 1180503. Why was it enabled in the
> first place, then?

See my review comments in comment 9.