Enable tracking protection in b2g

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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?
Attachment #8610928 - Flags: feedback?(francois)
(Assignee)

Updated

3 years ago
Blocks: 1154605
(Assignee)

Comment 1

3 years ago
I updated https://people.mozilla.org/~fdesre/ to serve as a test page as well.
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.
Attachment #8610928 - Flags: feedback?(francois)
(Assignee)

Comment 3

3 years ago
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?
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 :)
(Assignee)

Comment 5

3 years ago
You're right, my default profile has a stale list. A fresh desktop profile and the b2g one are matching!
(Assignee)

Comment 6

3 years ago
Created attachment 8613674 [details] [diff] [review]
tracking-protection.patch
Attachment #8610928 - Attachment is obsolete: true
Attachment #8613674 - Flags: review?(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.
Attachment #8613674 - Flags: review?(francois) → review-
(Assignee)

Comment 8

3 years ago
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.
Attachment #8613674 - Attachment is obsolete: true
Attachment #8617591 - Flags: review?(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.
Attachment #8617591 - Flags: review?(francois) → review+
https://hg.mozilla.org/mozilla-central/rev/3c8a737a5f8a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S14 (12june)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 2.2 S14 (12june) → ---
Duplicate of this bug: 1173613
(Assignee)

Comment 14

3 years ago
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
Attachment #8624973 - Flags: review?(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!
Attachment #8624973 - Flags: review?(kgrandon) → review+
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.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Setting ni? to ensure comment 18 is not missed.
Flags: needinfo?(fabrice)
Depends on: 1177553
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?
(Assignee)

Comment 21

3 years ago
(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).
Flags: needinfo?(fabrice)
We should also unset it globally like I mentioned in comment 18.
(Assignee)

Comment 23

3 years ago
(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 ;)
(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.
(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?
Flags: needinfo?(fabrice)
(Assignee)

Comment 26

3 years ago
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.
Flags: needinfo?(fabrice)
Depends on: 1179770
(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.
Depends on: 1180503
So safe browsing is disabled again in bug 1180503. Why was it enabled in the first place, then?
(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.
You need to log in before you can comment on or make changes to this bug.