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. |
RedYetiDev
changed the title
|
|
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 |
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? |
github-actions
bot
removed
the
request-ci
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 |
|
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
added
author ready
|
@aduh95 you beat me to it with adding the commit queue label 😃 |
|
Great work, everyone! |
nodejs-github-bot
removed
the
commit-queue
|
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>
PR-URL: nodejs#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