phpinsights icon indicating copy to clipboard operation
phpinsights copied to clipboard

Replace Packagist API call with Enlightn Security Checker

Open paras-malhotra opened this issue 3 years ago • 2 comments

Q A
Bug fix? no
New feature? no
Fixed tickets #...

Followup PR to https://github.com/nunomaduro/phpinsights/pull/453.

This PR replaces the native Packagist API call with the Enlightn Security Checker.

Benefits of this PR include:

  1. Reduce maintenance burden of API calls, etc. and lets an independent package handle this. Other packages such as GrumPHP are also moving to the Enlightn security checker as it is almost a drop-in replacement to the earlier Sensiolabs security checker.
  2. It has built-in HTTP caching, so is more efficient than the API calls to Packagist.

This is my first PR to PHP Insights, so let me know if you'd like me to make any changes.

WDYT?

paras-malhotra avatar Feb 03 '21 20:02 paras-malhotra

Hi :wave:

Thanks for working on this.

We quickly provided a fix on 1.14 to avoid problem for users, and we based this fix by changing the API called.

Adding your package could be interresting, did you made some benchmark against current implementation that ?

For now, I see 2 points :

  • Your dev should target master, as we don't provide anymore "new feature" on v1.x. (by new feature, I mean a new package added in dependency)
  • in master, we have a notion of cache, and with a command line flag, we force a complete analyse. Can you handle this flag to force update the database of advisories ?

Jibbarth avatar Feb 09 '21 20:02 Jibbarth

Hi @Jibbarth, I can make a new PR to target master. I haven't done any benchmarks against the current implementation.

As I understand the cache system, if certain files are unchanged the cache is triggered. Now, with the project there are 2 changes that can happen:

  1. Change in composer.lock for added dependencies or modified versions of dependencies.
  2. Change in security advisories database.

HTTP caching is already built-in with the package. So, if there is no change in security advisories database, there will be a 304 Not Modified response and the package will not attempt to re-download the database, and rather rely on the cached database.

In this scenario, how should we implement the cache? If we set the cache key as say the md5 of composer.lock file contents, we run the risk of missing a new security advisory for the same dependencies. And there is no need to cache the advisory database as this is already built-in.

paras-malhotra avatar Feb 09 '21 20:02 paras-malhotra

There has been no movement on this, and tests were failing. Not keen to add another dependency we have to then manage moving forward

JustSteveKing avatar Sep 06 '22 09:09 JustSteveKing