POST requests with no body include `content-length` header twice

[pilaf]

Minimal reproduction (requires a webserver running on localhost:9292):

require "rubygems"
require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "async-http-faraday", require: false
end

require "faraday"
require "async/http/faraday"

Faraday.default_adapter = :async_http

conn = Faraday.new(url: "http://localhost:9292")

conn.post("/")

Here's the raw request I get, notice the double content-length (although with different capitalization):

POST / HTTP/1.1\r\n
host: localhost:9200\r\n
User-Agent: Faraday v2.12.2\r\n
Content-Length: 0\r\n
content-length: 0\r\n
\r\n

Changing the Faraday adapter to :net_http doesn't have this problem.

The capitalized Content-Length seems to be set by Faraday itself at Faraday::Env#clear_body. Async::HTTP seems to be adding the lower-case one.

I also noticed that setting the body to empty string doesn't suffer from this issue, but I can't do that in my case since I'm using a 3rd party library:

conn.post("/") do |req|
  req.body = ""
end

Results in:

POST / HTTP/1.1\r\n
host: localhost:9200\r\n
User-Agent: Faraday v2.12.2\r\n
Content-Type: application/x-www-form-urlencoded\r\n
content-length: 0\r\n
\r\n

[ioquatix]

This feels like a bug in Faraday, as it should depend on the underlying protocol to set the content length (if needed).

1. Faraday should avoid setting this header.
2. Async::HTTP::Faraday could strip it out.

Separately, headers that contain upper case characters (e.g. Content-Length) are invalid for HTTP/2 HPACK and HTTP/3 QPACK.

cc @imactia

[pilaf]

@ioquatix I think you're right, but I wondered if all Faraday adapters would play nicely and add content-length: 0 when the body is empty, so I wrote this test that attempts to test all adapters listed on Awesome Faraday:

require "rubygems"
require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "async-http-faraday", require: false
  gem "faraday-em_http", require: false
  gem "faraday-em_synchrony", require: false
  gem "faraday-excon", require: false
  gem "faraday-httpclient", "~> 2.0", require: false
  gem "faraday-net_http_persistent", "~> 2.0", require: false
  gem "faraday-patron", require: false
  gem "faraday-typhoeus", require: false
  gem "faraday-http", require: false
  gem "httpx", require: false
end

require "socket"
require "faraday"

# Faraday adapters
require "async/http/faraday"
require "faraday/em_http"
#require "faraday/em_synchrony"
require "faraday/excon"
require "faraday/http"
require "faraday/httpclient"
require "faraday/net_http_persistent"
require "faraday/patron"
require "faraday/typhoeus"
require "httpx/adapters/faraday"

# Patch Faraday::Env to not set content-length, instead change it to x-content-length
class Faraday::Env
  remove_const(:ContentLength)
  ContentLength = "x-content-length".freeze
end

%i[
  async_http
  excon
  http
  httpclient
  httpx
  net_http
  net_http_persistent
  patron
  typhoeus
].each do |adapter|
  Faraday::METHODS_WITH_BODY.each do |method|
    server_thread = Thread.new do
      server = TCPServer.new(2000)
      socket = server.accept

      while l = socket.gets
        print l.inspect[1..-2]
        print " ✅" if l.match?(/^content-length: 0\r\n/i)
        puts
        break if l == "\r\n"
      end

      socket.close
      server.close
    end

    conn = Faraday.new(url: "http://localhost:2000") do |f|
      f.adapter adapter
    end

    puts "** #{method.upcase} with no body using #{adapter} adapter: **"

    begin
      conn.send(method, "/")
    rescue Faraday::ConnectionFailed
    end

    server_thread.join
    puts
  end
end

And here's the output, stripped to just the relevant parts:

** POST with no body using async_http adapter: **
POST / HTTP/1.1\r\n
x-content-length: 0\r\n
content-length: 0\r\n ✅

** PUT with no body using async_http adapter: **
PUT / HTTP/1.1\r\n
x-content-length: 0\r\n
content-length: 0\r\n ✅

** PATCH with no body using async_http adapter: **
PATCH / HTTP/1.1\r\n
x-content-length: 0\r\n
content-length: 0\r\n ✅

