pzavolinsky / ts-unused-exports

ts-unused-exports finds unused exported symbols in your Typescript project
MIT License
738 stars 46 forks source link

Suspicious dependency on the "4" package in ts-unused-exports 8.0.1 as published to NPM #238

Closed jstasiak closed 1 year ago

jstasiak commented 1 year ago

Hey!

I noticed that when we tried to upgrade ts-unused-exports from 8.0.0 to 8.0.1 a new dependency has been introduced there. The name of the dependency is 4 and it's visible here: https://www.npmjs.com/package/ts-unused-exports/v/8.0.1?activeTab=dependencies. It wasn't there in 8.0.0: https://www.npmjs.com/package/ts-unused-exports/v/8.0.0?activeTab=dependencies

What got me worried is there's no reference to that dependency in package.json on GH (https://github.com/pzavolinsky/ts-unused-exports/blob/bad085c96317d30463433db1af2b55636f95fcb5/package.json) and the 4 package (https://www.npmjs.com/package/4) just seems strange – very old, no readme, the version is 0.0.0 etc.

Should the dependency on 4 actually be there or is this an error somewhere?

pzavolinsky commented 1 year ago

Thanks for the report, this is not intentional, I'm unpublishing the version, pending some review.

pzavolinsky commented 1 year ago

NPM is not allowing me to unpublish that version so I manually published 8.0.2, no extraneous dependency.

We've been experimenting with CD via GH Actions and (without more info) I need to assume something weird is going on there. I'm disabling CD from the time being, until we can run a proper investigation.

Thanks again @jstasiak for the report, this was super useful and timely.

cc: @mrseanryan

pzavolinsky commented 1 year ago

btw, version 8.0.1 has been unpublished (npm just takes a while to update on their side)

pzavolinsky commented 1 year ago

Still unclear where did this came from, but this is what I can see so far (as to assess impact):

$ docker run --rm -ti node:16.13.2-slim sh
# bash
root@9adfdad8e109:/# mkdir m4
root@9adfdad8e109:/# cd m4
root@9adfdad8e109:/m4# echo '{}' > package.json
root@9adfdad8e109:/m4# npm i 4

added 1 package, and audited 2 packages in 3s

found 0 vulnerabilities
npm notice 
npm notice New major version of npm available! 8.1.2 -> 9.1.3
npm notice Changelog: https://github.com/npm/cli/releases/tag/v9.1.3
npm notice Run npm install -g npm@9.1.3 to update!
npm notice 
root@9adfdad8e109:/m4# ls
node_modules  package-lock.json  package.json
root@9adfdad8e109:/m4# cat package-lock.json 
{
  "name": "m4",
  "lockfileVersion": 2,
  "requires": true,
  "packages": {
    "": {
      "dependencies": {
        "4": "^0.0.0"
      }
    },
    "node_modules/4": {
      "version": "0.0.0",
      "resolved": "https://registry.npmjs.org/4/-/4-0.0.0.tgz",
      "integrity": "sha512-JWaJnETbU+8EfG4hPw1PFwEHsd7Fjlp0oNRYUYynn8t54P0459HAUiu1jgOnvgOYMZH9YnhfT+bgqcazmbMF5Q=="
    }
  },
  "dependencies": {
    "4": {
      "version": "0.0.0",
      "resolved": "https://registry.npmjs.org/4/-/4-0.0.0.tgz",
      "integrity": "sha512-JWaJnETbU+8EfG4hPw1PFwEHsd7Fjlp0oNRYUYynn8t54P0459HAUiu1jgOnvgOYMZH9YnhfT+bgqcazmbMF5Q=="
    }
  }
}
root@9adfdad8e109:/m4# cd node_modules/4/
root@9adfdad8e109:/m4/node_modules/4# ls
index.js  package.json
root@9adfdad8e109:/m4/node_modules/4# cat index.js 
root@9adfdad8e109:/m4/node_modules/4# cat package.json 
{
  "name": "4",
  "version": "0.0.0",
  "description": "WOOHOO! I GOT 4!",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC"
}
root@9adfdad8e109:/m4/node_modules/4# 
jstasiak commented 1 year ago

Nice, thank you for reacting to this so quickly and for all the work on this library!

As to how this could've happened: I suspect a line like this was executed somewhere in the publishing pipeline at some point, just without --development (?):

      - run: npm install --development ${{ matrix.typescript-version }}

https://github.com/pzavolinsky/ts-unused-exports/blob/bad085c96317d30463433db1af2b55636f95fcb5/.github/workflows/node.js.deploy.yml#L28

where typescript-version is 3 or 4

jstasiak commented 1 year ago

I'm happy with the status quo so I'll close the issue. Once again thank you for resolving this

mrseanryan commented 1 year ago

update:

1 - there was a bug in the workflows, where typescript@ was missing - so it was trying to install bogus package like 3 or 4 (!) 2 - the deploy workflow was using Node 16 which has a newer version of npm, that involves rewriting package-lock.json to version 2

fixed: 1 - typescript package is mentioned 2 - deploy uses Node 14 (at least until we agree to upgrade to package-lock.json, which may break for older projects... ?)