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

Updates the audit implementation to the bulk endpoint #5501

Merged
merged 14 commits into from
Jun 16, 2023
Merged

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Jun 13, 2023

What's the problem this PR addresses?

The current audit implementation uses the older /audit/quick endpoint, which has various problems. One particular is that its design requires to submit a nested payload, but since it doesn't make much sense in our case (because most of Yarn installs are flat), we flatten the package list. It causes problems when multiple packages with different versions can be found in the tree.

Fixes #3861
Fixes #4117
Fixes #5408

Closes #5409 (Supercedes it)


Edit by @merceyz

Fixes #5450
Fixes #2507
Fixes #3778
Fixes #3945
Closes #5309 (Doesn't have a reproduction so I'm assuming it's the same as the others)


How did you fix it?

This change rewrites yarn npm audit to use the new endpoint. As part of the migration a couple of fields are reworked (Via is replaced by Dependents, the versions are now part of a tree item rather than concatenated, we don't get the "recommendation" anymore). The options remain the same for now.

It's possible that some registries don't support the bulk endpoint. Given that it's fairly straightforward to implement, that it's been released for some time now, and that without it we would end up with an invalid audit implementation, I'd tend to let them deal with that.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a bug here, no vulnerabilities are reported for ansi-regex@4.1.0 even though it has one.

yarn add ansi-regex@4.1.0
yarn npm audit

@arcanis
Copy link
Member Author

arcanis commented Jun 14, 2023

Seems to work for me (tried in both the Yarn repo, and a new minimal one) 🤔

Screenshot 2023-06-14 at 09 53 51

@merceyz
Copy link
Member

merceyz commented Jun 14, 2023

I forgot to add the --dev flag to the reproduction steps, sorry about that.

yarn add ansi-regex@4.1.0 --dev
yarn npm audit

Also running a full audit on this repo throws the following error:

$ yarn npm audit --all --recursive
Range Error: Maximum call stack size exceeded
    at Object.stringifyIdent (/berry/packages/yarnpkg-core/sources/structUtils.ts:1:1)
    at Object.get [as stringifyIdent] (/berry/packages/yarnpkg-core/sources/structUtils.ts:15:45)      
    at Object.get [as stringifyIdent] (/berry/packages/yarnpkg-core/sources/index.ts:15:45)
    at processDescriptor (/berry/packages/plugin-npm-cli/sources/npmAuditUtils.ts:125:39)
    at processDescriptor (/berry/packages/plugin-npm-cli/sources/npmAuditUtils.ts:133:9)
    at processDescriptor (/berry/packages/plugin-npm-cli/sources/npmAuditUtils.ts:133:9)
    at processDescriptor (/berry/packages/plugin-npm-cli/sources/npmAuditUtils.ts:133:9)
    at processDescriptor (/berry/packages/plugin-npm-cli/sources/npmAuditUtils.ts:133:9)
    at processDescriptor (/berry/packages/plugin-npm-cli/sources/npmAuditUtils.ts:133:9)
    at processDescriptor (/berry/packages/plugin-npm-cli/sources/npmAuditUtils.ts:133:9)

@arcanis arcanis requested a review from merceyz June 14, 2023 08:35
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm that fixes the previous reproduction however based on the the information provided in #3861 here is another one

corepack npm@8.19.2 init -y
corepack npm@8.19.2 install eslint@8.4.1 webpack-dev-server@3.11.3 --before 2021-12-11
corepack npm@8.19.2 audit
corepack yarn@1.22.19 import
yarn
yarn npm audit

@arcanis
Copy link
Member Author

arcanis commented Jun 15, 2023

Fixed 👍

@arcanis arcanis requested a review from merceyz June 15, 2023 09:37
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, seems like it's not printing all vulnerabilities though.

yarn add axios@0.1.0
yarn npm audit

Ref #3945.

@arcanis
Copy link
Member Author

arcanis commented Jun 15, 2023

Turns out the library we use doesn't support multiple tree items with the same label. Fixed 👍

@arcanis arcanis requested a review from merceyz June 15, 2023 18:37
merceyz
merceyz previously approved these changes Jun 15, 2023
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, well done!

@arcanis arcanis merged commit d93fcea into master Jun 16, 2023
@arcanis arcanis deleted the mael/bulk-audit branch June 16, 2023 08:21
@noisekit noisekit mentioned this pull request Jun 20, 2023
@nitinkothwal
Copy link

Hello @arcanis, Do you know in which version this change is likely to be available? We are eagerly waiting for this change as our Github ci tasks which runs yarn audit fails with 400 bad requests. I checked with latest released yarn version 3.6.1 but this change wasn't there. Do you know any temporary workaround that we can use? Thanks!

@arcanis
Copy link
Member Author

arcanis commented Jul 4, 2023

Since it's a major change to the implementation, it won't be released in 3.x. You can already use the RCs, though.

@naugtur
Copy link

naugtur commented Jul 28, 2023

Would selective fixes to issues causing yarn3 to generate invalid input to the audit endpoint be addressed as part of ongoing v3 maintenance then?

@philmayfield
Copy link

Agreed, if its broken in v3 (which is current at the time of writing) it seems like a maintenance issue?

@ocostello
Copy link

We're on 3.5.1 and are also experiencing it. I'm surprised it's written off as an inclusion to 3.X, seems like a security vulnerability staying on this major version

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