privacycg / storage-access

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

Reject with meaningful exceptions vs undefined. #37

Closed Brandr0id closed 1 year ago

Brandr0id commented 4 years ago

It may be nice to reject failed calls to requestStorageAccess with an exception + message.

e.g. if no gesture => reject with a new DomException indicating "The method cannot be executed without a user gesture." and so on.

Any objections to this? I believe currently the promises are just rejected.

johnwilander commented 4 years ago

We have already seen misuse of the reject signal where the caller times it and if there’s a delay, it concludes that the user was prompted and clicked “Don’t allow.” In this specific case, it’s an embedded video and if the timer tells the embedee that the user explicitly denied them cookie access, they stop video playback. However, if the promise is rejected immediately, playback continues. This means users who are prompted and deny them access are punished. The result is typically that the user tries again, is prompted again, and this time opts in even though they originally didn’t want to.

If we give the caller explicit info on why the promise was rejected, we will likely see more misuse. I have instead considered introducing an artificial delay to make it harder to harass/punish users.

Brandr0id commented 4 years ago

We have already seen misuse of the reject signal where the caller times it and if there’s a delay, it concludes that the user was prompted and clicked “Don’t allow.”

Do you happen to have concrete examples of sites this was seen on?

However, if the promise is rejected immediately, playback continues.

Is this deliberate JS to do this or an accidental race in client side code?

If we give the caller explicit info on why the promise was rejected, we will likely see more misuse. I have instead considered introducing an artificial delay to make it harder to harass/punish users.

If a site wants to be punitive towards users who reject storage access and prevent playback or put up a paywall do we have any reason to believe they would be more inclined to do so for one rejection reason over another? The result is the same that they have no storage access.

annevk commented 4 years ago

The exception doesn't have to include information, but we if we reject a promise it must be with an exception: https://www.w3.org/2001/tag/doc/promises-guide#reasons-should-be-errors.

Rejecting with a JavaScript TypeError seems reasonable.

(User agents could of course include relevant information that's visible to the developer console, but not the page.)

jackfrankland commented 4 years ago

We have already seen misuse of the reject signal where the caller times it and if there’s a delay, it concludes that the user was prompted and clicked “Don’t allow.” In this specific case, it’s an embedded video and if the timer tells the embedee that the user explicitly denied them cookie access, they stop video playback. However, if the promise is rejected immediately, playback continues. This means users who are prompted and deny them access are punished. The result is typically that the user tries again, is prompted again, and this time opts in even though they originally didn’t want to.

If we give the caller explicit info on why the promise was rejected, we will likely see more misuse. I have instead considered introducing an artificial delay to make it harder to harass/punish users.

Just want to mention that a similar thing could be used in relation to the discussion on https://github.com/privacycg/storage-access/issues/25. If an explicit reject consumes the user activation, while an implicit reject does not, the third party could subsequently call a user activation gated api (maybe easiest would be audio.play?) to test the difference.

Brandr0id commented 4 years ago

The exception doesn't have to include information, but we if we reject a promise it must be with an exception: https://www.w3.org/2001/tag/doc/promises-guide#reasons-should-be-errors.

Rejecting with a JavaScript TypeError seems reasonable.

As long as we resolve with some type of error or exception that affords implementers the ability to make a messaging decision for developers that would be great. If we could all agree on common messaging that would be great but it seems reasonable to leave that up to implementers.

@johnwilander @englehardt does it seem reasonable to use TypeErrors for this purpose or are there any other preferences/better fits?

johannhof commented 3 years ago

For Firefox we're also not really interested in disclosing a user choice to the site via error type or message. We have recently added more extensive error logging which seems like a good compromise. It is a bit unfortunate for web developers that the Storage Access API has a rather complicated grant algorithm and many implementation specific pieces, so I can understand why you would want a better way to react to rejections. However, looking at potential ways for relaying the rejection to users I can see a lot more potential for abuse than helpful clarification from websites. So it might be preferable to stay unclear here.

But rejecting with a generic error type to follow Promise conventions would certainly be good.

johannhof commented 2 years ago

To reiterate past comments, we won't provide detailed reasons for rejection but it might be reasonable to discuss throwing a specific error to follow existing conventions, instead of rejecting with undefined, which I think the implementations do right now. The editors consider this future work.

annevk commented 2 years ago

https://github.com/privacycg/storage-access/issues/37#issuecomment-629587593 has a pretty concrete solution for this. Why can this not be addressed sooner? We don't have any instances of web platform promises that reject with undefined (unless explicitly instructed to do so as is possible with abort signals). They all reject with an exception. This is like having a function throw undefined, it's rather bad.

AramZS commented 2 years ago

I do think a generic exception is better than undefined. I see no problem from the developer point of view with this, a failure is a failure and the same actions would be taken in response. Presumably logging would allow for detection of a code vs permissions error due to scale. Almost certainly developers testing this would be able to distinguish between a Choice vs code breakage. If that is the issue and there is some reason why that might not be the case additional developer tooling might be the way to go, not more detailed errors.

johannhof commented 1 year ago

I think @bvandersloot-mozilla wanted to take a stab at this but I can't assign you, Ben. We should figure out access rights to the repo with the chairs.

johannhof commented 1 year ago

Fixed by #118