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

Tracking Issue for Runtime Deprecation of Buffer constructor #19079

Closed
Trott opened this issue Mar 1, 2018 · 87 comments
Closed

Tracking Issue for Runtime Deprecation of Buffer constructor #19079

Trott opened this issue Mar 1, 2018 · 87 comments
Labels

Comments

@Trott
Copy link
Member

@Trott Trott commented Mar 1, 2018

  • Version: 10.0.0
  • Platform: all
  • Subsystem: buffer

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.

  • Draft concise statement as to why we're doing this instead of leaving things where they are (with new Buffer() now zero-filling and a documentation deprecation for it). We're likely to see a lot of comments like https://twitter.com/shelleypowers/status/969568430105006080.
  • Land runtime deprecation on master (@seishun has a PR open: #15346)
  • Write material for upgrade guide (I believe @ChALkeR and @addaleax have expressed willingness to do parts of this, and @targos proposed upgrade guides in the first place so maybe him as well) nodejs/nodejs.org#1666
  • Identify important public modules that need patches and submit those patches (@ChALkeR has done a lot on this already)
  • Get the word out! Twitter, Node.js Collection post, whatever!

What have I missed?

@nodejs/community-committee @nodejs/buffer

FWIW, here's how the TSC voted:

@Trott
Copy link
Member Author

@Trott Trott commented Mar 2, 2018

One thing we'll need is a concise statement as to why we're doing this instead of leaving things where they are (with new Buffer() now zero-filling and a documentation deprecation for it). I have a feeling we'll see a lot of comments like this: https://twitter.com/shelleypowers/status/969568430105006080. If we can't answer them quickly and concisely, it could be bad. I'll add an item to the checklist for this.

@bnb
Copy link
Member

@bnb bnb commented Mar 2, 2018

@Trott would you like CommComm's help in crafting the statement? I think we'd be generally happy to help on messaging around this / reviewing a blog post if y'all want to write one.

@shelleyp
Copy link

@shelleyp shelleyp commented Mar 2, 2018

I think providing a good, concise statement is important because you're basically going back on an implicit promise outlined in https://medium.com/@jasnell/node-js-buffer-api-changes-3c21f1048f97, as well as the API docs for the last two years.

@Trott
Copy link
Member Author

@Trott Trott commented Mar 2, 2018

@Trott would you like CommComm's help in crafting the statement? I think we'd be generally happy to help on messaging around this / reviewing a blog post if y'all want to write one.

@bnb More than that, I'd like someone to lay out the technical reasons for doing this. I'd kind of like to collect bullet point reasons for doing this from @ChALkeR and @seishun on the one hand, and maybe from some of the used-to-be-no-never-ever-do-this-but-have-moved-a-bit-in-the-other-direction people (@addaleax and @evanlucas would be my choices there), and kind of try to come up with a statement from all of that. Then I can run it by @nodejs/community-committee and @nodejs/tsc.

@Trott
Copy link
Member Author

@Trott Trott commented Mar 2, 2018

you're basically going back on an implicit promise outlined in https://medium.com/@jasnell/node-js-buffer-api-changes-3c21f1048f97

@shelleyp Part of me wants to point out that that's not (as far as I know) an official communication from Node.js but the personal blog of Some Person Who Happens To Be On The Project Technical Steering Committee. You're not wrong to treat the statements in there with weight. At the same time, it's hard for the project to be bound by statements from its individuals. The members of the TSC disagree on things all the time, as do people in the larger project.

Again, though, that issue of communication is the project's problem to solve. You're not wrong.
(Maybe we can ask people to use a disclaimer similar to the ones you see that indicate someone's opinion is their own and not their employer. Seems like something that would be hard to enforce and probably ineffective to me, but I don't know what else to suggest.)

Martii added a commit to Martii/OpenUserJS.org that referenced this issue Mar 2, 2018
Applies to nodejs/node#19079 and initially referenced in da49fff
Martii added a commit to OpenUserJS/OpenUserJS.org that referenced this issue Mar 2, 2018
Applies to nodejs/node#19079 and initially referenced in da49fff

Auto-merge
@shelleyp
Copy link

@shelleyp shelleyp commented Mar 2, 2018

It's less that James Snell was a person on the TSC, but was seen as a pretty definitive authority on Node at the time. And he spoke with a great deal of finality about the future of Buffer constructor.

But regardless, Buffer constructor was such a fundamental element of earlier Node releases. Technology support groups typically hesitate or do anything they can to avoid 'breaking' backward compatibility.

Having said that, security is one impetus for breaking backward compatibility.

Still, I can see some poor soul somewhere having created a large complex Node application, who played by the rules and didn't use Buffer constructor, and all of a sudden she's hit with this warning. And she'll look at the app's module dependencies and they're fine, but it's some slub of a module buried layers deep that is perfectly safe...but uses Buffer constructor.

All of a sudden, Node seems far less stable than it once was. Especially when she starts tweeting about this blankity-blank warning that popped up that she can't do anything about.

I'm just trying to give you an outsider perspective, having been through these types of events with other technologies.

So if you all decide to do this, OK. But warn folks ahead of time, strongly highlight this change for 10.0 for those migrating, and provide an explanation, and even provide steps a person can take--either to remove the warning or to reduce the dependency on the module that generated the warning.

Node has had some issues in the last few years (more organizational than technical). It needs to present rock solid stability to the world.

My 2 cents worth

@jasnell
Copy link
Member

@jasnell jasnell commented Mar 2, 2018

It's less that James Snell was a person on the TSC, but was seen as a pretty definitive authority on Node at the time. And he spoke with a great deal of finality about the future of Buffer constructor.

Not so much "was". Still here.

So if you all decide to do this, OK. But warn folks ahead of time,

We have been actively doing so.

...strongly highlight this change for 10.0 for those migrating, and provide an explanation, and even provide steps a person can take--either to remove the warning or to reduce the dependency on the module that generated the warning.

As the person who is handling the 10.0.0 release, and the person who will be writing up the notable changes for that release, I intend full well to clearly highlight the new runtime deprecation for new Buffer(), and communication via other channels such as Twitter have already been used to communicate the change.

Other members of the TSC have been actively opening pull requests on ecosystem modules to update code.

@bnb
Copy link
Member

@bnb bnb commented Mar 2, 2018

@shelleyp I can't commit to anything for the TSC, but I have little doubt that the project will do everything in its power to communicate this as effectively as possible and push for stability of the ecosystem.

Judging by what's happened with prior releases, I imagine that the project will publish a release blog post for v10 and that this specific change will be front and center.

As a member of the Node.js Collection Initiative, I am happy to make myself available as a resource to the TSC help create any/all communication about this change - and any other major changes - in v10 and future releases. If you specifically have any concerns on things that need to be addressed before the release, sharing those would be extremely helpful for us so we can communicate as effectively as possible, addressing the needs of the community.

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Mar 2, 2018

@shelleyp You are describing an example of an affected person in details — I understand your concern, but to truly determine the best path forward, we need to understand how many people on average will be negatively affected by this (and how) and how many people are expected to be affected if we don't make this change (and how).

The sad thing is that the documentation-only deprecation doesn't help. The number of packages using the old unsafe API only increases over time (yes, we actually have done measurements), and overall impact on the ecosystem caused by modules using the old API also increases over time (yes, we have dome measurements of that too, in downloads/month).

We are trying hard to track down the old Buffer API usage in popular packages to minimize the negative effect that you are speaking about. I have a list of those sorted by downloads/month, a significant number from the top list are already fixed or have have PRs opened, others are still on the roadmap.

A relatively large number of those turned out to be actual or potential security problems (varying from API footguns to leaking secrets to remote unauthorized attacker in a highly popular package), and some turned out to be undefined behaviour problems, like moxystudio/node-cross-spawn#94.

If we don't deprecate — the problem would only intensify over time, given the current trend, and someday real people are going to actually loose their sensitive data.

This was referenced Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet