validate the length of names

[pjfanning]

• so far just a POC that only works with the UTF8StreamJsonParser and possibly NonBlockingUtf8JsonParsers
• start with 50,000 char limit on names (as a default, that can be adjusted by users)
• if this approach is ok, the other JsonParser implementations can be given the equivalent checks
  • I have added some checks to the ReaderBasedJsonParser but we need more checks (while the name is being streamed)
• see Add configurable limit for the maximum length of Object property names to parse before failing (default max: 50,000 chars) #1047

[pjfanning]

@cowtowncoder are these changes on the right track?

[cowtowncoder]

Ideally we should only (have to) check length when adding new name in CharsToNameCanonicalizer (and byte-based counterpart); not for every time we decode a name.

But to do that would need to pass constraints... that might be doable in makeChild() method. Maybe adding new variant that takes JsonFactory, instead of flags.

[cowtowncoder]

@pjfanning yes, this is pretty much along the lines I was thinking. The main concern would be trying to avoid checks for char-based names on every decoding -- seems like it'd make sense to move check into CharsToNameCanonicalizer but that requires some plumbing to pass StreamReadConstraints.

For byte/quad-based decoding I think checks on array expansion make sense; just need to pass byte-size, not quad size.

[pjfanning]

@pjfanning yes, this is pretty much along the lines I was thinking. The main concern would be trying to avoid checks for char-based names on every decoding -- seems like it'd make sense to move check into CharsToNameCanonicalizer but that requires some plumbing to pass StreamReadConstraints.

For byte/quad-based decoding I think checks on array expansion make sense; just need to pass byte-size, not quad size.

I created #1086

[cowtowncoder]

Should have asked this before but... any chance to get StreamReadConstraints additions in a separate PR first, to get merged to master -- and then the rest separately?
I think the first set of changes follows a pattern and is fine as-is so I can merge this, leaving the rest easier to review (and merge too)

EDIT: I'll merge StreamReadConstraints manually, easy enough to pick.

[cowtowncoder]

@pjfanning Actually, I think this is good -- I can make minor changes after merge as I see fit. So if you think this is ready, please change to regular PR from draft?

[pjfanning]

@cowtowncoder should I move some of the UTF8StreamJsonParser parser checks to addName? The code there is a bit different because the ByteQuadsCanonicalizer doesn't validate the name length.

[cowtowncoder]

Yes, this could be instead moved to inside addName() which is only called if findName() does not find already canonicalized name.

[cowtowncoder]

LGTM, will merge tomorrow (merging to master/3.0 will take some time)

[cowtowncoder]

@cowtowncoder should I move some of the UTF8StreamJsonParser parser checks to addName? The code there is a bit different because the ByteQuadsCanonicalizer doesn't validate the name length.

Yes, I think moving those couple of cases to only validate in addName() and not earlier would make sense.
Theoretically could refactor ByteQuadsCanonicalizer to take in validation similar to char-based one, but that's more work and not necessarily better.

[pjfanning]

@cowtowncoder should I move some of the UTF8StreamJsonParser parser checks to addName? The code there is a bit different because the ByteQuadsCanonicalizer doesn't validate the name length.

Yes, I think moving those couple of cases to only validate in addName() and not earlier would make sense. Theoretically could refactor ByteQuadsCanonicalizer to take in validation similar to char-based one, but that's more work and not necessarily better.

I moved the check to addName().

[cowtowncoder]

Thank you again @pjfanning ! This time merging to master went VERY smoothly, somehow. Good job!
I made some minor tweaks to naming but essentially implementation looks straight-forward.
Over time should add similar verification to Smile, CBOR, YAML, XML, Properties. But for now great to have it for JSON.