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: fix WebCryptoAPI WebIDL harness #45590

Closed
wants to merge 13 commits into from
20 changes: 19 additions & 1 deletion lib/internal/crypto/keys.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const {
ArrayFrom,
ArrayPrototypeSlice,
ObjectDefineProperty,
ObjectDefineProperties,
ObjectSetPrototypeOf,
Symbol,
Uint8Array,
Expand Down Expand Up @@ -38,6 +39,7 @@ const {
ERR_ILLEGAL_CONSTRUCTOR,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_THIS,
}
} = require('internal/errors');

Expand All @@ -61,6 +63,7 @@ const {

const {
customInspectSymbol: kInspect,
kEnumerableProperty,
} = require('internal/util');

const { inspect } = require('internal/util/inspect');
Expand Down Expand Up @@ -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

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.

class CryptoKey {
constructor() {
throw new ERR_ILLEGAL_CONSTRUCTOR();
}
Expand All @@ -657,18 +660,26 @@ class CryptoKey extends JSTransferable {
}

get type() {
if (!(this instanceof CryptoKey))
throw new ERR_INVALID_THIS('CryptoKey');
return this[kKeyObject].type;
}

get extractable() {
if (!(this instanceof CryptoKey))
throw new ERR_INVALID_THIS('CryptoKey');
return this[kExtractable];
}

get algorithm() {
if (!(this instanceof CryptoKey))
throw new ERR_INVALID_THIS('CryptoKey');
return this[kAlgorithm];
}

get usages() {
if (!(this instanceof CryptoKey))
throw new ERR_INVALID_THIS('CryptoKey');
return ArrayFrom(this[kKeyUsages]);
}

Expand Down Expand Up @@ -697,6 +708,13 @@ class CryptoKey extends JSTransferable {
}
}

ObjectDefineProperties(CryptoKey.prototype, {
type: kEnumerableProperty,
extractable: kEnumerableProperty,
algorithm: kEnumerableProperty,
usages: kEnumerableProperty,
});

// All internal code must use new InternalCryptoKey to create
// CryptoKey instances. The CryptoKey class is exposed to end
// user code but is not permitted to be constructed directly.
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/crypto/random.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const { Buffer, kMaxLength } = require('buffer');
const {
codes: {
ERR_INVALID_ARG_TYPE,
ERR_MISSING_ARGS,
ERR_OUT_OF_RANGE,
ERR_OPERATION_FAILED,
}
Expand Down Expand Up @@ -310,6 +311,8 @@ function onJobDone(buf, callback, error) {
// not allowed to exceed 65536 bytes, and can only
// be an integer-type TypedArray.
function getRandomValues(data) {
if (arguments.length < 1)
throw new ERR_MISSING_ARGS('typedArray');
if (!isTypedArray(data) ||
isFloat32Array(data) ||
isFloat64Array(data)) {
Expand Down
4 changes: 4 additions & 0 deletions lib/internal/process/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const {
} = require('internal/util');

const {
ERR_INVALID_THIS,
ERR_MANIFEST_ASSERT_INTEGRITY,
ERR_NO_CRYPTO,
} = require('internal/errors').codes;
Expand Down Expand Up @@ -278,6 +279,9 @@ function setupWebCrypto() {
ObjectDefineProperty(globalThis, 'crypto',
{ __proto__: null, ...ObjectGetOwnPropertyDescriptor({
get crypto() {
if (this !== globalThis && this !== undefined)
panva marked this conversation as resolved.
Show resolved Hide resolved
throw new ERR_INVALID_THIS(
'globalThis or undefined');
panva marked this conversation as resolved.
Show resolved Hide resolved
return webcrypto.crypto;
}
}, 'crypto') });
Expand Down
92 changes: 52 additions & 40 deletions test/parallel/test-crypto-key-objects-messageport.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ if (!common.hasCrypto)
common.skip('missing crypto');

const assert = require('assert');
const { createSecretKey, generateKeyPairSync, randomBytes } = require('crypto');
const {
generateKeySync,
generateKeyPairSync,
KeyObject,
webcrypto: { subtle },
} = require('crypto');
const { createContext } = require('vm');
const {
MessageChannel,
Expand All @@ -15,6 +20,9 @@ const {

function keyToString(key) {
let ret;
if (key instanceof CryptoKey) {
key = KeyObject.from(key);
}
if (key.type === 'secret') {
ret = key.export().toString('hex');
} else {
Expand All @@ -33,55 +41,59 @@ if (process.env.HAS_STARTED_WORKER) {
// Don't use isMainThread to allow running this test inside a worker.
process.env.HAS_STARTED_WORKER = 1;

// The main thread generates keys and passes them to worker threads.
const secretKey = createSecretKey(randomBytes(32));
const { publicKey, privateKey } = generateKeyPairSync('rsa', {
modulusLength: 1024
});
(async () => {
// The main thread generates keys and passes them to worker threads.
const secretKey = generateKeySync('aes', { length: 128 });
const { publicKey, privateKey } = generateKeyPairSync('rsa', {
modulusLength: 1024
});
const cryptoKey = await subtle.generateKey(
{ name: 'AES-CBC', length: 128 }, false, ['encrypt']);

// Get immutable representations of all keys.
const keys = [secretKey, publicKey, privateKey]
// Get immutable representations of all keys.
const keys = [secretKey, publicKey, privateKey, cryptoKey]
.map((key) => [key, keyToString(key)]);

for (const [key, repr] of keys) {
{
for (const [key, repr] of keys) {
{
// Test 1: No context change.
const { port1, port2 } = new MessageChannel();

port1.postMessage({ key });
assert.strictEqual(keyToString(key), repr);
const { port1, port2 } = new MessageChannel();

port2.once('message', common.mustCall(({ key }) => {
port1.postMessage({ key });
assert.strictEqual(keyToString(key), repr);
}));
}

{
port2.once('message', common.mustCall(({ key }) => {
assert.strictEqual(keyToString(key), repr);
}));
}

{
// Test 2: Across threads.
const worker = new Worker(__filename);
worker.once('message', common.mustCall((receivedRepresentation) => {
assert.strictEqual(receivedRepresentation, repr);
}));
worker.on('disconnect', () => console.log('disconnect'));
worker.postMessage({ key });
}
const worker = new Worker(__filename);
worker.once('message', common.mustCall((receivedRepresentation) => {
assert.strictEqual(receivedRepresentation, repr);
}));
worker.on('disconnect', () => console.log('disconnect'));
worker.postMessage({ key });
}

{
{
// Test 3: Across contexts (should not work).
const { port1, port2 } = new MessageChannel();
const context = createContext();
const port2moved = moveMessagePortToContext(port2, context);
assert(!(port2moved instanceof Object));
const { port1, port2 } = new MessageChannel();
const context = createContext();
const port2moved = moveMessagePortToContext(port2, context);
assert(!(port2moved instanceof Object));

// TODO(addaleax): Switch this to a 'messageerror' event once MessagePort
// implements EventTarget fully and in a cross-context manner.
port2moved.onmessageerror = common.mustCall((event) => {
assert.strictEqual(event.data.code,
'ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE');
});
// TODO(addaleax): Switch this to a 'messageerror' event once MessagePort
// implements EventTarget fully and in a cross-context manner.
port2moved.onmessageerror = common.mustCall((event) => {
assert.strictEqual(event.data.code,
'ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE');
});

port2moved.start();
port1.postMessage({ key });
port1.close();
port2moved.start();
port1.postMessage({ key });
port1.close();
}
}
}
})().then(common.mustCall());
23 changes: 20 additions & 3 deletions test/parallel/test-crypto-worker-thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,30 @@ if (!common.hasCrypto)

// Issue https://github.com/nodejs/node/issues/35263
// Description: Test that passing keyobject to worker thread does not crash.
const { createSecretKey } = require('crypto');
const {
generateKeySync,
generateKeyPairSync,
webcrypto: { subtle },
} = require('crypto');

const assert = require('assert');

const { Worker, isMainThread, workerData } = require('worker_threads');

if (isMainThread) {
const key = createSecretKey(Buffer.from('hello'));
new Worker(__filename, { workerData: key });
(async () => {
const secretKey = generateKeySync('aes', { length: 128 });
const { publicKey, privateKey } = generateKeyPairSync('rsa', {
modulusLength: 1024
});
const cryptoKey = await subtle.generateKey(
{ name: 'AES-CBC', length: 128 }, false, ['encrypt']);

for (const key of [secretKey, publicKey, privateKey, cryptoKey]) {
new Worker(__filename, { workerData: key });
}
})().then(common.mustCall());
} else {
console.log(workerData);
assert.notDeepStrictEqual(workerData, {});
}
3 changes: 0 additions & 3 deletions test/wpt/status/WebCryptoAPI.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,5 @@
},
"historical.any.js": {
"skip": "Not relevant in Node.js context"
},
"idlharness.https.any.js": {
"skip": "Various non-IDL-compliant things"
}
}
2 changes: 2 additions & 0 deletions test/wpt/test-webcrypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ const { WPTRunner } = require('../common/wpt');

const runner = new WPTRunner('WebCryptoAPI');

runner.pretendGlobalThisAs('Window');

// Set Node.js flags required for the tests.
runner.setFlags(['--experimental-global-webcrypto']);

Expand Down