Skip to content
This repository has been archived by the owner on Feb 16, 2023. It is now read-only.

update randomUUID() to SecureContext #24

Merged
merged 3 commits into from
May 14, 2021
Merged

update randomUUID() to SecureContext #24

merged 3 commits into from
May 14, 2021

Conversation

bcoe
Copy link
Collaborator

@bcoe bcoe commented May 12, 2021

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: #23
Refs: w3ctag/design-reviews#623
CC: @annevk, @cynthia


Preview | Diff

@domenic
Copy link
Collaborator

domenic commented May 12, 2021

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

@bcoe
Copy link
Collaborator Author

bcoe commented May 12, 2021

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

Screen Shot 2021-05-12 at 8 29 54 AM

@bcoe bcoe requested review from ctavan and broofa May 12, 2021 15:30
@broofa
Copy link
Collaborator

broofa commented May 12, 2021

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
Copy link

bakkot commented May 12, 2021

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
Copy link
Collaborator Author

bcoe commented May 12, 2021

@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
Copy link

bakkot commented May 12, 2021

@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
Copy link
Collaborator

broofa commented May 12, 2021

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
Copy link
Collaborator Author

bcoe commented May 12, 2021

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
Copy link
Collaborator

domenic commented May 12, 2021

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.

@broofa broofa mentioned this pull request May 12, 2021
Copy link
Collaborator

@ctavan ctavan left a comment

Choose a reason for hiding this comment

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

I‘m fine with matching the spec with the current Chromium implementation, although I also tend to favor lifting that restriction. We‘ll discuss in #23 but let’s make things consistent first.

@bcoe bcoe merged commit 5f620e9 into gh-pages May 14, 2021
@bcoe
Copy link
Collaborator Author

bcoe commented May 14, 2021

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants