Reorder reading algorithm and data arguments#426
Conversation
Read and normalize the algorithm argument(s) before copying the bytes held by data arguments, to enable avoiding the copy if the operation is performed on the same thread (which is currently not possible because the algorithm object getters may modify the data).
|
I asked someone who worked on this at the time and, from their memory, this was done to avoid vulnerabilities caused by the user callback mutating the type system? (They said it might actually have @annevk who originally suggested this solution way back, hehe.) Of course the spec and WebIDL have changed since then, so not sure what's still needed. I'll find an IDL expert on the Chromium side and get back to this. |
Sounds like we believe it's fine! (We also talked a bit about whether the algorithm parameter could just be modeled with a dictionary parameter that was the union of all of them or something, so we stop having to match objects to dictionaries by hand or something goofy like that, but we didn't dig too closely into what that would take.) |
|
I was certainly involved in IDL discussions in general way back, including these kind of "do dictionary conversions yourself so you can branch the dictionaries on something Web IDL does not have access to" patterns, but suggesting doing it after obtaining the buffer seems surprising. (I could see an argument though. If you are going in parallel anyway you have to make a copy, so you might as well make the copy before the buffer could potentially be invalidated. But it's a weak argument and I don't think I would argue that way today, if I ever did.) OP seems to suggest it, but just to double check, we have test coverage for this in WPT? |
No, we have test coverage for mutating the data after calling the function (but before the returned promise resolves), but not for mutating the data in an argument object getter method (which is the only case where this change is observable AFAIK). I'll update WPT to check for that. |
|
It would be good to both have a test for mutation and a test for detaching the buffer (both as a result of dictionary conversion). |
+1. Looks like there's a |
|
I added tests for mutation in web-platform-tests/wpt#57614; I'll add tests for transfering later as no aspect of that is tested today (so we should test that not just during algorithm normalization but also after the call to Web Crypto). Resizable ArrayBuffers are not supported by WebCrypto as the parameter types don't have the [AllowResizable] attribute. (But, we could add tests to check that they're rejected as well, of course.) |
|
@annevk Could I have an approval for this one and the newest 4 of https://github.com/web-platform-tests/wpt/pulls/twiss, please? 🙏 |
|
(I'll merge this as the tests got approved and all implementations except Chrome already behave this way, and @davidben also approved the change.) |
SHA: 5b57233 Reason: push, by twiss Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Add tests for the reading order of arguments in Web Crypto, updated in w3c/webcrypto#426 to match the majority of implementations.
…ad order, a=testonly Automatic update from web-platform-tests [WebCryptoAPI] Add tests for argument read order (#57614) Add tests for the reading order of arguments in Web Crypto, updated in w3c/webcrypto#426 to match the majority of implementations. -- wpt-commits: 42e47329fdc92c80d58c2816eb66cb2cf2b32a89 wpt-pr: 57614
3 participants
Read and normalize the algorithm argument(s) before copying the bytes held by data arguments, to enable avoiding the copy if the operation is performed on the same thread (which is currently not possible because the algorithm object getters may modify the data).
This also matches the current behavior of Firefox, Safari, and Node.js (only Chrome implements the current spec properly).
Closes #422.
Preview | Diff