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

Upgrade request handling: ignore http/2 and optionally ignore websocket #1661

Merged
merged 13 commits into from Oct 19, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/deployment.md
Expand Up @@ -69,6 +69,9 @@ Options:
--ws-per-message-deflate BOOLEAN
WebSocket per-message-deflate compression
[default: True]
--ws-ignore-upgrade BOOLEAN Upgrade requests will be ignored and
answered with normal HTTP responses
[default: False]
--lifespan [auto|on|off] Lifespan implementation. [default: auto]
--interface [auto|asgi3|asgi2|wsgi]
Select ASGI3, ASGI2, or WSGI as the
Expand Down
3 changes: 3 additions & 0 deletions docs/index.md
Expand Up @@ -136,6 +136,9 @@ Options:
--ws-per-message-deflate BOOLEAN
WebSocket per-message-deflate compression
[default: True]
--ws-ignore-upgrade BOOLEAN Upgrade requests will be ignored and
answered with normal HTTP responses
[default: False]
--lifespan [auto|on|off] Lifespan implementation. [default: auto]
--interface [auto|asgi3|asgi2|wsgi]
Select ASGI3, ASGI2, or WSGI as the
Expand Down
1 change: 1 addition & 0 deletions docs/settings.md
Expand Up @@ -71,6 +71,7 @@ Using Uvicorn with watchfiles will enable the following options (which are other
* `--ws-max-size <int>` - Set the WebSockets max message size, in bytes. Please note that this can be used only with the default `websockets` protocol.
* `--ws-ping-interval <float>` - Set the WebSockets ping interval, in seconds. Please note that this can be used only with the default `websockets` protocol.
* `--ws-ping-timeout <float>` - Set the WebSockets ping timeout, in seconds. Please note that this can be used only with the default `websockets` protocol.
* `--ws-ignore-upgrade` - Will ignore all upgrade requests and instead continue replying with HTTP responses.
Kludex marked this conversation as resolved.
Show resolved Hide resolved
* `--lifespan <str>` - Set the Lifespan protocol implementation. **Options:** *'auto', 'on', 'off'.* **Default:** *'auto'*.
* `--h11-max-incomplete-event-size <int>` - Set the maximum number of bytes to buffer of an incomplete event. Only available for `h11` HTTP protocol implementation. **Default:** *'16384'* (16 KB).

Expand Down
40 changes: 40 additions & 0 deletions tests/protocols/test_http.py
Expand Up @@ -76,6 +76,18 @@
]
)

UPGRADE_HTTP2_REQUEST = b"\r\n".join(
[
b"GET / HTTP/1.1",
b"Host: example.org",
b"Connection: upgrade",
b"Upgrade: h2c",
b"Sec-WebSocket-Version: 11",
b"",
b"",
]
)

