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

[feat] yarn audit #5808

Closed
rally25rs opened this issue May 11, 2018 · 89 comments
Closed

[feat] yarn audit #5808

rally25rs opened this issue May 11, 2018 · 89 comments

Comments

@rally25rs
Copy link
Contributor

Do you want to request a feature or report a bug?

feature

What is the current behavior?

npm added audit to warn about packages with known security issues. There was some conversation about this previously and one of the core npm folks said the API was likely to be open/public to pull this info. Therefore, yarn should be able to add this feature.

What is the expected behavior?

  • Add a yarn audit command that mimics npm audit
  • Add warnings when adding/installing packages with known issues.

Please mention your node.js, yarn and operating system version.

This would be a minor version bump, so likely target yarn v1.7.0 or v1.8.0 depending on timing.
This is probably too important to wait for v2.0.

@batjko
Copy link

batjko commented May 13, 2018

One thing that yarn could do, is improve on the concept. npm audit's output and configurability is somewhat basic right now.

Perhaps yarn could add some interactivity, allowing the user to automatically update packages, as long as the suggested version is just a semver patch update.

Also definitely useful would be different types of output (html, json, csv, or similar console outputs to test or lint runners etc.)

I'm sure there are more ways yarn can add a bunch of value here.

@chapati23
Copy link

Would love yarn to add this. We're currently using snyk.io for vulnerability management but I've never been 100% happy with it as it always felt like something that should be more integrated.

Was super excited when I heard that npm added the audit command. But also super disappointed when I realized that it only works with a corresponding package-lock.json, which would basically mean for us to either drop yarn completely and go back to npm. Or use both tools, which already sounds like a recipe for horrible merge-conflicts and syncing-hell between the two lock-files.

@Hypnosphi
Copy link

Hypnosphi commented May 16, 2018

horrible merge-conflicts

Right now both yarn and npm are able to autoresolve the lockfile conflicts

syncing-hell

Yeah, that's the real problem

@shinriyo
Copy link

yarn is upward compatibility to npm.
I hope yarn should support audit option officially!

@Hypnosphi
Copy link

Unfortunately, any new command addition is a breaking change for yarn, because of yarn scriptName shortcut

@batjko
Copy link

batjko commented May 17, 2018

Unfortunately, any new command addition is a breaking change for yarn, because of yarn scriptName shortcut

I don't see how.
npm start, for example, has a default behavior which kicks in if no explicit script for it is defined in package.json.
So if someone has yarn audit already defined, it will simply override the default which would be npm's audit. E.g.:

"scripts": {
  "start": "echo 'this will override the default behavior of npm start'",
  "audit": "echo 'my custom audit script will now be used'"
}

@Hypnosphi
Copy link

Ok I see

@rally25rs
Copy link
Contributor Author

rally25rs commented May 20, 2018

I haven't had time to actually implement anything here yet, so if anyone has free time, help is appreciated. Here are some notes from capturing the network traffic from npm:


After all manifests metadata is loaded and before any .tgz packages are downloaded, npm makes a new request:

POST https://registry.npmjs.org/-/npm/v1/security/audits/quick

headers:

