github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.32k stars 1.46k forks source link

MSBuild doesn't respect MvcBuildViews-setting in .csproj -file when run through CodeQL-cli or through codeql github action #11890

Open pekkasin opened 1 year ago

pekkasin commented 1 year ago

Issue Description

MSBuild doesn't seem to respect the MvcBuildViews-setting defined in a .NET-project's .csproj-file when it is run as a github action or through codeql cli. After trial and error, MSBuild seems to behave as if MvcBuildViews is hardcoded to be true. Mvc view precompilation works just as expected when MSBuild is run locally on its own.

In short, I can't disable mvc view precompilation by setting<MvcBuildViews>false</MvcBuildViews> in project configuration.

Possibly noteworthy: I've used two different versions of CodeQL (2.12.0 latest and 2.6.0) and this only happens with 2.12.0.

Steps to Reproduce

  1. Create a new ASP.NET Web Application called "TestProject" with MVC project template in Visual Studio 2022.
  2. Observe that MVC view precompilation is set to falseby default in the project's .csproj -file like so: <MvcBuildViews>false</MvcBuildViews>
  3. Observe that further down the same .csproj-file a build target MvcBuildViews is defined like so: <Target Name="MvcBuildViews" AfterTargets="AfterBuild" Condition="'$(MvcBuildViews)'=='true'"> <AspNetCompiler VirtualPath="temp" PhysicalPath="$(WebProjectOutputDir)" /> </Target>
  4. Introduce a syntax error to any of the .cshtml-files in the project to trigger compiler warnings when using msbuild with Mvc view precompilation (right now that should be set to false according to the configuration above). For example, add a semicolon to ViewBag.Title inViews/Home/Index.cshtml like this:
@{
    ViewBag.Title = "Home Page" + ;  <!-- The semicolon triggers a syntax error in Visual Studio -->
}
  1. nuget restore the dependencies if Visual Studio didn't do it for you.
  2. Try to build the solution locally with msbuild.exe TestProject.sln. The compilation should complete without errors.
  3. Define a GitHub Action for the solution (latest version of setup-msbuild or something older, doesn't matter) and try the same compilation process via setup-msbuild GitHub Action. Example CodeQL configuration including steps for MSBuild:
    
    name: "CodeQL"

on: push: branches: [ "main" ] pull_request: branches: [ "main" ] schedule:

jobs: analyze: name: Analyze runs-on: windows-latest permissions: actions: read contents: read security-events: write

strategy:
  fail-fast: false
  matrix:
    language: [ 'csharp' ]

steps:
- name: Checkout repository
  uses: actions/checkout@v3

- name: Initialize CodeQL
  uses: github/codeql-action/init@v2
  with:
    languages: ${{ matrix.language }}
    queries: security-extended   

- name: Add msbuild to PATH
  uses: microsoft/setup-msbuild@v1

- name: Install Nuget to restore packages
  uses: nuget/setup-nuget@v1

- name: Restore Nuget packages
  run: nuget restore TestProject.sln   

- name: Build app for release
  run: msbuild.exe TestProject.sln 

- name: Perform CodeQL Analysis
  uses: github/codeql-action/analyze@v2
or alternatively try to build a codeql database locally with codeql cli.

9. The compilation step fails because of the syntax error in the .cshtml -file, which implies that precompilation is happening despite a setting to the contrary in the .csproj-file.

### Expected Behavior
When run through GitHub Actions or codeql cli, MSBuild shouldn't return errors related to syntax errors in .cshtml -files when I've disabled Mvc view precompilation by setting `<MvcBuildViews>false</MvcBuildViews>`.

### Actual Behavior
MSBuild (through GitHub Actions or codeql cli) fails with an error regarding the arbitrary syntax error introduced in step 4, which implies that Mvc View precompilation is happening despite it being disabled. This is different than what happens when I run MSBuild locally (MSBuild completes without errors). 

This discrepancy between the results locally and in GitHub Actions (or codeql cli) is confusing since they both use the same version of MSBuild. **The build target defined above has the condition set that `MvcBuildViews` should be `true` for the precompilation to happen.** 

I tried out all the different "permutations" of the two settings (MvcBuildViews-element and the build target condition) both locally (msbuild on its own) and in GitHub Actions and I got the following results (if the setting and condition match, the precompilation should happen, if I've understood correctly):
       | <MvcBuildViews> | Build target condition | Did precompilation happen?

Local | False | False | Yes GH Actions | | | No

Local | True | False | No GH Actions | | | No

This is the default setting that the project template comes with

Local | False | True | No GH Actions | | | Yes

Local | True | True | Yes GH Actions | | | Yes


So the local run of MSBuild worked just as expected with the precompilation only happening when the MvcBuildView-setting matched the build target condition. On GH Actions the results varied quite a bit. **It basically looks like MSBuild on GH Actions behaves as if the** `<MvcBuildViews>`-**setting is hard coded to be** `true` (ignoring whatever is actually set in the .csproj-file). I also tried to set the MvcBuildViews-parameter by calling GH Action MsBuild with `/p:MvcBuildViews=false` but that did nothing (locally that also worked just as expected. 

Since this same behavior occurs when using codeql cli (and has only started happening somewhere between versions 2.6.0 and 2.12.0), I'm assuming that this is a problem with codeql itself and not with msbuild. 
smowton commented 1 year ago

You're right that CodeQL always sets MvcBuildViews to true in order to build and index .cshtml files, since they may be relevant to security issues that our analysis tries to identify. I'll ask the C# team to comment further re: whether this can be disabled, or if we would consider adding such a facility in future.

pekkasin commented 1 year ago

Hello, thanks for replying! I had a hunch that the reason could be something like what you just described. The thing is that this wasn't a "feature" a couple of months back, so this change caused me a bit of a headache trying to trace it down (couldn't find it documented anywhere either) :D A simple ability to disable it would be very welcome since it would remove the need for me to maintain a separate configuration with CodeQL in mind.

tamasvajk commented 1 year ago

Out of curiosity, doesn't the compile time failure (introduced by the codeql analysis) mean that your application would fail at runtime?

pekkasin commented 1 year ago

Yeah, it would for that particular view. I'll readily admit that this is probably an unusual use case :)

mohis560 commented 1 year ago

Any update on how we can ignore the error.

"D:\a\esd-coa\esd-coa\COA.Web.sln" (default target) (1) -> "D:\a\esd-coa\esd-coa\COA.User\COA.User.csproj" (default target) (12) -> (MvcBuildViews target) -> d:\a\esd-coa\esd-coa\COA.User\Views\Shared\EditorTemplates\String.cshtml(3): error CS1061: 'Kendo.Mvc.UI.Fluent.WidgetFactory' does not contain a definition for 'TextBoxFor' and no extension method 'TextBoxFor' accepting a first argument of type 'Kendo.Mvc.UI.Fluent.WidgetFactory' could be found (are you missing a using directive or an assembly reference?) [D:\a\esd-coa\esd-coa\COA.User\COA.User.csproj]

pekkasin commented 1 year ago

Until an option to turn this feature off turns up, I've resorted to tweaking the csproj file with powershell during the GitHub Action so that the hard-coded condition "gets fooled", basically. Something like this:

$filePath = (Join-Path $pwd '\SUBFOLDER\YOURCSPROJFILE.csproj')
$csproj = [xml](Get-Content $filePath)
$buildTargetNode = $csproj.Project.Target | ? name -eq "MvcBuildViews"
$buildTargetNode.SetAttribute("Condition", "'`$(MvcBuildViews)'=='false'")
$csproj.Save($filePath)
wabudd1 commented 1 year ago

Running into the same problem. My VS solution is ancient and something is broken that prevents MvcBuildViews from working in the first place. This is what msbuild.exe spits out when CodeQL is in the workflow:

(MvcBuildViews target) -> C:\Windows\Microsoft.NET\Framework\v4.0.30319\Config\web.config(129): error ASPCONFIG: Could not load type 'System.Data.Entity.Design.AspNet.EntityDesignerBuildProvider'.

As far as I know, all of my .cshtml Views compile correctly at runtime.

pekkasin commented 8 months ago

Another workaround is to add ContinueOnError="WarnAndContinue"-attribute to the MvcBuildViews build target like this:

<Target Name="MvcBuildViews" AfterTargets="AfterBuild" Condition="'$(MvcBuildViews)'=='true'">
    <AspNetCompiler VirtualPath="temp" PhysicalPath="$(WebProjectOutputDir)" ContinueOnError="WarnAndContinue" />
  </Target>