INVALID_REQUEST_TEMPLATE = b"\r\n".join(
[
b"%s",
Expand Down Expand Up @@ -716,6 +728,34 @@ async def test_supported_upgrade_request(protocol_cls):
assert b"HTTP/1.1 426 " in protocol.transport.buffer


@pytest.mark.anyio
@pytest.mark.parametrize("protocol_cls", HTTP_PROTOCOLS)
async def test_ignored_ws_upgrade_request(protocol_cls):
app = Response("Hello, world", media_type="text/plain")

protocol = get_connected_protocol(
app, protocol_cls, ws="wsproto", ws_ignore_upgrade=True
)
protocol.data_received(UPGRADE_REQUEST)
await protocol.loop.run_one()
assert b"HTTP/1.1 200 OK" in protocol.transport.buffer
assert b"Hello, world" in protocol.transport.buffer


@pytest.mark.anyio
@pytest.mark.parametrize("protocol_cls", HTTP_PROTOCOLS)
async def test_http2_upgrade_request(protocol_cls):
app = Response("Hello, world", media_type="text/plain")

protocol = get_connected_protocol(
app, protocol_cls, ws="wsproto", ws_ignore_upgrade=True
Kludex marked this conversation as resolved.
Show resolved Hide resolved
)
protocol.data_received(UPGRADE_HTTP2_REQUEST)
await protocol.loop.run_one()
assert b"HTTP/1.1 200 OK" in protocol.transport.buffer
assert b"Hello, world" in protocol.transport.buffer


async def asgi3app(scope, receive, send):
pass

Expand Down
2 changes: 2 additions & 0 deletions uvicorn/config.py
Expand Up @@ -220,6 +220,7 @@ def __init__(
ws_ping_interval: Optional[float] = 20.0,
ws_ping_timeout: Optional[float] = 20.0,
ws_per_message_deflate: bool = True,
ws_ignore_upgrade: bool = False,
lifespan: LifespanType = "auto",
env_file: Optional[Union[str, os.PathLike]] = None,
log_config: Optional[Union[Dict[str, Any], str]] = LOGGING_CONFIG,
Expand Down Expand Up @@ -267,6 +268,7 @@ def __init__(
self.ws_ping_interval = ws_ping_interval
self.ws_ping_timeout = ws_ping_timeout
self.ws_per_message_deflate = ws_per_message_deflate
self.ws_ignore_upgrade = ws_ignore_upgrade
self.lifespan = lifespan
self.log_config = log_config
self.log_level = log_level
Expand Down
11 changes: 11 additions & 0 deletions uvicorn/main.py
Expand Up @@ -169,6 +169,13 @@ def print_version(ctx: click.Context, param: click.Parameter, value: bool) -> No
help="WebSocket per-message-deflate compression",
show_default=True,
)
@click.option(
"--ws-ignore-upgrade",
type=bool,
Kludex marked this conversation as resolved.
Show resolved Hide resolved
default=False,
help="Upgrade requests will be ignored and answered with normal HTTP responses",
Kludex marked this conversation as resolved.
Show resolved Hide resolved
show_default=True,
)
@click.option(
"--lifespan",
type=LIFESPAN_CHOICES,
Expand Down Expand Up @@ -367,6 +374,7 @@ def main(
ws_ping_interval: float,
ws_ping_timeout: float,
ws_per_message_deflate: bool,
ws_ignore_upgrade: bool,
lifespan: LifespanType,
interface: InterfaceType,
reload: bool,
Expand Down Expand Up @@ -414,6 +422,7 @@ def main(
ws_ping_interval=ws_ping_interval,
ws_ping_timeout=ws_ping_timeout,
ws_per_message_deflate=ws_per_message_deflate,
ws_ignore_upgrade=ws_ignore_upgrade,
lifespan=lifespan,
env_file=env_file,
log_config=LOGGING_CONFIG if log_config is None else log_config,
Expand Down Expand Up @@ -464,6 +473,7 @@ def run(
ws_ping_interval: typing.Optional[float] = 20.0,
ws_ping_timeout: typing.Optional[float] = 20.0,
ws_per_message_deflate: bool = True,
ws_ignore_upgrade: bool = False,
lifespan: LifespanType = "auto",
interface: InterfaceType = "auto",
reload: bool = False,
Expand Down Expand Up @@ -516,6 +526,7 @@ def run(
ws_ping_interval=ws_ping_interval,
ws_ping_timeout=ws_ping_timeout,
ws_per_message_deflate=ws_per_message_deflate,
ws_ignore_upgrade=ws_ignore_upgrade,
lifespan=lifespan,
interface=interface,
reload=reload,
Expand Down
26 changes: 20 additions & 6 deletions uvicorn/protocols/http/h11_impl.py
Expand Up @@ -155,6 +155,23 @@ def _unset_keepalive_if_required(self) -> None:
self.timeout_keep_alive_task.cancel()
self.timeout_keep_alive_task = None

def _should_upgrade(
self, headers: List[Tuple[bytes, bytes]], event: H11Event
) -> bool:
Kludex marked this conversation as resolved.
Show resolved Hide resolved
connection = []
upgrade = None
for name, value in headers:
if name == b"connection":
connection = [token.lower().strip() for token in value.split(b",")]
if name == b"upgrade":
upgrade = value.lower().strip()
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Is there a reason for stripping?

Suggested change
upgrade = value.lower().strip()
upgrade = value.lower()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason, kept it for symmetry reasons and because I generally like to sanitize inputs. Removed it


return (
b"upgrade" in connection
and upgrade == b"websocket"
and not self.config.ws_ignore_upgrade
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm I think we could eliminate the new flag by changing it to this

Suggested change
and not self.config.ws_ignore_upgrade
and self.ws_protocol_class is not None

and removing the corresponding block from handle_websocket_upgrade. I think that would achieve the same thing.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think the warning should stay tho 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So trying to understand what that block tries to achieve is

  • logging a warning
  • logging another warning if no protocol was found
  • responding with 400

But should these warnings really be logged on every upgrade request again? Especially the second one.
In my use case this will lead to a lot of unnecessary log messages.

Copy link
Contributor Author

@Argannor Argannor Sep 27, 2022

Choose a reason for hiding this comment

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

What do you think about moving the second log statement to the run method in main.py? I feel like this indicates a possible configuration / setup issue which would be easier to catch if printed up front on startup. We should rephrase to account for the fact that there are applications which will use the default setting auto without the need for an implementation to be present.

I am still undecided on what to do with the first warning. Since I already have a custom log handler, I could filter this message out to prevent the broken window for me.

Alternatively we could document the behaviour and necessary remediation inside the docs to help confused devs figure out why their request gets ignored and or turn down the log level.

Side note: I am not available today, but will come back to this tomorrow

)

def data_received(self, data: bytes) -> None:
self._unset_keepalive_if_required()

Expand Down Expand Up @@ -204,12 +221,9 @@ def handle_events(self) -> None:
"headers": self.headers,
}

for name, value in self.headers:
if name == b"connection":
tokens = [token.lower().strip() for token in value.split(b",")]
if b"upgrade" in tokens:
self.handle_upgrade(event)
return
if self._should_upgrade(self.headers, event):
self.handle_upgrade(event)
return

# Handle 503 responses when 'limit_concurrency' is exceeded.
if self.limit_concurrency is not None and (
Expand Down
28 changes: 24 additions & 4 deletions uvicorn/protocols/http/httptools_impl.py
Expand Up @@ -149,6 +149,21 @@ def _unset_keepalive_if_required(self) -> None:
self.timeout_keep_alive_task.cancel()
self.timeout_keep_alive_task = None

def _should_upgrade(self, headers: List[Tuple[bytes, bytes]]) -> bool:
connection = []
upgrade = None
for name, value in headers:
if name == b"connection":
connection = [token.lower().strip() for token in value.split(b",")]
if name == b"upgrade":
upgrade = value.lower().strip()

return (
b"upgrade" in connection
and upgrade == b"websocket"
and not self.config.ws_ignore_upgrade
)

def data_received(self, data: bytes) -> None:
self._unset_keepalive_if_required()

Expand All @@ -160,7 +175,8 @@ def data_received(self, data: bytes) -> None:
self.send_400_response(msg)
return
except httptools.HttpParserUpgrade:
self.handle_upgrade()
if self._should_upgrade(self.headers):
self.handle_upgrade()

def handle_upgrade(self) -> None:
upgrade_value = None
Expand Down Expand Up @@ -244,7 +260,7 @@ def on_headers_complete(self) -> None:
self.scope["method"] = method.decode("ascii")
if http_version != "1.1":
self.scope["http_version"] = http_version
if self.parser.should_upgrade():
if self.parser.should_upgrade() and self._should_upgrade(self.headers):
return
parsed_url = httptools.parse_url(self.url)
raw_path = parsed_url.path
Expand Down Expand Up @@ -291,15 +307,19 @@ def on_headers_complete(self) -> None:
self.pipeline.appendleft((self.cycle, app))

def on_body(self, body: bytes) -> None:
if self.parser.should_upgrade() or self.cycle.response_complete:
if (
self.parser.should_upgrade() and self._should_upgrade(self.headers)
) or self.cycle.response_complete:
return
self.cycle.body += body
if len(self.cycle.body) > HIGH_WATER_LIMIT:
self.flow.pause_reading()
self.cycle.message_event.set()

def on_message_complete(self) -> None:
if self.parser.should_upgrade() or self.cycle.response_complete:
if (
self.parser.should_upgrade() and self._should_upgrade(self.headers)
) or self.cycle.response_complete:
return
self.cycle.more_body = False
self.cycle.message_event.set()
Expand Down