1571234 - Forget About This Site does not delete cookies by base domain

Status: VERIFIED FIXED

(Toolkit :: Data Sanitization, defect, P1)

Description

[gpimple]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Open History, search for a website, right-click on it, select Forget About This Site.

Actual results:

Pages from this site are removed from history, cookies stay in place.

Expected results:

As https://support.mozilla.org/en-US/kb/delete-browsing-search-download-history-firefox#w_remove-a-single-website-from-your-history states, "All history items (browsing and download history, cookies, cache, active logins, passwords, saved form data, exceptions for cookies, images, pop-ups) for that site will be removed."

Comment 1

[Florencia Di Ciocco, NI to vbalducci]

Hi Gpimple,

Thanks for taking the time of opening this bug and being very clear about the steps. However, this issue has already been pointed out on bug 1546295. Please do take a look at it. They are on their way to solving this issue.

Best regards, Flor.

Comment 2

[Johann Hofmann [:johannh]]

That doesn't feel like a dupe of bug 1546295 to me.

Can you reproduce this on a clean profile?

Comment 3

[gpimple]

(In reply to Johann Hofmann [:johannh] from comment #2)

That doesn't feel like a dupe of bug 1546295 to me.

Can you reproduce this on a clean profile?

Yes, it does happen on a clean profile too.
I created a new profile, navigated to cnn.com, closed the tab. Chose "Forget About This Site" in History, then opened Preferencies > Privacy: cookies for cnn.com are still present.

Comment 4

[Johann Hofmann [:johannh]]

[Tracking Requested - why for this release]:
Data Sanitization bug

So, for me, Forget About Site does delete some cookies, but apparently only the edition.cnn.com-specific subdomain cookies, none of the cookies for the cnn.com domain, which is not how it should be.

This seems to have regressed with 1515913 by using removeCookiesFromRootDomain without ensuring that we actually got passed a root domain in ClearDataService.

We should probably track this for 70 to get it fixed quickly. Not sure about 69.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Too late for the 69.0 release, but I'll keep this on the radar for a dot release ride-along if a low-risk patch is available.

Comment 6

[Johann Hofmann [:johannh]]

Attachment: Bug 1571234 - Always delete cookies by base domain in ClearDataService.deleteDataFromHost. r=baku

Comment 7

[Johann Hofmann [:johannh]]

Attachment: Bug 1571234 - Change RemoveCookiesFromRootDomain to RemoveCookiesFromExactHost. r=baku

This will change RemoveCookiesFromRootDomain to not remove cookies from all sub-domains but instead
only remove the single host that is passed. This is necessary to support removing all sub-domain
cookie data in ForgetAboutSite without compromising on the cookie permissions that are supported by
Sanitizer.jsm

As a side effect, this also fixes the issues described in https://bugzilla.mozilla.org/show_bug.cgi?id=1515913#c24
(which were caused by deleting sub-domains without checking their permission).

Sanitizer.jsm should retain its functionality even with this change, because it already collects
all the principals we have to delete individually and tries to delete them (so it would consider
mozilla.org and support.mozilla.org separately).

Comment 8

[Johann Hofmann [:johannh]]

Attachment: Bug 1571234 - Manually collect hosts to delete cookies from in ForgetAboutSite. r=baku

This is made necessary because ClearDataService will not delete sub-domains anymore. It
didn't correctly delete base domains before either, so we just switch to collecting the
correct hosts entirely outside of ClearDataService.

Depends on D45012

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a3d06a4087d
Change RemoveCookiesFromRootDomain to RemoveCookiesFromExactHost. r=baku
https://hg.mozilla.org/integration/autoland/rev/4a9668e42ac0
Manually collect hosts to delete cookies from in ForgetAboutSite. r=baku

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/3a3d06a4087d
https://hg.mozilla.org/mozilla-central/rev/4a9668e42ac0

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something you wanted to nominate for Beta approval (and maybe more)?

Comment 12

[Johann Hofmann [:johannh]]

I think Beta is a good idea (definitely not more). Let's give this a tiny bit more bake time (and it would be great to get QA on Nightly) before uplift.

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 9091037 [details]
Bug 1571234 - Change RemoveCookiesFromRootDomain to RemoveCookiesFromExactHost. r=baku

Beta/Release Uplift Approval Request

• User impact if declined: "Forget About This Site" will not delete enough cookies, resulting in a hypothetical breaking of privacy guarantees
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: - Go to https://edition.cnn.com/

• Close the tab
• Open History
• Right click on the history entry for edition.cnn.com and choose "Forget About this Site"
• Go to about:preferences -> Privacy -> Cookies and Site Data -> Manage Data
• Search for "cnn.com"
• Confirm that there are no entries

• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This is touching data sanitization, which is a bit tricky to get right with many rules and exceptions and unfortunately not as many tests as we should have.
• String changes made/needed: None

Comment 14

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce this issue using Firefox 70.0a1 (BuildId:20190803221448) on Windows 10 64bit.

This issue is verified fixed using Firefox 71.0a1 (BuildId:20190916155843) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 18.04 64bit.

Leaving a ni on myself as a reminder to verify this once it lands in beta.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9091037 [details]
Bug 1571234 - Change RemoveCookiesFromRootDomain to RemoveCookiesFromExactHost. r=baku

Fixes a privacy issue which can cause some cookies to not be deleted when they're supposed to be. Thanks for adding a new test. Approved for 70.0b8.

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/23fb7ac85e3b
https://hg.mozilla.org/releases/mozilla-beta/rev/b5bee0284acf

Comment 17

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 70.0b8 (provided in comment 16) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 18.04.

Pending a possible esr uplift.

Ciprian, I think that this is a good candidate for our "Anti-Tracking" test suite.

Comment 18

[Ciprian Georgiu, Desktop QA]

(In reply to Emil Ghitta, QA [:emilghitta] from comment #17)

Ciprian, I think that this is a good candidate for our "Anti-Tracking" test suite.

Although this seems to be covered by automated tests, I also added a manual regression test case here, in order to cover this scenario.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Sounds like this is a wontfix for ESR68 based on comment 12. Feel free to nominate for uplift if that's incorrect.

Comment 20

[Johann Hofmann [:johannh]]

Hmm. This seems pretty unproblematic so far, I was mostly hesitant to go to release quickly but we could consider ESR. Adding NI to consider uplifting.

Comment 21

[Johann Hofmann [:johannh]]

Comment on attachment 9091037 [details]
Bug 1571234 - Change RemoveCookiesFromRootDomain to RemoveCookiesFromExactHost. r=baku

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Local privacy issues for some users with advanced settings
• User impact if declined: "Forget About This Site" will not delete enough cookies, resulting in a hypothetical breaking of privacy guarantees. This also resolves bug 1515913.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is touching data sanitization, which is a bit tricky to get right and fixing one thing is prone to break something else.

However, by now we have a lot of tests for this behavior (we've been laboring at it for a while) and we added even more tests in bug 1515913 that confirm this is working as expected.

• String or UUID changes made by this patch: None

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9091037 [details]
Bug 1571234 - Change RemoveCookiesFromRootDomain to RemoveCookiesFromExactHost. r=baku

Fixes a privacy regression. Approved for 68.2esr.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/abf80e225448
https://hg.mozilla.org/releases/mozilla-esr68/rev/3d656f297a65

Comment 24

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 68.2.0esr (provided in comment 23) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 18.04 64bit.

Comment 25

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This bug has been identified as part of a pilot on determining root causes of blocking and dot release drivers.

It needs a root-cause set for it. Please see the list at https://docs.google.com/document/d/1FFEGsmoU8T0N8R9kk-MXWptOPtXXXRRIe4vQo3_HgMw/.

Add the root cause as a whiteboard tag in the form [rca - <cause> ] and remove the rca-needed keyword.

If you have questions, please contact :tmaity.

Comment 26

[Johann Hofmann [:johannh]]

Both testing and coding error.