1731739 - Increase Storage Access permission scope to site

Status: RESOLVED FIXED

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

Description

[Johann Hofmann [:johannh]]

See https://github.com/privacycg/storage-access/issues/39

Safari currently scopes SAA grants per site and we've seen breakage on bug 1624853 where access needs to be present on both history.com and play.history.com.

We should align with this behavior to increase compatibility and usefulness of the API.

Comment 1

[Anne (:annevk)]

I realize we are somewhat forced to do this, but I'd like to at least detail the problems here:

1. This requires websites to coordinate storage access grants across their site (i.e., scheme + registrable domain). They will not always be in control of those other domains. Safari already having this model alleviates this, but it's not the kind of model you would choose if you started from scratch.
2. This could be confusing for users. They will get prompted by google.com for Storage Access, but maps.google.com for Geolocation. And furthermore, google.com and maps.google.com have different meanings in this example. google.com means google.com including all its subdomains whereas maps.google.com means just that. We don't have a way of communicating a registrable domain at the moment (other than highlighting in the address bar). This will result in further confusion and difficulty in terms of UX design when it comes to all the various forms of permission management we have.

Comment 2

[Benjamin VanderSloot [:bvandersloot]]

Per discussion in a video call, we actually want to change the scope in the permission itself. i.e. https://example1.com should be granted the permission "3rdPartyStorage^example2.com" rather than "3rdPartyStorage^https://example2.com".

Having more than one origin in the permission (key and value) caused some confusion.

Comment 3

[Anne (:annevk)]

So I do think we should continue to take the scheme into account. A site is a registrable domain (eTLD+1) + a scheme. The scheme doesn't have to be shown to the user, but we should use it internally (and I believe we do already in StoragePrincipal).

Perhaps confusingly, that ends up serializing as https://example2.com as well I believe. (And to be clear, the site of https://www.example2.com would end up as https://example2.com.)

Comment 4

[Benjamin VanderSloot [:bvandersloot]]

Thank you for the clarification Anne. I failed to type what was in my head correctly.

Correcting the above to:

Per discussion in a video call, we actually want to change the scope in the permission itself, i.e. https://www.example1.com should be granted the permission "3rdPartyStorage^https://example2.com" rather than "3rdPartyStorage^https://www.example2.com".

Comment 5

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 1731739 part 1 - Extend eTLD service to have funtions to return the Site of a URI r=#necko-reviewers,#anti-tracking-reviewers

This functionality is a common use of the output of GetBaseDomain.
This patch rolls this functionality into the service, providing a standard implementation, reducing bugs/inconsistency of the use of "Site"
There are two functions provided, GetSchemelessSite and GetSite.
GetSite performs a standard serialization of a URI to its origin's site.
GetSchemelessSite is non-standard but this logic is repeated throughout the codebase.

GetSchemelessSite can remove the redundant logic in the following functions in the codebase:

JS:

• SiteDataManager.getBaseDomainFromHost
• TemporaryPermissions._uriToBaseDomain
• LoginManagerStorage_geckoview.searchLoginsAsync

C++:

• BasePrincipal::GetLocalStorageQuotaKey
• ThirdPartyUtil::GetBaseDomain
• ThirdPartyUtil::GetBaseDomainFromSchemeHost
• ReferrerInfo::HandleUserXOriginSendingPolicy
• CookieCommons::GetBaseDomain
• CookieCommons::GetBaseDomainFromHost
• URLDecorationStripper::StripToRegistrableDomain

Comment 6

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 1731739 part 2 - Use site in 3pStorageAccess key, rather than origin r=#anti-tracking-reviewers

This change has three core pieces, divided up by file:

All changes in the toolkit/antitracking/test folder are changes to get our tests to reflect the new behavior
Remaining JS changes are so frontend code looking at these permissions use the site
All C++ changes are there to support changing AntiTrackingUtils::CreateStoragePermissionKey to produce keys of the form 3rdPartyStorage^{site}

Depends on D129812

Comment 7

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 1731739 part 2 - Use site as key for 3rdPartyStorage permissions, rather than origin r=#anti-tracking-reviewers

• Adding browser tests to verify correct behavior in integration
  • New test that fails on previous version: toolkit/components/antitracking/test/browser/browser_storageAccessScopeSameSiteWrite.js
• Add the ability to store permission by site, use 3rdPartyStorage for this
• No change is made to permission reads. These already proceed recursively, which eventually reach the site.

Comment 8

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 1731739 part 3 - Migrate permissions for 3rdPartyStorage to site keys, r=#anti-tracking-reviewers

• Increment permissions database schema version
• Apply GetSite to origin keys with type prefixed by "3rdPartyStorage^"
  • Done in a transaction
• Add unit test for migration

Depends on D130675

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4a31c29c441
part 1 - Extend eTLD service to have funtions to return the Site of a URI r=necko-reviewers,anti-tracking-reviewers,pbz,valentin
https://hg.mozilla.org/integration/autoland/rev/7b053cfeab3e
part 2 - Use site as key for 3rdPartyStorage permissions, rather than origin   r=anti-tracking-reviewers,pbz,timhuang
https://hg.mozilla.org/integration/autoland/rev/2f723b6e625b
part 3 - Migrate permissions for 3rdPartyStorage to site keys, r=pbz,timhuang

Comment 10

[Marian-Vasile Laza]

Backed out 3 changesets (Bug 1731739) for causing build bustages on PermissionManager.cpp.
Backout link
Push with failures
Failure Log

Comment 11

[Benjamin VanderSloot [:bvandersloot]]

This failed because the return type of PermissionManager::GetAllForPrincipalHelper should have been nsresult because it was defined in PermissionManager.h, rather than NS_IMETHODIMP. Fixing, pushing to phabricator, and attempting to re-land.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97b78a1e9d9e
part 1 - Extend eTLD service to have funtions to return the Site of a URI r=necko-reviewers,anti-tracking-reviewers,pbz,valentin
https://hg.mozilla.org/integration/autoland/rev/c0614d18dc41
part 2 - Use site as key for 3rdPartyStorage permissions, rather than origin   r=anti-tracking-reviewers,pbz,timhuang
https://hg.mozilla.org/integration/autoland/rev/813c32c626e4
part 3 - Migrate permissions for 3rdPartyStorage to site keys, r=pbz,timhuang

Comment 13

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/97b78a1e9d9e
https://hg.mozilla.org/mozilla-central/rev/c0614d18dc41
https://hg.mozilla.org/mozilla-central/rev/813c32c626e4