Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 76012d0

Browse files
committedJan 27, 2025·
fix(pass-style): fix #2700 ignore more safe async_hook extra properties
1 parent 895ea0a commit 76012d0

File tree

3 files changed

+85
-97
lines changed

3 files changed

+85
-97
lines changed
 

‎packages/pass-style/src/safe-promise.js

+77-87
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,86 @@
22

33
import { isPromise } from '@endo/promise-kit';
44
import { q } from '@endo/errors';
5-
import { assertChecker, hasOwnPropertyOf, CX } from './passStyle-helpers.js';
5+
import {
6+
assertChecker,
7+
hasOwnPropertyOf,
8+
CX,
9+
isObject,
10+
} from './passStyle-helpers.js';
611

712
/** @import {Checker} from './types.js' */
813

914
const { isFrozen, getPrototypeOf, getOwnPropertyDescriptor } = Object;
1015
const { ownKeys } = Reflect;
11-
const { toStringTag } = Symbol;
1216

1317
/**
18+
* Explicitly tolerate symbol-named non-configurable non-writable data
19+
* property whose value is obviously harmless, such as a primitive value.
20+
*
21+
* The motivations are to tolerate `@@toStringTag` and those properties
22+
* that might be added by Node's async_hooks. Thus, beyond primitives, the
23+
* only values that must be tolerated are those safe values that might be
24+
* added by async_hooks.
25+
*
26+
* At the time of this writing, Node's async_hooks contains the
27+
* following code, which we need to tolerate if safe:
28+
*
29+
* ```js
30+
* function destroyTracking(promise, parent) {
31+
* trackPromise(promise, parent);
32+
* const asyncId = promise[async_id_symbol];
33+
* const destroyed = { destroyed: false };
34+
* promise[destroyedSymbol] = destroyed;
35+
* registerDestroyHook(promise, asyncId, destroyed);
36+
* }
37+
* ```
38+
*
39+
* @param {object} obj
40+
* @param {string|symbol} key
41+
* @param {Checker} check
42+
*/
43+
const checkSafeOwnKeyOf = (obj, key, check) => {
44+
const desc = getOwnPropertyDescriptor(obj, key);
45+
assert(desc);
46+
const quoteKey = q(String(key));
47+
if (!hasOwnPropertyOf(desc, 'value')) {
48+
return CX(
49+
check,
50+
)`Own ${quoteKey} must be a data property, not an accessor: ${obj}`;
51+
}
52+
const { value, writable, configurable } = desc;
53+
if (writable) {
54+
return CX(check)`Own ${quoteKey} must not be writable: ${obj}`;
55+
}
56+
if (configurable) {
57+
return CX(check)`Own ${quoteKey} must not be configurable: ${obj}`;
58+
}
59+
if (!isObject(value)) {
60+
return true;
61+
}
62+
63+
if (
64+
typeof value === 'object' &&
65+
value !== null &&
66+
isFrozen(value) &&
67+
getPrototypeOf(value) === Object.prototype
68+
) {
69+
const subKeys = ownKeys(value);
70+
if (subKeys.length === 0) {
71+
return true;
72+
}
73+
74+
if (subKeys.length === 1 && subKeys[0] === 'destroyed') {
75+
return checkSafeOwnKeyOf(value, 'destroyed', check);
76+
}
77+
}
78+
return CX(
79+
check,
80+
)`Unexpected Node async_hooks additions: ${obj}[${quoteKey}] is ${value}`;
81+
};
82+
83+
/**
84+
* @see https://github.com/endojs/endo/issues/2700
1485
* @param {Promise} pr The value to examine
1586
* @param {Checker} check
1687
* @returns {pr is Promise} Whether it is a safe promise
@@ -22,96 +93,15 @@ const checkPromiseOwnKeys = (pr, check) => {
2293
return true;
2394
}
2495

25-
/**
26-
* This excludes those symbol-named own properties that are also found on
27-
* `Promise.prototype`, so that overrides of these properties can be
28-
* explicitly tolerated if they pass the `checkSafeOwnKey` check below.
29-
* In particular, we wish to tolerate
30-
* * An overriding `toStringTag` non-enumerable data property
31-
* with a string value.
32-
* * Those own properties that might be added by Node's async_hooks.
33-
*/
34-
const unknownKeys = keys.filter(
35-
key => typeof key !== 'symbol' || !hasOwnPropertyOf(Promise.prototype, key),
36-
);
96+
const stringKeys = keys.filter(key => typeof key !== 'symbol');
3797

38-
if (unknownKeys.length !== 0) {
98+
if (stringKeys.length !== 0) {
3999
return CX(
40100
check,
41-
)`${pr} - Must not have any own properties: ${q(unknownKeys)}`;
101+
)`${pr} - Must not have any string-named own properties: ${q(stringKeys)}`;
42102
}
43103

