From ddf0a6f6adcabe0b77fe7c0bf8c8c47b2ccb2b80 Mon Sep 17 00:00:00 2001 From: David Kornel Date: Fri, 18 Sep 2026 10:22:52 +0200 Subject: [PATCH] StringSubstitutor recursive expansion has a cycle check but no fan-out, depth, or size bound checkCyclicSubstitution() only rejects a variable already on the current substitution stack. It does not bound acyclic fan-out (each of N references expanding to N more) or deep nesting, so a crafted variable map can drive interpolation into exponential output growth or a StackOverflowError without any variable repeating on the stack. Bound substitute() with a maximum interpolation depth (256) and a maximum total output size (16 MiB) per top-level substitution, both raising IllegalStateException. Mirrors the fix for the deprecated commons-lang3 StrSubstitutor (c8dc3121). --- src/changes/changes.xml | 1 + .../commons/text/StringSubstitutor.java | 47 ++++++++++++++++++- .../commons/text/StringSubstitutorTest.java | 29 ++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index c456633ab1..68681906ab 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,6 +45,7 @@ The type attribute can be add,update,fix,remove. + StringSubstitutor recursive variable expansion has a cycle check but no fan-out, depth, or size bound. Improve test coverage #732. TextStringBuilder.append(char[], int, int) uses wrong variable in exception message #735. StrBuilder.readFrom(Readable) exposes stale internal buffer to Readable parameter (#741). diff --git a/src/main/java/org/apache/commons/text/StringSubstitutor.java b/src/main/java/org/apache/commons/text/StringSubstitutor.java index 9c24a6deba..f1d8adda35 100644 --- a/src/main/java/org/apache/commons/text/StringSubstitutor.java +++ b/src/main/java/org/apache/commons/text/StringSubstitutor.java @@ -285,6 +285,12 @@ public String toString() { public static final StringMatcher DEFAULT_VALUE_DELIMITER = StringMatcherFactory.INSTANCE .stringMatcher(DEFAULT_VAR_DEFAULT); + /** Maximum variable-interpolation nesting depth; bounds deep nesting the cycle check does not. */ + private static final int MAX_SUBSTITUTION_DEPTH = 256; + + /** Maximum characters variable replacement may emit per top-level substitution; bounds acyclic fan-out. */ + private static final int MAX_SUBSTITUTION_LENGTH = 16 * 1024 * 1024; + /** * Creates a new instance using the interpolator string lookup * {@link StringLookupFactory#interpolatorStringLookup()}. @@ -489,6 +495,12 @@ public static String replaceSystemProperties(final Object source) { */ private StringMatcher valueDelimiterMatcher; + /** Current interpolation recursion depth. */ + private int substitutionDepth; + + /** Characters emitted by variable replacement in the current top-level substitution. */ + private long substitutionLength; + /** * Variable resolution is delegated to an implementor of {@link StringLookup}. */ @@ -1419,9 +1431,37 @@ protected boolean substitute(final TextStringBuilder builder, final int offset, * @param priorVariables The stack keeping track of the replaced variables, may be null. * @return The result. * @throws IllegalArgumentException if variable is not found and isEnableUndefinedVariableException() == true. + * @throws IllegalStateException if interpolation exceeds {@value #MAX_SUBSTITUTION_DEPTH} nesting levels or emits + * more than {@value #MAX_SUBSTITUTION_LENGTH} characters. * @since 1.9 */ - private Result substitute(final TextStringBuilder builder, final int offset, final int length, List priorVariables) { + private Result substitute(final TextStringBuilder builder, final int offset, final int length, final List priorVariables) { + if (substitutionDepth == 0) { + substitutionLength = 0; + } + if (substitutionDepth >= MAX_SUBSTITUTION_DEPTH) { + throw new IllegalStateException( + "Maximum interpolation depth (" + MAX_SUBSTITUTION_DEPTH + ") exceeded in variable substitution"); + } + substitutionDepth++; + try { + return substituteRecursive(builder, offset, length, priorVariables); + } finally { + substitutionDepth--; + } + } + + /** + * Recursive body of {@link #substitute(TextStringBuilder, int, int, List)}. + * + * @param builder The string builder to substitute into, not null. + * @param offset The start offset within the builder, must be valid. + * @param length The length within the builder to be processed, must be valid. + * @param priorVariables The stack keeping track of the replaced variables, may be null. + * @return The result. + * @throws IllegalArgumentException if variable is not found and isEnableUndefinedVariableException() == true. + */ + private Result substituteRecursive(final TextStringBuilder builder, final int offset, final int length, List priorVariables) { Objects.requireNonNull(builder, "builder"); final StringMatcher prefixMatcher = getVariablePrefixMatcher(); final StringMatcher suffixMatcher = getVariableSuffixMatcher(); @@ -1530,6 +1570,11 @@ private Result substitute(final TextStringBuilder builder, final int offset, fin final int varLen = varValue.length(); builder.replace(startPos, endPos, varValue); altered = true; + substitutionLength += varLen; + if (substitutionLength > MAX_SUBSTITUTION_LENGTH) { + throw new IllegalStateException("Maximum interpolation size (" + MAX_SUBSTITUTION_LENGTH + + " characters) exceeded in variable substitution"); + } int change = 0; if (!substitutionInValuesDisabled) { // recursive replace change = substitute(builder, startPos, varLen, priorVariables).lengthChange; diff --git a/src/test/java/org/apache/commons/text/StringSubstitutorTest.java b/src/test/java/org/apache/commons/text/StringSubstitutorTest.java index a6fdf45297..a3274d1450 100644 --- a/src/test/java/org/apache/commons/text/StringSubstitutorTest.java +++ b/src/test/java/org/apache/commons/text/StringSubstitutorTest.java @@ -234,6 +234,35 @@ void testDetectsCyclicSubstitution() { assertThrows(IllegalStateException.class, () -> StringSubstitutor.replace("Hi .", map, "<", ">")); } + @Test + void testDetectsDeepNesting() { + final Map map = new HashMap<>(); + for (int i = 0; i < 400; i++) { + map.put("v" + i, "${v" + (i + 1) + "}"); + } + map.put("v400", "x"); + final StringSubstitutor sub = new StringSubstitutor(map); + assertThrows(IllegalStateException.class, () -> sub.replace("${v0}")); + // Shallow nesting still resolves after the failure; the counters reset per call. + assertEquals("x", sub.replace("${v398}")); + } + + @Test + void testDetectsExponentialFanOut() { + // Acyclic fan-out: no variable repeats on the stack, so the cycle check never fires. + // Full expansion would be 10^6 leaves * 8 KiB = ~8 GiB. + final Map map = new HashMap<>(); + map.put("a6", StringUtils.repeat("x", 8192)); + for (int level = 5; level >= 0; level--) { + final StringBuilder value = new StringBuilder(); + for (int i = 0; i < 10; i++) { + value.append("${a").append(level + 1).append("}"); + } + map.put("a" + level, value.toString()); + } + assertThrows(IllegalStateException.class, () -> new StringSubstitutor(map).replace("${a0}")); + } + /** * Tests get set. */