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

Add TargetFramework awareness to NuGet detector #1266

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Oct 8, 2024

This PR does 3 things.

  1. Adds TargetFramework to NuGet package references. This can be useful when querying component data to understand if components are used in a place where a vulnerability applies.
  2. Adds framework package handling. The .NET SDK will do conflict resolution and drop assets from packages that overlap with the framework. NuGet is planning to do the same Support "Supplied by Platform" scenario in restore NuGet/Home#7344 but until then, it's beneficial to have component detection duplicate some of this logic. When a package is identified as overlapping with the framework we'll treat it as a Development Dependency so that it might be auto-dismissed.
  • .NETFramework projects do not get this - .NETFramework does not participate in conflict resolution by default. Even when enabled framework assemblies can be bypassed using bindingRedirects, or avoiding references to them. Due this fragility it's not safe to apply framework package rules to .NETFramework.
  • packages.config usage is also excluded since it precludes SDK conflict resolution (and is also only used on .NETFramework projects).
  1. Recognizes ExcludeAssets="Runtime" usage as a Development Dependencies.

A couple notes:

  1. I reused Development Dependency rather than plumbing a new concept.
  2. I only mapped data for the Microsoft.NETCore.App - the default shared framework. We could consider doing the same for Microsoft.ASPNETCore.App and Microsoft.WindowsDesktop.App but we'd need to plumb the reference information out of the assets file - currently that's not read and I'm not aware of a supported NuGet API for reading it (though it is present under project/frameworks/<framework>/frameworkReferences/<name>
  3. I've tested this manually on a simple project and a more complex repository. It's doing what I expect and I see the targetFramework information added to the component in the ScanManifest. I noticed that development dependencies are labeled as such if all references to the component are development dependencies. If any reference is not a development dependency then the component is not labeled as a development dependency.
  4. I need to add automated tests for this.
  5. I added the tool findLastPackage that I used to generate the sources for the framework lists - but this is just for reference. I don't intend to check this tool in.
  6. Maybe we could even do more with CG here to mark libaries as development dependencies when all their runtime assets are excluded. This technique is common for plugins and excluding them in this way would be beneficial for folks even when not in a framework dependency case. I'll add this.

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 92.50457% with 41 lines in your changes missing coverage. Please review.

Project coverage is 89.0%. Comparing base (402a932) to head (5b5ac57).

Files with missing lines Patch % Lines
...ctors/nuget/FrameworkPackages/FrameworkPackages.cs 42.4% 37 Missing and 1 partial ⚠️
...uGetProjectModelProjectCentricComponentDetector.cs 83.3% 0 Missing and 2 partials ⚠️
...ion.Detectors/nuget/NuGetPackagesConfigDetector.cs 92.8% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1266     +/-   ##
=======================================
- Coverage   89.1%   89.0%   -0.1%     
=======================================
  Files        361     374     +13     
  Lines      27742   28042    +300     
  Branches    1757    1758      +1     
=======================================
+ Hits       24720   24983    +263     
- Misses      2643    2678     +35     
- Partials     379     381      +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Successfully merging this pull request may close these issues.

2 participants