lib: replace string prototype usage with alternatives

[RedYetiDev]

This PR replaces the use of overridable functions (in strings) with alternatives, to prevent user interference when processing cli options

[MoLow]

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

[RedYetiDev]

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

[RedYetiDev]

When ready, I'll squash

[Uzlopak]

LGTM

[Uzlopak]

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

[RedYetiDev]

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

[MoLow]

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

[RedYetiDev]

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

[Uzlopak]

Just for the performance comparison:
fastify/help#711

[mcollina]

lgtm

[RedYetiDev]

I'll fix the lint issue and squash today.

[RedYetiDev]

• Squash
• Lint

  There was a trailing space in an IF statement

[RedYetiDev]

I'm ready to merge when you are

[aduh95]

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.

[joyeecheung]

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).

[joyeecheung]

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.

[benjamingr]

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?)

I think that implies that prototypes cannot be polluted at this point doesn't it?

[RedYetiDev]

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.

[benjamingr]

is possible for some.

Can you give an example?

[nodejs-github-bot]

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

[RedYetiDev]

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

[RedYetiDev]

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

[benjamingr]

Ah, didn't realize, thanks makes sense (as reliability)

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[RedYetiDev]

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?

[rluvaton]

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

[RedYetiDev]

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

Ah, thanks!

[rluvaton]

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

[RedYetiDev]

Great work, everyone!

[nodejs-github-bot]

Landed in bb7d748

[RedYetiDev]

🎉