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 StreamReadConstraints.maxNestingDepth() to constraint max nesting depth (default: 1000) #943

Merged
merged 1 commit into from Mar 6, 2023

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Mar 5, 2023

Basically #926 but depth is tracked on the JsonStreamContext instead. I have built a jackson-databind enhancement based on this.

@pjfanning pjfanning self-assigned this Mar 5, 2023
@pjfanning pjfanning marked this pull request as draft March 5, 2023 11:04
@pjfanning pjfanning changed the title apply constraint to nesting depth (approach 2) [DRAFT] apply constraint to nesting depth (approach 2) Mar 5, 2023
@cowtowncoder
Copy link
Member

Yes; this is along lines of what I was originally thinking. I think there are some benefits in avoiding possibility of accidentally forgotten "closing" of nesting levels, to avoid bugs,

public void validateNestingDepth(int depth) throws StreamConstraintsException
{
if (depth > _maxNestingDepth) {
throw new StreamConstraintsException(String.format("Depth (%d) exceeds the maximum allowed nesting depth (%d)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR specifically, but one thing we may want to do afterwards is to add something to note that StreamReadConstraints settings control this -- just add some more verbiage in exception message.
But I think that makes sense as a separate PR once we have nesting depth checks covered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StreamConstraintsException has Javadoc that describes what the exception is about. I'm not sure there is much to be gained by making the message more verbose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My perspective is that when someone sees a stack trace in log files, it is nice to have slightly more information on context to give Ops people bit more clue on what might be happening.

But I guess minimum level certainly is to have information on exception class itself, pointing to constraints settings. I'll see if I can add something to relevant JAva class(es) in jackson-core.

@cowtowncoder
Copy link
Member

@pjfanning Let me know if and when you are content with the base -- I think this is the last "must-have" feature before 2.15 release candidates.

@pjfanning pjfanning marked this pull request as ready for review March 5, 2023 20:30
@pjfanning pjfanning changed the title [DRAFT] apply constraint to nesting depth (approach 2) apply constraint to nesting depth Mar 5, 2023
@pjfanning
Copy link
Member Author

@pjfanning Let me know if and when you are content with the base -- I think this is the last "must-have" feature before 2.15 release candidates.

@cowtowncoder I'm happy to get this merged more or less as it is. There is uptake needed in numerous other Jackson modules so it would be good to make a start on that.

@cowtowncoder cowtowncoder merged commit 54f78a9 into FasterXML:2.15 Mar 6, 2023
cowtowncoder added a commit that referenced this pull request Mar 6, 2023
@cowtowncoder
Copy link
Member

@pjfanning Merged into 2.15, up to master -- will make some smaller changes (increase test coverage, add that one constructor).

Note: master will require quite a bit more work other formats as array/object context creation has moved from ParserBase (used by non-JSON backends) into JsonParserBase -- this to support custom context subtypes better.

@pjfanning pjfanning deleted the nesting2 branch March 6, 2023 09:39
@cowtowncoder cowtowncoder changed the title apply constraint to nesting depth Add StreamReadConstraints.maxNestingDepth() to constraint max nesting depth (default: 1000) Apr 26, 2023
cowtowncoder added a commit that referenced this pull request Jun 12, 2023
@cowtowncoder cowtowncoder added 2.15 Issue planned (at earliest) for 2.15 processing-limits Issues related to limiting aspects of input/output that can be processed without exception labels Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.15 Issue planned (at earliest) for 2.15 processing-limits Issues related to limiting aspects of input/output that can be processed without exception
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants