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>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07d6e09522
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| sawTrustFailure = !hasValidSslCertDir; | ||
| } |
There was a problem hiding this comment.
This assignment resets sawTrustFailure based solely on hasValidSslCertDir. If NSS trust failed earlier (e.g., TryAddCertificateToNssDb failed), sawTrustFailure was already true; when SSL_CERT_DIR already includes the cert directory, this line flips it back to false and the method returns TrustLevel.Full. That misreports trust as full even though a client trust path failed. Consider preserving earlier failures (e.g., sawTrustFailure |= !hasValidSslCertDir) so prior failures aren’t cleared.
Useful? React with 👍 / 👎.
| ? 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 => |
There was a problem hiding this comment.
Here the comparison uses prettyCertDir, which substitutes $HOME for display. Path.GetFullPath("$HOME/...") resolves relative to the current working directory, so it won’t match an SSL_CERT_DIR entry like /home/user/... even when the directory is already configured. That leads to false “append” guidance and (via sawTrustFailure = !hasValidSslCertDir) can mark trust as partial even when SSL_CERT_DIR already includes the actual cert directory. Use certDir or expand $HOME before computing the full path.
Useful? React with 👍 / 👎.
2 participants
Benchmark PR from agentic-review-benchmarks#3