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

replace minipass pipeline with simple pipe #63

Closed
wants to merge 1 commit into from

Conversation

everett1992
Copy link

I can reliably cause tar integrity errors while downloading packages
from npm thru an http proxy that uses chunked encoding. The error seems
to be in make-fetch-happen, minipass-fetch or one of the minipass
libraries. I don't understand the root cause but I'm unable to reproduce
the issue after applying this patch.

My guess is that something in the stack expects res.body to be a
Minipass instance but not MinipassPipeline, or creating the pipeline
starts body flowing before all listeners are connected, missing some
chunks of data.

fixes npm/cli#3884

To reproduce

  1. Start the chunked-encoding proxy
  2. In a new terminal create a npm project
    mkdir chunked-encoding-error && cd chunked-encoding-error && echo '{}' > package.json
  3. configure npm to use the registry
    npm config --location project set registry http://localhost:4000
  4. remove npm cache.
  5. install any package from the registry (this should fail).
    • I recommend a package without dependencies, like lodash or typescript, debugging is easier.
    • The error isn't 100% consistent, you may want to run this step in
      a loop
      while rm ~/.npm/_cacache node_modules package-lock.json -rf && npm install --no-save lodash; do; done
      

I can reliably cause tar integrity errors while downloading packages
from npm thru an http proxy that uses chunked encoding. The error seems
to be in make-fetch-happen, minipass-fetch or one of the minipass
libraries. I don't understand the root cause but I'm unable to reproduce
the issue after applying this patch.

My guess is that something in the stack expects res.body to be a
Minipass instance but not MinipassPipeline, or creating the pipeline
starts body flowing before all listeners are connected, missing some
chunks of data.

fixes npm/cli#3884

To reproduce

1. Start the [chunked-encoding
   proxy](https://github.com/everett1992/make-fetch-happen/blob/chunked-encoding/proxy.js)
2. In a new terminal create a npm project `mkdir chunked-encoding-error
   && cd chunked-encoding-error && echo '{}' > package.json`
3. configure npm to use the registry `npm config --location project set registry
   http://localhost:4000
4. remove node_modules, and npm cache
5. install any package from the registry (this should fail).
   - I recommend a package without dependencies, like lodash or typescript, debugging is easier.
   - The error isn't 100% consistent, you may want to run this step in
     a loop
     ```
     while rm ~/.npm/_cacache node_modules package-lock.json -rf && npm install --no-save lodash; do; done
     ```

[chunked-encoding proxy]: https://github.com/everett1992/make-fetch-happen/blob/chunked-encoding/proxy.js
everett1992 pushed a commit to everett1992/make-fetch-happen-tar-extract-error that referenced this pull request Nov 30, 2021
Related to npm/cli#3884, npm/make-fetch-happen#63

npm v7+ throws integrity and zlib corrupted errors when installing from
a registry that uses chunked encoding.

This package reproduces that error using make-fetch-happen and tar. The
error only occurs when fetch is passed integrity and the server uses
chunked encoding. Only the `extract with integirty` test fails.
@everett1992
Copy link
Author

I started carving chunks out of npm to create a smaller reproduction and I've reached make-fetch-happen and tar. Tomorrow I'll keep carving to try to eliminate tar.

https://github.com/everett1992/make-fetch-happen-tar-extract-error/blob/main/error.test.js

This test shows a lot of weird behavior.

  • why is it only reproduced with chunked-encoding?
  • why is it only reproduced when integrity is passed to fetch?
  • why is size correct but integrity is wrong?
  • why does piping ssri to tar.x yield incorrect integrity? How does the destination affect the source?

Switching to my branch of make-fetch-happens fixes the bug and all tests pass.

error.test.js
  res.body data events updating hash
    ✓ yields the right digest

  res.body pipe to createHash
    ✓ yields the right digest

  extract with integrity
    1) zlib: incorrect data check

  extract affects source stream?
    2) source hash is expected
    ✓ source size is expected

  extract with integrity and content-length
    ✓ succeeds

  extract without integrity
    ✓ succeeds


  5 passing (6s)
  2 failing

  1) error.test.js extract with integrity zlib: incorrect data check:
     Error: zlib: incorrect data check


  2) error.test.js extract affects source stream? source hash is expected:

      Error: source hash is expected
      + expected - actual

      -24dbddf17111f46417d2fdaa260b1a37f9b3142340e4145efe3f0937d77eb56c862d2a1d2901ca16271dc0d6335b0237c2346768a3ec1a3d579018f1fc5f7a0d
      +0fafc16d5a24c317008d3d76ac460b09fa8073918af84a979769716cb4560855f36627a02654845115bf931f99e5b883f65a6429b9a4b0367b64ee0ae16311f7

@nlf
Copy link
Contributor

nlf commented Dec 2, 2021

if i'm understanding correctly, your pull request to minipass linked above fixes this issue, is that right?

@everett1992
Copy link
Author

Yes, isaacs/minipass#28 closes this and npm/cli#3884

@everett1992 everett1992 closed this Dec 2, 2021
@nlf
Copy link
Contributor

nlf commented Dec 2, 2021

Yes, isaacs/minipass#28 closes this and npm/cli#3884

you're awesome, thanks for taking the time to chase this one down all the way to the end. i know what these bug hunts are like so i really appreciate it!

@everett1992
Copy link
Author

Thanks, I'm glad I followed it to the root cause instead of trying to merge this hot fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] tarball data currupted when installing from 'chunked encoding' server.
2 participants