WICG / uuid

UUID V4
Other
63 stars 10 forks source link

update randomUUID() to SecureContext #24

Closed bcoe closed 3 years ago

bcoe commented 3 years ago

Based on review during the intent to ship process, and based on the ongoing discussions linked below, we've opted to ship randomUUID() in a SecureContext.

Let's continue the discussion regarding this choice in #23, potentially soon with user feedback?

Refs: https://github.com/WICG/uuid/issues/23 Refs: https://github.com/w3ctag/design-reviews/issues/623 CC: @annevk, @cynthia


Preview | Diff

domenic commented 3 years ago

@marcoscaceres any idea what happened with the IPR checker? It was working fine for all previous PRs...

bcoe commented 3 years ago

👇 looks like I'm still listed in the group, hiccup?

Screen Shot 2021-05-12 at 8 29 54 AM
broofa commented 3 years ago

I'm confused. I read the conversation in #23 as allowing for an exception to be made here (i.e. randomUuid() should not be limited to secure contexts.) But this change reads as though it is. What am I missing?

bakkot commented 3 years ago

I'm not strongly invested in either outcome here, but I am interested in understanding the reasoning, and

based on the ongoing discussions linked below, we've opted to ship randomUUID() in a SecureContext

The conclusion in both of the linked discussions is that it should not be limited to secure contexts. Is there another discussion you meant to link?

(edit: whoops, ninja'd)

bcoe commented 3 years ago

@bakkot @broofa, re:

The conclusion in both of the linked discussions is that it should not be limited to secure contexts. Is there another discussion you meant to link? I'm confused. I read the conversation in #23 as allowing for an exception to be made here

@annevk makes the case in this comment that the decision to ship in insecure contexts is inconsistent with recent design decisions in Web Crypto.

@cynthia, in turn proposes that we continue the discussion in #23.

In seeing this, reviewers in the Chromium Intent to Ship thread suggested that we make the conservative choice of shipping in a secure context (at least initially).

If it becomes clear this is a huge pain in the neck for users, I think we can make a case for later removing this constraint.

bakkot commented 3 years ago

@annevk says

Limited to randomness it would be more reasonable as that's already exposed

But this proposal is limited to randomness. I read his comment to be addressing the preceding comment about "rolling your own crypto", not about this proposal specifically.

broofa commented 3 years ago

Is it appropriate to continue debating this, or do you just want a simple "make sure this change isn't going to break anything" code review here? If the latter, I'm happy to approve.

If the former... well, the arguments for why this should be restricted to secure contexts so far lack real substance. Meanwhile the counter-arguments seem pretty valid. E.g. @domenic's comment about this forcing dependent libraries to surface the secure-context requirement and / or ship with a polyfill is pretty astute.

For my part, I can't help but wonder about the conceptual similarity between getRandomValues() and randomUuid(). Two peas in a pod, so to speak. Explaining to developers why the former works in insecure contexts but the latter doesn't is going to be difficult.

bcoe commented 3 years ago

Is it appropriate to continue debating this

I think it's appropriate to keep debating this, let's move to #23 though.

I saw SecureContext as a reasonable compromise for now, as it's easier to go from a limited audience to a wider audience, than form a wide audience of users to a smaller audience.

domenic commented 3 years ago

In particular, I think merging this to the spec is a good idea, so that the spec reflects the one shipping implementation (Chrome) and the preference of at least one other implementer (Mozilla). Then we can see if we can sway those implementers in #23.

Maybe we should add a comment or <p class="note"> or something pointing people to #23 though, from the spec.

bcoe commented 3 years ago

Merged this so that the spec is inline with what has so far been shipped. We should follow up with a blurb about the discussion happening in #23, as suggested by @domenic.