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

catch CertificateError in tls verification #3166

Merged
merged 5 commits into from Mar 14, 2018

Conversation

@lucywang000
Copy link
Member

@lucywang000 lucywang000 commented Mar 12, 2018

This should fix #3029 (downlader errors when certificate is issued for ips instead of domains)

lucywang000 added 2 commits Mar 12, 2018
@codecov
Copy link

@codecov codecov bot commented Mar 13, 2018

Codecov Report

Merging #3166 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3166      +/-   ##
==========================================
+ Coverage   82.11%   82.12%   +<.01%     
==========================================
  Files         228      228              
  Lines        9588     9593       +5     
  Branches     1385     1385              
==========================================
+ Hits         7873     7878       +5     
  Misses       1456     1456              
  Partials      259      259
Impacted Files Coverage Δ
scrapy/core/downloader/tls.py 90% <100%> (+1.42%) ⬆️
@lucywang000
Copy link
Member Author

@lucywang000 lucywang000 commented Mar 13, 2018

The tests only fails when "TOXENV=jessie", I'll see how to fix that.

lucywang000 added 2 commits Mar 13, 2018
@curita curita requested review from redapple and removed request for redapple Mar 13, 2018
@curita
Copy link
Member

@curita curita commented Mar 13, 2018

Oops, sorry about that^, misclicked the reviewers tag.

@dangra
Copy link
Member

@dangra dangra commented Mar 13, 2018

LGTM. We can release it as 1.4.x bugfix.

@@ -65,7 +73,7 @@ def _identityVerifyingInfoCallback(self, connection, where, ret):
elif where & SSL_CB_HANDSHAKE_DONE:
try:
verifyHostname(connection, self._hostnameASCII)
except VerificationError as e:
except verification_errors as e:

This comment has been minimized.

@kmike

kmike Mar 13, 2018
Member

Could you please update docstring of ScrapyClientTLSOptions? It mentions VerificationError and ValueError, but not CertificateError.

This comment has been minimized.

@lucywang000

lucywang000 Mar 14, 2018
Author Member

+1 & done

@kmike
kmike approved these changes Mar 14, 2018
Copy link
Member

@kmike kmike left a comment

Thanks @lucywang000!

@dangra dangra merged commit 6cc6bbb into scrapy:master Mar 14, 2018
2 checks passed
2 checks passed
codecov/patch 100% of diff hit (target 82.11%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dangra
Copy link
Member

@dangra dangra commented Mar 14, 2018

thanks!

@lucywang000 lucywang000 deleted the lucywang000:catch-tls-certificate-error branch Mar 17, 2018
@lucywang000
Copy link
Member Author

@lucywang000 lucywang000 commented Mar 17, 2018

Thanks everyone for the reviews !

@kmike kmike added this to the v1.6 milestone Jul 4, 2018
@kmike kmike mentioned this pull request Jul 9, 2018
@kmike kmike modified the milestones: v1.6, v1.5.1 Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.