Limit nesting depth in Exception XContent (elastic#103741)

Poor constructed exceptions may contain cycles between causes and/or
supressed exceptions. This commit changes the toXContent rendering of
exceptions to limit the nesting depth in order to avoid generating
infinite nested JSON (which actually leads to a `StackOverflowError`)

docs/changelog/103741.yaml

@@ -0,0 +1,5 @@
+pr: 103741
+summary: Limit nesting depth in Exception XContent
+area: Infra/Resiliency
+type: bug
+issues: []

modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java

@@ -36,7 +36,7 @@ private ErrorCauseWrapper(Throwable realCause) {
         this.realCause = realCause;
     }
 
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+    protected XContentBuilder toXContent(XContentBuilder builder, Params params, int nestedLevel) throws IOException {
         builder.field("type", getExceptionName(realCause));
         builder.field("reason", realCause.getMessage());
         return builder;

server/src/main/java/org/elasticsearch/ElasticsearchException.java

@@ -342,12 +342,20 @@ public static int getId(Class<? extends ElasticsearchException> exception) {
     }
 
     @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+    public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        return toXContent(builder, params, 0);
+    }
+
+    /**
+     * Equivalent to {@link org.elasticsearch.xcontent.ToXContent#toXContent(XContentBuilder, Params)} except that it limits nesting depth
+     * so that it can avoid stackoverflow errors.
+     */
+    protected XContentBuilder toXContent(XContentBuilder builder, Params params, int nestedLevel) throws IOException {
         Throwable ex = ExceptionsHelper.unwrapCause(this);
         if (ex != this) {
-            generateThrowableXContent(builder, params, this);
+            generateThrowableXContent(builder, params, this, nestedLevel);
         } else {
-            innerToXContent(builder, params, this, getExceptionName(), getMessage(), headers, metadata, getCause());
+            innerToXContent(builder, params, this, getExceptionName(), getMessage(), headers, metadata, getCause(), nestedLevel);
         }
         return builder;
     }
@@ -360,8 +368,17 @@ protected static void innerToXContent(
         String message,
         Map<String, List<String>> headers,
         Map<String, List<String>> metadata,
-        Throwable cause
+        Throwable cause,
+        int nestedLevel
     ) throws IOException {
+
+        if (nestedLevel > MAX_NESTED_EXCEPTION_LEVEL) {
+            var terminalException = new IllegalStateException("too many nested exceptions");
+            builder.field(TYPE, getExceptionName(terminalException));
+            builder.field(REASON, terminalException.getMessage());
+            return;
+        }
+
         builder.field(TYPE, type);
         builder.field(REASON, message);
 
@@ -377,7 +394,7 @@ protected static void innerToXContent(
             if (cause != null) {
                 builder.field(CAUSED_BY);
                 builder.startObject();
-                generateThrowableXContent(builder, params, cause);
+                generateThrowableXContent(builder, params, cause, nestedLevel + 1);
                 builder.endObject();
             }
         }
@@ -399,7 +416,7 @@ protected static void innerToXContent(
             builder.startArray(SUPPRESSED.getPreferredName());
             for (Throwable suppressed : allSuppressed) {
                 builder.startObject();
-                generateThrowableXContent(builder, params, suppressed);
+                generateThrowableXContent(builder, params, suppressed, nestedLevel + 1);
                 builder.endObject();
             }
             builder.endArray();
@@ -552,18 +569,27 @@ public static ElasticsearchException innerFromXContent(XContentParser parser, bo
     /**
      * Static toXContent helper method that renders {@link org.elasticsearch.ElasticsearchException} or {@link Throwable} instances
      * as XContent, delegating the rendering to {@link #toXContent(XContentBuilder, Params)}
-     * or {@link #innerToXContent(XContentBuilder, Params, Throwable, String, String, Map, Map, Throwable)}.
+     * or {@link #innerToXContent(XContentBuilder, Params, Throwable, String, String, Map, Map, Throwable, int)}.
      *
      * This method is usually used when the {@link Throwable} is rendered as a part of another XContent object, and its result can
      * be parsed back using the {@link #fromXContent(XContentParser)} method.
      */
     public static void generateThrowableXContent(XContentBuilder builder, Params params, Throwable t) throws IOException {
+        generateThrowableXContent(builder, params, t, 0);
+    }
+
+    /**
+     * Equivalent to {@link #generateThrowableXContent(XContentBuilder, Params, Throwable)} but limits nesting depth
+     * so that it can avoid stackoverflow errors.
+     */
+    protected static void generateThrowableXContent(XContentBuilder builder, Params params, Throwable t, int nestedLevel)
+        throws IOException {
         t = ExceptionsHelper.unwrapCause(t);
 
         if (t instanceof ElasticsearchException) {
-            ((ElasticsearchException) t).toXContent(builder, params);
+            ((ElasticsearchException) t).toXContent(builder, params, nestedLevel);
         } else {
-            innerToXContent(builder, params, t, getExceptionName(t), t.getMessage(), emptyMap(), emptyMap(), t.getCause());
+            innerToXContent(builder, params, t, getExceptionName(t), t.getMessage(), emptyMap(), emptyMap(), t.getCause(), nestedLevel);
         }
     }
 

server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java

@@ -129,15 +129,25 @@ protected void metadataToXContent(XContentBuilder builder, Params params) throws
     }
 
     @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+    protected XContentBuilder toXContent(XContentBuilder builder, Params params, int nestedLevel) throws IOException {
         Throwable ex = ExceptionsHelper.unwrapCause(this);
         if (ex != this) {
-            generateThrowableXContent(builder, params, this);
+            generateThrowableXContent(builder, params, this, nestedLevel);
         } else {
             // We don't have a cause when all shards failed, but we do have shards failures so we can "guess" a cause
             // (see {@link #getCause()}). Here, we use super.getCause() because we don't want the guessed exception to
             // be rendered twice (one in the "cause" field, one in "failed_shards")
-            innerToXContent(builder, params, this, getExceptionName(), getMessage(), getHeaders(), getMetadata(), super.getCause());
+            innerToXContent(
+                builder,
+                params,
+                this,
+                getExceptionName(),
+                getMessage(),
+                getHeaders(),
+                getMetadata(),
+                super.getCause(),
+                nestedLevel
+            );
         }
         return builder;
     }

server/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

@@ -43,6 +43,7 @@
 import org.elasticsearch.search.internal.ShardSearchContextId;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.transport.RemoteTransportException;
+import org.elasticsearch.xcontent.ObjectPath;
 import org.elasticsearch.xcontent.ToXContent;
 import org.elasticsearch.xcontent.XContent;
 import org.elasticsearch.xcontent.XContentBuilder;
@@ -67,8 +68,11 @@
 import static org.hamcrest.CoreMatchers.hasItem;
 import static org.hamcrest.CoreMatchers.hasItems;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasEntry;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.startsWith;
 
 public class ElasticsearchExceptionTests extends ESTestCase {
@@ -724,6 +728,48 @@ public void testToXContentWithHeadersAndMetadata() throws IOException {
         );
     }
 
+    @SuppressWarnings("unchecked")
+    public void testToXContentWithObjectCycles() throws Exception {
+        ElasticsearchException root = new ElasticsearchException("root exception");
+
+        ElasticsearchException suppressed1 = new ElasticsearchException("suppressed#1", root);
+
+        ElasticsearchException suppressed2 = new ElasticsearchException("suppressed#2");
+        ElasticsearchException suppressed3 = new ElasticsearchException("suppressed#3");
+        suppressed3.addSuppressed(suppressed2);
+        suppressed2.addSuppressed(suppressed3);
+
+        root.addSuppressed(suppressed1);
+        root.addSuppressed(suppressed2);
+        root.addSuppressed(suppressed3);
+
+        // Because we support up to 100 nested exceptions, this JSON ends up very long.
+        // Rather than assert the full content, we check that
+        // (a) it generated successfully (no StackOverflowErrors)
+        BytesReference xContent = XContentHelper.toXContent(root, XContentType.JSON, randomBoolean());
+        // (b) it's valid JSON
+        final Map<String, Object> map = XContentHelper.convertToMap(xContent, false, XContentType.JSON).v2();
+        // (c) it contains the right content
+        assertThat(ObjectPath.eval("type", map), equalTo("exception"));
+        assertThat(ObjectPath.eval("reason", map), equalTo("root exception"));
+        assertThat(ObjectPath.eval("suppressed.0.reason", map), equalTo("suppressed#1"));
+        assertThat(ObjectPath.eval("suppressed.0.caused_by.reason", map), equalTo("root exception"));
+        assertThat(ObjectPath.eval("suppressed.0.caused_by.suppressed.0.reason", map), equalTo("suppressed#1"));
+        assertThat(ObjectPath.eval("suppressed.1.reason", map), equalTo("suppressed#2"));
+        assertThat(ObjectPath.eval("suppressed.1.suppressed.0.reason", map), equalTo("suppressed#3"));
+        assertThat(ObjectPath.eval("suppressed.1.suppressed.0.suppressed.0.reason", map), equalTo("suppressed#2"));
+        assertThat(ObjectPath.eval("suppressed.2.reason", map), equalTo("suppressed#3"));
+        assertThat(ObjectPath.eval("suppressed.2.suppressed.0.reason", map), equalTo("suppressed#2"));
+        assertThat(ObjectPath.eval("suppressed.2.suppressed.0.suppressed.0.reason", map), equalTo("suppressed#3"));
+
+        String tailExceptionPath = ".suppressed.0.caused_by".repeat(50).substring(1) + ".suppressed.0";
+        final Object tailException = ObjectPath.eval(tailExceptionPath, map);
+        assertThat(tailException, not(nullValue()));
+        assertThat(tailException, instanceOf(Map.class));
+        assertThat((Map<String, Object>) tailException, hasEntry("reason", "too many nested exceptions"));
+        assertThat((Map<String, Object>) tailException, hasEntry("type", "illegal_state_exception"));
+    }
+
     public void testFromXContent() throws IOException {
         final XContent xContent = randomFrom(XContentType.values()).xContent();
         XContentBuilder builder = XContentBuilder.builder(xContent)