GeezerVison

Could you please run the CITGM, I'm 90% sure it can break the ecosystem after adding to all the codebase

after that if there are a lot of conflicts I will revert the added proto to the code base and rerun it...

it'd be awesome if the autofix could handle the multiline piece properly too

@ljharb agree but before I fix this I want to run the CITGM as I'm afraid it gonna break the eco, can you please run it for me?

I'm not able to, unfortunately.

You would need to split this PR in chunks small enough to be reviewable, I think the diff is too big to be reviewed in one go, and you are going to run in an endless stream of conflicts.

You would need to split this PR in chunks small enough to be reviewable, I think the diff is too big to be reviewed in one go, and you are going to run in an endless stream of conflicts.

Yeah, I hate to say it, but this is going to have to be done in smaller pieces. Here's one option:

1. Convert this PR to draft mode.
2. Open a few PRs to make the changes that are in this PR but in only a single file (e.g., lib/assert.js) or a coherent set of files (e.g., lib/_http*). So, maybe one PR for lib/assert.js and one for lib/_http* and one for lib/zlib.js. Or whatever ones you want. But small PRs that can be reviewed.
3. They'll all be semver-major and need to go through CITGM, but on the upside, if a change in one PR breaks something in the ecosystem, at least it won't hold up the other changes.
4. Point people to this PR when they invariably comment that there must be a lint rule.
5. Keep rebasing this PR to get rid of conflicts. If that proves to be too much of a pain, close this and open a draft PR with just the lint rule stuff. Keep that one rebased. Eventually, GitHub Actions will show no more things to be fixed and it can be set as Ready For Review.

There are other valid approaches too. But an all-at-once approach like this is unlikely to get reviewed much, I'm sorry to say.

(Also: Thank you for doing this and wanting to make the Node.js runtime more secure!)

Would they be semver-major if the object isn't being passed out to user code?

CIGTM run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3177/ (it did not start yet but I queued it)

CIGTM run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3177/ (it did not start yet but I queued it)

It's unlikely to give us much insight, we are not going to land this PR in this state anyway, no matter the result of this run we will still want it split up in several PRs, and each PR will probably need a CITGM run 🤷‍♂️

Before I continue with this I wanna know what I'm dealing with...

I cancelled the run so it doesn't go in the way of the pending release (scheduled for tomorrow).

Before I continue with this I wanna know what I'm dealing with...

Well I'm afraid you are not going to get anything useful out of it; if the results show lots of regressions, that doesn't mean that we won't introduce the lint rule, just that we need to investigate where they are coming from, and that's going to be much easier on smaller PRs. Even if a CITGM show no regressions on this PR, I would argue this is a waste of time and resources because we probably want to run it again on the PRs that are actually going to land.

@Trott what if we gonna:

1. merge this PR with only the lint rule set as warning without the auto fix
2. Cover the entire code base through number of small PRs
3. Turn on the auto fix and set it to error

This way I won't need to keep rebasing, and I could work on this without much of a conflict

(Not sure if should have autofix enabled as it's can be dangerous)

WDYT?

Before I go through this journey, where should we NOT add the __proto__: null? When returning objects to userland? If so, there are a lot of those and the lint rule would just make a lot of noise when disabling for each one of them

1. merge this PR with only the lint rule set as warning without the auto fix

Our lint jobs treat warnings as errors. Changing that is not impossible but others might oppose the change.

I don't understand why not run CITGM, it will show a lot of regressions, @rluvaton can then go ahead and compare them to the release CITGM and then get an effort estimation of how much extra breakage there is for the whole thing.

It's entirely reasonable from his PoV to want to know roughly how much work there is here and whether he wants to invest time into this. I don't understand the reluctance to run CITGM here? (if it's just the release can you start one after the release?)

Before I go through this journey, where should we NOT add the proto: null?

I think in general this is only needed when we are accessing properties that may not have been defined in the object e.g. checking if it's undefined or falsy, when the object can come from user land or from internal code that does not define these properties. For completely internal objects where the creators always define the keys, a null prototype puts the object in dictionary mode and the lookup would be slower than it could've been. https://v8.dev/blog/fast-properties

Can we run the CITGM after the release, please?

I'm having second thoughts on adding this globally, when I start extracting this into modules, the http was rejected due to performance implications. Also we said we don't want to add it for objects returning to userland which there are quite a few so adding eslint ignore comment will just make noise.

WDYT?

Having the rules would be useful for when we introduce new modules, where we probably want to use null-prototype objects. For example, the test runner code is the perfect place where we would benefit from this; maybe it's the only place where we end up using it, but it's still worth it imo :)

Honestly I'm personally very skeptical of __proto__: null and primordials being useful outside of web standards (where the spec may require it) unless formal security verification is performed with escape analysis.

I also would strongly prefer if we had a build step to do this instead of it being in the codebase so contributors could write "regular" code and it would become primordials in core "under the hood". Even with a build step it would need escape analysis to be useful.

I also would strongly prefer if we had a build step to do this instead of it being in the codebase so contributors could write "regular" code and it would become primordials in core "under the hood". Even with a build step it would need escape analysis to be useful.

does having it as a build step meaning without the eslint rule and other that transform on build? if so, it's super complicated because not all places are equal - userland and some other places

I also would strongly prefer if we had a build step to do this instead of it being in the codebase so contributors could write "regular" code and it would become primordials in core "under the hood". Even with a build step it would need escape analysis to be useful.

That's theoretically impossible in JavaScript. It might be possible in some stricter variant of TypeScript. It was discussed by the TSC and the TSC voted against using a build step, and for using primordials as long as perf were not impacted.

@benjamingr the security risk of NOT using proto null is that anyone can modify Object.prototype and potentially get privileged access to node internals.

@benjamingr the security risk of NOT using proto null is that anyone can modify Object.prototype and potentially get privileged access to node internals.

Right, but by doing this in a non verifiable way we are creating a false sense of "node guards against this" when in fact we don't, creating a false sense of security. If you've given someone else the chance to run code in your Node.js server you pretty much have to assume they have access to anything.

It’s not black or white - every place it’s guarded against is an improvement, even while there remains places that it’s not.

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.