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

proxy: add support for scheme prefixes socks4:// and socks5:// in http_proxy environment variable #157

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dpocock
Copy link

@dpocock dpocock commented Feb 29, 2020

No description provided.

@codecov
Copy link

codecov bot commented Feb 29, 2020

Codecov Report

Merging #157 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
+ Coverage   75.64%   75.74%   +0.09%     
==========================================
  Files           8        8              
  Lines        2616     2626      +10     
==========================================
+ Hits         1979     1989      +10     
  Misses        637      637
Impacted Files Coverage Δ
python2/httplib2/__init__.py 82.98% <100%> (+0.08%) ⬆️
python3/httplib2/__init__.py 83.93% <100%> (+0.08%) ⬆️
python2/httplib2/socks.py 43.69% <0%> (ø) ⬆️
python2/httplib2/iri2uri.py 62.5% <0%> (ø) ⬆️
python2/httplib2/certs.py 86.2% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6746342...c188ae2. Read the comment docs.

Copy link
Member

@temoto temoto left a comment

Choose a reason for hiding this comment

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

Great idea, please add tests.

Comment on lines 1127 to +1131
proxy_type = 3 # socks.PROXY_TYPE_HTTP
if len(url.scheme) > 0:
_scheme_prefix = url.scheme.lower()
if _scheme_prefix in proxy_schemes:
proxy_type = proxy_schemes[_scheme_prefix]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
proxy_type = 3 # socks.PROXY_TYPE_HTTP
if len(url.scheme) > 0:
_scheme_prefix = url.scheme.lower()
if _scheme_prefix in proxy_schemes:
proxy_type = proxy_schemes[_scheme_prefix]
proxy_type = proxy_schemes.get(url.scheme.lower(), 3) # socks.PROXY_TYPE_HTTP

@@ -74,6 +74,8 @@
ssl_SSLError = getattr(ssl, "SSLError", None)
ssl_CertificateError = getattr(ssl, "CertificateError", None)

# socks.PROXY_TYPE_SOCKS4, socks.PROXY_TYPE_SOCKS5, socks.PROXY_TYPE_HTTP
proxy_schemes = { 'socks4' : 1, 'socks5' : 2, 'http' : 3 }
Copy link
Member

Choose a reason for hiding this comment

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

Please use what black --diff python2/ python3/ says here

Copy link
Member

Choose a reason for hiding this comment

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

Please put this after socks import or somewhere not between two ssl related blocks.

@dpocock
Copy link
Author

dpocock commented Mar 1, 2020

Thanks for taking the time to look at this so quickly and provide feedback. I agree with your suggestions but may not have time to make all these changes immediately. As the patch is quite small, would you consider accepting it without unit tests? I'm happy to move the definition of proxy_schemes to a better position as you request.

@dpocock
Copy link
Author

dpocock commented Mar 1, 2020

FYI, I tested this against the tor daemon running on the same host as the Python process.

I set this in my environment:

export http_proxy=socks5://127.0.0.1:9050

and it works well.

@temoto
Copy link
Member

temoto commented Mar 1, 2020

No worries, take your time. Tests are required not because I don't trust that it works, but because it should continue to work with all future changes. Luckily, there are good examples of checking proxy settings from environment, see here https://github.com/httplib2/httplib2/blob/master/tests/test_proxy.py

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.

None yet

2 participants