Consistently parse and validate user-provided status codes (#137)

mccutchen · web-flow · commit c9f4177de499 · 2023-07-10T12:58:36.000-04:00
In testing out error handling after #135, I happened to stumble across an unexpected panic for requests like `/status/1024` where the user-provided status code is outside the legal bounds. So, here we take a quick pass to ensure we're parsing and validating status codes the same way everywhere.
diff --git a/httpbin/handlers.go b/httpbin/handlers.go
@@ -254,9 +254,9 @@ func (h *HTTPBin) Status(w http.ResponseWriter, r *http.Request) {
 		writeError(w, http.StatusNotFound, nil)
 		return
 	}
-	code, err := strconv.Atoi(parts[2])
+	code, err := parseStatusCode(parts[2])
 	if err != nil {
-		writeError(w, http.StatusBadRequest, fmt.Errorf("invalid status %q: %w", parts[2], err))
+		writeError(w, http.StatusBadRequest, err)
 		return
 	}
 
@@ -297,7 +297,7 @@ func (h *HTTPBin) Unstable(w http.ResponseWriter, r *http.Request) {
 			writeError(w, http.StatusBadRequest, fmt.Errorf("invalid failure rate: %w", err))
 			return
 		} else if failureRate < 0 || failureRate > 1 {
-			writeError(w, http.StatusBadRequest, fmt.Errorf("invalid failure rate: %d not in interval [0, 1]", err))
+			writeError(w, http.StatusBadRequest, fmt.Errorf("invalid failure rate: %d not in range [0, 1]", err))
 			return
 		}
 	}
@@ -414,14 +414,10 @@ func (h *HTTPBin) RedirectTo(w http.ResponseWriter, r *http.Request) {
 	}
 
 	statusCode := http.StatusFound
-	rawStatusCode := q.Get("status_code")
-	if rawStatusCode != "" {
-		statusCode, err = strconv.Atoi(q.Get("status_code"))
+	if userStatusCode := q.Get("status_code"); userStatusCode != "" {
+		statusCode, err = parseBoundedStatusCode(userStatusCode, 300, 399)
 		if err != nil {
-			writeError(w, http.StatusBadRequest, fmt.Errorf("invalid status code: %w", err))
-			return
-		} else if statusCode < 300 || statusCode > 399 {
-			writeError(w, http.StatusBadRequest, errors.New("invalid status code: must be in range [300, 399]"))
+			writeError(w, http.StatusBadRequest, err)
 			return
 		}
 	}
@@ -617,18 +613,15 @@ func (h *HTTPBin) Drip(w http.ResponseWriter, r *http.Request) {
 			writeError(w, http.StatusBadRequest, fmt.Errorf("invalid numbytes: %w", err))
 			return
 		} else if numBytes < 1 || numBytes > h.MaxBodySize {
-			writeError(w, http.StatusBadRequest, fmt.Errorf("invalid numbytes: %d not in interval [1, %d]", numBytes, h.MaxBodySize))
+			writeError(w, http.StatusBadRequest, fmt.Errorf("invalid numbytes: %d not in range [1, %d]", numBytes, h.MaxBodySize))
 			return
 		}
 	}
 
 	if userCode := q.Get("code"); userCode != "" {
-		code, err = strconv.Atoi(userCode)
+		code, err = parseStatusCode(userCode)
 		if err != nil {
-			writeError(w, http.StatusBadRequest, fmt.Errorf("invalid code: %w", err))
-			return
-		} else if code < 100 || code >= 600 {
-			writeError(w, http.StatusBadRequest, fmt.Errorf("invalid code: %d not in interval [100, 599]", code))
+			writeError(w, http.StatusBadRequest, err)
 			return
 		}
 	}
@@ -713,7 +706,7 @@ func (h *HTTPBin) Range(w http.ResponseWriter, r *http.Request) {
 	w.Header().Add("Accept-Ranges", "bytes")
 
 	if numBytes <= 0 || numBytes > h.MaxBodySize {
-		writeError(w, http.StatusBadRequest, fmt.Errorf("invalid count: %d not in interval [1, %d]", numBytes, h.MaxBodySize))
+		writeError(w, http.StatusBadRequest, fmt.Errorf("invalid count: %d not in range [1, %d]", numBytes, h.MaxBodySize))
 		return
 	}
 
diff --git a/httpbin/handlers_test.go b/httpbin/handlers_test.go
@@ -799,6 +799,7 @@ func TestStatus(t *testing.T) {
 		headers map[string]string
 		body    string
 	}{
+		// 100 is tested as a special case below
 		{200, nil, ""},
 		{300, map[string]string{"Location": "/image/jpeg"}, `<!doctype html>
 <head>
@@ -822,6 +823,8 @@ func TestStatus(t *testing.T) {
 </html>`},
 		{401, unauthorizedHeaders, ""},
 		{418, nil, "I'm a teapot!"},
+		{500, nil, ""}, // maximum allowed status code
+		{599, nil, ""}, // maximum allowed status code
 	}
 
 	for _, test := range tests {
@@ -848,6 +851,8 @@ func TestStatus(t *testing.T) {
 		{"/status/200/foo", http.StatusNotFound},
 		{"/status/3.14", http.StatusBadRequest},
 		{"/status/foo", http.StatusBadRequest},
+		{"/status/600", http.StatusBadRequest},
+		{"/status/1024", http.StatusBadRequest},
 	}
 
 	for _, test := range errorTests {
@@ -860,6 +865,34 @@ func TestStatus(t *testing.T) {
 			assert.StatusCode(t, resp, test.status)
 		})
 	}
+
+	t.Run("HTTP 100 Continue status code supported", func(t *testing.T) {
+		// The stdlib http client automagically handles 100 Continue responses
+		// by continuing the request until a "final" 200 OK response is
+		// received, which prevents us from confirming that a 100 Continue
+		// response is sent when using the http client directly.
+		//
+		// So, here we instead manally write the request to the wire and read
+		// the initial response, which will give us access to the 100 Continue
+		// indication we need.
+		t.Parallel()
+
+		conn, err := net.Dial("tcp", srv.Listener.Addr().String())
+		assert.NilError(t, err)
+		defer conn.Close()
+
+		req := newTestRequest(t, "GET", "/status/100")
+		reqBytes, err := httputil.DumpRequestOut(req, false)
+		assert.NilError(t, err)
+
+		n, err := conn.Write(reqBytes)
+		assert.NilError(t, err)
+		assert.Equal(t, n, len(reqBytes), "incorrect number of bytes written")
+
+		resp, err := http.ReadResponse(bufio.NewReader(conn), req)
+		assert.NilError(t, err)
+		assert.StatusCode(t, resp, http.StatusContinue)
+	})
 }
 
 func TestUnstable(t *testing.T) {
diff --git a/httpbin/helpers.go b/httpbin/helpers.go
@@ -234,6 +234,21 @@ func encodeData(body []byte, contentType string) string {
 	return string("data:" + contentType + ";base64," + data)
 }
 
+func parseStatusCode(input string) (int, error) {
+	return parseBoundedStatusCode(input, 100, 599)
+}
+
+func parseBoundedStatusCode(input string, min, max int) (int, error) {
+	code, err := strconv.Atoi(input)
+	if err != nil {
+		return 0, fmt.Errorf("invalid status code: %q: %w", input, err)
+	}
+	if code < min || code > max {
+		return 0, fmt.Errorf("invalid status code: %d not in range [%d, %d]", code, min, max)
+	}
+	return code, nil
+}
+
 // parseDuration takes a user's input as a string and attempts to convert it
 // into a time.Duration. If not given as a go-style duration string, the input
 // is assumed to be seconds as a float.

Original file line number	Diff line number	Diff line change
`@@ -254,9 +254,9 @@ func (h HTTPBin) Status(w http.ResponseWriter, r http.Request) {`
`254`	`254`	`writeError(w, http.StatusNotFound, nil)`
`255`	`255`	`return`
`256`	`256`	`}`
`257`		`- code, err := strconv.Atoi(parts[2])`
	`257`	`+ code, err := parseStatusCode(parts[2])`
`258`	`258`	`if err != nil {`
`259`		`- writeError(w, http.StatusBadRequest, fmt.Errorf("invalid status %q: %w", parts[2], err))`
	`259`	`+ writeError(w, http.StatusBadRequest, err)`
`260`	`260`	`return`
`261`	`261`	`}`
`262`	`262`
`@@ -297,7 +297,7 @@ func (h HTTPBin) Unstable(w http.ResponseWriter, r http.Request) {`
`297`	`297`	`writeError(w, http.StatusBadRequest, fmt.Errorf("invalid failure rate: %w", err))`
`298`	`298`	`return`
`299`	`299`	`} else if failureRate < 0 \|\| failureRate > 1 {`
`300`		`- writeError(w, http.StatusBadRequest, fmt.Errorf("invalid failure rate: %d not in interval [0, 1]", err))`
	`300`	`+ writeError(w, http.StatusBadRequest, fmt.Errorf("invalid failure rate: %d not in range [0, 1]", err))`
`301`	`301`	`return`
`302`	`302`	`}`
`303`	`303`	`}`
`@@ -414,14 +414,10 @@ func (h HTTPBin) RedirectTo(w http.ResponseWriter, r http.Request) {`
`414`	`414`	`}`
`415`	`415`
`416`	`416`	`statusCode := http.StatusFound`
`417`		`- rawStatusCode := q.Get("status_code")`
`418`		`- if rawStatusCode != "" {`
`419`		`- statusCode, err = strconv.Atoi(q.Get("status_code"))`
	`417`	`+ if userStatusCode := q.Get("status_code"); userStatusCode != "" {`
	`418`	`+ statusCode, err = parseBoundedStatusCode(userStatusCode, 300, 399)`
`420`	`419`	`if err != nil {`
`421`		`- writeError(w, http.StatusBadRequest, fmt.Errorf("invalid status code: %w", err))`
`422`		`- return`
`423`		`- } else if statusCode < 300 \|\| statusCode > 399 {`
`424`		`- writeError(w, http.StatusBadRequest, errors.New("invalid status code: must be in range [300, 399]"))`
	`420`	`+ writeError(w, http.StatusBadRequest, err)`
`425`	`421`	`return`
`426`	`422`	`}`
`427`	`423`	`}`
`@@ -617,18 +613,15 @@ func (h HTTPBin) Drip(w http.ResponseWriter, r http.Request) {`
`617`	`613`	`writeError(w, http.StatusBadRequest, fmt.Errorf("invalid numbytes: %w", err))`
`618`	`614`	`return`
`619`	`615`	`} else if numBytes < 1 \|\| numBytes > h.MaxBodySize {`
`620`		`- writeError(w, http.StatusBadRequest, fmt.Errorf("invalid numbytes: %d not in interval [1, %d]", numBytes, h.MaxBodySize))`
	`616`	`+ writeError(w, http.StatusBadRequest, fmt.Errorf("invalid numbytes: %d not in range [1, %d]", numBytes, h.MaxBodySize))`
`621`	`617`	`return`
`622`	`618`	`}`
`623`	`619`	`}`
`624`	`620`
`625`	`621`	`if userCode := q.Get("code"); userCode != "" {`
`626`		`- code, err = strconv.Atoi(userCode)`
	`622`	`+ code, err = parseStatusCode(userCode)`
`627`	`623`	`if err != nil {`
`628`		`- writeError(w, http.StatusBadRequest, fmt.Errorf("invalid code: %w", err))`
`629`		`- return`
`630`		`- } else if code < 100 \|\| code >= 600 {`
`631`		`- writeError(w, http.StatusBadRequest, fmt.Errorf("invalid code: %d not in interval [100, 599]", code))`
	`624`	`+ writeError(w, http.StatusBadRequest, err)`
`632`	`625`	`return`
`633`	`626`	`}`
`634`	`627`	`}`
`@@ -713,7 +706,7 @@ func (h HTTPBin) Range(w http.ResponseWriter, r http.Request) {`
`713`	`706`	`w.Header().Add("Accept-Ranges", "bytes")`
`714`	`707`
`715`	`708`	`if numBytes <= 0 \|\| numBytes > h.MaxBodySize {`
`716`		`- writeError(w, http.StatusBadRequest, fmt.Errorf("invalid count: %d not in interval [1, %d]", numBytes, h.MaxBodySize))`
	`709`	`+ writeError(w, http.StatusBadRequest, fmt.Errorf("invalid count: %d not in range [1, %d]", numBytes, h.MaxBodySize))`
`717`	`710`	`return`
`718`	`711`	`}`
`719`	`712`