PHPCSStandards / composer-installer

Composer installer for PHP_CodeSniffer coding standards
https://packagist.org/packages/dealerdirect/phpcodesniffer-composer-installer
MIT License
549 stars 36 forks source link

Replace deprecated Sensiolabs security checker #130

Closed paras-malhotra closed 3 years ago

paras-malhotra commented 3 years ago

Proposed Changes

Replaces the deprecated Sensiolabs security checker with the Enlightn security checker.

paras-malhotra commented 3 years ago

I guess the line length PR (https://github.com/Dealerdirect/phpcodesniffer-composer-installer/pull/128) needs to be merged first.

jrfnl commented 3 years ago

@paras-malhotra Thanks for your willingness to contribute.

Just out of interest: is there any particular reason why you've chosen to go with this tool instead of the recommended successor of the project: https://github.com/fabpot/local-php-security-checker ?

paras-malhotra commented 3 years ago

Hi @jrfnl,

Before I answer that question, let me tell you that I built the Enlightn security checker and the reasons I chose to build it over using the local php security checker are:

  1. The Enlightn security checker is licensed under MIT so it can be used in any app. The local php security checker which is licensed under AGPLv3, and thus cannot be used by any non-AGPL app.
  2. The Enlightn security checker can be pulled in via composer whereas the local-php-security-checker needs to download binaries. This depends on system architecture (different binaries for different architecture) and needs hacky solutions / shell scripts to make it work.
jrfnl commented 3 years ago

Thanks for your reply. Let's also see what the others have to say about the PR.

The local php security checker which is licensed under AGPLv3, and thus cannot be used by any non-AGPL app.

Just a side-note about this: tooling does not always have to have a compatible license to be used by a project. The license only really comes into play, if the tooling would be distributed with a package or if the code would be modified.

Just running the software by a non-AGPL app does not constitute a license violation AFAIK.

paras-malhotra commented 3 years ago

@jrfnl here's a reference: https://softwareengineering.stackexchange.com/questions/107883/agpl-what-you-can-do-and-what-you-cant#:~:text=2%20Answers&text=The%20AGPL%20is%20based%20on,but%20this%20is%20murky%20ground.

The AGPL is based on the GPL, not the LGPL. It does not contain any linking exceptions, and any work using AGPL code (linked or otherwise, modified or not) must also be AGPL licensed and distributed.

This seems that even if the code isn't modified, it will still have to be AGPL licensed. I'm in no way a legal export though, but I based my decision on this.

jrfnl commented 3 years ago

I saw that, but I also read the license itself. Though, same as you, I'm not a lawyer (and I have seriously doubts whether the people who commented on stack exchange were....).

Potherca commented 3 years ago

Some specific points:

I guess the line length PR (#128) needs to be merged first.

Yeah, my bad, it fell off my radar. (Thankfully @jrfnl has gently nudged it back into my sight again).

[..] MIT so it can be used in any app [..] licensed under AGPLv3, and thus cannot be used by any non-AGPL app.

This statement is false. This project is merely a consumer of the product not the code. Our code can function 100% without the code, hence the licensing is irrelevant, from a consumer perspective. If we were to use the code things would be different.

I'm in no way a legal export though, but I based my decision on this.

I may have more experience in this, as I have done license management and compliance for several clients in the past. I am confident in my previous statement on this.

Further thoughts:

  1. The Sensio security checker has been deprecated, so we need to decide on a replacement.
  2. I would suggest the normal process (longlist; shortlist; vote/final choice) applies
  3. enlightn/security-checker can be placed on the long-list, it will be fairly judged against whatever criteria we deem relevant for any replacement. From what I've seen from browsing through the repo, it's a strong candidate. :+1:

I've opened #131 to come to a conclusion.

Potherca commented 3 years ago

I've discussed with @mjrider (and updated the related issue) and thus far the Enlightn security checker is the only candidate still standing. Unless @jrfnl has an opposing view, this is going to get merged.

@paras-malhotra I've merged #128. Could you pull the changes and rebase you code? (That should fix the YAML Lint errors).

On a side note, regarding a totally unrelated project, we're going to use your library to replace the Sensio lib in pipeline-components/php-security-checker. :smile:

paras-malhotra commented 3 years ago

@Potherca that's awesome! I've merged the upstream changes, so this should be good to go. Thanks!