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

Rule proposal: no-buffer-constructor #5614

Closed
jasnell opened this issue Mar 18, 2016 · 34 comments
Closed

Rule proposal: no-buffer-constructor #5614

jasnell opened this issue Mar 18, 2016 · 34 comments

Comments

@jasnell
Copy link

@jasnell jasnell commented Mar 18, 2016

In the upcoming Node.js v6 release, the existing Buffer() constructors are being soft-deprecated (docs-only deprecation) and replaced with new Buffer.alloc(), Buffer.allocUnsafe() and Buffer.from() alternatives. I am adding a rule to Node.js' own eslint configuration to flag uses of the deprecated constructors (see: nodejs/node#5740). The point has been raised that it may be worthwhile upstreaming this rule so that it can be of benefit to the ecosystem as a whole. Currently the rule is pretty simplistic and can be made a bit more robust if necessary.

The rule catches uses of the Buffer() constructor either as new Buffer(...) or simply Buffer(...) (without the new) and suggests using one of the new constructors instead.

If it is worthwhile, I can open a pull request to submit the rule here.

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Mar 18, 2016

Since we have a number of Node.js-specific rules already, this sounds good to me. The question is basically whether Buffer itself is commonly used in Node.js-- my intuition is yes, but I'm not qualified to decide.

@eslint/eslint-team, any thoughts on whether we should add this to core rules?

@jasnell
Copy link
Author

@jasnell jasnell commented Mar 18, 2016

Yes, in fact it's one of the most broadly used APIs in Node.

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 19, 2016

For additional context, the move to these new Buffer APIs stems from a security vulnerability. Moving forward, this will almost certainly be considered a best practice in Node.

@BYK
Copy link
Member

@BYK BYK commented Mar 19, 2016

👍

@nzakas
Copy link
Member

@nzakas nzakas commented Mar 19, 2016

I'm a bit concerned that this applies only to Node.js 6, and would therefore be confusing alongside other Node.js rules that are generally applicable. As such, I'm not sure it makes sense as a core rule at this point, but what do others think?

@xjamundx
Copy link
Contributor

@xjamundx xjamundx commented Mar 19, 2016

Plugin? 🤔

On Sat, Mar 19, 2016 at 8:54 AM Nicholas C. Zakas notifications@github.com
wrote:

I'm a bit concerned that this applies only to Node.js 6, and would
therefore be confusing alongside other Node.js rules that are generally
applicable. As such, I'm not sure it makes sense as a core rule at this
point, but what do others think?


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#5614 (comment)

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Mar 19, 2016

I assume Buffer.alloc is not going to be available in any version of Node prior to 6? In that case, it might be a bit confusing for users of Node 4 to see this rule in the list.

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Mar 19, 2016

I'm not sure I see a problem there. We have rules around ES6 constructs which either make no sense or actively promote syntax which will fail ES5 parsing. I think we can do the same here as long as we note (and cite a source via hyperlink) that this rule should only be used if you're writing for Node 6+.

@nzakas
Copy link
Member

@nzakas nzakas commented Mar 19, 2016

@platinumazure language versions are different than environment versions. This is more akin to having a rule to use only in jQuery 3.

@jasnell have you considered creating an ESLint plugin from the Node.js rules so you can share them with others?

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Mar 19, 2016

Just FYI: I have a plan that I make a rule to warn deprecated APIs of Node: mysticatea/eslint-plugin-node#26

@jasnell
Copy link
Author

@jasnell jasnell commented Mar 19, 2016

fwiw, we are currently discussing possibly backporting the new constructor APIs to v5 and v4. Creating a plugin in certainly a possibility.

@mysticatea .. that sounds like a great idea.

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Mar 19, 2016

Probably simplest to just put it in a plugin for now, and then when some other decisions are reached (e.g., whether to backport to Node v4/v5), we could revisit this at that time. Just my two cents.

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Mar 20, 2016

I agree. If this will get backported to Node v4/v5 then it's definitely worth revisiting inclusion into the core, but for now, I think it's better served as a plugin

@nzakas
Copy link
Member

@nzakas nzakas commented Mar 22, 2016

I think @mysticatea's plugin is the perfect spot for something like this.

@nzakas nzakas closed this Mar 22, 2016
@jasnell
Copy link
Author

@jasnell jasnell commented Mar 22, 2016

Thank you all very much for the discussion! Definitely appreciate the consideration on this. One quick update on it: we should have a decision tomorrow about whether or not the new API will be backported to v4 but general consensus so far is that it should be.

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Jun 1, 2016

I'm sorry, I remembered this issue now.
I wrote and released node/no-deprecated-api rule before, and it warns the constructor of Buffer.

/*eslint node/no-deprecated-api: "error" */

let a = new Buffer(size);         /*error 'Buffer' constructor was deprecated since v6. Use 'Buffer.alloc()' or 'Buffer.from()' instead.*/
let b = Buffer(size);             /*error 'Buffer' constructor was deprecated since v6. Use 'Buffer.alloc()' or 'Buffer.from()' instead.*/

let buffer = require("buffer");
let c = new buffer.Buffer(size);  /*error 'buffer.Buffer' constructor was deprecated since v6. Use 'buffer.Buffer.alloc()' or 'buffer.Buffer.from()' instead.*/

let Meow = require("buffer").Buffer;
let d = new Meow(size);           /*error 'buffer.Buffer' constructor was deprecated since v6. Use 'buffer.Buffer.alloc()' or 'buffer.Buffer.from()' instead.*/

let {Buffer: Bow} = require("buffer");
let e = new Bow(size);            /*error 'buffer.Buffer' constructor was deprecated since v6. Use 'buffer.Buffer.alloc()' or 'buffer.Buffer.from()' instead.*/

import {Buffer as Wow} from "buffer";
let f = new Wow(size);            /*error 'buffer.Buffer' constructor was deprecated since v6. Use 'buffer.Buffer.alloc()' or 'buffer.Buffer.from()' instead.*/
@Trott
Copy link
Contributor

@Trott Trott commented Mar 9, 2017

I agree. If this will get backported to Node v4/v5 then it's definitely worth revisiting inclusion into the core, but for now, I think it's better served as a plugin

Buffer.alloc() and friends have all been back-ported to Node.js 4.x. Can we re-visit this discussion?

IMO it would be hugely beneficial to the Node.js ecosystem if ESLint provided a rule that flagged use of Buffer() and new Buffer(), instructing people to use Buffer.from(), Buffer.alloc(), or Buffer.allocUnsafe() instead.

Version 10 of standard will flag such things. Even better if ESLint did too.

EDIT: Even better perhaps, flag all deprecated API use the way @mysticatea's plugin does. But if that's too much work or maintenance, the Buffer constructor alone is important enough that it would be incredibly helpful to have it in ESLint...

@Trott
Copy link
Contributor

@Trott Trott commented Mar 15, 2017

Ping! Can we at least get this issue re-opened (or an instruction to open a new issue)?

How to handle Buffer constructor is an important and controversial topic in the Node.js project and knowing that we do (or do not) have important tools like ESLint helping push the boulder uphill is likely to affect how we choose to approach things.

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Mar 15, 2017

@Trott Have you looked at @mysticatea's eslint-plugin-node? If it does what you need, then you can advertise that ESLint plugin to the Node community. (It does a lot of really useful stuff when configured properly, so it's probably worth sharing it in general even if it doesn't yet cover this Buffer case!)

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Mar 15, 2017

Reopening for further discussion since we said we were going to revisit this if the changes were backported.

