yarnpkg / yarn

The 1.x line is frozen - features and bugfixes now happen on https://github.com/yarnpkg/berry
https://classic.yarnpkg.com
Other
41.44k stars 2.72k forks source link

yarn audit --level returns non-zero exit code #7260

Closed eliihen closed 5 years ago

eliihen commented 5 years ago

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

Bug

What is the current behavior? When running yarn audit --level critical on a package with no critical vulnerabilities, the command exits with exit code 14. It seems to return a non-0 exit code whenever there are vulnerabilities for any level.

Because many CI tools use exit code to check for success this makes introducing the command in CI hard. It would be beneficial to be able to introduce it with a severity level of critical and gradually tighten the level.

If the current behavior is a bug, please provide the steps to reproduce.

$ yarn audit --level critical
yarn audit v1.16.0
32 vulnerabilities found - Packages audited: 873250
Severity: 2 Low | 15 Moderate | 15 High
Done in 6.19s.

$ echo $?                              
14

What is the expected behavior? Whenever yarn audit --level <level> prints no vulnerabilities for that level and above the exit code of the command should be 0.

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

Node v10.15.3
Yarn 1.16.0
OS Fedora 30
DanielRuf commented 5 years ago

Hi @esphen,

The --level only filters the results in the table. https://yarnpkg.com/en/docs/cli/audit

The command will exit with a non-0 exit code if there are issues of any severity found.

https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/audit.js#L133

https://github.com/yarnpkg/yarn/commit/298e0ea6cea3ab8a610cabf28de3fdf8e7fa8d1f

Only print advisories with severity greater than or equal to one of the following:

yarn audit --level critical
yarn audit v1.16.0
2 vulnerabilities found - Packages audited: 128
Severity: 1 Moderate | 1 High
✨  Done in 0.81s.

➜  yarn-audit-test yarn audit --level info    
yarn audit v1.16.0
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.4.0                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/796                         │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ high          │ Cross-Site Scripting (XSS)                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.0.0                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/328                         │
└───────────────┴──────────────────────────────────────────────────────────────┘
2 vulnerabilities found - Packages audited: 128
Severity: 1 Moderate | 1 High
✨  Done in 0.79s.
eliihen commented 5 years ago

Hi @DanielRuf

Fair enough, but this is unexpected behaviour imho. So while this may be a feature, not a bug, I'm not sure this feature has been properly thought through.

This basically means many people can't use yarn audit because they're not interested in breaking builds for "Low" issues. Since there is no way to discriminate when yarn exits with a non-zero exit code, it's an "all-or-nothing" situation, which is not very flexible.

DanielRuf commented 5 years ago

Hi @esphen,

yarn audit does generally output a non-zero exit code if it finds something.

The exit code should differ based on the result, see https://github.com/yarnpkg/yarn/blob/298e0ea6cea3ab8a610cabf28de3fdf8e7fa8d1f/src/cli/commands/audit.js#L158-L177

You may have to write a small shell or NodeJS script (child_process.spawn()) to catch the exit code and evaluate it (sort of a procelain mode).

Because the exit code is what you want to check.

yarn audit --level info
yarn audit v1.16.0
0 vulnerabilities found - Packages audited: 13
✨  Done in 0.54s.
➜  yarn-react-test echo $?
0
➜  yarn-react-test yarn add jquery@3.0.0
yarn add v1.16.0
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...
success Saved lockfile.
success Saved 1 new dependency.
info Direct dependencies
└─ jquery@3.0.0
info All dependencies
└─ jquery@3.0.0
✨  Done in 0.31s.
➜  yarn-react-test yarn audit --level info
yarn audit v1.16.0
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.4.0                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/796                         │
└───────────────┴──────────────────────────────────────────────────────────────┘
1 vulnerabilities found - Packages audited: 14
Severity: 1 Moderate
✨  Done in 0.69s.
➜  yarn-react-test echo $?
4
eliihen commented 5 years ago

Accidentally closed the issue, sorry about that.

So, I know what the exit code represents, and I am perfectly capable of writing a shell script. However in CI systems like Travis where a yaml file may say something like:

steps:
- yarn build
- yarn test
- yarn lint
- yarn audit --level foo 

The last step does not work as written and may be represented as bash scripts/do-audit.sh, but that is needless indirection imho. Why make this so hard?

Edit: Standing outside in the rain with my phone I missed the part where the exit codes were different for the different severity levels. That's very useful information, thank you

