Skip to content

Commit

Permalink
Fixes bugs in yarn npm audit (#5833)
Browse files Browse the repository at this point in the history
**What's the problem this PR addresses?**

The rewrite of `yarn npm audit` broke a couple of things in the advisory
reporting.

Fixes #5824 - The `--exclude` filter wasn't applied to deprecations
Fixes #5830 - The `--json` output was printing the raw registry output,
not the filtered tree

**How did you fix it?**

Fixed the deprecation iterations (we were iterating the unfiltered
data).

Fixed the json reporting to instead print the same tree as the one we
would have displayed outside of `--json`. It's _technically_ a breaking
change, but it was supposed to be like that so it's probably fine to
treat it as a 7.0.1 bugfix.

Added tests to prevent regressions.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.

---------

Co-authored-by: Wojciech Maj <kontakt@wojtekmaj.pl>
  • Loading branch information
arcanis and wojtekmaj authored Oct 24, 2023
1 parent 577e957 commit ccb28f2
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 12 deletions.
34 changes: 34 additions & 0 deletions .yarn/versions/1dfc7586.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
releases:
"@yarnpkg/cli": patch
"@yarnpkg/core": patch
"@yarnpkg/plugin-npm-cli": patch

declined:
- "@yarnpkg/plugin-compat"
- "@yarnpkg/plugin-constraints"
- "@yarnpkg/plugin-dlx"
- "@yarnpkg/plugin-essentials"
- "@yarnpkg/plugin-exec"
- "@yarnpkg/plugin-file"
- "@yarnpkg/plugin-git"
- "@yarnpkg/plugin-github"
- "@yarnpkg/plugin-http"
- "@yarnpkg/plugin-init"
- "@yarnpkg/plugin-interactive-tools"
- "@yarnpkg/plugin-link"
- "@yarnpkg/plugin-nm"
- "@yarnpkg/plugin-npm"
- "@yarnpkg/plugin-pack"
- "@yarnpkg/plugin-patch"
- "@yarnpkg/plugin-pnp"
- "@yarnpkg/plugin-pnpm"
- "@yarnpkg/plugin-stage"
- "@yarnpkg/plugin-typescript"
- "@yarnpkg/plugin-version"
- "@yarnpkg/plugin-workspace-tools"
- "@yarnpkg/builder"
- "@yarnpkg/doctor"
- "@yarnpkg/extensions"
- "@yarnpkg/nm"
- "@yarnpkg/pnpify"
- "@yarnpkg/sdks"
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,39 @@ describe(`Commands`, () => {
}, async ({path, run, source}) => {
await run(`install`);

await run(`npm`, `audit`, `--exclude`, `vulnerable`);
await expect(run(`npm`, `audit`, `--exclude`, `vulnerable`)).resolves.toMatchObject({
stdout: expect.stringContaining(`No audit suggestions`),
});
}),
);

test(
`it should allow excluding packages (deprecations)`,
makeTemporaryEnv({
dependencies: {
[`no-deps-deprecated`]: `1.0.0`,
},
}, async ({path, run, source}) => {
await run(`install`);

await expect(run(`npm`, `audit`, `--exclude`, `no-deps-deprecated`)).resolves.toMatchObject({
stdout: expect.stringContaining(`No audit suggestions`),
});
}),
);

test(
`it should allow excluding packages (when using --json)`,
makeTemporaryEnv({
dependencies: {
[`vulnerable`]: `1.0.0`,
},
}, async ({path, run, source}) => {
await run(`install`);

await expect(run(`npm`, `audit`, `--exclude`, `vulnerable`, `--json`)).resolves.toMatchObject({
stdout: ``,
});
}),
);

Expand All @@ -177,7 +209,24 @@ describe(`Commands`, () => {
}, async ({path, run, source}) => {
await run(`install`);

await run(`npm`, `audit`, `--ignore`, `1`);
await expect(run(`npm`, `audit`, `--ignore`, `1`)).resolves.toMatchObject({
stdout: expect.stringContaining(`No audit suggestions`),
});
}),
);

test(
`it should allow ignoring advisories (when using --json)`,
makeTemporaryEnv({
dependencies: {
[`vulnerable`]: `1.0.0`,
},
}, async ({path, run, source}) => {
await run(`install`);

await expect(run(`npm`, `audit`, `--ignore`, `1`, `--json`)).resolves.toMatchObject({
stdout: ``,
});
}),
);

Expand Down
14 changes: 5 additions & 9 deletions packages/plugin-npm-cli/sources/commands/npm/audit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export default class NpmAuditCommand extends BaseCommand {
...this.excludes,
]));

const payload = Object.create(null);
const payload: Record<string, Array<string>> = Object.create(null);

for (const [packageName, versions] of packages)
if (!excludedPackages.some(pattern => micromatch.isMatch(packageName, pattern)))
Expand All @@ -131,12 +131,12 @@ export default class NpmAuditCommand extends BaseCommand {
registry,
}) as unknown as Promise<npmAuditTypes.AuditResponse>;

const deprecations = await Promise.all(this.noDeprecations ? [] : Array.from(packages, async ([packageName, versions]) => {
const deprecations = this.noDeprecations ? [] : await Promise.all(Array.from(Object.entries(payload), async ([packageName, versions]) => {
const registryData = await npmHttpUtils.getPackageMetadata(structUtils.parseIdent(packageName), {
project,
});

return miscUtils.mapAndFilter(versions.keys(), version => {
return miscUtils.mapAndFilter(versions, version => {
const {deprecated} = registryData.versions[version];

// Apparently some packages have a `deprecated` field set to an empty string
Expand Down Expand Up @@ -213,7 +213,7 @@ export default class NpmAuditCommand extends BaseCommand {

const hasError = Object.keys(expandedResult).length > 0;

if (!this.json && hasError) {
if (hasError) {
treeUtils.emitTree(npmAuditUtils.getReportTree(expandedResult), {
configuration,
json: this.json,
Expand All @@ -229,11 +229,7 @@ export default class NpmAuditCommand extends BaseCommand {
json: this.json,
stdout: this.context.stdout,
}, async report => {
report.reportJson(result);

if (!hasError) {
report.reportInfo(MessageName.EXCEPTION, `No audit suggestions`);
}
report.reportInfo(MessageName.EXCEPTION, `No audit suggestions`);
});

return hasError ? 1 : 0;
Expand Down
2 changes: 1 addition & 1 deletion packages/yarnpkg-core/sources/treeUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export function emitTree(tree: TreeNode, {configuration, stdout, json, separator
// Another one for the second level fields. We run it twice because in some pathological cases the regex matches would
if (separators >= 2)
for (let t = 0; t < 2; ++t)
treeOutput = treeOutput.replace(/^([β”‚ ].{2}[β”œβ”‚ ].{2}[^\n]+\n)(([β”‚ ]).{2}[β”œβ””].{2}[^\n]*\n[β”‚ ].{2}[β”‚ ].{2}[β”œβ””]─)/gm, `$1$3 β”‚ (\\n)?\n$2`).replace(/^β”‚\n/, ``);
treeOutput = treeOutput.replace(/^([β”‚ ].{2}[β”œβ”‚ ].{2}[^\n]+\n)(([β”‚ ]).{2}[β”œβ””].{2}[^\n]*\n[β”‚ ].{2}[β”‚ ].{2}[β”œβ””]─)/gm, `$1$3 β”‚ \n$2`).replace(/^β”‚\n/, ``);

if (separators >= 3)
throw new Error(`Only the first two levels are accepted by treeUtils.emitTree`);
Expand Down

0 comments on commit ccb28f2

Please sign in to comment.