Skip to content

Respect pre-computed content-length for HEAD responses - #353

Merged
mtrudel merged 5 commits into
mtrudel:mainfrom
hyperspheretech:main
May 15, 2024
Merged

Respect pre-computed content-length for HEAD responses#353
mtrudel merged 5 commits into
mtrudel:mainfrom
hyperspheretech:main

Conversation

@meeq

@meeq meeq commented May 13, 2024

Copy link
Copy Markdown
Contributor

I have an application that streams extremely large payloads. Due to reasons beyond my control, HEAD requests must respond with HTTP status 200, so my Plug handler sends with:

conn
|> Plug.Conn.put_resp_header("content-length", payload_size)
|> Plug.Conn.send_resp(200, "")

However, Bandit.Headers.add_content_length/3 replaces my application's "content-length" header with content-length: 0.

As it stands the only way for me to set "content-length" on a HEAD 200 response seems to be to either wastefully render the full response or allocate a body of "content-length"-size and pass it into send_resp, but this is not practical or efficient. It also seems to violate the spirit of RFC9110 § 9.3.2:

A client SHOULD NOT generate content in a HEAD request unless it is made directly to an origin server that has previously indicated, in or out of band, that such a request has a purpose and will be adequately supported.

This patch introduces a simple, reasonably-safe heuristic to preserve the application's intent: if a HEAD response handler sets the content-length response header, treat it as the size of the body that would have been sent; RFC9110 §8.6:

A server MAY send a Content-Length header field in a response to a HEAD request; a server MUST NOT send Content-Length in such a response unless its field value equals the decimal number of octets that would have been sent in the content of a response if the same request had used the GET method. [...] A server MUST NOT send a Content-Length header field in any response with a status code of 1xx (Informational) or 204 (No Content).

Comment thread lib/bandit/adapter.ex Outdated
@mtrudel

mtrudel commented May 13, 2024

Copy link
Copy Markdown
Owner

Thanks for the PR! Left you some refactoring suggestions. We'll also need a test to cover this for both H1 and H2.

Comment thread lib/bandit/headers.ex Outdated
Comment thread test/bandit/http2/plug_test.exs

@mtrudel mtrudel left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

One minor typo otherwise this is good to go!

Comment thread lib/bandit/headers.ex Outdated
Comment thread lib/bandit/headers.ex Outdated
Comment thread test/bandit/http2/plug_test.exs
Comment thread lib/bandit/headers.ex Outdated
Co-authored-by: Mat Trudel <mat@geeky.net>
@mtrudel
mtrudel merged commit 225104a into mtrudel:main May 15, 2024
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.

2 participants