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 #131

Closed
4 tasks done
Potherca opened this issue Jan 31, 2021 · 8 comments · Fixed by #130
Closed
4 tasks done

Replace deprecated Sensiolabs security checker #131

Potherca opened this issue Jan 31, 2021 · 8 comments · Fixed by #130

Comments

@Potherca
Copy link
Member

Potherca commented Jan 31, 2021

As mentioned in #130, the Sensiolabs security checker has been deprecated and needs to be replaced by an alternative.

Steps to take:

  1. Create a longlist of potential replacement candidates
  2. Define selection criteria
  3. Reduce the longlist to a shortlist based on criteria
  4. Decide on a final replacement

Longlist

Replacement criteria

  • Must not be deprecated
  • Must be callable locally
  • Must be callable/wrappable from the CLI (for the build)
  • Should be lightweight
  • Should be easy to install/update
  • Should be versioned

Shortlist

Out of all the suggested candidates, only the Enlightn security-checker remains, as it is the only one that meets all 6 current criteria. (Pending other candidates or criteria).

Final choice

🔜 @TODO

@Potherca
Copy link
Member Author

@mjrider and @jrfnl Could you chime in here? I want all involved to be heard but I'd like to reach a conclusion with due speed on this.

@Potherca Potherca linked a pull request Jan 31, 2021 that will close this issue
@jrfnl
Copy link
Member

jrfnl commented Jan 31, 2021

@Potherca

Re: the criteria - the versioning criteria seems arbitrary as for security tools always using the latest info is kind of relevant.

Re: the shortlist: for documentation purposes it might be an idea to show a checklist for each of the tools on the long list and which criteria they met/didn't meet. Possibly with a short additional paragraph with the reasoning to (not) prefer the tool ?

@Potherca
Copy link
Member Author

Potherca commented Feb 1, 2021

versioning criteria seems arbitrary

It's more about when the code changes, independent of the DB/list it uses. Requiring dev-master is an anti-pattern I'd rather avoid.

for documentation purposes it might be an idea to show a checklist [..]

👍 I'll update the issue.

@jrfnl
Copy link
Member

jrfnl commented Mar 14, 2021

@Potherca #130 has been merged, but the arguments for using that solution over the other solutions were never added to this ticket... (which is why I didn't merge it before).

@Potherca
Copy link
Member Author

This issue got closed when I merged the accompanying MR, I'll re-open it as I do still intend to update it with more info.

@Potherca Potherca reopened this Mar 15, 2021
@jrfnl
Copy link
Member

jrfnl commented Jun 13, 2021

Okay, so I've started working on moving the rest of the CI from Travis to GH Actions and the first thing I run into straight away is that this new dependency conflicts with our supported PHP versions, so unless we want to jump through hoops to use it, I would strongly advocate replacing the enlightn/security-checker with a tool which actually is compatible with the PHP requirements of this project.

@jrfnl
Copy link
Member

jrfnl commented Jun 13, 2021

PR #137 will remove the enlightn/security-checker (which has a minimum PHP requirement of PHP 5.6, while this projects PHP requirement is >= 5.3) and replace it with the local-php-security-checker.

From the commit message:

Regarding the choice for this tool:

  • This tool will run independently of the PHP version used, making it more versatile.
  • It complies with most other criteria as set forth in Replace deprecated Sensiolabs security checker #131 (not deprecated, lightweight, versioned, easy to install/update).
  • As for running it locally - either devs can download the tool and run it locally or they can use the tooling available to run GH actions locally, so IMO that part is covered too.

If anyone has any objections or concerns about this, please speak up.

@Potherca
Copy link
Member Author

Closing this as #137 has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants