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..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 @@ -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,30 @@ private static boolean checkParameterBounds( return false; } + /** + * 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 + */ + 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) { + // 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()); + throw e; + } + } + @Override public void onStringFormat( @Nonnull final Iterable literals, 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..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 @@ -7,6 +7,7 @@ import datadog.trace.api.iast.propagation.StringModule; import javax.annotation.Nonnull; import scala.collection.immutable.StringOps; +import scala.math.ScalaNumber; @Propagation @CallSite( @@ -28,7 +29,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 +51,51 @@ 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: + * + *

+ * + *

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 (modified in-place) + * @return the same array with ScalaNumber instances unwrapped to their Java equivalents + */ + @Nonnull + private static Object[] unwrapScalaNumbers(@Nonnull final Object[] args) { + for (int i = 0; i < args.length; i++) { + if (args[i] instanceof scala.math.ScalaNumber) { + args[i] = ((ScalaNumber) args[i]).underlying(); + } + } + return args; + } } 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..76b539d0e53 100644 --- a/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala +++ b/dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala @@ -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/IastUnwrapScakaNumberSmokeTest.groovy b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy new file mode 100644 index 00000000000..014dd526cf6 --- /dev/null +++ b/dd-smoke-tests/iast-propagation/src/test/groovy/datadog/smoketest/IastUnwrapScakaNumberSmokeTest.groovy @@ -0,0 +1,63 @@ +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. + * + *

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 + */ +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() + response.code() == 200 + responseBodyStr == "Value: 123.456000" + } +}