Enable NuGet Audit and fix issues #107639

ericstj · 2024-09-10T18:54:46Z

Microsoft.NET.HostModel can reference the live builds of the packages it depends on. These will be deployed by the SDK.

Most other audit alerts were due to tasks pulling in old dependencies that aren't even used by the task. Avoid these by cherry-picking just the assemblies needed by the tasks and provided by MSBuild / SDK. This prevents NuGet from downloading the package closure with the vulnerable packages. We don't need those packages since the tasks aren't responsible for deploying them. A better solution in the future would be a targeting pack for MSBuild and the .NET SDK - so that components that contribute to these hosts have a surface area they can target without taking on responsibility for servicing.

There is once case where we have a test that references NuGet.* packages which also bring in stale dependencies that overlap with framework assemblies. Avoid these by cherry-picking the NuGet packages in the same way.

Keeping this draft as I want to test some more and add some docs / comments - but sharing this now since a lot of folks were working on this and I wanted to share the techniques I'm using.

Microsoft.NET.HostModel can reference the live builds of the packages it depends on. These will be deployed by the SDK.� Most other audit alerts were due to tasks pulling in old dependencies that aren't even used by the task. Avoid these by cherry-picking just the assemblies needed by the tasks and provided by MSBuild / SDK. This prevents NuGet from downloading the package closure with the vulnerable packages. We don't need those packages since the tasks aren't responsible for deploying them. A better solution in the future would be a targeting pack for MSBuild and the .NET SDK - so that components that contribute to these hosts have a surface area they can target without taking on responsibility for servicing. There is once case where we have a test that references NuGet.* packages which also bring in stale dependencies that overlap with framework assemblies. Avoid these by cherry-picking the NuGet packages in the same way.

dotnet-policy-service · 2024-09-10T18:55:50Z

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

eng/PackageDownloadAndReference.targets

jkoritzinsky · 2024-09-10T19:10:10Z

src/installer/managed/Microsoft.NET.HostModel/Microsoft.NET.HostModel.csproj

+    <ProjectReference Include="$(LibrariesProjectRoot)System.Reflection.Metadata\src\System.Reflection.Metadata.csproj" />
+    <ProjectReference Include="$(LibrariesProjectRoot)System.Text.Json\src\System.Text.Json.csproj" />


These references need to not move newer than MSBuild as this library is consumed by MSBuild tasks in the SDK (including for the .NET Framework build of the SDK targets). We can't reference live builds as a result.

The SDK is already on the live version of System.Text.Json for its tasks, but it seems it is not on the live version of System.Reflection.Metadata. We have another library (Microsoft.Extensions.DependencyModel) that SDK consumes with live JSON as well. I only needed to update to live STJ to fix the audit warning so I could undo the SRM change, but it feels wrong.

I'll file a follow up issue in the SDK for that. dotnet/sdk#43325

The SDK is already on the live version of System.Text.Json for its tasks

Would you mind elaborating? Last time I looked, NETFramework tasks in the SDK were using an 8.0.x version of STJ because MSBuild's binding redirects required that.

