Enable NuGet Audit and fix issues #107639
Conversation
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.
ericstj
requested review from
jaredpar,
carlossanlop,
jkoritzinsky,
rainersigwald,
zivkan and
mmitche
dotnet-issue-labeler
bot
added
the
needs-area-label
ericstj
added
area-Infrastructure-libraries
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries |
| <ProjectReference Include="$(LibrariesProjectRoot)System.Reflection.Metadata\src\System.Reflection.Metadata.csproj" /> | ||
| <ProjectReference Include="$(LibrariesProjectRoot)System.Text.Json\src\System.Text.Json.csproj" /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| <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')" /> |
There was a problem hiding this comment.
Won't this approach break the ability to use CPM and transitive pinning?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure but this means it's a different kind of package reference that you can't manage with CPM correct?
There was a problem hiding this comment.
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
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
requested review from
marek-safar,
sbomer,
maraf,
akoeplinger,
lewing and
pavelsavara
as code owners
On .NETCore we want to use the targeting pack and avoid rebuilding libs.
...eropServices.JavaScript/tests/JSImportGenerator.UnitTest/JSImportGenerator.Unit.Tests.csproj
Outdated
Show resolved
Hide resolved
…JSImportGenerator.UnitTest/JSImportGenerator.Unit.Tests.csproj Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
* 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 <jkoritzinsky@gmail.com> --------- Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
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.