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

Conversation

peterjaap
Copy link

@peterjaap peterjaap commented Nov 15, 2022

Q A
Branch master for features and deprecations
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Documented? yes
Fixed tickets #1037

Add allow-list config option to be able to ignore given vulns, see https://github.com/enlightn/security-checker#allow-vulnerabilities

@peterjaap
Copy link
Author

Needs some work, I'll get back to this.

@peterjaap peterjaap changed the title Add allow-list config option to securitychecker_enlightn Draft: Add allow-list config option to securitychecker_enlightn Nov 15, 2022
@peterjaap peterjaap force-pushed the patch-1 branch 2 times, most recently from 2e38185 to a93d611 Compare November 19, 2022 16:10
@peterjaap peterjaap changed the title Draft: Add allow-list config option to securitychecker_enlightn Add allow-list config option to securitychecker_enlightn Nov 19, 2022
@peterjaap
Copy link
Author

@veewee done! 🥳

Copy link
Contributor

@veewee veewee left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

It seems like it is not completely ready yet. Can you take a look at the remarks?

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

@@ -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

@veewee veewee changed the base branch from master to v2.x June 18, 2023 13:29
@veewee
Copy link
Contributor

veewee commented Sep 29, 2023

@peterjaap It's a nice feature... Care to finish up this PR? :)

@peterjaap
Copy link
Author

@veewee sure, but I've asked you some questions in the comments

@veewee
Copy link
Contributor

veewee commented Sep 29, 2023

@veewee sure, but I've asked you some questions in the comments

@peterjaap
Is it possible you made those comments as part of a code review but did not submit those comments? I am not seeing any.

@peterjaap
Copy link
Author

@veewee woops, you're right 🙈 Almost a year ago, lol.

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