1837969 - StorageAllowedForDocument() shouldn't return ePartitionForeignOrDeny for top-level windows

Status: REOPENED

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

Description

[Tim Huang[:timhuang]]

The StorageAllowedForDocument() will return ePartitionForeignOrDeny for top-level documents. This is incorrect because top-level documents should always have storage access.

This behavior doesn't cause any problems because we still use the NodePrincipal as the storage principal for top-level documents even with ePartitionForeignOrDeny. We should return eAllow if the document is top-level.

Comment 1

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

Set release status flags based on info from the regressing bug 1758745

:twisniewski, since you are the author of the regressor, bug 1758745, could you take a look?

For more information, please visit BugBot documentation.

Comment 2

[Tim Huang[:timhuang]]

I will find someone to work on this. Currently, this isn't a high priority.

Comment 3

[Thomas Wisniewski [:twisniewski]]

FWIW, it seems that just adding an if-statement isn't enough, as it's failing at least one test: https://treeherder.mozilla.org/jobs?repo=try&revision=3943d5820f5d1f57b9f2b1ddca5858f9a53dd7f5&selectedTaskRun=MSTQ8MCBQfuLOgneZxclTA.0

So either I used the wrong check for "is a top level document" or something else may be off.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

While improving the ServiceWorker storage access checks for PBM support in bug 1320796 I ran into this again, I'm going to take this.

(In reply to Thomas Wisniewski [:twisniewski] from comment #3)

FWIW, it seems that just adding an if-statement isn't enough, as it's failing at least one test: https://treeherder.mozilla.org/jobs?repo=try&revision=3943d5820f5d1f57b9f2b1ddca5858f9a53dd7f5&selectedTaskRun=MSTQ8MCBQfuLOgneZxclTA.0

So either I used the wrong check for "is a top level document" or something else may be off.

Hopefully it's the right check, but looking at the patch (inlined below in case try gets reset), I think the problem is that we want to return what the storage access check returned (cookieAllowed), not just eAllow. We expect cookieAllowed to have been calculated to be eAllow in the top-level case because of the third-party-window checks in mozilla::ShouldAllowAccessFor. (I am only able to say this confidently-ish because I am staring at a pernosco trace right now, noting that that this case is a BEHAVIOR_ACCEPT case because browser_antitracking.js forces it regardless of when run in dFPI mode).

 StorageAccess StorageAllowedForDocument(const Document* aDoc) {
+  // Top-level documents should always have storage access. Their
+  // NodePrincipal is used as their storage principal, so this is safe.
+  if (aDoc->IsTopLevelContentDocument()) {
+    return StorageAccess::eAllow;
+  }
+
   StorageAccess cookieAllowed = CookieAllowedForDocument(aDoc);
   if (StaticPrefs::
           privacy_partition_always_partition_third_party_non_cookie_storage() &&
       cookieAllowed > StorageAccess::eDeny) {
     return StorageAccess::ePartitionForeignOrDeny;
   }
   return cookieAllowed;
 }

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1837969 - StorageAllowedForDocument() shouldn't return ePartitionForeignOrDeny for top-level windows. r=#anti-tracking!

This is a slight variation on :twisniewski's prototype fix.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e7b9f87e4a3
StorageAllowedForDocument() shouldn't return ePartitionForeignOrDeny for top-level windows. r=anti-tracking-reviewers,timhuang

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/2e7b9f87e4a3

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/543c21c8e352
Backed out 6 changesets (bug 1837969, bug 1320796) for causing mochitets failures at test_file_blob_upload.html. CLOSED TREE

Comment 9

[Cristina Horotan [:chorotan]]

Backed out as requested by Aryx for causing perma mochitest failures at test_file_blob_upload.html

Backout link

Push with failures

Failure log

Comment 10

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/543c21c8e352