From 21404c95c78ac5cf7f838147b402569139cfc90c Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 6 Nov 2025 15:27:53 +0100 Subject: [PATCH 1/6] Fix --- .../iast/propagation/StringModuleImpl.java | 29 +++++++- .../iast/propagation/StringModuleTest.groovy | 71 +++++++++++++++++++ .../scala/StringContextCallSiteTest.groovy | 46 ++++++++++++ 3 files changed, 144 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java index b74efab5090..09d0ff1eac8 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java @@ -528,13 +528,13 @@ public void onStringFormat( final int parsedIndex = Integer.parseInt(index.substring(0, index.length() - 1)) - 1; // remove the index before the formatting without increment the current state parameter = parameters[parsedIndex]; - formattedValue = String.format(locale, placeholder.replace(index, ""), parameter); + formattedValue = formatValue(locale, placeholder.replace(index, ""), parameter); } else { if (!checkParameterBounds(format, parameters, paramIndex)) { return; // return without tainting the string in case of error } parameter = parameters[paramIndex++]; - formattedValue = String.format(locale, placeholder, parameter); + formattedValue = formatValue(locale, placeholder, parameter); } taintedObject = to.get(parameter); } @@ -571,6 +571,31 @@ private static boolean checkParameterBounds( return false; } + /** + * Formats a value using String.format with fallback to String.valueOf on IllegalFormatException. + * + * @param locale the locale to use for formatting + * @param placeholder the format placeholder (e.g., "%f", "%d") + * @param parameter the parameter to format + * @return the formatted value or String.valueOf(parameter) if formatting fails + */ + private static String formatValue( + @Nullable final Locale locale, final String placeholder, final Object parameter) { + try { + return String.format(locale, placeholder, parameter); + } catch (final java.util.IllegalFormatException e) { + // Fallback to String.valueOf if format conversion fails (e.g., wrong type for format + // specifier) + LOG.debug( + SEND_TELEMETRY, + "Format conversion failed for placeholder {} with parameter type {}: {}", + placeholder, + parameter == null ? "null" : parameter.getClass().getName(), + e.getMessage()); + return String.valueOf(parameter); + } + } + @Override public void onStringFormat( @Nonnull final Iterable literals, diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/propagation/StringModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/propagation/StringModuleTest.groovy index 77e406ffa15..53ef8712769 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/propagation/StringModuleTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/propagation/StringModuleTest.groovy @@ -1616,4 +1616,75 @@ class StringModuleTest extends IastModuleImplTestBase { return "my_input" } } + + void 'test string format with incompatible type for float specifier'() { + given: + final pattern = 'User: %s and Balance: %f' + final params = ['admin', 'not-a-number'] as Object[] + + when: + // This should not throw IllegalFormatConversionException + // The fix should handle it gracefully with String.valueOf() fallback + final result = String.format(pattern, params) + + then: + // Before the fix, this would throw IllegalFormatConversionException + // After the fix, it should work via formatValue() helper + thrown(IllegalFormatConversionException) + } + + void 'test onStringFormat with incompatible type for float specifier'() { + given: + final taintedObjects = ctx.getTaintedObjects() + final pattern = 'User: %s and Balance: %f' + final params = [ + addFromTaintFormat(taintedObjects, '==>admin<=='), + addFromTaintFormat(taintedObjects, '==>not-a-number<==') + ] as Object[] + final result = 'User: admin and Balance: not-a-number' + objectHolder.add(params[0]) + objectHolder.add(params[1]) + + when: + // This should not throw IllegalFormatConversionException thanks to the fix + // Result will have fallback formatting: "User: admin and Balance: not-a-number" + module.onStringFormat(pattern, params, result) + + then: + // Should complete without throwing IllegalFormatConversionException + notThrown(IllegalFormatConversionException) + + // Verify the result is tainted + final tainted = taintedObjects.get(result) + tainted != null + tainted.getRanges().length > 0 + } + + void 'test onStringFormat with multiple incompatible types'() { + given: + final taintedObjects = ctx.getTaintedObjects() + final pattern = 'Name: %s, Age: %d, Score: %f' + final params = [ + addFromTaintFormat(taintedObjects, '==>John<=='), + addFromTaintFormat(taintedObjects, '==>thirty<=='), + addFromTaintFormat(taintedObjects, '==>high<==') + ] as Object[] + final result = 'Name: John, Age: thirty, Score: high' + objectHolder.add(params[0]) + objectHolder.add(params[1]) + objectHolder.add(params[2]) + + when: + // This should not throw IllegalFormatConversionException thanks to the fix + module.onStringFormat(pattern, params, result) + + then: + // Should complete without throwing IllegalFormatConversionException + notThrown(IllegalFormatConversionException) + + // Verify the result is tainted + final tainted = taintedObjects.get(result) + tainted != null + tainted.getRanges().length > 0 + } } diff --git a/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringContextCallSiteTest.groovy b/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringContextCallSiteTest.groovy index baf43e9e091..862f8c35901 100644 --- a/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringContextCallSiteTest.groovy +++ b/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringContextCallSiteTest.groovy @@ -99,5 +99,51 @@ class StringContextCallSiteTest extends AbstractIastScalaTest { ) 0 * _ } + + void 'test string format with incompatible float type'() { + setup: + final iastModule = Mock(StringModule) + InstrumentationBridge.registerIastModule(iastModule) + + when: + // Directly test the StringModuleImpl.onStringFormat with incompatible types + // This simulates what happens when Scala f-interpolator passes wrong types at runtime + final pattern = 'User: %s and Balance: %f' + final params = ['admin', 'not-a-number'] as Object[] + iastModule.onStringFormat(pattern, params, _) >> { String fmt, Object[] args, String result -> + // Call the real implementation + def ctx = mock(datadog.trace.api.iast.IastContext) + def taintedObjects = mock(com.datadog.iast.taint.TaintedObjects) + ctx.getTaintedObjects() >> taintedObjects + datadog.trace.api.iast.IastContext.Provider.get() >> ctx + } + + then: + // Test should not throw IllegalFormatConversionException + // The fix should handle it gracefully + notThrown(IllegalFormatConversionException) + } + + void 'test string format with multiple incompatible types'() { + setup: + final iastModule = Mock(StringModule) + InstrumentationBridge.registerIastModule(iastModule) + + when: + // Test with multiple type mismatches + final pattern = 'Name: %s, Age: %d, Score: %f' + final params = ['John', 'thirty', 'high'] as Object[] + iastModule.onStringFormat(pattern, params, _) >> { String fmt, Object[] args, String result -> + // Call the real implementation + def ctx = mock(datadog.trace.api.iast.IastContext) + def taintedObjects = mock(com.datadog.iast.taint.TaintedObjects) + ctx.getTaintedObjects() >> taintedObjects + datadog.trace.api.iast.IastContext.Provider.get() >> ctx + } + + then: + // Test should not throw IllegalFormatConversionException + notThrown(IllegalFormatConversionException) + } } From bdda8b165f46e6e517abefd5eafe2e6bf1a1b86e Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 13 Nov 2025 11:10:44 +0100 Subject: [PATCH 2/6] New approach --- .../iast/propagation/StringModuleImpl.java | 9 +- .../scala/StringOpsCallSite.java | 88 ++++++++++- .../scala/StringOpsFormatCallSiteTest.groovy | 147 ++++++++++++++++++ .../foo/bar/TestStringOpsFormatSuite.scala | 63 ++++++++ dd-smoke-tests/iast-propagation/build.gradle | 2 +- .../src/main/scala/ScalaPropagation.scala | 11 +- .../smoketest/IastPropagationSmokeTest.groovy | 4 +- .../IastUnwrapScakaNumberSmokeTest.groovy | 89 +++++++++++ 8 files changed, 403 insertions(+), 10 deletions(-) create mode 100644 dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringOpsFormatCallSiteTest.groovy create mode 100644 dd-java-agent/instrumentation/scala/src/test/scala/foo/bar/TestStringOpsFormatSuite.scala create mode 100644 dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java index 09d0ff1eac8..ea88d0adb4e 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java @@ -572,27 +572,26 @@ private static boolean checkParameterBounds( } /** - * Formats a value using String.format with fallback to String.valueOf on IllegalFormatException. + * Formats a value using String.format with telemtry report on IllegalFormatException. * * @param locale the locale to use for formatting * @param placeholder the format placeholder (e.g., "%f", "%d") * @param parameter the parameter to format - * @return the formatted value or String.valueOf(parameter) if formatting fails + * @return the formatted value */ private static String formatValue( @Nullable final Locale locale, final String placeholder, final Object parameter) { try { return String.format(locale, placeholder, parameter); } catch (final java.util.IllegalFormatException e) { - // Fallback to String.valueOf if format conversion fails (e.g., wrong type for format - // specifier) + // Send telemetry to improve future bug fixes LOG.debug( SEND_TELEMETRY, "Format conversion failed for placeholder {} with parameter type {}: {}", placeholder, parameter == null ? "null" : parameter.getClass().getName(), e.getMessage()); - return String.valueOf(parameter); + throw e; } } diff --git a/dd-java-agent/instrumentation/scala/src/main/java/datadog/trace/instrumentation/scala/StringOpsCallSite.java b/dd-java-agent/instrumentation/scala/src/main/java/datadog/trace/instrumentation/scala/StringOpsCallSite.java index 74322d9319d..414c1640bad 100644 --- a/dd-java-agent/instrumentation/scala/src/main/java/datadog/trace/instrumentation/scala/StringOpsCallSite.java +++ b/dd-java-agent/instrumentation/scala/src/main/java/datadog/trace/instrumentation/scala/StringOpsCallSite.java @@ -6,7 +6,9 @@ import datadog.trace.api.iast.Propagation; import datadog.trace.api.iast.propagation.StringModule; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import scala.collection.immutable.StringOps; +import scala.math.ScalaNumber; @Propagation @CallSite( @@ -28,7 +30,10 @@ public static String afterInterpolation( if (module != null) { try { final Object[] args = ScalaJavaConverters.toArray(params); - module.onStringFormat(stringOps.toString(), args, result); + // Unwrap Scala ScalaNumber types to their underlying Java representation + // This replicates Scala's unwrapArg() behavior before calling String.format + final Object[] unwrappedArgs = unwrapScalaNumbers(args); + module.onStringFormat(stringOps.toString(), unwrappedArgs, result); } catch (final Throwable e) { module.onUnexpectedException("afterInterpolation threw", e); } @@ -47,11 +52,90 @@ public static String afterInterpolation( if (module != null) { try { final Object[] args = ScalaJavaConverters.toArray(params); - module.onStringFormat(pattern, args, result); + // Unwrap Scala ScalaNumber types to their underlying Java representation + // This replicates Scala's unwrapArg() behavior before calling String.format + final Object[] unwrappedArgs = unwrapScalaNumbers(args); + module.onStringFormat(pattern, unwrappedArgs, result); } catch (final Throwable e) { module.onUnexpectedException("afterInterpolation threw", e); } } return result; } + + /** + * Unwraps all Scala ScalaNumber types in an array to their underlying Java representations. + * + *

This method replicates Scala's unwrapArg behavior from StringLike.scala: + * + * 

+   * private def unwrapArg(arg: Any): AnyRef = arg match {
+   *   case x: ScalaNumber => x.underlying
+   *   case x => x.asInstanceOf[AnyRef]
+   * }
+   *
+ * + *

The conversion is: + * + *

    + *
  • scala.math.BigDecimal -> java.math.BigDecimal (via underlying()) + *
  • scala.math.BigInt -> java.math.BigInteger (via underlying()) + *
  • Other types -> unchanged + *
+ * + *

This ensures String.format receives java.math.BigDecimal/BigInteger instead of + * scala.math.BigDecimal/BigInt, preventing IllegalFormatConversionException while maintaining + * correct format results and taint tracking. + * + * @param args the array of arguments to unwrap + * @return a new array with ScalaNumber instances unwrapped, or the original array if no changes + */ + @Nonnull + private static Object[] unwrapScalaNumbers(@Nonnull final Object[] args) { + boolean needsUnwrap = false; + // First pass: check if any argument needs unwrapping + for (final Object arg : args) { + if (arg != null && isScalaNumber(arg)) { + needsUnwrap = true; + break; + } + } + // If no unwrapping needed, return original array + if (!needsUnwrap) { + return args; + } + // Second pass: create new array with unwrapped values + final Object[] unwrapped = new Object[args.length]; + for (int i = 0; i < args.length; i++) { + unwrapped[i] = unwrapScalaNumber(args[i]); + } + return unwrapped; + } + + /** + * Checks if an object is a Scala ScalaNumber (BigDecimal or BigInt). + * + * @param arg the object to check + * @return true if the object is a ScalaNumber instance + */ + private static boolean isScalaNumber(@Nonnull final Object arg) { + return arg instanceof scala.math.ScalaNumber; + } + + /** + * Unwraps a single Scala ScalaNumber to its underlying Java representation. + * + * @param arg the argument to unwrap (can be null) + * @return the unwrapped value if ScalaNumber, otherwise the original value + */ + @Nullable + private static Object unwrapScalaNumber(@Nullable final Object arg) { + if (arg == null) { + return null; + } + if (arg instanceof scala.math.ScalaNumber) { + return ((ScalaNumber) arg).underlying(); + } + return arg; + } } diff --git a/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringOpsFormatCallSiteTest.groovy b/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringOpsFormatCallSiteTest.groovy new file mode 100644 index 00000000000..5675d6d57dc --- /dev/null +++ b/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringOpsFormatCallSiteTest.groovy @@ -0,0 +1,147 @@ +package datadog.trace.instrumentation.scala + +import datadog.trace.api.iast.InstrumentationBridge +import datadog.trace.api.iast.propagation.StringModule + +/** + * Tests for StringOpsCallSite.unwrapScalaNumbers functionality. + * + * These tests verify that Scala ScalaNumber types (BigDecimal, BigInt) are properly + * unwrapped to their underlying Java representations (java.math.BigDecimal, java.math.BigInteger) + * before being passed to IAST's onStringFormat. + * + * This prevents IllegalFormatConversionException and ensures correct taint tracking. + */ +class StringOpsFormatCallSiteTest extends AbstractIastScalaTest { + + @Override + String suiteName() { + return 'foo.bar.TestStringOpsFormatSuite' + } + + void 'test formatBigDecimal with scala.math.BigDecimal'() { + setup: + final iastModule = Mock(StringModule) + InstrumentationBridge.registerIastModule(iastModule) + + when: + final result = testSuite.formatBigDecimal('123.456') + + then: + result == 'Value: 123.456000' + // Verify that the unwrapped java.math.BigDecimal is passed, not scala.math.BigDecimal + 1 * iastModule.onStringFormat( + 'Value: %f', + { Object[] args -> + args.length == 1 && + args[0] != null && + args[0].getClass() == BigDecimal && + args[0].toString() == '123.456' + }, + 'Value: 123.456000' + ) + 0 * iastModule.onUnexpectedException(_, _) + } + + void 'test formatBigInt with scala.math.BigInt'() { + setup: + final iastModule = Mock(StringModule) + InstrumentationBridge.registerIastModule(iastModule) + + when: + final result = testSuite.formatBigInt('12345') + + then: + result == 'Count: 12345' + // Verify that the unwrapped java.math.BigInteger is passed, not scala.math.BigInt + 1 * iastModule.onStringFormat( + 'Count: %d', + { Object[] args -> + args.length == 1 && + args[0] != null && + args[0].getClass() == BigInteger && + args[0].toString() == '12345' + }, + 'Count: 12345' + ) + 0 * iastModule.onUnexpectedException(_, _) + } + + void 'test formatMultipleScalaNumbers with BigDecimal and BigInt'() { + setup: + final iastModule = Mock(StringModule) + InstrumentationBridge.registerIastModule(iastModule) + + when: + final result = testSuite.formatMultipleScalaNumbers('99.99', '42') + + then: + result == 'Decimal: 99.990000, Integer: 42' + // Verify that both ScalaNumbers are unwrapped + 1 * iastModule.onStringFormat( + 'Decimal: %f, Integer: %d', + { Object[] args -> + args.length == 2 && + args[0] != null && + args[0].getClass() == BigDecimal && + args[0].toString() == '99.99' && + args[1] != null && + args[1].getClass() == BigInteger && + args[1].toString() == '42' + }, + 'Decimal: 99.990000, Integer: 42' + ) + 0 * iastModule.onUnexpectedException(_, _) + } + + void 'test formatMixed with BigDecimal and String'() { + setup: + final iastModule = Mock(StringModule) + InstrumentationBridge.registerIastModule(iastModule) + + when: + final result = testSuite.formatMixed('3.14', 'hello') + + then: + result == 'Value: 3.140000, Text: hello' + // Verify that BigDecimal is unwrapped but String remains unchanged + 1 * iastModule.onStringFormat( + 'Value: %f, Text: %s', + { Object[] args -> + args.length == 2 && + args[0] != null && + args[0].getClass() == BigDecimal && + args[0].toString() == '3.14' && + args[1] instanceof String && + args[1] == 'hello' + }, + 'Value: 3.140000, Text: hello' + ) + 0 * iastModule.onUnexpectedException(_, _) + } + + void 'test formatString with regular String arguments (no unwrapping)'() { + setup: + final iastModule = Mock(StringModule) + InstrumentationBridge.registerIastModule(iastModule) + + when: + final result = testSuite.formatString('left', 'right') + + then: + result == 'Left: left, Right: right' + // Verify that String arguments are passed unchanged (no unwrapping needed) + 1 * iastModule.onStringFormat( + 'Left: %s, Right: %s', + { Object[] args -> + args.length == 2 && + args[0] instanceof String && + args[0] == 'left' && + args[1] instanceof String && + args[1] == 'right' + }, + 'Left: left, Right: right' + ) + 0 * iastModule.onUnexpectedException(_, _) + } +} diff --git a/dd-java-agent/instrumentation/scala/src/test/scala/foo/bar/TestStringOpsFormatSuite.scala b/dd-java-agent/instrumentation/scala/src/test/scala/foo/bar/TestStringOpsFormatSuite.scala new file mode 100644 index 00000000000..f3c80ddde8b --- /dev/null +++ b/dd-java-agent/instrumentation/scala/src/test/scala/foo/bar/TestStringOpsFormatSuite.scala @@ -0,0 +1,63 @@ +package foo.bar + +import org.slf4j.LoggerFactory + +class TestStringOpsFormatSuite { + + private val LOGGER = LoggerFactory.getLogger("TestStringOpsFormatSuite") + + /** Test format with scala.math.BigDecimal using %f placeholder. + */ + def formatBigDecimal(value: String): String = { + LOGGER.debug("Before formatBigDecimal {}", value) + val bd = BigDecimal(value) + val pattern = "Value: %f" + val result: String = pattern.format(bd) + LOGGER.debug("After formatBigDecimal {}", result) + result + } + + /** Test format with scala.math.BigInt using %d placeholder. + */ + def formatBigInt(value: String): String = { + LOGGER.debug("Before formatBigInt {}", value) + val bi = BigInt(value) + val pattern = "Count: %d" + val result: String = pattern.format(bi) + LOGGER.debug("After formatBigInt {}", result) + result + } + + /** Test format with multiple ScalaNumber arguments. + */ + def formatMultipleScalaNumbers(decimal: String, integer: String): String = { + LOGGER.debug("Before formatMultipleScalaNumbers {} {}", Array(decimal, integer): _*) + val bd = BigDecimal(decimal) + val bi = BigInt(integer) + val pattern = "Decimal: %f, Integer: %d" + val result: String = pattern.format(bd, bi) + LOGGER.debug("After formatMultipleScalaNumbers {}", result) + result + } + + /** Test format with mixed ScalaNumber and regular arguments. + */ + def formatMixed(decimal: String, text: String): String = { + LOGGER.debug("Before formatMixed {} {}", Array(decimal, text): _*) + val bd = BigDecimal(decimal) + val pattern = "Value: %f, Text: %s" + val result: String = pattern.format(bd, text) + LOGGER.debug("After formatMixed {}", result) + result + } + + /** Test format with regular String arguments (no unwrapping needed). + */ + def formatString(left: String, right: String): String = { + LOGGER.debug("Before formatString {} {}", Array(left, right): _*) + val pattern = "Left: %s, Right: %s" + val result: String = pattern.format(left, right) + LOGGER.debug("After formatString {}", result) + result + } +} diff --git a/dd-smoke-tests/iast-propagation/build.gradle b/dd-smoke-tests/iast-propagation/build.gradle index 93e5f5a8a08..e9c0b0fd8a8 100644 --- a/dd-smoke-tests/iast-propagation/build.gradle +++ b/dd-smoke-tests/iast-propagation/build.gradle @@ -36,7 +36,7 @@ tasks.named("shadowJar", ShadowJar) { dependencies { implementation project(':dd-trace-api') implementation group: 'org.springframework.boot', name: 'spring-boot-starter-web', version: '2.5.4' - implementation libs.scala + implementation libs.scala213 implementation libs.groovy implementation libs.kotlin diff --git a/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala b/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala index 07674d00325..66dc9218410 100644 --- a/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala +++ b/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala @@ -4,7 +4,7 @@ import scala.collection.JavaConverters._ class ScalaPropagation extends Supplier[util.List[String]] { - override def get(): util.List[String] = List("plus", "s", "f", "raw").asJava + override def get(): util.List[String] = List("plus", "s", "f", "raw", "formatBigDecimal").asJava def plus(param: String): String = param + param @@ -13,4 +13,13 @@ class ScalaPropagation extends Supplier[util.List[String]] { def f(param: String): String = f"f interpolation: $param" def raw(param: String): String = raw"raw interpolation: $param" + + /** Test case to reproduce IllegalFormatConversionException from original stack trace. + * Root cause: Scala's format() calls unwrapArg() which converts BigDecimal -> Double. + * IAST receives BigDecimal without unwrapping, causing IllegalFormatConversionException. + */ + def formatBigDecimal(param: String): String = { + val numericValue = BigDecimal(param) + "Value: %f".format(numericValue) + } } diff --git a/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastPropagationSmokeTest.groovy b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastPropagationSmokeTest.groovy index e620c065ebb..5416eff45b0 100644 --- a/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastPropagationSmokeTest.groovy +++ b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastPropagationSmokeTest.groovy @@ -56,7 +56,9 @@ class IastPropagationSmokeTest extends AbstractIastServerSmokeTest { final languages = ['java', 'scala', 'groovy', 'kotlin'] return languages.collectMany { language -> final methods = new JsonSlurper().parse(new URL("http://localhost:${httpPort}/${language}")) as List - return methods.collect { method -> [language, method] } + // Exclude formatBigDecimal - it has a dedicated test in IastUnwrapScalaNumberSmokeTest + // because it requires special handling (no taint propagation expected) + return methods.findAll { it != 'formatBigDecimal' }.collect { method -> [language, method] } } } } diff --git a/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy new file mode 100644 index 00000000000..5beb69294d4 --- /dev/null +++ b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy @@ -0,0 +1,89 @@ +package datadog.smoketest + +import static datadog.trace.api.config.IastConfig.IAST_DEBUG_ENABLED +import static datadog.trace.api.config.IastConfig.IAST_DETECTION_MODE +import static datadog.trace.api.config.IastConfig.IAST_ENABLED + +import okhttp3.Request + +/** + * Smoke test to verify that StringOpsCallSite.unwrapScalaNumbers() correctly unwraps + * Scala ScalaNumber types (BigDecimal, BigInt) to their underlying Java representations + * before passing them to IAST's onStringFormat. + * + *

Background: + * Scala's String.format() internally calls unwrapArg() which converts: + * - scala.math.BigDecimal -> java.math.BigDecimal (via underlying() method) + * - scala.math.BigInt -> java.math.BigInteger (via underlying() method) + * + *

Problem: + * IAST's @CallSite.After interceptor captures arguments AFTER Scala's runtime has already + * called unwrapArg(), but our instrumentation receives the original Scala types. + * Without unwrapping, passing scala.math.BigDecimal to String.format("%f") causes + * IllegalFormatConversionException: f != scala.math.BigDecimal + * + *

Solution: + * StringOpsCallSite now calls unwrapScalaNumbers() before passing arguments to onStringFormat, + * ensuring java.math types are used instead of scala.math types. + * + *

This test verifies: + * 1. No IllegalFormatConversionException is thrown (200 response, not 500) + * 2. No error telemetry is logged (checked via isErrorLog override) + * 3. Correct formatted output is returned + */ +class IastUnwrapScakaNumberSmokeTest extends AbstractIastServerSmokeTest { + + @Override + boolean isErrorLog(String log) { + // Detect the specific telemetry error that would occur if unwrapScalaNumber didn't work. + // This exact message is logged by StringModuleImpl.formatValue() when String.format() + // throws IllegalFormatConversionException due to type mismatch. + return super.isErrorLog(log) || + log.contains("Format conversion failed for placeholder %f with parameter type scala.math.BigDecimal: f != scala.math.BigDecimal") + } + + @Override + ProcessBuilder createProcessBuilder() { + final jarPath = System.getProperty('datadog.smoketest.springboot.shadowJar.path') + List command = [] + command.add(javaPath()) + command.addAll(defaultJavaProperties) + command.addAll((String[]) [ + withSystemProperty(IAST_ENABLED, true), + withSystemProperty(IAST_DETECTION_MODE, 'FULL'), + withSystemProperty(IAST_DEBUG_ENABLED, true), + '-jar', + jarPath, + "--server.port=${httpPort}" + ]) + final processBuilder = new ProcessBuilder(command) + processBuilder.directory(new File(buildDirectory)) + return processBuilder + } + + void 'test scala formatBigDecimal with unwrapScalaNumber'() { + setup: + String url = "http://localhost:${httpPort}/scala/formatBigDecimal?param=123.456" + + when: + def request = new Request.Builder().url(url).get().build() + def response = client.newCall(request).execute() + + then: + def responseBodyStr = response.body().string() + // Verify no IllegalFormatConversionException occurred + // Without unwrapScalaNumber: scala.math.BigDecimal -> String.format("%f") -> exception -> 500 + // With unwrapScalaNumber: java.math.BigDecimal -> String.format("%f") -> success -> 200 + response.code() == 200 + responseBodyStr == "Value: 123.456000" + + // Note: Taint propagation is NOT expected in this scenario because: + // 1. The input String "123.456" is tainted (http.request.parameter) + // 2. BigDecimal(param) creates a numeric object - IAST does not taint non-String objects + // 3. format() converts back to String, but the BigDecimal was never tainted, so no taint to propagate + // + // This test's purpose is to verify that unwrapScalaNumber prevents IllegalFormatConversionException, + // not to verify taint propagation through String -> BigDecimal -> String conversions. + // Error detection happens automatically via isErrorLog() override + assertNoErrorLogs() in cleanupSpec(). + } +} From 083da2566aa37f5f5b6c47bb69b077027bbf5cd0 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 13 Nov 2025 13:43:48 +0100 Subject: [PATCH 3/6] remove tests --- .../iast/propagation/StringModuleTest.groovy | 71 ------------------- 1 file changed, 71 deletions(-) diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/propagation/StringModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/propagation/StringModuleTest.groovy index 53ef8712769..77e406ffa15 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/propagation/StringModuleTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/propagation/StringModuleTest.groovy @@ -1616,75 +1616,4 @@ class StringModuleTest extends IastModuleImplTestBase { return "my_input" } } - - void 'test string format with incompatible type for float specifier'() { - given: - final pattern = 'User: %s and Balance: %f' - final params = ['admin', 'not-a-number'] as Object[] - - when: - // This should not throw IllegalFormatConversionException - // The fix should handle it gracefully with String.valueOf() fallback - final result = String.format(pattern, params) - - then: - // Before the fix, this would throw IllegalFormatConversionException - // After the fix, it should work via formatValue() helper - thrown(IllegalFormatConversionException) - } - - void 'test onStringFormat with incompatible type for float specifier'() { - given: - final taintedObjects = ctx.getTaintedObjects() - final pattern = 'User: %s and Balance: %f' - final params = [ - addFromTaintFormat(taintedObjects, '==>admin<=='), - addFromTaintFormat(taintedObjects, '==>not-a-number<==') - ] as Object[] - final result = 'User: admin and Balance: not-a-number' - objectHolder.add(params[0]) - objectHolder.add(params[1]) - - when: - // This should not throw IllegalFormatConversionException thanks to the fix - // Result will have fallback formatting: "User: admin and Balance: not-a-number" - module.onStringFormat(pattern, params, result) - - then: - // Should complete without throwing IllegalFormatConversionException - notThrown(IllegalFormatConversionException) - - // Verify the result is tainted - final tainted = taintedObjects.get(result) - tainted != null - tainted.getRanges().length > 0 - } - - void 'test onStringFormat with multiple incompatible types'() { - given: - final taintedObjects = ctx.getTaintedObjects() - final pattern = 'Name: %s, Age: %d, Score: %f' - final params = [ - addFromTaintFormat(taintedObjects, '==>John<=='), - addFromTaintFormat(taintedObjects, '==>thirty<=='), - addFromTaintFormat(taintedObjects, '==>high<==') - ] as Object[] - final result = 'Name: John, Age: thirty, Score: high' - objectHolder.add(params[0]) - objectHolder.add(params[1]) - objectHolder.add(params[2]) - - when: - // This should not throw IllegalFormatConversionException thanks to the fix - module.onStringFormat(pattern, params, result) - - then: - // Should complete without throwing IllegalFormatConversionException - notThrown(IllegalFormatConversionException) - - // Verify the result is tainted - final tainted = taintedObjects.get(result) - tainted != null - tainted.getRanges().length > 0 - } } From 5da0e5835778a1056c9a1660bd2d047fab359156 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 13 Nov 2025 13:55:27 +0100 Subject: [PATCH 4/6] sef review --- .../iast-propagation/src/main/scala/ScalaPropagation.scala | 2 +- .../groovy/datadog/smoketest/IastPropagationSmokeTest.groovy | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala b/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala index 66dc9218410..76b539d0e53 100644 --- a/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala +++ b/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala @@ -4,7 +4,7 @@ import scala.collection.JavaConverters._ class ScalaPropagation extends Supplier[util.List[String]] { - override def get(): util.List[String] = List("plus", "s", "f", "raw", "formatBigDecimal").asJava + override def get(): util.List[String] = List("plus", "s", "f", "raw").asJava def plus(param: String): String = param + param diff --git a/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastPropagationSmokeTest.groovy b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastPropagationSmokeTest.groovy index 5416eff45b0..e620c065ebb 100644 --- a/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastPropagationSmokeTest.groovy +++ b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastPropagationSmokeTest.groovy @@ -56,9 +56,7 @@ class IastPropagationSmokeTest extends AbstractIastServerSmokeTest { final languages = ['java', 'scala', 'groovy', 'kotlin'] return languages.collectMany { language -> final methods = new JsonSlurper().parse(new URL("http://localhost:${httpPort}/${language}")) as List - // Exclude formatBigDecimal - it has a dedicated test in IastUnwrapScalaNumberSmokeTest - // because it requires special handling (no taint propagation expected) - return methods.findAll { it != 'formatBigDecimal' }.collect { method -> [language, method] } + return methods.collect { method -> [language, method] } } } } From 0b5232509908acab9d379d96356fbaeb3b2c760e Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 13 Nov 2025 14:04:48 +0100 Subject: [PATCH 5/6] sef review --- .../scala/StringContextCallSiteTest.groovy | 46 ------------------- 1 file changed, 46 deletions(-) diff --git a/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringContextCallSiteTest.groovy b/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringContextCallSiteTest.groovy index 862f8c35901..baf43e9e091 100644 --- a/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringContextCallSiteTest.groovy +++ b/dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringContextCallSiteTest.groovy @@ -99,51 +99,5 @@ class StringContextCallSiteTest extends AbstractIastScalaTest { ) 0 * _ } - - void 'test string format with incompatible float type'() { - setup: - final iastModule = Mock(StringModule) - InstrumentationBridge.registerIastModule(iastModule) - - when: - // Directly test the StringModuleImpl.onStringFormat with incompatible types - // This simulates what happens when Scala f-interpolator passes wrong types at runtime - final pattern = 'User: %s and Balance: %f' - final params = ['admin', 'not-a-number'] as Object[] - iastModule.onStringFormat(pattern, params, _) >> { String fmt, Object[] args, String result -> - // Call the real implementation - def ctx = mock(datadog.trace.api.iast.IastContext) - def taintedObjects = mock(com.datadog.iast.taint.TaintedObjects) - ctx.getTaintedObjects() >> taintedObjects - datadog.trace.api.iast.IastContext.Provider.get() >> ctx - } - - then: - // Test should not throw IllegalFormatConversionException - // The fix should handle it gracefully - notThrown(IllegalFormatConversionException) - } - - void 'test string format with multiple incompatible types'() { - setup: - final iastModule = Mock(StringModule) - InstrumentationBridge.registerIastModule(iastModule) - - when: - // Test with multiple type mismatches - final pattern = 'Name: %s, Age: %d, Score: %f' - final params = ['John', 'thirty', 'high'] as Object[] - iastModule.onStringFormat(pattern, params, _) >> { String fmt, Object[] args, String result -> - // Call the real implementation - def ctx = mock(datadog.trace.api.iast.IastContext) - def taintedObjects = mock(com.datadog.iast.taint.TaintedObjects) - ctx.getTaintedObjects() >> taintedObjects - datadog.trace.api.iast.IastContext.Provider.get() >> ctx - } - - then: - // Test should not throw IllegalFormatConversionException - notThrown(IllegalFormatConversionException) - } } From a7900ad5f75f3b5ab100753fec7300aecc221231 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 14 Nov 2025 09:38:30 +0100 Subject: [PATCH 6/6] review --- .../scala/StringOpsCallSite.java | 52 +++---------------- .../IastUnwrapScakaNumberSmokeTest.groovy | 28 +--------- 2 files changed, 7 insertions(+), 73 deletions(-) diff --git a/dd-java-agent/instrumentation/scala/src/main/java/datadog/trace/instrumentation/scala/StringOpsCallSite.java b/dd-java-agent/instrumentation/scala/src/main/java/datadog/trace/instrumentation/scala/StringOpsCallSite.java index 414c1640bad..41885b8f6b5 100644 --- a/dd-java-agent/instrumentation/scala/src/main/java/datadog/trace/instrumentation/scala/StringOpsCallSite.java +++ b/dd-java-agent/instrumentation/scala/src/main/java/datadog/trace/instrumentation/scala/StringOpsCallSite.java @@ -6,7 +6,6 @@ import datadog.trace.api.iast.Propagation; import datadog.trace.api.iast.propagation.StringModule; import javax.annotation.Nonnull; -import javax.annotation.Nullable; import scala.collection.immutable.StringOps; import scala.math.ScalaNumber; @@ -87,55 +86,16 @@ public static String afterInterpolation( * scala.math.BigDecimal/BigInt, preventing IllegalFormatConversionException while maintaining * correct format results and taint tracking. * - * @param args the array of arguments to unwrap - * @return a new array with ScalaNumber instances unwrapped, or the original array if no changes + * @param args the array of arguments to unwrap (modified in-place) + * @return the same array with ScalaNumber instances unwrapped to their Java equivalents */ @Nonnull private static Object[] unwrapScalaNumbers(@Nonnull final Object[] args) { - boolean needsUnwrap = false; - // First pass: check if any argument needs unwrapping - for (final Object arg : args) { - if (arg != null && isScalaNumber(arg)) { - needsUnwrap = true; - break; - } - } - // If no unwrapping needed, return original array - if (!needsUnwrap) { - return args; - } - // Second pass: create new array with unwrapped values - final Object[] unwrapped = new Object[args.length]; for (int i = 0; i < args.length; i++) { - unwrapped[i] = unwrapScalaNumber(args[i]); - } - return unwrapped; - } - - /** - * Checks if an object is a Scala ScalaNumber (BigDecimal or BigInt). - * - * @param arg the object to check - * @return true if the object is a ScalaNumber instance - */ - private static boolean isScalaNumber(@Nonnull final Object arg) { - return arg instanceof scala.math.ScalaNumber; - } - - /** - * Unwraps a single Scala ScalaNumber to its underlying Java representation. - * - * @param arg the argument to unwrap (can be null) - * @return the unwrapped value if ScalaNumber, otherwise the original value - */ - @Nullable - private static Object unwrapScalaNumber(@Nullable final Object arg) { - if (arg == null) { - return null; - } - if (arg instanceof scala.math.ScalaNumber) { - return ((ScalaNumber) arg).underlying(); + if (args[i] instanceof scala.math.ScalaNumber) { + args[i] = ((ScalaNumber) args[i]).underlying(); + } } - return arg; + return args; } } diff --git a/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy index 5beb69294d4..014dd526cf6 100644 --- a/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy +++ b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy @@ -11,25 +11,11 @@ import okhttp3.Request * Scala ScalaNumber types (BigDecimal, BigInt) to their underlying Java representations * before passing them to IAST's onStringFormat. * - *

Background: - * Scala's String.format() internally calls unwrapArg() which converts: - * - scala.math.BigDecimal -> java.math.BigDecimal (via underlying() method) - * - scala.math.BigInt -> java.math.BigInteger (via underlying() method) - * - *

Problem: + *

Why: * IAST's @CallSite.After interceptor captures arguments AFTER Scala's runtime has already * called unwrapArg(), but our instrumentation receives the original Scala types. * Without unwrapping, passing scala.math.BigDecimal to String.format("%f") causes * IllegalFormatConversionException: f != scala.math.BigDecimal - * - *

Solution: - * StringOpsCallSite now calls unwrapScalaNumbers() before passing arguments to onStringFormat, - * ensuring java.math types are used instead of scala.math types. - * - *

This test verifies: - * 1. No IllegalFormatConversionException is thrown (200 response, not 500) - * 2. No error telemetry is logged (checked via isErrorLog override) - * 3. Correct formatted output is returned */ class IastUnwrapScakaNumberSmokeTest extends AbstractIastServerSmokeTest { @@ -71,19 +57,7 @@ class IastUnwrapScakaNumberSmokeTest extends AbstractIastServerSmokeTest { then: def responseBodyStr = response.body().string() - // Verify no IllegalFormatConversionException occurred - // Without unwrapScalaNumber: scala.math.BigDecimal -> String.format("%f") -> exception -> 500 - // With unwrapScalaNumber: java.math.BigDecimal -> String.format("%f") -> success -> 200 response.code() == 200 responseBodyStr == "Value: 123.456000" - - // Note: Taint propagation is NOT expected in this scenario because: - // 1. The input String "123.456" is tainted (http.request.parameter) - // 2. BigDecimal(param) creates a numeric object - IAST does not taint non-String objects - // 3. format() converts back to String, but the BigDecimal was never tainted, so no taint to propagate - // - // This test's purpose is to verify that unwrapScalaNumber prevents IllegalFormatConversionException, - // not to verify taint propagation through String -> BigDecimal -> String conversions. - // Error detection happens automatically via isErrorLog() override + assertNoErrorLogs() in cleanupSpec(). } }