RestClient: Support percent encoded proxy user/pass #18105

jrafanie · 2018-10-17T18:04:54Z

https://bugzilla.redhat.com/show_bug.cgi?id=1566615

Monkey patch RestClient for patch releases of rest-client 2.0.0.
The underlying method, net_http_object, has not been modified since 2015
so it should be relatively safe to monkey patch this until the upstream
PR gets merged/released.

Upstream PR: rest-client/rest-client#665

To verify this works, run bin/rails console:

Before:

irb(main):004:0> r = RestClient::Request.new(method: :get, url:
'http://127.0.0.1/api', proxy: 'http://%24myuser:%24%[email protected]')
=> <RestClient::Request @method="get", @url="http://127.0.0.1/api">=>
<RestClient::Request @method="get", @url="https://127.0.0.1/api">
irb(main):002:0> r.net_http_object('host', 80).proxy_user
=> "%24myuser"
irb(main):003:0> r.net_http_object('host', 80).proxy_pass
=> "%24%3Fxxxx"

After:

irb(main):004:0> r = RestClient::Request.new(method: :get, url:
'http://127.0.0.1/api', proxy: 'http://%24myuser:%24%[email protected]')
=> <RestClient::Request @method="get", @url="http://127.0.0.1/api">=>
<RestClient::Request @method="get", @url="https://127.0.0.1/api">
irb(main):002:0> r.net_http_object('host', 80).proxy_user
=> "$myuser"
irb(main):003:0> r.net_http_object('host', 80).proxy_pass
=> "$?xxxx"

jrafanie · 2018-10-17T18:11:52Z

lib/patches/rest_client_patch.rb

+require 'restclient/version'
+if RestClient::VERSION >= "2.0.0" && RestClient::VERSION <= "2.1.0"
+  require 'restclient/request'
+  RestClient::Request.module_eval do


Note, this autoloads rest client regardless if you're going to use it. I'm open to suggestions on how to avoid this.

Should we prepend the class like we do with our other patches?

I'm trying to remember why Matthew convinced us a few years ago to use the module_eval thing instead of prepend. Either way, I think I'd be able to remove the require above it but both approaches would still autoload the constant for all processes.

Yeah, nothing we can do about requiring it all the time.

This will monkey patch the module itself. A prepend will override the parent of this class.
Pretty sure we want don't want prepend here.

reading the comments, I did not realize that we are monkey patching rest-client code onto rest-client because they haven't pulled the trigger in July 2017

yeah, good point @kbrock regarding monkey patch vs. prepend. I don't think it matters here but module_eval seems better for replacing a broken method vs. getting in "front" of it.

Yeah, rest-client is mostly dead. I've reached out to see if we can help maintain it but have heard nothing. Perhaps, we should fork it permanently at some point in the future. I don't know enough of it to be comfortable making changes throughout the codebase.

I'll verify I can remove the require line too.

I can't remove the require line since it doesn't autoload rest-client's request.

lib/vmdb_helper.rb

@method

https://bugzilla.redhat.com/show_bug.cgi?id=1566615 Monkey patch RestClient for patch releases of rest-client 2.0.0. The underlying method, net_http_object, has not been modified since 2015 so it should be relatively safe to monkey patch this until the upstream PR gets merged/released. Upstream PR: rest-client/rest-client#665 To verify this works, run bin/rails console: Before: ``` irb(main):004:0> r = RestClient::Request.new(method: :get, url: 'http://127.0.0.1/api', proxy: 'http://%24myuser:%24%[email protected]') => <RestClient::Request @method="get", @url="http://127.0.0.1/api">=> <RestClient::Request @method="get", @url="https://127.0.0.1/api"> irb(main):002:0> r.net_http_object('host', 80).proxy_user => "%24myuser" irb(main):003:0> r.net_http_object('host', 80).proxy_pass => "%24%3Fxxxx" ``` After: ``` irb(main):004:0> r = RestClient::Request.new(method: :get, url: 'http://127.0.0.1/api', proxy: 'http://%24myuser:%24%[email protected]') => <RestClient::Request @method="get", @url="http://127.0.0.1/api">=> <RestClient::Request @method="get", @url="https://127.0.0.1/api"> irb(main):002:0> r.net_http_object('host', 80).proxy_user => "$myuser" irb(main):003:0> r.net_http_object('host', 80).proxy_pass => "$?xxxx" ```

jrafanie · 2018-10-19T13:18:15Z

Ok, ready for final review.

miq-bot · 2018-10-19T13:20:48Z

Checked commit jrafanie@d2b0e32 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

JPrause · 2018-10-19T14:18:59Z

@miq-bot add_label blocker

…ent_encoded_proxy_user_password RestClient: Support percent encoded proxy user/pass (cherry picked from commit 5a784c3) https://bugzilla.redhat.com/show_bug.cgi?id=1566615

simaishi · 2018-10-19T20:04:05Z

Hammer backport details:

$ git log -1
commit 81bec89af724da633bfa316f40e04cca8f55e2de
Author: Brandon Dunne <[email protected]>
Date:   Fri Oct 19 10:19:26 2018 -0400

    Merge pull request #18105 from jrafanie/rest_client_monkey_patch4percent_encoded_proxy_user_password
    
    RestClient: Support percent encoded proxy user/pass
    (cherry picked from commit 5a784c39bfc87d4d281259061b7578ce140d0283)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1566615

Restclient as of version 2.1.0 uses provided credentials stored in a proxy URI object verbatim. If the credentials are escaped as they should be, then this leads to errors when we try to use the credentials because restclient sends the escaped form. There is a PR[1] in upstream restclient, which addresses this issue, but last movement there was in October 2018. ManageIQ folks decided to monkey patch restclient when they ran into the same issue, this commit is a loose adaptation of the original[1] and ManageIQ patches[2]. [1] - rest-client/rest-client#665 [2] - ManageIQ/manageiq#18105

jrafanie added bug core gaprindashvili/no labels Oct 17, 2018

jrafanie commented Oct 17, 2018

View reviewed changes

bdunne reviewed Oct 18, 2018

View reviewed changes

lib/vmdb_helper.rb Show resolved Hide resolved

jrafanie force-pushed the rest_client_monkey_patch4percent_encoded_proxy_user_password branch from d00e1f4 to d2b0e32 Compare October 19, 2018 13:17

bdunne approved these changes Oct 19, 2018

View reviewed changes

miq-bot added the blocker label Oct 19, 2018

bdunne merged commit 5a784c3 into ManageIQ:master Oct 19, 2018

bdunne added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 19, 2018

bdunne self-assigned this Oct 19, 2018

jrafanie deleted the rest_client_monkey_patch4percent_encoded_proxy_user_password branch October 19, 2018 14:23

simaishi added hammer/backported and removed hammer/yes labels Oct 19, 2018

jrafanie mentioned this pull request Feb 20, 2019

Add proxy URI schema #17318

Closed

adamruzicka mentioned this pull request Mar 24, 2021

Fixes #32192 - Handle special characters in HTTP proxy credentials theforeman/foreman#8412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RestClient: Support percent encoded proxy user/pass #18105

RestClient: Support percent encoded proxy user/pass #18105

jrafanie commented Oct 17, 2018 •

edited

Loading

jrafanie Oct 17, 2018

bdunne Oct 18, 2018

jrafanie Oct 18, 2018

bdunne Oct 18, 2018

kbrock Oct 18, 2018

jrafanie Oct 19, 2018

jrafanie Oct 19, 2018

jrafanie Oct 19, 2018

jrafanie commented Oct 19, 2018

miq-bot commented Oct 19, 2018

JPrause commented Oct 19, 2018

simaishi commented Oct 19, 2018

RestClient: Support percent encoded proxy user/pass #18105

RestClient: Support percent encoded proxy user/pass #18105

Conversation

jrafanie commented Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Oct 19, 2018

miq-bot commented Oct 19, 2018

JPrause commented Oct 19, 2018

simaishi commented Oct 19, 2018

jrafanie commented Oct 17, 2018 •

edited

Loading