search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
105.44k stars 28.6k forks source link

Tracking Issue for Runtime Deprecation of Buffer constructor #19079

Closed Trott closed 6 years ago

Trott commented 6 years ago

TSC approved runtime-deprecation of Buffer constructor for 10.0.0.

Here's the tracking list. @nodejs/tsc: Feel free to edit this to add more items to the checklist. I'm just getting it started, really.

What have I missed?

@nodejs/community-committee @nodejs/buffer

FWIW, here's how the TSC voted:

mafintosh commented 6 years ago

So just so people here get a sense of scale of the impact, this makes 141 modules i maintain atm print a deprecation. This is probably by far the most disruptive Node.js change I've experienced.

shelleyp commented 6 years ago

@ChALkeR I know you don't want to get into author shaming by publishing the list publicly, but I have to wonder if putting this list out, with concerns, couldn't encourage a community effort to update modules.

I was looking at the earlier list, and thinking that I need to roll up my sleeves and dig in.

Would a community effort to update modules create more harm than help?

BridgeAR commented 6 years ago

@shelleyp this has nothing to do with public shaming as there is nothing bad if someone used the Buffer constructor. Not having it changed is also nothing bad as it can be a huge overhead for the maintainers as e.g., described by @mafintosh.

BridgeAR commented 6 years ago

I actually consider building a small bot that would just open PRs in all modules with the buffer constructor. I do not think that it is feasible to manually open enough PRs to really get the upper hand for this. Having a bot should ease the pain a lot and parsing and fixing the code should be relatively straight forward in most cases. There might be some false positives but that is something that the maintainers should know when looking at the opened PR. What do others think about creating a bot for the problem?

ChALkeR commented 6 years ago

@BridgeAR Automatic fixing is not possible in cases where the argument isn't literal, and in most cases it's not literal. Fixing that requires human intervention atm.

I am planning to make a bot that would help opening issues (or otherwise let authors know, perhaps with their consent) citing exact problematic lines on top of Gzemnid, but that would be after 10.0 release.

For now, I think that a combination of manual PRs to top packages and sending authors lists of exact lines in all their modules has a significant positive effect.

shelleyp commented 6 years ago

@BridgeAR I just made an assumption as to why @ChALkeR was keeping the list private. I'm sure there's another reason. And I agree, buffer constructor was an accepted coding construct.

My point was, and still is, couldn't we have a community effort to take on a list of modules that need updated and possibly see about updating as many as can be updated before Node v10 is released. That way the burden isn't solely on the module authors.

targos commented 6 years ago

Just noting that the latest version of npm (also the next one) is affected

ChALkeR commented 6 years ago

@targos Yes, mostly due to deps. That is known. I filed some issues about that already and got 3 at least PRs merged in npm deps =).

ChALkeR commented 6 years ago

@shelleyp Generally speaking, there could be security implications in entries of old Buffer API usage, this is the main reason why I am not sharing an easy to consume list of all of those.

Re: community effort, I share a public list of specific top-impact packages after initial review of those, that list is available at https://github.com/nodejs/node/issues/19079#issuecomment-374536006 and includes the most important packages, so that people willing to help would be able to file PRs to those. Once those are processed — I will add new entries to the public list.

shelleyp commented 6 years ago

@ChALkeR Ah, slapping head, of course. Nothing like providing a map showing people where they can wreck havoc.

Too bad, though. It could have been a good community exercise.

Sounds good on the list you can provide. I'll link to it from an writing I'm doing on Buffer constructors and the new Node version.

ChALkeR commented 6 years ago

@targos @Trott npm primarily depends on the flush-write-stream geting published (the fix already landed, ping @mafintosh for the semver-patch version release).

After that, with a bit lower significance (but still very high) — at sshpk update. Fixing sshpk is very important =). Upd: I'm on it.

mafintosh commented 6 years ago

FYI, I'm fine publishing a map of my own stuff. There is no way I'm getting to get anywhere close to updating them in time for this release.

ghost commented 6 years ago

I second @mafintosh's concerns and I think these changes will be hugely disruptive to userland. The other thing to consider is that the ecosystem is largely a product of people gifting their work for free to support a commons. I think it's hugely disrespectful to demand that people update their code because of changes in node core in this way, and there will be a significant, non-trivial number of packages that will never update. That problem is compounded by dependencies of dependencies, where any package in your dependency graph could emit these warnings and you may be relatively powerless to prevent these issues aside from forking every package along that critical path to the offending module.

If this change is so important to node core, has anyone considered paying people to do this work? This is a very large, disruptive change that will very negatively impact the ecosystem unless a lot of effort and compensation is put into a coordinated effort.

ChALkeR commented 6 years ago

@substack This change is important not to node core (which also largely consists of people gifting their work for free, surprize-surprize), but to ecosystem to reduce the security impact and the potential damage caused on the ecosystem by insecure Buffer API usage, and to people using those modules from the ecosystem. This is not something core has decided to do at a whim. I suggest performing some research before writing condemning comments.

AyushG3112 commented 6 years ago

