Keep auth header during http->https redirect #5848
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
Codecov Report
@@ Coverage Diff @@
## master #5848 +/- ##
=======================================
Coverage 96.75% 96.75%
=======================================
Files 44 44
Lines 9851 9852 +1
Branches 1591 1591
=======================================
+ Hits 9531 9532 +1
Misses 182 182
Partials 138 138
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
tests/test_client_functional.py
Outdated
| server_a = await create_server(yarl_a, srv1) | ||
| server_b = await create_server(yarl_b, srv2) |
| .. note:: | ||
| ``Authorization`` header will be removed if you get redirected | ||
| to a different host or protocol, except the case when ``HTTP -> HTTPS`` | ||
| redirect is performed on the same host. |
There was a problem hiding this comment.
Plz keep that versionchanged with a very small note.
There was a problem hiding this comment.
I added versionchanged with text from CHANGES/5783.feature
tests/test_client_functional.py
Outdated
| ssl_ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23) | ||
| cert.configure_cert(ssl_ctx) | ||
| kwargs["ssl"] = ssl_ctx | ||
| return await aiohttp_server(app, **kwargs) |
There was a problem hiding this comment.
nit: you could drop await here and async at the top and the interface would remain the same.
tests/test_client_functional.py
Outdated
| async def srv1(request): | ||
| assert request.host == "host1.com" | ||
| assert request.host == yarl_a.host |
There was a problem hiding this comment.
same problem with naming: yarl_a? b? c? it needs to have words referring to things it represents.
There was a problem hiding this comment.
I've renamed all redirect related variables using from,to convention!
tests/test_client_functional.py
Outdated
| url_to: str, | ||
| is_drop_header_expected: bool, | ||
| ) -> None: | ||
| yarl_a = URL(url_from) |
There was a problem hiding this comment.
Why did you use a lib name to describe a URL? why not just a URL? You could even just reuse the same variable name, right? Maybe you could even just wrap params in the decorator with yarl.URL() and keep it out of the test body to reduce the cognitive complexity of this test.
There was a problem hiding this comment.
Yep, you're right.
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
greshilov
force-pushed
the
redirect-auth-header
branch
from
be0a29e
to
3af8f33
Compare
|
@webknjaz PTAL |
What do these changes do?
http->httpsredirect is a common thing today. It's safe to keepAuthorizationheader in that situation if the host remains the same.I'll create a separate PR for the
3.8branch without this feature, but with the test and doc update (to emphasize, that this feature is4+only).Are there changes in behavior for the user?
Authorizationheader wouldn't be dropped during thehttp->httpsredirect if the host remains the same.Related issue number
Resolves #5783
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.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.