Ensure SSL_CERT_DIR messages are always shown and check for existing value

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#3

[claude]

🔴 EventLevel.LogAlways (value 0) disables level filtering in EventSource, causing all Warning/Error events (NSS trust failures, OpenSSL issues, etc.) to be shown to users in non-verbose mode. This was correctly fixed to EventLevel.Critical in commit 60be2ab but accidentally reverted in this PR. Change EventLevel.LogAlways back to EventLevel.Critical on line 134.

Extended reasoning...

The Bug

On line 134 of Program.cs, the non-verbose code path calls listener.EnableEvents(CertificateManager.Log, System.Diagnostics.Tracing.EventLevel.LogAlways). The intent is to only receive LogAlways-level events (the SSL_CERT_DIR suggestion messages, events 110, 111, 114). However, EventLevel.LogAlways has the numeric value 0, and in .NET's EventSource implementation, level 0 means no level filtering at all — every event passes through regardless of its level.

How EventSource Filtering Works

The EventSource.IsEnabled check works like this:

if ((int)m_level != 0) {
    if ((int)m_level < (int)level) return false;
}

When m_level is 0 (LogAlways), the entire filtering block is skipped, so all events are delivered. This is documented behavior: "LogAlways: No level filtering is done on the event."

Step-by-Step Proof

1. User runs dotnet dev-certs https --trust without --verbose.
2. Code enters the else branch at line 133 and calls EnableEvents with EventLevel.LogAlways (value 0).
3. During trust operations, UnixCertificateManager.TrustCertificateCore encounters an NSS database issue and fires UnixNssDbTrustFailed (Event 90, EventLevel.Warning).
4. Because level filtering is disabled (level = 0), this Warning event is delivered to ReporterEventListener.
5. ReporterEventListener maps Warning events to _reporter.Warn(), which always writes to console (it is not gated by verbose mode).
6. The user sees unexpected warning output about NSS databases in non-verbose mode.

Impact

The CertificateManagerEventSource has dozens of Warning and Error level events (events 34, 37, 41, 71, 74, 75, 76, 77, 82-86, 88, 90, 92-101, 103-106, 108, 109, 112, etc.). All of these will be delivered to the listener and displayed to users, producing noisy, confusing output during normal dotnet dev-certs https --trust operations.

Git History Confirms the Regression

Commit 60be2ab ("Use critical for log level filter") correctly changed EventLevel.LogAlways to EventLevel.Critical (value 1). With EventLevel.Critical, only LogAlways (0) and Critical (1) events pass the filter — exactly the intended behavior. This PR's commit 07d6e09 ("update pr") reverted that fix back to EventLevel.LogAlways.

Fix

Change EventLevel.LogAlways to EventLevel.Critical on line 134:

listener.EnableEvents(CertificateManager.Log, System.Diagnostics.Tracing.EventLevel.Critical);

This enables only LogAlways (value 0) and Critical (value 1) events, which is the correct filtering for non-verbose mode.

[claude]

🔴 Bug: Line 408 uses sawTrustFailure = !hasValidSslCertDir which unconditionally overwrites sawTrustFailure, erasing any prior failure state from dotnet trust, OpenSSL trust, or NSS DB trust steps. If the cert directory is already in SSL_CERT_DIR (hasValidSslCertDir is true), sawTrustFailure becomes false even though earlier steps failed, causing TrustLevel.Full to be returned instead of TrustLevel.Partial. Fix: use sawTrustFailure |= !hasValidSslCertDir to preserve earlier failures.

Extended reasoning...

What the bug is

In TrustCertificateCore, the variable sawTrustFailure tracks whether any trust step has failed. Multiple earlier code paths can set it to true: dotnet trust failure (line ~217), OpenSSL trust failure (line 295), NSS DB trust failures (lines 322, 328), and missing certutil (line 304). However, line 408 unconditionally assigns sawTrustFailure = !hasValidSslCertDir, which overwrites the accumulated failure state.

The specific code path that triggers it

Consider this scenario:

1. OpenSSL trust succeeds (openSslTrustSucceeded = true)
2. NSS DB trust fails for a browser (e.g., Firefox) -- sawTrustFailure is set to true at line 322 or 328
3. The partial success check at lines 333-344 logs the warning but does NOT return (because sawTrustSuccess is also true)
4. Execution enters the if (openSslTrustSucceeded) block at line 346
5. SSL_CERT_DIR already contains the cert directory, so hasValidSslCertDir is set to true
6. Line 408: sawTrustFailure = !hasValidSslCertDir becomes sawTrustFailure = false
7. The return at line 411-413 evaluates sawTrustFailure as false and returns TrustLevel.Full

Why existing code does not prevent it

The check at lines 333-344 correctly detects the partial failure state and logs it, but it intentionally does not return early when there has been partial success (to allow for the SSL_CERT_DIR messaging). This is by design -- the method is supposed to continue and return Partial at line 411. However, the unconditional assignment at line 408 destroys the failure information that line 411 needs to make the correct decision.

Impact

When trust partially fails (e.g., NSS DB trust for Firefox fails, but OpenSSL and dotnet trust succeed) AND the certificate directory is already in SSL_CERT_DIR, the method incorrectly reports TrustLevel.Full. The caller (EnsureAspNetCoreHttpsDevelopmentCertificate) will then report complete success instead of PartiallyFailedToTrustTheCertificate, hiding the fact that the certificate is not trusted in some browsers.

How to fix

Change line 408 from:

sawTrustFailure = !hasValidSslCertDir;

to:

sawTrustFailure |= !hasValidSslCertDir;

The |= operator preserves any prior true value of sawTrustFailure while still setting it to true if hasValidSslCertDir is false.

[claude]

🔴 Path.GetFullPath(prettyCertDir) on line 365 resolves the literal string $HOME as a directory name (e.g. /cwd/$HOME/.aspnet/dev-certs/trust) because .NET does not expand shell variables, so the SSL_CERT_DIR comparison on line 375 always fails when the cert directory is under the home directory. This means the code always suggests appending to SSL_CERT_DIR even when it already contains the correct directory. Use certDir (the actual filesystem path) instead of prettyCertDir for the GetFullPath call, and change StringComparison.OrdinalIgnoreCase to StringComparison.Ordinal on line 375 since Unix paths are case-sensitive.

Extended reasoning...

Bug 1: Path.GetFullPath on prettyCertDir containing literal $HOME

On lines 354-355, prettyCertDir is set to Path.Combine("$HOME", certDir[homeDirectoryWithSlash.Length..]) when the cert directory starts with the home directory. This produces a display-friendly string like "$HOME/.aspnet/dev-certs/trust". On line 365, Path.GetFullPath(prettyCertDir) is called on this value.

Since Path.GetFullPath is a .NET API and does NOT expand shell environment variables, $HOME is treated as a literal directory name. The result is resolved relative to the current working directory, producing something like "/some/cwd/$HOME/.aspnet/dev-certs/trust". This bogus path is stored in certDirFullPath and used in the comparison on line 375.

Step-by-step proof:

1. Suppose homeDirectory = /home/user, so homeDirectoryWithSlash = /home/user/
2. certDir = /home/user/.aspnet/dev-certs/trust (starts with homeDirectoryWithSlash)
3. prettyCertDir = Path.Combine("$HOME", ".aspnet/dev-certs/trust") = "$HOME/.aspnet/dev-certs/trust"
4. certDirFullPath = Path.GetFullPath("$HOME/.aspnet/dev-certs/trust") = "/some/cwd/$HOME/.aspnet/dev-certs/trust" (bogus path)
5. SSL_CERT_DIR contains /home/user/.aspnet/dev-certs/trust
6. Path.GetFullPath("/home/user/.aspnet/dev-certs/trust") = /home/user/.aspnet/dev-certs/trust
7. Comparison: "/home/user/.aspnet/dev-certs/trust" == "/some/cwd/$HOME/.aspnet/dev-certs/trust" → false
8. Result: Code incorrectly reports the directory is not in SSL_CERT_DIR and suggests appending it

