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>
📝 WalkthroughWalkthroughThese changes enhance certificate validation by parameterizing environment variable names in log messages, introducing SSL_CERT_DIR validation logic in Unix certificate management that checks whether the OpenSSL certificate directory is already configured, and refactoring event listener initialization in the CLI tool. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Shared/CertificateGeneration/UnixCertificateManager.cs`:
- Around line 358-409: When checking whether the cert dir is already in
SSL_CERT_DIR, expand ~ and environment variables before calling Path.GetFullPath
(e.g., compute an expandedCertDir =
Environment.ExpandEnvironmentVariables(prettyCertDir) and if it starts with "~"
replace that prefix with
Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) before calling
Path.GetFullPath to produce certDirFullPath), and when updating sawTrustFailure
do not overwrite earlier failures—use a logical OR to preserve prior failures
(e.g., sawTrustFailure = sawTrustFailure || !hasValidSslCertDir or
sawTrustFailure |= !hasValidSslCertDir) instead of sawTrustFailure =
!hasValidSslCertDir; reference the symbols certDirFullPath, prettyCertDir,
existingSslCertDir, hasValidSslCertDir, TryGetOpenSslDirectory, and
sawTrustFailure.
Comment on lines
+358
to
409
| 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.
Preserve earlier trust failures and compare against the real certDir path.
Two issues here:
certDirFullPathis derived fromprettyCertDir, which may contain$HOME, causing false negatives when checking existingSSL_CERT_DIRentries.sawTrustFailure = !hasValidSslCertDiroverwrites prior failure state (e.g., .NET/NSS failures), which can incorrectly returnTrustLevel.Full.
🔧 Suggested fix
- var certDirFullPath = Path.GetFullPath(prettyCertDir);
+ var certDirFullPath = Path.GetFullPath(certDir);
...
- sawTrustFailure = !hasValidSslCertDir;
+ sawTrustFailure |= !hasValidSslCertDir;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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; | |
| } | |
| 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(certDir); | |
| 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; | |
| } |
🤖 Prompt for AI Agents
In `@src/Shared/CertificateGeneration/UnixCertificateManager.cs` around lines 358
- 409, When checking whether the cert dir is already in SSL_CERT_DIR, expand ~
and environment variables before calling Path.GetFullPath (e.g., compute an
expandedCertDir = Environment.ExpandEnvironmentVariables(prettyCertDir) and if
it starts with "~" replace that prefix with
Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) before calling
Path.GetFullPath to produce certDirFullPath), and when updating sawTrustFailure
do not overwrite earlier failures—use a logical OR to preserve prior failures
(e.g., sawTrustFailure = sawTrustFailure || !hasValidSslCertDir or
sawTrustFailure |= !hasValidSslCertDir) instead of sawTrustFailure =
!hasValidSslCertDir; reference the symbols certDirFullPath, prettyCertDir,
existingSslCertDir, hasValidSslCertDir, TryGetOpenSslDirectory, and
sawTrustFailure.
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
3 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
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores