[nvborisenko]

Fixes the following issue:

driver.GoTo("https://google.com"); // we see logs

Log.SetLevel(LogEventLevel.Trace);

driver.GoTo("https://google.com"); // we don't see trace logs

💥 What does this PR do?

Always wrap default http client handler for remote commands execution by our special client handler, which always treats current log level in runtime.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Type

Bug fix

---

Description

• Always wrap HTTP client handler with diagnostics handler

• Check log level at runtime in SendAsync method

• Fixes trace logs not appearing after SetLevel call

• Prevents unnecessary string building when trace disabled

---

File Walkthrough

Relevant files

Bug fix

HttpCommandExecutor.cs Runtime log level checking in HTTP diagnostics handler      dotnet/src/webdriver/Remote/HttpCommandExecutor.cs Removed conditional wrapping of DiagnosticsHttpHandler in CreateHttpClient() method Always wrap the HTTP client handler regardless of current log level Moved log level checks into SendAsync() method to evaluate at runtime Wrapped request and response logging blocks with _logger.IsEnabled(LogEventLevel.Trace) checks Prevents expensive string building operations when trace logging is disabled +26/-23

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Sensitive log exposure Description: Trace logging in DiagnosticsHttpHandler.SendAsync logs full HTTP request/response details (including request bodies and non-success response bodies), which can expose sensitive data (e.g., credentials, session tokens, cookies, PII) to log sinks when Trace is enabled at runtime. HttpCommandExecutor.cs [440-480] Referred Code if (_logger.IsEnabled(LogEventLevel.Trace)) { StringBuilder requestLogMessageBuilder = new(); requestLogMessageBuilder.AppendFormat(">> {0} {1}", request.Method, request.RequestUri?.ToString() ?? "null"); if (request.Content is not null) { #if NET8_0_OR_GREATER var requestContent = await request.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); #else var requestContent = await request.Content.ReadAsStringAsync().ConfigureAwait(false); #endif requestLogMessageBuilder.AppendFormat("{0}{1}", Environment.NewLine, requestContent); } _logger.Trace(requestLogMessageBuilder.ToString()); } var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); ... (clipped 20 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Sensitive data in logs: Trace logging includes raw HTTP request and (on errors) response bodies which can contain secrets/PII and is emitted as unstructured strings. Referred Code if (_logger.IsEnabled(LogEventLevel.Trace)) { StringBuilder requestLogMessageBuilder = new(); requestLogMessageBuilder.AppendFormat(">> {0} {1}", request.Method, request.RequestUri?.ToString() ?? "null"); if (request.Content is not null) { #if NET8_0_OR_GREATER var requestContent = await request.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); #else var requestContent = await request.Content.ReadAsStringAsync().ConfigureAwait(false); #endif requestLogMessageBuilder.AppendFormat("{0}{1}", Environment.NewLine, requestContent); } _logger.Trace(requestLogMessageBuilder.ToString()); } var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); ... (clipped 20 lines) Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Preserve response content after logging To prevent the logging from consuming the response body, read the content, log it, and then reassign response.Content with a new StringContent object containing the read data. dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [462-482] +string? loggedResponseContent = null; if (_logger.IsEnabled(LogEventLevel.Trace)) { StringBuilder responseLogMessageBuilder = new(); - responseLogMessageBuilder.AppendFormat("<< {0} {1}", (int)response.StatusCode, response.ReasonPhrase); - - if (!response.IsSuccessStatusCode && response.Content != null) + if (response.Content != null) { #if NET8_0_OR_GREATER - var responseContent = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); + loggedResponseContent = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); #else - var responseContent = await response.Content.ReadAsStringAsync().ConfigureAwait(false); + loggedResponseContent = await response.Content.ReadAsStringAsync().ConfigureAwait(false); #endif - responseLogMessageBuilder.AppendFormat("{0}{1}", Environment.NewLine, responseContent); + responseLogMessageBuilder.AppendFormat("{0}{1}", Environment.NewLine, loggedResponseContent); } - _logger.Trace(responseLogMessageBuilder.ToString()); } - +if (loggedResponseContent != null) +{ + var mediaType = response.Content.Headers.ContentType?.MediaType; + response.Content = new StringContent(loggedResponseContent, Encoding.UTF8, mediaType); +} return response; Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug where reading the response content for logging consumes the stream, making it unavailable for downstream consumers. This would break functionality when trace logging is enabled.	High
	Guard logging against exceptions Wrap the trace logging logic in a try/catch block to handle any potential exceptions and prevent them from interfering with the HTTP request execution. dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [440-458] if (_logger.IsEnabled(LogEventLevel.Trace)) { - StringBuilder requestLogMessageBuilder = new(); - // ... build log ... - _logger.Trace(requestLogMessageBuilder.ToString()); + try + { + StringBuilder requestLogMessageBuilder = new(); + // ... build log ... + _logger.Trace(requestLogMessageBuilder.ToString()); + } + catch (Exception) + { + // Ignore logging failures + } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: This is a good defensive programming practice that improves robustness by ensuring that potential (though likely rare) exceptions within the logging code do not disrupt the primary HTTP request flow.	Low
General	Log all response bodies Modify the condition for logging the response body from !response.IsSuccessStatusCode && response.Content != null to just response.Content != null to include successful responses in trace logs. dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [468-477] -if (!response.IsSuccessStatusCode && response.Content != null) +if (response.Content != null) { // read and append content } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that for 'Trace' level logging, it is more useful to log all response bodies, not just those from failed requests, which significantly improves debuggability.	Low
Update

[Copilot]

Pull request overview

Updates the .NET HttpCommandExecutor HTTP pipeline so that trace-level HTTP request/response logging can be enabled (or disabled) at runtime via Log.SetLevel(...), instead of being decided only when the HttpClient is first created.

Changes:

• Always wraps the underlying HttpClientHandler with DiagnosticsHttpHandler in CreateHttpClient().
• Moves trace logging work in DiagnosticsHttpHandler.SendAsync() behind runtime _logger.IsEnabled(LogEventLevel.Trace) checks (for both request and response).

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

dotnet/src/webdriver/Remote/HttpCommandExecutor.cs:259

• In CreateHttpClient(), the local handler variable is now always overwritten, and the wrapper is constructed with httpClientHandler instead of the current handler. This is redundant today, and it also makes it easy to accidentally bypass additional handler wrapping added later. Consider either removing the handler variable entirely, or wrap the current handler (not httpClientHandler).

        HttpMessageHandler handler = new DiagnosticsHttpHandler(httpClientHandler, _logger);

        var client = new HttpClient(handler);

        client.DefaultRequestHeaders.UserAgent.ParseAdd(this.UserAgent);

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.