Skip to content

Commit e790713

Browse files
panvaaduh95
authored andcommitted
lib: use Web IDL interface brand checks
Use explicit brand predicates for interface conversion instead of prototype ancestry. Update CryptoKey and AbortSignal together with the shared converter contract. Read internal AbortSignal state during composition. Preserve genuine signals after prototype changes without invoking shadowed getters. Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: GitHub Copilot PR-URL: #65846 Backport-PR-URL: #66233 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent c2ec40c commit e790713

6 files changed

Lines changed: 108 additions & 25 deletions

File tree

‎lib/internal/abort_controller.js‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,8 @@ function refreshCompositeSignal(signal) {
149149
continue;
150150
}
151151

152-
if (sourceSignal.aborted) {
153-
abortSignal(signal, sourceSignal.reason);
152+
if (sourceSignal[kAborted]) {
153+
abortSignal(signal, sourceSignal[kReason]);
154154
return;
155155
}
156156
}
@@ -170,8 +170,8 @@ function followCompositeSignal(signal) {
170170
continue;
171171
}
172172

173-
if (sourceSignal.aborted) {
174-
abortSignal(signal, sourceSignal.reason);
173+
if (sourceSignal[kAborted]) {
174+
abortSignal(signal, sourceSignal[kReason]);
175175
return;
176176
}
177177

@@ -217,6 +217,14 @@ function setWeakAbortSignalTimeout(weakRef, delay) {
217217
}
218218

219219
class AbortSignal extends EventTarget {
220+
#brand;
221+
222+
static {
223+
converters.AbortSignal = createInterfaceConverter(
224+
'AbortSignal',
225+
(value) => typeof value === 'object' && value !== null && #brand in value,
226+
);
227+
}
220228

221229
/**
222230
* @param {symbol | undefined} dontThrowSymbol
@@ -337,8 +345,9 @@ class AbortSignal extends EventTarget {
337345
gcPersistentSignals.add(signal);
338346
}
339347

340-
if (signal.aborted) {
341-
abortSignal(resultSignal, signal.reason);
348+
refreshCompositeSignal(signal);
349+
if (signal[kAborted]) {
350+
abortSignal(resultSignal, signal[kReason]);
342351
return resultSignal;
343352
}
344353

@@ -348,20 +357,15 @@ class AbortSignal extends EventTarget {
348357
} else if (!signal[kSourceSignals]) {
349358
continue;
350359
} else {
351-
refreshCompositeSignal(signal);
352-
if (signal.aborted) {
353-
abortSignal(resultSignal, signal.reason);
354-
return resultSignal;
355-
}
356360
for (const sourceSignalWeakRef of signal[kSourceSignals]) {
357361
const sourceSignal = sourceSignalWeakRef.deref();
358362
if (!sourceSignal) {
359363
continue;
360364
}
361365
assert(!sourceSignal[kComposite]);
362366

363-
if (sourceSignal.aborted) {
364-
abortSignal(resultSignal, sourceSignal.reason);
367+
if (sourceSignal[kAborted]) {
368+
abortSignal(resultSignal, sourceSignal[kReason]);
365369
return resultSignal;
366370
}
367371

@@ -470,7 +474,6 @@ class AbortSignal extends EventTarget {
470474
}
471475
}
472476

473-
converters.AbortSignal = createInterfaceConverter('AbortSignal', AbortSignal.prototype);
474477
converters['sequence<AbortSignal>'] = createSequenceConverter(converters.AbortSignal);
475478

476479
function ClonedAbortSignal() {

‎lib/internal/crypto/webidl.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,10 @@ const {
1919
isInt32,
2020
isUint32,
2121
} = require('internal/validators');
22-
const { CryptoKey } = require('internal/crypto/webcrypto');
2322
const {
2423
getCryptoKeyAlgorithm,
2524
getCryptoKeyType,
25+
isCryptoKey,
2626
} = require('internal/crypto/keys');
2727
const {
2828
bigIntArrayToUnsignedInt,
@@ -649,7 +649,7 @@ converters.AesCtrParams = createDictionaryConverter(
649649
]);
650650

651651
converters.CryptoKey = createInterfaceConverter(
652-
'CryptoKey', CryptoKey.prototype);
652+
'CryptoKey', isCryptoKey);
653653

654654
converters.EcdhKeyDeriveParams = createDictionaryConverter(
655655
'EcdhKeyDeriveParams', [

‎lib/internal/webidl.js‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ const {
2121
NumberMAX_SAFE_INTEGER,
2222
NumberMIN_SAFE_INTEGER,
2323
ObjectPrototypeHasOwnProperty,
24-
ObjectPrototypeIsPrototypeOf,
2524
SafeArrayIterator,
2625
SafeSet,
2726
String,
@@ -863,13 +862,13 @@ function createSequenceConverter(converter) {
863862
* Creates a converter for a Web IDL interface type.
864863
* @see https://webidl.spec.whatwg.org/#js-interface
865864
* @param {string} name Interface identifier.
866-
* @param {object} prototype Interface prototype object.
865+
* @param {(value: any) => boolean} brandCheck Interface brand predicate.
867866
* @returns {Converter}
868867
*/
869-
function createInterfaceConverter(name, prototype) {
868+
function createInterfaceConverter(name, brandCheck) {
870869
return (V, options = kEmptyObject) => {
871870
// Web IDL interface conversion step 1: return V if it implements I.
872-
if (ObjectPrototypeIsPrototypeOf(prototype, V)) {
871+
if (brandCheck(V)) {
873872
return V;
874873
}
875874
// Step 2: otherwise throw.

‎test/parallel/test-internal-webidl.js‎

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Flags: --expose-internals
22
'use strict';
33

4-
require('../common');
4+
const common = require('../common');
55
const assert = require('assert');
66
const vm = require('vm');
77
const webidl = require('internal/webidl');
@@ -544,10 +544,16 @@ for (const [prototype, value] of [
544544
}
545545

546546
{
547-
class Example {}
547+
class Example {
548+
#brand;
549+
550+
static is(value) {
551+
return typeof value === 'object' && value !== null && #brand in value;
552+
}
553+
}
548554
const converter = webidl.createInterfaceConverter(
549555
'Example',
550-
Example.prototype);
556+
Example.is);
551557
const example = new Example();
552558

553559
assert.strictEqual(converter(example), example);
@@ -556,6 +562,42 @@ for (const [prototype, value] of [
556562
code: 'ERR_INVALID_ARG_TYPE',
557563
message: 'Prefix: Context is not of type Example.',
558564
});
565+
assertInvalidArgType(() => converter({ __proto__: Example.prototype }));
566+
assertInvalidArgType(() => converter(new Proxy(example, {})));
567+
Object.setPrototypeOf(example, null);
568+
assert.strictEqual(converter(example), example);
569+
}
570+
571+
{
572+
const signal = AbortSignal.abort('reason');
573+
for (const value of [
574+
Object.create(AbortSignal.prototype, { aborted: { value: false } }),
575+
{ __proto__: signal },
576+
Object.create(AbortSignal.prototype, Object.getOwnPropertyDescriptors(signal)),
577+
new Proxy(signal, {}),
578+
]) {
579+
assertInvalidArgType(() => converters.AbortSignal(value));
580+
assertInvalidArgType(() => AbortSignal.any([value]));
581+
}
582+
583+
Object.setPrototypeOf(signal, null);
584+
assert.strictEqual(converters.AbortSignal(signal), signal);
585+
const composite = AbortSignal.any([signal]);
586+
assert.strictEqual(composite.aborted, true);
587+
assert.strictEqual(composite.reason, 'reason');
588+
}
589+
590+
{
591+
const controller = new AbortController();
592+
Object.defineProperties(controller.signal, {
593+
aborted: { get: common.mustNotCall('Unexpected aborted getter') },
594+
reason: { get: common.mustNotCall('Unexpected reason getter') },
595+
});
596+
const composite = AbortSignal.any([controller.signal]);
597+
assert.strictEqual(composite.aborted, false);
598+
controller.abort('reason');
599+
assert.strictEqual(composite.aborted, true);
600+
assert.strictEqual(composite.reason, 'reason');
559601
}
560602

561603
{

‎test/parallel/test-webcrypto-cryptokey-brand-check.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ const { subtle } = globalThis.crypto;
5454
assert.strictEqual(Object.getPrototypeOf(internalProto), CryptoKey.prototype);
5555

5656
const invalidThis = { code: 'ERR_INVALID_THIS', name: 'TypeError' };
57+
const invalidArgType = { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError' };
5758

5859
// Plain object receiver.
5960
Object.entries(getters).forEach(([, getter]) => {
@@ -94,10 +95,10 @@ const { subtle } = globalThis.crypto;
9495
assert.strictEqual(isCryptoKey(spoofed), false);
9596
await assert.rejects(
9697
subtle.sign('HMAC', spoofed, Buffer.from('payload')),
97-
invalidThis);
98+
invalidArgType);
9899
await assert.rejects(
99100
subtle.exportKey('jwk', spoofed),
100-
invalidThis);
101+
invalidArgType);
101102

102103
// Subvert `instanceof CryptoKey` via Symbol.hasInstance, then
103104
// invoke the native getters on a forged object. The C++ tag
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
// Flags: --expose-internals
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
const assert = require('assert');
9+
const { subtle } = globalThis.crypto;
10+
const { CryptoKey } = require('internal/crypto/keys');
11+
const { converters } = require('internal/crypto/webidl');
12+
13+
async function main() {
14+
const bytes = new Uint8Array(16);
15+
const key = await subtle.importKey('raw', bytes, 'AES-GCM', true, ['encrypt']);
16+
17+
for (const value of [
18+
{ __proto__: CryptoKey.prototype },
19+
{ __proto__: key },
20+
Object.create(CryptoKey.prototype, Object.getOwnPropertyDescriptors(key)),
21+
new Proxy(key, {}),
22+
]) {
23+
assert.throws(() => converters.CryptoKey(value), {
24+
name: 'TypeError',
25+
code: 'ERR_INVALID_ARG_TYPE',
26+
});
27+
await assert.rejects(subtle.exportKey('raw', value), {
28+
name: 'TypeError',
29+
code: 'ERR_INVALID_ARG_TYPE',
30+
});
31+
}
32+
33+
Object.setPrototypeOf(key, null);
34+
assert.strictEqual(converters.CryptoKey(key), key);
35+
assert.deepStrictEqual(new Uint8Array(await subtle.exportKey('raw', key)), bytes);
36+
}
37+
38+
main().then(common.mustCall());

0 commit comments

Comments
 (0)