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

NuGet vulnerability warnings: Warn in non-Release mode, Error in non-Release mode #17244

Open
wants to merge 4 commits into
base: contrib
Choose a base branch
from

Conversation

emmagarland
Copy link
Contributor

@emmagarland emmagarland commented Oct 10, 2024

Initial adjustment of the projects with package vulnerabilities that errored due to GHSA-qj66-m88j-hmgj 09/10/24.

Changed to ignore the four specific NuGet vulnerability warnings in Debug mode (but not Release) as per Microsoft docs (NU1901,NU1902,NU1903,NU1904)

Relates to #17235.

Also updated some formatting errors in tests.

Prerequisites

  • [x ] I have added steps to test this contribution in the description below

Description

  • In non-Release mode (e.g. Debug), the following NuGet vulnerability warnings should warn: NU1901,NU1902,NU1903,NU1904
  • In Release mode, they should error

Next steps

If this approach is confirmed to work, I will add the configuration to all projects in case they get any future vulnerabilities.

…errored, to change to ignore the four specific Nuget vulnerability warnings in Debug mode (but not Release) as per https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1901-nu1904 (NU1901,NU1902,NU1903,NU1904)
@emmagarland emmagarland changed the title Create different behaviour for NuGet vulnerability warnings in Release mode and non-Release mode Error for NuGet vulnerability warnings in Release mode and Warn in non-Release mode Oct 10, 2024
Copy link

github-actions bot commented Oct 10, 2024

Hi there @emmagarland, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

  • It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
  • The automated tests all pass (see "Checks" tab on this PR)
  • The level of security for this contribution is the same or improved
  • The level of performance for this contribution is the same or improved
  • Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
  • If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
  • 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@emmagarland emmagarland changed the title Error for NuGet vulnerability warnings in Release mode and Warn in non-Release mode NuGet vulnerability warnings: Warn in non-Release mode, Error in non-Release mode Oct 10, 2024
@emmagarland emmagarland marked this pull request as ready for review October 10, 2024 20:22
…d.props, combine WarningsNotAsErrors and fix minor issues
@ronaldbarendse
Copy link
Contributor

I've slightly changed the way the WarningsNotAsErrors are set and can confirm that debug builds only emit a warning for vulnerable NuGet dependencies, but release builds error out.

Testing was as easy as downgrading one on the package versions, for example:

<PackageVersion Include="System.Runtime.Caching" Version="9.0.0-rc.1.24431.7" />

And running dotnet build -c Release to see the error 👌🏻

We might want to change the condition to only emit the error when building a public version (without the --preview version prefix) though, as regular PR builds will do release builds and we don't really want to fail all PR builds/checks when a vulnerability is reported on any dependency. I'll have to see check how we can do this though!

@ronaldbarendse
Copy link
Contributor

ronaldbarendse commented Oct 11, 2024

We might want to change the condition to only emit the error when building a public version (without the --preview version prefix) though, as regular PR builds will do release builds and we don't really want to fail all PR builds/checks when a vulnerability is reported on any dependency. I'll have to see check how we can do this though!

I had a quick look into this and although Nerdbank.GitVersioning does set the PublicRelease MSBuild property, the NuGet vulnerability audit is not really part of the build (as it is also done when running a dotnet restore for example).

Maybe a better solution is to simply disable the auditing (using <NuGetAudit>false</NuGetAudit> in Directory.Build.props, as documented) and update the Azure Pipelines build to enable this again for public release builds by adding it to the dotnet commands (e.g. dotnet restore -p:NuGetAudit=true). We could also opt for changing the audit level, so PRs still fail if they have high or critical advisories, but release builds also fail on low advisories (using NuGetAuditLevel).

@emmagarland
Copy link
Contributor Author

Ah ok, so if that works for the build process that's a cool idea.

I'm happy to go with you and the HQ team's preferences on that! It's definitely a good shout to have more urgent behaviour for the more severe warnings.

@emmagarland
Copy link
Contributor Author

@ronaldbarendse is there anything else needed for this or can we progress it to be merged? Would that be HQ or Core Collage?

Just aware that I didn't want it to get out of date with all the pushes and then have the same issue for everyone again.

@umbrabot
Copy link

umbrabot commented Dec 4, 2024

Hi there @emmagarland!

First of all: A big #H5YR for making an Umbraco related contribution during Hacktoberfest! We are very thankful for the huge amount of PRs submitted, and all the amazing work you've been doing 🥇

Due to the amazing work you and others in the community have been doing, we've had a bit of a hard time keeping up. 😅 While all of the PRs for Hacktoberfest might not have been merged yet, you still qualify for receiving some Umbraco swag, congratulations! 🎉

In the spirit of Hacktoberfest we've prepared some exclusive Umbraco swag for all our contributors - including you!
This year's swag is a custom designed notebook and custom Umbraco Hacktoberfest sticker:

image

As an alternative choice, you can opt-out of receiving anything and ask us to help improve the planet instead by planting a tree on your behalf. 🌳

Receive your swag or plant a tree! 👈 Please follow this link to fill out and submit the form, before December 25nd, 2024, 23:59:00 UTC.

Following this date we'll be sending out all the swag, but please note that it might not reach your doorstep for a few weeks/months, so please bear with us and be patient 🙏

The only thing left to say is thank you so much for participating in Hacktoberfest! We really appreciate the help!

Kind regards,
The various Umbraco teams.

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

Successfully merging this pull request may close these issues.

5 participants