UPSTREAM PR #17458: fix(http): apply TLS certificate verification to connection pool#37
Open
UPSTREAM PR #17458: fix(http): apply TLS certificate verification to connection pool#37
Conversation
The http_client_pool() function was missing TLS configuration, causing pooled HTTPS connections (via --pool flag) to potentially skip certificate verification. This differs from http_client() which properly applies tls_config(). This fix: - Adds tls_config(false) to enable certificate verification by default - Changes return type to Result<Arc<Agent>, ShellError> to handle potential TLS initialization errors - Updates all call sites to propagate errors Security impact: Without this fix, `http get --pool https://...` could connect to servers with invalid certificates without warning, enabling potential MITM attacks. Users who need to disable verification for pooled connections should use `http pool --insecure` to explicitly reset the pool. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
loci-dev
had a problem deploying
to
PROD__AL_DEMO
GitHub Actions
Failure
loci-dev
force-pushed
the
main
branch
22 times, most recently
from
da22582 to
3f38f88
Compare
loci-dev
force-pushed
the
main
branch
6 times, most recently
from
cd989c3 to
64bcc48
Compare
loci-dev
force-pushed
the
main
branch
3 times, most recently
from
6102c29 to
d8ed90c
Compare
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.
Note
Source pull request: nushell/nushell#17458
Description
The
http_client_pool()function was missing TLS configuration, causing pooled HTTPS connections (via--poolflag) to not verify certificates. This fix aligns the pooled client behavior with the regularhttp_client()which properly appliestls_config().Changes:
tls_config(false)to enable certificate verification by defaultResult<Arc<Agent>, ShellError>to handle potential TLS initialization errors?Background: I asked Claude to review the nushell HTTP code. It identified this inconsistency and implemented the fix. I don't have deep Rust expertise, so I'd appreciate review from maintainers familiar with this area.
User-Facing Changes
Users of
http get --pool,http post --pool, etc. will now have TLS certificate verification enabled by default, matching the behavior of non-pooled requests.Tests + Formatting
cargo fmt --all -- --checkpassescargo clippy --workspace -- -D warnings -D clippy::unwrap_usedpassesAfter Submitting
N/A
Release notes summary
Fixed an inconsistency where
httpcommands with--poolflag were not applying TLS certificate verification. Pooled HTTPS connections now properly validate certificates, matching the behavior of regular (non-pooled) requests.