martian icon indicating copy to clipboard operation
martian copied to clipboard

HTTP (not HTTPS) response has no Content-Length

Open itaisod opened this issue 6 years ago • 15 comments

Steps to reproduce (tested on both mac and ubuntu):

  1. go get github.com/google/martian/ && go install github.com/google/martian/cmd/proxy
  2. ~/go/bin/proxy
  3. curl -v "http://www.example.com/" --proxy localhost:8080

Result: curl hangs forever.

lab@ubuntu:~$ curl -v "http://www.example.com/" --proxy localhost:8080 > /dev/null
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET http://www.example.com/ HTTP/1.1
> Host: www.example.com
> User-Agent: curl/7.58.0
> Accept: */*
> Proxy-Connection: Keep-Alive
>
< HTTP/1.1 200 OK
< Cache-Control: max-age=604800
< Content-Type: text/html; charset=UTF-8
< Date: Sun, 05 May 2019 15:22:16 GMT
< Etag: "1541025663+ident+gzip"
< Expires: Sun, 12 May 2019 15:22:16 GMT
< Last-Modified: Fri, 09 Aug 2013 23:54:35 GMT
< Server: ECS (dcb/7EA2)
< Vary: Accept-Encoding
< X-Cache: HIT
* no chunk, no close, no size. Assume close to signal end
<
{ [1270 bytes data]
100  1270    0  1270    0     0    172      0 --:--:--  0:00:07 --:--:--     0

itaisod avatar May 05 '19 15:05 itaisod

Thanks for the repro! This looks bad, we'll have someone take a look soon.

hueich avatar May 17 '19 20:05 hueich

I was unable to reproduce the issue, but I had to take the following steps because Martian wasn't set up.

  1. go get github.com/google/martian/
  2. go install github.com/google/martian/cmd/proxy
  3. go get golang.org/x/net/websocket
  4. go install github.com/google/martian/cmd/proxy
  5. ~/go/bin/proxy
  6. curl -v "http://www.example.com/" --proxy localhost:8080

And curl returns the expected response.

permafrosh avatar Jun 03 '19 18:06 permafrosh

It does not terminate though. curl does not know when the response body is complete. Try this: curl "http://www.example.com/" --proxy localhost:8080 > /dev/null 2> /dev/null && echo SUCCESS It won't display SUCCESS. This one however, will display SUCCESS: curl "http://www.example.com/" > /dev/null 2> /dev/null && echo SUCCESS

itaisod avatar Jun 03 '19 21:06 itaisod

@itaisod What's the version of Go you're using?

hueich avatar Jun 09 '19 03:06 hueich

1.12.5

itaisod avatar Jun 09 '19 18:06 itaisod

After investigating, seems that example.com just doesn't respond with a Content-Length header, so Martian just faithfully proxies whatever it got. Try the same thing with another website without https, such as http://www.baidu.com/ and they return a Content-Length. Thanks for reporting this, and let us know if you find any more bugs in the future.

hueich avatar Jul 06 '19 15:07 hueich

image image image I dunno, to me it clearly looks like it's returning a Content-Length. But whatever.

itaisod avatar Nov 05 '19 13:11 itaisod

You're right. During testing I somehow mistook the proxied output for the no proxy output and that was missing the Content-Length header. I'm starting to suspect there's some order dependency in the header processing, and perhaps anything after the X-Cache header is getting dropped, which in the case of example.com was the Content-Length header. Will take a closer look soon.

hueich avatar Nov 05 '19 17:11 hueich

This is the behavior of Go's Transport, according to this issue.

If we want to retain the Content-Length header, we must set DisableCompression of http.Transport to true.

I want to create a PR, but should I add the Accept-Encoding: gzip header for every request like Go do, or I just let users decide if they want gzip or not?

thanks173 avatar Jun 17 '20 03:06 thanks173

I don't think that setting DisableCompression to true should be the default behavior. If you need this behavior, I would suggest writing your own main.go implementation and swapping in your own http.Transport with SetRoundTripper like here: https://github.com/google/martian/blob/master/cmd/proxy/main.go#L270

On Tue, Jun 16, 2020 at 11:49 PM Thanh Nguyen [email protected] wrote:

This is the behavior of Go's Transport, according to this issue https://github.com/golang/go/issues/18132.

If we want to retain the Content-Length header, we must set DisableCompression of http.Transport to true.

I want to create a PR, but should I add the Accept-Encoding: gzip header for every request like Go do, or I just let users decide if they want gzip or not?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/google/martian/issues/288#issuecomment-645130469, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAMR6NAUWS3LXPIBB3NAGDRXA4NZANCNFSM4HK3P3XA .

bramhaghosh avatar Jun 17 '20 15:06 bramhaghosh

So we just leave this bug there?

thanks173 avatar Jun 18 '20 03:06 thanks173

If martian can't provide Content-Length header in the response, at the very least it should close the connection when it's done and not just leave it hanging forever... Otherwise, it can either pipe the original data unmodified along with the original Content-Length and Content-Encoding, or read the whole data into a buffer first, and then set Content-Length in the response to whatever size it wants, compressed/uncompressed.

I'm 100% sure it's fixable. Not that I mind though, I'm not using martian.

itaisod avatar Jun 18 '20 10:06 itaisod

Yeah I think @itaisod is correct. The fix is to "pipe the original data unmodified along with the original Content-Length and Content-Encoding". My concern with setting DisableCompression to true is that it would be decompressed for all requests - regardless of whether they want it or not.

bramhaghosh avatar Jun 18 '20 15:06 bramhaghosh

My concern with setting DisableCompression to true is that it would be decompressed for all requests - regardless of whether they want it or not.

Setting DisableCompression to true will not disable compression for all request. Instead, it will respect the client's request: if the request contains the Accept-Encoding: gzip header, the response would be compressed, and then it's the client's responsibility to decompress the gzip response. The DisableCompression actually means DisableForcingCompression.

The fix is to "pipe the original data unmodified along with the original Content-Length and Content-Encoding".

With setting DisableCompression to false, all requests would be compressed by internal go package. go will do the decompression for us internally if the original request doesn't contain the Accept-Encoding: gzip header, so the original Content-Length and Content-Encoding would be incorrect, that's why they remove them.

I thinks that martian, as a proxy, should respect the client's request and let them decide if they want the Accept-Encoding: gzip header or not.

thanks173 avatar Jun 21 '20 03:06 thanks173