Ensure SSL_CERT_DIR messages are always shown and check for existing value#3
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Code Review by Qodo
1. NssDb avoids primary constructor
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+994
to
+1006
| private sealed class NssDb | ||
| { | ||
| public string Path => path; | ||
| public bool IsFirefox => isFirefox; | ||
| private readonly string _path; | ||
| private readonly bool _isFirefox; | ||
|
|
||
| public NssDb(string path, bool isFirefox) | ||
| { | ||
| _path = path; | ||
| _isFirefox = isFirefox; | ||
| } | ||
|
|
||
| public string Path => _path; | ||
| public bool IsFirefox => _isFirefox; |
There was a problem hiding this comment.
1. nssdb avoids primary constructor 📘 Rule violation ✓ Correctness
• NssDb was changed from a C# 12 primary constructor to a traditional constructor with repetitive parameter-to-field assignment. • This adds boilerplate and diverges from the required modern DI/boilerplate-reduction convention, making the code less concise and consistent. • Reverting to a primary constructor would satisfy the requirement and keep intent clear.
Agent prompt
## Issue description
`NssDb` was rewritten to use a traditional constructor and backing fields, which violates the requirement to use C# primary constructors where the class is a simple parameter-to-field mapping.
## Issue Context
This is a small private sealed helper type that previously fit the primary-constructor pattern.
## Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[994-1006]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+373
to
+381
| try | ||
| { | ||
| return string.Equals(Path.GetFullPath(dir), certDirFullPath, StringComparison.OrdinalIgnoreCase); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore invalid directory entries in SSL_CERT_DIR | ||
| return false; | ||
| } |
There was a problem hiding this comment.
2. Bare catch ignores exceptions 📘 Rule violation ⛯ Reliability
• Parsing SSL_CERT_DIR entries uses a bare catch that silently ignores all exceptions from Path.GetFullPath(dir). • This can mask unexpected failures (beyond just malformed entries) and removes actionable context needed to diagnose trust issues. • Catch specific exceptions and/or log at Verbose with safe context (e.g., exception type and the offending entry) to preserve debuggability.
Agent prompt
## Issue description
A bare `catch` swallows all exceptions when processing entries from `SSL_CERT_DIR`, which can hide unexpected issues and violates the robust error-handling requirement.
## Issue Context
The code is attempting to ignore malformed directory entries, but it currently suppresses every exception type without any diagnostic trail.
## Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[360-382]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| hasValidSslCertDir = false; | ||
| } | ||
|
|
||
| sawTrustFailure = !hasValidSslCertDir; |
There was a problem hiding this comment.
3. Trust flag overwritten 🐞 Bug ✓ Correctness
• TrustCertificateCore now reassigns sawTrustFailure based only on SSL_CERT_DIR validity, overwriting earlier failures from dotnet store / NSS trust steps. • This can incorrectly return TrustLevel.Full (and avoid non-zero exit) even when earlier trust steps failed, misleading users about actual trust state.
Agent prompt
### Issue description
`UnixCertificateManager.TrustCertificateCore` overwrites `sawTrustFailure` with `!hasValidSslCertDir`, losing earlier failures from other trust steps and potentially returning `TrustLevel.Full` incorrectly.
### Issue Context
`sawTrustFailure` is an accumulator across dotnet/OpenSSL/NSS trust operations and drives the final `TrustLevel`.
### Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[408-413]
### Suggested change
- Replace `sawTrustFailure = !hasValidSslCertDir;` with `sawTrustFailure |= !hasValidSslCertDir;` (or `if (!hasValidSslCertDir) sawTrustFailure = true;`).
- Consider keeping a separate variable for “OpenSSL trust effective for external clients” if you want distinct reporting, but don’t lose prior failures.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+364
to
+366
| var existingDirs = existingSslCertDir.Split(Path.PathSeparator); | ||
| var certDirFullPath = Path.GetFullPath(prettyCertDir); | ||
| var isCertDirIncluded = existingDirs.Any(dir => |
There was a problem hiding this comment.
4. $home used for comparison 🐞 Bug ✓ Correctness
• The new SSL_CERT_DIR inclusion check compares Path.GetFullPath(prettyCertDir) even though prettyCertDir may contain the literal $HOME prefix for display. • This makes the comparison against real filesystem paths in SSL_CERT_DIR fail, causing false “not configured” guidance and forcing TrustLevel.Partial even when the env var is correctly set.
Agent prompt
### Issue description
The SSL_CERT_DIR validation uses `prettyCertDir` (which may contain a literal `$HOME` for display) for filesystem comparisons, causing false negatives.
### Issue Context
`prettyCertDir` is designed for user-friendly output; it should not be fed into `Path.GetFullPath` for equality checks.
### Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[354-366]
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[373-377]
### Suggested change
- Compute comparison target from the real path:
- `var certDirFullPath = Path.GetFullPath(certDir);`
- Keep logging/UX using `prettyCertDir`.
- (Optional) On Unix, consider using `StringComparison.Ordinal` rather than `OrdinalIgnoreCase` for path equality.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+127
to
+135
| var listener = new ReporterEventListener(reporter); | ||
| if (verbose.HasValue()) | ||
| { | ||
| var listener = new ReporterEventListener(reporter); | ||
| listener.EnableEvents(CertificateManager.Log, System.Diagnostics.Tracing.EventLevel.Verbose); | ||
| } | ||
| else | ||
| { | ||
| listener.EnableEvents(CertificateManager.Log, System.Diagnostics.Tracing.EventLevel.LogAlways); | ||
| } |
There was a problem hiding this comment.
5. Logalways output can crash 🐞 Bug ⛯ Reliability
• dotnet-dev-certs now enables CertificateManager.Log at EventLevel.LogAlways even when not
verbose.
• Event 111’s message uses a {2} placeholder but only writes two payload values;
ReporterEventListener formats messages with string.Format without handling FormatException,
which can crash the tool when that event is emitted.
Agent prompt
### Issue description
Event 111 uses `{2}` in its message but only supplies two payload arguments, and `ReporterEventListener` calls `string.Format` without catching `FormatException`.
### Issue Context
This becomes more likely to surface because Program now enables LogAlways events even when not `--verbose`.
### Fix Focus Areas
- src/Shared/CertificateGeneration/CertificateManager.cs[1307-1309]
- src/Tools/dotnet-dev-certs/src/ReporterEventListener.cs[21-35]
### Suggested change
- Correct Event 111 message to use `{1}` (envVarName) instead of `{2}`.
- (Optional hardening) Wrap the `string.Format(...)` call in a try/catch and fall back to the raw message/payload dump to avoid crashing on malformed event templates.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Benchmark PR from agentic-review-benchmarks#3