doc: add async function caveats to primordials.md

[Renegade334]

No description provided.

[nodejs-github-bot]

Review requested:

• @nodejs/tsc

[aduh95]

IIUC you're trying to show a "JS implementation" of https://tc39.es/ecma262/#sec-promise-resolve, but it's very confusing to have the else Promise.resolve(v) at the end when the whole operation is a PromiseResolve. I think a more accurate description would be:

Suggested change

	let promise;
	if (isPromise(v) && v.constructor === Promise) {
	promise = v;
	} else if (typeof v === 'object' && typeof v.then === 'function') {
	promise = new Promise((resolve, reject) => {
	v.then(resolve, reject);
	});
	} else {
	promise = Promise.resolve(v);
	}
	let promise;
	if (isPromise(v) && Object.is(v.constructor, Promise)) {
	promise = v;
	} else {
	let resolve;
	({ promise, resolve } = Promise.withResolvers());
	// 1. Looks up `then` property on `v` (user-mutable if user-provided).
	// 2. Looks up `then` property on %Promise.prototype% (user-mutable).
	resolve(v);
	}

Maybe we should stick with the style of the other sections that do not try to show the implementation details but just list the properties being potentially accessed:

Suggested change

	let promise;
	if (isPromise(v) && v.constructor === Promise) {
	promise = v;
	} else if (typeof v === 'object' && typeof v.then === 'function') {
	promise = new Promise((resolve, reject) => {
	v.then(resolve, reject);
	});
	} else {
	promise = Promise.resolve(v);
	}
	// 1. Looks up `constructor` property on `v` (user-mutable if user-provided).
	// 2. Looks up `constructor` property on `%Promise.prototype%` (user-mutable).
	// 3. Looks up `then` property on `v` (user-mutable if user-provided).
	// 4. Looks up `then` property on `%Promise.prototype%` (user-mutable).
	const promise = Promise.resolve(v);

[Renegade334]

Understood, but the whole point of the pseudoexample is really to show where the handling short-circuits, and under what conditions the vulnerability occurs. Just saying "look up these properties in order, and stop when you find one", as for the more basic prototype lookups, doesn't adequately describe these operations and doesn't provide enough context for the recommendation imo.

Quite happy to go with something more along the lines of the first suggestion, though.

[aduh95]

Ok sounds good. I still think we need to demonstrate how this can be exploited, e.g.:

// Internal code
async function internalFunction() {
    const internalPromise = new Promise((r) => {r('expected value')});
    const resolvedValue = await internalPromise;
    return resolvedValue;
}

// Userland
delete Promise.prototype.constructor;
Promise.prototype.then = function then(resolve, reject) {
    resolve('overwritten value');
}

// Internal code
PromisePrototypeThen(internalFunction(), console.log); // Outputs: 'overwritten value'

[aduh95]

I think this would be more informative if you shows a code trying to exploit that, having the implementation details is hardly helpful and less convincing IMO

[aduh95]

Also maybe worth specifying it will look up the property of whatever value as long as it’s a non-null Object, e.g.:

$ node --expose-internals -r internal/test/binding <<'EOF'
// Internal code
async function internalFunction() {
    return [];
}

// Userland
Object.prototype.then = function then(resolve, reject) {
    resolve('overwritten value');
}

// Internal code
primordials.PromisePrototypeThen(internalFunction(), console.log); // overwritten value           
EOF
(node:44266) internal/test/binding: These APIs are for internal testing only. Do not use them.
(Use `node --trace-warnings ...` to show where the warning was created)
overwritten value