connection: keep-alive
user-agent: npm/6.0.1 node/v8.11.2 win32 x64
npm-in-ci: false
npm-scope: 
npm-session: 4e017f155c654f93
referer: undefined
content-type: application/json
content-type: application/octet-stream
authorization: Bearer {removed}
accept: */*
content-length: 654
accept-encoding: gzip,deflate
Host: registry.npmjs.org

request body:

{
  "name": "demo-npm",
  "version": "1.0.0",
  "requires": {
    "moment": "^2.10.5",
    "minimatch": "^1.0.0"
  },
  "dependencies": {
    "lru-cache": {
      "version": "2.7.3",
      "integrity": "sha1-bUUk6LlV+V1PW1iFHOId1y+06VI="
    },
    "minimatch": {
      "version": "1.0.0",
      "integrity": "sha1-4N0hILSeG3JM6NcUxSCCKpQ4V20=",
      "requires": {
        "lru-cache": "2",
        "sigmund": "~1.0.0"
      }
    },
    "moment": {
      "version": "2.22.1",
      "integrity": "sha512-shJkRTSebXvsVqk56I+lkb2latjBs8I+pc2TzWc545y2iFnSjm7Wg0QMh+ZWcdSLQyGEau5jI8ocnmkyTgr9YQ=="
    },
    "sigmund": {
      "version": "1.0.1",
      "integrity": "sha1-P\/IfGYytIXX587eBhT\/ZTQ0ZtZA="
    }
  },
  "install": [
    "minimatch@1.0.0"
  ],
  "remove": [
    
  ],
  "metadata": {
    "npm_version": "6.0.1",
    "node_version": "v8.11.2",
    "platform": "win32"
  }
}

response headers:

content-type: application/json; charset=utf-8
access-control-allow-origin: *
access-control-allow-credentials: true
Cache-Control: max-age=300
Accept-Ranges: bytes
Date: Sun, 20 May 2018 21:25:49 GMT
Via: 1.1 varnish
Connection: keep-alive
X-Served-By: cache-mdw17321-MDW
X-Cache: MISS
X-Cache-Hits: 0
X-Timer: S1526851550.558067,VS0,VE246
Vary: Accept-Encoding, Accept
Content-Length: 1605

response body:

{
  "actions": [
    
  ],
  "advisories": {
    "118": {
      "findings": [
        {
          "version": "1.0.0",
          "paths": [
            "minimatch"
          ],
          "dev": false,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 118,
      "created": "2016-05-25T16:37:20.000Z",
      "updated": "2018-03-01T21:58:01.072Z",
      "deleted": null,
      "title": "Regular Expression Denial of Service",
      "found_by": {
        "name": "Nick Starke"
      },
      "reported_by": {
        "name": "Nick Starke"
      },
      "module_name": "minimatch",
      "cves": [
        "CVE-2016-10540"
      ],
      "vulnerable_versions": "<=3.0.1",
      "patched_versions": ">=3.0.2",
      "overview": "Affected versions of `minimatch` are vulnerable to regular expression denial of service attacks when user input is passed into the `pattern` argument of `minimatch(path, pattern)`.\n\n\n## Proof of Concept\n```\nvar minimatch = require(\u201cminimatch\u201d);\n\n\/\/ utility function for generating long strings\nvar genstr = function (len, chr) {\n  var result = \u201c\u201d;\n  for (i=0; i<=len; i++) {\n    result = result + chr;\n  }\n  return result;\n}\n\nvar exploit = \u201c[!\u201d + genstr(1000000, \u201c\\\\\u201d) + \u201cA\u201d;\n\n\/\/ minimatch exploit.\nconsole.log(\u201cstarting minimatch\u201d);\nminimatch(\u201cfoo\u201d, exploit);\nconsole.log(\u201cfinishing minimatch\u201d);\n```",
      "recommendation": "Update to version 3.0.2 or later.",
      "references": "",
      "access": "public",
      "severity": "high",
      "cwe": "CWE-400",
      "metadata": {
        "module_type": "Multi.Library",
        "exploitability": 4,
        "affected_components": "Internal::Code::Function::minimatch({type:'args', key:0, vector:{type:'string'}})"
      }
    }
  },
  "muted": [
    
  ],
  "metadata": {
    "vulnerabilities": {
      "info": 0,
      "low": 0,
      "moderate": 0,
      "high": 1,
      "critical": 0
    },
    "dependencies": 4,
    "devDependencies": 0,
    "optionalDependencies": 0,
    "totalDependencies": 4
  }
}

The request includes the integrity for each package, but I'm not sure how that is generated or if yarn already figures that out somewhere.


In addition to that, manually running the yarn audit command does:

request headers:

POST https://registry.npmjs.org/-/npm/v1/security/audits

connection: keep-alive
user-agent: npm/6.0.1 node/v8.11.2 win32 x64
npm-in-ci: false
npm-scope: 
npm-session: 686c3e1b85b7d5d2
referer: undefined
content-encoding: gzip
content-type: application/json
content-type: application/octet-stream
authorization: Bearer {removed}
accept: */*
content-length: 401
accept-encoding: gzip,deflate
Host: registry.npmjs.org

