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

Add no-server-header support for Web Socket implementations #1581

Closed
wants to merge 31 commits into from

Conversation

iudeen
Copy link
Contributor

@iudeen iudeen commented Jul 25, 2022

No description provided.

iudeen and others added 13 commits July 15, 2022 07:50
adopted change suggested.

Co-authored-by: Marcelo Trylesinski <[email protected]>
as per suggestion, moved the limitation to --no-server-header area
Added logic to add server header support for websockets and also to pass default headers
Added logic to add server header support for websockets and also to pass default headers
* added logic to accept default headers in websocket

* added default_headers to class init

* fix

* feat(websockets): added server header support for websockets

Added logic to add server header support for websockets and also to pass default headers

* feat(websockets-wsproto): added server header support for websockets

Added logic to add server header support for websockets and also to pass default headers

Co-authored-by: Irfanuddin <[email protected]>
@iudeen iudeen marked this pull request as draft July 25, 2022 10:26
Irfanuddin added 4 commits July 25, 2022 15:59
# Conflicts:
#	uvicorn/protocols/websockets/websockets_impl.py
#	uvicorn/protocols/websockets/wsproto_impl.py
@iudeen
Copy link
Contributor Author

iudeen commented Jul 25, 2022

I am not sure if these features are needed at this point of time, but I went ahead and added as I was not able to conclusively find a reason why server headers were not added to Websocket protocols.

Note: Also, we can keep this in WIP till Websocket library provides an API to disable server header completely.

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

I'd prefer to not use the workaround on the websockets implementation 🤔

There's already an issue on websockets about this, and we already documented the limitation. Let's modify the note to specify that it does not work on websockets implementation and add the headers to wsproto.

@Kludex Kludex added this to the Version 0.19.0 milestone Aug 12, 2022
@iudeen
Copy link
Contributor Author

iudeen commented Aug 12, 2022

@Kludex, agreed will make the changes today! thanks for your feedback

@Kludex
Copy link
Member

Kludex commented Aug 12, 2022

Thanks 🙏

@iudeen iudeen marked this pull request as ready for review August 12, 2022 10:23
Copy link
Contributor Author

@iudeen iudeen left a comment

Choose a reason for hiding this comment

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

implemented request changes

@iudeen iudeen requested a review from Kludex August 12, 2022 10:25
@iudeen
Copy link
Contributor Author

iudeen commented Aug 12, 2022

Test Results:

Code:

# app.py

from fastapi import FastAPI
from starlette.websockets import WebSocket

app = FastAPI()


@app.websocket("/ws")
async def websocket_endpoint(websocket: WebSocket):
    await websocket.accept()
    while True:
        data = await websocket.receive_text()
        await websocket.send_text(f"Message text was: {data}")

Websockets implementation without --no-server-header

>uvicorn app:app
INFO:     Started server process [9848]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
HTTP/1.1 101 Switching Protocols
Upgrade: websocket
Connection: Upgrade
Sec-WebSocket-Accept: bxNWrBsDyxZs3+p8aaNcwTzh6ac=
Sec-WebSocket-Extensions: permessage-deflate
date: Fri, 12 Aug 2022 09:26:02 GMT
server: uvicorn

Websockets implementation with --no-server-header

>uvicorn app:app --no-server-header
INFO:     Started server process [9848]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
HTTP/1.1 101 Switching Protocols
Upgrade: websocket
Connection: Upgrade
Sec-WebSocket-Accept: voUjj70rAcpXQJnB1yI+0lLZuGg=
Sec-WebSocket-Extensions: permessage-deflate
date: Fri, 12 Aug 2022 09:29:49 GMT
Server: Python/3.10 websockets/10.3

Wsproto implementation with --no-server-header

>uvicorn app:app --no-server-header --ws wsproto
INFO:     Started server process [9848]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
HTTP/1.1 101
Upgrade: WebSocket
Connection: Upgrade
Sec-WebSocket-Accept: u1f5bQpPsn8/VPZAo5ynbrRRkag=
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits=15
date: Fri, 12 Aug 2022 17:33:05 GMT

Wsproto implementation without --no-server-header

>uvicorn app:app  --ws wsproto
INFO:     Started server process [9848]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
HTTP/1.1 101
Upgrade: WebSocket
Connection: Upgrade
Sec-WebSocket-Accept: r0+014Ywn5SbmlY9tNSEPg6FRfg=
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits=15
date: Fri, 12 Aug 2022 17:34:04 GMT
server: uvicorn

Irfanuddin added 2 commits August 12, 2022 23:14
--no-date-header is also not compatible with Websockets
--no-date-header is also not compatible with Websockets
@iudeen
Copy link
Contributor Author

iudeen commented Aug 12, 2022

--no-date-header is not compatible with Websockets implementation. Added that also as a limitation in documentation.

@iudeen iudeen changed the title WIP: Added server header support for Websockets Add no-server-header support for Web Socket implementations Aug 13, 2022
@Kludex
Copy link
Member

Kludex commented Aug 15, 2022

We also need tests here, jfyk.

iudeen and others added 3 commits August 15, 2022 23:18
Suggestion removes duplication check on header names to keep it consistent with http implementations.

Co-authored-by: Marcelo Trylesinski <[email protected]>
@iudeen iudeen requested a review from Kludex August 15, 2022 19:17
@iudeen
Copy link
Contributor Author

iudeen commented Aug 21, 2022

Closing this request, as it was made from master branch of the forked repo. Sorry!

Please refer to #1606 for further updates.

@iudeen iudeen closed this Aug 21, 2022
@Kludex Kludex removed this from the Version 0.19.0 milestone Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants