Support verify_hostname #25
Conversation
| @@ -184,6 +184,7 @@ def request_with_wrapped_block(http, env) | |||
| ssl_version: :version, | |||
| min_version: :min_version, | |||
| max_version: :max_version | |||
| verify_hostname: :verify_hostname | |||
There was a problem hiding this comment.
Faraday defaults this option to true, so we don't need to hardcode any default in the adapter.
stefanmb
force-pushed
the
stefanmb/verify_hostname
branch
from
640f7c9 to
17e8020
Compare
faraday-net_http_persistent.gemspec
Outdated
| @@ -25,5 +25,5 @@ Gem::Specification.new do |spec| | |||
| spec.require_paths = ["lib"] | |||
|
|
|||
| spec.add_dependency "faraday", "~> 2.5" | |||
| spec.add_dependency "net-http-persistent", "~> 4.0" | |||
| spec.add_dependency "net-http-persistent", "~> 4.0.4" | |||
There was a problem hiding this comment.
This change will also lock the version to be <= 4.1, which is probably too restrictive.
If we need 4.0.4 as a minimum version, but still want to allow any version until 5.0, we could do either:
"~> 4.0", ">= 4.0.4"; or">= 4.0.4", "< 5"
There was a problem hiding this comment.
The alternative is to leave ~> 4.0 as the min version and change the code to support both version (maybe with a respond_to? check. If we do this, you can disregard my other comment on specs.
Personally tough, I don't think it's gonna be a big ask to ask users to upgrade form 4.0+ to 4.0.4+, so happy to keep the higher version constraint
There was a problem hiding this comment.
happy to keep the higher version constraint
Done, I've opted for ">= 4.0.4", "< 5" since it's very literal.
| @@ -50,7 +62,7 @@ | |||
| http = adapter.send(:connection, url: url, request: {}) | |||
| adapter.send(:configure_ssl, http, min_version: :TLS1_2) | |||
|
|
|||
| # `min_version` is only present in net_http_persistent >= 3.1 (UNRELEASED) | |||
| # `min_version` is only present in net_http_persistent >= 3.1 | |||
There was a problem hiding this comment.
Since we now depend on Net::HTTP::Persistent v4+, we don't need this respond_to?(:min_version) check anymore 🙌
There was a problem hiding this comment.
Done, removed all the respond_to? checks for old net_http_persistent versions and their respective comments.
| adapter.send(:configure_ssl, http, verify_hostname: false) | ||
|
|
||
| # `verify_hostname` is only present in net_http_persistent >= 4.0.4 | ||
| expect(http.verify_hostname).to eq(false) if http.respond_to?(:verify_hostname) |
There was a problem hiding this comment.
If we're depending on v4.0.4+, we don't need to check for the presence of verify_hostname
There was a problem hiding this comment.
Done, removed all the respond_to? checks for old net_http_persistent versions and their respective comments.
The SSL option
verify_hostnameis now supported in net-http-persistent 4.0.4:This trivial PR passes through the
verify_hostnameoption so this feature can be used.Would it be possible to get a new version of the adapter released? @olleolleolle @iMacTia