From c7b488fbc82604ec23b862ec1edc5a0be9c7793d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 5 Nov 2018 14:50:33 +0100 Subject: [PATCH] API: properly handle invalid JSON to return a 400 status The API did not treat invalid JSON payloads as a 400 error, as a result returning a 500 error; Before this change, an invalid JSON body would return a 500 error; ```bash curl -v \ --unix-socket /var/run/docker.sock \ -X POST \ "http://localhost/v1.30/networks/create" \ -H "Content-Type: application/json" \ -d '{invalid json' ``` ``` > POST /v1.30/networks/create HTTP/1.1 > Host: localhost > User-Agent: curl/7.52.1 > Accept: */* > Content-Type: application/json > Content-Length: 13 > * upload completely sent off: 13 out of 13 bytes < HTTP/1.1 500 Internal Server Error < Api-Version: 1.40 < Content-Type: application/json < Docker-Experimental: false < Ostype: linux < Server: Docker/dev (linux) < Date: Mon, 05 Nov 2018 11:55:20 GMT < Content-Length: 79 < {"message":"invalid character 'i' looking for beginning of object key string"} ``` Empty request: ```bash curl -v \ --unix-socket /var/run/docker.sock \ -X POST \ "http://localhost/v1.30/networks/create" \ -H "Content-Type: application/json" ``` ``` > POST /v1.30/networks/create HTTP/1.1 > Host: localhost > User-Agent: curl/7.54.0 > Accept: */* > Content-Type: application/json > < HTTP/1.1 500 Internal Server Error < Api-Version: 1.38 < Content-Length: 18 < Content-Type: application/json < Date: Mon, 05 Nov 2018 12:00:18 GMT < Docker-Experimental: true < Ostype: linux < Server: Docker/18.06.1-ce (linux) < {"message":"EOF"} ``` After this change, a 400 is returned; ```bash curl -v \ --unix-socket /var/run/docker.sock \ -X POST \ "http://localhost/v1.30/networks/create" \ -H "Content-Type: application/json" \ -d '{invalid json' ``` ``` > POST /v1.30/networks/create HTTP/1.1 > Host: localhost > User-Agent: curl/7.52.1 > Accept: */* > Content-Type: application/json > Content-Length: 13 > * upload completely sent off: 13 out of 13 bytes < HTTP/1.1 400 Bad Request < Api-Version: 1.40 < Content-Type: application/json < Docker-Experimental: false < Ostype: linux < Server: Docker/dev (linux) < Date: Mon, 05 Nov 2018 11:57:15 GMT < Content-Length: 79 < {"message":"invalid character 'i' looking for beginning of object key string"} ``` Empty request: ```bash curl -v \ --unix-socket /var/run/docker.sock \ -X POST \ "http://localhost/v1.30/networks/create" \ -H "Content-Type: application/json" ``` ``` > POST /v1.30/networks/create HTTP/1.1 > Host: localhost > User-Agent: curl/7.52.1 > Accept: */* > Content-Type: application/json > < HTTP/1.1 400 Bad Request < Api-Version: 1.40 < Content-Type: application/json < Docker-Experimental: false < Ostype: linux < Server: Docker/dev (linux) < Date: Mon, 05 Nov 2018 11:59:22 GMT < Content-Length: 49 < {"message":"got EOF while reading request body"} ``` Signed-off-by: Sebastiaan van Stijn --- api/server/router/container/copy.go | 7 ++- api/server/router/container/exec.go | 11 ++++- api/server/router/network/network_routes.go | 16 +++++-- api/server/router/plugin/plugin_routes.go | 7 ++- api/server/router/swarm/cluster_routes.go | 52 +++++++++++++++++---- api/server/router/volume/volume_routes.go | 2 +- integration/container/container_test.go | 42 +++++++++++++++++ integration/network/network_test.go | 34 ++++++++++++++ integration/plugin/common/main_test.go | 27 +++++++++++ integration/plugin/common/plugin_test.go | 38 +++++++++++++++ integration/volume/volume_test.go | 29 ++++++++++++ 11 files changed, 248 insertions(+), 17 deletions(-) create mode 100644 integration/container/container_test.go create mode 100644 integration/plugin/common/main_test.go create mode 100644 integration/plugin/common/plugin_test.go diff --git a/api/server/router/container/copy.go b/api/server/router/container/copy.go index 837836d001..47de35a455 100644 --- a/api/server/router/container/copy.go +++ b/api/server/router/container/copy.go @@ -6,12 +6,14 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "io" "net/http" "github.com/docker/docker/api/server/httputils" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/errdefs" gddohttputil "github.com/golang/gddo/httputil" ) @@ -37,7 +39,10 @@ func (s *containerRouter) postContainersCopy(ctx context.Context, w http.Respons cfg := types.CopyConfig{} if err := json.NewDecoder(r.Body).Decode(&cfg); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } if cfg.Resource == "" { diff --git a/api/server/router/container/exec.go b/api/server/router/container/exec.go index 25125edb5f..cf92be1d2b 100644 --- a/api/server/router/container/exec.go +++ b/api/server/router/container/exec.go @@ -3,6 +3,7 @@ package container // import "github.com/docker/docker/api/server/router/containe import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -44,7 +45,10 @@ func (s *containerRouter) postContainerExecCreate(ctx context.Context, w http.Re execConfig := &types.ExecConfig{} if err := json.NewDecoder(r.Body).Decode(execConfig); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } if len(execConfig.Cmd) == 0 { @@ -84,7 +88,10 @@ func (s *containerRouter) postContainerExecStart(ctx context.Context, w http.Res execStartCheck := &types.ExecStartCheck{} if err := json.NewDecoder(r.Body).Decode(execStartCheck); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } if exists, err := s.backend.ExecExists(execName); !exists { diff --git a/api/server/router/network/network_routes.go b/api/server/router/network/network_routes.go index 8b9382cb45..aa8280c8ac 100644 --- a/api/server/router/network/network_routes.go +++ b/api/server/router/network/network_routes.go @@ -3,6 +3,7 @@ package network // import "github.com/docker/docker/api/server/router/network" import ( "context" "encoding/json" + "io" "net/http" "strconv" "strings" @@ -215,7 +216,10 @@ func (n *networkRouter) postNetworkCreate(ctx context.Context, w http.ResponseWr } if err := json.NewDecoder(r.Body).Decode(&create); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } if nws, err := n.cluster.GetNetworksByName(create.Name); err == nil && len(nws) > 0 { @@ -261,7 +265,10 @@ func (n *networkRouter) postNetworkConnect(ctx context.Context, w http.ResponseW } if err := json.NewDecoder(r.Body).Decode(&connect); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } // Unlike other operations, we does not check ambiguity of the name/ID here. @@ -282,7 +289,10 @@ func (n *networkRouter) postNetworkDisconnect(ctx context.Context, w http.Respon } if err := json.NewDecoder(r.Body).Decode(&disconnect); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } return n.backend.DisconnectContainerFromNetwork(disconnect.Container, vars["id"], disconnect.Force) diff --git a/api/server/router/plugin/plugin_routes.go b/api/server/router/plugin/plugin_routes.go index 4e816391d1..9508ca8c9d 100644 --- a/api/server/router/plugin/plugin_routes.go +++ b/api/server/router/plugin/plugin_routes.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "encoding/json" + "io" "net/http" "strconv" "strings" @@ -12,6 +13,7 @@ import ( "github.com/docker/docker/api/server/httputils" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/streamformatter" "github.com/pkg/errors" @@ -276,7 +278,10 @@ func (pr *pluginRouter) pushPlugin(ctx context.Context, w http.ResponseWriter, r func (pr *pluginRouter) setPlugin(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var args []string if err := json.NewDecoder(r.Body).Decode(&args); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } if err := pr.backend.Set(vars["name"], args); err != nil { return err diff --git a/api/server/router/swarm/cluster_routes.go b/api/server/router/swarm/cluster_routes.go index 600551714c..dda2a92d91 100644 --- a/api/server/router/swarm/cluster_routes.go +++ b/api/server/router/swarm/cluster_routes.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "strconv" @@ -21,7 +22,10 @@ import ( func (sr *swarmRouter) initCluster(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var req types.InitRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } nodeID, err := sr.backend.Init(req) if err != nil { @@ -34,7 +38,10 @@ func (sr *swarmRouter) initCluster(ctx context.Context, w http.ResponseWriter, r func (sr *swarmRouter) joinCluster(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var req types.JoinRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } return sr.backend.Join(req) } @@ -61,7 +68,10 @@ func (sr *swarmRouter) inspectCluster(ctx context.Context, w http.ResponseWriter func (sr *swarmRouter) updateCluster(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var swarm types.Spec if err := json.NewDecoder(r.Body).Decode(&swarm); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } rawVersion := r.URL.Query().Get("version") @@ -112,7 +122,10 @@ func (sr *swarmRouter) updateCluster(ctx context.Context, w http.ResponseWriter, func (sr *swarmRouter) unlockCluster(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var req types.UnlockRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } if err := sr.backend.UnlockSwarm(req); err != nil { @@ -175,7 +188,10 @@ func (sr *swarmRouter) getService(ctx context.Context, w http.ResponseWriter, r func (sr *swarmRouter) createService(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var service types.ServiceSpec if err := json.NewDecoder(r.Body).Decode(&service); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } // Get returns "" if the header does not exist @@ -207,7 +223,10 @@ func (sr *swarmRouter) createService(ctx context.Context, w http.ResponseWriter, func (sr *swarmRouter) updateService(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var service types.ServiceSpec if err := json.NewDecoder(r.Body).Decode(&service); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } rawVersion := r.URL.Query().Get("version") @@ -309,7 +328,10 @@ func (sr *swarmRouter) getNode(ctx context.Context, w http.ResponseWriter, r *ht func (sr *swarmRouter) updateNode(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var node types.NodeSpec if err := json.NewDecoder(r.Body).Decode(&node); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } rawVersion := r.URL.Query().Get("version") @@ -388,7 +410,10 @@ func (sr *swarmRouter) getSecrets(ctx context.Context, w http.ResponseWriter, r func (sr *swarmRouter) createSecret(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var secret types.SecretSpec if err := json.NewDecoder(r.Body).Decode(&secret); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } version := httputils.VersionFromContext(ctx) if secret.Templating != nil && versions.LessThan(version, "1.37") { @@ -426,6 +451,9 @@ func (sr *swarmRouter) getSecret(ctx context.Context, w http.ResponseWriter, r * func (sr *swarmRouter) updateSecret(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var secret types.SecretSpec if err := json.NewDecoder(r.Body).Decode(&secret); err != nil { + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } return errdefs.InvalidParameter(err) } @@ -459,7 +487,10 @@ func (sr *swarmRouter) getConfigs(ctx context.Context, w http.ResponseWriter, r func (sr *swarmRouter) createConfig(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var config types.ConfigSpec if err := json.NewDecoder(r.Body).Decode(&config); err != nil { - return err + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } + return errdefs.InvalidParameter(err) } version := httputils.VersionFromContext(ctx) @@ -498,6 +529,9 @@ func (sr *swarmRouter) getConfig(ctx context.Context, w http.ResponseWriter, r * func (sr *swarmRouter) updateConfig(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { var config types.ConfigSpec if err := json.NewDecoder(r.Body).Decode(&config); err != nil { + if err == io.EOF { + return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) + } return errdefs.InvalidParameter(err) } diff --git a/api/server/router/volume/volume_routes.go b/api/server/router/volume/volume_routes.go index e892d1a524..4fa4cb53aa 100644 --- a/api/server/router/volume/volume_routes.go +++ b/api/server/router/volume/volume_routes.go @@ -56,7 +56,7 @@ func (v *volumeRouter) postVolumesCreate(ctx context.Context, w http.ResponseWri if err == io.EOF { return errdefs.InvalidParameter(errors.New("got EOF while reading request body")) } - return err + return errdefs.InvalidParameter(err) } volume, err := v.backend.Create(ctx, req.Name, req.Driver, opts.WithCreateOptions(req.DriverOpts), opts.WithCreateLabels(req.Labels)) diff --git a/integration/container/container_test.go b/integration/container/container_test.go new file mode 100644 index 0000000000..a4ae31e948 --- /dev/null +++ b/integration/container/container_test.go @@ -0,0 +1,42 @@ +package container // import "github.com/docker/docker/integration/container" + +import ( + "net/http" + "testing" + + "github.com/docker/docker/internal/test/request" + "gotest.tools/assert" + is "gotest.tools/assert/cmp" +) + +func TestContainerInvalidJSON(t *testing.T) { + defer setupTest(t)() + + endpoints := []string{ + "/containers/foobar/copy", + "/containers/foobar/exec", + "/exec/foobar/start", + } + + for _, ep := range endpoints { + t.Run(ep, 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")) + }) + } +} diff --git a/integration/network/network_test.go b/integration/network/network_test.go index 3829dd728e..5f62550f05 100644 --- a/integration/network/network_test.go +++ b/integration/network/network_test.go @@ -3,6 +3,7 @@ package network // import "github.com/docker/docker/integration/network" import ( "bytes" "context" + "net/http" "os/exec" "strings" "testing" @@ -10,6 +11,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/integration/internal/container" "github.com/docker/docker/internal/test/daemon" + "github.com/docker/docker/internal/test/request" "gotest.tools/assert" is "gotest.tools/assert/cmp" "gotest.tools/skip" @@ -56,3 +58,35 @@ func TestRunContainerWithBridgeNone(t *testing.T) { assert.NilError(t, err) assert.Check(t, is.Equal(stdout.String(), result.Combined()), "The network namspace of container should be the same with host when --net=host and bridge network is disabled") } + +func TestNetworkInvalidJSON(t *testing.T) { + defer setupTest(t)() + + endpoints := []string{ + "/networks/create", + "/networks/bridge/connect", + "/networks/bridge/disconnect", + } + + for _, ep := range endpoints { + t.Run(ep, 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")) + }) + } +} diff --git a/integration/plugin/common/main_test.go b/integration/plugin/common/main_test.go new file mode 100644 index 0000000000..47400129b6 --- /dev/null +++ b/integration/plugin/common/main_test.go @@ -0,0 +1,27 @@ +package common // import "github.com/docker/docker/integration/plugin/common" + +import ( + "fmt" + "os" + "testing" + + "github.com/docker/docker/internal/test/environment" +) + +var testEnv *environment.Execution + +func TestMain(m *testing.M) { + var err error + testEnv, err = environment.New() + if err != nil { + fmt.Println(err) + os.Exit(1) + } + testEnv.Print() + os.Exit(m.Run()) +} + +func setupTest(t *testing.T) func() { + environment.ProtectAll(t, testEnv) + return func() { testEnv.Clean(t) } +} diff --git a/integration/plugin/common/plugin_test.go b/integration/plugin/common/plugin_test.go new file mode 100644 index 0000000000..5b6bfef765 --- /dev/null +++ b/integration/plugin/common/plugin_test.go @@ -0,0 +1,38 @@ +package common // import "github.com/docker/docker/integration/plugin/common" + +import ( + "net/http" + "testing" + + "github.com/docker/docker/internal/test/request" + "gotest.tools/assert" + is "gotest.tools/assert/cmp" +) + +func TestPluginInvalidJSON(t *testing.T) { + defer setupTest(t)() + + endpoints := []string{"/plugins/foobar/set"} + + for _, ep := range endpoints { + t.Run(ep, 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")) + }) + } +} diff --git a/integration/volume/volume_test.go b/integration/volume/volume_test.go index f6ffb5eb50..f29e3669d1 100644 --- a/integration/volume/volume_test.go +++ b/integration/volume/volume_test.go @@ -2,6 +2,7 @@ package volume import ( "context" + "net/http" "path/filepath" "strings" "testing" @@ -95,6 +96,34 @@ func TestVolumesInspect(t *testing.T) { assert.Check(t, createdAt.Truncate(time.Minute).Equal(now.Truncate(time.Minute)), "CreatedAt (%s) not equal to creation time (%s)", createdAt, now) } +func TestVolumesInvalidJSON(t *testing.T) { + defer setupTest(t)() + + endpoints := []string{"/volumes/create"} + + for _, ep := range endpoints { + t.Run(ep, 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")) + }) + } +} + func getPrefixAndSlashFromDaemonPlatform() (prefix, slash string) { if testEnv.OSType == "windows" { return "c:", `\`