codeql icon indicating copy to clipboard operation
codeql copied to clipboard

C# SSRF Improvements

Open Kwstubbs opened this issue 4 months ago • 5 comments

This PR resolves most of the FPs I found in the SSRF query.

Kwstubbs avatar Aug 26 '25 06:08 Kwstubbs

:warning: The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

csharp

Generated file changes for csharp

  • Changes to framework-coverage-csharp.rst:
-    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.AspNetCore.Components``, ``Microsoft.AspNetCore.Http``, ``Microsoft.AspNetCore.Mvc``, ``Microsoft.AspNetCore.WebUtilities``, ``Microsoft.CSharp``, ``Microsoft.Data.SqlClient``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.DotNet.Build.Tasks``, ``Microsoft.DotNet.PlatformAbstractions``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.JSInterop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.VisualBasic``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",60,2257,159,4
+    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.AspNetCore.Components``, ``Microsoft.AspNetCore.Http``, ``Microsoft.AspNetCore.Mvc``, ``Microsoft.AspNetCore.WebUtilities``, ``Microsoft.CSharp``, ``Microsoft.Data.SqlClient``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.DotNet.Build.Tasks``, ``Microsoft.DotNet.PlatformAbstractions``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.JSInterop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.VisualBasic``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",59,2257,159,4
-    Totals,,107,14505,407,9
+    Totals,,106,14505,407,9
  • Changes to framework-coverage-csharp.csv:
- Microsoft.AspNetCore.Components,2,4,2,,,,,,,2,,,,,,,,,4,,,1,1
+ Microsoft.AspNetCore.Components,2,3,2,,,,,,,2,,,,,,,,,3,,,1,1

github-actions[bot] avatar Aug 26 '25 06:08 github-actions[bot]

DCA looks good. We see five fewer cs/log-forging alerts on dotnet/aspnetcore, which is expected: those alerts were caused by the request method or protocol being included in log output, and the reduction matches removing those properties as remote sources.

michaelnebel avatar Aug 28 '25 09:08 michaelnebel

If the PR is rebased on main then I expect that the Blazor test will stop failing.

michaelnebel avatar Aug 28 '25 11:08 michaelnebel

@michaelnebel I was going ask for a merge but I see there are some issues with documentation unrelated to my PR. Should I wait or is everything fixed now?

Kwstubbs avatar Oct 26 '25 23:10 Kwstubbs

@michaelnebel I was going ask for a merge but I see there are some issues with documentation unrelated to my PR. Should I wait or is everything fixed now?

We can disregard the failing QLDoc check, but there is also a failing test that we need to address. I expect, if you cherry-pick this commit it will be fixed. Thank you for the persistence and patience!

michaelnebel avatar Oct 27 '25 07:10 michaelnebel