-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement support for the crypto.subtle.digest
operation
#23
Conversation
7c6483a
to
4ea9992
Compare
5a4f6e8
to
a38c42c
Compare
a38c42c
to
aaca416
Compare
// 3. | ||
normalizedAlgorithm, err := NormalizeAlgorithm(algorithm, OperationIdentifierDigest) | ||
if err != nil { | ||
// "if an error occurred, return a Promise rejected with NormalizedAlgorithm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I spotted this in WebCrypto specification, but to be honest, I'm not sure if I get this comment. Maybe I'm (and imagine like any other reader of this code) missing some context that we can put here.
What does mean rejected with NormalizedAlgorithm"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is legacy and reminded me to change the older implementation. I'll drop it.
The Web Crypto specification is very cryptic in certain regards, indeed. It took me a long time and a lot of research to determine what that meant. Most of what I've done on that front is inspired by the Deno implementation.
The algorithm normalization is essentially the step of converting an input Object
or string
, in a predefined format, into a (set of) internal structures that can be worked on while also ensuring to fail if any validation steps fail. It's a step that's quite easy in a dynamic language but was quite hard to do in a static language. I'm still not happy with the results, but it will do for now :)
I'll add a comment here to explain just that 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this comment made it worse 🤣
The specification says:
Let normalizedAlgorithm be the result of [normalizing an algorithm](https://www.w3.org/TR/WebCryptoAPI/#dfn-normalize-an-algorithm), with alg set to algorithm and op set to "digest".
If an error occurred, return a Promise rejected with normalizedAlgorithm.
And as the Specification does not have multiple returns normalizedAlgorithm
is just the result of the operation which can be an error (which "occurred").
So from that I would recommend not having a comment as that IMO makes it worse
// "if an error occurred, return a Promise rejected with NormalizedAlgorithm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, great work on this, but I think you're still working on it, right? Since the test doesn't actually work for me, even after converting the data to a TypedArray, and the CI fails. It would be better if this PR was converted to draft if so.
Still, I left some comments from the first look.
webcrypto/subtle_crypto.go
Outdated
asObject := data.ToObject(sc.vu.Runtime()) | ||
arrayBuffer, ok := asObject.Export().(goja.ArrayBuffer) | ||
if !ok { | ||
reject(fmt.Errorf("could not get ArrayBuffer from data")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the reason for the CI failure. I get this error when running the test locally, and when I fix the example to pass a TypedArray. Does this work for you?
I think we'll need a type switch here, but I'm not sure how we can get an ArrayBuffer from a TypedArray, or even assert that data
is a TypedArray instance, given that they're private in Goja. 😕 If you can't find a way, I would ask Mihail for help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was indeed the issue. I forgot to Get("buffer")
on it. Probably a fail during a rebase 🦖
This is fixed now, and the tests pass 🎉
TypedArray is a "view" over ArrayBuffers. They are an indication for the runtime of how to group and interpret bytes in an ArrayBuffer. As such, there's no conversion needed, and one can access their underlying ArrayBuffer using the buffer
property 🙇🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm familiar with TypedArrays, but I was hoping we could get their ArrayBuffer using the Goja Go API, instead of doing it in JS via the buffer
property. It would be safer if we didn't rely on duck typing 😅 If that's not possible, then I suppose it's the only thing we can do.
Though your latest change breaks passing an actual ArrayBuffer
instance, which should be supported according to the spec, so we should type assert on both data.ToObject(rt)
and data.ToObject(rt).Get("buffer")
.
Also, let's check for null
/undefined
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. I understand that doing it in Go is not possible now. If I remember correctly, when I experimented with it, treating the value as a goja.ArrayBuffer
led to a behavior where the ArrayBuffer was empty. The solution I adopted here is based on this discussion: dop251/goja#379.
Good catch on the ArrayBuffer
breaking change 👍🏻 I'll update it accordingly 🙇🏻
ea42bd7
to
b987b0a
Compare
b987b0a
to
813000a
Compare
crypto.subtle.digest("SHA-256", stringToArrayBuffer("Hello, world!")).then( | ||
(hash) => { | ||
console.log(arrayBufferToHex(hash)); | ||
}, | ||
(err) => { | ||
throw err; | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: probably should be using the async
/await
syntax at this point in time.
return Algorithm{}, NewError(0, SyntaxError, "algorithm name is not a string") | ||
// 2. | ||
var initialAlg Algorithm | ||
switch algorithm.ExportType().Kind() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really a fan of using ExportType
as IME it is really ... fragile.
Usually I would recommend actually checking the prototype of the value, but this is fine for now.
// 6. | ||
// FIXME: handle this case in later versions | ||
err := NewError(0, ImplementationError, fmt.Sprintf("unsupported algorithm type: %s", desiredType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keep the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a rebase mistake, for sure 🤦🏻
// 3. | ||
normalizedAlgorithm, err := NormalizeAlgorithm(algorithm, OperationIdentifierDigest) | ||
if err != nil { | ||
// "if an error occurred, return a Promise rejected with NormalizedAlgorithm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this comment made it worse 🤣
The specification says:
Let normalizedAlgorithm be the result of [normalizing an algorithm](https://www.w3.org/TR/WebCryptoAPI/#dfn-normalize-an-algorithm), with alg set to algorithm and op set to "digest".
If an error occurred, return a Promise rejected with normalizedAlgorithm.
And as the Specification does not have multiple returns normalizedAlgorithm
is just the result of the operation which can be an error (which "occurred").
So from that I would recommend not having a comment as that IMO makes it worse
// "if an error occurred, return a Promise rejected with NormalizedAlgorithm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I have some small comments but arguably all of them can be fixed later or not at all.
Thanks a lot for taking the time to review it @mstoykov much appreciated. I'll merge the PR for now, to move forward, and will address your comments separately 🙇🏻 |
This PR addresses #8 .
It adds an implementation for the
crypto.subtle.digest
operation to the extension.It implements the function according to the [Web Crypto API's specification](https://w3c.github.io/webcrypto/#Crypto-method-randomUUID). Please note that this feature is part of a restricted feature set of the API.
Note that the test suite is inherited and adapted from the WPT test suite dedicated to the
digest
operation.Checklist
Let me know what you think 🙇🏻