I use a tool https://snyk.io/, to scan my projects for vulnerable dependencies. These guys have a vulnerability database with is matched with your projects' package.json, and can find a bunch of vulnerabilities in your projects dependencies, including but not limited to use of the Buffer constructor.

I'd say we scan our projects if possible to find which packages are vulnerable and start opening issues and PRs?

P.S. I'm not affiliated to Snyk.

ChALkeR commented 6 years ago

@AyushG3112 Not all usage of Buffer constructor is reported there, and even not all insecure usage of Buffer constructor is. But fixing those would be a good thing.

watson commented 6 years ago

It's worth pointing out that the Buffer constructor is in it self not insecure. The issue is that it's easy to write code that's insecure by accident when using the Buffer constructor. So Snyk (and similar) will never mark all modules that uses the of Buffer constructor as insecure.

jasnell commented 6 years ago

I have been pro runtime deprecation for this in 10.0.0 largely because... in an ideal world, that's exactly the right thing to do. Unfortunately, the way runtime deprecations are currently handled by core makes this entirely too disruptive right now. After going through an evaluating the position @mafintosh is in on this, I'm forced to admit that I'm reconsidering my position on this for 10.0.0. For 10.0.0, we should keep the --pending-deprecation gate on the warning but we shouldn't leave it at that. We need to revisit how we do runtime deprecations in general and put focus on developing a better approach to handling these. @mafintosh's suggestion that we emit the process warning only for usercode is a viable path. Changing the way we emit deprecation warnings is another.

Not deprecating legacy APIs is not a solution. Node.js is not a sandboxed environment. Our trust model means that we must be able to deprecate and make breaking changes when it is necessary to do so.

geek commented 6 years ago

If we aren't going to "runtime-deprecate" something that's been "pending-deprecation" for years then we made the wrong choice in marking it for "pending-deprecation" to begin with. We should either "runtime-deprecate" it in 10 or remove it from pending deprecation entirely.

lpinca commented 6 years ago

The point is that nothing will change if we keep postponing this. The same problem and discussion will arise again in version 11, then in 12 and so on. We should draw a line and stick with it or not draw it at all.

watson commented 6 years ago

@geek @lpinca what @jasnell is saying is that we should consider having a better way of doing deprecation first, not that we shouldn't deprecate it at all or just postpone it

benjamingr commented 6 years ago

Note that Node can also do this gradually:

I agree this is frustrating - but I still shudder when thinking about graceful-fs and ecosystem breakage. I get that new Buffer has been deprecated for a while now - but the fact authors like @mafintosh and @substack feel that this is sudden and threatening to their work is also a very valid concern IMO.

@substack

The other thing to consider is that the ecosystem is largely a product of people gifting their work for free to support a commons. I think it's hugely disrespectful to demand that people update their code because of changes in node core in this way, and there will be a significant, non-trivial number of packages that will never update.

While I agree with that part (as a part of the ecosystem) note that this is also largely the case for Node.js core, most of us are doing work for free to support a commons because we believe it helps better - so it's important to remember that the people for a runtime deprecation are in the exact same boat here.

joyeecheung commented 6 years ago

