Add allow-list config option to securitychecker_enlightn #1052
Conversation
|
Needs some work, I'll get back to this. |
peterjaap
changed the title
peterjaap
force-pushed
the
patch-1
branch
2 times, most recently
from
2e38185 to
a93d611
Compare
peterjaap
changed the title
|
@veewee done! 🥳 |
| @@ -45,6 +47,9 @@ public function run(ContextInterface $context): TaskResultInterface | |||
| $arguments = $this->processBuilder->createArgumentsForCommand('security-checker'); | |||
| $arguments->add('security:check'); | |||
| $arguments->addOptionalArgument('%s', $config['lockfile']); | |||
| foreach ($config['allow-list'] as $cve) { | |||
There was a problem hiding this comment.
Wouldn't it be better to
->addArgumentArray('--allow-list=%s', $config['allow-list'])or is there a specific reason you want to add an optional argument?
There was a problem hiding this comment.
I've never heard of addArgumentArray, I can't find documentation on addArgumentArray anywhere?
There was a problem hiding this comment.
In your current code, you are iterating over the allow-list configuration just to add it as an argument;
Inside the $arguments (of type \GrumPHP\Collection\ProcessArgumentsCollection), there is a shortcut method addArgumentArray() that does this for you.
This is being used all over the repository to make the tasks easier to read.
You could use it like this:
->addArgumentArray('--allow-list=%s', $config['allow-list'])| @@ -27,6 +27,7 @@ public function provideConfigurableOptions(): iterable | |||
| [ | |||
| 'lockfile' => './composer.lock', | |||
| 'run_always' => false, | |||
| 'allow-list' => [], | |||
| ] | |||
There was a problem hiding this comment.
Can you also add test-cases covering this new argument?
There was a problem hiding this comment.
The test cases should cover all options : you currently changed the default test-case, which has an empty allow list.
On top of that, a new case must be added that tests that if you pass a non-empty array to the allow-list, that the resulting CLI command contains one or multiple --allow-list flags
|
@peterjaap It's a nice feature... Care to finish up this PR? :) |
|
@veewee sure, but I've asked you some questions in the comments |
@peterjaap |
|
@veewee woops, you're right 🙈 Almost a year ago, lol. |
|
We notices this PR became inactive. If you wish to continue, feel free to pick it up again. |
|
Duplicate of newer PR #1161 |
Add
allow-listconfig option to be able to ignore given vulns, see https://github.com/enlightn/security-checker#allow-vulnerabilities