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.

Sign up for GitHub

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

Jump to bottom

crypto: fix WebCryptoAPI WebIDL harness #45590

Closed
wants to merge 13 commits into from
Closed

crypto: fix WebCryptoAPI WebIDL harness #45590

wants to merge 13 commits into from

Conversation

panva
Copy link
Member

@panva panva commented Nov 22, 2022

Individual commits tell the journey. This fixes the remaining WebCryptoAPI idlharness WPTs. Only landing where #42083 is.

@panva panva added crypto Issues and PRs related to the crypto subsystem. webcrypto dont-land-on-v16.x commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Nov 22, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Nov 22, 2022
@panva
Copy link
Member Author

panva commented Nov 22, 2022 •
edited
Loading

@jasnell @addaleax @tniessen asking for your review wrt. the JSTransferrable removal from CryptoKey. CryptoKey prototype must be Function.prototype.

624ecb0 (#45590)

I've had a look at existing tests for KeyObject that deal with transferring the objects and made sure to add CryptoKey instances to those tests, they still pass, but I really don't pretend to fully understand the consequences of this change.

@panva
Copy link
Member Author

panva commented Nov 22, 2022

@aduh95 asking for your review wrt. the global getter. It must not be callable unless this is globalThis (or undefined which is a special case for Window-like (globalThis) getters).

321fab6 (#45590)

aduh95
aduh95 reviewed Nov 22, 2022
View reviewed changes
lib/internal/crypto/keys.js Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Nov 23, 2022
View reviewed changes
lib/internal/process/pre_execution.js Outdated Show resolved Hide resolved
lib/internal/process/pre_execution.js Outdated Show resolved Hide resolved
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48192/

@github-actions github-actions bot mentioned this pull request Nov 26, 2022
Open
21 tasks
@panva panva added the review wanted PRs that need reviews. label Nov 26, 2022
@tniessen
Copy link
Member

cc @jasnell because of the JSTransferrable change.

@github-actions github-actions bot mentioned this pull request Nov 27, 2022
Open
21 tasks
jasnell
jasnell requested changes Nov 27, 2022
View reviewed changes
lib/internal/crypto/keys.js
@@ -634,7 +637,7 @@ function isKeyObject(obj) {
// here similar to other things like URL. A chromium provided CryptoKey
// will not be recognized as a Node.js CryptoKey, and vice versa. It
// would be fantastic if we could find a way of making those interop.
class CryptoKey extends JSTransferable {
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change. While WebCrypto doesn't define CryptoKey as transferable, we've implemented it this way from the beginning. Especially since we've already marked Web Crypto as stable we should preserve this.

Copy link
Member Author

@panva panva Nov 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Would you take a closer look please? The change does not seem to have impacted CryptoKey's transferability. I added tests for it too.

#45590 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

ping @addaleax @jasnell

@panva panva requested a review from jasnell November 27, 2022 16:54
This was referenced Nov 28, 2022
Open
Open
Open
Open
Open
Open
@panva
Copy link
Member Author

panva commented Dec 10, 2022

I'll slice this up into smaller bits, #45659 messed up the idlharness tests even more anyway.

@panva panva closed this Dec 10, 2022
@panva panva deleted the fix-webcrypto-idlharness branch January 2, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@tniessen tniessen tniessen approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. crypto Issues and PRs related to the crypto subsystem. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. review wanted PRs that need reviews. webcrypto
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@panva @nodejs-github-bot @tniessen @jasnell @aduh95