Skip to content

Conversation

@Kwstubbs
Copy link
Contributor

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

@Kwstubbs Kwstubbs requested a review from a team as a code owner August 26, 2025 06:33
Copilot AI review requested due to automatic review settings August 26, 2025 06:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the C# SSRF (Server-Side Request Forgery) query by reducing false positives through more precise filtering of remote flow sources. The changes remove certain HttpRequest fields and NavigationManager.BaseUri from being considered as remote flow sources.

  • Excludes specific HttpRequest properties (Method, Scheme, IsHttps, Protocol) from remote flow sources
  • Removes NavigationManager.BaseUri from the remote flow source model
  • Updates change notes to document the SSRF query improvement

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
csharp/ql/src/change-notes/2025-08-25.md Documents the fix for reduced false positives in the SSRF query
csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll Adds exclusion logic for specific HttpRequest properties
csharp/ql/lib/ext/Microsoft.AspNetCore.Components.model.yml Removes NavigationManager.BaseUri from remote flow sources

@github-actions
Copy link
Contributor

github-actions bot commented Aug 26, 2025

⚠️ 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

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this!

  • Only one minor change suggestion.
  • I will trigger a DCA run to see the effects on our test suite.

@michaelnebel
Copy link
Contributor

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
Copy link
Contributor

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

@Kwstubbs Kwstubbs force-pushed the csharp-ssrf-improvements branch from 45c05c5 to b39b6b0 Compare September 2, 2025 09:04
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The failing change note check is unrelated.

@Kwstubbs Kwstubbs force-pushed the csharp-ssrf-improvements branch from b39b6b0 to c4c77e8 Compare September 23, 2025 22:43
@Kwstubbs
Copy link
Contributor Author

@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?

@michaelnebel
Copy link
Contributor

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants