1865749 - Storage Access API requires page refresh to read new cookies

Status: RESOLVED FIXED

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

Description

[Barry Pollard]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:109.0) Gecko/20100101 Firefox/119.0

Steps to reproduce:

• Start a New Private Window
• Visit: https://freckle-like-move.glitch.me/
• If a permission pop-up happens then accept it.
• Click on the Settings icon in the URL bar and note that "Cross-site cookies" are allowed for the framed site.
• Click first "Refresh Cookies" button which is an in iframe - notice no Cookies are loaded (because there are no cookies yet, so this is correct)
• Click the link referenced in text above which opens in a new tab
• Submit a value in that new tab. This sets a cookie.
• Return to first tab
• Click first "Refresh Cookies" button - notice no Cookies are loaded.
• Refresh the page
• Notice the cookies are now shown

Actual results:

Third party cookies set and allowed with the Storage Access API were not accessible until the page was reloaded.

Expected results:

When third party cookies are allowed with the Storage Access API they should not require a page refresh to show.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Privacy: Anti-Tracking' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Paul Zühlcke [:pbz]]

Thanks for filing a bug! I can reproduce on Nightly too, also outside of private browsing.
It seems unexpected that the frame wouldn't get the newly set cookies. It already has first-party cookie access. I double checked with executing await document.hasStorageAccess() in the frame context, which returns true. It also doesn't reload so it shouldn't loose storage access.

Running mozregression with the STR results in the following pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=2895842928e98ccf141664d1c03033acebe36706 . Nothing obvious there.

Ben, do you agree that this is unexpected behavior and do you know what might be the root cause?

Comment 3

[Barry Pollard]

Yeah the private browsing comment was just to make the test repeatable easily but agree it's not a private browsing issue. Sorry, I should have made that clear.

FYI, I'm seeing similar issues in Safari (best to use Private Windows there as difficult to clear all caches in Safari IMHO), but I don't think this is intentional despite both browsers seeing this. It's almost like having empty storage on init breaks it and it needs a reload to get out of that situation.

