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))

This comment has been minimized.

@yarafan

yarafan Jan 12, 2021
Author Contributor

Not sure whether final uri should be unescaped

This comment has been minimized.

@iMacTia

iMacTia Jan 13, 2021
Member

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

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))

This comment has been minimized.

@iMacTia

iMacTia Jan 13, 2021
Member

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 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 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

@luizkowalski luizkowalski commented Jan 17, 2021

@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

@luizkowalski luizkowalski commented Jan 18, 2021

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
7 checks passed
7 checks passed
@github-actions
linting
Details
@github-actions
build 2.4
Details
@github-actions
build 2.5
Details
@github-actions
build 2.6
Details
@github-actions
build 2.7
Details
@github-actions
build 3.0
Details
codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants