Skip to content

Pass request = true to write_body so HTTP/1.0 request bodies of unknown length raise instead of being close-delimited. - #260

Open
samuel-williams-shopify wants to merge 2 commits into
mainfrom
http10-request-body-length
Open

samuel-williams-shopify wants to merge 2 commits into
mainfrom
http10-request-body-length

Conversation

@samuel-williams-shopify

Copy link
Copy Markdown
Contributor

Async::HTTP::Protocol::HTTP1::Client now passes request = true to Protocol::HTTP1::Connection#write_body, so an HTTP/1.0 request body of unknown length raises instead of being written without framing where the server expects the next request. Companion to socketry/protocol-http1#65.

Release dependency: this depends on the request argument added in socketry/protocol-http1#65 and bumps the protocol-http1 constraint to ~> 0.42. Against the released protocol-http1 (0.41.1) the extra positional argument raises ArgumentError, so this must not merge until that change is released.

Problem

A request body cannot be delimited by closing the connection. Per RFC 1945 §7.2.2 and RFC 9112 §6.3, a request with neither content-length nor transfer-encoding has a zero-length body, so the server reads zero body bytes and either discards the rest, or, if the connection is kept alive, parses the body bytes as the start of the next request.

HTTP/1.0 has no chunked encoding, so when the HTTP/1 client is asked to send an HTTP/1.0 request with a body of unknown length (for example a Protocol::HTTP::Body::Writable), write_body falls through to the close-delimited path that is only valid for responses. Reproduced against main with an Async::HTTP HTTP/1.0 client and server: a POST / with connection: keep-alive and a writable body containing GET /smuggled HTTP/1.1\r\nhost: localhost\r\n\r\n resulted in the server invoking the application for / with an empty body and then again for /smuggled.

Solution

write_body in protocol-http1 gains a trailing request argument, mirroring head, so the framing decision is made from information the caller already has. Both call sites in Protocol::HTTP1::Client#call (the streaming child task and the synchronous path) now pass true. The server call sites are unchanged, since response bodies may legitimately be close-delimited under HTTP/1.0.

With this change the client raises instead of sending the body. Because the body is written in a child task which closes the connection on failure, the error currently surfaces to the caller of client.post as Protocol::HTTP1::ProtocolError: Cannot read response in state: closed! rather than the underlying Cannot write HTTP/1.0 request body of unknown length!. The new test asserts what is observable: the request raises, the body is closed, and the server application is never invoked. Propagating the original error from the child task is a separate follow-up.

Tests

This branch has not been deployed

No deployments
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.

1 participant