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

lib,crypto: add optional cause to DOMException #44224

Closed
wants to merge 1 commit into from
Closed

lib,crypto: add optional cause to DOMException #44224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions lib/internal/crypto/aes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,11 +241,10 @@ async function aesGenerateKey(algorithm, extractable, keyUsages) {
}

const key = await generateKey('aes', { length }).catch((err) => {
// TODO(@panva): add err as cause to DOMException
throw lazyDOMException(
'The operation failed for an operation-specific reason' +
`[${err.message}]`,
'OperationError');
'The operation failed for an operation-specific reason',
'OperationError',
err);
});

return new InternalCryptoKey(
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/crypto/cfrg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,10 @@ async function cfrgGenerateKey(algorithm, extractable, keyUsages) {
}

const keyPair = await generateKeyPair(genKeyType).catch((err) => {
// TODO(@panva): add err as cause to DOMException
throw lazyDOMException(
'The operation failed for an operation-specific reason',
'OperationError');
'OperationError',
err);
});

let publicUsages;
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/crypto/ec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ async function ecGenerateKey(algorithm, extractable, keyUsages) {
}

const keypair = await generateKeyPair('ec', { namedCurve }).catch((err) => {
// TODO(@panva): add err as cause to DOMException
throw lazyDOMException(
'The operation failed for an operation-specific reason',
'OperationError');
'OperationError',
err);
});

let publicUsages;
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/crypto/mac.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,10 @@ async function hmacGenerateKey(algorithm, extractable, keyUsages) {
}

const key = await generateKey('hmac', { length }).catch((err) => {
// TODO(@panva): add err as cause to DOMException
throw lazyDOMException(
'The operation failed for an operation-specific reason',
'OperationError');
'OperationError',
err);
});

return new InternalCryptoKey(
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/crypto/rsa.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,10 @@ async function rsaKeyGenerate(
modulusLength,
publicExponent: publicExponentConverted,
}).catch((err) => {
// TODO(@panva): add err as cause to DOMException
throw lazyDOMException(
'The operation failed for an operation-specific reason',
'OperationError');
'OperationError',
err);
});

