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

Replace deprecated Sensiolabs security checker #130

Merged
merged 2 commits into from
Mar 14, 2021

Conversation

paras-malhotra
Copy link
Contributor

Proposed Changes

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

@paras-malhotra
Copy link
Contributor Author

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

@jrfnl
Copy link
Member

jrfnl commented Jan 28, 2021

@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 ?

@jrfnl jrfnl added the triage label Jan 28, 2021
@paras-malhotra
Copy link
Contributor Author

paras-malhotra commented Jan 28, 2021

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
Copy link
Member

jrfnl commented Jan 28, 2021

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
Copy link
Contributor Author

@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
Copy link
Member

jrfnl commented Jan 28, 2021

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
Copy link
Member

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. 👍

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

@Potherca
Copy link
Member

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. 😄

Potherca
Potherca previously approved these changes Jan 31, 2021
@paras-malhotra
Copy link
Contributor Author

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

@Potherca Potherca merged commit c960cf4 into PHPCSStandards:master Mar 14, 2021
@jrfnl jrfnl mentioned this pull request Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace deprecated Sensiolabs security checker
3 participants