This is the common case since the default cert directory is always under the home directory. The fix is to use certDir (the actual filesystem path) instead of prettyCertDir for the Path.GetFullPath comparison, reserving prettyCertDir only for display/logging purposes.

Bug 2: OrdinalIgnoreCase comparison for Unix paths

On line 375, the comparison uses StringComparison.OrdinalIgnoreCase. UnixCertificateManager only runs on Linux, where file paths are case-sensitive. Using case-insensitive comparison could cause a false positive match where differently-cased paths (e.g. /home/user/.aspnet/Dev-Certs/Trust vs /home/user/.aspnet/dev-certs/trust) are incorrectly treated as equal. This is inconsistent with line 354, which correctly uses StringComparison.Ordinal. The comparison should use StringComparison.Ordinal to match Unix filesystem semantics.

While bug 2 has lower practical impact (case differences in paths are uncommon), it is a correctness issue in new code and should be fixed alongside bug 1.

[claude]

🟣 Pre-existing bug: Event 111 (UnixSuggestSettingEnvironmentVariableWithoutExample) message uses {2} placeholder but the method only accepts 2 parameters (certDir={0}, envVarName={1}), so {2} has no corresponding argument. This will cause a FormatException at runtime when string.Format in ReporterEventListener.OnEventWritten tries to resolve {2} with only 2 payload items. The fix is to change {2} to {1} in the message string. This is a pre-existing issue not introduced by this PR, but it is directly adjacent to the modified Event 110.

Extended reasoning...

What the bug is

Event 111 (UnixSuggestSettingEnvironmentVariableWithoutExample) in CertificateManager.cs has a message string that references placeholder {2}:

[Event(111, ..., Message = "For OpenSSL trust to take effect, '{0}' must be listed in the {2} environment variable. ...")]
internal void UnixSuggestSettingEnvironmentVariableWithoutExample(string certDir, string envVarName) => WriteEvent(111, certDir, envVarName);

The method only has two parameters: certDir (index 0) and envVarName (index 1). There is no parameter at index 2.

How it manifests at runtime

In ReporterEventListener.OnEventWritten (ReporterEventListener.cs:33), the event message is formatted using:

var message = string.Format(CultureInfo.InvariantCulture, eventData.Message ?? "", eventData.Payload?.ToArray() ?? Array.Empty<object>());

When Event 111 fires, string.Format receives a format string containing {2} but only a 2-element payload array (indices 0 and 1). This causes a FormatException to be thrown at runtime.

Step-by-step proof

1. User runs dotnet dev-certs https --trust on Linux without --verbose.
2. OpenSSL trust succeeds but SSL_CERT_DIR is not set.
3. TryGetOpenSslDirectory returns false (e.g., OpenSSL is not installed or returns an unexpected format).
4. Code falls into the else branch in UnixCertificateManager.TrustCertificateCore, calling Log.UnixSuggestSettingEnvironmentVariableWithoutExample(prettyCertDir, OpenSslCertificateDirectoryVariableName).
5. The ReporterEventListener receives Event 111 with Payload = [prettyCertDir, "SSL_CERT_DIR"] (2 items).
6. string.Format tries to resolve {2} in the message but there is no index 2 in the payload array.
7. A FormatException is thrown, crashing the event listener.

Why existing code does not prevent it

The EventSource infrastructure does not validate that format string placeholders match method parameter counts at compile time. The mismatch is only caught at runtime when the message is actually formatted. Previously this event was only delivered when --verbose was passed (the old code only created the listener in verbose mode), making this harder to hit. With this PR now enabling event listening in non-verbose mode at EventLevel.LogAlways, Event 111 (which is LogAlways level) will always be delivered to the listener, making this crash more likely to be exercised.

Impact

When a user trusts a dev certificate on Linux and TryGetOpenSslDirectory fails, the tool will crash with a FormatException instead of showing a helpful message about setting SSL_CERT_DIR.

Fix

Change {2} to {1} in the Event 111 message string, since envVarName is the second parameter (index 1). Compare with the sibling Event 110, where {2} correctly maps to the third parameter envVarName (which is the third of three parameters in that method).