const keyAlgorithm = {
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/crypto/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,10 +279,10 @@ const validateByteSource = hideStackFrames((val, name) => {

function onDone(resolve, reject, err, result) {
if (err) {
// TODO(@panva): add err as cause to DOMException
return reject(lazyDOMException(
'The operation failed for an operation-specific reason',
'OperationError'));
'OperationError',
err));
}
resolve(result);
}
Expand Down
14 changes: 12 additions & 2 deletions lib/internal/per_context/domexception.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,23 @@ const disusedNamesSet = new SafeSet()
.add('ValidationError');

class DOMException {
constructor(message = '', name = 'Error') {
constructor(message = '', name = 'Error', cause = undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this signature preferable over any of these?

Suggested change
constructor(message = '', name = 'Error', cause = undefined) {
constructor(message = '', name = 'Error', options = { cause }) { // pseudocode

or

Suggested change
constructor(message = '', name = 'Error', cause = undefined) {
constructor(message = '', nameOrOptions = 'Error') {

ErrorCaptureStackTrace(this);
internalsMap.set(this, {
message: `${message}`,
name: `${name}`
name: `${name}`,
cause
});
Comment on lines +57 to 58
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't that be more correct? (at least that's the 'cause' in new Error === false, and 'cause' in new Error('', { cause: undefined})) === true)

Suggested change
cause
});
});
if (arguments.length > 2) {
ObjectDefineProperty(this, 'cause', {
__proto__: null,
configurable: true,
enumerable: true,
value: cause,
writable: true,
});
}

}

get cause() {
Copy link
Member

Choose a reason for hiding this comment

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

Per the debate (whatwg/webidl#1179) that is happening in the PR that is updating DOMException to have a cause there is a disagreement on whether cause should be an own property or an accessor. For all intrinsic Errors, TC-39 decided that cause should be an own data property in order to distinguish between the case where cause is explicitly not given vs. cause is explicitly undefined

const err1 = new Error('no cause');
console.log('cause' in err1); // false
console.log(err1.cause); // undefined

const err2 = new Error('undefined cause', { cause: undefined });
console.log('cause' in err2); // true
console.log(err2.cause); // undefined

From all appearances, it seems a decision is going to be made that DOMException should be different that intrinsic errors and will use an accessor instead, making it impossible to distinguish between these cases.

const err1 = new DOMException('no cause', 'ABORT_ERR');
console.log('cause' in err1); // true
console.log(err1.cause); // undefined

const err2 = new DOMException('undefined cause', { name: 'ABORT_ERR', cause: undefined });
console.log('cause' in err2); // true
console.log(err2.cause); // undefined

The reasons for diverging do not seem that well defined beyond consistency with the way name and message are currently handled for DOMException but it's not really a compelling reason for me.

For our implementation, I'd rather we choose to align with TC-39's definition and intentionally diverge from the official definition that is likely to land in the WebIDL spec.

tl;dr ... let's not use an accessor for cause. Let's make it an own data property on the DOMException instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

#44224 (comment)

const internals = internalsMap.get(this);
if (internals === undefined) {
throwInvalidThisError(TypeError, 'DOMException');
}
return internals.cause;
}

get name() {
const internals = internalsMap.get(this);
if (internals === undefined) {
Expand Down Expand Up @@ -93,6 +102,7 @@ ObjectDefineProperties(DOMException.prototype, {
[SymbolToStringTag]: { __proto__: null, configurable: true, value: 'DOMException' },
name: { __proto__: null, enumerable: true, configurable: true },
message: { __proto__: null, enumerable: true, configurable: true },
cause: { __proto__: null, enumerable: true, configurable: true },
code: { __proto__: null, enumerable: true, configurable: true }
});

Expand Down
4 changes: 2 additions & 2 deletions lib/internal/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,9 +498,9 @@ const lazyDOMExceptionClass = () => {
return _DOMException;
};

const lazyDOMException = hideStackFrames((message, name) => {
const lazyDOMException = hideStackFrames((message, name, cause) => {
_DOMException ??= internalBinding('messaging').DOMException;
return new _DOMException(message, name);
return new _DOMException(message, name, cause);
});

const kEnumerableProperty = ObjectCreate(null);
Expand Down
21 changes: 15 additions & 6 deletions test/parallel/test-webcrypto-encrypt-decrypt-aes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,11 @@ async function testDecrypt({ keyBuffer, algorithm, result }) {
});

decryptionFailing.forEach((vector) => {
variations.push(assert.rejects(testDecrypt(vector), {
name: 'OperationError'
variations.push(assert.rejects(testDecrypt(vector), (err) => {
assert.strictEqual(err.name, 'OperationError');
assert.ok(err.cause instanceof Error);
assert.match(err.cause?.message, /bad decrypt/);
return true;
}));
});

Expand Down Expand Up @@ -157,8 +160,11 @@ async function testDecrypt({ keyBuffer, algorithm, result }) {
});

decryptionFailing.forEach((vector) => {
variations.push(assert.rejects(testDecrypt(vector), {
name: 'OperationError'
variations.push(assert.rejects(testDecrypt(vector), (err) => {
assert.strictEqual(err.name, 'OperationError');
assert.ok(err.cause instanceof Error);
assert.match(err.cause?.message, /foo/);
return true;
}));
});

Expand Down Expand Up @@ -194,8 +200,11 @@ async function testDecrypt({ keyBuffer, algorithm, result }) {
});

decryptionFailing.forEach((vector) => {
variations.push(assert.rejects(testDecrypt(vector), {
name: 'OperationError'
variations.push(assert.rejects(testDecrypt(vector), (err) => {
assert.strictEqual(err.name, 'OperationError');
assert.ok(err.cause instanceof Error);
assert.match(err.cause?.message, /foo/);
return true;
}));
});

Expand Down
7 changes: 5 additions & 2 deletions test/parallel/test-webcrypto-encrypt-decrypt-rsa.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,11 @@ async function testEncryptionLongPlaintext({ algorithm,
newplaintext[plaintext.byteLength] = 32;

return assert.rejects(
subtle.encrypt(algorithm, publicKey, newplaintext), {
name: 'OperationError'
subtle.encrypt(algorithm, publicKey, newplaintext), (err) => {
assert.strictEqual(err.name, 'OperationError');
assert.ok(err.cause instanceof Error);
assert.match(err.cause?.message, /data too large for key size/);
return true;
});
}

Expand Down
7 changes: 5 additions & 2 deletions test/parallel/test-webcrypto-keygen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,11 @@ const vectors = {
modulusLength,
publicExponent: new Uint8Array(publicExponent),
hash
}, true, usages), {
name: 'OperationError',
}, true, usages), (err) => {
assert.strictEqual(err.name, 'OperationError');
assert.ok(err.cause instanceof Error);
assert.match(err.cause?.message, /pub exponent out of range/);
return true;
});
}));
}
Expand Down