Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

document.requestStorageAccess should consider user explicit settings for unpartitioned data access #186

Merged
merged 5 commits into from
Feb 19, 2024

Conversation

shuranhuang
Copy link
Contributor

@shuranhuang shuranhuang commented Sep 11, 2023

This change tries to make rSA behavior aligns with hSA and user expectations by including a check of whether the user agent allows the document to access unpartitioned data based on user settings, and move early resolve cases, such as top-level case and same-site case, behind the user settings check. Specifically:

  • For the cases where hSA returns true, calling rSA will not show a prompt.
  • For the cases where hSA returns false, calling rSA may or may not show a prompt:
    • If user settings explicitly disallow unpartitioned data access, calling rSA will resolve with "denied" and not show a prompt.
    • If no user settings exist, calling rSA may either resolve with "granted" depending on heuristics and not show a prompt, or show a prompt when there isn't an existing grant.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@shuranhuang shuranhuang marked this pull request as ready for review September 12, 2023 13:43
storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs Outdated Show resolved Hide resolved
@shuranhuang shuranhuang requested a review from annevk October 2, 2023 19:18
Copy link
Collaborator

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again and also the commit that already landed I think this has strayed a bit from #171 (comment) "will setting a non-partitioned cookie work". These commits end up duplicating some of the checks the permission is for which I don't think is good.

That is, taking into account what navigator.cookieEnabled already exposes seems fine, but we shouldn't handle user settings separately from the permission. That leads to too much duplication and room for error.

@johannhof
Copy link
Member

@annevk not sure I understand, the permission state for SAA can be separate from the 3PC access settings that the user has applied, no? Are you saying that they should not be?

@shuranhuang might remember more context on why we're splitting out user settings and permission checks.

@annevk
Copy link
Collaborator

annevk commented Oct 13, 2023

I think I don't understand why they need to be different.

Looking at the two use cases that started this in #171 (comment) the top one requires a change as hasStorageAccess() needs to be able to return false in a top-level context. Similar to cookieEnabled. The second use case however seems equivalent to granting storage access for a particular key.

Maybe @cfredric can weigh in if the second use case has some kind of subtlety that could not be explained by the permission store.

@shuranhuang
Copy link
Contributor Author

shuranhuang commented Oct 13, 2023

Hi @annevk , I agree with you that this PR is not what #171 (comment) directly translated to regarding "will setting a non-partitioned cookie work". But this PR is to make sure the behavior is aligned between the two methods.

From the two examples in #171 (comment), in the first use case, if user blocks all cookies (or blocks cookies on specific sites), document.hasStorageAccess will return false (with the change in #174), then document.requestStorageAccess might be called for getting access, which shows user a prompt that could be misleading, i.e. granting the access does not really unblock cookie access in this context. Checking user settings in requestStorageAccess can help avoid this case.

Does that make sense to you?

@cfredric
Copy link
Contributor

Maybe @cfredric can weigh in if the second use case has some kind of subtlety that could not be explained by the permission store.

I haven't read through this PR, but IIUC, you're asking "why wouldn't we just grant the storage-access permission if requestStorageAccess is called and user settings already allow cookie access"?

If so, the reason I didn't want to do that is to preserve the causality/provenance for why cookies are allowed.

E.g.: suppose the user changes their settings to allow 3P cookie access for some site embed.com, then visits a top-level site that embeds embed.com. embed.com then calls requestStorageAccess, gets a new permission grant that lasts 30 days (e.g.), and accesses their cookies.

Now suppose the user changes their mind and decides they don't want to allow cookie access for embed.com anymore. The user therefore modifies their cookie settings again to remove embed.com from the allowlist. But upon future visits, they find that embed.com can still access 3P cookies since the storage-access permission grant hasn't expired, even though the user settings (not permissions) no longer explicitly allow that access.

In my opinion, that's unfortunate because it means the user agent isn't doing what the user intended. That mismatch can be avoided if requestStorageAccess only creates a new permission grant when it needs to, and avoids creating one when a new permission grant isn't necessary to access cookies.

(In theory we could also avoid the mismatch if we decided that user agents weren't allowed to support cookie settings outside of the storage-access permission, but IMO that is not in the purview of this spec. Better to make this spec account for having multiple sources of cookie access info, instead of mandating that there's only one source of info.)

@annevk
Copy link
Collaborator

annevk commented Oct 16, 2023

I think the latter thing is what I'm expecting. In particular I don't think there's a need to standardize such UI features as they can be entirely up to the user agent, including ensuring that if the user disables a certain website, it isn't enabled access via some other means.

But the one thing that doesn't get us is the top-level change, which is essentially about aligning with cookieEnabled.

@shuranhuang I do think that if cookieEnabled is false, we should return early from requestStorageAccess() as well. Not just hasStorageAccess().

@johannhof
Copy link
Member

@annevk your last sentence sounds like you're okay with what the PR does, then? Or are you saying that we should literally check against https://html.spec.whatwg.org/#dom-navigator-cookieenabled in rSA / hSA?

Personally I don't really like cookieEnabled (it's also not consistent across browsers because e.g. Safari shims it, potentially to avoid breakage) and might prefer to deprecate it.

@shuranhuang shuranhuang requested a review from annevk December 8, 2023 15:26
@johannhof
Copy link
Member

@annevk friendly ping on the above, otherwise I'd be happy to (take another look and) merge this and fix potential follow-ups separately, in the interest of moving forward.

@johannhof johannhof merged commit c957718 into privacycg:main Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants