Verify project template dependencies

[halter73]

Some of our project templates transitively reference vulnerable NuGet packages. This PR adds automated testing to verify restoring NuGet packages for our project templates does not produce any vulnerability warnings.

This is complicated by the fact that we do not restore packages from nuget.org to avoid supply chain attacks, but the vulnerability warnings only appear if you use nuget.org as a package source. To work around this, I updated the RunDotNetNewAsync method used by most of our project template tests to call dotnet list package --vulnerable --include-transitive --source https://api.nuget.org/v3/index.json

I updated the templates that I already know produce vulnerability warnings to directly reference the vulnerable packages and require up-to-date versions of them. I also made some updates to make it easier to run our template tests locally.

I'm leaving this as a draft for now since I want to confirm that it will be necessary to directly reference these packages. Some of these references, like the System.Text.Json ones, shouldn't be necessary because the version should already be hoisted to 9.* by the net9.0 shared runtime, but dotnet restore, dotnet nuget why and dotnet list package --vulnerable --include-transitive don't appear to realize this. However, this wasn't a problem until recently because Microsoft.Extensions.DependencyModel took a new enough NuGet dependency until dotnet/runtime#106172 changed that for the net9.0 target. I plan to follow up with the SDK team about this.

I'm also looking into whether Microsoft.EntityFrameworkCore.SqlServer can update its Microsoft.Data.SqlClient dependency to 5.2.2 (which was just released a few hours ago) before .NET 9 ships. @dotnet/efteam Do you know if this is something you're already planning?

I tried to encourage efcore to update its Microsoft.CodeAnalysis dependency and thereby its System.Drawing.Common dependency by opening dotnet/efcore#34116, but this caused some test failures, and there doesn't appear to be interest in updating this dependency for .NET 9. I think this means we'll be forced to at least add a direct System.Drawing.Common dependency to some of our templates.

[halter73]

@javiercn Do you have an idea why this doesn't work when running tests in VS?

I saw that the GenerateContent target has basically the same inputs and outputs as this, so this is probably redundant anyway. I just wanted to confirm that configurating DisableFastUpToDateCheck is really the only way to get VS to automatically rebuild this project when running a test after changing a .csproj.in file.

[javiercn]

It's the safest way.

We could take a look together if you want and figure out the details. I recently had to implement this for other reasons, so I'm relatively familiar with how it works.

[roji]

I'm also looking into whether Microsoft.EntityFrameworkCore.SqlServer can update its Microsoft.Data.SqlClient dependency to 5.2.2 (which was just released a few hours ago) before .NET 9 ships. @dotnet/efteam Do you know if this is something you're already planning?

