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

Add audit command to check for security issues #10798

Conversation

GuySartorelli
Copy link
Contributor

@GuySartorelli GuySartorelli commented May 24, 2022

Closes #10329

Description

Adds an audit command which checks for security vulnerability advisories for installed packages using the API on packagist.org.

Also adds optional auditing (on by default) to the following commands:

  • create-project
  • update
  • install
  • require
  • remove

Table output (default)

audit-output-table

Plain output

audit-output-plain

Things the audit command does not do:

  • Confirm the packages in vendor dir match what composer.lock says they should be
  • If composer.lock is missing, check for advisories based on the constraints in the composer.json file

Out of scope, but would be a good future enhancement

  • A new --no-insecure (or --allow-insecure) flag could be added to update and require. This could use Auditor to take versions with advisories out of the version candidate pool, so that you can't accidentally update to or require an insecure version of a package.
  • new config in composer.json where you can declare specific advisories which should be ignored by the audit (e.g. the advisory doesn't apply for your project and you don't want audit to keep failing because of it). Would likely need to include the advisory ID in audit output for this to be feasible.

Things I'm unsure about

  • I don't know how to resolve the PHPStan failures - some seem overly strict so I'm hesitant to just blindly follow them.
  • Is there a clean way to test the different formats are working as expected in a unit test?

@stof
Copy link
Contributor

stof commented May 24, 2022

I also got the 502 error when trying the query in the browser. This seems to be caused by too many packages in a single request; if I significantly reduce the number of packages in the query, it returns an appropriate response. This is a major blocker for this functionality being implemented, so if someone knows a way around this, please let me know.

Well, one thing you could do is splitting the list of packages into batches (the batch size should be chosen based on the supported size for the Packagist API) and send multiple requests. Thanks to the parallelization support in Composer 2+, you can even send those requests in parallel.

@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented May 24, 2022

That's a good idea - I couldn't find any information about what the limit actually is but I'll have another look, and failing that I can always do some trial and error to find it. Bonus points for anyone who can point me in the right direction or who already knows what the limit is.

Edit: apparently the API supports POST requests - I'll experiment a bit and see if I can get that to work, cause it's likely POST requests have a much higher limit.

@Seldaek
Copy link
Member

Seldaek commented May 24, 2022

Yes IMO putting the package list in the POST body will fix this as this is AFAIK not size limited at all, unlike URL length which has limits in various places.

@Seldaek Seldaek added this to the 2.4 milestone May 24, 2022
@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented May 26, 2022

Currently blocked because the packagist API returns a 404 error on POST requests. I've created an issue for it under composer/packagist#1309

Edit: It does if I do things correctly. :P Not blocked anymore.

@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented May 26, 2022

The basic functionality is working now - any review on the AuditCommand or Auditor, updates to existing commands, and related documentation would be useful at this stage.

I'm especially eager for feedback on the UX - I'm aware that the current output is probably not ideal, but I'm not sure how it should look. The parent issue references yarn audit which formats the output as a table - should we look at doing something like that? If so can someone point me in the right direction for how to format such an output?

@GuySartorelli GuySartorelli force-pushed the pulls/main/check-security-advisories branch from 42fc493 to 577d350 Compare May 26, 2022 10:49
@GuySartorelli
Copy link
Contributor Author

It looks like there's some PHPStan issues as well - to be honest I don't understand what I need to do to resolve those. A lot of the errors are saying the new command doesn't define some options... but it doesn't need those options.

@GuySartorelli
Copy link
Contributor Author

I've resolved some of the PHPStan issues. The ones that I think are false alarms I've tried to add to the baseline, but it doesn't seem to have taken. There are a couple of PHPStan issues there as well which are unrelated to this PR.

This is consistent with the return value of all the other setX() methods
in this class.
@GuySartorelli GuySartorelli force-pushed the pulls/main/check-security-advisories branch 3 times, most recently from 4673e43 to 5300806 Compare June 3, 2022 11:56
@ktomk
Copy link
Contributor

ktomk commented Jun 10, 2022

For the output format could JSON Text be another variant? I've seen there is a table, so perhaps some very straight forward JSON Array of JSON Objects could be nice to pass along to further (automated) processing.

@GuySartorelli
Copy link
Contributor Author

@ktomk At this stage I'd just be keen to see the feature merged - that sounds like an enhancement that can be added after the fact quite easily if there is real world use for it. I'd be quite happy for someone to create a follow-up PR with that format if there is a need for it but it's not something I want to add to this PR right now.

Copy link
Member

@Seldaek Seldaek left a comment

Choose a reason for hiding this comment

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

Thanks, looking good I think now, a couple typos/nitpicks and then it's mergeable I think.

doc/03-cli.md Outdated Show resolved Hide resolved
doc/03-cli.md Outdated Show resolved Hide resolved
doc/03-cli.md Outdated Show resolved Hide resolved
doc/03-cli.md Outdated Show resolved Hide resolved
src/Composer/Installer.php Outdated Show resolved Hide resolved
src/Composer/Util/Auditor.php Outdated Show resolved Hide resolved
src/Composer/Command/AuditCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/AuditCommand.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented Jun 12, 2022

Requested changes have been made.
Edit: Oops looks like I broke some tests... will fix those now.

@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented Jun 12, 2022

Tests fixed - there is one failure there but I think it's unrelated to this PR.

@Seldaek
Copy link
Member

Seldaek commented Jun 22, 2022

Thanks! Merged as d93239d and fixed up some remaining issues in 611b215 🥳

@Seldaek Seldaek closed this Jun 22, 2022
@GuySartorelli
Copy link
Contributor Author

Awesome, thank you!
Should I create an issue to track/remind about moving Auditor into its own repository post-stable, or do you have an alternative to-do list somewhere with that on it?

@Seldaek
Copy link
Member

Seldaek commented Jun 23, 2022

The more I think of it the more I think we can't really extract this, as it'll probably need to be integrated deeper into the Repository code than it is now.

I think the better way would be to publish a packagist.org API client library really, that would be completely separate from Composer, and would not require Package objects or anything to function.

@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented Jun 23, 2022

That's fair. I'd be happy to help develop such a library with as much or as little guidance as you want to provide. While I could implement such a library in some arbitrary repo it makes sense to me that there would be such a repository under the composer organisation which people can just add to their dependencies when and as they need it.

@stof
Copy link
Contributor

stof commented Jun 23, 2022

You mean something like https://packagist.org/packages/knplabs/packagist-api ?

@Seldaek
Copy link
Member

Seldaek commented Jun 23, 2022

Ah yes there's that, then maybe worth sending a PR there porting the Auditor querying bits @GuySartorelli if you feel like it?

@GuySartorelli GuySartorelli deleted the pulls/main/check-security-advisories branch June 23, 2022 10:22
@GuySartorelli
Copy link
Contributor Author

I'll take a look - I wasn't aware of that package. Still doesn't feel as appropriate as having one in GitHub.com/composer but I won't push for that if the appetite for creating it isnt here 😅

@barryvdh
Copy link
Sponsor Contributor

For the output format could JSON Text be another variant? I've seen there is a table, so perhaps some very straight forward JSON Array of JSON Objects could be nice to pass along to further (automated) processing.

I created a follow-up PR for the json format here: #10965

emahorvat52 pushed a commit to emahorvat52/composer that referenced this pull request Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security Advisory Features
8 participants