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

[Feature]: Ignore devDependencies #91

Closed
mandric opened this issue Jun 6, 2019 · 13 comments
Closed

[Feature]: Ignore devDependencies #91

mandric opened this issue Jun 6, 2019 · 13 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@mandric
Copy link

mandric commented Jun 6, 2019

Hi, this tools looks great and I hope to replace some home grown stuff I wrote.

would it be useful to have an "ignore dev" option? So vulnerabilities in dev dependencies would be ignored. For example:

audit-ci --high fails on vulnerabilities that are high or critical
audit-ci --high-prod would only fail only on non-dev dependencies that are high or critical.

I'm curious what you think! Thank you.

@quinnturner
Copy link
Member

quinnturner commented Jun 6, 2019

I was wondering when this question would come up! 😄

My personal belief is that this is not the desired behaviour; if {npm|yarn} audit identifies fixable vulnerabilities, the developer should address the issue. If a dev package got severely compromised such as grabbing the user's or server's SSH keys or running a crypto miner, this should be caught immediately.

Regardless, if you want this behaviour, the appropriate course of action would be to allowlist each dev dependency individually. I would suggest creating a config file to make the CLI command less verbose.

Here's an example:

audit-ci.json

{
  "high": true,
  "allowlist": ["mocha", "chai"]
}

Then for the CLI:

audit-ci --config ./audit-ci.json

@quinnturner quinnturner added enhancement New feature or request question Further information is requested labels Jun 6, 2019
@mandric
Copy link
Author

mandric commented Jun 6, 2019

Fair enough!

@quinnturner
Copy link
Member

Thanks for the feature suggestion though. I will close for now but will pin this issue for future users.

@quinnturner quinnturner pinned this issue Jun 6, 2019
@quinnturner quinnturner changed the title ignore dev [Feature]: Ignore devDependencies Jun 6, 2019
@mandric
Copy link
Author

mandric commented Jun 6, 2019

👍

@shockey
Copy link

shockey commented Jul 19, 2019

@quinnturner, would you consider reopening this?

Our use case is that we want to have different severity cutoffs for dev and production dependencies: any production dependency vulnerability should cause a failure since users will be notified of them upon install of our module, but only high/critical dev dependency vulnerabilities should be show-stoppers.

Currently, we're doing this in another tool by doing two passes with different configurations.

@ghost
Copy link

ghost commented Oct 4, 2019

Seconding here that this would be incredibly helpful for a project I'm working on too

@philstavri
Copy link

It would be really useful to be able to target prod dependencies only, for example when building PRs and targeting all dependencies, i.e. for an overnight build.

Also worth noting it looks like the reporting of the dependencies is a bit messed up (they are all coming through as dependencies despite most being in devDependencies):

"dependencies": 60754,
"devDependencies": 0,
"optionalDependencies": 0,
"totalDependencies": 60754

@quinnturner
Copy link
Member

@JoeNewmanMSM What specifically would be useful in this case? Is it the multiple levels of dependency vulnerability catching?

Does the current solution of specifying what vulnerabilities to ignore satisfy your needs? There are three options available.

"advisories": [12, 23, 24], <-- across all packages, ignore advisory xxx
"whitelist": ["example1", "example2"], <-- ignore all advisories for package yyy (useful for dev)
"path-whitelist": ["52|example3", "880|example4", "880|example5>example4"], <-- Ignore specific advisories for specific packages

@philstavri, I don't know what to suggest in your case other than the above solution.

@betimer
Copy link

betimer commented Nov 15, 2019

How is this feature going? Has it been developed? @quinnturner

@quinnturner
Copy link
Member

@betimer, while I understand the convenience listing all devDependencies as whitelisted, I still stand by my initial reason why it is likely not appropriate behaviour in my previous comment: #91 (comment). I believe that my previously mentioned solution is still valid. If you have a particular use-case that is not covered by that solution I will consider integrating it.

@simontabor
Copy link

@quinnturner would you consider an option to set the severity level for dev dependencies? I agree with your justification about not skipping dev devs (disaster waiting to happen, a dev dep can easily affect your prod code), but I think it's a common and justified use-case to set different failure thresholds.

Something like this:

audit-ci --moderate --dev-critical

@quinnturner
Copy link
Member

quinnturner commented Aug 30, 2020

@simontabor I would consider that as an option. I do understand that there may be different requirements for severities in dev vs. prod.

I do have one major concern with the devDependency approach. I just ran yarn audit --json on a Yarn project with one vulnerable devDependency and no dependencies. It explicitly does not handle devDependencies and treated it as a regular dependency. To accommodate this, we would have to traverse the original package.json and negotiate whether a dependency is dev or not.

{
   "type":"auditAdvisory",
   "data":{
      "resolution":{
         "id":690,
         "path":"cryo",
         "dev":false,
         "bundled":false,
         "optional":false
      },
      "advisory":{
         "findings":[
            {
               "version":"0.0.6",
               "paths":[
                  "cryo"
               ]
            }
         ],
         "id":690,
         "created":"2018-08-16T19:50:35.895Z",
         "updated":"2019-06-24T23:04:26.491Z",
         "deleted":null,
         "title":"Code Injection",
         "found_by":{
            "link":"",
            "name":"Alexey Tyurin"
         },
         "reported_by":{
            "link":"",
            "name":"Alexey Tyurin"
         },
         "module_name":"cryo",
         "cves":[
            "CVE-2018-3784"
         ],
         "vulnerable_versions":">=0.0.0",
         "patched_versions":"<0.0.0",
         "overview":"All versions of `cryo` are vulnerable to code injection due to an Insecure implementation of deserialization.\n\n\n## Proof of concept\n\n```\nvar Cryo = require('cryo');\nvar frozen = '{\"root\":\"_CRYO_REF_3\",\"references\":[{\"contents\":{},\"value\":\"_CRYO_FUNCTION_function () {console.log(\\\\\"defconrussia\\\\\"); return 1111;}\"},{\"contents\":{},\"value\":\"_CRYO_FUNCTION_function () {console.log(\\\\\"defconrussia\\\\\");return 2222;}\"},{\"contents\":{\"toString\":\"_CRYO_REF_0\",\"valueOf\":\"_CRYO_REF_1\"},\"value\":\"_CRYO_OBJECT_\"},{\"contents\":{\"__proto__\":\"_CRYO_REF_2\"},\"value\":\"_CRYO_OBJECT_\"}]}'\nvar hydrated = Cryo.parse(frozen);\nconsole.log(hydrated);\n```",
         "recommendation":"No fix is currently available. Consider using an alternative module until a fix is made available.",
         "references":"- [HackerOne Report](https://hackerone.com/reports/350418)",
         "access":"public",
         "severity":"high",
         "cwe":"CWE-502",
         "metadata":{
            "module_type":"",
            "exploitability":7,
            "affected_components":""
         },
         "url":"https://npmjs.com/advisories/690"
      }
   }
}
{
   "type":"auditSummary",
   "data":{
      "vulnerabilities":{
         "info":0,
         "low":0,
         "moderate":0,
         "high":1,
         "critical":0
      },
      "dependencies":1,
      "devDependencies":0,
      "optionalDependencies":0,
      "totalDependencies":1
   }
}

Here's the package.json:

{
  "name": "audit-ci-yarn-high-vulnerability",
  "description": "Test package.json with high vulnerability",
  "devDependencies": {
    "cryo": "0.0.6"
  }
}

I will note that NPM's results are correct:

{
  "actions": [
    {
      "action": "review",
      "module": "cryo",
      "resolves": [
        {
          "id": 690,
          "path": "cryo",
          "dev": true,
          "bundled": false,
          "optional": false
        }
      ]
    }
  ],
  "advisories": {
    "690": {
      "findings": [
        {
          "version": "0.0.6",
          "paths": [
            "cryo"
          ]
        }
      ],
      "id": 690,
      "created": "2018-08-16T19:50:35.895Z",
      "updated": "2019-06-24T23:04:26.491Z",
      "deleted": null,
      "title": "Code Injection",
      "found_by": {
        "link": "",
        "name": "Alexey Tyurin"
      },
      "reported_by": {
        "link": "",
        "name": "Alexey Tyurin"
      },
      "module_name": "cryo",
      "cves": [
        "CVE-2018-3784"
      ],
      "vulnerable_versions": ">=0.0.0",
      "patched_versions": "<0.0.0",
      "overview": "All versions of `cryo` are vulnerable to code injection due to an Insecure implementation of deserialization.\n\n\n## Proof of concept\n\n```\nvar Cryo = require('cryo');\nvar frozen = '{\"root\":\"_CRYO_REF_3\",\"references\":[{\"contents\":{},\"value\":\"_CRYO_FUNCTION_function () {console.log(\\\\\"defconrussia\\\\\"); return 1111;}\"},{\"contents\":{},\"value\":\"_CRYO_FUNCTION_function () {console.log(\\\\\"defconrussia\\\\\");return 2222;}\"},{\"contents\":{\"toString\":\"_CRYO_REF_0\",\"valueOf\":\"_CRYO_REF_1\"},\"value\":\"_CRYO_OBJECT_\"},{\"contents\":{\"__proto__\":\"_CRYO_REF_2\"},\"value\":\"_CRYO_OBJECT_\"}]}'\nvar hydrated = Cryo.parse(frozen);\nconsole.log(hydrated);\n```",
      "recommendation": "No fix is currently available. Consider using an alternative module until a fix is made available.",
      "references": "- [HackerOne Report](https://hackerone.com/reports/350418)",
      "access": "public",
      "severity": "high",
      "cwe": "CWE-502",
      "metadata": {
        "module_type": "",
        "exploitability": 7,
        "affected_components": ""
      },
      "url": "https://npmjs.com/advisories/690"
    }
  },
  "muted": [],
  "metadata": {
    "vulnerabilities": {
      "info": 0,
      "low": 0,
      "moderate": 0,
      "high": 1,
      "critical": 0
    },
    "dependencies": 0,
    "devDependencies": 1,
    "optionalDependencies": 0,
    "totalDependencies": 1
  },
  "runId": "e561d7a9-89c4-4e9b-8c94-32be0598a5aa"
}

Given these results, it seems only reasonable to implement it in NPM (without writing a lot more complex and potentially brittle code... What happens when using Yarn workspaces and the dependency is using different versions at different levels of the tree and some are dev and some are prod?)

@quinnturner
Copy link
Member

With #186, this highly requested feature will have a solid middle-ground solution in the upcoming v4.1.0 release. While I still stand by #91 (comment) that skipping devDependencies is strictly inferior for security than using allowlist if using it as your only vulnerability detection mechanism, I understand that there are several workflows that skipping dev dependencies makes sense. Thanks to @WhatIfWeDigDeeper for the PR!

@quinnturner quinnturner unpinned this issue Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants