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

always disable the builtin hostname verification #526

Merged
merged 1 commit into from Feb 4, 2015

Conversation

@t-8ch
Copy link
Contributor

@t-8ch t-8ch commented Dec 19, 2014

It conflicts with our own, more flexible functionality

Fixes #524

@t-8ch t-8ch force-pushed the t-8ch:disable_builtin_hostname_verification branch 2 times, most recently from 8c47d80 to 5a61b61 Dec 19, 2014
@t-8ch
Copy link
Contributor Author

@t-8ch t-8ch commented Dec 19, 2014

I give up on travis. Tried 3 times and everytime some random timeout or proxy test broke.

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Dec 20, 2014

LGTM, though we do need a clean test run.

@sigmavirus24
Copy link
Contributor

@sigmavirus24 sigmavirus24 commented Dec 21, 2014

So I'm -1 on this. If I can get some time to work on #507 and move the context logic into that method (like was suggested in the review comments) then this wouldn't be necessary and we'll be able to handle this logic a lot better.

@shazow
Copy link
Member

@shazow shazow commented Dec 23, 2014

@sigmavirus24 What's your ETA? I'm cool on blocking on your fix if it's soon-ish. :)

@shazow
Copy link
Member

@shazow shazow commented Dec 23, 2014

@Lukasa Do you think this is urgent enough to push out a 1.10.1 hotfix, or should we wait for @sigmavirus24 to Fix It Properly? :P

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Dec 24, 2014

Ehh....pass?

The summary is that hostname verification is broken in 1.10, and if you do anything other than the default behaviour the stdlib will ruin your day. Now, it might be enough to publish an advisory that says "hey, 1.10 has this bug in it, please avoid upgrading if you rely on this functionality" and then wait for @sigmavirus24. That way we never have a temporary fix in place.

@shazow
Copy link
Member

@shazow shazow commented Dec 24, 2014

If only there was some medium to publish such an advisory on that is not my twitter account. :P

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Dec 24, 2014

Haha, quite.

@alecz20
Copy link
Contributor

@alecz20 alecz20 commented Jan 8, 2015

We should also add a test case for this issue. Even if this might not break in the future, a test case would prevent a release if the functionality is broken somewhere else.

It conflicts with our own, more flexible functionality

Fixes #524
@t-8ch t-8ch force-pushed the t-8ch:disable_builtin_hostname_verification branch from 5a61b61 to 6340b18 Jan 8, 2015
@t-8ch
Copy link
Contributor Author

@t-8ch t-8ch commented Jan 8, 2015

There comes a patch.

@sigmavirus24
Copy link
Contributor

@sigmavirus24 sigmavirus24 commented Feb 4, 2015

👍 when the tests pass

@t-8ch t-8ch closed this Feb 4, 2015
@t-8ch t-8ch reopened this Feb 4, 2015
@t-8ch t-8ch closed this Feb 4, 2015
@t-8ch t-8ch reopened this Feb 4, 2015
@t-8ch
Copy link
Contributor Author

@t-8ch t-8ch commented Feb 4, 2015

They passed, finally...

@Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Feb 4, 2015

LGTM. 👍 🍰

shazow added a commit that referenced this pull request Feb 4, 2015
always disable the builtin hostname verification.
@shazow shazow merged commit bc8bfdf into urllib3:master Feb 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@Yasumoto
Copy link

@Yasumoto Yasumoto commented Feb 7, 2015

Howdy all,

Any chance you can cut a new release that includes this? I'm relatively sure that we're running into #543 and I'd be willing to pitch in if needed so we can get pip (and thus virtualenv) upgraded.

Thanks!
Joe

@shazow
Copy link
Member

@shazow shazow commented Feb 7, 2015

@Yasumoto Good call. How about early next week? We have a couple of issues I'd like to get merged and then push out 1.10.1

@Yasumoto
Copy link

@Yasumoto Yasumoto commented Feb 7, 2015

That sounds awesome! We're pretty far downstream, so much appreciated :)

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.

6 participants