Skip to content

Conversation

@joperezr
Copy link
Member

@joperezr joperezr commented Oct 14, 2024

Description

Enabling NuGet Audit so that our build can catch issues when one of our dependencies is marked as vulnerable, so we are able to pin to a higher version in order to lift the dependency out of the vulnerability.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@ViktorHofer
Copy link
Member

Contributes to dotnet/arcade#15019

@joperezr we recommend also setting the following WarningsNotAsErrors property so that local dev builds / PRs don't stop working on a Patch Tuesday: https://github.com/dotnet/arcade/blob/964e434191bd3ca5675743d08ff742ec0f1e79a9/Directory.Build.props#L17-L18

It's important to keep this enabled for official builds though.

<DashboardPublishedArtifactsOutputDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsDir)', 'DashboardArtifacts', '$(Configuration)'))</DashboardPublishedArtifactsOutputDir>
<WorkloadsPackageSource>$(ArtifactsShippingPackagesDir)</WorkloadsPackageSource>
<!-- Only upgrade NuGetAudit warnings to errors for official builds. -->
<WarningsNotAsErrors Condition="'$(OfficialBuild)' != 'true'">$(WarningsNotAsErrors);NU1901;NU1902;NU1903;NU1904</WarningsNotAsErrors>
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a way to alert the team, or a subset of the team, when an OfficialBuild fails?

Maybe this shouldn't be based on OfficialBuild, but instead in our rolling CI build? For sure I don't want PRs to start failing on the day a security release comes out.

Copy link
Member

Choose a reason for hiding this comment

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

Along those lines.... this PR is failing due to NU1903

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@ViktorHofer ViktorHofer Oct 15, 2024

Choose a reason for hiding this comment

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

The workaround doesn't work. I would hold this PR off for a few days until we reach consensus on how to react to the msbuild bug.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like understanding why WarningsNotAsErrors doesn't correctly work with solutions will take longer. If things work for you I would not block this PR on it. You always have the option to turn NuGet Audit on/off if something doesn't work. You can NoWarn individual nuget warnings, suppress specific vulerability reports and/or disable NuGet Audit entirely.

As this feature is enabled by default for our customers starting with .NET 8.0.2xx and .NET 9, please give this a try and give feedback back to the NuGet team and me/us in the Arcade issue.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should merge this change until it doesn't fail PRs. Getting all our PRs (and local builds) broken when someone marks a NuGet package as vulnerable isn't a good situation to be in.

@eerhardt
Copy link
Member

eerhardt commented Oct 15, 2024

FYI - #6319 should fix the current failure in this PR - transitively referencing Microsoft.Extensions.Caching.Memory 8.0.0. I've pushed that chagne to this branch as well.

@joperezr joperezr added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 17, 2024
@danmoseley
Copy link
Member

@joperezr is this still a project or should we close

@github-actions github-actions bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Mar 10, 2025
@joperezr
Copy link
Member Author

We should at some point enable this, but it is not clear when, so I'll go ahead and close for now and we can reopen when ready.

@joperezr joperezr closed this Mar 18, 2025
@joperezr joperezr deleted the NuGetAudit branch April 1, 2025 17:43
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-codeflow for labeling automated codeflow. intentionally a different color! NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants