Add server.request.body.files_content AppSec address for Tomcat and Netty multipart

dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy

@@ -136,6 +136,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     ss.registerCallback(events.requestBodyDone(), callbacks.requestBodyEndCb)
     ss.registerCallback(events.requestBodyProcessed(), callbacks.requestBodyObjectCb)
     ss.registerCallback(events.requestFilesFilenames(), callbacks.requestFilesFilenamesCb)
+    ss.registerCallback(events.requestFilesContent(), callbacks.requestFilesContentCb)
     ss.registerCallback(events.responseBody(), callbacks.responseBodyObjectCb)
     ss.registerCallback(events.responseStarted(), callbacks.responseStartedCb)
     ss.registerCallback(events.responseHeader(), callbacks.responseHeaderCb)
@@ -372,6 +373,10 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     false
   }
 
+  boolean testBodyFilesContent() {
+    false
+  }
+
   boolean testBodyJson() {
     false
   }
@@ -1652,6 +1657,82 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     response.close()
   }
 
+  def 'test instrumentation gateway file upload content'() {
+    setup:
+    assumeTrue(testBodyFilesContent())
+    RequestBody fileBody = RequestBody.create(MediaType.parse('application/octet-stream'), 'file content')
+    def body = new MultipartBody.Builder()
+    .setType(MultipartBody.FORM)
+    .addFormDataPart('file', 'test.bin', fileBody)
+    .build()
+    def httpRequest = request(BODY_MULTIPART, 'POST', body).build()
+    def response = client.newCall(httpRequest).execute()
+
+    when:
+    TEST_WRITER.waitForTraces(1)
+
+    then:
+    TEST_WRITER.get(0).any {
+      it.getTag('request.body.files_content') == '[file content]'
+    }
+
+    cleanup:
+    response.close()
+  }
+
+  def 'test instrumentation gateway file upload content truncated at max size'() {
+    setup:
+    assumeTrue(testBodyFilesContent())
+    def maxContentBytes = Config.get().getAppSecMaxFileContentBytes()
+    def body = new MultipartBody.Builder()
+    .setType(MultipartBody.FORM)
+    .addFormDataPart('file', 'large.bin',
+    RequestBody.create(MediaType.parse('application/octet-stream'), 'X' * (maxContentBytes + 500)))
+    .build()
+    def httpRequest = request(BODY_MULTIPART, 'POST', body).build()
+    def response = client.newCall(httpRequest).execute()
+
+    when:
+    TEST_WRITER.waitForTraces(1)
+
+    then:
+    TEST_WRITER.get(0).any {
+      span ->
+      span.getTag('request.body.files_content') == '[' + 'X' * maxContentBytes + ']'
+    }
+
+    cleanup:
+    response.close()
+  }
+
+  def 'test instrumentation gateway file upload content max files limit'() {
+    setup:
+    assumeTrue(testBodyFilesContent())
+    def maxFilesToInspect = Config.get().getAppSecMaxFileContentCount()
+    def bodyBuilder = new MultipartBody.Builder().setType(MultipartBody.FORM)
+    (1..maxFilesToInspect + 1).each {
+      i ->
+      bodyBuilder.addFormDataPart("file$i", "file${i}.bin",
+      RequestBody.create(MediaType.parse('application/octet-stream'), "content_of_file_$i"))
+    }
+    def httpRequest = request(BODY_MULTIPART, 'POST', bodyBuilder.build()).build()
+    def response = client.newCall(httpRequest).execute()
+
+    when:
+    TEST_WRITER.waitForTraces(1)
+
+    then:
+    TEST_WRITER.get(0).any {
+      span ->
+      def tag = span.getTag('request.body.files_content') as String
+      tag?.contains("content_of_file_$maxFilesToInspect") &&
+      !tag.contains("content_of_file_${maxFilesToInspect + 1}")
+    }
+
+    cleanup:
+    response.close()
+  }
+
   def 'test instrumentation gateway json request body'() {
     setup:
     assumeTrue(testBodyJson())
@@ -2589,6 +2670,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
       boolean responseBodyTag
       Object responseBody
       List<String> uploadedFilenames
+      List<String> uploadedFilesContent
     }
 
     static final String stringOrEmpty(String string) {
@@ -2765,6 +2847,15 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
       Flow.ResultFlow.empty()
     } as BiFunction<RequestContext, List<String>, Flow<Void>>)
 
+    final BiFunction<RequestContext, List<String>, Flow<Void>> requestFilesContentCb =
+    ({
+      RequestContext rqCtxt, List<String> contents ->
+      rqCtxt.traceSegment.setTagTop('request.body.files_content', contents as String)
+      Context context = rqCtxt.getData(RequestContextSlot.APPSEC)
+      context.uploadedFilesContent = contents
+      Flow.ResultFlow.empty()
+    } as BiFunction<RequestContext, List<String>, Flow<Void>>)
+
     final BiFunction<RequestContext, Object, Flow<Void>> responseBodyObjectCb =
     ({
       RequestContext rqCtxt, Object obj ->

dd-java-agent/instrumentation/commons-fileupload-1.5/src/main/java/datadog/trace/instrumentation/commons/fileupload/CommonsFileUploadAppSecInstrumentation.java

@@ -26,7 +26,6 @@
 @AutoService(InstrumenterModule.class)
 public class CommonsFileUploadAppSecInstrumentation extends InstrumenterModule.AppSec
     implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice {
-
   public CommonsFileUploadAppSecInstrumentation() {
     super("commons-fileupload");
   }
@@ -54,7 +53,6 @@ public void methodAdvice(MethodTransformer transformer) {
 
   @RequiresRequestContext(RequestContextSlot.APPSEC)
   public static class ParseRequestAdvice {
-
     @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
     static void after(
         @Advice.Return final List<FileItem> fileItems,
@@ -73,22 +71,22 @@ static void after(
         return;
       }
 
-      List<String> filenames = new ArrayList<>();
+      List<String> filenames = filenamesCallback != null ? new ArrayList<>() : null;
+      List<String> filesContent = contentCallback != null ? new ArrayList<>() : null;
       for (FileItem fileItem : fileItems) {
         if (fileItem.isFormField()) {
           continue;
         }
         String name = fileItem.getName();
-        if (name != null && !name.isEmpty()) {
+        if (filenames != null && name != null && !name.isEmpty()) {
           filenames.add(name);
         }
-      }
-      if (filenames.isEmpty() && contentCallback == null) {
-        return;
+        if (filesContent != null) {
+          FileItemContentReader.addToContents(fileItem, filesContent);
+        }
       }
 
-      // Fire filenames event
-      if (filenamesCallback != null && !filenames.isEmpty()) {
+      if (filenames != null && !filenames.isEmpty()) {
         Flow<Void> flow = filenamesCallback.apply(reqCtx, filenames);
         Flow.Action action = flow.getAction();
         if (action instanceof Flow.Action.RequestBlockingAction) {
@@ -98,28 +96,21 @@ static void after(
             brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba);
             t = new BlockingException("Blocked request (multipart file upload)");
             reqCtx.getTraceSegment().effectivelyBlocked();
-            return;
           }
         }
       }
 
-      // Fire content event only if not blocked
-      if (contentCallback == null) {
-        return;
-      }
-      List<String> filesContent = FileItemContentReader.readContents(fileItems);
-      if (filesContent.isEmpty()) {
-        return;
-      }
-      Flow<Void> contentFlow = contentCallback.apply(reqCtx, filesContent);
-      Flow.Action contentAction = contentFlow.getAction();
-      if (contentAction instanceof Flow.Action.RequestBlockingAction) {
-        Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) contentAction;
-        BlockResponseFunction brf = reqCtx.getBlockResponseFunction();
-        if (brf != null) {
-          brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba);
-          t = new BlockingException("Blocked request (multipart file upload content)");
-          reqCtx.getTraceSegment().effectivelyBlocked();
+      if (t == null && filesContent != null && !filesContent.isEmpty()) {
+        Flow<Void> contentFlow = contentCallback.apply(reqCtx, filesContent);
+        Flow.Action contentAction = contentFlow.getAction();
+        if (contentAction instanceof Flow.Action.RequestBlockingAction) {
+          Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) contentAction;
+          BlockResponseFunction brf = reqCtx.getBlockResponseFunction();
+          if (brf != null) {
+            brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba);
+            t = new BlockingException("Blocked request (multipart file upload content)");
+            reqCtx.getTraceSegment().effectivelyBlocked();
+          }
         }
       }
     }

dd-java-agent/instrumentation/commons-fileupload-1.5/src/main/java/datadog/trace/instrumentation/commons/fileupload/FileItemContentReader.java

@@ -1,45 +1,31 @@
 package datadog.trace.instrumentation.commons.fileupload;
 
+import datadog.trace.api.Config;
 import datadog.trace.api.http.MultipartContentDecoder;
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.ArrayList;
 import java.util.List;
 import org.apache.commons.fileupload.FileItem;
 
 /** Reads uploaded file content for WAF inspection. */
 public final class FileItemContentReader {
-  public static final int MAX_CONTENT_BYTES = 4096;
-  public static final int MAX_FILES_TO_INSPECT = 25;
-
-  public static List<String> readContents(List<FileItem> fileItems) {
-    List<String> result = new ArrayList<>();
-    for (FileItem fileItem : fileItems) {
-      if (result.size() >= MAX_FILES_TO_INSPECT) {
-        break;
-      }
-      if (fileItem.isFormField()) {
-        continue;
-      }
-      result.add(readContent(fileItem));
-    }
-    return result;
-  }
+  public static final int MAX_CONTENT_BYTES = Config.get().getAppSecMaxFileContentBytes();
+  public static final int MAX_FILES_TO_INSPECT = Config.get().getAppSecMaxFileContentCount();
 
   public static String readContent(FileItem fileItem) {
     try (InputStream is = fileItem.getInputStream()) {
-      byte[] buf = new byte[MAX_CONTENT_BYTES];
-      int total = 0;
-      int n;
-      while (total < MAX_CONTENT_BYTES
-          && (n = is.read(buf, total, MAX_CONTENT_BYTES - total)) != -1) {
-        total += n;
-      }
-      return MultipartContentDecoder.decodeBytes(buf, total, fileItem.getContentType());
+      return MultipartContentDecoder.readInputStream(
+          is, MAX_CONTENT_BYTES, fileItem.getContentType());
     } catch (IOException ignored) {
       return "";
     }
   }
 
+  public static void addToContents(FileItem fileItem, List<String> contents) {
+    if (contents.size() < MAX_FILES_TO_INSPECT) {
+      contents.add(readContent(fileItem));
+    }
+  }
+
   private FileItemContentReader() {}
 }

dd-java-agent/instrumentation/commons-fileupload-1.5/src/test/groovy/FileItemContentReaderTest.groovy

@@ -48,61 +48,41 @@ class FileItemContentReaderTest extends Specification {
     FileItemContentReader.readContent(item) == text
   }
 
-  void 'readContents returns content for each non-form file with a name'() {
+  void 'addToContents adds content when below MAX_FILES_TO_INSPECT'() {
     given:
-    def items = [fileItem('content-a', 'file-a.txt'), fileItem('content-b', 'file-b.txt'),]
+    def contents = []
+    def item = fileItem('hello')
 
     when:
-    def result = FileItemContentReader.readContents(items)
+    FileItemContentReader.addToContents(item, contents)
 
     then:
-    result == ['content-a', 'content-b']
+    contents == ['hello']
   }
 
-  void 'readContents skips form fields'() {
+  void 'addToContents does not add when at MAX_FILES_TO_INSPECT limit'() {
     given:
-    FileItem formField = Stub(FileItem)
-    formField.isFormField() >> true
-    def items = [formField, fileItem('content', 'real.txt')]
+    def contents = (1..FileItemContentReader.MAX_FILES_TO_INSPECT).collect { "content-${it}" }
+    def item = fileItem('extra')
 
     when:
-    def result = FileItemContentReader.readContents(items)
+    FileItemContentReader.addToContents(item, contents)
 
     then:
-    result == ['content']
+    contents.size() == FileItemContentReader.MAX_FILES_TO_INSPECT
   }
 
-  void 'readContents includes file parts with empty or null name'() {
+  void 'addToContents fills up to exactly MAX_FILES_TO_INSPECT'() {
     given:
-    def items = [
-      fileItem('content-no-name', null),
-      fileItem('content-empty-name', ''),
-      fileItem('content-named', 'named.txt'),
-    ]
+    def contents = (1..<FileItemContentReader.MAX_FILES_TO_INSPECT).collect { "content-${it}" }
+    def item = fileItem('last')
 
     when:
-    def result = FileItemContentReader.readContents(items)
+    FileItemContentReader.addToContents(item, contents)
 
     then:
-    result == ['content-no-name', 'content-empty-name', 'content-named']
-  }
-
-  void 'readContents stops after MAX_FILES_TO_INSPECT files'() {
-    given:
-    def items = (1..FileItemContentReader.MAX_FILES_TO_INSPECT + 1).collect {
-      fileItem("content-${it}", "file-${it}.txt")
-    }
-
-    when:
-    def result = FileItemContentReader.readContents(items)
-
-    then:
-    result.size() == FileItemContentReader.MAX_FILES_TO_INSPECT
-  }
-
-  void 'readContents returns empty list for empty input'() {
-    expect:
-    FileItemContentReader.readContents([]) == []
+    contents.size() == FileItemContentReader.MAX_FILES_TO_INSPECT
+    contents.last() == 'last'
   }
 
   private FileItem fileItem(String content) {