lostisland/faraday

Passing a URL with embedded basic auth is broken

iMacTia opened this issue ยท 6 comments

The recent authentication middleware refactoring broke passing a URL with embedded basic auth in the user info part of the URI, because basic_auth is still used in url_prefix=

with_uri_credentials(uri) do |user, password|
basic_auth user, password
uri.user = uri.password = nil
end

~/src/github.com/lostisland/faraday (1.x) $ ruby -Ilib -rfaraday -rfaraday/net_http -rjson -e "puts JSON.load(Faraday.new(url: 'https://user:password@httpbin.org/headers').get.body)['headers']['Authorization']"
WARNING: `Faraday::Connection#basic_auth` is deprecated; it will be removed in version 2.0.
While initializing your connection, use `#request(:basic_auth, ...)` instead.
See https://lostisland.github.io/faraday/middleware/authentication for more usage info.
Basic dXNlcjpwYXNzd29yZA==
~/src/github.com/lostisland/faraday (main) $ ruby -Ilib -rfaraday -rfaraday/net_http -rjson -e "puts JSON.load(Faraday.new(url: 'https://user:password@httpbin.org/headers').get.body)['headers']['Authorization']"
lib/faraday/connection.rb:365:in `block in url_prefix=': undefined method `basic_auth' for #<Faraday::Connection:0x00007fed250abaa8 @parallel_manager=nil, @headers={}, @params={}, @options=#<Faraday::RequestOptions (empty)>, @ssl=#<Faraday::SSLOptions (empty)>, @default_parallel_manager=nil, @manual_proxy=nil, @builder=#<Faraday::RackBuilder:0x00007fed25092210 @adapter=Faraday::Adapter::NetHttp, @handlers=[Faraday::Request::UrlEncoded]>, @url_prefix=#<URI::HTTPS https://user:password@httpbin.org/headers>> (NoMethodError)
	from lib/faraday/connection.rb:506:in `with_uri_credentials'
	from lib/faraday/connection.rb:364:in `url_prefix='
	from lib/faraday/connection.rb:84:in `initialize'
	from lib/faraday.rb:96:in `new'
	from lib/faraday.rb:96:in `new'
	from -e:1:in `<main>'

A test demonstrating this is available here: 1.x...etiennebarrie:test-basic-auth-in-url:

it 'uses User Information from the URI for Basic authentication' do
  conn.url_prefix = 'http://user:password@sushi.com'
  expect(conn.url_prefix.to_s).to eq('http://sushi.com/')
  request = conn.build_request(:get)
  expect(request.headers['Authorization']).to eq("Basic #{Base64.strict_encode64('user:password')}")
end

We should decide if we want to fix this or remove support for this feature.

See comment by @etiennebarrie in #1308 (comment)_

@etiennebarrie I'm quoting your thoughts from the comment:

While I'm not against deprecating basic_auth and authorization on Connection, I think being able to have basic auth embedded in the URL is really useful (it can be configured in secrets for example).

I'm seriously thinking of removing the support for this.
It seems like this practice have been deprecated back in 2005 and most of the major browsers have been dropping its support as well.

I'm also curious about your example above:

it can be configured in secrets for example
I don't see how using the basic_auth middleware would not allow to use secrets?
Wouldn't something like this work?

# Faraday 1.x
conn = Faraday.new(url_with_no_userinfo) do |f|
  conn.request :basic_auth, Secrets.basic_auth_user, Secrets.basic_auth_pass
  ...
end

We basically do:

def connection
  Faraday.new(url: server) do
    # other config
  end
end

def server
  if global?
    secrets.global
  elsif something?
    secrets.other
  elsif something_else?
    secrets.another
  end
end

So we just need one secret per URL/user/password.


We can totally split each secret into three, or even keep the single URL secret but extract the user password before passing it down to Faraday.

Yes that's what I thought, thanks for confirming @etiennebarrie ๐Ÿ‘

Hi, here it is another use case: elastic/elasticsearch-ruby#1479

Thanks for the input @tagliala, ultimately we want to please the community, so it's important to understand how much this feature is used in order to decide about its future.

This is obviously a widespread library, so many thanks for pointing it out

@etiennebarrie main branch is now fixed and supports passing basic auth as part of the url again.
I'm also happy to confirm that I'm not currently planning on removing this feature anymore as the maintenance burden is quite small ๐ŸŽ‰.

@tagliala the deprecation warning has also been fixed and I've just released v1.7.2 ๐Ÿ™Œ