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.

Sign up for GitHub

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

Jump to bottom

Get coverage back to 100% #1726

Merged
merged 3 commits into from Nov 22, 2019
Merged

Get coverage back to 100% #1726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c109b24
Cover AppEngine methods about environ
pquentin Oct 31, 2019
14db804
Assume that _encode_target urls start with slash
pquentin Oct 31, 2019
011d857
Remove unused HTTPSConnection.connect()
pquentin Oct 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 0 additions & 34 deletions src/urllib3/connection.py
View file
Open in desktop
Expand Up @@ -251,40 +251,6 @@ def __init__(
# HTTPS requests to go out as HTTP. (See Issue #356)
self._protocol = "https"

def connect(self):
Copy link
Member

Choose a reason for hiding this comment

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

We probably shouldn't remove this, what happens if a user has an unverified HTTPS connection, is this class used?

Copy link
Member Author

@pquentin pquentin Oct 31, 2019 •
edited

Choose a reason for hiding this comment

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

No, it's not used, unverified connections go through VerifiedHTTPSConnection.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, huh. I wonder what the impact of nuking the class and rebinding the name would do to downstream. 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think anybody is using this class: connect() referenced variables that don't exist, so could not work. An additional wrinkle here is the bottom of the file:

if ssl:
    # Make a copy for testing.
    UnverifiedHTTPSConnection = HTTPSConnection
    HTTPSConnection = VerifiedHTTPSConnection

So one option is to merge VerifiedHTTPSConnection into HTTPSConnection, and add UnverifiedHTTPSConnection = HTTPSConnection at the end of the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sethmlarson I opened #1730 to unblock this pull request and talk about our options here, including the upstream impact.

Copy link
Member

Choose a reason for hiding this comment

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

@zogzog Sorry for causing an issue. I've edited your above comment as I read it as sarcastic which isn't constructive. Hope you agree with my edit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aurélien, je comprends ta colère, je sais ce que c'est de tomber sur un commit quelque part sur GitHub qui casse ton code. Je te présente d'ailleurs mes excuses. Maintenant que ça c'est dit, on apprécierait des interventions plus courtoises !

Indeed I had to add [...] 6 months ago because someone removed them or something

We would have appreciated a heads-up at this point! This would have avoided the whole issue.

I'd be happy to accept a pull request that restores a working and covered connect().

People should really understand that, even though it is generally considered a bad thing and should be discouraged, there is a need for unverified https connections that don't spit warnings on the console.

Can you use our fingerprinting feature if the certificate does not change too often?

Is silencing the warning an option?

By the way, it looks like the TLS certificate of https://pythonian.fr/ is currently expired.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, why don't you just silence the warning? Isn't this working for you?

import urllib3

urllib3.disable_warnings(urllib3.exceptions.SecurityWarning)
http = urllib3.PoolManager(cert_reqs="CERT_NONE")
r = http.request("GET", "https://pythonian.fr/webfiles/css.css")
print(r.status)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, for what it's worth, here are the thoughts of Seth on verifying HTTPS: https://sethmlarson.dev/blog/2019-11-26/designing-for-real-world-https. I happen to fully agree.

Copy link
Member

Choose a reason for hiding this comment

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

There's also this method (using certificate pinning as mentioned above) that doesn't require you to disable security warnings. :)

http = urllib3.PoolManager(
    cert_reqs="CERT_NONE",
    assert_fingerprint="e677022f45ad8ecfc7a66dbb8d57e9c7b01a52c9f880ecb104af6ee67ee9f18a"
)
resp = http.request("GET", "https://pythonian.fr/webfiles/css.css")

Just remember this PoolManager will only work for the one website. Might be worth even using a ConnectionPool instead.

conn = self._new_conn()
self._prepare_conn(conn)

# Wrap socket using verification with the root certs in
# trusted_root_certs
default_ssl_context = False
if self.ssl_context is None:
default_ssl_context = True
self.ssl_context = create_urllib3_context(
ssl_version=resolve_ssl_version(self.ssl_version),
cert_reqs=resolve_cert_reqs(self.cert_reqs),
)

# Try to load OS default certs if none are given.
# Works well on Windows (requires Python3.4+)
context = self.ssl_context
if (
not self.ca_certs
and not self.ca_cert_dir
and default_ssl_context
and hasattr(context, "load_default_certs")
):
context.load_default_certs()

self.sock = ssl_wrap_socket(
sock=conn,
keyfile=self.key_file,
certfile=self.cert_file,
key_password=self.key_password,
ssl_context=self.ssl_context,
server_hostname=self.server_hostname,
)


class VerifiedHTTPSConnection(HTTPSConnection):
"""
Expand Down
3 changes: 0 additions & 3 deletions src/urllib3/util/url.py
View file
Open in desktop
Expand Up @@ -322,9 +322,6 @@ def _idna_encode(name):

def _encode_target(target):
"""Percent-encodes a request target so that there are no invalid characters"""
if not target.startswith("/"):
return target

path, query = TARGET_RE.match(target).groups()
target = _encode_invalid_chars(path, PATH_CHARS)
query = _encode_invalid_chars(query, QUERY_CHARS)
Expand Down
8 changes: 8 additions & 0 deletions test/appengine/test_gae_manager.py
View file
Open in desktop
Expand Up @@ -168,3 +168,11 @@ def setup_method(self, method):
# Disable urlfetch which doesn't respect Retry-After header.
self.manager = appengine.AppEngineManager(urlfetch_retries=False)
self.pool = MockPool(self.host, self.port, self.manager)


def test_gae_environ():
assert not appengine.is_appengine()
assert not appengine.is_appengine_sandbox()
assert not appengine.is_local_appengine()
assert not appengine.is_prod_appengine()
assert not appengine.is_prod_appengine_mvms()