Closed Bug 1865749 Opened 5 months ago Closed 2 months ago

Storage Access API requires page refresh to read new cookies

Categories

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

Firefox 119
defect

Tracking

()

RESOLVED FIXED
124 Branch
Tracking Status
firefox-esr115 --- wontfix
firefox120 --- wontfix
firefox121 --- wontfix
firefox122 --- wontfix
firefox123 --- wontfix
firefox124 --- fixed

People

(Reporter: barry, Assigned: pbz)

Details

Attachments

(2 files, 1 obsolete file)

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.

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.

Component: Untriaged → Privacy: Anti-Tracking
Product: Firefox → Core

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?

Flags: needinfo?(bvandersloot)

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

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.

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.

Flags: needinfo?(bvandersloot)
Severity: -- → S3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Assignee: nobody → pbz
Status: NEW → ASSIGNED

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.

Flags: needinfo?(pbz)
Keywords: regression

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.

Flags: needinfo?(pbz)
Regressed by: 1713203

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

Attachment #9368943 - Attachment is obsolete: true

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.

Keywords: regression
No longer regressed by: 1713203
Attachment #9375838 - Attachment description: WIP: Bug 1865749 - Add a test for accessing cookies after storage access grant. r=#anti-tracking! → Bug 1865749 - Add a test for accessing cookies after storage access grant. r=bvandersloot!
Attachment #9375841 - Attachment description: WIP: Bug 1865749 - Update CookieServiceChild cookie list on storage access grant. r=#anti-tracking! → Bug 1865749 - Update CookieServiceChild cookie list on storage access grant. r=#cookie-reviewers!,bvandersloot!
Pushed by pzuhlcke@mozilla.com:
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

Backed out for causing mochitests failures.


  • 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 ==)
Flags: needinfo?(pbz)

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

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

Flags: needinfo?(pbz) → needinfo?(sstanca)

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

Pushed by pzuhlcke@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch

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

Flags: needinfo?(sstanca)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: