lib: replace string prototype usage with alternatives#52440
Conversation
|
this should be benchmarked as it is a pretty central function. |
|
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. |
Yes, most of the time. I've noticed that is it used during runtime (a bit).
|
|
Probably a tiny bit, but it presents less of a security issue, as a user can overwrite |
|
Just for the performance comparison: |
|
I'll fix the lint issue and squash today. |
|
|
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
|
|
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 |
benjamingr
left a comment
There was a problem hiding this comment.
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?
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. |
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 |
|
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
left a comment
There was a problem hiding this comment.
Ah, didn't realize, thanks makes sense (as reliability)
|
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 |
Ah, thanks! |
|
@aduh95 you beat me to it with adding the commit queue label 😃 |
|
Great work, everyone! |
|
Landed in bb7d748 |
🎉 |
PR-URL: #52440 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #52440 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #52440 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This PR replaces the use of overridable functions (in strings) with alternatives, to prevent user interference when processing cli options