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

escape colon in path segment #1237

Merged
merged 5 commits into from Jan 18, 2021
Merged

Conversation

yarafan
Copy link
Contributor

@yarafan yarafan commented Jan 12, 2021

Description

Considering example

f =  Faraday.new(url: 'https://service.com/') do |faraday|
    faraday.adapter(Faraday.default_adapter)
  end
f.post('service:search')

URI.parse parses relative path service:search as opaque_part (according to https://tools.ietf.org/html/rfc2396#section-3) (eg mailto:foo@example.com), so Faraday::Connection#build_exclusive_url produces wrong URI as result and passing query params leads to InvalidURIError

irb(main):004:0> URI.parse('mailto:foo@example.com').opaque
=> "foo@example.com"
irb(main):005:0> URI.parse('service:search').opaque
=> "search"

Fixes #1214

Todos

List any remaining work that needs to be done, i.e:

  • Rubocop (not sure what to do with ClassLength)

uri = url ? base + url : base
uri = URI.parse(CGI.unescape(uri.to_s))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether final uri should be unescaped

Copy link
Member

@iMacTia iMacTia Jan 13, 2021

Choose a reason for hiding this comment

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

That's a good point, this may cause issues in case the URL contains escaped characters like spaces? Escaped columns may also be part of the URL, so I'm not sure unescaping here is a good idea at all.
E.g. Faraday.get('https://service.com/path%20with%20spaces')

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

I like the approach and even more the test coverage! But unfortunately the escaping solution might not be the best one 🤔. I'm curious to see if other tests are passing (I'd expect at least one to fail, if we're testing spaces!), but Rubocop is in the way...

The ClassLength cop is a tricky one, so feel free to increase the limit in the .rubocop_todo.yml file for this time. Since you added 2 lines that should be 236 now, I guess?

uri = url ? base + url : base
uri = URI.parse(CGI.unescape(uri.to_s))
Copy link
Member

@iMacTia iMacTia Jan 13, 2021

Choose a reason for hiding this comment

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

That's a good point, this may cause issues in case the URL contains escaped characters like spaces? Escaped columns may also be part of the URL, so I'm not sure unescaping here is a good idea at all.
E.g. Faraday.get('https://service.com/path%20with%20spaces')

@iMacTia
Copy link
Member

iMacTia commented Jan 13, 2021

@yarafan this looks good now, but is your service call working as expected even with the : escaped?
@luizkowalski maybe you want to have a quick look as well?

@yarafan
Copy link
Contributor Author

yarafan commented Jan 13, 2021

@iMacTia I don't know any service with such uri. I tried to access uri in issue and got 302.
Would be great if @luizkowalski also check this case

@luizkowalski
Copy link

@yarafan @iMacTia wow I disabled email from comments from GitHub (cause I get too many) so I missed this one, guys, so sorry! I will check tomorrow morning first thing and will come back to you!

Thanks for the PR!

@luizkowalski
Copy link

Hey guys!

I can confirm it works and this is how the URL appeared on the logs

INFO  Rails : request: POST https://subdomain.service.com/api/loans%3Asearch?limit=50&offset=2900

@iMacTia iMacTia merged commit 0ed6480 into lostisland:master Jan 18, 2021
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.

URI validation fails when it contains reserved characters
3 participants