request body:

{
  "name": "demo-npm",
  "version": "1.0.0",
  "requires": {
    "minimatch": "^1.0.0",
    "moment": "^2.10.5"
  },
  "dependencies": {
    "lru-cache": {
      "version": "2.7.3",
      "integrity": "sha1-bUUk6LlV+V1PW1iFHOId1y+06VI="
    },
    "minimatch": {
      "version": "1.0.0",
      "integrity": "sha1-4N0hILSeG3JM6NcUxSCCKpQ4V20=",
      "requires": {
        "lru-cache": "2",
        "sigmund": "~1.0.0"
      }
    },
    "moment": {
      "version": "2.22.1",
      "integrity": "sha512-shJkRTSebXvsVqk56I+lkb2latjBs8I+pc2TzWc545y2iFnSjm7Wg0QMh+ZWcdSLQyGEau5jI8ocnmkyTgr9YQ=="
    },
    "sigmund": {
      "version": "1.0.1",
      "integrity": "sha1-P\/IfGYytIXX587eBhT\/ZTQ0ZtZA="
    }
  },
  "install": [
    
  ],
  "remove": [
    
  ],
  "metadata": {
    "npm_version": "6.0.1",
    "node_version": "v8.11.2",
    "platform": "win32"
  }
}

response headers:

content-type: application/json; charset=utf-8
access-control-allow-origin: *
access-control-allow-credentials: true
Cache-Control: max-age=300
Accept-Ranges: bytes
Date: Sun, 20 May 2018 21:26:06 GMT
Via: 1.1 varnish
Connection: keep-alive
X-Served-By: cache-mdw17381-MDW
X-Cache: MISS
X-Cache-Hits: 0
X-Timer: S1526851566.039865,VS0,VE325
Vary: Accept-Encoding, Accept
Content-Length: 1766

response body:

{
  "actions": [
    {
      "action": "install",
      "module": "minimatch",
      "target": "3.0.4",
      "isMajor": true,
      "resolves": [
        {
          "id": 118,
          "path": "minimatch",
          "dev": false,
          "optional": false,
          "bundled": false
        }
      ]
    }
  ],
  "advisories": {
    "118": {
      "findings": [
        {
          "version": "1.0.0",
          "paths": [
            "minimatch"
          ],
          "dev": false,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 118,
      "created": "2016-05-25T16:37:20.000Z",
      "updated": "2018-03-01T21:58:01.072Z",
      "deleted": null,
      "title": "Regular Expression Denial of Service",
      "found_by": {
        "name": "Nick Starke"
      },
      "reported_by": {
        "name": "Nick Starke"
      },
      "module_name": "minimatch",
      "cves": [
        "CVE-2016-10540"
      ],
      "vulnerable_versions": "<=3.0.1",
      "patched_versions": ">=3.0.2",
      "overview": "Affected versions of `minimatch` are vulnerable to regular expression denial of service attacks when user input is passed into the `pattern` argument of `minimatch(path, pattern)`.\n\n\n## Proof of Concept\n```\nvar minimatch = require(\u201cminimatch\u201d);\n\n\/\/ utility function for generating long strings\nvar genstr = function (len, chr) {\n  var result = \u201c\u201d;\n  for (i=0; i<=len; i++) {\n    result = result + chr;\n  }\n  return result;\n}\n\nvar exploit = \u201c[!\u201d + genstr(1000000, \u201c\\\\\u201d) + \u201cA\u201d;\n\n\/\/ minimatch exploit.\nconsole.log(\u201cstarting minimatch\u201d);\nminimatch(\u201cfoo\u201d, exploit);\nconsole.log(\u201cfinishing minimatch\u201d);\n```",
      "recommendation": "Update to version 3.0.2 or later.",
      "references": "",
      "access": "public",
      "severity": "high",
      "cwe": "CWE-400",
      "metadata": {
        "module_type": "Multi.Library",
        "exploitability": 4,
        "affected_components": "Internal::Code::Function::minimatch({type:'args', key:0, vector:{type:'string'}})"
      }
    }
  },
  "muted": [
    
  ],
  "metadata": {
    "vulnerabilities": {
      "info": 0,
      "low": 0,
      "moderate": 0,
      "high": 1,
      "critical": 0
    },
    "dependencies": 4,
    "devDependencies": 0,
    "optionalDependencies": 0,
    "totalDependencies": 4
  }
}

@rally25rs
Copy link
Contributor Author

Discovered that the integrity may be returned by the registry for newer packages in the metadata under versions[version_number].dist.integrity

When that does not exist, the npm lockfile docs says "the sha1 in shasum" but I haven't quite figured out how that is generated.

https://github.com/zkat/pacote/blob/ccc6e9094c2e872f09cc12ae966a0cbc1a570eed/lib/finalize-manifest.js#L100 leads me to believe that npm generates the missing integrity from the tarball, but watching network traffic, it has not yet downloaded the tarball 🤔

@zkat @imsnif can you provide any details as to how this works? (just asking is probably a lot easier than me hunting through the code 😄I appreciate any help!)

@imsnif
Copy link
Member

imsnif commented May 21, 2018

The manifest json (eg. https://registry.npmjs.org/left-pad) should also have a shasum entry under dist - versions[version_number].dist.shasum
Was that the question? I don't have full context here, but can read back if I misunderstood. :)

@rally25rs
Copy link
Contributor Author

@imsnif sorry for the lack of clarity on my part :) If you look at the registry response for minimatch v1.0.0, you get:

            "dist": {
                "shasum": "e0dd2120b49e1b724ce8d714c520822a9438576d",
                "tarball": "https://registry.npmjs.org/minimatch/-/minimatch-1.0.0.tgz"
            },

and the audit request sends:

    "minimatch": {
      "version": "1.0.0",
      "integrity": "sha1-4N0hILSeG3JM6NcUxSCCKpQ4V20=",

so I'm wondering how it takes e0dd2120b49e1b724ce8d714c520822a9438576d and turns it into sha1-4N0hILSeG3JM6NcUxSCCKpQ4V20= (or is there something else that it s generated from?)

@imsnif
Copy link
Member

imsnif commented May 21, 2018

Ah, I think you're looking for this: https://github.com/zkat/ssri/blob/latest/index.js#L163-L173
Specifically (in regards to minimatch):

Buffer.from('e0dd2120b49e1b724ce8d714c520822a9438576d', 'hex').toString('base64')
// 4N0hILSeG3JM6NcUxSCCKpQ4V20=

@imsnif
Copy link
Member

imsnif commented May 21, 2018

Also - reading back now - if we need to talk to an npm api and send it the integrity of packages in the form of an sri, I'd very much recommend waiting for this to be merged: #5042
In short, it adds an integrity field to yarn.lock, handled by the same ssri module created and used by npm. I think that will solve this part?

@rally25rs
Copy link
Contributor Author

Oh cool, yes that's probably something we want. Thanks for pointing it out 👍

@zkat
Copy link
Contributor

zkat commented May 21, 2018

https://github.com/zkat/pacote/blob/latest/lib/fetchers/registry/manifest.js#L129

This is where we fill this in for shasum-only packages

@zkat
Copy link
Contributor

zkat commented May 21, 2018

And if a registry doesn't give either a shasum or an integrity field, then we calculate it off the tarball (ditto for other types that don't have this data, like remote tarball deps)

@batjko
Copy link

batjko commented May 22, 2018

But also super disappointed when I realized that it only works with a corresponding package-lock.json, which would basically mean for us to either drop yarn completely and go back to npm.

I do think that was quite intentional on the npm team's part.

@zkat
Copy link
Contributor

zkat commented May 22, 2018

@batjko Yeah uh, yarn.lock isn't our format. We'll eventually have support for importing yarn.lock, and that should enable us to then do this, but we don't have that functionality right now. :) Were someone to provide a PR that grants npm the ability to parse yarn.lock and build a tree out of it (much like #5745 does, but on our end), that would speed up that part of the process a lot. It's already on our roadmap, but it's not exactly the highest-priority thing there.

An interesting note is that we require a package-lock.json right now because it was the fastest way to implement this feature effectively. It was a bit more complex and involved to calculate a full tree first, and then post it. It's coming eventually, though, and that would allow projects using yarn.lock even before we're able to parse the file, so long as they had an existing node_modules we could read concrete trees from.

But tbh, in the end, you want this in Yarn itself. It's not gonna be as nice to be switching back and forth to npm just for the one feature and it won't have as good integration. npm@next can already auto-fix vulnerabilities, to boot, and that's not something we'd do for Yarn projects at all (because lockfile).

@zkat
Copy link
Contributor

zkat commented May 22, 2018

(and it should tell you something that I'm showing up in this issue tracker and helping out, if you're concerned about Competition Issues)

@rally25rs
Copy link
Contributor Author

Just a quick update. I actually found some time to get started on this feature.

~/Projects/yarn-test 🐒   yarn audit
yarn audit v1.8.0-0
2 vulnerabilities found - Packages audited: 5
Severity: 1 Low | 1 High
✨  Done in 0.70s.

There is still a ton of work to do for the more in-depth reporting, but it prints a summary now at least. That's all, just wanted to let people know that this is at least in progress.

@rodrigooler
Copy link

@rally25rs excellent progress, I can not wait to test this new feature.

@openjck
Copy link

openjck commented Jun 13, 2018

@zkat To be clear, are you saying that even if Yarn adds an audit command, it would not add an audit fix command? The latter is super useful. I can't speak for anyone but me, but I'd switch back to NPM for improved security if Yarn doesn't plan to add an audit fix command.

@DanielRuf
Copy link
Contributor

You might want to try Snyk.

@jonas-pietzsch
Copy link

@DanielRuf Sorry, I didn't express myself well. I was referring to Yarn 1.12.3 being available in the Docker image of Node 8.14.0 – off topic ;-)
I know that NSP was bought and just today I saw some red pipelines due to the final shutdown.
But I could imagine that people maybe want to maintain an exception/ignore file for vulnerabilities that yarn audit discovers because of several reasons.

@timja
Copy link

timja commented Dec 11, 2018

@jverhoelen #6669

@fedorareis
Copy link

@alejandroiglesias you can use the --audit flag on both yarn add and yarn install to run an audit with those commands. I personally think it is better for those to run by default as it makes it easier to know there is a vulnerability without needing to remember to either add the flag or run yarn audit hopefully the default will change in time.

@arcanis
Copy link
Member

arcanis commented Jan 11, 2019

You can make it a default by adding the following into your yarnrc:

--install.audit true
--add.audit true

@alejandroiglesias
Copy link

@arcanis the .yarnrc in my home directory says that I shouldn't edit that file directly? Is there where I should add the flags you suggested?

@arcanis
Copy link
Member

arcanis commented Jan 18, 2019

Because the yarnrc files and the lockfiles share the same format, all yarn-generated configuration file have this header, but it's perfectly safe to edit the rc files (and even the lockfile, if you know what you're doing). It'll be fixed in the next major.

@alejandroiglesias
Copy link

@arcanis thanks!

@alejandroiglesias
Copy link

@arcanis one more question: is that alternatively achievable by using yarn config? I never used that command, but was wondering about that being easier than locating the .yarnrc file in my Meteor Node env (+ re-adding the entries after each Meteor upgrade since it resets the env and I even have to re-install Yarn).

@fedorareis
Copy link

@alejandroiglesias it is achievable with yarn config that's how I did it. The commands would be

yarn config set add.audit true
yarn config set install.audit true

for add and install respectively

@KinTsang
Copy link

@arcanis I added both CLI arguments in my project's yarnrc file:

--install.audit true
--add.audit true

It now runs yarn audit automatically during yarn install but I am having trouble getting yarn audit to run for yarn add.

My yarn version is 1.13.0

@Berkmann18
Copy link

Am I right in saying that there's no plan to add the fix option for yarn audit?

@mesqueeb
Copy link

Is there any chance we're able to use yarn audit fix just like we would with npm audit fix? Or is this not supported and not planned to support with yarn?

@shopglobal

This comment has been minimized.

@shopglobal

This comment has been minimized.

@MatrixFrog
Copy link

@shopglobal the latest stable version is v1.13.0 -- 1.3.2 is rather old at this point

@DanielRuf
Copy link
Contributor

#5808 (comment)

@FDiskas
Copy link

FDiskas commented Jan 6, 2020

Current solution for fixiing audit results is like so:

  1. delete node_modules directory.
  2. delete yarn.lock file
  3. use npm i
  4. run npm audit fix --force
  5. delete node_modules directory
  6. run yarn
  7. run again yarn audit to see results.
  8. delete package-lock.json file

why it works? Yarn respects package-lock.json file when installing packages

@DanielRuf
Copy link
Contributor

why it works? Yarn respects package-lock.json file when installing packages

Not anymore afaik. Also this is not really a solution. I think you have a different issue.

@fengerzh
Copy link

fengerzh commented Jan 6, 2020

Current solution for fixiing audit results is like so:

  1. delete node_modules directory.
  2. delete yarn.lock file
  3. use npm i
  4. run npm audit fix --force
  5. delete node_modules directory
  6. run yarn
  7. run again yarn audit to see results.
  8. delete package-lock.json file

why it works? Yarn respects package-lock.json file when installing packages

That's really a smart and fast and convinient solution.

@arcanis
Copy link
Member

arcanis commented Jan 7, 2020

why it works? Yarn respects package-lock.json file when installing packages

Not anymore afaik.

That's never been the case - yarn.lock predates package-lock.json so we've never had to do this (even the old shrinkwrap was never used afaik).

My guess is that by removing the lockfile OP simply forced Yarn to upgrade all their packages, hereby "fixing" the versions. You could do the same thing without calling npm and have the same result.

@greybax
Copy link

greybax commented May 14, 2020

the idea is using resolutions: {} into package.json works! I've wrote post how it workd in my case:
How to fix security vulnerabilities in NPM/Yarn dependencies

@DanielRuf
Copy link
Contributor

Or try snyk or use dependabot which can help here too (if you use a lockfile) =)

@gluons
Copy link

gluons commented May 15, 2020

Or sometime you can just remove node_modules and yarn.lock then reinstall to fix vulnerabilities.

I use this method to fix vulnerabilities on my project many times. 😁

@FDiskas
Copy link

FDiskas commented May 15, 2020

@gluons in this case all packages will be updated and this is probably not what you want. Audit should update only effected packages or sub packages.

@DanielRuf
Copy link
Contributor

Or sometime you can just remove node_modules and yarn.lock

Why not use yarn upgrade-interactive? Also see what dependabot on GitHub and snyk CLI do.

@gluons
Copy link

gluons commented May 15, 2020

Why not use yarn upgrade-interactive?

From my experience, some deep dependencies still haven't been updated. (Some vulnerabilities are from deep nested dependencies.)

If I remove yarn.lock, every dependencies are definitely updated.
So some vulnerabilities are fixed by the latest dependencies.

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

No branches or pull requests