Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net_http adapter: Fix to avoid calling configure_ssl for HTTP connections #38

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

ma2gedev
Copy link
Contributor

env[:ssl] is an instance of Faraday::SSLOptions, and as a result, configure_ssl is always executed, even when the connection is to HTTP. This MR fixes the code so that SSL configuration is only performed when necessary, similar to other adapters such as faraday-httpclient and faraday-patron.

Details can be found in #37 .

`env[:ssl]` is an instance of `Faraday::SSLOptions`, and as a result,
`configure_ssl` is always executed, even when the connection is to HTTP.
Fixes lostisland#37
@ma2gedev
Copy link
Contributor Author

I've found that the bug also exists in previous versions such as 1.0.1. I'm not sure about the maintenance policy of this library, but I think it would be beneficial to backport the fix to those versions as well. What do you think?

@@ -42,8 +42,9 @@ def initialize(app = nil, opts = {}, &block)

def build_connection(env)
net_http_connection(env).tap do |http|
http.use_ssl = env[:url].scheme == 'https' if http.respond_to?(:use_ssl=)
configure_ssl(http, env[:ssl])
if env[:url].scheme == 'https' && env[:ssl]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: What if this is even smaller? Making the smallest check first.

Suggested change
if env[:url].scheme == 'https' && env[:ssl]
if env[:ssl] && env[:url].scheme == 'https'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, it's not important!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olleolleolle Thank you so much!

@olleolleolle olleolleolle changed the title Fix conditions for calling configure_ssl net_http adapter: Fix conditions for calling configure_ssl Jul 24, 2024
@olleolleolle olleolleolle changed the title net_http adapter: Fix conditions for calling configure_ssl net_http adapter: Fix conditions for calling configure_ssl when using a HTTP connection Jul 24, 2024
@olleolleolle olleolleolle changed the title net_http adapter: Fix conditions for calling configure_ssl when using a HTTP connection net_http adapter: Fix to avoid calling configure_ssl for HTTP connections Jul 24, 2024
@olleolleolle olleolleolle merged commit 417da45 into lostisland:main Jul 24, 2024
@ma2gedev ma2gedev deleted the fix/configure-ssl branch July 24, 2024 07:55
@olleolleolle
Copy link
Member

@ma2gedev If you can make a backport, I think I can make a release.

@olleolleolle
Copy link
Member

Right, now https://rubygems.org/gems/faraday-net_http/versions/3.1.1 exists.

Since CI was stopped, you had no way of seeing the lint issue which was easy to fix. I made the CI start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants