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.53k stars 1.5k forks source link

C sharp build is failing after enabling AdvancedSecurity-Codeql-Init@1 #15679

Open keeplearning-rgb opened 7 months ago

keeplearning-rgb commented 7 months ago

I am trying to run the codeql scan on a azure devops repo, with the steps defined in the official documentation.

Without putting GHAS task inside the yaml file build is successful, as soon as I introduce the module it fails.

This is the warning I get in logs after enabling GHAS in my pipeline which was not there earlier.

[warning]c:\program files\microsoft visual studio\2022\enterprise\msbuild\current\bin\Roslyn\Microsoft.Managed.Core.targets(336,5): Warning : EmitCompilerGeneratedFiles was true, but no CompilerGeneratedFilesOutputPath was provided. CompilerGeneratedFilesOutputPath must be set in order to emit generated files.

After that there are more than 100 errors stating the below which were not there earlier.

##[error]c:\Windows\Microsoft.NET\Framework\v4.0.30319\Temporary ASP.NET Files\temp\662ac91c\e2d2875f\App_Web_fe22zids.0.cs(44,0): Error CS0103: The name 'Context' does not exist in the current context

I am not changing anything in the successful build apart from adding the steps needed to run GHAS on ADO.

Please provide your insights, Thank you

hvitved commented 7 months ago

Looks like the same as https://github.com/github/code-scanning/issues/13024. There the conclusion from @tamasvajk was:

I don't think we can offer a solution to this issue. The warning doesn't have any impact on the analysis, so it's safe to ignore.

However, now it appears that the build actually breaks. @tamasvajk can you take a look?

tamasvajk commented 7 months ago

I think these might be two separate issues:

  1. Warning : EmitCompilerGeneratedFiles was true, but no CompilerGeneratedFilesOutputPath was provided. CompilerGeneratedFilesOutputPath must be set in order to emit generated files.

As @hvitved mentioned, these are safe to ignore.

  1. [error]c:\Windows\Microsoft.NET\Framework\v4.0.30319\Temporary ASP.NET Files\temp\662ac91c\e2d2875f\App_Web_fe22zids.0.cs(44,0): Error CS0103: The name 'Context' does not exist in the current context

Temporary ASP.NET Files suggests that you're compiling an ASP.NET MVC application. To fully analyze such an app, we're injecting /p:MvcBuildViews=true into the build command. Could you check if a build without the GHAS task, but with this property works?

keeplearning-rgb commented 7 months ago

Hi @tamasvajk Thanks for the quick response !!

I tried what you mentioned without GHAS task and YES the build is failing if I set the property /p:MvcBuildViews=true

tamasvajk commented 7 months ago

ASP.NET MVC is compiling the view files on the fly when the first request comes in. If your view file contained a compiler error, the user would face the server error page. /p:MvcBuildViews=true is precompiling the views, which means that all the possible compiler errors are now reported during build time. Could you check the view files and see if there are any errors in them?

keeplearning-rgb commented 7 months ago

I am not seeing any errors in the view files. @tamasvajk

I tried running it with /p:MvcBuildViews=false and adding GHAS task but it fails still. (just an update may not be relevant)

tamasvajk commented 7 months ago

I tried what you mentioned without GHAS task and YES the build is failing if I set the property /p:MvcBuildViews=true

Can you share the compiler errors? Could you share the lines of the generated files in Temporary ASP.NET Files where the compiler errors are reported? In case of the below, I'd be interested in line 44 and maybe a couple of lines around it. Also, can you figure out what Context is in this case? What fully qualified type name it has? And where does the assembly defining that type come from?

[error]c:\Windows\Microsoft.NET\Framework\v4.0.30319\Temporary ASP.NET Files\temp\662ac91c\e2d2875f\App_Web_fe22zids.0.cs(44,0): Error CS0103: The name 'Context' does not exist in the current context
keeplearning-rgb commented 7 months ago

@tamasvajk So the error codes are same for all the errors. But yes the lines numbers are different and its in .csproj file as mentioned the error

