snyk / snyk-nuget-plugin

Basic Snyk CLI plugin for .NET support.
Other
5 stars 13 forks source link

Support for Imports in .csproj files #41

Closed julienduchesne closed 3 years ago

julienduchesne commented 6 years ago

It is impossible for us to use snyk for .NET since our TargetFramework attribute resides in a common file. Ex: File1.csproj:

<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <ProjectGuid>{1234}</ProjectGuid>
    <AssemblyName>File1</AssemblyName>
    <RootNamespace>File1</RootNamespace>
    <RelRootDir>..\..\</RelRootDir>
  </PropertyGroup>
  <Import Project="$(RelRootDir)\Shared\File2.props" />
</Project>

File2.props:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project=".\CommonSettings.props" />
</Project>

CommonSettings.props:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
.
.
.
    <TargetFrameworkVersion Condition=" '$(TargetFrameworkVersion)' == '' ">v4.6.1</TargetFrameworkVersion>
.
.
.
  </PropertyGroup>
</Project>
orkamara commented 6 years ago

Thanks for pointing this out for us! Handling props files (specifically for extracting the TargetFrameworkVersion) is definitely on our roadmap, but I can't commit to a specific date right now.

If you feel like tackling it before we get to it, you are more than a welcome! :) A good place to start might be: https://github.com/snyk/snyk-nuget-plugin/blob/master/lib/proj-parser.js#L25

We'll be happy to help with any question.

Thanks, Or

onyxmaster commented 5 years ago

Same issue here, this is blocker for us. I double that "parsing" msbuild files of any non-trivial project is an effective way to do this, since there are many things (like Directory.Build.props and Condition directives) that cannot be effectively parsed. I hacked together a small gist that might illustrate how I propose this to be handled. It assumes that msbuild path is known (I included the command line for default MSBuild installation for VS2017). Use with <full-path-to>/WriteFrameworkVersions.cmd <project-file.csproj>:

> C:\Users\xm\Projects\wf\WriteFrameworkVersions.cmd website.csproj
  TargetFrameworkVersion=v4.7.2
> C:\Users\xm\Projects\wf\WriteFrameworkVersions.cmd library.csproj
  TargetFramework=netstandard2.0

The rest should be easy to implement.

If this isn't that easy, adding a command line option along the lines of --force-target-framework=netstandard2.0 would alleviate the problem.

denissnykio commented 5 years ago

Hi, My name is Denis and I'm the Customer Success Manager for Coveo Since November 2018, Snyk made a lot of progress in .NET support For example, regarding .NET support in GitHub, BitBucket & GitLab: a) supports testing and monitoring .NET projects that have their dependencies managed by NuGet. b) supported manifest files & formats: Packages.config .csproj, .vbproj, *.fsproj

format in *.csproj, *.vbproj, *.fsproj files We also support TargetFrameworks Can you please check if the issue persists If this is the case, please write in this thread (that I wasn't aware of) and also in our slack channel #customer-coveo (Jean Philippe Lachance was invited in slack) Note that I'll be on vacation til March 24th, but Snykers will answer to you Regards, Denis
onyxmaster commented 5 years ago

I'm not an employee of @coveo, but our case that was described above didn't improve at all. It looks that Snyk went with the "let's parse the project file manually" approach, which, while easier to implement, just won't work when any kind of conditional MSBuild logic is involved, thus prohibiting its use in codebases with custom build setups. Of course, I'm not in position to try to comment on the Snyk .NET development direction, but adding the ability to override default target framework detection from the command line, like I suggested before, might solve this problem without throwing too much resources a tit. The more correct solution, as I mentioned before is using MSBuild to evaluate properties, but this, of course, is a bit harder to implement.

denissnykio commented 5 years ago

Hi @onyxmaster I'll ask our Product and R&D team to evaluate your proposal Thanks for the idea! Denis

nrodrigues1 commented 5 years ago

I'm an employee of Coveo, and the problem was mostly solved with the last PR. Since we use PackageReference format, it fallback to project.assets.json to find the target framework if it is not in the CSPROJ and that is perfect for us. However, we still have an issue on how the plugin finds project.assets.json files because like the target framework, if you have some MSBuild logic (which most of the medium-sized solutions have), they could be anywhere, not just at projectPath/obj.

denissnykio commented 5 years ago

Hi, I'll check with our Product Team what is planned to detect project.assets.json in a location different from projectPath/obj Can you provide an example of "exotic" path ? Thanks, Denis Snyk CSM

nrodrigues1 commented 5 years ago

Hi,

Yes, if you add a Directory.Build.props at the root of your repository with: <BaseIntermediateOutputPath>$(MSBuildThisFileDirectory)obj\$(MSBuildProjectName)\</BaseIntermediateOutputPath>, it will put assets to root\obj\projectname\project.assets.json

Quite frankly, the best way to handle "TargetFramework" and assets location should be by running C# code with the plugin. Microsoft provides libraries to achieve this with like 10 lines of codes. Implementing MSBuild logic javascript side can be hard since it is complex and from what I know not really documented.

However, we might be able to pass the folder where the project.assets.json files reside and for each project looking if there are any of the project.assets.json files that its "projectPath" fits the csproj path.

In our case, we cannot wait any longer, we will probably build our solution and putting obj at the root of each project when we run Snyk, but keep the original location for other builds or I will modify our fork to do what I said above.

denissnykio commented 5 years ago

Hi, To decide what we can develop, can we have a 1/2 hour call on Wed 17/4 at 09:00 Quebec time with our Product Manager ? If it's OK, please send me your email, so I can send you a Zoom invite Thanks, Denis

denissnykio commented 5 years ago

Hi If the slot is OK for you, please send an email to support@snyk.io (cc denis@snyk.io) and I'll send you a Zoom invite Thanks, Denis

nrodrigues1 commented 5 years ago

@denissnykio snyk test --file=pathtoprojectassets\project.assets.json --org=some org

Testing project

.csproj file not found in "path"

So, passing the project.assets.json does not work currently. I might do a PR to support it...

denissnykio commented 5 years ago

Hi I'll check with @orsagie (R&D) tomorrow morning She was on our call earlier today

orsagie commented 5 years ago

Hi @nrodrigues1, I had not realized we fail when no csproj file is found! I released a new version of the plugin and cli (v1.151.2) that will not fail when csproj is not found. This should unblock you.

nrodrigues1 commented 5 years ago

@orsagie

Thank you very much for the quick fix, much appreciated!

lili2311 commented 4 years ago

@nrodrigues1 can this issue be closed now?

nrodrigues1 commented 4 years ago

@lili2311

Sure :)