Cannot parse non-numerical ID

[hsanjuan]

Per the json-rpc spec, the "Id" in the request can be a number, a string or NULL, but this library throws an error when using a string:

{
  "jsonrpc": "2.0",
  "id": 0,
  "error": {
    "code": -32700,
    "message": "unmarshaling request: json: cannot unmarshal string into Go struct field request.id of type int64"
  }
}

Additionally, the response breaks the spec too because If there was an error in detecting the id in the Request object (e.g. Parse error/Invalid Request), it MUST be Null..

[olizilla]

+1. This is still an issue and means you cannot use clients generated with https://github.com/open-rpc/generator from the OpenRPC definitions in Lotus, as they pass id as a string.

id: An identifier established by the Client that MUST contain a String, Number, or NULL value if included.
– https://www.jsonrpc.org/specification#request_object

$ curl -H "Content-Type: application/json" \
  --data '{ "jsonrpc": "2.0", "method": "Filecoin.ChainHead", "params": [], "id": "1" }' \
  'http://127.0.0.1:1234/rpc/v0'
{"jsonrpc":"2.0","id":0,"error":{"code":-32700,"message":"unmarshaling request: json: cannot unmarshal string into Go struct field request.id of type int64"}}

[rvagg]

@mvdan as our resident encoding/json expert, can you educate us a little on how this might be solved in as elegant way as possible?

The decode happens into:

go-jsonrpc/handler.go

Lines 38 to 44 in 45ea43a

	type request struct {
	Jsonrpc string `json:"jsonrpc"`
	ID *int64 `json:"id,omitempty"`
	Method string `json:"method"`
	Params []param `json:"params"`
	Meta map[string]string `json:"meta,omitempty"`
	}

Using a simple json.NewDecoder(bufferedRequest).Decode(&req)

Then the response is built as:

go-jsonrpc/handler.go

Lines 64 to 69 in 45ea43a

	type response struct {
	Jsonrpc string `json:"jsonrpc"`
	Result interface{} `json:"result,omitempty"`
	ID int64 `json:"id"`
	Error *respError `json:"error,omitempty"`
	}

Using a json.NewEncoder(w).Encode(resp).

We're already handling the NULL case:

go-jsonrpc/handler.go

Lines 265 to 267 in 45ea43a

	if req.ID == nil {
	return // notification
	}

But if we want to also handle the String case and do it both in and out (i.e. if you give it an int, return an int, but if you give a string, return a string), what are our options here? Do we have to first inspect it as an map[string]interface{} before deciding how to proceed?

I don't know the answer to this but I'm wondering if we can strictly require that any String ID be a string representation of an int64. That would probably satisfy the OpenRPC model, as long as we returned it as a string. I'm a bit weary about accepting arbitrary strings as it seems like an opportunity for novelty that we may not want to introduce. So maybe it's enough to have an int64 but also a flag that says "it came as a string".

[mvdan]

I don't know the answer to this but I'm wondering if we can strictly require that any String ID be a string representation of an int64.

My reading of the spec is different than yours, at least. It says the ID can be a string, but it doesn't say that the string must contain a valid integer representation. So it could be a UUID or some random base64, for example, and I don't think that would break the spec.

Assuming you don't want to enforce ID strings to be integers, I think the best option is to implement a custom type that knows how to unmarshal itself. I initially thought about suggesting ID interface{} and using a type switch to check the received type after decoding, but that wouldn't quite work as integers are decoded as float64, so you could potentially have fun int vs float precision issues there.

Here's an example of the decode type: https://play.golang.org/p/SVQFikqZrpg. You can run it and see how it behaves with a few different inputs.

You're fully in control with that UnmarshalJSON method, so you could, for example, tweak it to only accept strings which represent valid int64 by doing a second Unmarshal in the string case. You could then, for example, swap the internal interface{} representation with *int64.

[rvagg]

neat, thanks @mvdan, that sounds right for this problem.

Re

It says the ID can be a string, but it doesn't say that the string must contain a valid integer representation

I was just expressing some discomfort with the anything nature of a string in this position, allowing arbitrary length strings to pass through state like this is how attack vectors emerge so I was pondering if there was a way to limit that. As far as we know from playing with OpenRPC so far it's defaulting to using integer strings (at least the JS/TypeScript codegen client does), but I don't know how universal that is and maybe there's clients out there that we want to support that use some other nonce type here.

[mvdan]

The code I shared is entirely custom, so you could always restrict string length. Though, personally, I'd do that before any JSON decoding happens, e.g. via a LimitReader.

[olizilla]

Assuming you don't want to enforce ID strings to be integers

I would move conservatively and assume that the spec was simply being kind to JavaScript by allowing integers to be sent as numbers or strings. If it simplifies anything to require the strings to be representations of integers, then I would go that route.

[olizilla]

ok, i take it back. "Whatever the client sends should be exactly what is sent back" is the spirit of the thing, not "be nice to JS".

v1 of the spec captures the intent:

id - The request id. This can be of any type. It is used to match the response with the request that it is replying to.
– https://www.jsonrpc.org/specification_v1#a1.1Requestmethodinvocation

and there are these guys:

If I send id:'2.0a', I must get back id:'2.0a' - is there anything in the spec that indicates its supposed to specifically be a numeric id?

Personally I use randomized non-sequential strings...

– https://groups.google.com/g/json-rpc/c/_6VLMN_SHco/m/JG9mQCKp-mAJ

[mvdan]

Yeah, in my mind being conservative is "allow what the spec says is allowed" :)

[rvagg]

ah, there's a LimitReader in here already, with a max of 100 Mb (seems quite large for a message, I wonder what that's accounting for? but at least it's bounded)

[rvagg]

I thought I'd try my rusty Go hands at this one for something different, would appreciate reviews @ #48