From b549cb356ad07fbbe93dba816c6f181880ce0379 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Thu, 12 Nov 2020 13:21:18 -0300 Subject: [PATCH 1/5] limit JSON request size via HTTP body --- handler.go | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/handler.go b/handler.go index 7243a0d..c786909 100644 --- a/handler.go +++ b/handler.go @@ -111,7 +111,32 @@ func (s *RPCServer) handleReader(ctx context.Context, r io.Reader, w io.Writer, } var req request - if err := json.NewDecoder(r).Decode(&req); err != nil { + // Limit request size. Ideally this limit should be specific for each field + // in the JSON request but as simple defensive measure we just limit the + // entire HTTP body. + MAX_REQUEST_SIZE := 10 << 10 // FIXME: Agree on a value and extract. + buf := new(bytes.Buffer) + // We use LimitReader to avoid reading over the maximum, since it won't + // return an EOF we can't clearly distinguish between a valid request that + // is exactly the MAX_REQUEST_SIZE and one that exceeds it. To get around it + // we try to read one extra byte to discriminate if we need to error out or + // not. + // FIXME: Maybe there's a cleaner way to do this. + reqSize, err := buf.ReadFrom(io.LimitReader(r, int64(MAX_REQUEST_SIZE + 1))) + if err != nil { + // ReadFrom will ignore an EOF from LimitReader so this is an unexpected + // error. + rpcError(wf, &req, rpcParseError, xerrors.Errorf("reading request: %w", err)) + return + } + if reqSize > int64(MAX_REQUEST_SIZE) { + // FIXME: Do we want to define a new error or can we consider reading errors + // as part of the parsing stage (`rpcParseError`) and discriminate by error string? + rpcError(wf, &req, rpcParseError, xerrors.Errorf("request bigger than maximum %d allowed", MAX_REQUEST_SIZE)) + return + } + + if err := json.NewDecoder(buf).Decode(&req); err != nil { rpcError(wf, &req, rpcParseError, xerrors.Errorf("unmarshaling request: %w", err)) return } From 531d93838bf6dbf78c7d732b453e4791db718858 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Mon, 16 Nov 2020 13:04:53 -0300 Subject: [PATCH 2/5] make max request size a server configuration --- handler.go | 16 +++++++++------- options_server.go | 8 ++++++++ server.go | 3 +++ 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/handler.go b/handler.go index c786909..5ea5089 100644 --- a/handler.go +++ b/handler.go @@ -43,6 +43,12 @@ type request struct { Meta map[string]string `json:"meta,omitempty"` } +// Limit request size. Ideally this limit should be specific for each field +// in the JSON request but as simple defensive measure we just limit the +// entire HTTP body. +// Configured by WithMaxRequestSize. +const DEFAULT_MAX_REQUEST_SIZE = 100 << 20 // 100 MiB + type respError struct { Code int `json:"code"` Message string `json:"message"` @@ -111,10 +117,6 @@ func (s *RPCServer) handleReader(ctx context.Context, r io.Reader, w io.Writer, } var req request - // Limit request size. Ideally this limit should be specific for each field - // in the JSON request but as simple defensive measure we just limit the - // entire HTTP body. - MAX_REQUEST_SIZE := 10 << 10 // FIXME: Agree on a value and extract. buf := new(bytes.Buffer) // We use LimitReader to avoid reading over the maximum, since it won't // return an EOF we can't clearly distinguish between a valid request that @@ -122,17 +124,17 @@ func (s *RPCServer) handleReader(ctx context.Context, r io.Reader, w io.Writer, // we try to read one extra byte to discriminate if we need to error out or // not. // FIXME: Maybe there's a cleaner way to do this. - reqSize, err := buf.ReadFrom(io.LimitReader(r, int64(MAX_REQUEST_SIZE + 1))) + reqSize, err := buf.ReadFrom(io.LimitReader(r, s.maxRequestSize + 1)) if err != nil { // ReadFrom will ignore an EOF from LimitReader so this is an unexpected // error. rpcError(wf, &req, rpcParseError, xerrors.Errorf("reading request: %w", err)) return } - if reqSize > int64(MAX_REQUEST_SIZE) { + if reqSize > s.maxRequestSize { // FIXME: Do we want to define a new error or can we consider reading errors // as part of the parsing stage (`rpcParseError`) and discriminate by error string? - rpcError(wf, &req, rpcParseError, xerrors.Errorf("request bigger than maximum %d allowed", MAX_REQUEST_SIZE)) + rpcError(wf, &req, rpcParseError, xerrors.Errorf("request bigger than maximum %d allowed", s.maxRequestSize)) return } diff --git a/options_server.go b/options_server.go index 6d5eb64..51d55ac 100644 --- a/options_server.go +++ b/options_server.go @@ -9,6 +9,7 @@ type ParamDecoder func(ctx context.Context, json []byte) (reflect.Value, error) type ServerConfig struct { paramDecoders map[reflect.Type]ParamDecoder + maxRequestSize int64 } type ServerOption func(c *ServerConfig) @@ -16,6 +17,7 @@ type ServerOption func(c *ServerConfig) func defaultServerConfig() ServerConfig { return ServerConfig{ paramDecoders: map[reflect.Type]ParamDecoder{}, + maxRequestSize: DEFAULT_MAX_REQUEST_SIZE, } } @@ -24,3 +26,9 @@ func WithParamDecoder(t interface{}, decoder ParamDecoder) ServerOption { c.paramDecoders[reflect.TypeOf(t).Elem()] = decoder } } + +func WithMaxRequestSize(max int64) ServerOption { + return func(c *ServerConfig) { + c.maxRequestSize = max + } +} diff --git a/server.go b/server.go index d35d0ea..bb67de0 100644 --- a/server.go +++ b/server.go @@ -22,6 +22,8 @@ type RPCServer struct { methods map[string]rpcHandler paramDecoders map[reflect.Type]ParamDecoder + + maxRequestSize int64 } // NewServer creates new RPCServer instance @@ -34,6 +36,7 @@ func NewServer(opts ...ServerOption) *RPCServer { return &RPCServer{ methods: map[string]rpcHandler{}, paramDecoders: config.paramDecoders, + maxRequestSize: config.maxRequestSize, } } From 408ed44bc30d1931a045d6e857f64fee77972fc9 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Mon, 16 Nov 2020 13:08:18 -0300 Subject: [PATCH 3/5] settle on json spec error --- handler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/handler.go b/handler.go index 5ea5089..6788fba 100644 --- a/handler.go +++ b/handler.go @@ -129,11 +129,11 @@ func (s *RPCServer) handleReader(ctx context.Context, r io.Reader, w io.Writer, // ReadFrom will ignore an EOF from LimitReader so this is an unexpected // error. rpcError(wf, &req, rpcParseError, xerrors.Errorf("reading request: %w", err)) + // rpcParseError is the closest to what we want from the standard errors + // defined in [jsonrpc spec](https://www.jsonrpc.org/specification#error_object) return } if reqSize > s.maxRequestSize { - // FIXME: Do we want to define a new error or can we consider reading errors - // as part of the parsing stage (`rpcParseError`) and discriminate by error string? rpcError(wf, &req, rpcParseError, xerrors.Errorf("request bigger than maximum %d allowed", s.maxRequestSize)) return } From abd9f44465794747ca2bdd1138cf345b848ed458 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Mon, 16 Nov 2020 13:41:15 -0300 Subject: [PATCH 4/5] comments --- handler.go | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/handler.go b/handler.go index 6788fba..722feeb 100644 --- a/handler.go +++ b/handler.go @@ -44,7 +44,7 @@ type request struct { } // Limit request size. Ideally this limit should be specific for each field -// in the JSON request but as simple defensive measure we just limit the +// in the JSON request but as a simple defensive measure we just limit the // entire HTTP body. // Configured by WithMaxRequestSize. const DEFAULT_MAX_REQUEST_SIZE = 100 << 20 // 100 MiB @@ -117,28 +117,33 @@ func (s *RPCServer) handleReader(ctx context.Context, r io.Reader, w io.Writer, } var req request - buf := new(bytes.Buffer) - // We use LimitReader to avoid reading over the maximum, since it won't - // return an EOF we can't clearly distinguish between a valid request that - // is exactly the MAX_REQUEST_SIZE and one that exceeds it. To get around it - // we try to read one extra byte to discriminate if we need to error out or - // not. + // We read the entire request upfront in a buffer to be able to tell if the + // client sent more than maxRequestSize and report it back as an explicit error, + // instead of just silently truncating it and reporting a more vague parsing + // error. + bufferedRequest := new(bytes.Buffer) + // We use LimitReader to enforce maxRequestSize. Since it won't return an + // EOF we can't actually know if the client sent more than the maximum or + // not, so we read one byte more over the limit to explicitly query that. // FIXME: Maybe there's a cleaner way to do this. - reqSize, err := buf.ReadFrom(io.LimitReader(r, s.maxRequestSize + 1)) + reqSize, err := bufferedRequest.ReadFrom(io.LimitReader(r, s.maxRequestSize + 1)) if err != nil { - // ReadFrom will ignore an EOF from LimitReader so this is an unexpected - // error. + // ReadFrom will discard EOF so any error here is unexpected and should + // be reported. rpcError(wf, &req, rpcParseError, xerrors.Errorf("reading request: %w", err)) - // rpcParseError is the closest to what we want from the standard errors - // defined in [jsonrpc spec](https://www.jsonrpc.org/specification#error_object) return } if reqSize > s.maxRequestSize { - rpcError(wf, &req, rpcParseError, xerrors.Errorf("request bigger than maximum %d allowed", s.maxRequestSize)) + rpcError(wf, &req, rpcParseError, + // rpcParseError is the closest we have from the standard errors defined + // in [jsonrpc spec](https://www.jsonrpc.org/specification#error_object) + // to report the maximum limit. + xerrors.Errorf("request bigger than maximum %d allowed", + s.maxRequestSize)) return } - if err := json.NewDecoder(buf).Decode(&req); err != nil { + if err := json.NewDecoder(bufferedRequest).Decode(&req); err != nil { rpcError(wf, &req, rpcParseError, xerrors.Errorf("unmarshaling request: %w", err)) return } From 27f51378eab707e2eb4822b286177f05cb4418b6 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Mon, 16 Nov 2020 13:47:09 -0300 Subject: [PATCH 5/5] go fmt --- handler.go | 2 +- options_server.go | 4 ++-- server.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/handler.go b/handler.go index 722feeb..ef5f53a 100644 --- a/handler.go +++ b/handler.go @@ -126,7 +126,7 @@ func (s *RPCServer) handleReader(ctx context.Context, r io.Reader, w io.Writer, // EOF we can't actually know if the client sent more than the maximum or // not, so we read one byte more over the limit to explicitly query that. // FIXME: Maybe there's a cleaner way to do this. - reqSize, err := bufferedRequest.ReadFrom(io.LimitReader(r, s.maxRequestSize + 1)) + reqSize, err := bufferedRequest.ReadFrom(io.LimitReader(r, s.maxRequestSize+1)) if err != nil { // ReadFrom will discard EOF so any error here is unexpected and should // be reported. diff --git a/options_server.go b/options_server.go index 51d55ac..2da0f1c 100644 --- a/options_server.go +++ b/options_server.go @@ -8,7 +8,7 @@ import ( type ParamDecoder func(ctx context.Context, json []byte) (reflect.Value, error) type ServerConfig struct { - paramDecoders map[reflect.Type]ParamDecoder + paramDecoders map[reflect.Type]ParamDecoder maxRequestSize int64 } @@ -16,7 +16,7 @@ type ServerOption func(c *ServerConfig) func defaultServerConfig() ServerConfig { return ServerConfig{ - paramDecoders: map[reflect.Type]ParamDecoder{}, + paramDecoders: map[reflect.Type]ParamDecoder{}, maxRequestSize: DEFAULT_MAX_REQUEST_SIZE, } } diff --git a/server.go b/server.go index bb67de0..6d0b634 100644 --- a/server.go +++ b/server.go @@ -34,8 +34,8 @@ func NewServer(opts ...ServerOption) *RPCServer { } return &RPCServer{ - methods: map[string]rpcHandler{}, - paramDecoders: config.paramDecoders, + methods: map[string]rpcHandler{}, + paramDecoders: config.paramDecoders, maxRequestSize: config.maxRequestSize, } }