tools: add eslint rule for `proto: null` in object #48646

rluvaton · 2023-07-03T16:40:47Z

instead of each review someone need to remind that you need to add __proto__: null when creating an object, I created this eslint rule

before continue let's agree on the name so I won't need to rename after adding ignore comments

this does not add __proto__: null to module.exports as it break the ecosystem.

cjihrig

should it only run for the lib folder?

Yes, I think so.

cjihrig · 2023-07-03T16:53:05Z

tools/eslint-rules/set-proto-to-null-in-object.js

+        let hasProto = false;
+
+        for (const property of properties) {
+          if (property.key && property.key.name === '__proto__') {


Can we verify that the value is null?

yes, and I'm gonna make it fixable

there are cases where we want to have __proto__ to not be null, see https://github.com/nodejs/node/blob/757c10414776b6f035ced453599527a841f47d6e/tools/eslint-rules/prefer-proto.js

rluvaton · 2023-07-03T17:51:22Z

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

ljharb

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

lib/_http_outgoing.js

lib/_http_incoming.js

rluvaton · 2023-07-03T18:04:58Z

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?

ljharb · 2023-07-03T18:05:33Z

I'm not able to, unfortunately.

aduh95 · 2023-07-03T18:07:21Z

lib/_http_client.js

  let optsWithoutSignal = options;
  if (optsWithoutSignal.signal) {
-    optsWithoutSignal = ObjectAssign({}, options);
+    optsWithoutSignal = ObjectAssign({ __proto__: null }, options);
    delete optsWithoutSignal.signal;
  }


Suggested change

let optsWithoutSignal = options;

if (optsWithoutSignal.signal) {

optsWithoutSignal = ObjectAssign({}, options);

optsWithoutSignal = ObjectAssign({ __proto__: null }, options);

delete optsWithoutSignal.signal;

}

const { signal, ...optsWithoutSignal } = options;

I want to avoid code changes that are not related to this 😄

aduh95 · 2023-07-03T18:12:04Z

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.

lib/zlib.js

Trott · 2023-07-03T18:44:42Z

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:

Convert this PR to draft mode.
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.
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.
Point people to this PR when they invariably comment that there must be a lint rule.
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.

Trott · 2023-07-03T18:45:10Z

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

ljharb · 2023-07-03T18:53:17Z

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

benjamingr · 2023-07-03T20:18:04Z

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

aduh95 · 2023-07-03T21:17:28Z

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 🤷‍♂️

rluvaton · 2023-07-03T21:21:43Z

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

aduh95 · 2023-07-03T21:48:22Z

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.

rluvaton · 2023-07-03T22:06:05Z

@Trott what if we gonna:

merge this PR with only the lint rule set as warning without the auto fix
Cover the entire code base through number of small PRs
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?

rluvaton · 2023-07-03T22:10:46Z

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

Trott · 2023-07-03T23:26:03Z

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.

benjamingr · 2023-07-04T11:48:17Z

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

joyeecheung · 2023-07-04T14:15:18Z

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

rluvaton · 2023-07-05T16:55:57Z

Can we run the CITGM after the release, please?

rluvaton · 2023-07-10T06:41:49Z

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?

aduh95 · 2023-07-12T23:10:31Z

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

benjamingr · 2023-07-13T06:34:20Z

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.

rluvaton · 2023-07-13T07:31:43Z

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

aduh95 · 2023-07-13T09:58:24Z

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.

ljharb · 2023-07-13T12:58:34Z

@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 · 2023-07-15T19:08:35Z

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

ljharb · 2023-07-15T19:17:57Z

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

tools: add eslint rule for __proto__ null in object

f45c225

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jul 3, 2023

rluvaton mentioned this pull request Jul 3, 2023

test_runner: add shards support #48639

Merged

5 tasks

rluvaton added 2 commits July 3, 2023 19:47

tools: made it fixable

361734b

tools: made it fixable

a721c26

cjihrig reviewed Jul 3, 2023

View reviewed changes

rluvaton added 4 commits July 3, 2023 20:04

run only for lib files

94ab2e1

fix eslint rule to not support literal proto property

bb69b0f

ignore adding proto for the module exports

ab4e640

do not add __proto__: null to module.exports

94e8626

rluvaton marked this pull request as ready for review July 3, 2023 17:52

ljharb reviewed Jul 3, 2023

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

lib/_http_incoming.js Outdated Show resolved Hide resolved

aduh95 reviewed Jul 3, 2023

View reviewed changes

Trott reviewed Jul 3, 2023

View reviewed changes

lib/zlib.js Outdated Show resolved Hide resolved

rluvaton added 2 commits July 5, 2023 00:03

fix eslint rule to add multiline when needed

843c110

fix eslint rule for single property object

68b033b

rluvaton force-pushed the add-proto-null-in-object-eslint-rule branch from 1a91514 to 68b033b Compare July 4, 2023 21:09

Merge branch 'main' into add-proto-null-in-object-eslint-rule

aafd615

rluvaton force-pushed the add-proto-null-in-object-eslint-rule branch from a5f145c to aafd615 Compare July 4, 2023 21:18

fix multiline

8726909

rluvaton added 2 commits July 5, 2023 20:35

disable proto null for realm to avoid breaking exports

db06343

disable proto null for module exports

9093d9a

rluvaton force-pushed the add-proto-null-in-object-eslint-rule branch 2 times, most recently from c54494c to 9093d9a Compare July 5, 2023 17:43

rluvaton added 4 commits July 5, 2023 20:43

Merge branch 'main' into add-proto-null-in-object-eslint-rule

48625ca

update comment

ccd24d6

move to multiline the code that keep getting bigger after fix lint

f9cb501

run for the entire codebase

db021f1

This was referenced Jul 5, 2023

assert: add __proto__ null #48661

Open

http: add __proto__ null #48662

Closed

test_runner: add __proto__ null #48663

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: add eslint rule for `proto: null` in object #48646

tools: add eslint rule for `proto: null` in object #48646

rluvaton commented Jul 3, 2023 •

edited

Loading

cjihrig left a comment

cjihrig Jul 3, 2023

rluvaton Jul 3, 2023

rluvaton Jul 3, 2023

rluvaton commented Jul 3, 2023 •

edited

Loading

ljharb left a comment

rluvaton commented Jul 3, 2023

ljharb commented Jul 3, 2023

aduh95 Jul 3, 2023

rluvaton Jul 4, 2023

aduh95 commented Jul 3, 2023

Trott commented Jul 3, 2023

Trott commented Jul 3, 2023

ljharb commented Jul 3, 2023

benjamingr commented Jul 3, 2023 •

edited

Loading

aduh95 commented Jul 3, 2023 •

edited

Loading

rluvaton commented Jul 3, 2023

aduh95 commented Jul 3, 2023

rluvaton commented Jul 3, 2023 •

edited

Loading

rluvaton commented Jul 3, 2023 •

edited

Loading

Trott commented Jul 3, 2023

benjamingr commented Jul 4, 2023

joyeecheung commented Jul 4, 2023

rluvaton commented Jul 5, 2023

rluvaton commented Jul 10, 2023

aduh95 commented Jul 12, 2023

benjamingr commented Jul 13, 2023

rluvaton commented Jul 13, 2023

aduh95 commented Jul 13, 2023

ljharb commented Jul 13, 2023

benjamingr commented Jul 15, 2023

ljharb commented Jul 15, 2023

tools: add eslint rule for __proto__: null in object #48646

Are you sure you want to change the base?

tools: add eslint rule for __proto__: null in object #48646

Conversation

rluvaton commented Jul 3, 2023 • edited Loading

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Jul 3, 2023

Choose a reason for hiding this comment

rluvaton Jul 3, 2023

Choose a reason for hiding this comment

rluvaton Jul 3, 2023

Choose a reason for hiding this comment

rluvaton commented Jul 3, 2023 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

rluvaton commented Jul 3, 2023

ljharb commented Jul 3, 2023

aduh95 Jul 3, 2023

Choose a reason for hiding this comment

rluvaton Jul 4, 2023

Choose a reason for hiding this comment

aduh95 commented Jul 3, 2023

Trott commented Jul 3, 2023

Trott commented Jul 3, 2023

ljharb commented Jul 3, 2023

benjamingr commented Jul 3, 2023 • edited Loading

aduh95 commented Jul 3, 2023 • edited Loading

rluvaton commented Jul 3, 2023

aduh95 commented Jul 3, 2023

rluvaton commented Jul 3, 2023 • edited Loading

rluvaton commented Jul 3, 2023 • edited Loading

Trott commented Jul 3, 2023

benjamingr commented Jul 4, 2023

joyeecheung commented Jul 4, 2023

rluvaton commented Jul 5, 2023

rluvaton commented Jul 10, 2023

aduh95 commented Jul 12, 2023

benjamingr commented Jul 13, 2023

rluvaton commented Jul 13, 2023

aduh95 commented Jul 13, 2023

ljharb commented Jul 13, 2023

benjamingr commented Jul 15, 2023

ljharb commented Jul 15, 2023

tools: add eslint rule for `proto: null` in object #48646

tools: add eslint rule for `proto: null` in object #48646

rluvaton commented Jul 3, 2023 •

edited

Loading

rluvaton commented Jul 3, 2023 •

edited

Loading

benjamingr commented Jul 3, 2023 •

edited

Loading

aduh95 commented Jul 3, 2023 •

edited

Loading

rluvaton commented Jul 3, 2023 •

edited

Loading

rluvaton commented Jul 3, 2023 •

edited

Loading