Body from 'upgrade' request is not read

[kristjanvalur]

ASGI defines the extension websocket.http.response. This extension allows an application to send an arbitrary http response
as a connection rejection.

the websockets module, however, states: (in legacy.http.py)

    :func:`read_request` doesn't attempt to read the response body because
    WebSocket handshake responses don't have one. If the response contains a
    body, it may be read from ``stream`` after this coroutine returns.

the legacy.client.read_http_response() method again states that a body may be read from after the headers, but
the pertinent code https://github.com/aaugustin/websockets/blob/ba1ed7a65cc876ff4e0fcd4dd4711402836475e2/src/websockets/legacy/client.py#L322-L328 does not do that. The result is that the raised exception does not contain any body which may have been provided with the response.

This issue comes from this PR: encode/uvicorn#1907. uvicorn is using websockets in its unit tests and we can't verify that the received body is indeed the body which was sent.

If there is interest, I could attempt to provide a PR to rectify this issue.

[aaugustin]

states that a body may be read from after the headers...
... but the pertinent code does not do that.

What I really mean here is "go read it yourself directly from the transport if you want it". There's no public API.

For the purposes of writing a unit test, did you consider using an HTTP client (urllib2, httpx, etc.) instead websockets?

(Long term, we should look into switching uvicorn from the "legacy", asyncio-only implementation to the Sans-I/O implementation. The latter can read HTTP response bodies. See also #975.)

[kristjanvalur]

I'm not the author of Uvicorn, but a contributor. It already uses websockets as its test client, and that one relies on catching the InvalidStatusCode exception to catch the rejection messages.

Of course we can workaround this in Uvicorn, switch to other libraries in fact. But since I was working in this area it occurred to me that this repo migth want to get an enhancement as a PR. If not, then that's fine too.

[aaugustin]

Well, if you want to implement it and write tests, I'll take it. I haven't done it myself because I'm trying to limit my investment on the historical implementation and to focus on the new one.

[kristjanvalur]

Good to know. I'm not promising I will, just asking for permission upfront, I've sometimes done PRs that weren't welcome.
Cheers!

[aaugustin]

It would require:

• reproducing this logic from the Sans-I/O implementation in the legacy implementation;
• adding tests to reach 100% branch coverage (standard for this library);
• updating the docs.

The test suite for the legacy implementation is poorly designed. At this point, I have forgotten about its design, which means that no one on Earth masters it anymore :-( That will be the main hurdle.

[aaugustin]

I'm counting about a dozen branches. That's what it takes to read a response body — thank you HTTP. This means about a dozen tests :-(

[kristjanvalur]

I don't see the code you link to actually reading chunked encoding either... but for the purposes of websocket denial response bodies, I think it is safe to support only a subset of possible response types... Let's see how much free time I have :)

[aaugustin]

Indeed it doesn't support chunked encoding. But it does raise a proper error if this happens :-)

[aaugustin]

For clarity: don't add support for chunked encoding. If you write the code, I get to maintain it. But I'm not in the business of maintaining a HTTP library. The support of HTTP shouldn't go beyond the bare minimum required to open a WebSocket connection.

[aaugustin]

Related: #795

[aaugustin]

Considering the lack of activity, I'm closing this as a duplicate of #795.