Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configurable limit for the maximum length of Object property names to parse before failing (default max: 50,000 chars) #1047

Closed
cowtowncoder opened this issue Jun 12, 2023 · 5 comments
Labels
2.16 Issue planned (at earliest) for 2.16 processing-limits Issues related to limiting aspects of input/output that can be processed without exception
Milestone

Comments

@cowtowncoder
Copy link
Member

(note: part of #637)

Similar to token-length limits for Numbers (see #815) and Strings (see #863), we need an option to limit maximum Object Property name length to something reasonable, as otherwise name tokenization can become performance issue for very long names.

As with other limits, units would related to underlying input units -- bytes or chars depending on input source.

We need to decide on reasonable defaults to use: my initial suggestion is to limit to 64k but this probably needs to correlated to:

  1. Performance-characteristics (can we find a pivot point where length increase has super-linear negative effect?)
  2. Longest legit (non-malicious) property names used in the wild.

That is: figure out highest limit that works for DoS aspect, balanced with lowest limit that would be unlikely to break existing legit usage.

@cowtowncoder cowtowncoder added 2.16 Issue planned (at earliest) for 2.16 processing-limits Issues related to limiting aspects of input/output that can be processed without exception labels Jun 12, 2023
@ghost
Copy link

ghost commented Jul 6, 2023

I am also in favor of this ticket, as I wrote in this comment: #1014 (comment)

@cowtowncoder
Copy link
Member Author

@AlessandroPerucchi note that there is already ability to change existing limits: this would introduce one more limit.

@yhojann-cl
Copy link

yhojann-cl commented Aug 2, 2023

Related issue: spring-projects/spring-boot#36666 the key names are not controlled and produces an integer overflow when try copy the byte array. Confirmed in Java Spring Framework.

@cowtowncoder
Copy link
Member Author

Quick note: based on simple local testing with longer names, it looks like performance of, say, 1_000_000 (ascii) bytes/chars decoding is not superbly bad -- actually, unit test with cold start seems to process doc with 10 meg name in about 150 msecs. And although other factors (escaping, UTF-8 chars) no doubt change things (quick testing with escapes and 2-byte chars actually increases time to almost 500 msecs), I think we can -- if we want to -- allow larger names than 50k, by default, if we want to.

I'll see if I can find limits other processors impose.

@cowtowncoder cowtowncoder added this to the 2.16.0 milestone Aug 30, 2023
@cowtowncoder
Copy link
Member Author

Implemented via #1078.

@cowtowncoder cowtowncoder changed the title Add configurable limit for the maximum length of Object property names to parse before failing Add configurable limit for the maximum length of Object property names to parse before failing (default max: 50,000 chars) Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.16 Issue planned (at earliest) for 2.16 processing-limits Issues related to limiting aspects of input/output that can be processed without exception
Projects
None yet
Development

No branches or pull requests

2 participants