privacycg / storage-access

The Storage Access API
https://privacycg.github.io/storage-access/
199 stars 26 forks source link

Synchronous hasStorageAccess? #146

Closed johannhof closed 1 year ago

johannhof commented 1 year ago

Given the changes in #138 and #141 it seems like we can access a static flag on the window/environment to determine storage access. If I'm not mistaken this would enable a sync hasStorageAccess call without the promise. If this is useful for developers we could consider adding a new attribute and deprecating regular Promise-based hSA?

bvandersloot-mozilla commented 1 year ago

I like making this sync.

cfredric commented 1 year ago

I think I'm supportive of making it sync.

My gut was to argue against it, since the environment's has storage access bool seems like a security-sensitive thing that Chromium would not want to store in a renderer (since we don't trust those processes). However, if we did store it in the renderer, a compromised renderer would only be able to modify the has storage access bool and shouldn't be able to forge a permission for the storage-access feature. So as long as the new parts of Fetch/Cookies (in #145) follow the determine the storage access policy algorithm (as written) and check the permission state (in addition to the has storage access bool), then a compromised renderer would gain access to unpartitioned cookies iff the user had already granted permission for that <top level, embed> pair. And in that case, the renderer could have gained access more easily by calling doc.requestStorageAccess() anyway, regardless of whether the bool is trusted or untrusted. So storing it somewhere other than the renderer process (where it's easy to implement a sync hasStorageAccess) doesn't actually accomplish anything for security.

So, I'm not seeing a reason not to support sync hSA.

annevk commented 1 year ago

For a moment I thought this would lead to badness across processes, but hasStorageAccess() only gives you information about the current document (i.e., did requestStorageAccess() succeed). The permission is the one that travels across processes.

Still, I'm not sure we can change this API fundamentally. Developers presumably already rely on it. If they want something synchronous they can use a variable instead...

Options we do have:

  1. Change the task queueing. Resolving the promise directly ought to be compatible. However, we want to be sure this doesn't create issues for the future with buckets. Cannot immediately think of any issues though so this is probably okay.
  2. We could also try something fancier, where it waits for requestStorageAccess() to complete before resolving, if that was already invoked. Given that you can already build this yourself this is probably not that useful, but I wanted to mention it anyway.
johannhof commented 1 year ago

I think sync hSA is off the table given the direction where going in #171