net/http: panic on invalid WriteHeader status code

Panic if an http Handler does:

    rw.WriteHeader(0)

... or other invalid values. (for a forgiving range of valid)

I previously made it kinda work in https://golang.org/cl/19130 but
there's no good way to fake it in HTTP/2, and we want HTTP/1 and
HTTP/2 behavior to be the same, regardless of what programs do.
Currently HTTP/2 omitted the :status header altogether, which was a
protocol violation. In fixing that, I found CL 19130 added a test
about bogus WriteHeader values with the comment:

  // This might change at some point, but not yet in Go 1.6.

This now changes. Time to be strict.

Updates golang/go#228800

Change-Id: I20eb6c0e514a31f4bba305ac4c24266f39b95fd5
Reviewed-on: https://go-review.googlesource.com/80077
Reviewed-by: Tom Bergan <tombergan@google.com>

diff --git a/src/net/http/clientserver_test.go b/src/net/http/clientserver_test.go
index 8738c8ff7c19e3419dae7bee4bb3e0614b5ab210..5017ebe468ff6fb91fc502a895386e43294b1fb0 100644 (file)
--- a/src/net/http/clientserver_test.go
+++ b/src/net/http/clientserver_test.go
@@ -1141,27 +1141,6 @@ func testTransportRejectsInvalidHeaders(t *testing.T, h2 bool) {
        }
 }
 
-// Tests that we support bogus under-100 HTTP statuses, because we historically
-// have. This might change at some point, but not yet in Go 1.6.
-func TestBogusStatusWorks_h1(t *testing.T) { testBogusStatusWorks(t, h1Mode) }
-func TestBogusStatusWorks_h2(t *testing.T) { testBogusStatusWorks(t, h2Mode) }
-func testBogusStatusWorks(t *testing.T, h2 bool) {
-       defer afterTest(t)
-       const code = 7
-       cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {
-               w.WriteHeader(code)
-       }))
-       defer cst.close()
-
-       res, err := cst.c.Get(cst.ts.URL)
-       if err != nil {
-               t.Fatal(err)
-       }
-       if res.StatusCode != code {
-               t.Errorf("StatusCode = %d; want %d", res.StatusCode, code)
-       }
-}
-
 func TestInterruptWithPanic_h1(t *testing.T)     { testInterruptWithPanic(t, h1Mode, "boom") }
 func TestInterruptWithPanic_h2(t *testing.T)     { testInterruptWithPanic(t, h2Mode, "boom") }
 func TestInterruptWithPanic_nil_h1(t *testing.T) { testInterruptWithPanic(t, h1Mode, nil) }
@@ -1408,3 +1387,43 @@ func TestBadResponseAfterReadingBody(t *testing.T) {
                t.Errorf("closes = %d; want 1", closes)
        }
 }
+
+func TestWriteHeader0_h1(t *testing.T) { testWriteHeader0(t, h1Mode) }
+func TestWriteHeader0_h2(t *testing.T) { testWriteHeader0(t, h2Mode) }
+func testWriteHeader0(t *testing.T, h2 bool) {
+       if h2 {
+               t.Skip("skipping until CL 80076 is vendored into std")
+       }
+       defer afterTest(t)
+       gotpanic := make(chan bool, 1)
+       cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {
+               defer close(gotpanic)
+               defer func() {
+                       if e := recover(); e != nil {
+                               got := fmt.Sprintf("%T, %v", e, e)
+                               want := "string, invalid WriteHeader code 0"
+                               if got != want {
+                                       t.Errorf("unexpected panic value:\n got: %v\nwant: %v\n", got, want)
+                               }
+                               gotpanic <- true
+
+                               // Set an explicit 503. This also tests that the WriteHeader call panics
+                               // before it recorded that an explicit value was set and that bogus
+                               // value wasn't stuck.
+                               w.WriteHeader(503)
+                       }
+               }()
+               w.WriteHeader(0)
+       }))
+       defer cst.close()
+       res, err := cst.c.Get(cst.ts.URL)
+       if err != nil {
+               t.Fatal(err)
+       }
+       if res.StatusCode != 503 {
+               t.Errorf("Response: %v %q; want 503", res.StatusCode, res.Status)
+       }
+       if !<-gotpanic {
+               t.Error("expected panic in handler")
+       }
+}

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 7a4ff88baf8da58e4c86476c47bee6d9fc703854..45877096e283581bcf0fdac6bca34422d9aaed14 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -1046,7 +1046,25 @@ func (w *response) Header() Header {
 // well read them)
 const maxPostHandlerReadBytes = 256 << 10
 
+func checkWriteHeaderCode(code int) {
+       // Issue 22880: require valid WriteHeader status codes.
+       // For now we only enforce that it's three digits.
+       // In the future we might block things over 599 (600 and above aren't defined
+       // at http://httpwg.org/specs/rfc7231.html#status.codes)
+       // and we might block under 200 (once we have more mature 1xx support).
+       // But for now any three digits.
+       //
+       // We used to send "HTTP/1.1 000 0" on the wire in responses but there's
+       // no equivalent bogus thing we can realistically send in HTTP/2,
+       // so we'll consistently panic instead and help people find their bugs
+       // early. (We can't return an error from WriteHeader even if we wanted to.)
+       if code < 100 || code > 999 {
+               panic(fmt.Sprintf("invalid WriteHeader code %v", code))
+       }
+}
+
 func (w *response) WriteHeader(code int) {
+       checkWriteHeaderCode(code)
        if w.conn.hijacked() {
                w.conn.server.logf("http: response.WriteHeader on hijacked connection")
                return
@@ -3140,6 +3158,7 @@ func (tw *timeoutWriter) Write(p []byte) (int, error) {
 }
 
 func (tw *timeoutWriter) WriteHeader(code int) {
+       checkWriteHeaderCode(code)
        tw.mu.Lock()
        defer tw.mu.Unlock()
        if tw.timedOut || tw.wroteHeader {