44-
/**
45-
* Explicitly tolerate a `toStringTag` symbol-named non-enumerable
46-
* data property whose value is a string. Otherwise, tolerate those
47-
* symbol-named properties that might be added by NodeJS's async_hooks,
48-
* if they obey the expected safety properties.
49-
*
50-
* At the time of this writing, Node's async_hooks contains the
51-
* following code, which we can safely tolerate
52-
*
53-
* ```js
54-
* function destroyTracking(promise, parent) {
55-
* trackPromise(promise, parent);
56-
* const asyncId = promise[async_id_symbol];
57-
* const destroyed = { destroyed: false };
58-
* promise[destroyedSymbol] = destroyed;
59-
* registerDestroyHook(promise, asyncId, destroyed);
60-
* }
61-
* ```
62-
*
63-
* @param {string|symbol} key
64-
*/
65-
const checkSafeOwnKey = key => {
66-
if (key === toStringTag) {
67-
// TODO should we also enforce anything on the contents of the string,
68-
// such as that it must start with `'Promise'`?
69-
const tagDesc = getOwnPropertyDescriptor(pr, toStringTag);
70-
assert(tagDesc !== undefined);
71-
return (
72-
(hasOwnPropertyOf(tagDesc, 'value') ||
73-
CX(
74-
check,
75-
)`Own @@toStringTag must be a data property, not an accessor: ${q(tagDesc)}`) &&
76-
(typeof tagDesc.value === 'string' ||
77-
CX(
78-
check,
79-
)`Own @@toStringTag value must be a string: ${q(tagDesc.value)}`) &&
80-
(!tagDesc.enumerable ||
81-
CX(check)`Own @@toStringTag must not be enumerable: ${q(tagDesc)}`)
82-
);
83-
}
84-
const val = pr[key];
85-
if (val === undefined || typeof val === 'number') {
86-
return true;
87-
}
88-
if (
89-
typeof val === 'object' &&
90-
val !== null &&
91-
isFrozen(val) &&
92-
getPrototypeOf(val) === Object.prototype
93-
) {
94-
const subKeys = ownKeys(val);
95-
if (subKeys.length === 0) {
96-
return true;
97-
}
98-
99-
if (
100-
subKeys.length === 1 &&
101-
subKeys[0] === 'destroyed' &&
102-
val.destroyed === false
103-
) {
104-
return true;
105-
}
106-
}
107-
return CX(
108-
check,
109-
)`Unexpected Node async_hooks additions to promise: ${pr}.${q(
110-
String(key),
111-
)} is ${val}`;
112-
};
113-
114-
return keys.every(checkSafeOwnKey);
104+
return keys.every(key => checkSafeOwnKeyOf(pr, key, check));
115105
};
116106

117107
/**

‎packages/pass-style/test/passStyleOf.test.js

+4-2
Original file line numberDiff line numberDiff line change
@@ -116,14 +116,16 @@ test('some passStyleOf rejections', t => {
116116
prbad2.extra = 'unexpected own property';
117117
harden(prbad2);
118118
t.throws(() => passStyleOf(prbad2), {
119-
message: /\[Promise\]" - Must not have any own properties: \["extra"\]/,
119+
message:
120+
/\[Promise\]" - Must not have any string-named own properties: \["extra"\]/,
120121
});
121122

122123
const prbad3 = Promise.resolve();
123124
Object.defineProperty(prbad3, 'then', { value: () => 'bad then' });
124125
harden(prbad3);
125126
t.throws(() => passStyleOf(prbad3), {
126-
message: /\[Promise\]" - Must not have any own properties: \["then"\]/,
127+
message:
128+
/\[Promise\]" - Must not have any string-named own properties: \["then"\]/,
127129
});
128130

129131
const thenable1 = harden({ then: () => 'thenable' });

‎packages/pass-style/test/safe-promise.test.js

+4-8
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ test('safe promise loophole', t => {
1818
const p2 = Promise.resolve('p2');
1919
p2.silly = 'silly own property';
2020
t.throws(() => passStyleOf(harden(p2)), {
21-
message: '"[Promise]" - Must not have any own properties: ["silly"]',
21+
message:
22+
'"[Promise]" - Must not have any string-named own properties: ["silly"]',
2223
});
2324
t.is(p2[toStringTag], 'Promise');
2425
t.is(`${p2}`, '[object Promise]');
@@ -39,9 +40,7 @@ test('safe promise loophole', t => {
3940
defineProperty(p3, toStringTag, {
4041
value: 3,
4142
});
42-
t.throws(() => passStyleOf(harden(p3)), {
43-
message: 'Own @@toStringTag value must be a string: 3',
44-
});
43+
t.is(passStyleOf(harden(p3)), 'promise');
4544
}
4645

4746
{
@@ -50,10 +49,7 @@ test('safe promise loophole', t => {
5049
value: 'Promise p4',
5150
enumerable: true,
5251
});
53-
t.throws(() => passStyleOf(harden(p4)), {
54-
message:
55-
'Own @@toStringTag must not be enumerable: {"configurable":false,"enumerable":true,"value":"Promise p4","writable":false}',
56-
});
52+
t.is(passStyleOf(harden(p4)), 'promise');
5753

5854
const p5 = Promise.resolve('p5');
5955
defineProperty(p5, toStringTag, {

0 commit comments

Comments
 (0)