I may be joking so feel free to ignore me, but there is another way to deprecate things (Inspired by https://twitter.com/johnregehr/status/920691341738123264?lang=en):

diff --git a/lib/buffer.js b/lib/buffer.js
index b369d27a1e..39227f1c2f 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -172,7 +172,11 @@ const doFlaggedDeprecation =
  * Deprecation Code: DEP0005
  **/
 function Buffer(arg, encodingOrOffset, length) {
-  doFlaggedDeprecation();
+  let num = 0;
+  for (let i = 0; i < 1e7; ++i) {
+    num += Date.now();
+  }
+  require('assert')(num);  // in case it gets optimized away
   // Common case.
   if (typeof arg === 'number') {
     if (typeof encodingOrOffset === 'string') {
ChALkeR commented 6 years ago

Delaying this won't help, the situation becomes worse over time. I actually measured the ecosystem usage and I know what I am talking about, this is not just something I am imagining out of my head.

Any other approach won't help to resolve those potential security issues (which sum up into large security holes in real software) — as developers of deps won't get informed and won't fix those. Users should be informed of the location of potentially unsafe code in their deps, and the warning is way to achieve that.

The only way to mitigate the huge security impact of this problem is to actually fix all the modules. Delaying only suggests not doing that. The module authors are not happy that they need to fix the code, but the warning is just the messenger.

Without the warning, this issue will just continue piling up.

Ecosystem usage of unsafe Buffer API grows over time by itself, waiting more would actually mean more porting work done by ecosystem module authors, not less.

What we could do is to gradually deprecate it in 10, but as a dependency over time, not 10.x version. I already metioned that earlier and will elaborate again today — but the intention is to decrease the number of users seeing the deprecation warnings, not to reduce or delay the porting work.

There is nothing we can do with module authors having to upgrade the code (except for helping them to do that, by providing PRs, exact usage lists and things like an excelent tool by @joyeecheung). The deprecation is not the reason why that code needs to be upgraded in the ecosystem. The massive amount of potential security issues that transform into a significant number of real security holes is that in turn affect real people out there is.

And if we don't do a runtime deprecation now — the issue would become just worse with more code needing to be ported and more work needing to be done.

ChALkeR commented 6 years ago

@joyeecheung You are not the first one mentioning that, and no — synchronously waiting is going to cause more impact and more havoc to the ecosystem than just printing a concise and suppressible warning to the console.

jasnell commented 6 years ago

The proposal I'm at right now:

  1. Revert the runtime deprecation for 10.0.0... return to the --pending-deprecation
  2. Modify core's runtime deprecation warnings such that the console output is not emitted by default for code running in node-modules. The process.on('warning') event would still be emitted. When --trace-deprecation or --throw-deprecation is used, the console output for node-modules would be enabled. Point being, it's explicitly opt-in.
  3. Once 2 is done, re-apply the runtime deprecation for new Buffer within 10.x before it goes LTS in October
  4. Work with TC-39 on a longer term improved deprecation strategy for JavaScript is general so we don't keep fighting the same problems for eternity. (beginnings of a rough proposal here)
  5. In parallel to all this, keep actively working with the ecosystem to update existing modules to use the new, safer APIs
benjamingr commented 6 years ago

@jasnell @ChALkeR it sounds to me like deprecation for non node_modules runtime deprecation is a good strategy for 10.0. I still feel like we should runtime deprecate it (inside node_modules) eventually though.

jasnell commented 6 years ago

@benjamingr ... deprecating is usercode and not node-modules would end up having the side-effect of causing the deprecation warning to show up in test runs for the modules (e.g. every time someone runs npm test). That, along with an active effort to help userland move to the new APIs would hopefully allow us to make some progress on this.

addaleax commented 6 years ago

Modify core's runtime deprecation warnings such that the console output is not emitted by default for code running in node-modules.

You know that's trickier than this -- we mostly do runtime deprecations to warn about API usage that is going to be broken in some way, which is also something that application users do want to know about; this does not fall into that category.

We could and should consider doing per-case decisions for this, though.

jasnell commented 6 years ago

One thought I had was to modify the pending deprecation mechanism such that it would always emit when running as user-code and only emit for node-modules when the --pending-deprecation flag was used.

ChALkeR commented 6 years ago

@benjamingr No, just excluding node_modules is not a good strategy — that would mean that most of the packages won't be fixed for yet another 6 months if not a year. See the discussion at https://github.com/nodejs/node/pull/15346#issuecomment-369559160.

See also https://github.com/nodejs/node/issues/19079#issuecomment-373938852 for an alternate suggestion (excluding node_modules at 99% chance and at 1% — printing the warning for that).

mcollina commented 6 years ago

I'm 👍 on making it node_modules only. However, I'm also ok in runtime deprecate it in 10.0.0 before we go LTS. I'm also ok on @ChALkeR suggestion in https://github.com/nodejs/node/issues/19079#issuecomment-375022270. We need to do something on this, as it is getting out of track. They must be deprecated eventually.

This does not change the basic economics of what @substack described in https://github.com/nodejs/node/issues/19079#issuecomment-374947889. This is a lot of work, and someone has to do it. We can decide that we will not deprecate them, because the amount of legacy code to change is too big. IMHO, buying 6 months is a good strategy if we spend 6 months fixing the issue everywhere. If we don't, then we have just delayed the problem. I'm -1 on that.

Trott commented 6 years ago

Revert the runtime deprecation for 10.0.0.

@jasnell Nothing to revert yet because it hasn't landed. So you can get to work on that new runtime deprecation warning setup straightaway. :-D

EDIT: This next paragraph is not directed at @jasnell. It's an FYI for anyone and everyone.

By the way, to repeat something that @addaleax wrote: Put export NODE_PENDING_DEPRECATION=1 in your .bashrc to get a feel for just how noisy/disruptive this change would be right now. (That's not necessarily an argument against the change. You could also take it as "OMG, there's so much Buffer ctor usage out there, we need to do something!" It's a piece of information.)

jasnell commented 6 years ago

Nothing to revert yet ...

Ah, good, I couldn't remember whether or not that had actually landed yet. That makes things easier ;-)

seishun commented 6 years ago

@jasnell

After going through an evaluating the position @mafintosh is in on this, I'm forced to admit that I'm reconsidering my position on this for 10.0.0.

Modify core's runtime deprecation warnings such that the console output is not emitted by default for code running in node-modules.

Could you clarify how exactly hiding warnings for node_modules would help @mafintosh?

Trott commented 6 years ago

At the TSC meeting today, there was consensus (14 TSC attendees, no objections) that the runtime deprecation in 10.0.0 should only warn for code outside of node_modules to reduce the impact on end users for now at least, but to make it less likely that new code will use Buffer constructor.

@nodejs/tsc in case there are any objection, questions, comments, etc.

seishun commented 6 years ago

@Trott The full deprecation still hasn't landed, so perhaps this should remain open until then?