util: Fix UnmarshalJSON parses invalid json in some cases #2346
Conversation
util/json.go
Outdated
| // If the type of err is MalformedJSONInputError, the data is decoded. | ||
| // In this case, this func should not be panicked. | ||
| if _, ok := err.(MalformedJSONInputError); ok { | ||
| return x | ||
| } |
There was a problem hiding this comment.
I'm not sure about this, it seems like it should be considered an error still. I think the assumption here is that the bytes given must be valid json, as-is something like {"k":1}{!{!!!!! would be OK, which IMO should cause the panic.
There was a problem hiding this comment.
I fixed it to be panicked, but other tests are failed because of that 🤔
Why topdown_test.go passes every expected value into MustUnmarshalJSON?
It's weird because some of the value obviously invalid as a JSON(like foo), but still succeeded before the fix.
I want to know what kind of values are expected and how to fix it.
There was a problem hiding this comment.
I want to fix it, but this test method is tightly-coupled with unpredictable input,
so I workaround by using UnmarshalJSON for that and left a comment 😢
If it's not acceptable, I'll try, but I think it will take a long time to fix.
There was a problem hiding this comment.
Interesting.. 🤔 I'm wondering what those tests were expecting to happen before if they're passing in things that would start to fail now.
I'll take a look. We will probably want to sort this out before merging the changes as this affects a lot of places, but I don't think we need to change the implementation here. It seems like we are now doing the right thing when decoding.
There was a problem hiding this comment.
Ok, I did a quick pass over them and I think we're in good shape. This was turning up real errors where the stuff we passed in was just bad JSON. It was only in a handful of tests.
The required changes to fix them are here: 383ccda Please include them with the PR and undo the changes in topdown_test.go. Then we should be good to go.
There was a problem hiding this comment.
Thanks for the fix!
I cherry-picked your commit and erase FIXME comment 😄
Please review it again.
util/json.go
Outdated
|
|
||
| // Since decoder.Decode validates only the first json structure in bytes, | ||
| // check if decoder has more bytes to consume to validate whole input bytes. | ||
| if decoder.More() { |
There was a problem hiding this comment.
Looking a little deeper at the implementation for decoder.More I think what we actually want is to use decoder.Token, so something more like:
if _, err := decoder.Token(); err != io.EOF {
return fmt.Errorf("invalid JSON at offset %d, expected EOF", decoder.InputOffset())
}There was a problem hiding this comment.
Thanks for the review.
I agree with you, and I fixed to use decoder.Token to validate.
katsew
force-pushed
the
validate-whole-json-input
branch
6 times, most recently
from
e1a027a to
0c50ae1
Compare
util/json.go
Outdated
| // check if decoder has more bytes to consume to validate whole input bytes. | ||
| tok, err := decoder.Token() | ||
| if tok != nil { | ||
| return fmt.Errorf("invalid JSON input caused by '%s', expected EOF", tok) |
There was a problem hiding this comment.
Can we change the error message to be more similar to what the standard json.Unmarshal gives?
It has an error message like: error: invalid character '{' after top-level value
topdown/topdown_test.go
Outdated
| @@ -3015,7 +3015,7 @@ func assertTopDownWithPathAndContext(ctx context.Context, t *testing.T, compiler | |||
| t.Fatal(err) | |||
| } | |||
|
|
|||
| expected := util.MustUnmarshalJSON([]byte(e)) | |||
| expected = util.MustUnmarshalJSON([]byte(e)) | |||
There was a problem hiding this comment.
Lets revert this line and keep it using := instead of =, no need to change it.
Fixes: open-policy-agent#2331 Signed-off-by: katsew <y.katsew@gmail.com>
katsew
force-pushed
the
validate-whole-json-input
branch
from
ae77490 to
12870bd
Compare
|
I fixed from the review and squashed the commit, and I hope it's ok to be merged. |
Fixes: #2331
Signed-off-by: katsew y.katsew@gmail.com