[bug] Panic on nil Hijack interface due to compress handler

[muirdm]

Describe the bug

The compress handler leaves an empty http.Hijacker interface in the wrapped response writer even when the incoming response writer does not implement http.Hijacker. This causes the wrapped response writer to still implement http.Hijacker, but if you try to call Hijack() it will panic.

Steps to Reproduce

type myWriter struct {
	http.ResponseWriter
}

func main() {
	http.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request) {
		if h, ok := w.(http.Hijacker); ok {
			h.Hijack()
		}
	})

	myMiddleware := func(h http.Handler) http.Handler {
		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			h.ServeHTTP(&myWriter{w}, r)
		})
	}

	handler := myMiddleware(handlers.CompressHandler(http.DefaultServeMux))

	go func() {
		log.Panic(http.ListenAndServe("127.0.0.1:1337", handler))
	}()

	time.Sleep(1 * time.Second)

	http.Get("http://127.0.0.1:1337/test")
}

[stale]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

[muirdm]

Can a project contributor shares their thoughts before this is automatically closed? I don't think an issue with no responses should be marked as stale.

[elithrar]

This doesn’t seem ideal. Are you willing to submit a PR + tests to fix?

Happy to review & merge but don’t have time to push a PR for a while.

[fharding1]

Agreed, this seems weird. When I changed how CompressHandlerLevel works a bit I thought that code for checking interfaces and creating the compressResponseWriter was a bit smelly but I didn't really check it out since that's not what I was focused on changing. I can also help out with a review, or push a PR myself if needed. 👍

[stale]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

[muirdm]

Sorry, I'm not able to submit a PR because I don't know what the best solution is. Here is an example of what is required for proper ResponseWriter wrapping: https://github.com/aws/aws-xray-sdk-go/blob/master/xray/response_capturer.go.

Since gorilla is widely used, what do you think about gorilla providing a wrapping mechanism other libraries can use to safely wrap a ResponseWriter?

[muirdm]

Since my last comment we found the "httpsnoop" package which seems like the best option to properly wrap response writers. It uses code generation to cover all the cases and handles changes across Go versions properly. We used it to fix a similar problem in opentelemetry. If that looks reasonable I can open a PR using that package.

[elithrar]

@muirdm Seems pretty straightforward - would love a PR for this.