I'm not entirely opposed to this, but in general I'm hesitant to add rules that pertain specifically to Node APIs. (We do already have some rules like this, but we've tightened our requirements for adding new rules since the existing Node-specific rules were added.)

Could you clarify the advantage of having this be a new rule in core? If the goal is to be able to flag the Buffer() and new Buffer() pattern, then that's already possible with eslint-plugin-node (which is what standard uses to flag the pattern). When selector support is added (hopefully in the next release or two), it will be possible to flag this using the no-restricted-syntax rule without needing a plugin.

@Trott
Copy link
Contributor

@Trott Trott commented Mar 15, 2017

Could you clarify the advantage of having this be a new rule in core?

The goal is to help eliminate use of a deprecated API that often leads to easy-to-miss coding errors with security implications. The goal is to help people move to less error-prone APIs. Unforunately, yes, it's Node.js-specific of course and I totally understand if that makes this a harder sell...

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Mar 16, 2017

@Trott Did you see this message I added yesterday? Your problem might already be solved 😄

@Trott
Copy link
Contributor

@Trott Trott commented Mar 16, 2017

Have you looked at @mysticatea's eslint-plugin-node?

The problem is that a separate plugin not bundled with ESLint is vastly less visibile. No one will find it unless they are actively looking for it. The hope is that the rule that will help curtail this problematic but ubiquitous API in the ecosystem. That won't happen with a separate plugin. That will only happen if the biggest tooling projects (of which ESLint is the biggest/most important) provide the rule.

I'm advocating for tooling projects to take this on because the alternatives are hugely controversial and potentially enormously disruptive to the Node.js ecosystem. See nodejs/node#9531 for the latest iteration of the conversation, although you might want to make sure you top off your coffee/tea mug first. If tooling projects can help us greatly reduce the problematic coding pattern in the ecosystem, other options become more feasible. (Even the "do nothing whatsoever" option probably goes, for at least some people, from totally-unacceptable to we-can-probably-live-with-that if there is evidence that the usage of the API in the ecosystem is decreasing.)

I totally understand if "important to the Node.js project" does not translate to "important to the ESLint project". I actually don't expect this to get a 👍 but I'd be delighted to be wrong. You don't know until you ask, and all that...

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Mar 16, 2017

We do have a few Node specific rules in the core set, so I don't see adding another one as a huge problem. I think this is pretty good example where ESLint can help enforce good practices and avoid potential issues. My only concern is setting a precedence. Node is an ever-growing project. Things like that will most likely keep cropping up, and I'm not sure ESLint core is the right medium for enforcing them.

I would support adding this to the core, with the provision that if, at some point there will be more rules like that, NodeJS team might start thinking about creating official plugin of their own.

@alberto
Copy link
Member

@alberto alberto commented Mar 16, 2017

I agree, this being a security issue, it's worth being included.

However, being a node specific rule, would we be able to enable it by default in eslint:recommended? Otherwise, it would defeat most of the benefits of it being in core.

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Mar 16, 2017

@alberto Currently none of our rules that are specific to Node are in eslint:recommended and I think it's a good idea to keep it that way. It's hard to say how many people use eslint:recommended but my wild guess (without any data to fall back onto) is not that many.

@alberto
Copy link
Member

@alberto alberto commented Mar 16, 2017

Then I don't see much advantage over recommending eslint-plugin-node in the node section of the rules page.

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Mar 16, 2017

@alberto The advantage is that it's more accessible. Otherwise, by the same logic, every rule in the core should be in eslint:recommended and visa versa.

@feross
Copy link
Contributor

@feross feross commented Mar 17, 2017

I think preventing usage of deprecated APIs which have security implications is probably one of the few cases where it makes sense to add another node-related rule in ESLint core. Preventing the anti-pattern where Buffer(var) accidentally returns uninitialized program memory fits in with other similar ESLint rules that prevent unintended program behavior.

Even if it's not included in eslint:recommended, I believe the mere inclusion in ESLint will have a significant effect on the visibility of the rule.

I'm really keen to see community tooling (like standard and hopefully ESLint) step up and discourage soon-to-be-deprecated Node.js APIs. That way, when deprecation finally happens most modules are upgraded and it's a lot less painful for the community.

That said, I understand the maintenance burden that adding another rule entails, so I'll respect the team's decision. 👍

@misterdjules
Copy link

@misterdjules misterdjules commented Mar 20, 2017

It's hard to say how many people use eslint:recommended but my wild guess (without any data to fall back onto) is not that many.

GitHub's search engine 's results can be difficult to interpret, but https://github.com/search?q=eslint%3Arecommended&type=Code seems to suggest usage of eslint:recommended is not insignificant.

Maybe @ChALkeR can help us determine usage a bit better by using https://github.com/ChALkeR/Gzemnid?

@Trott
Copy link
Contributor

@Trott Trott commented Apr 2, 2017

Commenting in the hopes of getting a resolution one way or the other. (I'm under the impression that conversations that stall in the ESLint repo generally end up getting closed after a period of time.)

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Apr 2, 2017

I'm 👍 for including in core.

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Apr 2, 2017

I think we have enough 👍 to accept this, we just need somebody on the team to champion this change.

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Apr 5, 2017

I'll champion this.

@not-an-aardvark not-an-aardvark self-assigned this Apr 5, 2017
@not-an-aardvark not-an-aardvark added accepted and removed evaluating labels Apr 5, 2017
not-an-aardvark added a commit that referenced this issue Apr 5, 2017
not-an-aardvark added a commit that referenced this issue Apr 5, 2017
not-an-aardvark added a commit that referenced this issue Apr 5, 2017
@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018
@eslint eslint bot added the archived due to age label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.