The SDK tasks themselves ('Microsoft.NET.Build.Tasks.dll`) reference the latest. Because it's an exact match they don't need the redirects. Other assemblies loaded by the SDK (NuGet, for example) do reference lower versions, and they rely on MSBuild's redirects, but they don't exchange types with the SDK tasks. The SDK also ships the latest STJ along side its tasks.
For example:

"C:\Program Files\dotnet\sdk\9.0.100-preview.7.24407.12\Sdks\Microsoft.NET.Sdk\tools\net472\System.Text.Json.dll" => 9.0.0.0 "C:\Program Files\dotnet\sdk\8.0.400\Sdks\Microsoft.NET.Sdk\tools\net472\System.Text.Json.dll" => 8.0.0.4

The SDK's MSBuild SDK resolver does need to stay on the VS copy of STJ because it doesn't carry a copy next to itself.
CC @marcpopMSFT

SDK pins S.R.M and a few others, so don't make them upgrade yet.

jaredpar · 2024-09-10T21:10:21Z

eng/PackageDownloadAndReference.targets

+    <PackageDownload Include="@(PackageDownloadAndReference)" />
+    <PackageDownload Update="@(PackageDownloadAndReference)" Version="[%(Version)]"/>
+    <PackageDownloadAndReference Update="@(PackageDownloadAndReference)" PackageFolder="$([System.String]::new(%(Identity)).ToLowerInvariant())" />
+    <Reference Include="@(PackageDownloadAndReference->'$(NuGetPackageRoot)%(PackageFolder)/%(Version)/%(Folder)/%(AssemblyName).dll')" />


Won't this approach break the ability to use CPM and transitive pinning?

This doesn't pull any transitive dependencies at all. That's the point. It's a stand-in for a targeting pack. We pull down just single package and use it only for reference. It's very manual, but it avoids accidentally lifting dependencies that aren't in the control of the plugin (or pulling down dependencies just to drop them in favor of a framework library). It should be used sparingly as it is bypassing nuget resolution. It also doesn't persist into a packed project, so it shouldn't be used in "normal" libraries. It's for tasks, analyzers, and private assemblies intended for use in similar circumstances.

Sure but this means it's a different kind of package reference that you can't manage with CPM correct?

We'd just use the same properties when setting these that we'd feed into CPM. It's a temporary solution. We need an official package or SDK from MSBuild (and roslyn) for targeting hosts.

Nonetheless I added a note to NuGet/Home#8476 that would improve PackageDownload if it didn't need to mention versions.

If you felt strongly about making this hack honor CPM I could add that functionality by merging the version defined in @(PackageVersion) when not specified.

Consolidate representation of msbuild-provided task dependencies

Feedback addressed

src/tasks/WasmAppBuilder/WasmAppBuilder.csproj

eng/Versions.props

…uGetAudit

This ensures projects referenced will not be rebuilt by tests. This also means the HostModel package will not list these as references, but that's OK since the SDK provides them and this is not a shipping package.

On .NETCore we want to use the targeting pack and avoid rebuilding libs.

jkoritzinsky

One comment, but otherwise LGTM if everything builds.

...eropServices.JavaScript/tests/JSImportGenerator.UnitTest/JSImportGenerator.Unit.Tests.csproj

…JSImportGenerator.UnitTest/JSImportGenerator.Unit.Tests.csproj Co-authored-by: Jeremy Koritzinsky <[email protected]>

* Enable NuGet Audit and fix issues Microsoft.NET.HostModel can reference the live builds of the packages it depends on. These will be deployed by the SDK.� Most other audit alerts were due to tasks pulling in old dependencies that aren't even used by the task. Avoid these by cherry-picking just the assemblies needed by the tasks and provided by MSBuild / SDK. This prevents NuGet from downloading the package closure with the vulnerable packages. We don't need those packages since the tasks aren't responsible for deploying them. A better solution in the future would be a targeting pack for MSBuild and the .NET SDK - so that components that contribute to these hosts have a surface area they can target without taking on responsibility for servicing. There is once case where we have a test that references NuGet.* packages which also bring in stale dependencies that overlap with framework assemblies. Avoid these by cherry-picking the NuGet packages in the same way. * Fix package path on linux * Only use live JSON from HostModel SDK pins S.R.M and a few others, so don't make them upgrade yet. * Add a couple missing assembly references * Refactor tasks dependencies Consolidate representation of msbuild-provided task dependencies * Fix audit warnings in tests * Remove MetadataLoadContext from WasmAppBuilder package * Update Analyzer.Testing packages * Reduce exposure of Microsoft.Build.Tasks.Core * Fix audit warnings that only occur on browser * Update Asn1 used by linker analyzer tests * React to breaking change in analyzer test SDK * Enable working DryIoc tests * Fix double-write when LibrariesConfiguration differs from Configuration * Fix LibrariesConfiguration update target * Clean up references and add comments. * Make HostModel references private This ensures projects referenced will not be rebuilt by tests. This also means the HostModel package will not list these as references, but that's OK since the SDK provides them and this is not a shipping package. * Use ProjectReferenceExclusion to avoid framework project references On .NETCore we want to use the targeting pack and avoid rebuilding libs. * Update src/libraries/System.Runtime.InteropServices.JavaScript/tests/JSImportGenerator.UnitTest/JSImportGenerator.Unit.Tests.csproj Co-authored-by: Jeremy Koritzinsky <[email protected]> --------- Co-authored-by: Jeremy Koritzinsky <[email protected]>

* Enable NuGet Audit and fix issues (#107639) * Enable NuGet Audit and fix issues Microsoft.NET.HostModel can reference the live builds of the packages it depends on. These will be deployed by the SDK.� Most other audit alerts were due to tasks pulling in old dependencies that aren't even used by the task. Avoid these by cherry-picking just the assemblies needed by the tasks and provided by MSBuild / SDK. This prevents NuGet from downloading the package closure with the vulnerable packages. We don't need those packages since the tasks aren't responsible for deploying them. A better solution in the future would be a targeting pack for MSBuild and the .NET SDK - so that components that contribute to these hosts have a surface area they can target without taking on responsibility for servicing. There is once case where we have a test that references NuGet.* packages which also bring in stale dependencies that overlap with framework assemblies. Avoid these by cherry-picking the NuGet packages in the same way. * Fix package path on linux * Only use live JSON from HostModel SDK pins S.R.M and a few others, so don't make them upgrade yet. * Add a couple missing assembly references * Refactor tasks dependencies Consolidate representation of msbuild-provided task dependencies * Fix audit warnings in tests * Remove MetadataLoadContext from WasmAppBuilder package * Update Analyzer.Testing packages * Reduce exposure of Microsoft.Build.Tasks.Core * Fix audit warnings that only occur on browser * Update Asn1 used by linker analyzer tests * React to breaking change in analyzer test SDK * Enable working DryIoc tests * Fix double-write when LibrariesConfiguration differs from Configuration * Fix LibrariesConfiguration update target * Clean up references and add comments. * Make HostModel references private This ensures projects referenced will not be rebuilt by tests. This also means the HostModel package will not list these as references, but that's OK since the SDK provides them and this is not a shipping package. * Use ProjectReferenceExclusion to avoid framework project references On .NETCore we want to use the targeting pack and avoid rebuilding libs. * Update src/libraries/System.Runtime.InteropServices.JavaScript/tests/JSImportGenerator.UnitTest/JSImportGenerator.Unit.Tests.csproj Co-authored-by: Jeremy Koritzinsky <[email protected]> --------- Co-authored-by: Jeremy Koritzinsky <[email protected]> * Remove live System.Text.Json reference from HostModel (#108263) * Reduce changes to src/installer Since we're no longer trying to reference live S.T.J we don't need these. * Update JSON toolset version * Don't error for NuGet audit on non-official builds (#108718) * Reference live S.T.JSON from DI.ExternalContainers.Tests * Update STJ in Wasm.Build.Tests * Make SystemTextJsonToolsetVersion 8.0.4 We cannot count on VS and MSBuild updating by the time 9.0 ships GA. Fix WASM projects which only target .NET by referencing the LKG and dropping all assets. For Microsoft.NET.HostModel and other build tasks, keep them on the version we can garuntee is present in VS. NoWarn the Audit warnings here. This is safe because we can ensure one of two things. 1. The package is non-shipping and customers won't see the warning and the referencing repo in the product will ensure an update or exclusion of the dependency. (HostModel) 2. The project excludes the reference entirely as making it PrivateAssets (not in package) and ExcludeAssets=runtime (no possibility of using runtime). * Fix STJ audit warning in installer tests --------- Co-authored-by: Jeremy Koritzinsky <[email protected]>

ericstj requested review from jaredpar, carlossanlop, jkoritzinsky, rainersigwald, zivkan and mmitche September 10, 2024 18:54

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 10, 2024

dotnet-policy-service bot assigned ericstj Sep 10, 2024

ericstj added area-Infrastructure-libraries and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 10, 2024

ericstj commented Sep 10, 2024

View reviewed changes

eng/PackageDownloadAndReference.targets Outdated Show resolved Hide resolved

jkoritzinsky previously requested changes Sep 10, 2024

View reviewed changes

ericstj added 3 commits September 10, 2024 13:26

Fix package path on linux

8bdf2f3

Only use live JSON from HostModel

8c96173

SDK pins S.R.M and a few others, so don't make them upgrade yet.

Add a couple missing assembly references

69e424c

ericstj requested a review from jkoritzinsky September 10, 2024 20:28

jaredpar reviewed Sep 10, 2024

View reviewed changes

ericstj added 2 commits September 10, 2024 17:58

Refactor tasks dependencies

6408e8b

Consolidate representation of msbuild-provided task dependencies

Fix audit warnings in tests

294f280

This was referenced Sep 11, 2024

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

ericstj mentioned this pull request Sep 11, 2024

Enable NuGet Audit in .NET repositories dotnet/arcade#15019

Open

28 tasks

ericstj commented Sep 11, 2024

View reviewed changes

src/tasks/WasmAppBuilder/WasmAppBuilder.csproj Show resolved Hide resolved

Remove MetadataLoadContext from WasmAppBuilder package

4453c4f

ericstj commented Sep 12, 2024

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

ericstj added 2 commits September 12, 2024 07:14

Update Analyzer.Testing packages

5a22046

Reduce exposure of Microsoft.Build.Tasks.Core

fe775ec

ericstj added 2 commits September 19, 2024 15:58

Merge branch 'main' of https://github.com/dotnet/runtime into enableN…

b365311

…uGetAudit

Fix LibrariesConfiguration update target

04ffd0e

build-analysis bot mentioned this pull request Sep 20, 2024

'chrome-DebuggerTests.ExceptionTests' timing out #106376

Open

ericstj mentioned this pull request Sep 20, 2024

WasmTemplateTests.RunWithDifferentAppBundleLocations timeout #108074

Closed

This was referenced Sep 20, 2024

GetAsync_ServerNeedsAuthAndNoCredential_StatusCodeUnauthorized got cancelled #108019

Open

System.Data.Common.Tests Assert failure on Linx x64 CI test run #108070

Open

ericstj mentioned this pull request Sep 20, 2024

chrome-DebuggerTests timing out #108078

Open

lewing mentioned this pull request Sep 22, 2024

failure in Wasm.Build.Tests.WasmTemplateTests.RunWithDifferentAppBundleLocations #108107

Open

ericstj added 2 commits September 23, 2024 11:11

Clean up references and add comments.

72cedfe

Make HostModel references private

3772ac9

This ensures projects referenced will not be rebuilt by tests. This also means the HostModel package will not list these as references, but that's OK since the SDK provides them and this is not a shipping package.

ericstj marked this pull request as ready for review September 23, 2024 19:10

ericstj requested review from marek-safar, sbomer, maraf, akoeplinger, lewing and pavelsavara as code owners September 23, 2024 19:10

Use ProjectReferenceExclusion to avoid framework project references

d93f35c

On .NETCore we want to use the targeting pack and avoid rebuilding libs.

build-analysis bot mentioned this pull request Sep 24, 2024

[9.0] Timeout - [Long Running Test] 'Wasm.Build.Tests.TestAppScenarios.LazyLoadingTests.LoadLazyAssemblyBeforeItIsNeeded #107897

Open

jkoritzinsky approved these changes Sep 24, 2024

View reviewed changes

...eropServices.JavaScript/tests/JSImportGenerator.UnitTest/JSImportGenerator.Unit.Tests.csproj Outdated Show resolved Hide resolved

Update src/libraries/System.Runtime.InteropServices.JavaScript/tests/…

3da813d

…JSImportGenerator.UnitTest/JSImportGenerator.Unit.Tests.csproj Co-authored-by: Jeremy Koritzinsky <[email protected]>

This was referenced Sep 25, 2024

ProcessThreadTests.TestStartTimeProperty failure in CI #105526

Open

[Wasm][Test Failure] LoadLazyAssemblyBeforeItIsNeeded(lazyLoadingTestExtension: "wasm", allLazyLoadingTestExtensions: ["wasm", "dll", "NoExtension"]) #106160

Open

ericstj merged commit df76a01 into dotnet:main Sep 25, 2024
158 of 164 checks passed

ericstj mentioned this pull request Oct 16, 2024

[CG] Bump Microsoft.IO.Redist to 6.0.1 #106102

Closed

steveharter mentioned this pull request Oct 25, 2024

Add DI specification test for ensuring externally provided instance services are not disposed on provider disposal. #104853

Closed

github-actions bot locked and limited conversation to collaborators Oct 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable NuGet Audit and fix issues #107639

Enable NuGet Audit and fix issues #107639

ericstj commented Sep 10, 2024

dotnet-policy-service bot commented Sep 10, 2024

jkoritzinsky Sep 10, 2024

ericstj Sep 10, 2024 •

edited

Loading

ViktorHofer Sep 14, 2024

ericstj Sep 18, 2024 •

edited

Loading

jaredpar Sep 10, 2024

ericstj Sep 11, 2024 •

edited

Loading

jaredpar Sep 12, 2024

ericstj Sep 12, 2024 •

edited

Loading

jkoritzinsky left a comment

		<ProjectReference Include="$(LibrariesProjectRoot)System.Reflection.Metadata\src\System.Reflection.Metadata.csproj" />
		<ProjectReference Include="$(LibrariesProjectRoot)System.Text.Json\src\System.Text.Json.csproj" />

Enable NuGet Audit and fix issues #107639

Enable NuGet Audit and fix issues #107639

Conversation

ericstj commented Sep 10, 2024

dotnet-policy-service bot commented Sep 10, 2024

jkoritzinsky Sep 10, 2024

Choose a reason for hiding this comment

ericstj Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

ViktorHofer Sep 14, 2024

Choose a reason for hiding this comment

ericstj Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

jaredpar Sep 10, 2024

Choose a reason for hiding this comment

ericstj Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

jaredpar Sep 12, 2024

Choose a reason for hiding this comment

ericstj Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

jkoritzinsky left a comment

Choose a reason for hiding this comment

ericstj Sep 10, 2024 •

edited

Loading

ericstj Sep 18, 2024 •

edited

Loading

ericstj Sep 11, 2024 •

edited

Loading

ericstj Sep 12, 2024 •

edited

Loading