Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node: warn for Object.prototype.__* accessors common in security warnings #39824

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Aug 20, 2021

These accessors cause too much noise in npm security audits for the ecosystem, we should start a path to removal. Figuring out/warning on usage seems a good first step prior to actual removal. This would also be a way to verify that a security audit warning actually is affecting something rather than being false positives. These are optional/legacy in the JS language specification https://tc39.es/ecma262/#sec-object.prototype-legacy-accessor-methods

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 20, 2021
warnedForProto = true;
process.emitWarning('usage of Object.prototype.__* properties are a' +
' common security concern, use static methods on Object instead');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should make these proper runtime deprecations?
/cc @addaleax

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed, is it just adding to the docs a new number to get the DEP###?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added to DEP doc, idk if there is a process to do instead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look for other DEP codes in the code and you'll see how those are emitted :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:

node/lib/sys.js

Lines 28 to 29 in e46c680

process.emitWarning('sys is deprecated. Use util instead.',
'DeprecationWarning', 'DEP0025');

As soon as this is changed to emit a deprecation warning, I'll launch a CITGM run with --throw-deprecation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems difficult to wrangle without giving a unique DEP id to each accessor given the other feedback of giving more actionable feedback in #39824 (comment)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to document this somewhere. Most experienced folks would now but not newbies.

@mcollina
Copy link
Member

Here is a relevant discussion: #31951

Co-authored-by: Voltrex <mohammadkeyvanzade94@gmail.com>
doc/api/deprecations.md Outdated Show resolved Hide resolved

const common = require('../common');

process.on('warning', common.mustCall());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a common.expectWarning() utility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, but it seems deprecate(...) in lib seems to only fire once per dep code so having specialized messages per accessor seems to require unique DEP codes.

doc/api/deprecations.md Outdated Show resolved Hide resolved
@targos
Copy link
Member

targos commented Aug 22, 2021

CITGM results: https://ci.nodejs.org/job/citgm-smoker/nodes=ubuntu1804-64/2751/#showFailuresLink

Unfortunately, all modules that depend on node-gyp fail at install time because of another deprecation.

I'm stopping here but there are many more.

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cli-table is used by 375k GitHub repositories and >2.5k npm packages
chalk versions < 4 are probably used by a lot of projects
yargs is used by millions of projects
express is used by millions of projects

Maybe we should start with a pending deprecation?

@addaleax addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Aug 22, 2021
* `Object.prototype.__defineSetter__`
* `Object.prototype.__lookupGetter__`
* `Object.prototype.__lookupSetter__`
* `Object.prototype.__proto__`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that __proto__ is annoying, but it’s also widely used (which is why I had excluded it from #39576 as well). Runtime-deprecating that is a big breaking change.

Copy link
Member Author

@bmeck bmeck Aug 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__proto__ is the main problem for CVEs regarding prototype pollution, it is the most important to figure out how to address. I am not arguing popularity or utility, just that the API is problematic in practice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right – that’s what --disable-proto is for, no? Anyway, if we do this, it should be communicated very clearly to users.

(I also don’t think putting this behind --pending-deprecation is particularly useful, given that there already is a flag to opt-out of this behavior. If we do make the decision to runtime-deprecate fully eventually, then I guess that decision can also be made now.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--disable-proto is a bit different, it doesn't let you see that something is using __proto__ so you can fix it. It just removes it or makes it throw; also it is opt-in so the ecosystem noise is just permanent since it doesn't actually cause any sort of signal that security warning isn't a false positive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I would say that throwing exceptions definitely lets you do that :)

In any case, to be clear I’m not -1 on this per se, I just think that this is a big change and we should call it out very explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing alters behavior, so maybe --disable-proto could get a warning mode that lets programs run and you fix it when you see it rather than taking down a process potentially?

Seems fine to have whole blog posts before this and waiting for a major to me on this PR. Since this affects legacy codebases as well it will likely also take some effort to PR things.

We could also add a flag to re-add the accessors if we ever do remove them for people needing to run legacy code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing alters behavior, so maybe --disable-proto could get a warning mode that lets programs run and you fix it when you see it rather than taking down a process potentially?

I mean – yes, throwing alters behavior, but practically speaking, people will notice whether they are using __proto__ with either method, which is the point here anyw2ay.

We could also add a flag to re-add the accessors if we ever do remove them for people needing to run legacy code.

Yeah, I think in the long run that might be a good idea – just remove the accessors, but add a flag to add them for those who really need them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad that --disable-proto=log wasn't an option ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, --disable-proto=throw during development should spot all issues (if using a package lock).

@bmeck
Copy link
Member Author

bmeck commented Aug 22, 2021

@targos added a guard

@bmeck
Copy link
Member Author

bmeck commented Aug 22, 2021

@targos after some digging, a variety of those are all from the test reporter used: tapjs/tap-mocha-reporter#68

@bmeck
Copy link
Member Author

bmeck commented Oct 28, 2021

we have patched some stuff in the wild, we should run CITGM again

doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
@bmeck
Copy link
Member Author

bmeck commented Mar 4, 2022

https://github.com/tapjs/tap-mocha-reporter has landed a fix, we should be good to try a new CITGM to see what the status is in the ecosystem.

bmeck and others added 2 commits March 4, 2022 15:44
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
@bmeck
Copy link
Member Author

bmeck commented Mar 4, 2022

CITGM smoker running https://ci.nodejs.org/job/citgm-smoker/2863/

@aduh95
Copy link
Contributor

aduh95 commented Apr 6, 2022

The last CITGM run reports 81 failures, which seems to be more or less the same as master. Maybe we could land this in v18.0.0?

@bmeck can you rebase please?

@bmeck
Copy link
Member Author

bmeck commented Apr 6, 2022 via email

@mcollina
Copy link
Member

mcollina commented Sep 9, 2022

Congrats!

@kapouer
Copy link
Contributor

kapouer commented Sep 9, 2022

Was passing by here too. Nice one. Babies are so cute, and they don't have proto !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet