api/server/httputils: add ReadJSON() utility and fix handling of invalid JSON by thaJeztah · Pull Request #43463 · moby/moby

thaJeztah · 2022-04-05T11:31:49Z

Implement a httputils.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)
close body after reading (some code did this)

We should consider to add a "max body size" on this function, similar to

moby/api/server/middleware/debug.go

Lines 27 to 40 in 7b9275c

    
           maxBodySize := 4096 // 4KB 
        
           if r.ContentLength > int64(maxBodySize) { 
        
           	return handler(ctx, w, r, vars) 
        
           } 
        
           body := r.Body 
        
           bufReader := bufio.NewReaderSize(body, maxBodySize) 
        
           r.Body = ioutils.NewReadCloserWrapper(bufReader, func() error { return body.Close() }) 
        
           b, err := bufReader.Peek(maxBodySize) 
        
           if err != io.EOF { 
        
           	// either there was an error reading, or the buffer is full (in which case the request is too large) 
        
           	return handler(ctx, w, r, vars) 
        
           }

In addition to the changes above, two commits are included in this PR that:

api/server/httputils: matchesContentType(): return error instead of logging
api/server/httputils: move WriteJSON() together with ReadJSON()

- Description for the changelog

API: Improve validation of invalid JSON requests

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

samuelkarp

This is a good improvement to consistency and readability, LGTM!

tianon · 2022-04-11T18:17:13Z

+		return errdefs.InvalidParameter(errors.Wrap(err, "invalid JSON"))
+	}
+
+	if dec.More() {


Does this method (officially/intentionally) still work correctly after the body is closed?
(The implementation in it appears to do a peek() but I'm not sure if that's on an internal buffer?)

I changed the r.Body.Close() to a defer r.Body.Close(), just in case 👍

tianon · 2022-04-11T18:22:04Z

 	return w.Config, hc, w.NetworkingConfig, nil
 }
+
+func loadJSON(src io.Reader, out interface{}) error {


Maybe we could have a very small comment that explains why this is separate with a link back to the "original" implementation? 😇

Added a comment:

// loadJSON is similar to api/server/httputils.ReadJSON()

tianon · 2022-04-11T18:24:34Z

(Two minor comments - neither are blockers IMO)

…ogging Signed-off-by: Sebastiaan van Stijn <[email protected]>

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 <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Implement similar logic as is used in httputils.ReadJSON(). Before this patch, endpoints using the ContainerDecoder would incorrectly return a 500 (internal server error) status. Signed-off-by: Sebastiaan van Stijn <[email protected]>

tianon

👍

thaJeztah · 2022-04-11T21:23:05Z

Windows 2022 failure is unrelated (known flaky test); #43012

=== RUN   TestDockerSuite/TestSlowStdinClosing/0
    docker_cli_run_test.go:4165: d:\CI\PR-43463\10\binary\docker.exe: Error response from daemon: failed to create shim task: open \\.\pipe\containerd-588a1f287c5980794c7b99e7c997b2949137ae811a72e1cef75dd971934d3e91-init-stdin: The system cannot find the file specified.: not found.
        
    docker_cli_run_test.go:4174: assertion failed: error is not nil: exit status 127
        --- FAIL: TestDockerSuite/TestSlowStdinClosing/0 (0.81s)

thaJeztah added area/api API status/2-code-review impact/changelog labels Apr 5, 2022

thaJeztah added this to the 21.xx milestone Apr 5, 2022

thaJeztah force-pushed the httputils_readjson branch from 1248903 to a7b55f2 Compare April 6, 2022 00:06

thaJeztah commented Apr 6, 2022

View reviewed changes

Comment thread api/server/httputils/httputils.go

thaJeztah force-pushed the httputils_readjson branch from a7b55f2 to 2d95e3b Compare April 8, 2022 21:31

thaJeztah changed the title ~~api/server/httputils: add ReadJSON() utility~~ api/server/httputils: add ReadJSON() utility and fix handling of invalid JSON Apr 8, 2022

thaJeztah force-pushed the httputils_readjson branch 4 times, most recently from ee3c291 to 34c20ce Compare April 10, 2022 21:23

thaJeztah marked this pull request as ready for review April 10, 2022 22:32

samuelkarp approved these changes Apr 11, 2022

View reviewed changes

tianon approved these changes Apr 11, 2022

View reviewed changes

thaJeztah added 4 commits April 11, 2022 21:37

api/server/httputils: matchesContentType(): return error instead of l…

ef490ca

…ogging Signed-off-by: Sebastiaan van Stijn <[email protected]>

api/server/httputils: move WriteJSON() together with ReadJSON()

ff5f70e

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the httputils_readjson branch from 34c20ce to b6d58d7 Compare April 11, 2022 19:45

tianon approved these changes Apr 11, 2022

View reviewed changes

thaJeztah merged commit bca8d9f into moby:master Apr 11, 2022

thaJeztah deleted the httputils_readjson branch April 11, 2022 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api/server/httputils: add ReadJSON() utility and fix handling of invalid JSON#43463

api/server/httputils: add ReadJSON() utility and fix handling of invalid JSON#43463
thaJeztah merged 4 commits into
moby:masterfrom
thaJeztah:httputils_readjson

thaJeztah commented Apr 5, 2022

Uh oh!

Uh oh!

samuelkarp left a comment

Uh oh!

tianon Apr 11, 2022

Uh oh!

thaJeztah Apr 11, 2022

Uh oh!

tianon Apr 11, 2022

Uh oh!

thaJeztah Apr 11, 2022

Uh oh!

tianon commented Apr 11, 2022

Uh oh!

tianon left a comment

Uh oh!

thaJeztah commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	maxBodySize := 4096 // 4KB
	if r.ContentLength > int64(maxBodySize) {
	return handler(ctx, w, r, vars)
	}

	body := r.Body
	bufReader := bufio.NewReaderSize(body, maxBodySize)
	r.Body = ioutils.NewReadCloserWrapper(bufReader, func() error { return body.Close() })

	b, err := bufReader.Peek(maxBodySize)
	if err != io.EOF {
	// either there was an error reading, or the buffer is full (in which case the request is too large)
	return handler(ctx, w, r, vars)
	}

Conversation

thaJeztah commented Apr 5, 2022

Uh oh!

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

tianon Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

tianon Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

tianon commented Apr 11, 2022

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants