Persisting tracking protection exceptions for private tabs
Categories
(GeckoView :: General, enhancement, P1)
Tracking
(Not tracked)
People
(Reporter: sebastian, Assigned: droeh)
References
Details
(Whiteboard: [geckoview:m79][geckoview:m80])
Attachments
(1 file)
I am trying to replace the custom tracking exception code in Focus for Android with the GeckoView and Android Components implementation.
One problem I have is persisting those exceptions. This works by AC calling ContentBlockingController.saveExceptionList() and saving the result to disk. On the next app start we restore from disk and pass it back to GeckoView.
Now in Focus everything is a private tab. This works fine with all methods (e.g. checkException()) except saveExceptionList() which does not return exceptions for private sessions.
Looking at the code this seems to be on purpose and this also makes a lot of sense normally in browsers since we wouldn't want to save anything from the private session to disk. But with Focus everything is a private tab and we do want those exceptions to be persisted.
https://searchfox.org/mozilla-central/source/toolkit/components/antitracking/ContentBlockingAllowList.jsm#131-134
I am not sure how to resolve this with the current API. Could we make this configurable so that if I explicitly request it then I'll also get exceptions for private tabs returned by saveExceptionList()?
As a workaround in my branch I am using non-private tabs and delete data manually using the StorageController at the end of a session. But this also feels hacky and fragile and I'd rather use actual private tabs.
|
Reporter | |
Comment 1•4 years ago
|
||
Dylan, what do you think we should do here? :)
|
Assignee | |
Comment 2•4 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian; :pocmo) from comment #1)
Dylan, what do you think we should do here? :)
The technical side of this is easy, of course -- we could expand saveExceptionList and restoreExceptionList to optionally support private browsing exceptions without any issue. But I'm not sure I even agree that the end goal here -- persisting exceptions for Focus -- is a good idea, and I'm a bit surprised to hear that we apparently do it currently! Is there background on how we came to the conclusion that Focus should persist tracking protection exceptions? If not, I think I'd like to discuss this at the next mobile privacy & security meeting before deciding on how/if to support it in GV. What sort of timeline are we looking at here?
|
|
Reporter | |
Comment 3•4 years ago
|
||
Is there background on how we came to the conclusion that Focus should persist tracking protection exceptions?
This has happened a long time ago, so I do not remember the details or whether I was even involved. 😅 But I remember that there were issues with sites that were always broken with TP on and that was annoying in Focus where you had to always disable TP for that site when you loaded it.
Note that the exceptions are added/removed via the TP toggle in the menu. They can also be accessed via settings and cleared all together.
Those are the related issues I have found so far:
- https://github.com/mozilla-mobile/focus-android/issues/957
- https://github.com/mozilla-mobile/focus-android/issues/3921
Looks like this has been the behavior in Focus since end of 2018 already.
If not, I think I'd like to discuss this at the next mobile privacy & security meeting before deciding on how/if to support it in GV. What sort of timeline are we looking at here?
This is blocking us to fully migrate Focus over to our engine component and other AC components. I would like to resolve this as soon as possible. I am open to rediscussing if that feature makes sense, but I would like to find a way to not fully block this work on it.
|
|
Assignee | |
Comment 4•4 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian; :pocmo) from comment #3)
This is blocking us to fully migrate Focus over to our engine component and other AC components. I would like to resolve this as soon as possible. I am open to rediscussing if that feature makes sense, but I would like to find a way to not fully block this work on it.
Alright, I talked this over with Eugen and I suppose it's reasonable to support the API necessary to do this in GV even if I think the particular use-case here is a mistake; it's a pretty simple change, so I should have a patch up soon. I would like to see this decision re-discussed at some point (and to be involved if/when it is), because this definitely has the potential to reveal information about a user's browsing even after deliberately clearing data.
|
|
Reporter | |
Comment 5•4 years ago
|
||
would like to see this decision re-discussed at some point (and to be involved if/when it is), because this definitely has the potential to reveal information about a user's browsing even after deliberately clearing data.
Yeah, let's rediscuss! While hooking up the Focus UI to AC and the latest GeckoView API it became obvious that there are more things that have changed since then. We have many more options (e.g. cryptomining, fingerprinting, other tp categories, ..) and I think it would be helpful if the right "setup" for Focus gets reviewed/discussed. Is this "mobile privacy & security" the right place to start this?
|
||
Comment 6•4 years ago
|
||
I think the best place to start the discussion is the "Anti-tracking Engineering Meeting" as it has people from multiple teams desktop/gv/ac
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 7•4 years ago
|
||
(In reply to Arturo Mejia from comment #6)
I think the best place to start the discussion is the "Anti-tracking Engineering Meeting" as it has people from multiple teams desktop/gv/ac
Good point! Sebastian, I added you to the next anti-tracking engineering meeting (June 23rd). When it gets closer I'll add an item to the agenda to discuss this.
|
|
Assignee | |
Comment 8•4 years ago
|
||
|
||
Updated•4 years ago
|
|
|
Reporter | |
Comment 9•4 years ago
|
||
I just remembered ... since Focus is using release builds of GV. Do you think it is possible to uplift this to Beta to get it in Focus sooner? Merge day is next week, so getting this into Beta now would be perfect. :)
|
|
Assignee | |
Comment 10•4 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian; :pocmo) from comment #9)
I just remembered ... since Focus is using release builds of GV. Do you think it is possible to uplift this to Beta to get it in Focus sooner? Merge day is next week, so getting this into Beta now would be perfect. :)
It'll be a close call, but I'll try to get it into 78 beta if possible; otherwise 79 should certainly be doable.
Also, I rethought the appropriate meeting for re-discussing this design decision and I think the "MOBILE: Privacy & Security" next week (July 1st) is probably a better fit; there's not really anything to clarify here on the technical side of things, it's basically just a product decision. I'll add you to that meeting and try to make sure it doesn't get canceled/postponed (as it often does).
|
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 11•4 years ago
|
||
Follow-up from the "MOBILE: Privacy & Security" meeting: Tanvi said that it would be best, given Focus's role as a privacy-first browser, to not persist exceptions without explicit user permission. If/when we have the time (and if plans for Focus allow it) it would be good to revisit this and add support for optionally persisting some exceptions with a clear message to users about the privacy concerns of doing so.
Vesta, Tanvi recommended running this by you -- are you OK with making tracking protection exceptions temporary rather than persistent in Focus? Let me know if you need more information on this.
Sebastian -- assuming Vesta doesn't have any objections, is there any reason for me to still land this API in GV?
|
|
Reporter | |
Comment 12•4 years ago
|
||
Yes, I think it would be great if we could land and with that complete the migration of Focus to use functionality from AC/GV instead of custom, unmaintained code. This is going to be the last blocker.
Personally I am okay with making the exception temporary, but that may need user messaging and probably input from a product team that doesn't exist around Focus right now. I would love to first have a 1:1 migration before changing the product and removing existing features. I'll NI Vesta here, but I am not sure she has the time to evaluate this right now.
|
||
Comment 13•4 years ago
|
||
Hi all, sorry I didn't review/respond sooner.
This feature was built a while back but I do remember that making the exceptions list persistent was intentional because (1) users complained about some sites not working and needing to turn off TP every time they visited the same site and (2) We only stored the URLs (upon user taking explicit action) but still never retained any site data or cookies or history. The goal was to provide the user with an informed choice for faster/better browsing experience and I remember creating a ticket for making sure we explicitly let users know how this feature behaved but I can't remember if that ever made it into the product before Focus was put on maintenance mode.
So considering all that, I think re-evaluate our position does make sense BUT I'd be cautious about how we do it. We don't want to just erase existing users' exceptions lists without letting them know. Let's confirm our position at the next mobile privacy meeting and in the meantime I will discuss messaging with content strategy.
Just to clarify, this is a blocker for migrating Focus to use AC/GV because currently we don't have the method to save exceptions for private sessions, correct?
|
|
Reporter | |
Comment 14•4 years ago
|
||
Just to clarify, this is a blocker for migrating Focus to use AC/GV because currently we don't have the method to save exceptions for private sessions, correct?
Yeah, correct.
So considering all that, I think re-evaluate our position does make sense BUT I'd be cautious about how we do it.
Agreed. I would also like to complete the migration with 1:1 feature parity and then we can take time to evaluate the feature and decide how to continue and communicate that to users.
Dylan, can we land this patch for now (and if possible uplift)?
|
|
Assignee | |
Comment 15•4 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian; :pocmo) from comment #14)
Just to clarify, this is a blocker for migrating Focus to use AC/GV because currently we don't have the method to save exceptions for private sessions, correct?
Yeah, correct.
So considering all that, I think re-evaluate our position does make sense BUT I'd be cautious about how we do it.
Agreed. I would also like to complete the migration with 1:1 feature parity and then we can take time to evaluate the feature and decide how to continue and communicate that to users.
Dylan, can we land this patch for now (and if possible uplift)?
I've discussed this with Emily and snorp and the consensus is that it doesn't make sense for us to rush to add a work-around API to GV for this. We have an overhaul of the concept of permissions planned for H2 which will include tracking protection exceptions and will give the flexibility needed; anything we land now will end up being an ugly addition that gets deprecated in a matter of months.
Can I ask why the current approach Focus uses (modifying useTrackingProtection in the session settings in onLoadRequest, I believe?) is not workable with up-to-date AC/GV? It seems like it should still be usable even if it is ugly.
|
|
Reporter | |
Comment 16•4 years ago
|
||
Can I ask why the current approach Focus uses (modifying useTrackingProtection in the session settings in onLoadRequest, I believe?) is not workable with up-to-date AC/GV? It seems like it should still be usable even if it is ugly.
On the integration branch we did not just update to the latest AC/GV, we switched to using the same underlying components as Fenix - which depends on GeckoView managing the exceptions. The goal is to get Focus on the same code foundation as Fenix without having all that custom code and hacks.
|
|
Assignee | |
Comment 17•4 years ago
|
||
Closing this bug because the necessary features will be added as part of bug 1654832.
|
||
Comment 18•2 years ago
|
||
Move GeckoView::Tracking Protection bugs to the GeckoView::General component.
Description
•