api/server/httputils: add ReadJSON() utility

Implement a ReadJSON() utility to help reduce some code-duplication,
and to make sure we handle JSON requests consistently (e.g. always
check for the content-type).

Differences compared to current handling:

- prevent possible panic if request.Body is nil ("should never happen")
- always require Content-Type to be "application/json"
- be stricter about additional content after JSON (previously ignored)
- but, allow the body to be empty (an empty body is not invalid);
  update TestContainerInvalidJSON accordingly, which was testing the
  wrong expectation.
- close body after reading (some code did this)

We should consider to add a "max body size" on this function, similar to
https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/api/server/middleware/debug.go#L27-L40

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

4 files changed, 180 insertions, 62 deletions

diff --git a/integration/container/container_test.go b/integration/container/container_test.go
index 0402828a77..06dce0f776 100644
--- a/integration/container/container_test.go
+++ b/integration/container/container_test.go
@@ -2,6 +2,7 @@ package container // import "github.com/docker/docker/integration/container"
 
 import (
 	"net/http"
+	"runtime"
 	"testing"
 
 	"github.com/docker/docker/testutil/request"
@@ -9,34 +10,69 @@ import (
 	is "gotest.tools/v3/assert/cmp"
 )
 
+// TestContainerInvalidJSON tests that POST endpoints that expect a body return
+// the correct error when sending invalid JSON requests.
 func TestContainerInvalidJSON(t *testing.T) {
 	defer setupTest(t)()
 
+	// POST endpoints that accept / expect a JSON body;
 	endpoints := []string{
 		"/containers/foobar/exec",
+		"/containers/foobar/update",
 		"/exec/foobar/start",
 	}
 
+	// windows doesnt support API < v1.24
+	if runtime.GOOS != "windows" {
+		endpoints = append(
+			endpoints,
+			"/v1.23/containers/foobar/copy", // deprecated since 1.8 (API v1.20), errors out since 1.12 (API v1.24)
+		)
+	}
+
 	for _, ep := range endpoints {
 		ep := ep
-		t.Run(ep, func(t *testing.T) {
+		t.Run(ep[1:], func(t *testing.T) {
 			t.Parallel()
 
-			res, body, err := request.Post(ep, request.RawString("{invalid json"), request.JSON)
-			assert.NilError(t, err)
-			assert.Equal(t, res.StatusCode, http.StatusBadRequest)
+			t.Run("invalid content type", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString("{}"), request.ContentType("text/plain"))
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "unsupported Content-Type header (text/plain): must be 'application/json'"))
+			})
+
+			t.Run("invalid JSON", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString("{invalid json"), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "invalid JSON: invalid character 'i' looking for beginning of object key string"))
+			})
 
-			buf, err := request.ReadBody(body)
-			assert.NilError(t, err)
-			assert.Check(t, is.Contains(string(buf), "invalid character 'i' looking for beginning of object key string"))
+			t.Run("extra content after JSON", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString(`{} trailing content`), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
 
-			res, body, err = request.Post(ep, request.JSON)
-			assert.NilError(t, err)
-			assert.Equal(t, res.StatusCode, http.StatusBadRequest)
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "unexpected content after JSON"))
+			})
 
-			buf, err = request.ReadBody(body)
-			assert.NilError(t, err)
-			assert.Check(t, is.Contains(string(buf), "got EOF while reading request body"))
+			t.Run("empty body", func(t *testing.T) {
+				// empty body should not produce an 500 internal server error, or
+				// any 5XX error (this is assuming the request does not produce
+				// an internal server error for another reason, but it shouldn't)
+				res, _, err := request.Post(ep, request.RawString(``), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, res.StatusCode < http.StatusInternalServerError)
+			})
 		})
 	}
 }
diff --git a/integration/network/network_test.go b/integration/network/network_test.go
index ec0546aa0e..00603094a2 100644
--- a/integration/network/network_test.go
+++ b/integration/network/network_test.go
@@ -62,9 +62,12 @@ func TestRunContainerWithBridgeNone(t *testing.T) {
 	assert.Check(t, is.Equal(stdout.String(), result.Combined()), "The network namespace of container should be the same with host when --net=host and bridge network is disabled")
 }
 
+// TestNetworkInvalidJSON tests that POST endpoints that expect a body return
+// the correct error when sending invalid JSON requests.
 func TestNetworkInvalidJSON(t *testing.T) {
 	defer setupTest(t)()
 
+	// POST endpoints that accept / expect a JSON body;
 	endpoints := []string{
 		"/networks/create",
 		"/networks/bridge/connect",
@@ -73,24 +76,47 @@ func TestNetworkInvalidJSON(t *testing.T) {
 
 	for _, ep := range endpoints {
 		ep := ep
-		t.Run(ep, func(t *testing.T) {
+		t.Run(ep[1:], func(t *testing.T) {
 			t.Parallel()
 
-			res, body, err := request.Post(ep, request.RawString("{invalid json"), request.JSON)
-			assert.NilError(t, err)
-			assert.Equal(t, res.StatusCode, http.StatusBadRequest)
-
-			buf, err := request.ReadBody(body)
-			assert.NilError(t, err)
-			assert.Check(t, is.Contains(string(buf), "invalid character 'i' looking for beginning of object key string"))
-
-			res, body, err = request.Post(ep, request.JSON)
-			assert.NilError(t, err)
-			assert.Equal(t, res.StatusCode, http.StatusBadRequest)
-
-			buf, err = request.ReadBody(body)
-			assert.NilError(t, err)
-			assert.Check(t, is.Contains(string(buf), "got EOF while reading request body"))
+			t.Run("invalid content type", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString("{}"), request.ContentType("text/plain"))
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "unsupported Content-Type header (text/plain): must be 'application/json'"))
+			})
+
+			t.Run("invalid JSON", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString("{invalid json"), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "invalid JSON: invalid character 'i' looking for beginning of object key string"))
+			})
+
+			t.Run("extra content after JSON", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString(`{} trailing content`), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "unexpected content after JSON"))
+			})
+
+			t.Run("empty body", func(t *testing.T) {
+				// empty body should not produce an 500 internal server error, or
+				// any 5XX error (this is assuming the request does not produce
+				// an internal server error for another reason, but it shouldn't)
+				res, _, err := request.Post(ep, request.RawString(``), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, res.StatusCode < http.StatusInternalServerError)
+			})
 		})
 	}
 }
diff --git a/integration/plugin/common/plugin_test.go b/integration/plugin/common/plugin_test.go
index fb37a5e174..37ef0b3374 100644
--- a/integration/plugin/common/plugin_test.go
+++ b/integration/plugin/common/plugin_test.go
@@ -29,31 +29,61 @@ import (
 	"gotest.tools/v3/skip"
 )
 
+// TestPluginInvalidJSON tests that POST endpoints that expect a body return
+// the correct error when sending invalid JSON requests.
 func TestPluginInvalidJSON(t *testing.T) {
 	defer setupTest(t)()
 
-	endpoints := []string{"/plugins/foobar/set"}
+	// POST endpoints that accept / expect a JSON body;
+	endpoints := []string{
+		"/plugins/foobar/set",
+		"/plugins/foobar/upgrade",
+		"/plugins/pull",
+	}
 
 	for _, ep := range endpoints {
 		ep := ep
-		t.Run(ep, func(t *testing.T) {
+		t.Run(ep[1:], func(t *testing.T) {
 			t.Parallel()
 
-			res, body, err := request.Post(ep, request.RawString("{invalid json"), request.JSON)
-			assert.NilError(t, err)
-			assert.Equal(t, res.StatusCode, http.StatusBadRequest)
-
-			buf, err := request.ReadBody(body)
-			assert.NilError(t, err)
-			assert.Check(t, is.Contains(string(buf), "invalid character 'i' looking for beginning of object key string"))
-
-			res, body, err = request.Post(ep, request.JSON)
-			assert.NilError(t, err)
-			assert.Equal(t, res.StatusCode, http.StatusBadRequest)
-
-			buf, err = request.ReadBody(body)
-			assert.NilError(t, err)
-			assert.Check(t, is.Contains(string(buf), "got EOF while reading request body"))
+			t.Run("invalid content type", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString("[]"), request.ContentType("text/plain"))
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "unsupported Content-Type header (text/plain): must be 'application/json'"))
+			})
+
+			t.Run("invalid JSON", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString("{invalid json"), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "invalid JSON: invalid character 'i' looking for beginning of object key string"))
+			})
+
+			t.Run("extra content after JSON", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString(`[] trailing content`), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "unexpected content after JSON"))
+			})
+
+			t.Run("empty body", func(t *testing.T) {
+				// empty body should not produce an 500 internal server error, or
+				// any 5XX error (this is assuming the request does not produce
+				// an internal server error for another reason, but it shouldn't)
+				res, _, err := request.Post(ep, request.RawString(``), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, res.StatusCode < http.StatusInternalServerError)
+			})
 		})
 	}
 }
diff --git a/integration/volume/volume_test.go b/integration/volume/volume_test.go
index f465e47b05..395c51e1f4 100644
--- a/integration/volume/volume_test.go
+++ b/integration/volume/volume_test.go
@@ -104,31 +104,57 @@ func TestVolumesInspect(t *testing.T) {
 	assert.Check(t, createdAt.Unix()-now.Unix() < 60, "CreatedAt (%s) exceeds creation time (%s) 60s", createdAt, now)
 }
 
+// TestVolumesInvalidJSON tests that POST endpoints that expect a body return
+// the correct error when sending invalid JSON requests.
 func TestVolumesInvalidJSON(t *testing.T) {
 	defer setupTest(t)()
 
+	// POST endpoints that accept / expect a JSON body;
 	endpoints := []string{"/volumes/create"}
 
 	for _, ep := range endpoints {
 		ep := ep
-		t.Run(ep, func(t *testing.T) {
+		t.Run(ep[1:], func(t *testing.T) {
 			t.Parallel()
 
-			res, body, err := request.Post(ep, request.RawString("{invalid json"), request.JSON)
-			assert.NilError(t, err)
-			assert.Equal(t, res.StatusCode, http.StatusBadRequest)
-
-			buf, err := request.ReadBody(body)
-			assert.NilError(t, err)
-			assert.Check(t, is.Contains(string(buf), "invalid character 'i' looking for beginning of object key string"))
-
-			res, body, err = request.Post(ep, request.JSON)
-			assert.NilError(t, err)
-			assert.Equal(t, res.StatusCode, http.StatusBadRequest)
-
-			buf, err = request.ReadBody(body)
-			assert.NilError(t, err)
-			assert.Check(t, is.Contains(string(buf), "got EOF while reading request body"))
+			t.Run("invalid content type", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString("{}"), request.ContentType("text/plain"))
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "unsupported Content-Type header (text/plain): must be 'application/json'"))
+			})
+
+			t.Run("invalid JSON", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString("{invalid json"), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "invalid JSON: invalid character 'i' looking for beginning of object key string"))
+			})
+
+			t.Run("extra content after JSON", func(t *testing.T) {
+				res, body, err := request.Post(ep, request.RawString(`{} trailing content`), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, is.Equal(res.StatusCode, http.StatusBadRequest))
+
+				buf, err := request.ReadBody(body)
+				assert.NilError(t, err)
+				assert.Check(t, is.Contains(string(buf), "unexpected content after JSON"))
+			})
+
+			t.Run("empty body", func(t *testing.T) {
+				// empty body should not produce an 500 internal server error, or
+				// any 5XX error (this is assuming the request does not produce
+				// an internal server error for another reason, but it shouldn't)
+				res, _, err := request.Post(ep, request.RawString(``), request.JSON)
+				assert.NilError(t, err)
+				assert.Check(t, res.StatusCode < http.StatusInternalServerError)
+			})
 		})
 	}
 }