Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using Crypto.getRandomValues() in Shadow Realms #361

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Ms2ger
Copy link

@Ms2ger Ms2ger commented Jan 23, 2024

Note that this depends on the introduction of UniversalGlobalScope in whatwg/html#9893.

Closes #338

The following implementers have shown interest:

The following tasks have been completed:

Implementation issues:


Preview | Diff

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This exposes much more in Shadow Realms, no?

@Ms2ger
Copy link
Author

Ms2ger commented Jan 23, 2024

My understanding at this point is that we will not expose anything behind [SecureContext] in ShadowRealms. I don't think that's entirely clear from the prose in IDL yet; I'm looking into clarifying that at the moment.

@annevk
Copy link
Member

annevk commented Jan 23, 2024

I think it's worth adding explicit [Exposed=Window,Worker] to the [SecureContext] members and maybe even asserting that needs to be present for [SecureContext] members when their corresponding class has [Exposed=*]. Seems a tad too magical otherwise.

But maybe we should also take a step back and have a discussion on that as it's not clear to me what principles are behind it. At least to me it seems somewhat reasonable to offer UUIDs. But perhaps we don't want the additional [SecureContext] bookkeeping? I can understand that, especially for a v1.

@twiss
Copy link
Member

twiss commented Jan 23, 2024

My understanding at this point is that we will not expose anything behind [SecureContext] in ShadowRealms. I don't think that's entirely clear from the prose in IDL yet; I'm looking into clarifying that at the moment.

I don't know that much about ShadowRealms, but I would intuitively expect [SecureContext] to expose things if the ShadowRealm was created in a secure context. If that's not the intention I personally agree with Anne that it might make sense to make that explicit using [Exposed].


A more general comment: this PR only talks about Shadow Realms but the diff makes the Crypto interface [Exposed=*], which also exposes it (including SubtleCrypto and randomUUID) for Worklets (as discussed in #338), right? That might not be an issue, but if you want to expose it for Shadow Realms only, you might want a more specific [Exposed] attribute. Alternatively, if you want to expose it everywhere, I think it'd be good to make that clear in the PR description etc.

Also, the WebIDL spec says:

[Exposed=*] is to be used with care. It is only appropriate when an API does not expose significant new capabilities. If the API might be restricted or disabled in some environments, it is preferred to list the globals explicitly.

That being said, I'm not sure if there's any significant concern about exposing crypto.getRandomValues everywhere, and the other members are gated behind [SecureContext], so perhaps that already sufficiently addresses the concern. But, it might still seem safer to explicitly list the places where it should be exposed?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks correct, but you should make it clear in OP what other PR this depends on and that this cannot merge before that as UniversalGlobalScope is not a thing as far as I know.

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

Successfully merging this pull request may close these issues.

Can we expose Crypto/SubtleCrypto to all Worklet / all scopes?
3 participants