-
Notifications
You must be signed in to change notification settings - Fork 84
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
README: update text #63
Conversation
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- A standard should have sensible defaults (and I presumed the Security standard does).
- Using
Security
as the standard will automatically include every single sniff from the Security standard. - 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 ?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
* 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.
0a30d71
to
0fc79c5
Compare
@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. |
Thanks! I actually never activated it on Travis-CI.. working on it right now. |
@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. |
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: |
--standard=Security
instead of pointing to a specific example ruleset.