DanielRuf commented 5 years ago

The last step does not work as written and may be represented as bash scripts/do-audit.sh, but that is needless indirection imho. Why make this so hard?

So far any security issue / vulnerability is problematic in most cases. And this is the default behavior which is also documented in the Yarn docs. If you need another one you have to create a small JS file which runs the check, checks the exit code and return the needed exit code - which is added as script in the scripts field of package.json.

This would be my proposed solution until we have proper exit code filtering and some sort of porcelain mode (this may be a good feature request but would have to be reworded or filed as separate issue then - as a user I want to filter the exit code also based on the provided --level flag).

DanielRuf commented 5 years ago

Just as some real world example, the jQuery vulnerability from my example would be critical for eCommerce and many other solutions.

yarn audit v1.16.0
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.4.0                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ jquery                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/796                         │
└───────────────┴──────────────────────────────────────────────────────────────┘
1 vulnerabilities found - Packages audited: 14

Not moderate imho. https://github.com/DanielRuf/snyk-js-jquery-174006 It was higher before this but the vulnerability in this case has to be evaluated on a per-project basis.

https://snyk.io/vuln/SNYK-JS-JQUERY-174006

CVSS scoring is hard. If security matters, every vulnerability is relevant and important, especially in audits (done with Lighthouse which uses the Snyk vulnerability database).

DanielRuf commented 5 years ago

The last step does not work as written and may be represented as bash scripts/do-audit.sh, but that is needless indirection imho. Why make this so hard?

In general I have something like this in package.json in a few of our internal projects (so we can also run these with npm-run-all -p security:** -l

  "scripts": {
    "start": "gulp",
    "clean": "gulp clean",
    "build": "gulp build --production",
    "security": "npm-run-all -p security:** -l",
    "security:auditjs": "auditjs --suppressExitError --level error --tokenxxx --username xxx@xxx.xx",
    "security:retired": "retire --cachedir ~/.retire-cache",
    "security:snyk": "snyk auth xxx && snyk test --file=yarn.lock || true"
  },
DanielRuf commented 5 years ago

dependabot which was bought by GitHub may be more useful for opensource projects to keep track of patches and GitHub also checks lockfiles and informs you as maintainer and owner if there are any vulnerabilities found.

In all other cases, evaluate the exit code and make it an additional job instead of putting it into the same which does your build (if security is not important to you).

pitgrap commented 5 years ago

Could you please update the documentation about the different exit codes (and the new flags level and groups are missing too): https://yarnpkg.com/lang/en/docs/cli/audit/

We ran into the same problem today calling "yarn audit --level=high" via mvn,frontend-maven-plugin on a jenkins server with some low vulnerabilities in the result and the build failed because of the exit code.

christiankiely commented 5 years ago

Here's a one liner I'm using in Jenkins to only emit a non-zero exit code for High vulnerabilities or higher: /bin/bash -c 'yarn audit; [[ $? -ge 8 ]] && exit 1 || exit 0'

antrew commented 5 years ago

There is an open pull request that documents the exit code: https://github.com/yarnpkg/website/pull/894/files I think it will help a lot to merge it, so that there are less questions on what the exit code means.

Haroenv commented 5 years ago

The PR has been merged, and this should be clear enough now, thanks @antrew

cyberfox1 commented 3 years ago

Any progress on this?

imgalli commented 3 years ago

I just hit into this, we run yarn audit in CI with the expectation that it's roughly the same as npm audit.

I take the points that any security issues should be dealt with and things like Dependabot can help but this removes users ability to define what level an issue is.

Clemens85 commented 2 years ago

I also stumbled over this the last days, and although this issue is closed (and yes, there is a workaround), I still want to strongly suggest to add at least an own flag to get an exit code matching the specified level, because this is the expectation that many developers have obviously. And if running in a CI this is still the result which most developers might want to achieve... All those security stuff is already hard enough, so it would make sense not to make it even harder...

bf commented 1 year ago

There should be a command line flag for this, why complicate it forcing devs to use a complex bash oneliner...

Exit only for moderate vulnerabilities:

/bin/bash -c "(yarn audit --groups 'dependencies devDependencies' --level moderate; [[ $? -ge 4 ]] && exit 1 || exit 0)" && \

Exit only for high vulnerabilities:

/bin/bash -c "(yarn audit --groups 'dependencies devDependencies' --level high; [[ $? -ge 8 ]] && exit 1 || exit 0)" && \