test: throw when using public methods instead of primordials
This PR adds a check on our tests to unsure core doesn't use any built-in methods from the global object which could be mitigated by users.
Simple example
' '.repeat(4)
If that line of code is inside an internal module, it would raise the following error:
Error: Unsafe use of `<object>.repeat(...<arguments>)`. Use `primordials.StringPrototypeRepeat(<object>, ...<arguments>)` instead, or `callUnsafe(<object>, 'repeat', ...<arguments>)`.
When we meant to use the global builtins
Sometimes, we really want to call the user-mitigated methods. A typical example are Thenable
s, where we usually want to support third-party libraries, and not always call Promise.prototype.then
. It's usually done like this:
const then = promise.then;
if (typeof then === 'function') then.call(promise, onSuccess, onError);
That would raise two errors:
Error: Unsafe use of `<object>.call(...<arguments>)`. Use `primordials.ReflectApply(<object>, ...<arguments>)` instead, or `callUnsafe(<object>, 'call', ...<arguments>)`.
Error: Unsafe use of `<object>.then(...<arguments>)`. Use `primordials.PromisePrototypeThen(<object>, ...<arguments>)` instead, or `callUnsafe(<object>, 'then', ...<arguments>)`.
The correctcallUnsafe
:
callUnsafe(promise, 'then', onSuccess, onError);
or
const then = new CallUnsafe(promise, 'then');
if(then.is_callable) then.call(onSuccess, onError);
else { /* Do something else */ }
Limitations
- Unlike a linter check, there is no guarantee this tool can check all unsafe uses of JS builtins. That limitation would go away with a test coverage of 100%.
- I'm using
Error#stack
to test if the call was made by an internal or by the test files. It's detectingnode:
prefix, which was introduced in Node.js 15. I don't think this can be backported to earlier release lines. - There are SO MANY failing tests currently. Changing all internal calls to primordials is an enormous work, however I'm confident it's feasible, maybe by encouraging first-time contributors to pick up certain files. It's a great way of diving into Node.js internals, which helps to better understand how it's working and help gain confidence to contribute further on the code base.
Next steps
We might want to land a modified version without waiting for all the tests to pass:
- Make the
remove-primordials
module opt-in. - Emit warnings instead of throwing errors.
- Have a tool that counts the number of warning to make sure the number is getting down (I.E. we are not introducing more unsafe calls to internals).
Fixes: https://github.com/nodejs/node/issues/30697
Checklist
-
make -j4 test
(UNIX), orvcbuild test
(Windows) passes -
tests and/or benchmarks are included -
commit message follows commit guidelines