1543314 - Cookies are not immediately propagated to new tabs when cookieBehavior is set to 2

Status: VERIFIED FIXED

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

Description

[Andrea Marchesini [:baku]]

If cookieBehavior is set to 2, and the current website is in the allow list, when that website is loaded in a new process, we don't propagate the cookies correctly.

This happens because, when CookieServiceParent::TrackCookieLoad() is called, we check if the channel is allowed only if its a 3rd party resource:

https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/netwerk/cookie/CookieServiceParent.cpp#143

For top-level channel, we don't check if the storage access is allowed and, because of this, we don't send the cookies to the content process.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: Bug 1543314 - Cookies should be sent to the content process also for first-party channels when cookieBehavior is set to 2, r?ehsan

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 9057175 [details]
Bug 1543314 - Cookies should be sent to the content process also for first-party channels when cookieBehavior is set to 2, r?ehsan

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: AntiTracking
• User impact if declined: Cookies are not correctly propagated to the content process together with the first top-level channel
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This bug was about a storage-access check. The fix is 1 line and it's well covered by tests. Low risk.
• String changes made/needed:

Comment 3

[Pascal Chevrel:pascalc (PTO until April 26)]

Tracking for 67 as there is an uplift request. The bug isn't prioritized though, and the patch hasn't landed on nightly and there is no manual QA requested while the description in comment #0 looks like STR that could be used by QA to reproduce the bug and verify the fix.

I'd like to see this land on Nightly and be verified by QA before taking an uplift. Setting the QE-verify? flag for QA evaluation.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/624c13aec1c3
Cookies should be sent to the content process also for first-party channels when cookieBehavior is set to 2, r=Ehsan

Comment 5

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/624c13aec1c3

Comment 6

[David Olah]

Hi Andrea, can you please help out with some steps, actual and expected results? I tried to reproduce this issue but didn't manage to observe any difference between a build from before the fix and from after it. Thank you!

Comment 7

[Andrea Marchesini [:baku]]

I managed to reproduce this issue with the following STR:

1. I wrote a page that was setting a cookie via document.cookie = "foo=bar". And before this it was printing the current document.cookie value.
2. cookieBehavior set to 2
3. http://localhost:8000 has a custom cookie permission set to 'allow'
4. expose page 1 as: http://localhost:8000 in 1 tab.
5. Open it again in a second tab and check what it's shown. It should show 'foo=bar'.

Without my patch, the second tab should now show 'foo=bar'.

Comment 8

[David Olah]

So, would you attach the page you wrote, please?

Comment 9

[Andrea Marchesini [:baku]]

Attachment: test page

Here what I used to test this bug.

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

David, now that there is a testcase, could you verify this bug on Nightly please? Thanks

Comment 11

[David Olah]

I managed to reproduce the issue on 67.0b13. It is no longer reproducible on Nightly 68.0a1 (2019-04-23), so I'll mark it as verified on 68.

Thanks Andrea!

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9057175 [details]
Bug 1543314 - Cookies should be sent to the content process also for first-party channels when cookieBehavior is set to 2, r?ehsan

Minimal patch that baked on nightly and was verified by QA, uplift approved for 67 beta 14, thanks.

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ee086cda5837

Comment 14

[David Olah]

It is no longer reproducible on Firefox 67.0b14, so I'll mark it as verified on 67.