Tightening the runtime type check for ssl (#7698) by Dreamsorcerer · Pull Request #8042 · aio-libs/aiohttp

Dreamsorcerer · 2024-01-20T20:52:27Z

Currently, the valid types of ssl parameter are SSLContext, Literal[False], Fingerprint or None.

If user sets ssl = False, we disable ssl certificate validation which makes total sense. But if user set ssl = True by mistake, instead of enabling ssl certificate validation or raising errors, we silently disable the validation too which is a little subtle but weird.

In this PR, we added a check that if user sets ssl=True, we enable certificate validation by treating it as using Default SSL Context.

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Sviatoslav Sydorenko [email protected]
Co-authored-by: Sam Bull [email protected]
Co-authored-by: J. Nick Koston [email protected]
Co-authored-by: Sam Bull [email protected]
(cherry picked from commit 9e14ea1)

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

Currently, the valid types of ssl parameter are SSLContext, Literal[False], Fingerprint or None. If user sets ssl = False, we disable ssl certificate validation which makes total sense. But if user set ssl = True by mistake, instead of enabling ssl certificate validation or raising errors, we silently disable the validation too which is a little subtle but weird. In this PR, we added a check that if user sets ssl=True, we enable certificate validation by treating it as using Default SSL Context. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Sam Bull <[email protected]> Co-authored-by: J. Nick Koston <[email protected]> Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit 9e14ea1)

aiohttp/client.py

aiohttp/connector.py

aiohttp/client_reqrep.py

aiohttp/client.py

codecov · 2024-01-20T22:40:59Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (641f4ae) 97.40% compared to head (47ed9fe) 97.39%.

Files	Patch %	Lines
aiohttp/client.py	0.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             3.10    #8042      +/-   ##
==========================================
- Coverage   97.40%   97.39%   -0.01%     
==========================================
  Files         108      108              
  Lines       32810    32813       +3     
  Branches     3902     3903       +1     
==========================================
  Hits        31958    31958              
- Misses        648      650       +2     
- Partials      204      205       +1

Flag	Coverage Δ
CI-GHA	`97.30% <88.88%> (-0.01%)`	⬇️
OS-Linux	`96.99% <88.88%> (-0.01%)`	⬇️
OS-Windows	`94.52% <100.00%> (+<0.01%)`	⬆️
OS-macOS	`96.80% <88.88%> (-0.02%)`	⬇️
Py-3.10.11	`94.42% <100.00%> (ø)`
Py-3.10.13	`96.77% <88.88%> (-0.02%)`	⬇️
Py-3.11.7	`96.48% <88.88%> (+0.02%)`	⬆️
Py-3.12.1	`?`
Py-3.8.10	`94.39% <100.00%> (ø)`
Py-3.8.18	`96.71% <88.88%> (-0.01%)`	⬇️
Py-3.9.13	`94.41% <100.00%> (+<0.01%)`	⬆️
Py-3.9.18	`96.75% <88.88%> (-0.01%)`	⬇️
Py-pypy7.3.15	`96.30% <88.88%> (-0.01%)`	⬇️
VM-macos	`96.80% <88.88%> (-0.02%)`	⬇️
VM-ubuntu	`96.99% <88.88%> (-0.01%)`	⬇️
VM-windows	`94.52% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

patchback · 2024-01-20T22:50:36Z

Backport to 3.9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.9/4b91b530e851acec62c7e9db4cf5c086bf153340/pr-8042

Backported as #8043

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Currently, the valid types of ssl parameter are SSLContext, Literal[False], Fingerprint or None. If user sets ssl = False, we disable ssl certificate validation which makes total sense. But if user set ssl = True by mistake, instead of enabling ssl certificate validation or raising errors, we silently disable the validation too which is a little subtle but weird. In this PR, we added a check that if user sets ssl=True, we enable certificate validation by treating it as using Default SSL Context. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Sam Bull <[email protected]> Co-authored-by: J. Nick Koston <[email protected]> Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit 9e14ea1) (cherry picked from commit 4b91b53)

…or ssl (#7698) (#8043) **This is a backport of PR #8042 as merged into 3.10 (4b91b53).** Currently, the valid types of ssl parameter are SSLContext, Literal[False], Fingerprint or None. If user sets ssl = False, we disable ssl certificate validation which makes total sense. But if user set ssl = True by mistake, instead of enabling ssl certificate validation or raising errors, we silently disable the validation too which is a little subtle but weird. In this PR, we added a check that if user sets ssl=True, we enable certificate validation by treating it as using Default SSL Context. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Sam Bull <[email protected]> Co-authored-by: J. Nick Koston <[email protected]> Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit 9e14ea1)

Dreamsorcerer requested a review from asvetlov as a code owner January 20, 2024 20:52

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 20, 2024

Dreamsorcerer commented Jan 20, 2024

View reviewed changes

aiohttp/client.py Outdated Show resolved Hide resolved

Dreamsorcerer commented Jan 20, 2024

View reviewed changes

aiohttp/connector.py Outdated Show resolved Hide resolved

Apply suggestions from code review

ed86b27

Dreamsorcerer added the backport-3.9 label Jan 20, 2024

Dreamsorcerer added 2 commits January 20, 2024 21:46

Update client_reqrep.py

37ac9cf

Update client_reqrep.py

30cc724

Dreamsorcerer commented Jan 20, 2024

View reviewed changes

aiohttp/client_reqrep.py Outdated Show resolved Hide resolved

Dreamsorcerer commented Jan 20, 2024

View reviewed changes

aiohttp/client.py Outdated Show resolved Hide resolved

Dreamsorcerer added 2 commits January 20, 2024 22:22

Apply suggestions from code review

045ec4b

Update test_client_fingerprint.py

47ed9fe

Dreamsorcerer merged commit 4b91b53 into 3.10 Jan 20, 2024

Dreamsorcerer deleted the patchback/backports/3.10/9e14ea19b5a48bb26797babc32202605066cb5f5/pr-7698 branch January 20, 2024 22:50

patchback bot mentioned this pull request Jan 20, 2024

[PR #8042/4b91b530 backport][3.9] Tightening the runtime type check for ssl (#7698) #8043

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Tightening the runtime type check for ssl (#7698)#8042

Tightening the runtime type check for ssl (#7698)#8042
Dreamsorcerer merged 6 commits into3.10from
patchback/backports/3.10/9e14ea19b5a48bb26797babc32202605066cb5f5/pr-7698

Dreamsorcerer commented Jan 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 20, 2024 •

edited

Loading

Uh oh!

patchback bot commented Jan 20, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Dreamsorcerer commented Jan 20, 2024

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

patchback bot commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backport to 3.9: 💚 backport PR created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jan 20, 2024 •

edited

Loading

patchback bot commented Jan 20, 2024 •

edited

Loading