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

README: update text #63

Merged
merged 2 commits into from
Mar 16, 2020

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Feb 18, 2020

  • Add some badges.
  • Improve some of the grammar of various texts.
  • Improve use of markdown.
  • Let the default suggested way of running the tooling be with --standard=Security instead of pointing to a specific example ruleset.
  • Update the example output.

README.md Outdated
```
phpcs --extensions=php,inc,lib,module,info --standard=./vendor/pheromone/phpcs-security-audit/example_base_ruleset.xml /your/php/files/
phpcs --standard=Security /your/php/files/ --extensions=php,inc,lib,module,info
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick question, how come this change will use any ruleset file? You need to load example_base_ruleset.xml to have the proper settings (paranoia, some details for some sniffs) and currently this changes implies that it's just running the standard, so all load rules without any settings as defined in a ruleset file?

Copy link
Contributor Author

@jrfnl jrfnl Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. A standard should have sensible defaults (and I presumed the Security standard does).
  2. Using Security as the standard will automatically include every single sniff from the Security standard.
  3. As paranoia is a config setting, it can be set from the command-line, so setting up a custom ruleset for that - while a good idea - is not strictly necessary.

To be fair, the example rulesets as they are don't make much sense to me at all. They are extremely explicit and can be simplified a lot while still doing the same.

I also wonder if it wouldn't make more sense to have separate named rulesets for common projects, like SecurityDrupal7, SecurityDrupal8. Those can easily be hosted within this package, it would just require adding a top-level folder with the standard name + a ruleset.xml file within that folder with the customizations for that type of project.

For an example of such a setup, have a look at WordPressCS: https://github.com/WordPress/WordPress-Coding-Standards
They have a WordPress standard in which all the sniffs live.
In addition to that, they have WordPress-Core, WordPress-Docs and WordPress-Extra standards which are, as per the suggestion above, just a folder with a ruleset.xml file.

Basically having example rulesets which require a lot of understanding of how PHPCS works makes adoption more difficult, while being able to just use a preset standard lowers the threshold for entry.

Does that explanation help at all ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it does help thank you. You're offering me a point of view I don't see much otherwise. That's important because the initial audience of this tool was security professionals that wanted to have something to help them do code review, but now it had clearly evolved.

I'm willing to accept the changes to make it compatible with life in 2020, however I want to make sure that old installations still run and that people who just want to run the tool while not knowing very much about phpcs (actually knowing nothing) can still do it and tweak the sniffs.

I also have to point out a very important detail that I just realized with your explanation: if you load all Sniffs in Security/ right now, the tool will be broken in two ways:

  • If you take a look with what happens with Utils.php and UtilsFactory.php it needs the CmsFramework config entry to run properly
  • Since we are running all sniffs, this means that we'll run Drupal rules for a normal PHP project

To fix that I think we could have the configuration settings from the base example config needed copied into ruleset.xml.

So I'm afraid the tool wasn't simply designed to work exactly like you are proposing right now and we can't merge this part of this README update.

I really want to merge the rest tho, so can you please update this PR without the -standard=Security changes?

For the --standard=Security changes, I propose we create another PR with it in the README and copy base ruleset into Security/ruleset.xml. I still want to keep the base and drupal7 ruleset files for backward compatibility of integrations and giving some people a "in your face" way of customizing the rules. I can see room also for an example file with Paranoia mode at off. If you don't 100% get what I'm saying, let me do the PR I think my code will express my ideas better.

For following Wordpress model with empty folders and ruleset.xml files, I think it could work although we'll be changing literally the name of the standard for Drupal7 and right now this could break existing integrations/customization of sniffs.

Since my time is limited for this project, I think that's where I would draw the line on where to stop refactoring. Like I stated a few times left and right in other requests, I'd highly prefer to put my time on things that needs some security knowledge (eg. how to tweak rules and documentation). It's motivating to have someone contribute as much as you do, but at the same time I have to honor what I've committed to (documentation) and so far I'm getting sidetracked and need to select what I'm working on.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmarcil I'm currently travelling for a conference, but will try and update the PR some time over the next few days.

Once I'm back in the office, I will also have a closer look at the issues identified in this discussion with the main and the example rulesets and see if I can come up with some simple tweaks to make them more easily usable.

I know and respect that your time on this project is limited. Please do keep your focus where you think it should be. I just hope that while your focus is on other things, you will still welcome PRs created by me (and others) to fix the things which are not your immediate focus area.

jrfnl added 2 commits March 9, 2020 17:34
* Add some badges.
* Improve some of the grammar of various texts.
* Improve use of markdown.
* Let the default suggested way of running the tooling be with `--standard=Security` instead of pointing to a specific example ruleset.
* Update the example output.
... and two more tiny grammar fixes.
@jrfnl jrfnl force-pushed the feature/readme-update branch from 0a30d71 to 0fc79c5 Compare March 9, 2020 16:45
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 9, 2020

@jmarcil I've reverted the example commands for now as discussed above

This includes reverting (updating) the example output to be in line with the actual command in the readme.

The other changes related to the rulesets discussed above will/should be addressed in separate PRs.

P.S.: I rebased the PR to see the Travis build being triggered, but it appears to not be turned on correctly yet. Let me know if you need help getting it working.

@jmarcil jmarcil merged commit f029fcd into FloeDesignTechnologies:master Mar 16, 2020
@jmarcil
Copy link
Collaborator

jmarcil commented Mar 16, 2020

Thanks!

I actually never activated it on Travis-CI.. working on it right now.

@jrfnl jrfnl deleted the feature/readme-update branch March 16, 2020 00:54
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 16, 2020

@jmarcil Should be a case of just toggling the switch for the repo. Let me know if you need help/screencast on how to do it.

@jmarcil
Copy link
Collaborator

jmarcil commented Mar 16, 2020

Should be 😄

I don't know why, but I couldn't see FloeDesignTechnologies in my organizations (yet I can see another one), but I could still browse https://travis-ci.org/github/FloeDesignTechnologies and activate the repo from there.

Build is working as far as I can tell:
https://travis-ci.org/github/FloeDesignTechnologies/phpcs-security-audit

@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 16, 2020

@jmarcil Excellent! Glad you got it working. Will be helpful for adding the rest of the checks proposed in #56 and making sure the automated tests are all running & passing.

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

Successfully merging this pull request may close these issues.

2 participants