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

feat: [SUP-2192] Adding manifest file to vuln card if scanning multi-project #170

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

dotkas
Copy link
Contributor

@dotkas dotkas commented Dec 5, 2023

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

What this does

Before

A while back there was a change done to the CLI which altered the way we did naming for certain projects. It had a lot of regression errors introduced and this change is one of the remediations for fixing this, as rolling back was no longer an option.

For nuget packages, when scanning multiple projects, the overview would not adequately give a user an overview of what exact projects has been scanned:

Screenshot 2023-12-05 at 15 11 17

Further, the list of vulnerabilities would also not give the user an idea about which project contains the vulnerability:

Screenshot 2023-12-05 at 15 12 07

This change

This change adds a manifest path to all vulnerabilities when multiple projects are scanned:

Screenshot 2023-12-05 at 15 13 03

As well as a more details list of paths if nuget is scanned specifically:

Screenshot 2023-12-05 at 15 13 29

.. but I've limited this last change to nuget only, as this does not seem to be an issue for other ecosystems. Should that change, we can alter the conditional logic pretty quickly.

More information

Screenshots

See inline.

@dotkas dotkas force-pushed the dotkas/SUP-2192/add-target-file-to-nuget-results branch 3 times, most recently from a985e82 to 9f13587 Compare December 5, 2023 13:47
@dotkas dotkas force-pushed the dotkas/SUP-2192/add-target-file-to-nuget-results branch from 9f13587 to f741b90 Compare December 5, 2023 13:51
@dotkas dotkas marked this pull request as ready for review December 5, 2023 14:15
@dotkas dotkas requested a review from a team as a code owner December 5, 2023 14:15
@dotkas dotkas force-pushed the dotkas/SUP-2192/add-target-file-to-nuget-results branch from 52e38e4 to d816d27 Compare December 5, 2023 14:15
Copy link
Contributor

@j-luong j-luong left a comment

Choose a reason for hiding this comment

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

question: if the nuget conditional is removed, this will just add the full manifest path to all results? I don't see this as an issue, is there any reason for the caution?

Asking as, having specific branching logic like the one we're introducing can be a pain to understand without prior context.

@dotkas
Copy link
Contributor Author

dotkas commented Dec 5, 2023

if the nuget conditional is removed, this will just add the full manifest path to all results? I don't see this as an issue, is there any reason for the caution?

@j-luong

Yes, should I swap the logic?

I can add a story in comments about why. I have seen other conditionals in the code related to ecosystem oddities so I figured it was an ok way to go.

I was not comfortable just adding this logic for all ecosystems.. was not sure if it would cause regression or angry users of other ecosystems.

I can do it if you think it would be OK - for sure

@j-luong
Copy link
Contributor

j-luong commented Dec 5, 2023

@dotkas
I think if all it does is amend the path with the full filepath and add that to each vuln as Manifest File in the report, then it's minor enough to include everywhere IMO.

Although I tried a quick test with a Node project and it looks like a bit more work is required on top of just omitting the conditionals in the .hbs templates:
Screenshot 2023-12-05 at 18 08 50

@dotkas
Copy link
Contributor Author

dotkas commented Dec 6, 2023

@j-luong i'll ping you on Slack, need to understand more.

@dotkas dotkas force-pushed the dotkas/SUP-2192/add-target-file-to-nuget-results branch from fce9323 to 854f3df Compare December 6, 2023 19:01
@dotkas dotkas force-pushed the dotkas/SUP-2192/add-target-file-to-nuget-results branch from 854f3df to ce935fb Compare December 6, 2023 19:20
@dotkas dotkas requested a review from j-luong December 6, 2023 19:21
@dotkas dotkas enabled auto-merge December 7, 2023 10:45
@dotkas dotkas merged commit 4b86151 into master Dec 7, 2023
11 checks passed
@dotkas dotkas deleted the dotkas/SUP-2192/add-target-file-to-nuget-results branch December 7, 2023 10:47
Copy link

github-actions bot commented Dec 7, 2023

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants