View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0005411 | libmicrohttpd | compliance | public | 2018-07-25 13:07 | 2021-09-02 17:54 |
Reporter | samh | Assigned To | Christian Grothoff | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | 0.9.59 | ||||
Target Version | 0.9.60 | Fixed in Version | 0.9.60 | ||
Summary | 0005411: MHD incorrectly sets Transfer-Encoding when Content-Length header is present | ||||
Description | Hi, We have a piece of software which uses libmicrohttpd to serve HTTP responses. The data to be returned is of a known length, and as such we set the Content-Length header in the response. In our particular use-case, the client needs to know the full length of the response upfront. However, we've observed that when setting the Content-Length header via MHD_add_response_header, there's nothing in MHD to flag that it must not (as per https://tools.ietf.org/html/rfc7230#section-3.3.2 ) send a Content-Length header in a response that contains a Transfer-Encoding header. I've attached a quick patch which adds an additional flag into build_header_response so that Transfer-Encoding is not set if the application has already provided a Content-Length header. Let me know what you think. -Sam | ||||
Tags | No tags attached. | ||||
Attached Files | 0001-Don-t-set-transfer-encoding-if-content-length-suppli.patch (2,155 bytes)
From 406da99114e5fb972793897ab1dc0e973e3b8057 Mon Sep 17 00:00:00 2001 From: Sam Hurst <samuelh@rd.bbc.co.uk> Date: Wed, 25 Jul 2018 12:05:49 +0100 Subject: [PATCH] Don't set transfer encoding if content-length supplied --- src/microhttpd/connection.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/microhttpd/connection.c b/src/microhttpd/connection.c index 1778c59..555d18a 100644 --- a/src/microhttpd/connection.c +++ b/src/microhttpd/connection.c @@ -1419,6 +1419,7 @@ build_header_response (struct MHD_Connection *connection) bool client_requested_close; bool response_has_close; bool response_has_keepalive; + bool response_has_content_length; const char *have_encoding; const char *have_content_length; int must_add_close; @@ -1483,6 +1484,7 @@ build_header_response (struct MHD_Connection *connection) must_add_keep_alive = MHD_NO; must_add_content_length = MHD_NO; response_has_close = false; + response_has_content_length = false; switch (connection->state) { case MHD_CONNECTION_FOOTERS_RECEIVED: @@ -1495,6 +1497,8 @@ build_header_response (struct MHD_Connection *connection) client_requested_close = MHD_lookup_header_s_token_ci (connection, MHD_HTTP_HEADER_CONNECTION, "close"); + if (NULL != MHD_get_response_header(connection->response, "Content-Length")) + response_has_content_length = true; if (0 != (connection->response->flags & MHD_RF_HTTP_VERSION_1_0_ONLY)) connection->keepalive = MHD_CONN_MUST_CLOSE; @@ -1512,7 +1516,8 @@ build_header_response (struct MHD_Connection *connection) (NULL == connection->response->upgrade_handler) && #endif /* UPGRADE_SUPPORT */ (! response_has_close) && - (! client_requested_close) ) + (! client_requested_close) && + (! response_has_content_length)) { /* size is unknown, and close was not explicitly requested; need to either to HTTP 1.1 chunked encoding or -- 2.7.4 | ||||
|
Ok, interesting. But re-reading RFC 7230 it seems to me that we need to do a bit more about Transfer-Encoding vs. Content-Length: if a client sets 'Transfer-Encoding: gzip' we must also detect this, append ", chunked" *and* then disable setting of Content-Length and instead do the chunking. I think we don't do that either. Let me know if this is urgent, then I could apply the patch quickly. Otherwise, I'd rather spend the time to address this issue more comprehensively. |
|
It's not urgent for us, I can always add patches to our own packages to fix the specific issue that we're seeing for now. By all means add your own changes to cover the case of gzip as well. |
|
I've looked more carefully at the RFC and our code, and decided this: 1) There is an issue where MHD would set the "Transfer-Encoding" to "identity" and "Content-length" as well. I'm changing the code to prevent this. 2) I'm changing (and documenting) MHD_add_response_header() to prevent applications from setting "Content-length" at all. It was never needed, always a bad idea, and can obviously cause all kinds of protocol violations. So the right check is to prevent that, rather than stopping MHD from computing the proper "Transfer-Encoding". 3) I'm also preventing applications from setting a "Transfer-Encoding" that MHD does not support (so only 'identity' and 'chunked' are now allowed). The change is largely backwards-compatible as we now simply return "MHD_NO" to the application whenever it requests something stupid. Well-written applications should tolerate this change in the return value (fingers crossed...). |
|
Git commit is bc8e12c8..0db81a92 |
|
Fix committed to master branch. |
Date Modified | Username | Field | Change |
---|---|---|---|
2018-07-25 13:07 | samh | New Issue | |
2018-07-25 13:07 | samh | File Added: 0001-Don-t-set-transfer-encoding-if-content-length-suppli.patch | |
2018-08-05 22:09 | Christian Grothoff | Note Added: 0013174 | |
2018-08-05 22:09 | Christian Grothoff | Assigned To | => Christian Grothoff |
2018-08-05 22:09 | Christian Grothoff | Status | new => assigned |
2018-08-08 11:13 | samh | Note Added: 0013188 | |
2018-10-05 19:24 | Christian Grothoff | Note Added: 0013259 | |
2018-10-05 19:24 | Christian Grothoff | Status | assigned => resolved |
2018-10-05 19:24 | Christian Grothoff | Resolution | open => fixed |
2018-10-05 19:24 | Christian Grothoff | Fixed in Version | => 0.9.60 |
2018-10-05 19:24 | Christian Grothoff | Note Added: 0013260 | |
2018-10-05 19:24 | Christian Grothoff | Target Version | => 0.9.60 |
2018-11-06 19:46 | Christian Grothoff | Status | resolved => closed |
2021-09-02 17:54 | Christian Grothoff | Changeset attached | => libmicrohttpd master 0db81a92 |
2021-09-02 17:54 | Christian Grothoff | Note Added: 0018180 |