From 893c5dde4dc9594566556d3ef9d78fe3a4f859b7 Mon Sep 17 00:00:00 2001 From: buongarzoni Date: Mon, 5 Oct 2026 02:46:17 -0300 Subject: [PATCH] feat(java): add cycle detection and depth limit to JSON serializer --- .../sender/json/JsonSerializerImpl.java | 102 ++++++++++-- .../sender/json/JsonSerializerImplTest.java | 150 +++++++++++++++++- 2 files changed, 236 insertions(+), 16 deletions(-) diff --git a/rollbar-java/src/main/java/com/rollbar/notifier/sender/json/JsonSerializerImpl.java b/rollbar-java/src/main/java/com/rollbar/notifier/sender/json/JsonSerializerImpl.java index aeede921..8ffe61df 100755 --- a/rollbar-java/src/main/java/com/rollbar/notifier/sender/json/JsonSerializerImpl.java +++ b/rollbar-java/src/main/java/com/rollbar/notifier/sender/json/JsonSerializerImpl.java @@ -6,13 +6,24 @@ import com.rollbar.api.payload.Payload; import com.rollbar.notifier.sender.result.Result; import java.util.Collection; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; /** * Implementation of the {@link JsonSerializer json serializer}. + * + *

+ * Payloads carry arbitrary user data, such as custom maps and captured local variables, so the + * serializer guards against object graphs that would otherwise make it recurse without bound. A + * map, collection, array or {@link JsonSerializable} that refers back to one of its enclosing + * values is replaced by {@value #CIRCULAR_REFERENCE_PLACEHOLDER}, and one nested deeper than + * {@code maxDepth} levels is replaced by {@value #MAX_DEPTH_PLACEHOLDER}. + *

*/ public class JsonSerializerImpl implements JsonSerializer { @@ -38,8 +49,19 @@ public class JsonSerializerImpl implements JsonSerializer { REPLACEMENT_CHARS['\f'] = "\\f"; } + /** + * The default maximum nesting depth, see {@link #JsonSerializerImpl(boolean, int)}. + */ + public static final int DEFAULT_MAX_DEPTH = 100; + + static final String CIRCULAR_REFERENCE_PLACEHOLDER = ""; + + static final String MAX_DEPTH_PLACEHOLDER = ""; + private final boolean prettyPrint; + private final int maxDepth; + /** * Construct a JsonSerializerImpl that does not pretty print the Payload. */ @@ -52,7 +74,22 @@ public JsonSerializerImpl() { * @param prettyPrint whether or not to pretty print the payload. */ public JsonSerializerImpl(boolean prettyPrint) { + this(prettyPrint, DEFAULT_MAX_DEPTH); + } + + /** + * Construct a JsonSerializerImpl. + * @param prettyPrint whether or not to pretty print the payload. + * @param maxDepth the maximum number of nested maps, collections and arrays to serialize, + * counting the outermost object. Anything nested deeper is replaced by a + * placeholder string. Must be at least 1. + */ + public JsonSerializerImpl(boolean prettyPrint, int maxDepth) { + if (maxDepth < 1) { + throw new IllegalArgumentException("The max depth must be at least 1, got " + maxDepth); + } this.prettyPrint = prettyPrint; + this.maxDepth = maxDepth; } @Override @@ -71,7 +108,10 @@ public String toJson(Payload payload) { */ public String toJson(Map map) { StringBuilder builder = new StringBuilder(); - serializeObject(map, builder, 0); + // Identity based: the values being tracked are the ones that may contain themselves, and the + // equals() and hashCode() of such a map or collection recurse forever as well. + Set visiting = Collections.newSetFromMap(new IdentityHashMap()); + serializeValue(builder, map, 0, visiting); return builder.toString(); } @@ -109,7 +149,8 @@ public Result resultFrom(String response) { } - private void serializeObject(Map content, StringBuilder builder, int level) { + private void serializeObject(Map content, StringBuilder builder, int level, + Set visiting) { builder.append('{'); String comma = ""; @@ -128,7 +169,7 @@ private void serializeObject(Map content, StringBuilder builder, builder.append(" "); } - serializeValue(builder, entry.getValue(), level + 1); + serializeValue(builder, entry.getValue(), level + 1, visiting); } if (prettyPrint) { builder.append("\n"); @@ -137,7 +178,8 @@ private void serializeObject(Map content, StringBuilder builder, builder.append('}'); } - private void serializeValue(StringBuilder builder, Object value, int level) { + private void serializeValue(StringBuilder builder, Object value, int level, + Set visiting) { if (value == null) { serializeNull(builder); } else if (value instanceof Boolean) { @@ -146,15 +188,9 @@ private void serializeValue(StringBuilder builder, Object value, int level) { serializeNumber(builder, (Number) value); } else if (value instanceof String) { serializeString(builder, (String) value); - } else if (value instanceof JsonSerializable) { - serializeValue(builder, ((JsonSerializable) value).asJson(), level); - } else if (value instanceof Map) { - Map obj = asMap((Map) value); - serializeObject(obj, builder, level); - } else if (value instanceof Collection) { - serializeArray(builder, ((Collection) value).toArray(), level); - } else if (value instanceof Object[]) { - serializeArray(builder, (Object[]) value, level); + } else if (value instanceof JsonSerializable || value instanceof Map + || value instanceof Collection || value instanceof Object[]) { + serializeNested(builder, value, level, visiting); } else if (value instanceof Throwable) { serializeThrowable(builder, (Throwable) value); } else { @@ -162,6 +198,41 @@ private void serializeValue(StringBuilder builder, Object value, int level) { } } + /** + * Serializes a value that can contain other values, unless doing so would recurse without + * bound. {@code visiting} holds the values enclosing this one, so finding the value there means + * it contains itself. Values are removed again on the way out, so the same value reached twice + * through different paths, without being its own ancestor, is serialized both times. + */ + private void serializeNested(StringBuilder builder, Object value, int level, + Set visiting) { + if (visiting.contains(value)) { + serializeString(builder, CIRCULAR_REFERENCE_PLACEHOLDER); + return; + } + if (level >= maxDepth) { + serializeString(builder, MAX_DEPTH_PLACEHOLDER); + return; + } + + visiting.add(value); + try { + if (value instanceof JsonSerializable) { + // Not a level of its own: it stands for the value it converts to. + serializeValue(builder, ((JsonSerializable) value).asJson(), level, visiting); + } else if (value instanceof Map) { + Map obj = asMap((Map) value); + serializeObject(obj, builder, level, visiting); + } else if (value instanceof Collection) { + serializeArray(builder, ((Collection) value).toArray(), level, visiting); + } else { + serializeArray(builder, (Object[]) value, level, visiting); + } + } finally { + visiting.remove(value); + } + } + private static void serializeThrowable(StringBuilder builder, Throwable value) { serializeToString(builder, value); } @@ -220,7 +291,8 @@ private static void serializeNull(StringBuilder builder) { builder.append("null"); } - private void serializeArray(StringBuilder builder, Object[] array, int level) { + private void serializeArray(StringBuilder builder, Object[] array, int level, + Set visiting) { builder.append('['); String comma = ""; for (Object obj : array) { @@ -231,7 +303,7 @@ private void serializeArray(StringBuilder builder, Object[] array, int level) { builder.append("\n"); indent(builder, level); } - serializeValue(builder, obj, level + 1); + serializeValue(builder, obj, level + 1, visiting); } builder.append(']'); } diff --git a/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonSerializerImplTest.java b/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonSerializerImplTest.java index 7a139001..3fba12e9 100644 --- a/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonSerializerImplTest.java +++ b/rollbar-java/src/test/java/com/rollbar/notifier/sender/json/JsonSerializerImplTest.java @@ -3,18 +3,25 @@ import static com.rollbar.notifier.sender.json.JsonTestHelper.assertValidJson; import static com.rollbar.notifier.sender.json.JsonTestHelper.fromString; import static com.rollbar.notifier.sender.json.JsonTestHelper.getValue; +import static com.rollbar.notifier.sender.json.JsonSerializerImpl.CIRCULAR_REFERENCE_PLACEHOLDER; +import static com.rollbar.notifier.sender.json.JsonSerializerImpl.MAX_DEPTH_PLACEHOLDER; import static java.lang.String.format; +import static java.util.Arrays.asList; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.core.Is.is; +import com.rollbar.api.json.JsonSerializable; import com.rollbar.api.payload.Payload; import com.rollbar.api.payload.data.Data; import com.rollbar.notifier.sender.result.Result; import org.junit.Test; +import java.util.ArrayList; import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; public class JsonSerializerImplTest { @@ -218,7 +225,148 @@ public void shouldSerializeNullMapKeyWithoutProducingInvalidJson() { assertValidJson(serialized); Map custom = getValue(fromString(serialized), "data", "custom"); - assertThat(custom.get(""), equalTo((Object) "a value")); + assertThat(custom.get(""), equalTo("a value")); + } + + @Test + public void shouldReplaceMapThatContainsItself() { + Map map = new HashMap<>(); + map.put("self", map); + map.put("name", "value"); + + String serialized = new JsonSerializerImpl().toJson(map); + + assertValidJson(serialized); + Map result = fromString(serialized); + assertThat(result.get("self"), equalTo(CIRCULAR_REFERENCE_PLACEHOLDER)); + assertThat(result.get("name"), equalTo("value")); + } + + @Test + public void shouldReplaceCycleThroughNestedCollections() { + Map parent = new LinkedHashMap<>(); + List children = new ArrayList<>(); + Map child = new LinkedHashMap<>(); + child.put("parent", parent); + children.add(child); + parent.put("children", children); + + String serialized = new JsonSerializerImpl().toJson(payloadWithCustom("parent", parent)); + + assertValidJson(serialized); + List serializedChildren = getValue(fromString(serialized), + "data", "custom", "parent", "children"); + Map serializedChild = (Map) serializedChildren.get(0); + assertThat(serializedChild.get("parent"), equalTo(CIRCULAR_REFERENCE_PLACEHOLDER)); + } + + @Test + public void shouldReplaceCollectionThatContainsItself() { + List list = new ArrayList<>(); + list.add("first"); + list.add(list); + + String serialized = new JsonSerializerImpl().toJson(payloadWithCustom("list", list)); + + assertValidJson(serialized); + List result = getValue(fromString(serialized), "data", "custom", "list"); + assertThat(result, equalTo(asList((Object) "first", CIRCULAR_REFERENCE_PLACEHOLDER))); + } + + @Test + public void shouldReplaceArrayThatContainsItself() { + Object[] array = new Object[2]; + array[0] = "first"; + array[1] = array; + + String serialized = new JsonSerializerImpl().toJson(payloadWithCustom("array", array)); + + assertValidJson(serialized); + List result = getValue(fromString(serialized), "data", "custom", "array"); + assertThat(result, equalTo(asList((Object) "first", CIRCULAR_REFERENCE_PLACEHOLDER))); + } + + @Test + public void shouldReplaceJsonSerializableThatContainsItself() { + SelfReferencing value = new SelfReferencing(); + + String serialized = new JsonSerializerImpl().toJson(payloadWithCustom("value", value)); + + assertValidJson(serialized); + Map result = getValue(fromString(serialized), "data", "custom", "value"); + assertThat(result.get("self"), equalTo(CIRCULAR_REFERENCE_PLACEHOLDER)); + } + + @Test + public void shouldSerializeSharedValueEveryTimeItIsReached() { + Map shared = new HashMap<>(); + shared.put("key", "value"); + Map map = new LinkedHashMap<>(); + map.put("first", shared); + map.put("second", shared); + + String serialized = new JsonSerializerImpl().toJson(map); + + assertValidJson(serialized); + Map result = fromString(serialized); + assertThat(result.get("first"), equalTo(shared)); + assertThat(result.get("second"), equalTo(shared)); + } + + @Test + public void shouldReplaceValuesNestedDeeperThanMaxDepth() { + Map innermost = new HashMap<>(); + innermost.put("key", "value"); + Map middle = new HashMap<>(); + middle.put("innermost", innermost); + Map outer = new HashMap<>(); + outer.put("middle", middle); + outer.put("list", asList("a", "b")); + + String serialized = new JsonSerializerImpl(false, 2).toJson(outer); + + assertValidJson(serialized); + Map result = fromString(serialized); + assertThat(result.get("list"), equalTo(asList("a", "b"))); + Map serializedMiddle = (Map) result.get("middle"); + assertThat(serializedMiddle.get("innermost"), equalTo(MAX_DEPTH_PLACEHOLDER)); + } + + @Test + public void shouldLimitDeeplyNestedValuesToDefaultMaxDepth() { + Map root = new HashMap<>(); + Map current = root; + for (int i = 0; i < 100_000; i++) { + Map next = new HashMap<>(); + current.put("next", next); + current = next; + } + + String serialized = new JsonSerializerImpl().toJson(root); + + assertValidJson(serialized); + Object value = fromString(serialized); + int depth = 0; + while (value instanceof Map) { + value = ((Map) value).get("next"); + depth++; + } + assertThat(depth, is(JsonSerializerImpl.DEFAULT_MAX_DEPTH)); + assertThat(value, equalTo(MAX_DEPTH_PLACEHOLDER)); + } + + @Test(expected = IllegalArgumentException.class) + public void shouldRejectMaxDepthBelowOne() { + new JsonSerializerImpl(false, 0); + } + + private static class SelfReferencing implements JsonSerializable { + @Override + public Object asJson() { + Map json = new HashMap<>(); + json.put("self", this); + return json; + } } private Payload payloadWithCustom(String key, Object value) {