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

Add allow-list config option to securitychecker_enlightn #1052

Open
wants to merge 1 commit into
base: v2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions doc/tasks/securitychecker/enlightn.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ grumphp:
securitychecker_enlightn:
lockfile: ./composer.lock
run_always: false
allow-list: []
```

**lockfile**
Expand All @@ -32,3 +33,9 @@ If your `composer.lock` file is located in an exotic location, you can specify t
*Default: false*

When this option is set to `false`, the task will only run when the `composer.lock` file has changed. If it is set to `true`, the `composer.lock` file will be checked on every commit.

**allow-list**

*Default: empty array*

When an array is set, these values (vulnerabilities) will be passed on to the security checker to ignore the given values. See [the enlightn/security-checker docs](https://github.com/enlightn/security-checker#allow-vulnerabilities) for more information.
2 changes: 2 additions & 0 deletions src/Task/SecurityChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ public static function getConfigurableOptions(): OptionsResolver
'end_point' => null,
'timeout' => null,
'run_always' => false,
'allow-list' => [],
]);

$resolver->addAllowedTypes('lockfile', ['string']);
$resolver->addAllowedTypes('format', ['null', 'string']);
$resolver->addAllowedTypes('end_point', ['null', 'string']);
$resolver->addAllowedTypes('timeout', ['null', 'int']);
$resolver->addAllowedTypes('run_always', ['bool']);
$resolver->addAllowedTypes('allow-list', ['array']);

return $resolver;
}
Expand Down
5 changes: 5 additions & 0 deletions src/Task/SecurityCheckerEnlightn.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ public static function getConfigurableOptions(): OptionsResolver
$resolver->setDefaults([
'lockfile' => './composer.lock',
'run_always' => false,
'allow-list' => []
]);

$resolver->addAllowedTypes('lockfile', ['string']);
$resolver->addAllowedTypes('run_always', ['bool']);
$resolver->addAllowedTypes('allow-list', ['array']);

return $resolver;
}
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

I've never heard of addArgumentArray, I can't find documentation on addArgumentArray anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

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'])

$arguments->addOptionalArgument('--allow-list=%s', $cve);
}

$process = $this->processBuilder->buildProcess($arguments);
$process->run();
Expand Down
1 change: 1 addition & 0 deletions test/Unit/Task/SecurityCheckerEnlightnTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function provideConfigurableOptions(): iterable
[
'lockfile' => './composer.lock',
'run_always' => false,
'allow-list' => [],
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add test-cases covering this new argument?

Copy link
Author

Choose a reason for hiding this comment

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

Like which cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

];
}
Expand Down