@halter73 EF will definitely be bumping its version of Microsoft.Data.SqlClient - not to 5.2.2 (which isn't LTS and so would run out of support before EF 9.0 does), but to 5.1.6, which also shouldn't have the security issue. PR dotnet/efcore#34540 is out and should get merged for rc2.

I tried to encourage efcore to update its Microsoft.CodeAnalysis dependency and thereby its System.Drawing.Common dependency by opening dotnet/efcore#34116, but this caused some test failures, and there doesn't appear to be interest in updating this dependency for .NET 9. I think this means we'll be forced to at least add a direct System.Drawing.Common dependency to some of our templates.

Is the System.Drawing.Common dependency causing issues, even though it's not a runtime dependency (only referenced from an analyzer)? If so (would be interested in some context), we could also also add a direct dependency to a newer version in EF itself, rather than it being done in templates...

[roji]

/cc @ericstj

[ericstj]

It seems like there would be a product bug if an EF package has a dependency on CodeAnalysis that is visible to ASP.NET Core. EF should just be referencing CodeAnalysis for its surface area and hiding that dependency from any of its packages (since it doesn't participate in the library dependency graph). I chatted with @roji and mentioned it should be safe to add a drawing dependency and hide it in a similar way. It doesn't impact what ships to customers and it's safer since the tools that validate dependencies won't see the vulnerable reference.

The fact that ASPNETCore is able to observe the CodeAnalysis dependency though makes we think there might be another bug to fix. @halter73 can you share the dotnet nuget why output for System.Drawing.Common?

[roji]

I double-checked, and am noting that CodeAnalysis is also referenced from Microsoft.EntityFrameworkCore.Design, which is another non-runtime package that is require for executing various EF CLI commands (dotnet ef), e.g. for executing migrations (CodeAnalysis is required there because one of the CLI commands analyzes the user project like an analyzer, for NativeAOT). I'm guessing the templates also include Microsoft.EntityFrameworkCore.Design so that users can e.g. manage migrations, so that would be a problem via that path as well.

[ericstj]

Is Microsoft.EntityFrameworkCore.Design meant to be used in a way that it brings CodeAnalysis along with it and is responsible for deploying it? That feels a bit unusual. Typically these sort of design-time dependencies should not be represented as package dependencies that would leak into the user's application / output directory.

[AndriySvyryd]

In typical applications Microsoft.EntityFrameworkCore.Design and Microsoft.EntityFrameworkCore.Tools should only be referenced as design-time dependencies.

[ericstj]

In typical applications Microsoft.EntityFrameworkCore.Design and Microsoft.EntityFrameworkCore.Tools should only be referenced as design-time dependencies.

I'm not sure what that means. I see a mix a references that look an awful lot like normal references from regular applications: https://github.com/search?q=org%3Adotnet+%2FPackageReference.*Include%3D%22Microsoft.EntityFrameworkCore.Design%22%2F&type=code

Even for those that use PrivateAssets that doesn't prevent the dependency closure of the package from being dumped in the app/library and passed to the compiler. If you want this to truly be something used by a design time tool it shouldn't be packaged like a library.

EDIT: it does look like EF avoids passing those to the compiler (by omitting Compile from IncludeAssets), but they still get restored and copied to the output directory. If I add that package to an app project I see 26 MB of files (including the compiler libs) get copied to my app's output directory.

[AndriySvyryd]

We allow Microsoft.EntityFrameworkCore.Design to be referenced normally and deployed with the application if certain operations like migrations management need to be performed at runtime. I am just saying that's not a common scenario.

[ericstj]

We allow Microsoft.EntityFrameworkCore.Design to be referenced normally and deployed with the application if certain operations like migrations management need to be performed at runtime. I am just saying that's not a common scenario.

I'm not sure I see a way to use this package at all that doesn't result in the compiler being copied to the output directory.
Even when the package is referenced like this:

    <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="8.0.8">
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>

Which is the default for add package, the compiler is still copied to the output directory.

So even if that's uncommon, I think the support for it is leaking into all uses. Also if I can trust the what I'm seeing from the simple search of dotnet repos, most are referencing in the uncommon way as a regular PackageReference without include/private.

IMO we should change this package to optimize for the common scenario rather than the uncommon one.

[halter73]

can you share the dotnet nuget why output for System.Drawing.Common?

PS C:\dev\temp\MVCWebApplication\MVCWebApplication> dotnet nuget why .\MVCWebApplication.csproj System.Drawing.Common
Project 'MVCWebApplication' has the following dependency graph(s) for 'System.Drawing.Common':

  [net9.0]
   │
   └─ Microsoft.EntityFrameworkCore.Tools (v9.0.0-preview.7.24405.3)
      └─ Microsoft.EntityFrameworkCore.Design (v9.0.0-preview.7.24405.3)
         └─ Microsoft.CodeAnalysis.Workspaces.MSBuild (v4.8.0)
            └─ Microsoft.Build.Framework (v16.10.0)
               └─ System.Security.Permissions (v4.7.0)
                  └─ System.Windows.Extensions (v4.7.0)
                     └─ System.Drawing.Common (v4.7.0)

[AndriySvyryd]

Which is the default for add package, the compiler is still copied to the output directory.

What is the issue with the dependencies being present in the output directory if they aren't referenced from the app?

IMO we should change this package to optimize for the common scenario rather than the uncommon one.

Even for the common scenario there are at least 3 design-time consumers of Microsoft.EntityFrameworkCore.Design:

• Microsoft.EntityFrameworkCore.Tools - PS commands loaded in PMC
• Microsoft.EntityFrameworkCore.Tasks - An MSBuild task
• dotnet-ef - A dotnet tool

All of them need to load Microsoft.EntityFrameworkCore.Design dynamically and the output directory is the most reliable place to find it.

[ericstj]

What is the issue with the dependencies being present in the output directory if they aren't referenced from the app?

You're taking on a very large dependency set that is causing people to need to update packages, for dependencies they don't even use. You may also be bloating the app significantly if no-one actually uses Microsoft.CodeAnalysis at runtime.

Even for the common scenario there are at least 3 design-time consumers of Microsoft.EntityFrameworkCore.Design:
Microsoft.EntityFrameworkCore.Tools - PS commands loaded in PMC
Microsoft.EntityFrameworkCore.Tasks - An MSBuild task
dotnet-ef - A dotnet tool
All of them need to load Microsoft.EntityFrameworkCore.Design dynamically and the output directory is the most reliable place to find it.

Right, I don't disagree with you having a package that puts Microsoft.EntityFrameworkCore.Design.dll in the output directory. I'm questioning if all these scenarios require a reference to the Microsoft.EntityFrameworkCore.Design to bring with it package references to the Compiler and it's closure. What if you just omitted all your references by using PrivateAssets="all" and you made your consumers responsible for bringing those in the cases they need them? That's what happens for things like MSBuild tasks and analyzers which are also "design time" components. They don't bring compiler / MSBuild references with them - they expect them to be provided.

[AndriySvyryd]

Right, I don't disagree with you having a package that puts Microsoft.EntityFrameworkCore.Design.dll in the output directory. I'm questioning if all these scenarios require a reference to the Microsoft.EntityFrameworkCore.Design to bring with it package references to the Compiler and it's closure. What if you just omitted all your references by using PrivateAssets="all" and you made your consumers responsible for bringing those in the cases they need them? That's what happens for things like MSBuild tasks and analyzers which are also "design time" components. They don't bring compiler / MSBuild references with them - they expect them to be provided.

In many cases they would still be needed when running the tools. Seems like it would just add friction with the same result.

[AndriySvyryd]

In general, while this type of a swiss-knife project is a good place to start as everything is readily available we recommend that the users move their migrations to a separate project. This migrations project wouldn't be deployed with the app and only used at design time (or deploy time) and would be the project that references Microsoft.EntityFrameworkCore.Tools and/or Microsoft.EntityFrameworkCore.Design

[roji]

@ericstj I understand the concerns about including the compiler in our Design package... But at least at the moment (for 9.0) it is what it is, I think. However, we still need to decide whether:

1. EF itself takes a dependency on a newer System.Drawing.Common (from the Design and Analyzers packages)
2. The templates here do that, as @halter73 proposed above
3. Nobody does it, in which case all EF users get a security warning out of the box

[ericstj]

For the analyzers package you should upgrade and keep the dependency private. It's already that way. It doesn't leak to your users: https://github.com/dotnet/efcore/blob/f0965d6e88ac88ee235deef21804e64bbc62decf/src/EFCore.Analyzers/EFCore.Analyzers.csproj#L32-L33

For the designer package I do think you should rethink your dependencies for the next version. Seems to me more of those should be private given the little I've learned about the customers for this package.

For the short term - can EF update the version of Microsoft.Build that it's pulling in?

From above (thanks @halter73!) I see:

  [net9.0]
   │
   └─ Microsoft.EntityFrameworkCore.Tools (v9.0.0-preview.7.24405.3)
      └─ Microsoft.EntityFrameworkCore.Design (v9.0.0-preview.7.24405.3)
         └─ Microsoft.CodeAnalysis.Workspaces.MSBuild (v4.8.0)
            └─ Microsoft.Build.Framework (v16.10.0)
               └─ System.Security.Permissions (v4.7.0)
                  └─ System.Windows.Extensions (v4.7.0)
                     └─ System.Drawing.Common (v4.7.0)

It looks like the version of CodeAnalysis is 4.8 which can't move due to VS support and Roslyn hasn't provided a patched version to resolve these. Microsoft.Build.Framework looks rather old (16.10) and I think you could update to 17.8.3 and still maintain the same VS support (17.8) while getting greatly reduced dependencies.

Take care when adding this reference to match the characteristics it has from Microsoft.CodeAnalysis.Workspaces.MSBuild:
https://github.com/dotnet/roslyn/blob/e0d0febde64cfe4cece7c1ea0654741b4efa09d7/src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj#L19-L20

So you can have EF.Design add

    <PackageReference Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" Version="$(MicrosoftCodeAnalysisVersion)" />
    <!-- Upgrade the version of Microsoft.Build.Framework referenced by Microsoft.CodeAnalysis.Workspaces.MSBuild -->
    <PackageReference Include="Microsoft.Build.Framework" Version="17.8.3" ExcludeAssets="Runtime" />

PS: with CPM that is not hard at all.

@rainersigwald @marcpopMSFT @jaredpar

[AndriySvyryd]

Microsoft.Build.Framework looks rather old (16.10) and I think you could update to 17.8.3 and still maintain the same VS support (17.8) while getting greatly reduced dependencies.

It's more sustainable for the transitive dependencies to be updated by the packages that directly depend on them. Also, at this point of the release it would be very risky for us to upgrade to a new major version of a dependency. For the scenarios that would be potentially impacted by this change in particular we don't have any automated tests (dotnet/efcore#33136)

[ericstj]

It's more sustainable for the transitive dependencies to be updated by the packages that directly depend on them.

So here that would mean you'd need an update to Microsoft.CodeAnalysis.Workspaces.MSBuild to a 4.8.1 version since that's the component that hasn't taken a newer version but you're asserting you need to stay on due to VS compat. You'd need to ask them to make an update, and they'd need to work out with MSBuild if it was necessary to build a 16.10.1 or if they themselves could take the update to 17.8.3.

at this point of the release it would be very risky for us to upgrade to a new major version of a dependency. For the scenarios that would be potentially impacted by this change in particular we don't have any automated tests

You could probably diff your assembly and see if it changed at all with this update. There is a good chance that it doesn't change at all since I don't see any references to Microsoft.Build.Framework. Since the dependency is marked with ExcludeAssets="Runtime" it means it actually doesn't even reach your output directory, so the only thing changed is the version of assembly passed to the compiler for your users. I checked and these two version 16.10.0 and 17.8.3 both expose the same assembly version (15.1.0.0). The new Microsoft.Build.Framework has much fewer dependencies (as in none) so it would be beneficial for you and your users to take this update and have fewer unused packages to worry about updating. @rainersigwald do you see any risk here for this update given the other dependencies this has?

[rainersigwald]

I agree that bumping the MSBuild dependency around the Roslyn Workspaces package should generally be low-impact. I can't dig into this deeply today but I think @ericstj is looking at the right things.