Correctly label packages as dev-only in yarn audit

[as3richa]

Summary

yarn audit currently does not distinguish packages that are depended on only in dev versus packages that are depended on in prod (in particular it doesn't send a dev: true key in the payload for the registry audit API endpoint).

Because of this, the registry endpoint implicitly assumes that every package is depended on in prod, and yields dev: false for every finding of every advisory it uncovers, even if that instance of the package is only a (transitive) dev dependency. yarn audit --json basically echos the response verbatim, hence those findings are incorrectly labelled as affecting production in the output.

I initially attempted to fix this in #6910. While that solution worked adequately, I was told (and I agree) that the additional complexity introduced to the hoister wasn't worth the improvement.

In this PR, I take an alternative approach: I introduced a new utility function that, given a package manifest, workspace layout, and lockfile, walks the dependency tree, enumerating transitive dependencies that are strictly development-only (i.e. transitive development dependencies that are not also transitive production dependencies).

With this machinery, we can enumerate dev-only dependencies in yarn audit, and trivially label any such dependencies with dev: true in the request. This causes the registry to correctly label any advisories against such dependencies with dev: true in the response. Since yarn audit --json basically echoes the response from the registry verbatim, we get the correct client-side behavior for free.

In the interest of keeping the PR small and focused, I didn't add the dev-vs.-prod information to the human-readable output of yarn audit.

Test plan

Running yarn audit --json and trimming out the noise:

1.13.0

{
  "type": "auditAdvisory",
  "data": {
    "resolution": {
      "id": 535,
      "path": "mime",
      "dev": false,
      "optional": false,
      "bundled": false
    },
    "advisory": {
      "findings": [
        {
          "version": "1.4.0",
          "paths": [
            "mime"
          ],
          "dev": false,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 535,
      "created": "2017-09-25T19:02:28.152Z",
      "updated": "2018-04-09T00:38:22.785Z",
      "deleted": null,
      "title": "Regular Expression Denial of Service",
      "module_name": "mime"
    }
  }
}
{
  "type": "auditAdvisory",
  "data": {
    "resolution": {
      "id": 566,
      "path": "hoek",
      "dev": false,
      "optional": false,
      "bundled": false
    },
    "advisory": {
      "findings": [
        {
          "version": "4.2.0",
          "paths": [
            "hoek"
          ],
          "dev": false,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 566,
      "created": "2018-04-20T21:25:58.421Z",
      "updated": "2018-04-20T21:25:58.421Z",
      "deleted": null,
      "title": "Prototype pollution",
      "module_name": "hoek"
    }
  }
}

My PR

{
  "type": "auditAdvisory",
  "data": {
    "resolution": {
      "id": 535,
      "path": "mime",
      "dev": false,
      "optional": false,
      "bundled": false
    },
    "advisory": {
      "findings": [
        {
          "version": "1.4.0",
          "paths": [
            "mime"
          ],
          "dev": false,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 535,
      "created": "2017-09-25T19:02:28.152Z",
      "updated": "2018-04-09T00:38:22.785Z",
      "deleted": null,
      "title": "Regular Expression Denial of Service",
      "module_name": "mime"
    }
  }
}
{
  "type": "auditAdvisory",
  "data": {
    "resolution": {
      "id": 566,
      "path": "hoek",
      "dev": true,
      "optional": false,
      "bundled": false
    },
    "advisory": {
      "findings": [
        {
          "version": "4.2.0",
          "paths": [
            "hoek"
          ],
          "dev": true,
          "optional": false,
          "bundled": false
        }
      ],
      "id": 566,
      "created": "2018-04-20T21:25:58.421Z",
      "updated": "2018-04-20T21:25:58.421Z",
      "deleted": null,
      "title": "Prototype pollution",
      "module_name": "hoek"
    }
  }
}

In the first case, the second advisory's findings are incorrectly labelled as affecting production; my PR demonstrates the correct behavior.

[buildsize]

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	1.11 MB	1.11 MB	1.27 KB (0%)
yarn-[version].js	4.47 MB	4.47 MB	4.41 KB (0%)
yarn-legacy-[version].js	4.66 MB	4.67 MB	5.08 KB (0%)
yarn-v[version].tar.gz	1.12 MB	1.12 MB	849 bytes (0%)
yarn_[version]all.deb	815.75 KB	816.25 KB	514 bytes (0%)

[arcanis]

Much better, thanks! It looks good to me - @rally25rs, want to have a look?

[as3richa]

Anything I can do to help this through? 🙏

[as3richa]

@arcanis Bump

[as3richa]

@arcanis? 😁

[as3richa]

@arcanis @rally25rs I've just rebased off latest master. I'd really appreciate if I could get some further 👀 here.

[arcanis]

Hey - sorry for not taking a look sooner. Most of my time the past few week has been spent on the v2 codebase, and since I'm not super familiar with the audit code it wasn't at the top of my priorities 😅

I'll try to take a look this week unless @rally25rs beats me to it 🙂

[rally25rs]

Sorry for the extremely slow response... I remember just ignoring the prod vs dev thing when I originally did the audit code because there wasn't an easy way to tell from the data that was available.

I'll try to take a look at this as soon as I find some free time.

Do you know what will happen if a dep is both a prod and a dev dep? From a quick glance at the code I think it might flag it as a dev dep, which I think it only should if it is also not in the production deps tree.

[as3richa]

@rally25rs

Do you know what will happen if a dep is both a prod and a dev dep? From a quick glance at the code I think it might flag it as a dev dep

This isn't the case; getTransitiveDevDependencies enumerates transitive development dependencies that are not also production dependencies. It's a bit of a misnomer, but I couldn't think of a better name of reasonable length, so I erred on keeping the name legible and documenting the behavior in comments.

[rally25rs]

it's unfortunate that we have to traverse over all the packages again, but not sure that there is a better solution given the information we have available at that time.

Thanks for the contribution, and sorry again that this took so long to look at. 👍

[as3richa]

Thanks for taking the time @arcanis @rally25rs 😁