net/http/httputil: ReverseProxy does not set flushinterval to -1 for Content-Type: text/event-stream in all cases

[cmfcmf]

What version of Go are you using (go version)?

1.13. The issue/oversight seems to exists at least since 1.12, since the corresponding functionality was introduced here: 5440bfc

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""

GOARCH="amd64"

GOBIN=""

GOCACHE="/home/cmfcmf/.cache/go-build"

GOENV="/home/cmfcmf/.config/go/env"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="linux"

GONOPROXY=""

GONOSUMDB=""

GOOS="linux"

GOPATH="/home/cmfcmf/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/usr/lib/go-1.13"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/usr/lib/go-1.13/pkg/tool/linux_amd64"

GCCGO="gccgo"

AR="ar"

CC="gcc"

CXX="g++"

CGO_ENABLED="1"

GOMOD="/home/cmfcmf/dev/go/src/go.mod"

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build362045526=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Originally, I was using the Caddy web server and noticed that websocket streams were not correctly reverse-proxied when their Content-Type header included a charset parameter (text/event-stream;charset=utf-8). See caddyserver/caddy#3765 for the issue with Caddy.
I am raising the same issue here, since Caddy's code is based on the code found in go, which appears to have the same problem/oversight.

What did you expect to see?

I expected my reverse-proxied websocket connection to work, regardless of whether or not the optional charset parameter is specified.

What did you see instead?

The connection did not work, since the request/response was not flushed.

---

This only happens when the websocket server uses the RFC 1521 optional "charset" parameter as part of the Content-Type header. This parameter is also documented here: https://www.w3.org/TR/eventsource/#text-event-stream. Webservers that use this parameter will send a content-type header like text/event-stream;charset=utf-8.
The code causing this problem is located here:

go/src/net/http/httputil/reverseproxy.go

Lines 401 to 416 in 052da57

	func (p *ReverseProxy) flushInterval(res *http.Response) time.Duration {
	resCT := res.Header.Get("Content-Type")
	// For Server-Sent Events responses, flush immediately.
	// The MIME type is defined in https://www.w3.org/TR/eventsource/#text-event-stream
	if resCT == "text/event-stream" {
	return -1 // negative means immediately
	}
	// We might have the case of streaming for which Content-Length might be unset.
	if res.ContentLength == -1 {
	return -1
	}
	return p.FlushInterval
	}

You can see how it returns -1 for responses with a text/event-stream content type. It should be extended to ignore the charset part of the Content-Type header, similar to what I proposed in my pull request for Caddy: https://github.com/caddyserver/caddy/pull/3758/files, so that it also returns -1 for responses with a text/event-stream;charset=utf-8content-type header.

I have a patch ready to fix that (very similar to the patch I proposed for Caddy), which I can submit on Gerrit/GitHub. However, as per the contribution guidelines, I wanted to open an issue first to discuss potential implications.

[fraenkel]

@cmfcmf Feel free to submit a fix. The RFC states the charset is optional so it was just an oversight in the implementation.
Also include a test.

[gopherbot]

Change https://golang.org/cl/336929 mentions this issue: net/http/httputil/reverseproxy: set the flush interval to -1 even if the Content-Type includes additional parameters

[thanm]

@neild

[gopherbot]

Change https://golang.org/cl/340529 mentions this issue: net/http/httputil: use mime.ParseMediaType to parse Content-Type header

[neild]

This was fixed by https://go.dev/cl/350509.