Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR adjusts nullable reference type contexts across several source files (adding and in one case removing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.githubusercontent.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
lrfalslev
commented
Feb 23, 2026
lrfalslev
commented
Feb 23, 2026
lrfalslev
commented
Feb 23, 2026
lrfalslev
commented
Feb 23, 2026
lrfalslev
commented
Feb 23, 2026
ktstrader
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Chores