Closed Bug 1543314 Opened 1 year ago Closed 1 year ago

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

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox67 + verified
firefox68 --- verified

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(2 files)

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 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:
Attachment #9057175 - Flags: approval-mozilla-beta?

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.

Flags: qe-verify?
Pushed by amarchesini@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Flags: qe-verify? → qe-verify+
QA Whiteboard: [qa-triaged]

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!

Flags: needinfo?(amarchesini)

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'.

Flags: needinfo?(amarchesini)

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

Flags: needinfo?(amarchesini)
Attached file test page

Here what I used to test this bug.

Flags: needinfo?(amarchesini)

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

Flags: needinfo?(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!

Flags: needinfo?(david.olah)

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.

Attachment #9057175 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1543527
You need to log in before you can comment on or make changes to this bug.