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 support for code suppression fields #192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

z4ce
Copy link

@z4ce z4ce commented Nov 7, 2024

This changes the issue count, adds a card for suppressions, and also moves any suppressed issues to the end of the list.

  • Tests written and linted ℹ︎
  • Commits are squashed and tidy and are suitable to become release notes

What this does

Customer with code consistent ignores and who are using snyk-to-html want to see the fact they have ignored things be reflected in the snyk-to-html output. This PR does three things:

  1. Displays a card on each suppressed vulnerability showing the details of the suppression
  2. Modifies the issue count to remove suppressed issues from the count
  3. Alters the order of the displayed vulnerabilities such that suppressed issues are always at the bottom

Notes for the reviewer

Instructions on how to run this locally, background context, what to review, questions…

More information

Screenshots

Visuals that may help the reviewer
image
unified-ignores.html.zip

@z4ce z4ce force-pushed the suppressions branch 3 times, most recently from 279d5dc to 8a1a007 Compare November 8, 2024 21:45
@z4ce z4ce marked this pull request as ready for review November 8, 2024 21:49
@z4ce z4ce requested a review from a team as a code owner November 8, 2024 21:49
ignoredOn: suppression.properties?.ignoredOn,
ignoredBy: suppression.properties?.ignoredBy,
};
Copy link
Member

Choose a reason for hiding this comment

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

issue: We should add explicit fallback values here for each of the optional values. This would be particularly useful for the ignoredBy property which is an object we use in templates. For example:

ignoredBy: suppression.properties?.ignoredBy || {
  name: 'unknown',
  email: '?'
}

Alternatively we could return null for ignoredBy and then the templates should guard against this scenario by gating each ignoredBy.propertyName lookup with a #if suppression.ignoredBy

Comment on lines 355 to 378
OrderedIssuesArray.forEach(project => {
project.vulnerabilities = project.vulnerabilities.map(vuln => {
if (vuln.suppressions && vuln.suppressions.length > 0) {
vuln.suppression = processSuppression(vuln.suppressions[0]);
}
return vuln;
}).sort((a, b) => {
if (a.suppression && !b.suppression) return 1;
if (!a.suppression && b.suppression) return -1;
return 0;
});
});
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: We only need to perform this sort option if one or more of the project vulnerabilities had suppression data. So we could skip this step if hasSuppressedVulns = false within the forEach loop.

Suggested change
OrderedIssuesArray.forEach(project => {
project.vulnerabilities = project.vulnerabilities.map(vuln => {
if (vuln.suppressions && vuln.suppressions.length > 0) {
vuln.suppression = processSuppression(vuln.suppressions[0]);
}
return vuln;
}).sort((a, b) => {
if (a.suppression && !b.suppression) return 1;
if (!a.suppression && b.suppression) return -1;
return 0;
});
});
OrderedIssuesArray.forEach(project => {
let hasSuppressedVulns = false;
const projectVulns = project.vulnerabilities.map(vuln => {
if (vuln.suppressions && vuln.suppressions.length > 0) {
hasSuppressedVulns = true;
vuln.suppression = processSuppression(vuln.suppressions[0]);
}
return vuln;
});
if (!hasSuppressedVulns) {
project.vulnerabilities = projectVulns;
return; // Early return if no suppressions
}
// Sort only if necessary
projectVulns.sort((a, b) => {
if (a.suppression && !b.suppression) return 1;
if (!a.suppression && b.suppression) return -1;
return 0;
});
project.vulnerabilities = projectVulns;
});

@@ -36,6 +36,43 @@
<h2 class="card__panel__heading"><span class="heading-char">✓</span> Fix Analysis</h2>
<div class="card__panel__markdown">{{{markdown ruleiddesc.help.markdown}}}</div>
</div>
{{#if suppression}}
Copy link
Member

Choose a reason for hiding this comment

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

praise: This gating is good and will ensure our other customers do not get the information.

(report) => {
t.contains(
report,
'<div class="suppression-card">',
Copy link
Member

@thisislawatts thisislawatts Nov 11, 2024

Choose a reason for hiding this comment

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

suggestion: To better protect ourselves from introducing a regression we could add a guard assertion to one of our existing test cases to check that the output doesNotHave <div class="suppression-card"> in the output.

color: #333;
}

.user-initial {
Copy link
Member

Choose a reason for hiding this comment

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

question: Should this be namespaced under .suppression-card?

This changes the issue count, adds a card for suppressions,
and also moves any suppressed issues to the end of the list.
@z4ce
Copy link
Author

z4ce commented Dec 5, 2024

@thisislawatts I thought I had pushed these changes a week ago.. I think this addresses everything we talked about

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