Skip to content

Change KeyUsage[] in SubtleCrypto to ReadonlyArray<KeyUsage> #1287

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

Merged
merged 2 commits into from
Mar 15, 2022

Conversation

jakubwolny
Copy link
Contributor

SubtleCrypto methods don't mutate input array, so we can allow ReadonlyArrays to be passed

This makes also this syntax possible:

const keyUsages = ['decrypt'] as const
await generateKey(..., keyUsages)

which wasn't possible before

@ghost
Copy link

ghost commented Mar 8, 2022

CLA assistant check
All CLA requirements met.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2022

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@saschanaz
Copy link
Contributor

saschanaz commented Mar 9, 2022

ReadonlyArray is okay and allows ES5 case, but I think ideally this should be Iterable<> which should cover const arrays and also non-array iterables too. Could you try Iterable, and add a unit test? unittests/files have some existing examples.

@jakubwolny
Copy link
Contributor Author

thanks for the fast response! I checked your suggestion and it seems to work fine (i.e. with Set)

I just have mixed feelings because MDN states: "keyUsages is an [Array] indicating what can be done with the key", so even though browser accepts something else I'm not sure whether we should encourage exploiting this undocumented behavior.

My another argument to rather have ReadonlyArray is to have the same type in both webcrypto usages and JWK definition, because according to the W3 specs (https://www.w3.org/TR/WebCryptoAPI/#dfn-CryptoKey-slot-usages and https://www.w3.org/TR/WebCryptoAPI/#JsonWebKey-dictionary) they should be the same: Sequence<KeyUsage>
And all the browsers return of course an Array when a key is exported in JWK format.

@saschanaz
Copy link
Contributor

keyUsages is an [Array] indicating what can be done with the key

It's a Web IDL sequence that can be converted from a JS array, but it doesn't have to be an array: https://w3c.github.io/webcrypto/#subtlecrypto-interface

  Promise<any> deriveKey(AlgorithmIdentifier algorithm,
                         CryptoKey baseKey,
                         AlgorithmIdentifier derivedKeyType,
                         boolean extractable,
                         sequence<KeyUsage> keyUsages );

https://webidl.spec.whatwg.org/#create-sequence-from-iterable

To create an IDL value of type sequence given an iterable iterable and an iterator getter method,

So it's very well defined in the spec.

That dictionary thing is a little bit unfortunate, since we have no way to overload a dictionary member. But that shouldn't block a method overload.

@jakubwolny
Copy link
Contributor Author

Ok, so it's the MDN which could be more accurate, I'll submit PR there as well

I tried to update it to the format you suggested Iterable<> and everything seems to be fine, but then when I run tests I got:
generated/dom.generated.d.ts(13763,101): error TS2304: Cannot find name 'Iterable'.

So, if I understand correctly if we want to support ES5 syntax in webcrypto, it needs to stay as ReadonlyArray

How would like me to proceed here? Sorry if it's causing you troubles, it's my first time updating built-in TS types.

@saschanaz
Copy link
Contributor

Err, yes it should be ReadonlyArray to support ES5, but then it should be Iterable to keep non-arrays working in ES2015+. I guess something more advanced should be done here to use the proper one for each variant 🤔

@saschanaz
Copy link
Contributor

Oh, actually ES2015+ was using arrays anyway, never mind then. Please add some unit tests and then I'll merge this.

@jakubwolny
Copy link
Contributor Author

Thanks for your help! I added the unit test.

@saschanaz
Copy link
Contributor

lgtm

@github-actions github-actions bot merged commit 80d7cd3 into microsoft:main Mar 15, 2022
@github-actions
Copy link
Contributor

Merging because @saschanaz is a code-owner of all the changes - thanks!

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.

2 participants