w3c / web-share

Web API proposal for sharing data from a web page
https://www.w3.org/TR/web-share/
Other
353 stars 65 forks source link

A security review; some nits #223

Closed richsalz closed 2 years ago

richsalz commented 2 years ago

@samuelweiler asked me to review the document. I am not an expert in this area. I have a few questions/nits and it seems reasonable to merge them into one issue.

Sec 2.1; why is "SHOULD NOT" not a "MUST NOT" ? What is the use-case for exposing an API that is not supported? Is there some interoperability issue that I don't understand (quite likely).

Sec 2.1.2, I like the highly-detailed processing steps; made it easy to follow.

Sec 3, the note about "fire and forget" should be mentioned in 2.1.2 with a pointer here if needed.

Sec 4. Is there a registry of permission strings (i.e., where "web-share" would appear?) If not, I strongly recommend W3C maintain one.

Sec 6. I could not think of other security concerns, but note again that I am not an expert in this field. The last bullet leads me to think that the share target should fetch the content and use that for any downstream processing rather than handing off URL's. Is that the intent? Is that a best practice? Should that be made more explicit?

marcoscaceres commented 2 years ago

Sec 2.1; why is "SHOULD NOT" not a "MUST NOT" ? What is the use-case for exposing an API that is not supported? Is there some interoperability issue that I don't understand (quite likely).

I agree. This wording needs to go. No one is exposing .share() without supporting share.

Sec 2.1.2, I like the highly-detailed processing steps; made it easy to follow.

👍

Sec 3, the note about "fire and forget" should be mentioned in 2.1.2 with a pointer here if needed.

Moving the "fire and forget" to 2.1.2. makes sense.

Sec 4. Is there a registry of permission strings (i.e., where "web-share" would appear?) If not, I strongly recommend W3C maintain one.

Good call. It now lives here: https://w3c.github.io/permissions-registry/#registry-table-of-standardized-permissions

Sec 6. I could not think of other security concerns, but note again that I am not an expert in this field. The last bullet leads me to think that the share target should fetch the content and use that for any downstream processing rather than handing off URL's. Is that the intent? Is that a best practice? Should that be made more explicit?

It's application dependent. I'm of the opinion that the guidance as is clear enough. However, I'd be open to specific suggestions.

marcoscaceres commented 2 years ago

Hi @richsalz (and @samuelweiler for sec-review tracking purposes), I've sent a pull request addressing your feedback in https://github.com/w3c/web-share/pull/243 ... To allow us to close this issue (and per the W3C process for wide review), could you let me know if you are satisfied with this response and the proposed changes in #243?

ericwilligers commented 2 years ago

Yes, I am satisfied. Thank you for the changes.

On Tue, Jun 7, 2022 at 3:47 PM Marcos Cáceres @.***> wrote:

Hi @richsalz https://github.com/richsalz (and @samuelweiler https://github.com/samuelweiler for sec-review tracking purposes), I've sent a pull request addressing your feedback in #243 https://github.com/w3c/web-share/pull/243 ... To allow us to close this issue (and per the W3C process for wide review), could you let me know if you are satisfied with this response and the proposed changes in #243 https://github.com/w3c/web-share/pull/243?

— Reply to this email directly, view it on GitHub https://github.com/w3c/web-share/issues/223#issuecomment-1148223408, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA5JIYOZE5L7OXZMS2KS53LVN3PA3ANCNFSM5ICD66AQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

richsalz commented 2 years ago

The PR looks fine to me.

marcoscaceres commented 2 years ago

Thanks for confirming!

marcoscaceres commented 2 years ago

@samuelweiler, given @richsalz approval, could you close the related issue? https://github.com/w3c/security-review/issues/122