Panic when claim contains an array with null values

[FlorianSW]

Hi 👋

we encountered an issue when our app received an invalid JWT, which looked like this:

{"aud": [null]}

(simplified)

When this is processed by golang-jwt, e.g. with this simplified example code:

var t jwt.RegisteredClaims
err := json.NewDecoder(bytes.NewBuffer([]byte(`{"aud": [null]}`))).Decode(&t)
err.Error()

then, the program panics:

runtime error: invalid memory address or nil pointer dereference
encoding/json.(*UnsupportedTypeError).Error(0x14000000280?)
    	/opt/homebrew/opt/go/libexec/src/encoding/json/encode.go:234 +0x20

The error seems to be in

jwt/types.go

Line 124 in 5e00fbc

return &json.UnsupportedTypeError{Type: reflect.TypeOf(vv)}

where an UnsupportedTypeError is constructed where the referenced Type is nil. However, the UnsupportedTypeError, in it's Error() method, adds the unsupported type from the Type field. That panics, as Type is nil.

I understand, that such a token is probably invalid according to it's spec, however, I feel that the code should not panic in these cases, but return the appropriate error (which it at least tried to do :D).

I'm also unsure, if that is something to be reported to golang-jwt (which constructs the UnsupportedTypeError) or to encoding/json, which uses the Type field without checking if it is nil. If I wrongfully reported this here, please give me a hint and I go report it to encoding/json instead ❤️

[oxisto]

That is indeed something that golang-jwt should avoid and probably can avoid by not using reflect.TypeOf on a nil value. I will prepare a fix for that.

[oxisto]

Ok, after having another look I would say that you should probably report this to the golang team. I initially thought that the usage of reflect.TypeOf panics, but only the Error() function inside encoding/json panics. We could of course avoid this by not using this particular type of error in this case.

[mfridman]

Thanks for the report, indeed this (or any other code in this package) should never panic.

I think returning an invalid error defined in this package should be sufficient.

[FlorianSW]

Thanks for the quick fix, awesome :)
One little caveat: I think, any additional information to the app code (using this library) which claim or type was invalid, would be nice. However, this was barely the case with the UnsupportedTypeError as well, as it only said which type the claim was. Maybe something for a future PR :)