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.

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

crypto: remove incorrect constructor invocation #40300

Closed
wants to merge 2 commits into from
Closed

Conversation

gc
Copy link
Contributor

@gc gc commented Oct 3, 2021

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 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
Copy link
Contributor

cjihrig commented Oct 3, 2021

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

@panva panva changed the title Remove incorrect constructor invocation crypto: remove incorrect constructor invocation Oct 4, 2021
@panva 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 pending requests for changes, 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 panva removed the needs-ci PRs that need a full CI run. label Oct 4, 2021
@github-actions 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
Copy link
Collaborator

@gc
Copy link
Contributor Author

gc commented Oct 4, 2021

@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
PR-URL: #40300
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@panva
Copy link
Member

panva commented Oct 5, 2021

Landed in 28f711b

@panva
Copy link
Member

panva commented Oct 5, 2021

Thank you for your contribution @gc!

@danielleadams
Copy link
Contributor

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

@panva
Copy link
Member

panva commented Oct 7, 2021

@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
PR-URL: #40300
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@targos
Copy link
Member

targos commented Oct 9, 2021

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
PR-URL: #40300
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@richardlau richardlau mentioned this pull request Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants