fix(core,ext): Fix not to be affected by Promise.prototype.then

[petamoriken]

fix #16196

[petamoriken]

It seems that if I rewrite Promise.prototype.then in repl yet, it will be called. Maybe we need to disallow await.

[petamoriken]

I ran the following file and had no problem. It seems that the problem is not with await, but with Promise.prototype.then being called somewhere in the repl.

console.log("start");

const original = Promise.prototype.then;
Promise.prototype.then = function () {
  console.log("bang!");
  return original.apply(this, arguments);
};

await new Promise((resolve) => setTimeout(resolve, 100));

console.log("end");

[kt3k]

Is it unsafe to expose SafePromise.prototype to user-land? I think it can't be modified even if it's exposed because it's frozen. (I might be missing something...

[petamoriken]

This implementation is referenced from SafePromisePrototypeFinally. I think it is meant to prevent the internal code from being strangely utilized in user-land.

deno/core/00_primordials.js

Lines 439 to 455 in 45ac6e6

	/**
	* Attaches a callback that is invoked when the Promise is settled (fulfilled or
	* rejected). The resolved value cannot be modified from the callback.
	* Prefer using async functions when possible.
	* @param {Promise<any>} thisPromise
	* @param {() => void) | undefined | null} onFinally The callback to execute
	* when the Promise is settled (fulfilled or rejected).
	* @returns A Promise for the completion of the callback.
	*/
	primordials.SafePromisePrototypeFinally = (thisPromise, onFinally) =>
	// Wrapping on a new Promise is necessary to not expose the SafePromise
	// prototype to user-land.
	new Promise((a, b) =>
	new SafePromise((a, b) => PromisePrototypeThen(thisPromise, a, b))
	.finally(onFinally)
	.then(a, b)
	);

[kt3k]

Can you add some integration test cases that modify Promise.prototype.then and check the behaviors of APIs which were vulnerable before this change?

[petamoriken]

@kt3k I added tests for Deno.serve and Deno.spawn.

[kt3k]

LGTM

[kt3k]

While SafePromiseAll is good for preventing tampering of Promise.prototype.then, we need to be careful to use it as it seems having performance penalty compared to Promise.all.

I think the usages of SafePromiseAll in this PR doesn't affect hot paths of benchmarks we've been tracking in the benchmark page.

[magurotuna]

Great to see it's landed! Is it also good to have some lint rule to enforce (or at least make developers be aware of) the use of these new SafePromise stuff?

[kt3k]

Is it also good to have some lint rule to enforce (or at least make developers be aware of) the use of these new SafePromise stuff?

That makes sense to me. SafePromiseAll is added in this PR and I think that can be checked by the linter. I'm not sure if we can do similar check for PromisePrototypeThen as it probably requires the type inference of objects..

[magurotuna]

I'm not sure if we can do similar check for PromisePrototypeThen as it probably requires the type inference of objects..

Yeah that's true. My rudimentary idea is that we have the linter check for all .then(...) and warn them. Yes, this approach would produce lots of false positives, i.e. the linter would warn the call .then(...) even if this then method is not for Promise.
In my opinion, however, false positives are much better than false negatives in this case. When false positives occur we can suppress them, saying "hey linter, trust me, this is a false positive." However, when it comes to false negatives, we can never notice that there is problematic code that's prone to pollution from malicious users.

[magurotuna]

Opened an issue for deno_lint denoland/deno_lint#1095