FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: remove incorrect constructor invocation by gc · Pull Request #40300 · nodejs/node · GitHub

/ node Public

crypto: remove incorrect constructor invocation - #40300

Closed
gc wants to merge 2 commits into
nodejs:masterfrom
gc:patch-1
ConversationCommits (2)ChecksFiles changed
Closed

crypto: remove incorrect constructor invocation#40300
gc wants to merge 2 commits into
nodejs:masterfrom
gc:patch-1

Conversation

gc commented Oct 3, 2021

Copy link
Copy Markdown
Contributor

This line is incorrectly trying to do throw new lazyDOMException, causing it to throw the wrong error.

Reproduction:

const { webcrypto } = require('crypto');

async function t() {
    	const { publicKey, privateKey } = await webcrypto.subtle.generateKey(
		{
			name: 'NODE-ED25519',
			namedCurve: 'NODE-ED25519'
		},
		true,
		['sign', 'verify']
	);
    const signature = await webcrypto.subtle.sign(
		{
			name: 'NODE-ED25519',
			hash: 'SHA-256'
		},
		privateKey,
		'-'
	);
}
t();
node:internal/crypto/ec:461
        throw new lazyDOMException(`Hash is not permitted for ${name}`);
              ^

TypeError: lazyDOMException is not a constructor
←[90m    at Object.ecdsaSignVerify (node:internal/crypto/ec:461:15)←[39m
←[90m    at signVerify (node:internal/crypto/webcrypto:609:10)←[39m
←[90m    at SubtleCrypto.sign (node:internal/crypto/webcrypto:623:10)←[39m
    at t (C:\dev\redacted\test.js:12:46)

nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Oct 3, 2021

cjihrig commented Oct 3, 2021

Copy link
Copy Markdown
Contributor

Thanks for the PR. Could you add a regression test please.

panva changed the title Remove incorrect constructor invocation crypto: remove incorrect constructor invocation Oct 4, 2021
panva added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. and removed needs-ci PRs that need a full CI run. labels Oct 4, 2021
panva removed the needs-ci PRs that need a full CI run. label Oct 4, 2021
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2021

nodejs-github-bot commented Oct 4, 2021

Copy link
Copy Markdown
Collaborator

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

gc commented Oct 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@panva Thank you so much for that! I tried to setup nodejs locally to properly test it, but ran into many issues/errors and didn't have time to continue trying to get it working today.

panva pushed a commit that referenced this pull request Oct 5, 2021
crypto: remove incorrect constructor invocation
28f711b 
PR-URL: #40300
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>

panva commented Oct 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Landed in 28f711b

panva commented Oct 5, 2021

Copy link
Copy Markdown
Member

Thank you for your contribution @gc!

panva closed this Oct 5, 2021

danielleadams commented Oct 7, 2021

Copy link
Copy Markdown
Contributor

@gc do you mind backporting this PR? It broke the v16.x build. thanks!

panva commented Oct 7, 2021

Copy link
Copy Markdown
Member

@danielleadams i can help, which branch do I base off of and open a PR against?

targos pushed a commit that referenced this pull request Oct 9, 2021
crypto: remove incorrect constructor invocation
954df09 
PR-URL: #40300
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>

targos commented Oct 9, 2021

Copy link
Copy Markdown
Member

The test failed on v16.x-staging because the DOMException global doesn't exist there.
I made the following fix and pushed the commit 954df09:

diff --git a/test/parallel/test-webcrypto-ed25519-ed448.js b/test/parallel/test-webcrypto-ed25519-ed448.js
index a0d858a4ab..b18f3f9d3b 100644
--- a/test/parallel/test-webcrypto-ed25519-ed448.js
+++ b/test/parallel/test-webcrypto-ed25519-ed448.js
@@ -1,3 +1,4 @@
+// Flags: --expose-internals
 'use strict';
 
 const common = require('../common');
@@ -11,6 +12,9 @@ const {
   webcrypto: { subtle }
 } = require('crypto');
 
+const { internalBinding } = require('internal/test/binding');
+const { DOMException } = internalBinding('messaging');
+
 async function generateKey(namedCurve) {
   return subtle.generateKey(
     {

danielleadams pushed a commit that referenced this pull request Oct 12, 2021
crypto: remove incorrect constructor invocation
a8926d1 
PR-URL: #40300
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
BethGriggs mentioned this pull request Oct 14, 2021
Merged
2 tasks
richardlau mentioned this pull request Oct 18, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

panva panva approved these changes
benjamingr benjamingr approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. crypto Issues and PRs related to the crypto subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL