Skip to content
This repository has been archived by the owner on Oct 13, 2023. It is now read-only.

[18.09 backport] API: properly handle invalid JSON to return a 400 status #110

Merged

Conversation

thaJeztah
Copy link
Member

Backport of moby#38141 for 18.09

This PR depends on #109 (which on itself depends on #80) for a clean cherry-pick, so this was built on top of that

git checkout -b 18.09_backport_handle_invalid_json  ce-engine/18.09
git cherry-pick -s -S -x c7b488fbc82604ec23b862ec1edc5a0be9c7793d
git push -u origin

cherry-pick was clean; no conflicts

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;

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:

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;

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:

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"}

- Description for the changelog

* Fix API returning a 500 error instead of a 400, if an invalid JSON payload is sent [moby/moby#38141](https://github.com/moby/moby/pull/38141)

- A picture of a cute animal (not mandatory but encouraged)

vdemeester and others added 7 commits November 8, 2018 13:55
- Add windows CI entrypoint script.

Signed-off-by: John Howard <jhoward@microsoft.com>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
(cherry picked from commit d3cc071)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Salahuddin Khan <salah@docker.com>
(cherry picked from commit 4c8b1fd)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Deep Debroy <ddebroy@docker.com>
(cherry picked from commit 7d1c1a4)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Some improvements in this test;

- use the volume-information that's returned by VolumeCreate as "expected"
- don't use an explict name for the volume, as it was only used to reference
  the volume for inspection
- improve the test-output on failure, so that "expected" and "actual" values
  are printed

Without this patch applied;

    === RUN   TestVolumesInspect
    --- FAIL: TestVolumesInspect (0.02s)
     	volume_test.go:108: assertion failed: false (bool) != true (true bool): Time Volume is CreatedAt not equal to current time
    FAIL

With this patch applied;

    === RUN   TestVolumesInspect
    --- FAIL: TestVolumesInspect (0.02s)
        volume_test.go:95: assertion failed: expression is false: createdAt.Truncate(time.Minute).Equal(now.Truncate(time.Minute)): CreatedAt (2018-11-01 16:15:20 +0000 UTC) not equal to creation time (2018-11-01 16:15:20.2421166 +0000 UTC m=+13.733512701)
    FAIL

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 8e8cac8)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 05e1842)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These tests don't seem to have anything Linux-specific,
so enable them on Windows

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit b334198)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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 <github@gone.nl>
(cherry picked from commit c7b488f)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah modified the milestones: 18.09.0, 18.09.1 Nov 8, 2018
@thaJeztah
Copy link
Member Author

This test is really flaky; https://jenkins.dockerproject.org/job/Docker-PRs-s390x/11948/console

tracked through moby#32673, and being investigated in moby#37833

14:22:04 FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection
14:22:04 
14:22:04 [d42131a9a3088] waiting for daemon to start
14:22:04 [d42131a9a3088] daemon started
14:22:04 
14:22:04 [d253e390a4e6e] waiting for daemon to start
14:22:04 [d253e390a4e6e] daemon started
14:22:04 
14:22:04 [d524e324b6fc7] waiting for daemon to start
14:22:04 [d524e324b6fc7] daemon started
14:22:04 
14:22:04 [d42131a9a3088] exiting daemon
14:22:04 assertion failed: error is not nil: Error response from daemon: rpc error: code = DeadlineExceeded desc = context deadline exceeded
14:22:04 [d253e390a4e6e] exiting daemon
14:22:04 [d524e324b6fc7] exiting daemon

@thaJeztah
Copy link
Member Author

ping @kolyshkin @cpuguy83 PTAL

Copy link

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewhsu andrewhsu merged commit 8f18fea into docker-archive:18.09 Nov 27, 2018
@thaJeztah thaJeztah deleted the 18.09_backport_handle_invalid_json branch November 27, 2018 18:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants