Ensure SSL_CERT_DIR messages are always shown and check for existing value#26
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. prettyCertDir used for fullpath
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
355
to
409
| ? Path.Combine("$HOME", certDir[homeDirectoryWithSlash.Length..]) | ||
| : certDir; | ||
|
|
||
| if (TryGetOpenSslDirectory(out var openSslDir)) | ||
| var hasValidSslCertDir = false; | ||
|
|
||
| // Check if SSL_CERT_DIR is already set and if certDir is already included | ||
| var existingSslCertDir = Environment.GetEnvironmentVariable(OpenSslCertificateDirectoryVariableName); | ||
| if (!string.IsNullOrEmpty(existingSslCertDir)) | ||
| { | ||
| var existingDirs = existingSslCertDir.Split(Path.PathSeparator); | ||
| var certDirFullPath = Path.GetFullPath(prettyCertDir); | ||
| var isCertDirIncluded = existingDirs.Any(dir => | ||
| { | ||
| if (string.IsNullOrWhiteSpace(dir)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| return string.Equals(Path.GetFullPath(dir), certDirFullPath, StringComparison.OrdinalIgnoreCase); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore invalid directory entries in SSL_CERT_DIR | ||
| return false; | ||
| } | ||
| }); | ||
|
|
||
| if (isCertDirIncluded) | ||
| { | ||
| // The certificate directory is already in SSL_CERT_DIR, no action needed | ||
| Log.UnixOpenSslCertificateDirectoryAlreadyConfigured(prettyCertDir, OpenSslCertificateDirectoryVariableName); | ||
| hasValidSslCertDir = true; | ||
| } | ||
| else | ||
| { | ||
| // SSL_CERT_DIR is set but doesn't include our directory - suggest appending | ||
| Log.UnixSuggestAppendingToEnvironmentVariable(prettyCertDir, OpenSslCertificateDirectoryVariableName); | ||
| hasValidSslCertDir = false; | ||
| } | ||
| } | ||
| else if (TryGetOpenSslDirectory(out var openSslDir)) | ||
| { | ||
| Log.UnixSuggestSettingEnvironmentVariable(prettyCertDir, Path.Combine(openSslDir, "certs"), OpenSslCertificateDirectoryVariableName); | ||
| hasValidSslCertDir = false; | ||
| } | ||
| else | ||
| { | ||
| Log.UnixSuggestSettingEnvironmentVariableWithoutExample(prettyCertDir, OpenSslCertificateDirectoryVariableName); | ||
| hasValidSslCertDir = false; | ||
| } | ||
|
|
||
| sawTrustFailure = !hasValidSslCertDir; | ||
| } |
There was a problem hiding this comment.
1. prettycertdir used for fullpath 📘 Rule violation ✓ Correctness
• The code computes certDirFullPath from prettyCertDir, which may be rewritten to start with $HOME for display purposes. • When prettyCertDir contains $HOME, Path.GetFullPath(prettyCertDir) will not expand $HOME and can resolve to an unrelated path, causing isCertDirIncluded to be computed incorrectly. • This breaks edge-case handling for common home-directory installs and can incorrectly mark trust as failed (sawTrustFailure = !hasValidSslCertDir).
Agent prompt
## Issue description
`Path.GetFullPath(prettyCertDir)` uses a display-friendly `$HOME`-prefixed string to compute `certDirFullPath`, which can mis-resolve paths and incorrectly decide whether `SSL_CERT_DIR` already includes the certificate directory.
## Issue Context
`prettyCertDir` is intended for messaging, but it is used for filesystem comparison (`certDirFullPath`). On Unix, `$HOME` is not expanded by `GetFullPath`, so the comparison can fail even when `SSL_CERT_DIR` is correctly configured.
## Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[354-409]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+408
to
409
| sawTrustFailure = !hasValidSslCertDir; | ||
| } |
There was a problem hiding this comment.
2. Trustlevel can be wrong 🐞 Bug ✓ Correctness
• TrustCertificateCore accumulates failures across .NET/OpenSSL/NSS trust steps, but the new SSL_CERT_DIR block overwrites sawTrustFailure based only on the env-var check. • If NSS trust (or .NET trust) failed earlier but SSL_CERT_DIR is configured, the final result may incorrectly become TrustLevel.Full. • This can also create inconsistent output (logging “partially succeeded” but returning Full).
Agent prompt
### Issue description
`TrustCertificateCore` overwrites `sawTrustFailure` based only on the SSL_CERT_DIR check, losing prior failures from other trust mechanisms (e.g., NSS trust failures). This can cause incorrect `TrustLevel.Full` results.
### Issue Context
`sawTrustFailure` is used as the final gate for returning `TrustLevel.Partial` vs `TrustLevel.Full`. It must represent *any* failures encountered across all trust targets.
### Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[346-414]
ⓘ 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