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

Send HTTP 400 response for invalid request #1352

Merged
merged 2 commits into from Feb 11, 2022
Merged

Send HTTP 400 response for invalid request #1352

merged 2 commits into from Feb 11, 2022

Conversation

bertramr
Copy link
Contributor

@bertramr bertramr commented Feb 1, 2022

Given an invalid request, respond with an HTTP 400 error instead of
closing the connection without a response.

Please find here a rebased version of #205

Given an invalid request, respond with an HTTP 400 error instead of
closing the connection without a response.
Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

would it be possible for both implementations of send_400_response to have the same signature ? one take str the other bytes.

or maybe there's a good reason

@bertramr
Copy link
Contributor Author

Yes, would be possible. What would be the preferred datatype?

Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

thanks @bertramr

@euri10 euri10 merged commit 93e07fd into encode:master Feb 11, 2022
Comment on lines +176 to +180
from uvicorn.protocols.websockets.auto import AutoWebSocketsProtocol

if AutoWebSocketsProtocol is None:
msg = "No supported WebSocket library detected. Please use 'pip install uvicorn[standard]', or install 'websockets' or 'wsproto' manually." # noqa: E501
self.logger.warning(msg)
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it needs to be deleted.

Copy link
Member

Choose a reason for hiding this comment

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

this was part of improving user experience in #926 @tomchristie
Do you want we get rid of it ?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it just needs to stay within the if upgrade_value != b"websocket" or self.ws_protocol_class is None: block. It doesn't make sense for this to be within send_400_response right? (Eg. it shouldn't log if it's called because of except httptools.HttpParserError as exc:)

Copy link
Member

Choose a reason for hiding this comment

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

ha yes you're right I didn't notice if was sent also in that case, I misread thinking it was just for the upgrade case

Comment on lines +262 to +266
from uvicorn.protocols.websockets.auto import AutoWebSocketsProtocol

if AutoWebSocketsProtocol is None:
msg = "No supported WebSocket library detected. Please use 'pip install uvicorn[standard]', or install 'websockets' or 'wsproto' manually." # noqa: E501
self.logger.warning(msg)
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it needs to be deleted.

Kludex pushed a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
* Send HTTP 400 response for invalid request

Given an invalid request, respond with an HTTP 400 error instead of
closing the connection without a response.

* changed signature of send_400_response to msg as str

Co-authored-by: Mark Breedlove <mb@dp.la>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants