this should be benchmarked as it is a pretty central function.
@nodejs/performance any recommendations on what benchmarks should run?

I've made a few changes based on the suggestions, there are more to be made still (such as fixing imports)

I'll go through the file tonight

When ready, I'll squash

It should be faster, but I dont think we can benchmark it as it "only" speeds up the startup time of node.

It should be faster, but I dont think we can benchmark it as it "only" speeds up the startup time of node.

Yes, most of the time. I've noticed that is it used during runtime (a bit).

Now that this PR has been approved, I'll squash when I get a chance

It should be faster, but I dont think we can benchmark it as it "only" speeds up the startup time of node.

StringPrototypeSlice is probably slower than String.prototype.slice

It should be faster, but I dont think we can benchmark it as it "only" speeds up the startup time of node.

StringPrototypeSlice is probably slower than String.prototype.slice

Probably a tiny bit, but it presents less of a security issue, as a user can overwrite String.prototype.slice

Just for the performance comparison:
fastify/help#711

I'll fix the lint issue and squash today.

• Squash
• Lint

  There was a trailing space in an IF statement

I'm ready to merge when you are

I'm ready to merge when you are

This PR requires a full CI run before landing. However, because there's a security release in preparation, the CI request is queued up (notice your PR has the request-ci Add this label to start a Jenkins CI on a PR. label). Once the security release is out, the CI will be unlocked, and it will start processing PRs.

I think the proposition in the PR description is misleading - primordials are not a security measure and they never will be because we will never enforce them throughout the codebase. They are only a UX enhancement (e.g. don't blow up the process just because someone patched a global prototype).

I think further special casing in JS land is not ideal - we should just add --no- entries to the options map/dictionary. And to that end we should split the map into two - one for option -> value pairs, another for option -> option information, so the first one can have additional --no- entries while the second one doesn't need it. I opened #52451 to implement that.

Hey, thanks for this PR! I don't understand this change - this code runs when Node boots before an user code runs when options values are checked (right?)

While this code is ran before runtime, it code is also used during runtime. This means that prototype pollution, while not possible for all cli arguments, is possible for some.

is possible for some.

Can you give an example?

CI: https://ci.nodejs.org/job/node-test-pull-request/58235/

is possible for some.

Can you give an example?

Sure! With a snippet like

const startsWith = String.prototype.startsWith;
Object.defineProperty(String.prototype, "startsWith", {
    value: function (search, pos) {
        if (search === "--no-") {
            process.nextTick(() => {
                console.log(this, search, pos);
            });
        }
        return startsWith.call(this, search, pos);
    }
})

we can see exactly what uses it during runtime.

For example, when I run fetch("https://www.google.com/"), I get the following output:

[String: '--network-family-autoselection'] --no- undefined
[String: '--insecure-http-parser'] --no- undefined
[String: '--trace-tls'] --no- undefined
[String: '--tls-keylog'] --no- undefined
[String: '--tls-cipher-list'] --no- undefined
[String: '--tls-min-v1.0'] --no- undefined
[String: '--tls-min-v1.1'] --no- undefined
[String: '--tls-min-v1.2'] --no- undefined
[String: '--tls-min-v1.3'] --no- undefined
[String: '--tls-max-v1.3'] --no- undefined
[String: '--tls-max-v1.2'] --no- undefined
[String: '--max-http-header-size'] --no- undefined

out/Release/node /Users/iojs/build/workspace/node-test-commit-osx-arm/nodes/osx11/test/pummel/test-crypto-timing-safe-equal-benchmarks.js

Failed a test run, I don't think this change had anything to do with it, but idk

CI: https://ci.nodejs.org/job/node-test-pull-request/58265/

CI: https://ci.nodejs.org/job/node-test-pull-request/58290/

Hi Team, I noticed that the CI bot keeps re-CI-ing this PR, isn't that a waste of resources? Is there a reason for this?

It's not re-CI it's manual due to flakiness

It's not re-CI it's manual due to flakiness

Ah, thanks!

@aduh95 you beat me to it with adding the commit queue label 😃

Great work, everyone!

Landed in bb7d748

🎉