2024-02-21T11:45:46.2889491Z ##[error]c:\Windows\Microsoft.NET\Framework\v4.0.30319\Temporary ASP.NET Files\temp\662ac91c\e2d2875f\App_Web_kgjocdgo.0.cs(44,0): Error CS0103: The name 'Context' does not exist in the current context
2024-02-21T11:45:46.2891471Z c:\Windows\Microsoft.NET\Framework\v4.0.30319\Temporary ASP.NET Files\temp\662ac91c\e2d2875f\App_Web_kgjocdgo.0.cs(44): error CS0103: The name 'Context' does not exist in the current context [D:\a\1\s\MAPortal\MaintenanceAdvantage\MaintenanceAdvantage.csproj]
2

As I trace back to the line number mentioned in the error, below are some of the lines I can provide you.

Line numer 44 - 76 (In this error are on line number 44,49,53,55,59,61,65,67,71,73)

image

Errors are starting from line number 44 till 583.

tamasvajk commented 7 months ago

The compiler error is reported in a generated .cs file: c:\Windows\Microsoft.NET\Framework\v4.0.30319\Temporary ASP.NET Files\temp\662ac91c\e2d2875f\App_Web_kgjocdgo.0.cs. Could you check line 44 and its surroundings in this file?

keeplearning-rgb commented 7 months ago

I was building the code on ADO and the VMs were temporary, I tried running it with an agent which has windows-2019 server and giving only one error and failing but the error is still same. Tried fetching some logs, have a look if it helps.

  public _Page_Content_EmailTemplates_HyperScalePlatformUpdateAlert_cshtml() {
        }
        protected ASP.global_asax ApplicationInstance {
            get {
                return ((ASP.global_asax)(Context.ApplicationInstance));
            }
        }
        public override void Execute() {
WriteLiteral("<table");

WriteLiteral(" border=\"0\"");

WriteLiteral(" cellspacing=\"0\"");
tamasvajk commented 7 months ago

That Context seems to be the Context property of System.Web.Mvc.WebViewPage. What happens during runtime if you load this problematic page? How is System.Web.Mvc.dll referenced by your project? Which version of Asp.Net MVC are you using? I start to think that your issue is not CodeQL related, so you might need to reach out to folks who are more knowledgeable on Asp.Net MVC.

From CodeQL perspective, you could try to disable the view building, but it's somewhat cumbersome. See more details here: https://github.com/advanced-security/advanced-security-material/blob/main/troubleshooting/codeql-builds/compiled-languages-csharp.md#mvcbuildviews-target-failures.

keeplearning-rgb commented 7 months ago
image

These were the lines already mentioned in the .csproj file of the repo changing the condition to false makes it work. One thing is isn't this the same as enabling /p:MvcBuildViews=true since it is also trying to build the views after the build is happening? (correct me if I am wrong)

Also a clarification , I wanted to understand if the root cause analysis we did here I got it completely or not. As I have understood it, during the initial build without the GHAS enabled we are not building the views along with our initial build as soon as we enable it it breaks due to some misconfigurations in our .csproj. Though it's a mandatory step for CodeQL it enables it internally and our build breaks because of that.

By. providing this condition inside the Target Explicitly we are overriding that condition and making it somehow work, but it is possible that codeql will not be able to address the issues inside the views during this scan.

tamasvajk commented 7 months ago

These were the lines already mentioned in the .csproj file of the repo changing the condition to false makes it work. One thing is isn't this the same as enabling /p:MvcBuildViews=true since it is also trying to build the views after the build is happening? (correct me if I am wrong)

I'm not sure I understand your question. The condition '$(MvcBuildViews)'=='false' means that this target will only be executed if MvcBuildViews is set to false. So if you or CodeQL is passing in /p:MvcBuildViews=true, then the view compilation will not be run because 'true'=='false' doesn't hold, and therefore your build will work.

As I have understood it, during the initial build without the GHAS enabled we are not building the views along with our initial build as soon as we enable it it breaks due to some misconfigurations in our .csproj. Though it's a mandatory step for CodeQL it enables it internally and our build breaks because of that.

In essence it's a correct summary. Some extensions to it: I'm not sure we concluded that "it breaks due to some misconfigurations in our .csproj". It could also be the case that the CI environment is configured differently than your production system, so view compilation fails on one system, but not on the other. Also, it's not a mandatory step for CodeQL, it's simply the default CodeQL behavior that we inject the /p:MvcBuildViews=true to the build command. You could change this behavior by specifying another tracer configuration with the --extra-tracing-config option.

By. providing this condition inside the Target Explicitly we are overriding that condition and making it somehow work, but it is possible that codeql will not be able to address the issues inside the views during this scan.

Yes, this is correct.