max-mapper / extract-zip

Zip extraction written in pure JavaScript. Extracts a zip into a directory.
BSD 2-Clause "Simplified" License
388 stars 126 forks source link

Buffer() is deprecated due to security and usability issues #99

Closed soryy708 closed 3 years ago

soryy708 commented 4 years ago

When I use this library like so:

function unzip(src, dest) {
    return extractZip(src, {dir: dest});
}

I get the following error:

(node:3912) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.

Please fix

papb commented 4 years ago

Which Node version are you using?

soryy708 commented 4 years ago

node --version: v12.16.2 (LTS) npm --version: 6.14.4

valkyrienyanko commented 4 years ago

I'm getting this as well

Node: v12.13.0 npm: 6.14.4

malept commented 4 years ago

It'd help if you run your script with node --throw-deprecation so it's more obvious where the Buffer deprecation warning is coming from - because there's no Buffer references in the extract-zip code base.

Jozzzef commented 3 years ago

extract-zip is dependent on yauzl

on line 3 of yauzl's index.js: var crc32 = require("buffer-crc32"); we see its dependent on buffer-crc32

on line 63 of buffer-crc32's index.js var buffer = new Buffer(length); and on line 81 return hasNewBufferAPI ? Buffer.from(input) : new Buffer(input); We see it calls on new instances of the buffer class

Node now has this feature deprecated, instead its recommended that one should use Buffer.from(<array>)

Its been 4 years since buffer-crc32 has been updated, so i understand why its still there. But I am unfamiliar with implementing cyclical redundancy checks (like i can even distinguish the importance of it being 32 bit), so this is where my contribution stops

malept commented 3 years ago

Since this is an issue in a transitive dependency, there's nothing that we can do in the code here to fix it. Please subscribe to https://github.com/thejoshwolfe/yauzl/issues/114 if you want to know the status.

hwlmatt commented 3 years ago

yauzl has had an open PR with a potential fix for this since Feb. Namely the fd-slicer2 package. (it does having breaking changes for node versions 4 and under I believe)

So using that, and as a temporary measure, in an extract-zip fork I'm aliasing a forked yauzl that is aliasing a fixed version (fd-slicer2) of the broken and no longer maintained fd-slicer package.

I post this here because this is something that extract-zip could do since it already requires node 10.17.0+, and could do it without any actual code changes to the forked yauzl (beyond aliasing fd-slicer). So if/when yauzl is able to resolve this, this change can be reverted rather easily.

It's more of a broken transitive dependency workaround than a real solution. I agree with @malept that the real solution is yauzl resolving 114. That said, it is quick to workaround:

in forked yauzl

and that's it.


additionally, if you wish to fork fd-slicer2 as well, in forked yauzl just change that alias: npm install fd-slicer@<GithubUser>/<GithubRepoOfForkedFd-Slicer2>.git

and as a last note, somewhere along the way i was told to peg an aliased fork to a commit. (Something about npm's cache and a git repo's HEAD only getting pulled when a dep is initially installed. honestly not sure if that's a relevant/current thing anymore)

To do that just append the alias with #<CommitHash> like @<GithubUser>/yauzl.git#71c42b5ea24170eeab7147f4b77fbc2c74

malept commented 3 years ago

It'd probably be easier to use yarn resolutions in this case. Something along the lines of (note: untested):

{
  // ...rest of package.json...
  "resolutions": {
    "extract-zip/yauzl/fd-slicer": "fd-slicer2"
  }
  // ...rest of package.json...
}

But of course, this requires you to use yarn instead of npm.

hwlmatt commented 3 years ago

Oh I wish I could get that work, but Yarn (classic, 1xx version) doesn't seem to support resolutions in this manner (aliasing within a resolution or resolving to a different package), only resolving specific versions.

I tried previously with no luck, but would gladly adopt this instead if I've misunderstood and someone can point me to the correct syntax. (the syntax you suggested doesn't seem to do it)

It IS a feature of Yarn2 (modern), however. The syntax there is closer to an alias:

  "resolutions": {
    "extract-zip/yauzl/fd-slicer@npm:fd-slicer2": "^1.1.0"
  },

But Yarn2 is a whole other thing, of course, and not available to me at the moment.

I simply don't know enough about it to speculate whether it's applicable to extract-zip solving this transient dep created issue.

But cheers and thanks regardless, @malept !

malept commented 3 years ago

Perhaps then, you could specify a git branch as a replacement version?

hwlmatt commented 3 years ago

Perhaps then, you could specify a git branch as a replacement version?

I'm not sure which way to read the question, so apologies if I missed it.

If you're asking if that is doable:

Yes, you could:.

"dependencies": {
...
"module": "user/repo#feature\/branch"
...
}

npm install <Module>@<GithubUser>/<GithubRepoOfForkedModule>.git#<branch>

If you're asking if I can refer one:

Due to the nature of the error, I think it would be preferable for extract-zip to fork the yauzl repo as described above and offer a minor version bump or branch. Not at all my call, obviously, but it would keep this within the extract-zip maintainers' control until yauzl resolves their issue.

That being said, here are my forks for both extract-zip and yauzl with those single line edits. (they won't be updated once the above issue is resolved by either as I will revert back, full FYI):

my temp fork of yauzl my temp fork of extract-zip

malept commented 3 years ago

I do know that you can't specify a git branch in a released npm module, it'd have to specify a real version (range). So specifying that for extract-zip itself is a non-starter.

For the record, I have no interest in maintaining yet another module (I'm stretched way too thin as it is) or changing to a forked version where I don't know who the maintainer is (basically, avoid an event-stream incident).