Decode user and password from env configured proxy #5

leipert · 2020-12-30T20:41:55Z

If someone sets an env variable defining a http_proxy, containing a
username / password with percent-encoded characters, then the resulting
base64 encoded auth header will be wrong.

For example, suppose a username is Y\X and the password is R%S] ?X.
Properly URL encoded the proxy url would be:

http://Y%5CX:R%25S%5D%20%3FX@proxy.example:8000

The resulting proxy auth header should be: WVxYOlIlU10gP1g=, but the
getters defined by ruby StdLib URI return a username Y%5CX and
password R%25S%5D%20%3FX, resulting in WSU1Q1g6UiUyNVMlNUQlMjAlM0ZY.
As a result the proxy will deny the request.

Please note that this is my first contribution to the ruby ecosystem, to
standard lib especially and I am not a ruby developer.

Sorry for that and a happy and healthy 2021!

References:

leipert · 2021-02-03T14:22:33Z

Switched to using CGI.unescape over URI.decode_www_form_component as per:

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52368#note_500425249

leipert · 2021-02-03T14:24:29Z

@hsbt / @nobu Could either of you review this contribution? Thank you in advance 🙇

Have also created this issue in the bug tracker: https://bugs.ruby-lang.org/issues/17542

nobu · 2021-02-05T07:29:11Z

lib/net/http.rb

@@ -1180,7 +1180,8 @@ def proxy_port
    # The username of the proxy server, if one is configured.
    def proxy_user
      if ENVIRONMENT_VARIABLE_IS_MULTIUSER_SAFE && @proxy_from_env
-        proxy_uri&.user
+        user = proxy_uri&.user
+        CGI.unescape(user) unless user.nil?


As cgi.rb isn't required here, it results in uninitialized constant Net::HTTP::CGI error.

And why unless user.nil? instead of if user?

@nobu Thanks for the review, changed it to if user and if pass respectively and added a require 'cgi' at the top.

Maybe require "cgi/escape" is enough?

@mame / @nobu Changed it to require "cgi/escape"!

Wouldn't it be require "cgi/util" though? And what is the difference between CGI::unescape and CGI.unescape

Changed it to require "cgi/util". Hope that is alright.

As for method calls, :: and . are equivalent.
. seems preferred in these days though.

And, as CGI is used only when proxy env is given, it can be deferred to that cases only.

Thank you @nobu. I don't understand the following aspect completely:

And, as CGI is used only when proxy env is given, it can be deferred to that cases only.

I don't know how the Ruby dependency loading works exactly (I was wondering that the tests didn't catch uninitialized constant Net::HTTP::CGI) you described in the first place.

Should I use autoload for the CGI module (autoload :CGI, 'cgi/util'), or should I potentially require it someplace else, for example in one of those places:

https://github.com/ruby/net-http/blob/master/lib/net/http.rb#L650-L655

https://github.com/ruby/net-http/blob/master/lib/net/http.rb#L1177-L1182

@nobu Moved back to the . syntax and moved the require to the conditionals.

arturoherrero · 2021-02-09T10:32:30Z

lib/net/http.rb

+        user = proxy_uri&.user
+        CGI::unescape(user) if user


Related to #5 (comment), I think they are referring to move the require 'cgi/util' L24 inside the conditions:

Suggested change

user = proxy_uri&.user

CGI::unescape(user) if user

user = proxy_uri&.user

if user

require 'cgi/util'

CGI::unescape(user)

end

An alternative to this is to extract a private method and re-use it in both places:

unescape(user) if user private def unescape(string) require 'cgi/util' CGI::unescape(string) end

Related to #5 (comment), I think they are referring to move the require 'cgi/util' L24 inside the conditions:

I agree on this.

If someone sets an env variable defining a http_proxy, containing a username / password with percent-encoded characters, then the resulting base64 encoded auth header will be wrong. For example, suppose a username is `Y\X` and the password is `R%S] ?X`. Properly URL encoded the proxy url would be: http://Y%5CX:R%25S%5D%20%3FX@proxy.example:8000 The resulting proxy auth header should be: `WVxYOlIlU10gP1g=`, but the getters defined by ruby StdLib `URI` return a username `Y%5CX` and password `R%25S%5D%20%3FX`, resulting in `WSU1Q1g6UiUyNVMlNUQlMjAlM0ZY`. As a result the proxy will deny the request. Please note that this is my first contribution to the ruby ecosystem, to standard lib especially and I am not a ruby developer. References: - https://gitlab.com/gitlab-org/gitlab/-/issues/289836 - https://bugs.ruby-lang.org/projects/ruby-master/repository/trunk/revisions/58461 - https://bugs.ruby-lang.org/issues/17542

leipert · 2021-02-17T10:19:06Z

@nobu / @mame I have moved the unescape and require to a private method. How do you feel about it?

jeremyevans

I'm in favor of this change, and the implementation seems fine.

leipert · 2021-03-11T08:22:22Z

Thank you @nobu / @mame / @jeremyevans / @arturoherrero! 👍

leipert force-pushed the leipert-add-decoding-to-proxy-env branch 2 times, most recently from aab98eb to 8e3fcf0 Compare December 30, 2020 20:43

leipert force-pushed the leipert-add-decoding-to-proxy-env branch 2 times, most recently from 18ea109 to a698159 Compare February 3, 2021 14:22

nobu requested changes Feb 5, 2021

View reviewed changes

leipert force-pushed the leipert-add-decoding-to-proxy-env branch 4 times, most recently from c6899e4 to 5b57481 Compare February 6, 2021 10:03

arturoherrero reviewed Feb 9, 2021

View reviewed changes

leipert force-pushed the leipert-add-decoding-to-proxy-env branch 2 times, most recently from 622e321 to 5d9ee8e Compare February 9, 2021 10:41

leipert force-pushed the leipert-add-decoding-to-proxy-env branch from 5d9ee8e to a3b1b67 Compare February 17, 2021 10:17

jeremyevans approved these changes Mar 1, 2021

View reviewed changes

nobu approved these changes Mar 8, 2021

View reviewed changes

jeremyevans merged commit e57d4f3 into ruby:master Mar 8, 2021

leipert deleted the leipert-add-decoding-to-proxy-env branch March 11, 2021 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode user and password from env configured proxy #5

Decode user and password from env configured proxy #5

leipert commented Dec 30, 2020 •

edited

Loading

leipert commented Feb 3, 2021

leipert commented Feb 3, 2021 •

edited

Loading

nobu Feb 5, 2021

nobu Feb 5, 2021

leipert Feb 5, 2021

mame Feb 6, 2021

leipert Feb 6, 2021

leipert Feb 6, 2021

leipert Feb 6, 2021

nobu Feb 7, 2021

leipert Feb 9, 2021 •

edited

Loading

leipert Feb 9, 2021

arturoherrero Feb 9, 2021

arturoherrero Feb 9, 2021

nobu Feb 10, 2021

leipert commented Feb 17, 2021

jeremyevans left a comment

leipert commented Mar 11, 2021

Decode user and password from env configured proxy #5

Decode user and password from env configured proxy #5

Conversation

leipert commented Dec 30, 2020 • edited Loading

leipert commented Feb 3, 2021

leipert commented Feb 3, 2021 • 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

leipert Feb 9, 2021 • 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

leipert commented Feb 17, 2021

jeremyevans left a comment

Choose a reason for hiding this comment

leipert commented Mar 11, 2021

leipert commented Dec 30, 2020 •

edited

Loading

leipert commented Feb 3, 2021 •

edited

Loading

leipert Feb 9, 2021 •

edited

Loading