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

URI validation fails when it contains reserved characters #1214

Closed
luizkowalski opened this issue Dec 4, 2020 · 3 comments · Fixed by #1237
Closed

URI validation fails when it contains reserved characters #1214

luizkowalski opened this issue Dec 4, 2020 · 3 comments · Fixed by #1237

Comments

@luizkowalski
Copy link

Basic Info

  • Faraday Version: 1.1.0
  • Ruby Version: 2.7.1

Issue description

One of the service providers I'm using has URLs like this: https://service.com/service:search. This is how we setup Faraday:

class Provider::Client
 ...
 delegate :post, :patch, :delete, :put, to: :client

 def client
  Faraday.new(url: 'https://service.com/') do |faraday|
    faraday.headers = headers.merge(content_type: content_type)
    faraday.response(:json, content_type: CONTENT_TYPE)
    faraday.basic_auth(USER, PASSWORD)
    faraday.adapter(Faraday.default_adapter)
  end
 end
end

and when I invoke it

data = Provider::Client.post(
  "service:search?limit=#{LIMIT}&offset=#{offset}", search_params
)

but when I do this with the URL in question, I get this:

URI::InvalidURIError: query conflicts with opaque
from /Users/luiz/.rbenv/versions/2.7.1/lib/ruby/2.7.0/uri/generic.rb:832:in `query='

I assume it was something related to net/http adapter since it is the default adapter so I did the same call manually

require "uri"
require "net/http"

url = URI("https://service.com/service:search?limit=50&offset=400")

https = Net::HTTP.new(url.host, url.port);
https.use_ssl = true
request = Net::HTTP::Post.new(url)
request["Accept"] = "custom_header"
request["Authorization"] = "Basic xxx="
request["Content-Type"] = "application/json"
request.body = "long_json"
response = https.request(request)

puts response.read_body

and this actually works so I suspect that something in Faraday is validating this URL in a different way than net/http

@iMacTia
Copy link
Member

iMacTia commented Dec 4, 2020

Hi @luizkowalski, the exception is coming from URI and is raised in the first lines of the query= method:

def query=(v)
  return @query = nil unless v
  raise InvalidURIError, "query conflicts with opaque" if @opaque
  ...
end

Faraday does some internal magic on the request url by splitting and recombining it to help with some common transformations, and I presume at some point it may cause the @opaque to be set thanks to the presence of the : in the URL.
I've traced the issue down to this line:

uri = url ? base + url : base

Using your example code, base would be #<URI::HTTPS https://service.com/> and url would be service:search?limit=50&offset=400.
Now, interestingly enough, if the second parameter of the + method for URI contains a :, it seems like the result is quite unexpected!

2.7.1 > url
 => "service:search?limit=50&offset=400" 
2.7.1 > base
 => #<URI::HTTPS https://service.com/> 
2.7.1 > base + url
 => #<URI::Generic service:search?limit=50&offset=400>

For some reason, the base is removed and only the url is left.
The fix is quite easy in this case, it's enough to just prepend a / to the url and things work as expected:

data = Provider::Client.post(
  "/service:search?limit=#{LIMIT}&offset=#{offset}", search_params
)

This however doesn't work in all cases as you may need to append a relative path to the base which can't start with /.
Please give this a try by prepending the / and let me know if that works, but I'll leave this issue open to have a look and find a possible way to fix that line of code in Faraday so that it works in this case.
If you want to give it a try yourself I'd happily review a PR 😃

@luizkowalski
Copy link
Author

luizkowalski commented Dec 7, 2020

@iMacTia it worked now. The base URL is: https://service.com/api. When I added the / to service:search I got an error and noticed that the /api was gone from the base URL so I added /api/service:search and now it's fine

@iMacTia
Copy link
Member

iMacTia commented Dec 7, 2020

@luizkowalski perfect, yes that's exactly what I meant with

This however doesn't work in all cases as you may need to append a relative path to the base which can't start with /.

As faraday assumes the provided URL is absolute if it starts with / and removed the path provided to the base.
Glad you already found the workaround, but this should still be fixed properly so that workaround is not necessary anymore

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

Successfully merging a pull request may close this issue.

2 participants