Look into skipping audit of composer operations in package manager tests

I would imagine we will never show the results of the audit directly in the composer UI, it would be an explicit operation, so we should probably look into passing --no-audit both inside and outside of tests, although this could be two separate issues.

So the way I compare run times on gitlab is to click into the individual job, and look at the run-tests.sh summary.

e.g.

Run 1 https://git.drupalcode.org/issue/drupal-3491268/-/jobs/3571254

Drupal\Tests\package_manager\Kernel\PhpTufValidatorTest       13 passes  216s  

Run 2
https://git.drupalcode.org/issue/drupal-3491268/-/jobs/3571532

Drupal\Tests\package_manager\Kernel\PhpTufValidatorTest       13 passes  129s  

Run 3 https://git.drupalcode.org/issue/drupal-3491268/-/jobs/3572029

Drupal\Tests\package_manager\Kernel\PhpTufValidatorTest       13 passes  149s   

However the times are so variable on gitlab at the moment that this is very inconclusive. Also possible it looks different for a different package_manager test.

I think running one of the package_manager tests with a lot of methods/data providers locally before/after is probably a better bet for comparison at the moment.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

First want to say I am so glad Package Manager is in core so we can get more eyes on it to make improvements.

Theoretically we might even want this to be this way in the actual package manager module?

Just want to say that we did the audit in package manager because we were a bit paranoid that our test setup can only test so much and there might be many many differences among real composer projects. So we thought it safest to use audit to hopefully at least ensure the projects are the way composer expects or would want things set up.

I think running composer operations on live site, even though we do it in sandbox first, is potentially dangerous so I think being paranoid and trying to ensure setting up things the *right* way, as much as possible, is warranted.

Doing a require with audit takes longer time, and increases the chance of different forms of timeout.

We also have additional overhead (quite a lot at the moment, hopefully it will improve) from PHP-TUF, so the chances of timeouts are non-zero and agreed that the theoretical chance of a timeout seems more likely than the theoretical chance of composer audit finding a mistake.

What's a good way to test this one?

It can be manually tested with project_browser via contrib. There's no way to manually test it with just core.

Rebased on 11.x

Just one run, but https://git.drupalcode.org/project/drupal/-/pipelines/445049 the build tests, usually the slowest due to package_manager, finished faster than some functional and functional js tests, so this may be bringing the relative time down a bit?

Seems like a small change to a test class, so why not?

Given @tedbow's comment, I'd prefer to have subsystem maintainer review here.

Also I think we should skip the audit operation in regular package_manager operations too - even with php-tuf performance improvements recently there is still a lot of additional http requests etc.

I want to re-afirm @tedbow comments from #11 Generally with testing you want to test "worst case" in this case that means Audit On as it will likely be enabled on sites.

could exit with exit code 1, even if the install was succesful. In a programmatic way, this will for sure create confusion

Arguably an error has occurred and package_manager does need to deal with it.

I would imagine we will never show the results of the audit directly in the composer UI, it would be an explicit operation,

Also I think we should skip the audit operation in regular package_manager operations too -

I can understand not displaying, however silently allowing a 'known vulnerable' package to be installed? To my knowledge php-tuf will not be replacement for composer audit(security advisory parsing).

Large commercial sites are much less likely to use Package Manager, they are going to run controlled labs with dev environments. Tooling like this is really an advantage for the SMB/Home User market. Both of these are signifcantly more likely to run in production. Home users have much less concern, the SMB market however, their insurance agent will look for any reason to reject a claim, intentionally ignoring/not processing AUDIT results may make a very good reason to reject.

Even just having the code on the server may be enough for an auditor to flag a concern. From an audit standpoint it would likely be better to encourage users to set the COMPOSER_NO_AUDIT environment variable through settings.php to disable processing so that it does not impact more secure sites and is not enabled by default.

Is #18 a formal Security Review or should this issue be flagged for that as well?

I can understand not displaying, however silently allowing a 'known vulnerable' package to be installed?

Currently any potential output from composer --audit is discarded by package_manager, so "silently allowing a 'known vulnerable' package to be installed" is the status quo.

The --audit flag is enabled for every package manager operation - this includes unattended automatic updates which could be run via a cli cron job. Possibly the output could be logged there, but logs are ephemeral (especially on smaller sites) so it could easily be missed even if it was logged. Also don't think the output is useful to show people if they're e.g. updating a specific contrib module and the CVE is for a completely different dependency of a different module.

So if we want to alert site admins to known security vulnerabilities in non-Drupal dependencies, then I don't think running with --audit and then throwing the results away is useful at all, it would be better to try to incorporate an explicit composer audit call into update status somewhere (e.g. only when package_manager is enabled) - then it would be available in admin/reports, integrated with e-mail notifications etc. It's pretty unusual to newly install or update a package that has a known CVE, much more likely a package that is already installed has a CVE issued for it and needs update.

Might as well tag for security review as well, but I think a new issue to add an explicit composer audit and show the results would do a lot more here.

Also I'm not sure #11 is really describing what the --audit flag does - it doesn't audit your composer project as such, it checks for security advisories. If there's a problem with a require or update, you get an error long before --audit would happen because it's (iirc) the very last thing to run.

The --audit flag is enabled for every package manager operation - this includes unattended automatic updates which could be run via a cli cron job.

I was reading #4 and following as suggesting that audits NOT be a part of the package manager operation (wasn't aware flag explicitly was there vs the implicit composer use of it by default).

so "silently allowing a 'known vulnerable' package to be installed" is the status quo.

Understood on that being current and IMO bug on its own that it should be handled rather than silently allowed.

I think a new issue to add an explicit composer audit and show the results would do a lot more here.

There can be some value in this. Assuming that an audit call is added as part of a transaction that prevents a vulnerable package from being installed beyond just a 'sandbox' and transferred to 'production' that would likely be acceptable.

I would suggest that should be done before passing the audit ignore flag which may cause a successful return from Composer (in other words, postpone this issue).

Note: I'm not familiar with Project Browsers internals. I see comments such as 'sandbox' above and assume its doing a standard A/B on disk with a staged swap over only after passing basic checks such as composer success.

Untagging for security review because this is a test-only change and clarifying that in the title.

If we want to remove --audit everywhere that could possibly use security review but given the --audit does literally nothing when run via package_manager at the moment, I don't see what it's doing that would be remotely useful.

I've opened #3531138: Consider showing composer audit results in a report as a spin-off though.

Assuming that an audit call is added as part of a transaction that prevents a vulnerable package from being installed

Feel free to open an issue about this, but simply checking if composer audit returns any results isn't sufficient - it would for example prevent updating to a security release of a dependency, just because another dependency had a CVE issued in the last ten minutes or similar. The composer audit results would have to be filtered down to only whether the project is being newly installed, and composer doesn't actually provide a useful list of what is being newly installed. #3525563: Create a Composer plugin that records what packages are being updated, installed, or removed is open to try to workaround that, but it would at minimum need to be postponed on that issue.

Asked this in slack #core-development https://drupal.slack.com/archives/C079NQPQUEN/p1755969156766519 and @phenaproxima
mentioned we should skip audits in tests, so removing that tag.

Re-ran AssetAggregationAcrossPagesTest twice though and it's consistently failing, can that be looked at

Thanks!