Incorrect handling of `Expect: 100-continue` during large file uploads

[solarispika]

Description

When using curl to upload large files (>1M) to a server written with cpp-httplib, curl adds the Expect: 100-continue header. After the server rejects the request by returning a non-100 status code and some data, curl blocks for 1 second after receiving the data and then fails with a broken pipe error.

Steps to Reproduce

1. Set up a server using cpp-httplib.

2. Use curl to upload a file larger than 1M to the server.

3. Observe the --trace output from curl, which shows that it still sends data after handling the 100-continue response.

   == Info: Done waiting for 100-continue
   => Send data, 65536 bytes (0x10000)
   

Expected Behavior

According to RFC 7231 section 5.1.1:

A server that responds with a final status code before reading the
entire message body SHOULD indicate in that response whether it
intends to close the connection or continue reading and discarding
the request message (see Section 6.6 of [RFC7230]).

The expected behavior is for the server to either:

1. Read and discard the remaining request data, or
2. Close the connection immediately with a Connection: close header.

Current Behavior

Currently, cpp-httplib does not handle this scenario correctly. It neither reads and discards the remaining request data nor closes the connection immediately. Instead, it leaves the connection as-is and continues to respond with the Keep-Alive header for CPPHTTPLIB_KEEPALIVE_MAX_COUNT times if the client does not close the connection in the request header.

Attempted Solution

The following modification to the code at https://github.com/yhirose/cpp-httplib/blob/548dfff0aef25e36e971af96b49ce7fbb72d840e/httplib.h#L6706 resolves the issue for curl:

default:
  connection_closed = true;
  return write_response(strm, true, req, res);

With this change, curl exits without the broken pipe error.

Additional Context

• Link to the relevant curl documentation: https://everything.curl.dev/http/post/expect100.html

[solarispika]

Update: The latest RFC 9110 section 10.1.1 says A server that responds with a final status code before reading the entire request content SHOULD indicate whether it intends to close the connection (e.g., see Section 9.6 of [HTTP/1.1]) or continue reading the request content.

[yhirose]

@solarispika thanks for the report. Could you send a pull request that you suggests and a unit test in test/test.cc? Thanks!

[solarispika]

Sure! I'll take some time to see how I can add some proper tests.

[solarispika]

@yhirose I have difficulty where httplib::Client doesn't implement the Expect: 100-continue behavior, so I can't use it to test the server behavior effectively. It simply sends all the data and encounters a write error.

To comprehensively test the server implementation, the client should also support the required behavior as per the specification. However, I don't have enough time to implement the necessary changes in httplib::Client to handle the Expect: 100-continue header and the associated server responses.

Instead, I propose using an external tool or library that already supports the Expect: 100-continue behavior to test the server implementation. This approach will allow me to focus on the server-side changes while leveraging existing solutions that correctly handle the required client-side behavior.

Please let me know if you have any concerns or suggestions regarding this proposed approach.

[yhirose]

@solarispika sorry for the late reply.

Instead, I propose using an external tool or library that already supports the Expect: 100-continue behavior to test the server implementation. This approach will allow me to focus on the server-side changes while leveraging existing solutions that correctly handle the required client-side behavior.

No problem. You can just focus on the server-side changes.

[paulharris]

How do I test this to see the broken behaviour? I compiled the upload example, and then ran curl -F 'text_file=@toupload' -F 'image_file=@toupload' http://localhost:8080/upload-test-post --trace tracefile.log

But it appears to work fine, where "toupload" is an 8MB+ file.

[yhirose]

@solarispika could you replay to the @paulharris's comment?

[solarispika]

@paulharris You need to setup 100-continue handler to reject uploads. For example,

  svr.set_expect_100_continue_handler([](Request const& req, Response& res) {
    res.status = 400;
    res.set_content("{\"error\": \"You Shall Not Upload!!!!\"}", "application/json; charset=utf-8");
    return 400;
  });

[yhirose]

@solarispika

Instead, I propose using an external tool or library that already supports the Expect: 100-continue behavior to test the server implementation. This approach will allow me to focus on the server-side changes while leveraging existing solutions that correctly handle the required client-side behavior.

Please let me know if you have any concerns or suggestions regarding this proposed approach.

Are you planning to implement it (only server-side is ok) anytime soon?

[solarispika]

@yhirose Sorry for late response. I've pushed my commit for review, please check.

[yhirose]

@solarispika The code looks good to me. I fixed some build problems with Makefile on GitHub Actions workflow. Thanks for your fine contribution!