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.

Sign up for GitHub

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

[DRAFT] check for nesting limits #3805

Closed
wants to merge 4 commits into from
Closed

[DRAFT] check for nesting limits #3805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92adb61
check for nesting limits
pjfanning Mar 5, 2023
8f88c8e
try new test
pjfanning Mar 5, 2023
f8b925e
Update DeepJsonTreeTraversingTest.java
pjfanning Mar 5, 2023
d68a693
Merge branch '2.15' into nesting
pjfanning Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/main/java/com/fasterxml/jackson/databind/node/NodeCursor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public NodeCursor(int contextType, NodeCursor p)
_type = contextType;
_index = -1;
_parent = p;
_nestingDepth = p == null ? 0 : p._nestingDepth + 1;
}

/*
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/fasterxml/jackson/databind/node/TreeTraversingParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,11 @@ public JsonToken nextToken() throws IOException
switch (_currToken) {
case START_OBJECT:
_nodeCursor = _nodeCursor.startObject();
streamReadConstraints().validateNestingDepth(_nodeCursor.getNestingDepth());
break;
case START_ARRAY:
_nodeCursor = _nodeCursor.startArray();
streamReadConstraints().validateNestingDepth(_nodeCursor.getNestingDepth());
break;
case END_OBJECT:
case END_ARRAY:
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/fasterxml/jackson/databind/util/TokenBuffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1620,8 +1620,10 @@ public JsonToken nextToken() throws IOException
_parsingContext.setCurrentName(name);
} else if (_currToken == JsonToken.START_OBJECT) {
_parsingContext = _parsingContext.createChildObjectContext();
_streamReadConstraints.validateNestingDepth(_parsingContext.getNestingDepth());
} else if (_currToken == JsonToken.START_ARRAY) {
_parsingContext = _parsingContext.createChildArrayContext();
_streamReadConstraints.validateNestingDepth(_parsingContext.getNestingDepth());
} else if (_currToken == JsonToken.END_OBJECT
|| _currToken == JsonToken.END_ARRAY) {
// Closing JSON Object/Array? Close matching context
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/fasterxml/jackson/databind/util/TokenBufferReadContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ protected TokenBufferReadContext(JsonStreamContext base, ContentReference srcRef
} else {
_startLocation = JsonLocation.NA;
}
_nestingDepth = _parent == null ? 0 : _parent.getNestingDepth() + 1;
}

@Deprecated // @since 2.13
Expand All @@ -61,6 +62,7 @@ protected TokenBufferReadContext(JsonStreamContext base, JsonLocation startLoc)
_currentName = base.getCurrentName();
_currentValue = base.getCurrentValue();
_startLocation = startLoc;
_nestingDepth = _parent == null ? 0 : _parent.getNestingDepth() + 1;
}

/**
Expand All @@ -77,6 +79,7 @@ protected TokenBufferReadContext(TokenBufferReadContext parent, int type, int in
super(type, index);
_parent = parent;
_startLocation = parent._startLocation;
_nestingDepth = _parent == null ? 0 : _parent.getNestingDepth() + 1;
}

@Override
Expand Down
68 changes: 68 additions & 0 deletions src/test/java/com/fasterxml/jackson/databind/deser/dos/DeepJsonTreeTraversingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package com.fasterxml.jackson.databind.deser.dos;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.core.StreamReadConstraints;
import com.fasterxml.jackson.core.exc.StreamConstraintsException;
import com.fasterxml.jackson.databind.BaseMapTest;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.json.JsonMapper;

public class DeepJsonTreeTraversingTest extends BaseMapTest
{
private final static int TOO_DEEP_NESTING = 10_000;

private final JsonFactory unconstrainedFactory = JsonFactory.builder()
.streamReadConstraints(StreamReadConstraints.builder().maxNestingDepth(Integer.MAX_VALUE).build())
.build();
private final ObjectMapper unconstrainedMapper = JsonMapper.builder(unconstrainedFactory).build();

public void testTreeWithArray() throws Exception
{
final String doc = _nestedDoc(TOO_DEEP_NESTING, "[ ", "] ");
JsonNode tree = unconstrainedMapper.readTree(doc);
Copy link
Member Author

Choose a reason for hiding this comment

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

@cowtowncoder I was trying to add a test that checks the new logic in TreeTraversingParser (to validate nesting depth constraint). I found that 'readTree' validates already because it uses the jackson-core ReaderbasedJsonParser. I had to change the mapper to loosen the default nesting constraint to get this line to pass. This got me to thinking. Is there much point in having TreeTraversingParser also validate the nesting depth?

A similar question could be asked for the TokenBuffer Parser.

Copy link
Member

Choose a reason for hiding this comment

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

@pjfanning Good question: I don't think they should (have to) validate nesting depth: only things that read actual input should. It seems like pure overhead, plus in some cases it may be unclear which constraints to use.
Same sort of goes for String length checks too I suppose.

One counter-case might be numeric length: if buffering String tokens but requesting number, number-length checks should be applied.

try (JsonParser jp = tree.traverse()) {
JsonToken jt;
while ((jt = jp.nextToken()) != null) {

}
fail("expected StreamConstraintsException");
} catch (StreamConstraintsException e) {
assertEquals("Depth (1001) exceeds the maximum allowed nesting depth (1000)", e.getMessage());
}
}

public void testTreeWithObject() throws Exception
{
final String doc = "{"+_nestedDoc(TOO_DEEP_NESTING, "\"x\":{", "} ") + "}";
JsonNode tree = unconstrainedMapper.readTree(doc);
try (JsonParser jp = tree.traverse()) {
JsonToken jt;
while ((jt = jp.nextToken()) != null) {

}
fail("expected StreamConstraintsException");
} catch (StreamConstraintsException e) {
assertEquals("Depth (1001) exceeds the maximum allowed nesting depth (1000)", e.getMessage());
}
}

private String _nestedDoc(int nesting, String open, String close) {
StringBuilder sb = new StringBuilder(nesting * (open.length() + close.length()));
for (int i = 0; i < nesting; ++i) {
sb.append(open);
if ((i & 31) == 0) {
sb.append("\n");
}
}
for (int i = 0; i < nesting; ++i) {
sb.append(close);
if ((i & 31) == 0) {
sb.append("\n");
}
}
return sb.toString();
}
}