Skip to content

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 Thenables, 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 correct™️ way of doing that would indeed be to use callUnsafe:

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 detecting node: 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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Merge request reports

Loading