PSA: Use `json.Decoder` & `Decode` instead of `json.Unmarshal` when using the original request body bytes in business logic

[Kumm-Kai]

🐛 Bug Report

Not an actual bug, more a PSA for others, with similar issues, to stumble upon.

We had a component consisting of a runtime.WithMetadata annotator which added multiple fields to the gRPC context and a UnaryServerInterceptor that reads the previously added metadata fields from the gRPC context.

One of the added fields was the original HTTP request body.

The grpc-gateway usually creates a NewDecoder from the marshaler of the specific request and calls Decode on the request body.
In our setup we've used the default marshaler, so runtime.HTTPBodyMarshaler which is wrapping runtime.JSONPb.

Now to our actual problem:
The UnaryServerInterceptor retrieves the original HTTP request body from the gRPC context and calls json.Unmarshal on the raw bytes.
This is not a problem for normal JSON bodies that are entirely valid.
But for requests where, for whatever reason, additional bytes are present after the initial JSON object, it will result in grpc-gateway being able to properly Decode the body, passing it to the business logic, and json.Unmarshal in the UnaryServerInterceptor later failing to unmarshal.

Such an invalid body might look like this:
{"a": "b"}x or {"a":"b"}{"x":"y"}

This happens because of the difference between runtime.JSONPb, which uses json.Decoder and json.Unmarshal.
The json.Decoder decodes the input []byte JSON object by JSON object, while json.Unmarshal tries to decode the entire input []byte.

The solution:

• If you control the code which uses json.Unmarshal, is quite easy: Just use the json.Decoder & Decode instead.
• If you are unable to change the code and/or instead want to ensure that the original HTTP request body only contains the valid JSON object and nothing else: Configure grpc-gateway to use a custom marshaler (runtime.WithMarshalerOption) that wraps runtime.jsonPb.

An implementation of such a custom marshaler wrapper might look like this:

Custom Marshaler

package jsonpb

import (
	"bytes"
	"encoding/json"
	"errors"
	"io"

	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
)

type JSONPb struct {
	runtime.JSONPb
}

func (j *JSONPb) NewDecoder(r io.Reader) runtime.Decoder {
	d := json.NewDecoder(r)
	return DecoderWrapper{
		original: runtime.DecoderWrapper{
			Decoder:          d,
			UnmarshalOptions: j.UnmarshalOptions,
		},
	}
}

type DecoderWrapper struct {
	original runtime.DecoderWrapper
}

func (d DecoderWrapper) Decode(v any) error {
	if err := d.original.Decode(v); err != nil {
		return err
	}
	_, err := d.original.Token()
	if err != io.EOF {
		return errors.New("request body is not a valid JSON object: body contains additional bytes after the JSON object")
	}
	return nil
}

func (j *JSONPb) Unmarshal(data []byte, v any) error {
	return j.NewDecoder(bytes.NewReader(data)).Decode(v)
}

Configure grpc-gateway like this:

runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.HTTPBodyMarshaler{
	Marshaler: &jsonpb.JSONPb{
		JSONPb: runtime.JSONPb{
			MarshalOptions: protojson.MarshalOptions{
				EmitUnpopulated: true,
			},
			UnmarshalOptions: protojson.UnmarshalOptions{
				DiscardUnknown: true,
			},
		},
	},
}),

I hope this is useful to someone 🙂

(Feel free to close this issue)

[johanbrandhorst]

Thanks for your issue! It seems to me like there might be a security issue here - what if a malicious user smuggled two valid payloads after one another, one that gets grabbed by your middleware and one that gets decoded by the grpc gateway? Are you able to detect such an attack?

[Kumm-Kai]

Luckily that was not the case, but you are 100% correct, this could have been a security issue.
In our case the middleware just read the entire body from the io.Reader, added it to the gRPC context metadata encoded as base64 and wrote the original body bytes back to the http.Request using io.NopCloser(bytes.NewBuffer(byteBody)). So there was no risk of the two parts reading a different JSON object.
And no, we would have had no way to detect such an attack, we basically got lucky because the part that was doing the json.Unmarshal just failed in cases where a bad body was provided.

That's ultimately also why we've decided to wrap the runtime.JSONPb marshaler and make it stricter, as there could be more improper usages of json.Unmarshal in the future.

But yes, you need to be 100% sure that if an HTTP middleware reads the body, it must write the body back into the http.Request in its entirety.