-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Change audit.ignore behavior before 2.6.0 #11605
Conversation
a8b998b
to
c66fd04
Compare
src/Composer/Advisory/Auditor.php
Outdated
$pkgPlurality = $ignoredAffectedPackagesCount === 1 ? '' : 's'; | ||
$punctuation = $format === 'summary' ? '.' : ':'; | ||
$io->writeError("<info>$ignoredTotalAdvisoryCount ignored security vulnerability advisor{$plurality} affecting $ignoredAffectedPackagesCount package{$pkgPlurality}{$punctuation}</info>"); | ||
$this->outputAdvisories($io, $ignoredAdvisories, $format); |
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.
Since we may have the reason for ignoring a given security advisory, I'd consider adding that to the output (also to the JSON above). What do you think?
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.
Yes makes sense to me. It probably needs to be highlighted like <warning>Ignored: $reason</>
And for the table output add an Ignore column with Yes
/empty and no reason to keep it small perhaps, because otherwise the table width will blow up
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.
Yes makes sense to me. It probably needs to be highlighted like Ignored: $reason</>
Just to be clear, do you mean display all ignore reasons as console message unless the output format is JSON?
Speaking of JSON, partially because the JSON output leverages that advisories are JSON serializable, should I just introduce a new class IgnoredSecurityAdvisory extends SecurityAdvisory{}
that can also hold the reason?
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 I was also wondering how to best achieve this. For sure for JSON it would be easier with a subclass..
For the plain text output I meant it probably should go here https://github.com/composer/composer/blob/main/src/Composer/Advisory/Auditor.php#L207 as a new line with a so it is highlighted as being ignored.
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.
It seems to me that wrapping text in tables works OOTB, so is there any need for workarounds like...?
$ignoreReason = '';
if ($advisory instanceof IgnoredSecurityAdvisory && $advisory->ignoreReason !== NULL) {
if (strlen($advisory->ignoreReason) > 80) {
$ignoreReason = substr($advisory->ignoreReason, 0, 77) . '...';
}
else {
$ignoreReason = $advisory->ignoreReason;
}
}
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.
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.
f8e5182
to
b0a7d05
Compare
e570c5a
to
708db5e
Compare
src/Composer/Advisory/Auditor.php
Outdated
} | ||
else { | ||
if ($advisory instanceof SecurityAdvisory) { | ||
$advisory = new IgnoredSecurityAdvisory( |
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.
should we create an object that extends SecurityAdvisory in which you copy all the metadata or should we rather have a IgnoredSecurityAdvisory
that would take a SecurityAdvisory|PartialSecurityAdvisory
as argument and implements its jsonSerialize
method by adding an ignore_reason key in the array returned by the delegated call ?
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.
These were the options that I also considered, the solution in the PR just felt better, even if it is "ugly" a bit.
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.
Perhaps IgnoredSecurityAdvisory
could be a proxy class or have a factory method which takes in a SecurityAdvisory
object. That'd make the code here much cleaner.
$advisory = IgnoredSecurityAdvisory::create($advisory, $ignoreReason);
or $advisory = new IgnoredSecurityAdvisory($advisory, $ignoreReason);
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.
IgnoredSecurityAdvisory::createFromSecurityAdvisory($advisory
I would have gone with this approach by default, but since everything in public on these objects (as public properties) this approach just moves copying object data to somewhere else. Would that have a real added value? In this case, should I make the constructor of IgnoredSecurityAdvisory
private so it could be only constructed from a SecurityAdvisory from now?
src/Composer/Advisory/Auditor.php
Outdated
} | ||
else { | ||
if ($advisory instanceof SecurityAdvisory) { | ||
$advisory = new IgnoredSecurityAdvisory( |
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.
Perhaps IgnoredSecurityAdvisory
could be a proxy class or have a factory method which takes in a SecurityAdvisory
object. That'd make the code here much cleaner.
$advisory = IgnoredSecurityAdvisory::create($advisory, $ignoreReason);
or $advisory = new IgnoredSecurityAdvisory($advisory, $ignoreReason);
src/Composer/Advisory/Auditor.php
Outdated
$advisory->cve, | ||
$advisory->link | ||
); | ||
if ($ignoreReason !== NULL) { |
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.
I thought that the reason was a required field.
Edit: I've now seen that there are two accepted values and only one has the 'reason' field. I'd vote that a reason should always be provided/required.
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.
Actually I decided to give more freedom to developers... so it cannot be mandatory.
https://github.com/composer/composer/pull/11605/files#diff-89992649ff6652d737d4dac645fbbac6ceeb112ab29010d0eaba3b42c658dc7eR112-R132
@mxr576 please don't change anything anymore, I'm editing a couple things here and then merging :) thanks a lot for the quick work here! |
I think it looks good now but I perhaps broke some tests.. anyway bedtime, will wrap up tomorrow. |
Roger! 🙂 Time well spent! 🍻 |
Ok looking good, thanks again. |
Closes #11604