In Chrome this seems to work (full disclosure, I'm on the Chrome Dev Rel team and looking at this API for demos and docs).

Comment 4

[Paul Zühlcke [:pbz]]

FWIW there is a cookie code change in the mozregression range: https://hg.mozilla.org/mozilla-central/rev/33f21aec0e20d39623554816399ee76d13d7e08f

Not sure if that regressed it though.

Comment 5

[Benjamin VanderSloot [:bvandersloot]]

I am able to reproduce as well, including the top-level reload fixing it. This is definitely unexpected behavior- if you have storage access in the frame, you should be touching the same cookie jar as the top-level document.

As for a root cause... that pushlog doesn't show anything that looks obvious to me either. It is about 10 days after the last changes to update the Storage Access API.

Although now that you link that change that looks very suspicious. No broadcasting to a different tab could be a source of this type of error. I had a draft typed here thinking aloud about not grabbing the right principal if the cookie jar is empty but came to a dead end. If a change in the top level is not being broadcast to just-unpartitioned or initially-partitioned-unpartitioned iframes, that would look exactly like this.

We should definitely have a WPT that covers this case though.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Based on comment #2, this bug contains a bisection range found by mozregression. However, the Regressed by field is still not filled.

:pbz, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit BugBot documentation.

Comment 7

[Paul Zühlcke [:pbz]]

I think it's a regression of Bug 1713203. When I revert https://hg.mozilla.org/mozilla-central/rev/33f21aec0e20d39623554816399ee76d13d7e08f and https://hg.mozilla.org/mozilla-central/rev/ba89445b47f1 the STR work as expected. I'll look into it more.

Comment 8

[Paul Zühlcke [:pbz]]

Attachment: WIP: Bug 1865749 - Update mCookieKeysInContent on storage access grant. r=bvandersloot!

Comment 9

[Paul Zühlcke [:pbz]]

I've attached a draft patch for addressing the issue where cookies wouldn't get sent to the iframe (in ContentParent.cpp).

However I'm still seeing issues with the frame not receiving cookies even after reload. The test page code seems to behave correctly. It activates the previously granted storage access with another call to document.requestStorageAccess and then reads document.cookie. Though for some reason that returns "". I've talked about this with Ben a bit on Matrix and we're currently trying to find where the cookie service (parent) sends the cookie list to the child side when storage access is granted. This seems necessary since the content process doesn't have all the cookies and should only get the ones matching the domain / OA combination. Before the storage access activation the key would be https://rainbow-pumped-radiator.glitch.me^partitionKey=%28https%2Cfreckle-like-move.glitch.me%29 (partitioned) and after it would be https://rainbow-pumped-radiator.glitch.me (unpartitioned).

Edit: Here are all the calls accessing the cookie map in the content process: https://searchfox.org/mozilla-central/query/default?q=calls-to%3A%27mozilla%3A%3Anet%3A%3ACookieServiceChild%3A%3AmCookiesMap%27%20depth%3A4

Comment 10

[Paul Zühlcke [:pbz]]

Attachment: Bug 1865749 - Add a test for accessing cookies after storage access grant. r=bvandersloot!

Comment 11

[Paul Zühlcke [:pbz]]

Attachment: Bug 1865749 - Update CookieServiceChild cookie list on storage access grant. r=#cookie-reviewers!,bvandersloot!

Depends on D199265

Comment 12

[Paul Zühlcke [:pbz]]

Following up on comment 9. It looks like there isn't such a mechanism.

The patch in comment 11 adds a call from content to parent process to fetch the updated cookie list on storage access grant. This also updates the mCookieKeysInContent list. It fixes 2 bugs. Both are covered by the test I'm adding in comment 10:

1. The bug STR: A cookie change in another tab doesn't arrive in the iframe.
2. When there is no first party context present (e.g. opened in a separate tab) cookies don't get sent to the iframe on storage access grant. Which means document.cookie does not contain the first party cookies.

While mCookieKeysInContent which leads to (1) was introduced in Bug 1713203, I don't think we should treat this as a regression. The real bug is (2) old. Mozregression was inconclusive, but led me back to versions that didn't even support the Storage Access API.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77f629fe7298
Add a test for accessing cookies after storage access grant. r=bvandersloot,anti-tracking-reviewers
https://hg.mozilla.org/integration/autoland/rev/967745446a3f
Update CookieServiceChild cookie list on storage access grant. r=cookie-reviewers,bvandersloot,valentin

Comment 14

[Serban Stanca [:SerbanS]]

Backed out for causing mochitests failures.

• Backout link

---

• Push with failures - some mochitests
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_blockingIndexedDbInWorkers2.js | Should not have worked without user interaction - false == true - got false, expected true (operator ==)

---

• Push with failures - another mochitests
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_partitionedCookies.js | Should not have worked without user interaction - false == true - got false, expected true (operator ==)

Comment 15

[Paul Zühlcke [:pbz]]

Looks like MozPromise chaining doesn't work like I expected it would. We're running SaveStorageAccessPermissionGranted even when RequestStorageAccessAsyncHelper rejects.

Comment 16

[Paul Zühlcke [:pbz]]

Sorry, I don't think this was backed out correctly, I can see the patches on central.

Comment 17

[Paul Zühlcke [:pbz]]

Hmm, I might be wrong, I can't see the changes on hg.mozilla.org anymore, but they appear in the pushlog.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/834233a67e7e
Add a test for accessing cookies after storage access grant. r=bvandersloot,anti-tracking-reviewers
https://hg.mozilla.org/integration/autoland/rev/554e3deacbd1
Update CookieServiceChild cookie list on storage access grant. r=cookie-reviewers,bvandersloot,valentin

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/834233a67e7e
https://hg.mozilla.org/mozilla-central/rev/554e3deacbd1

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.