** POST with no body using excon adapter: **
POST / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PUT with no body using excon adapter: **
PUT / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PATCH with no body using excon adapter: **
PATCH / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** POST with no body using http adapter: **
POST / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PUT with no body using http adapter: **
PUT / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PATCH with no body using http adapter: **
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** POST with no body using httpclient adapter: **
POST / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PUT with no body using httpclient adapter: **
PUT / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PATCH with no body using httpclient adapter: **
PATCH / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** POST with no body using httpx adapter: **
POST / HTTP/1.1\r\n
X-Content-Length: 0\r\n
Content-Length: 0\r\n ✅

** PUT with no body using httpx adapter: **
PUT / HTTP/1.1\r\n
X-Content-Length: 0\r\n
Content-Length: 0\r\n ✅

** PATCH with no body using httpx adapter: **
PATCH / HTTP/1.1\r\n
X-Content-Length: 0\r\n
Content-Length: 0\r\n ✅

** POST with no body using net_http adapter: **
POST / HTTP/1.1\r\n
X-Content-Length: 0\r\n
Content-Length: 0\r\n ✅

** PUT with no body using net_http adapter: **
PUT / HTTP/1.1\r\n
X-Content-Length: 0\r\n
Content-Length: 0\r\n ✅

** PATCH with no body using net_http adapter: **
PATCH / HTTP/1.1\r\n
X-Content-Length: 0\r\n
Content-Length: 0\r\n ✅

** POST with no body using net_http_persistent adapter: **
POST / HTTP/1.1\r\n
X-Content-Length: 0\r\n
Content-Length: 0\r\n ✅

** PUT with no body using net_http_persistent adapter: **
PUT / HTTP/1.1\r\n
X-Content-Length: 0\r\n
Content-Length: 0\r\n ✅

** PATCH with no body using net_http_persistent adapter: **
PATCH / HTTP/1.1\r\n
X-Content-Length: 0\r\n
Content-Length: 0\r\n ✅

** POST with no body using patron adapter: **
POST / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PUT with no body using patron adapter: **
PUT / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PATCH with no body using patron adapter: **
PATCH / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** POST with no body using typhoeus adapter: **
POST / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PUT with no body using typhoeus adapter: **
PUT / HTTP/1.1\r\n
x-content-length: 0\r\n
Content-Length: 0\r\n ✅

** PATCH with no body using typhoeus adapter: **
PATCH / HTTP/1.1\r\n
x-content-length: 0\r\n
❌

So it seems all adapters correctly add content-length: 0 when the body is empty for PATCH, POST and PUT methods, except for Typhoeus on PATCH requests.

Also, I didn't test EM::HTTP and EM::Synchrony, since I couldn't make those adapters work on first try.

Not sure how important the content-length: 0 is on an empty PATCH request to conform to the HTTP spec, but for the most part it seems like this confirms your point that:

1. Faraday should avoid setting this header.

Should I open an issue on Faraday for this then?

[ioquatix]

This is most excellent work, well done and thanks for the extremely detailed comparison. I do believe that we should open an issue on Faraday and see what they think.

In the mean time, I'm okay to implement a patch to strip that header from the request. If you have time, do you want to submit a PR for that?

[iMacTia]

This is great @pilaf, really love the thorough research here 👏 ! I wouldn't worry too much about EM adapters as I don't think those are actively being used (will need to double-check), but I'd like to have typhoeus patched BEFORE we make a breaking change in Faraday and stop setting the Content-Length header 🙏

[pilaf]

@iMacTia Got it, and thanks for taking the time to look into this.

I opened a ticket on Faraday's GitHub since it seemed more appropriate to continue the discussion over there.

[pilaf]

@ioquatix

In the mean time, I'm okay to implement a patch to strip that header from the request. If you have time, do you want to submit a PR for that?

I'll see if I can come up with a patch, but I can't promise I'll deliver one since I'm not familiar with Async::HTTP::Faraday's internals so I'll likely abandon it if it takes me too long to figure out.

[ioquatix]

Okay, this is released. Thanks everyone.