diff --git a/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy index ac7e2fd17b7..92f07ca6a8f 100644 --- a/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -136,6 +136,7 @@ abstract class HttpServerTest extends WithHttpServer { 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 extends WithHttpServer { false } + boolean testBodyFilesContent() { + false + } + boolean testBodyJson() { false } @@ -1652,6 +1657,82 @@ abstract class HttpServerTest extends WithHttpServer { 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 extends WithHttpServer { boolean responseBodyTag Object responseBody List uploadedFilenames + List uploadedFilesContent } static final String stringOrEmpty(String string) { @@ -2765,6 +2847,15 @@ abstract class HttpServerTest extends WithHttpServer { Flow.ResultFlow.empty() } as BiFunction, Flow>) + final BiFunction, Flow> requestFilesContentCb = + ({ + RequestContext rqCtxt, List 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, Flow>) + final BiFunction> responseBodyObjectCb = ({ RequestContext rqCtxt, Object obj -> diff --git a/dd-java-agent/instrumentation/commons-fileupload-1.5/src/main/java/datadog/trace/instrumentation/commons/fileupload/CommonsFileUploadAppSecInstrumentation.java b/dd-java-agent/instrumentation/commons-fileupload-1.5/src/main/java/datadog/trace/instrumentation/commons/fileupload/CommonsFileUploadAppSecInstrumentation.java index 8d2b12f7e95..f5b37d0ee02 100644 --- a/dd-java-agent/instrumentation/commons-fileupload-1.5/src/main/java/datadog/trace/instrumentation/commons/fileupload/CommonsFileUploadAppSecInstrumentation.java +++ b/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 fileItems, @@ -73,22 +71,22 @@ static void after( return; } - List filenames = new ArrayList<>(); + List filenames = filenamesCallback != null ? new ArrayList<>() : null; + List 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 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 filesContent = FileItemContentReader.readContents(fileItems); - if (filesContent.isEmpty()) { - return; - } - Flow 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 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(); + } } } } diff --git a/dd-java-agent/instrumentation/commons-fileupload-1.5/src/main/java/datadog/trace/instrumentation/commons/fileupload/FileItemContentReader.java b/dd-java-agent/instrumentation/commons-fileupload-1.5/src/main/java/datadog/trace/instrumentation/commons/fileupload/FileItemContentReader.java index 1686f66de61..58ade1d656d 100644 --- a/dd-java-agent/instrumentation/commons-fileupload-1.5/src/main/java/datadog/trace/instrumentation/commons/fileupload/FileItemContentReader.java +++ b/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 readContents(List fileItems) { - List 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 contents) { + if (contents.size() < MAX_FILES_TO_INSPECT) { + contents.add(readContent(fileItem)); + } + } + private FileItemContentReader() {} } diff --git a/dd-java-agent/instrumentation/commons-fileupload-1.5/src/test/groovy/FileItemContentReaderTest.groovy b/dd-java-agent/instrumentation/commons-fileupload-1.5/src/test/groovy/FileItemContentReaderTest.groovy index 4940c46bcbf..c4651b55827 100644 --- a/dd-java-agent/instrumentation/commons-fileupload-1.5/src/test/groovy/FileItemContentReaderTest.groovy +++ b/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..> callback = cbp.getCallback(EVENTS.requestBodyProcessed()); - if (callback == null) { + BiFunction, Flow> filenamesCb = + cbp.getCallback(EVENTS.requestFilesFilenames()); + + BiFunction, Flow> contentCb = + cbp.getCallback(EVENTS.requestFilesContent()); + + if (callback == null && filenamesCb == null && contentCb == null) { return; } - RuntimeException exc = null; - - Map> attributes = new LinkedHashMap<>(); - List filenames = new ArrayList<>(); - for (InterfaceHttpData data : thiz.getBodyHttpDatas()) { - if (data.getHttpDataType() == InterfaceHttpData.HttpDataType.Attribute) { - String name = data.getName(); - List values = attributes.get(name); - if (values == null) { - attributes.put(name, values = new ArrayList<>(1)); - } - - try { - values.add(((Attribute) data).getValue()); - } catch (IOException e) { - exc = new UndeclaredThrowableException(e); - } - } else if (data.getHttpDataType() == InterfaceHttpData.HttpDataType.FileUpload) { - String filename = ((FileUpload) data).getFilename(); - if (filename != null && !filename.isEmpty()) { - filenames.add(filename); - } + Map> attributes = callback != null ? new LinkedHashMap<>() : null; + List filenames = filenamesCb != null ? new ArrayList<>() : null; + List filesContent = contentCb != null ? new ArrayList<>() : null; + + RuntimeException exc = + NettyMultipartHelper.collectBodyData( + thiz.getBodyHttpDatas(), attributes, filenames, filesContent); + + if (callback != null) { + // effectivelyBlocked() is intentionally absent: tryCommitBlockingResponse finishes + // the span synchronously in this Netty path; calling it on a finished span throws. + Throwable block = + NettyMultipartHelper.tryBlock( + requestContext, + callback.apply(requestContext, attributes), + "Blocked request (multipart/urlencoded post data)"); + if (block != null) { + thr = block; } } - Flow flow = callback.apply(requestContext, attributes); - Flow.Action action = flow.getAction(); - if (action instanceof Flow.Action.RequestBlockingAction) { - Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; - BlockResponseFunction brf = requestContext.getBlockResponseFunction(); - if (brf != null) { - brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); + if (filenames != null && !filenames.isEmpty()) { + Flow filenamesFlow = filenamesCb.apply(requestContext, filenames); + if (thr == null) { + thr = + NettyMultipartHelper.tryBlock( + requestContext, filenamesFlow, "Blocked request (multipart file upload)"); } - thr = new BlockingException("Blocked request (multipart/urlencoded post data)"); } - if (!filenames.isEmpty()) { - BiFunction, Flow> filenamesCb = - cbp.getCallback(EVENTS.requestFilesFilenames()); - if (filenamesCb != null) { - Flow filenamesFlow = filenamesCb.apply(requestContext, filenames); - Flow.Action filenamesAction = filenamesFlow.getAction(); - if (thr == null && filenamesAction instanceof Flow.Action.RequestBlockingAction) { - Flow.Action.RequestBlockingAction rba = - (Flow.Action.RequestBlockingAction) filenamesAction; - BlockResponseFunction brf = requestContext.getBlockResponseFunction(); - if (brf != null) { - brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba); - } - thr = new BlockingException("Blocked request (multipart file upload)"); - } - } + if (thr == null && filesContent != null && !filesContent.isEmpty()) { + thr = + NettyMultipartHelper.tryBlock( + requestContext, + contentCb.apply(requestContext, filesContent), + "Blocked request (multipart file upload content)"); } if (exc != null) { diff --git a/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyMultipartHelper.java b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyMultipartHelper.java new file mode 100644 index 00000000000..b268216f2aa --- /dev/null +++ b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyMultipartHelper.java @@ -0,0 +1,100 @@ +package datadog.trace.instrumentation.netty41; + +import datadog.appsec.api.blocking.BlockingException; +import datadog.trace.api.Config; +import datadog.trace.api.gateway.BlockResponseFunction; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.http.MultipartContentDecoder; +import io.netty.buffer.ByteBuf; +import io.netty.handler.codec.http.multipart.Attribute; +import io.netty.handler.codec.http.multipart.FileUpload; +import io.netty.handler.codec.http.multipart.InterfaceHttpData; +import java.io.FileInputStream; +import java.io.IOException; +import java.lang.reflect.UndeclaredThrowableException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +public final class NettyMultipartHelper { + public static final int MAX_CONTENT_BYTES = Config.get().getAppSecMaxFileContentBytes(); + public static final int MAX_FILES_TO_INSPECT = Config.get().getAppSecMaxFileContentCount(); + + /** + * Iterates multipart body parts populating the provided output collections. Pass {@code null} for + * any collection to skip that category. Returns any {@link IOException} encountered reading an + * attribute value, wrapped as {@link UndeclaredThrowableException}, or {@code null} if none. + */ + public static RuntimeException collectBodyData( + List parts, + Map> attributes, + List filenames, + List filesContent) { + RuntimeException exc = null; + for (InterfaceHttpData data : parts) { + if (attributes != null + && data.getHttpDataType() == InterfaceHttpData.HttpDataType.Attribute) { + String name = data.getName(); + List values = attributes.get(name); + if (values == null) { + attributes.put(name, values = new ArrayList<>(1)); + } + try { + values.add(((Attribute) data).getValue()); + } catch (IOException e) { + exc = new UndeclaredThrowableException(e); + } + } else if (data.getHttpDataType() == InterfaceHttpData.HttpDataType.FileUpload) { + FileUpload fileUpload = (FileUpload) data; + String filename = fileUpload.getFilename(); + if (filenames != null && filename != null && !filename.isEmpty()) { + filenames.add(filename); + } + if (filesContent != null && filesContent.size() < MAX_FILES_TO_INSPECT) { + filesContent.add(readContent(fileUpload)); + } + } + } + return exc; + } + + public static String readContent(FileUpload fileUpload) { + try { + if (fileUpload.isInMemory()) { + ByteBuf buf = fileUpload.getByteBuf(); + int length = Math.min(MAX_CONTENT_BYTES, buf.readableBytes()); + byte[] bytes = new byte[length]; + buf.getBytes(buf.readerIndex(), bytes); + return MultipartContentDecoder.decodeBytes(bytes, length, fileUpload.getContentType()); + } else { + try (FileInputStream fis = new FileInputStream(fileUpload.getFile())) { + return MultipartContentDecoder.readInputStream( + fis, MAX_CONTENT_BYTES, fileUpload.getContentType()); + } + } + } catch (Exception ignored) { + return ""; + } + } + + /** + * Checks if the flow action is a blocking action and, if so, commits the blocking response. + * Returns a {@link BlockingException} to be re-thrown by the advice, or {@code null} if no + * blocking action was taken. + */ + public static BlockingException tryBlock(RequestContext ctx, Flow flow, String message) { + Flow.Action action = flow.getAction(); + if (action instanceof Flow.Action.RequestBlockingAction) { + Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action; + BlockResponseFunction brf = ctx.getBlockResponseFunction(); + if (brf != null) { + brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba); + return new BlockingException(message); + } + } + return null; + } + + private NettyMultipartHelper() {} +} diff --git a/dd-java-agent/instrumentation/netty/netty-4.1/src/test/groovy/Netty41ServerTest.groovy b/dd-java-agent/instrumentation/netty/netty-4.1/src/test/groovy/Netty41ServerTest.groovy index c9b37933033..1c4831d0ce5 100644 --- a/dd-java-agent/instrumentation/netty/netty-4.1/src/test/groovy/Netty41ServerTest.groovy +++ b/dd-java-agent/instrumentation/netty/netty-4.1/src/test/groovy/Netty41ServerTest.groovy @@ -27,6 +27,7 @@ import io.netty.handler.codec.http.HttpResponseStatus import io.netty.handler.codec.http.HttpServerCodec import io.netty.handler.codec.http.multipart.Attribute import io.netty.handler.codec.http.multipart.HttpPostRequestDecoder +import io.netty.handler.codec.http.multipart.InterfaceHttpData import io.netty.handler.codec.http.websocketx.BinaryWebSocketFrame import io.netty.handler.codec.http.websocketx.CloseWebSocketFrame import io.netty.handler.codec.http.websocketx.ContinuationWebSocketFrame @@ -36,6 +37,7 @@ import io.netty.handler.logging.LogLevel import io.netty.handler.logging.LoggingHandler import io.netty.util.CharsetUtil +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION @@ -73,7 +75,7 @@ abstract class Netty41ServerTest extends HttpServerTest { ChannelPipeline pipeline = ch.pipeline() pipeline.addFirst("logger", LOGGING_HANDLER) pipeline.addLast(new HttpServerCodec()) - pipeline.addLast(new HttpObjectAggregator(1024)) + pipeline.addLast(new HttpObjectAggregator(1024 * 1024)) pipeline.addLast(new WebSocketServerProtocolHandler("/websocket")) pipeline.addLast([ channelRead0 : { ChannelHandlerContext ctx, msg -> @@ -133,6 +135,31 @@ abstract class Netty41ServerTest extends HttpServerTest { response = new DefaultFullHttpResponse(HTTP_1_1, HttpResponseStatus.valueOf(endpoint.status), content) } break + case BODY_MULTIPART: + if (msg instanceof FullHttpRequest) { + HttpPostRequestDecoder decoder = new HttpPostRequestDecoder( + new HttpRequest() { + @Delegate + HttpRequest delegate = request + }) + + Map m + try { + decoder.offer(msg) + + m = decoder.bodyHttpDatas + .findAll { it.httpDataType == InterfaceHttpData.HttpDataType.Attribute } + .collectEntries { d -> [d.name, [((Attribute) d).value]] } + } finally { + try { + decoder.destroy() + } catch (Exception ignored) {} + } + + content = Unpooled.copiedBuffer(m as String, CharsetUtil.UTF_8) + response = new DefaultFullHttpResponse(HTTP_1_1, HttpResponseStatus.valueOf(endpoint.status), content) + } + break case REDIRECT: response = new DefaultFullHttpResponse(HTTP_1_1, HttpResponseStatus.valueOf(endpoint.status)) response.headers().set(HttpHeaderNames.LOCATION, endpoint.body) @@ -270,6 +297,21 @@ abstract class Netty41ServerTest extends HttpServerTest { true } + @Override + boolean testBodyMultipart() { + true + } + + @Override + boolean testBodyFilenames() { + true + } + + @Override + boolean testBodyFilesContent() { + true + } + @Override boolean testBlocking() { true diff --git a/dd-java-agent/instrumentation/netty/netty-4.1/src/test/groovy/NettyMultipartHelperTest.groovy b/dd-java-agent/instrumentation/netty/netty-4.1/src/test/groovy/NettyMultipartHelperTest.groovy new file mode 100644 index 00000000000..c7b5f388900 --- /dev/null +++ b/dd-java-agent/instrumentation/netty/netty-4.1/src/test/groovy/NettyMultipartHelperTest.groovy @@ -0,0 +1,482 @@ +import datadog.appsec.api.blocking.BlockingException +import datadog.trace.api.gateway.BlockResponseFunction +import datadog.trace.api.gateway.Flow +import datadog.trace.api.gateway.RequestContext +import datadog.trace.api.internal.TraceSegment +import datadog.trace.instrumentation.netty41.NettyMultipartHelper +import io.netty.buffer.Unpooled +import io.netty.handler.codec.http.multipart.Attribute +import io.netty.handler.codec.http.multipart.FileUpload +import io.netty.handler.codec.http.multipart.InterfaceHttpData +import spock.lang.Specification +import spock.lang.TempDir + +import java.lang.reflect.UndeclaredThrowableException +import java.nio.charset.StandardCharsets +import java.nio.file.Path + +class NettyMultipartHelperTest extends Specification { + + @TempDir + Path tempDir + + // ========================================================= + // readContent — in-memory path + // ========================================================= + + void 'readContent returns content from in-memory FileUpload'() { + given: + def upload = inMemoryUpload('hello world') + + expect: + NettyMultipartHelper.readContent(upload) == 'hello world' + } + + void 'readContent truncates in-memory content at MAX_CONTENT_BYTES'() { + given: + def upload = inMemoryUpload('X' * inputSize) + + when: + def result = NettyMultipartHelper.readContent(upload) + + then: + result.length() == expectedSize + + where: + inputSize | expectedSize + 13 | 13 + NettyMultipartHelper.MAX_CONTENT_BYTES - 1 | NettyMultipartHelper.MAX_CONTENT_BYTES - 1 + NettyMultipartHelper.MAX_CONTENT_BYTES | NettyMultipartHelper.MAX_CONTENT_BYTES + NettyMultipartHelper.MAX_CONTENT_BYTES + 500 | NettyMultipartHelper.MAX_CONTENT_BYTES + } + + void 'readContent returns empty string for empty in-memory content'() { + given: + def upload = inMemoryUpload('') + + expect: + NettyMultipartHelper.readContent(upload) == '' + } + + void 'readContent does not advance readerIndex of the underlying ByteBuf'() { + given: + def content = 'sensitive data' + def upload = inMemoryUpload(content) + def buf = upload.getByteBuf() + def indexBefore = buf.readerIndex() + + when: + NettyMultipartHelper.readContent(upload) + + then: + buf.readerIndex() == indexBefore + } + + // ========================================================= + // readContent — disk-backed path + // ========================================================= + + void 'readContent returns content from disk-backed FileUpload'() { + given: + def upload = diskBackedUpload('hello from disk') + + expect: + NettyMultipartHelper.readContent(upload) == 'hello from disk' + } + + void 'readContent truncates disk-backed content at MAX_CONTENT_BYTES'() { + given: + def upload = diskBackedUpload('Y' * inputSize) + + when: + def result = NettyMultipartHelper.readContent(upload) + + then: + result.length() == expectedSize + + where: + inputSize | expectedSize + 13 | 13 + NettyMultipartHelper.MAX_CONTENT_BYTES - 1 | NettyMultipartHelper.MAX_CONTENT_BYTES - 1 + NettyMultipartHelper.MAX_CONTENT_BYTES | NettyMultipartHelper.MAX_CONTENT_BYTES + NettyMultipartHelper.MAX_CONTENT_BYTES + 500 | NettyMultipartHelper.MAX_CONTENT_BYTES + } + + void 'readContent returns empty string for empty disk-backed file'() { + given: + def upload = diskBackedUpload('') + + expect: + NettyMultipartHelper.readContent(upload) == '' + } + + // ========================================================= + // readContent — charset decoding + // ========================================================= + + void 'readContent uses Content-Type charset for in-memory content'() { + given: + def text = 'héllo wörld' + def buf = Unpooled.copiedBuffer(text, StandardCharsets.UTF_8) + FileUpload upload = Stub(FileUpload) + upload.isInMemory() >> true + upload.getByteBuf() >> buf + upload.getContentType() >> 'text/plain; charset=UTF-8' + + expect: + NettyMultipartHelper.readContent(upload) == text + } + + void 'readContent uses Content-Type charset for disk-backed content'() { + given: + def text = 'héllo wörld' + def file = tempDir.resolve('upload.bin').toFile() + file.bytes = text.getBytes(StandardCharsets.UTF_8) + FileUpload upload = Stub(FileUpload) + upload.isInMemory() >> false + upload.getFile() >> file + upload.getContentType() >> 'text/plain; charset=UTF-8' + + expect: + NettyMultipartHelper.readContent(upload) == text + } + + // ========================================================= + // readContent — error handling + // ========================================================= + + void 'readContent returns empty string when getByteBuf throws'() { + given: + FileUpload upload = Stub(FileUpload) + upload.isInMemory() >> true + upload.getByteBuf() >> { throw new RuntimeException('simulated error') } + + expect: + NettyMultipartHelper.readContent(upload) == '' + } + + void 'readContent returns empty string when getFile throws'() { + given: + FileUpload upload = Stub(FileUpload) + upload.isInMemory() >> false + upload.getFile() >> { throw new IOException('simulated error') } + + expect: + NettyMultipartHelper.readContent(upload) == '' + } + + // ========================================================= + // collectBodyData — empty / null inputs + // ========================================================= + + void 'collectBodyData returns null and leaves collections empty when parts list is empty'() { + given: + def attributes = [:] + def filenames = [] + def filesContent = [] + + expect: + NettyMultipartHelper.collectBodyData([], attributes, filenames, filesContent) == null + attributes.isEmpty() + filenames.isEmpty() + filesContent.isEmpty() + } + + // ========================================================= + // collectBodyData — attribute handling + // ========================================================= + + void 'collectBodyData collects attribute value into map'() { + given: + def attr = attribute('key', 'value') + def attributes = [:] + + when: + NettyMultipartHelper.collectBodyData([attr], attributes, null, null) + + then: + attributes == [key: ['value']] + } + + void 'collectBodyData accumulates multiple values for the same attribute key'() { + given: + def parts = [attribute('k', 'v1'), attribute('k', 'v2')] + def attributes = [:] + + when: + NettyMultipartHelper.collectBodyData(parts, attributes, null, null) + + then: + attributes == [k: ['v1', 'v2']] + } + + void 'collectBodyData skips attribute parts when attributes map is null'() { + given: + def attr = attribute('key', 'value') + def filenames = [] + + when: + NettyMultipartHelper.collectBodyData([attr], null, filenames, null) + + then: + filenames.isEmpty() + } + + void 'collectBodyData wraps IOException from getValue as UndeclaredThrowableException'() { + given: + def cause = new IOException('disk error') + Attribute attr = Stub(Attribute) + attr.getHttpDataType() >> InterfaceHttpData.HttpDataType.Attribute + attr.getName() >> 'k' + attr.getValue() >> { throw cause } + def attributes = [:] + + when: + def exc = NettyMultipartHelper.collectBodyData([attr], attributes, null, null) + + then: + exc instanceof UndeclaredThrowableException + exc.cause.is(cause) + } + + void 'collectBodyData continues collecting after an IOException from getValue'() { + given: + Attribute failing = Stub(Attribute) + failing.getHttpDataType() >> InterfaceHttpData.HttpDataType.Attribute + failing.getName() >> 'bad' + failing.getValue() >> { throw new IOException('disk error') } + + def good = attribute('good', 'ok') + def attributes = [:] + + when: + NettyMultipartHelper.collectBodyData([failing, good], attributes, null, null) + + then: + attributes == [bad: [], good: ['ok']] + } + + // ========================================================= + // collectBodyData — filename handling + // ========================================================= + + void 'collectBodyData collects non-empty filename from file upload'() { + given: + def filenames = [] + + when: + NettyMultipartHelper.collectBodyData([fileUploadPart('report.pdf', '')], null, filenames, null) + + then: + filenames == ['report.pdf'] + } + + void 'collectBodyData skips empty filename from file upload'() { + given: + def filenames = [] + + when: + NettyMultipartHelper.collectBodyData([fileUploadPart('', '')], null, filenames, null) + + then: + filenames.isEmpty() + } + + void 'collectBodyData skips null filename from file upload'() { + given: + FileUpload upload = Stub(FileUpload) + upload.getHttpDataType() >> InterfaceHttpData.HttpDataType.FileUpload + upload.getFilename() >> null + upload.isInMemory() >> true + upload.getByteBuf() >> Unpooled.EMPTY_BUFFER + def filenames = [] + + when: + NettyMultipartHelper.collectBodyData([upload], null, filenames, null) + + then: + filenames.isEmpty() + } + + void 'collectBodyData skips filenames when filenames list is null'() { + given: + def filesContent = [] + + when: + NettyMultipartHelper.collectBodyData( + [fileUploadPart('report.pdf', 'data')], null, null, filesContent) + + then: + filesContent == ['data'] + } + + // ========================================================= + // collectBodyData — content handling + // ========================================================= + + void 'collectBodyData reads file content into filesContent list'() { + given: + def filesContent = [] + + when: + NettyMultipartHelper.collectBodyData( + [fileUploadPart('f.txt', 'hello')], null, null, filesContent) + + then: + filesContent == ['hello'] + } + + void 'collectBodyData skips content when filesContent list is null'() { + given: + def filenames = [] + + when: + NettyMultipartHelper.collectBodyData( + [fileUploadPart('f.txt', 'hello')], null, filenames, null) + + then: + filenames == ['f.txt'] + } + + void 'collectBodyData stops reading content after MAX_FILES_TO_INSPECT but keeps collecting filenames'() { + given: + def max = NettyMultipartHelper.MAX_FILES_TO_INSPECT + def parts = (1..max + 1).collect { i -> fileUploadPart("file${i}.txt", "content${i}") } + def filenames = [] + def filesContent = [] + + when: + NettyMultipartHelper.collectBodyData(parts, null, filenames, filesContent) + + then: + filesContent.size() == max + filenames.size() == max + 1 + } + + // ========================================================= + // collectBodyData — mixed parts + // ========================================================= + + void 'collectBodyData handles mixed attributes and file uploads in one pass'() { + given: + def parts = [ + attribute('field1', 'val1'), + fileUploadPart('upload.txt', 'file-body'), + attribute('field2', 'val2'), + ] + def attributes = [:] + def filenames = [] + def filesContent = [] + + when: + def exc = NettyMultipartHelper.collectBodyData(parts, attributes, filenames, filesContent) + + then: + exc == null + attributes == [field1: ['val1'], field2: ['val2']] + filenames == ['upload.txt'] + filesContent == ['file-body'] + } + + // ========================================================= + // tryBlock + // ========================================================= + + void 'tryBlock returns null when flow action is not a blocking action'() { + given: + Flow flow = Stub(Flow) + flow.getAction() >> Flow.Action.Noop.INSTANCE + RequestContext ctx = Stub(RequestContext) + + expect: + NettyMultipartHelper.tryBlock(ctx, flow, 'msg') == null + } + + void 'tryBlock returns BlockingException with provided message when brf commits response'() { + given: + def segment = Stub(TraceSegment) + def rba = Stub(Flow.Action.RequestBlockingAction) + Flow flow = Stub(Flow) + flow.getAction() >> rba + BlockResponseFunction brf = Stub(BlockResponseFunction) + RequestContext ctx = Stub(RequestContext) + ctx.getBlockResponseFunction() >> brf + ctx.getTraceSegment() >> segment + + when: + def result = NettyMultipartHelper.tryBlock(ctx, flow, 'blocked!') + + then: + result instanceof BlockingException + result.message == 'blocked!' + } + + void 'tryBlock calls tryCommitBlockingResponse on brf with segment and rba'() { + given: + def segment = Stub(TraceSegment) + def rba = Stub(Flow.Action.RequestBlockingAction) + Flow flow = Stub(Flow) + flow.getAction() >> rba + BlockResponseFunction brf = Mock(BlockResponseFunction) + RequestContext ctx = Stub(RequestContext) + ctx.getBlockResponseFunction() >> brf + ctx.getTraceSegment() >> segment + + when: + NettyMultipartHelper.tryBlock(ctx, flow, 'msg') + + then: + 1 * brf.tryCommitBlockingResponse(segment, rba) + } + + void 'tryBlock returns null when brf is null despite blocking action'() { + given: + def rba = Stub(Flow.Action.RequestBlockingAction) + Flow flow = Stub(Flow) + flow.getAction() >> rba + RequestContext ctx = Stub(RequestContext) + ctx.getBlockResponseFunction() >> null + + expect: + NettyMultipartHelper.tryBlock(ctx, flow, 'msg') == null + } + + // ========================================================= + // helpers + // ========================================================= + + private FileUpload inMemoryUpload(String content) { + def buf = Unpooled.copiedBuffer(content, StandardCharsets.ISO_8859_1) + FileUpload upload = Stub(FileUpload) + upload.isInMemory() >> true + upload.getByteBuf() >> buf + return upload + } + + private FileUpload diskBackedUpload(String content) { + def file = tempDir.resolve('upload.bin').toFile() + file.bytes = content.getBytes(StandardCharsets.ISO_8859_1) + FileUpload upload = Stub(FileUpload) + upload.isInMemory() >> false + upload.getFile() >> file + return upload + } + + private Attribute attribute(String name, String value) { + Attribute attr = Stub(Attribute) + attr.getHttpDataType() >> InterfaceHttpData.HttpDataType.Attribute + attr.getName() >> name + attr.getValue() >> value + return attr + } + + private FileUpload fileUploadPart(String filename, String content) { + def buf = Unpooled.copiedBuffer(content, StandardCharsets.ISO_8859_1) + FileUpload upload = Stub(FileUpload) + upload.getHttpDataType() >> InterfaceHttpData.HttpDataType.FileUpload + upload.getFilename() >> filename + upload.isInMemory() >> true + upload.getByteBuf() >> buf + return upload + } +} diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParameterCollector.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParameterCollector.java index 0c41f59eaba..faf095fb4f5 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParameterCollector.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParameterCollector.java @@ -1,5 +1,8 @@ package datadog.trace.instrumentation.tomcat7; +import datadog.trace.api.Config; +import datadog.trace.api.http.MultipartContentDecoder; +import java.io.InputStream; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collections; @@ -20,6 +23,8 @@ public interface ParameterCollector { List getFilenames(); + List getContents(); + class ParameterCollectorNoop implements ParameterCollector { public static final ParameterCollector INSTANCE = new ParameterCollectorNoop(); @@ -46,11 +51,25 @@ public void addPart(Object part) {} public List getFilenames() { return Collections.emptyList(); } + + @Override + public List getContents() { + return Collections.emptyList(); + } } class ParameterCollectorImpl implements ParameterCollector { + private static final int MAX_CONTENT_BYTES = Config.get().getAppSecMaxFileContentBytes(); + private static final int MAX_FILES_TO_INSPECT = Config.get().getAppSecMaxFileContentCount(); + + private final boolean inspectContent; private Map> map; private List filenames; + private List contents; + + public ParameterCollectorImpl(boolean inspectContent) { + this.inspectContent = inspectContent; + } public boolean isEmpty() { return map == null; @@ -91,12 +110,25 @@ public Map> getMap() { public void addPart(Object part) { try { String filename = getFilename(part); - if (filename != null && !filename.isEmpty()) { + // null means no filename parameter at all → form field, skip entirely. + // empty string means filename="" was sent → file upload without a name, still inspect. + if (filename == null) { + return; + } + if (!filename.isEmpty()) { if (filenames == null) { filenames = new ArrayList<>(); } filenames.add(filename); } + if (inspectContent) { + if (contents == null) { + contents = new ArrayList<>(); + } + if (contents.size() < MAX_FILES_TO_INSPECT) { + contents.add(readContent(part)); + } + } } catch (Throwable ignored) { } } @@ -106,20 +138,94 @@ public List getFilenames() { return filenames != null ? filenames : Collections.emptyList(); } - private static String getFilename(Object part) { - // Try getSubmittedFileName() first — Servlet 3.1+ / Tomcat 8+ (both javax and jakarta) + @Override + public List getContents() { + return contents != null ? contents : Collections.emptyList(); + } + + // Immutable snapshot of resolved methods for a single Part concrete class. + // Held in a single volatile reference so readers get a consistent view: + // reading methodCache once into a local variable and using that local + // guarantees all three Method objects belong to the same class, even if + // another thread concurrently replaces methodCache with an entry for a + // different class. + private static final class CachedMethods { + final Class partClass; + final Method getInputStream; + final Method getContentType; + // getSubmittedFileName (Servlet 3.1+) or getFilename (Tomcat 7); null if neither found + final Method getFilename; + + CachedMethods(Class c, Method is, Method ct, Method fn) { + partClass = c; + getInputStream = is; + getContentType = ct; + getFilename = fn; + } + } + + private static volatile CachedMethods methodCache; + + private static CachedMethods resolveAndCache(Class partClass) { + Method getInputStream = null; + Method getContentType = null; + Method getFilename = null; try { - Method m = part.getClass().getMethod("getSubmittedFileName"); - return (String) m.invoke(part); + getInputStream = partClass.getMethod("getInputStream"); } catch (Exception ignored) { } - // Fall back to getFilename() — Tomcat 7 ApplicationPart specific try { - Method m = part.getClass().getMethod("getFilename"); + getContentType = partClass.getMethod("getContentType"); + } catch (Exception ignored) { + } + try { + getFilename = partClass.getMethod("getSubmittedFileName"); + } catch (Exception ignored) { + } + if (getFilename == null) { + try { + getFilename = partClass.getMethod("getFilename"); + } catch (Exception ignored) { + } + } + CachedMethods cache = + new CachedMethods(partClass, getInputStream, getContentType, getFilename); + methodCache = cache; + return cache; + } + + private static CachedMethods getCachedMethods(Class partClass) { + CachedMethods cache = methodCache; + if (cache != null && cache.partClass == partClass) { + return cache; + } + return resolveAndCache(partClass); + } + + private static String readContent(Object part) { + try { + CachedMethods cache = getCachedMethods(part.getClass()); + String contentType = + cache.getContentType != null ? (String) cache.getContentType.invoke(part) : null; + try (InputStream is = (InputStream) cache.getInputStream.invoke(part)) { + return MultipartContentDecoder.readInputStream(is, MAX_CONTENT_BYTES, contentType); + } + } catch (Exception ignored) { + return ""; + } + } + + private static String getFilename(Object part) { + CachedMethods cache = getCachedMethods(part.getClass()); + Method m = cache.getFilename; + if (m == null) { + return null; + } + try { return (String) m.invoke(part); } catch (Exception ignored) { + return null; } - return null; } } } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java index c249db9839a..ad7417a380d 100644 --- a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/main/java/datadog/trace/instrumentation/tomcat7/ParsePartsInstrumentation.java @@ -59,6 +59,7 @@ public String[] helperClassNames() { "datadog.trace.instrumentation.tomcat7.ParameterCollector", "datadog.trace.instrumentation.tomcat7.ParameterCollector$ParameterCollectorNoop", "datadog.trace.instrumentation.tomcat7.ParameterCollector$ParameterCollectorImpl", + "datadog.trace.instrumentation.tomcat7.ParameterCollector$ParameterCollectorImpl$CachedMethods", }; } @@ -86,7 +87,12 @@ static void before( RequestContext requestContext = agentSpan.getRequestContext(); if (requestContext != null && requestContext.getData(RequestContextSlot.APPSEC) != null) { reqCtx = requestContext; - collector = new ParameterCollector.ParameterCollectorImpl(); + boolean inspectContent = + AgentTracer.get() + .getCallbackProvider(RequestContextSlot.APPSEC) + .getCallback(EVENTS.requestFilesContent()) + != null; + collector = new ParameterCollector.ParameterCollectorImpl(inspectContent); return; } } @@ -117,9 +123,8 @@ static void after( BlockResponseFunction blockResponseFunction = reqCtx.getBlockResponseFunction(); if (blockResponseFunction != null) { blockResponseFunction.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); - if (t == null) { - t = new BlockingException("Blocked request (for Request/parseParts)"); - } + t = new BlockingException("Blocked request (for Request/parseParts)"); + reqCtx.getTraceSegment().effectivelyBlocked(); } } } @@ -138,8 +143,31 @@ static void after( BlockResponseFunction brf = reqCtx.getBlockResponseFunction(); if (brf != null) { brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba); + t = new BlockingException("Blocked request (multipart file upload)"); + reqCtx.getTraceSegment().effectivelyBlocked(); + } + } + } + } + + if (t == null) { + List contents = collector.getContents(); + if (!contents.isEmpty()) { + BiFunction, Flow> contentCb = + cbp.getCallback(EVENTS.requestFilesContent()); + if (contentCb != null) { + Flow contentFlow = contentCb.apply(reqCtx, contents); + 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(); + } } - t = new BlockingException("Blocked request (multipart file upload)"); } } } diff --git a/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/groovy/datadog/trace/instrumentation/tomcat7/ParameterCollectorImplTest.groovy b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/groovy/datadog/trace/instrumentation/tomcat7/ParameterCollectorImplTest.groovy new file mode 100644 index 00000000000..378fe33d837 --- /dev/null +++ b/dd-java-agent/instrumentation/tomcat/tomcat-appsec/tomcat-appsec-7.0/src/test/groovy/datadog/trace/instrumentation/tomcat7/ParameterCollectorImplTest.groovy @@ -0,0 +1,326 @@ +package datadog.trace.instrumentation.tomcat7 + +import datadog.trace.api.Config +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.CountDownLatch +import spock.lang.Specification + +class ParameterCollectorImplTest extends Specification { + + void 'getContents returns empty list when no parts added'() { + expect: + new ParameterCollector.ParameterCollectorImpl(true).getContents().isEmpty() + } + + void 'addPart skips content when filename is null'() { + given: + def collector = new ParameterCollector.ParameterCollectorImpl(true) + + when: + collector.addPart(new TestPart(null, 'some body')) + + then: + collector.getContents().isEmpty() + collector.getFilenames().isEmpty() + } + + void 'addPart reads content but skips filename when filename is empty string'() { + given: + def collector = new ParameterCollector.ParameterCollectorImpl(true) + + when: + collector.addPart(new TestPart('', 'some body')) + + then: + collector.getContents() == ['some body'] + collector.getFilenames().isEmpty() + } + + void 'addPart reads content for file part with filename'() { + given: + def collector = new ParameterCollector.ParameterCollectorImpl(true) + + when: + collector.addPart(new TestPart('file.txt', 'hello world')) + + then: + collector.getContents() == ['hello world'] + collector.getFilenames() == ['file.txt'] + } + + void 'addPart reads content for multiple files'() { + given: + def collector = new ParameterCollector.ParameterCollectorImpl(true) + + when: + collector.addPart(new TestPart('a.txt', 'content-a')) + collector.addPart(new TestPart('b.txt', 'content-b')) + + then: + collector.getContents() == ['content-a', 'content-b'] + collector.getFilenames() == ['a.txt', 'b.txt'] + } + + void 'addPart truncates content at MAX_CONTENT_BYTES'() { + given: + def maxBytes = Config.get().getAppSecMaxFileContentBytes() + def collector = new ParameterCollector.ParameterCollectorImpl(true) + def longContent = 'X' * (maxBytes + 500) + + when: + collector.addPart(new TestPart('big.bin', longContent)) + + then: + collector.getContents()[0].length() == maxBytes + } + + void 'addPart reads exactly MAX_CONTENT_BYTES when content is exactly that size'() { + given: + def maxBytes = Config.get().getAppSecMaxFileContentBytes() + def collector = new ParameterCollector.ParameterCollectorImpl(true) + def content = 'Y' * maxBytes + + when: + collector.addPart(new TestPart('exact.bin', content)) + + then: + collector.getContents()[0].length() == maxBytes + } + + void 'addPart stops collecting content after MAX_FILES_TO_INSPECT files but still collects filenames'() { + given: + def maxFiles = Config.get().getAppSecMaxFileContentCount() + def collector = new ParameterCollector.ParameterCollectorImpl(true) + + when: + (1..maxFiles + 1).each { i -> collector.addPart(new TestPart("file${i}.txt", "content${i}")) } + + then: + collector.getContents().size() == maxFiles + collector.getFilenames().size() == maxFiles + 1 + } + + void 'addPart adds empty string when getInputStream throws'() { + given: + def collector = new ParameterCollector.ParameterCollectorImpl(true) + + when: + collector.addPart(new FailingPart('bad.txt')) + + then: + collector.getContents() == [''] + collector.getFilenames() == ['bad.txt'] + } + + void 'addPart preserves ISO-8859-1 bytes'() { + given: + def collector = new ParameterCollector.ParameterCollectorImpl(true) + def bytes = (0..255).collect { (byte) it } as byte[] + def expected = new String(bytes, 'ISO-8859-1') + + when: + collector.addPart(new RawBytesPart('binary.bin', bytes)) + + then: + collector.getContents()[0] == expected + } + + void 'addPart skips content when inspectContent is false but still collects filename'() { + given: + def collector = new ParameterCollector.ParameterCollectorImpl(false) + + when: + collector.addPart(new TestPart('file.txt', 'hello world')) + + then: + collector.getContents().isEmpty() + collector.getFilenames() == ['file.txt'] + } + + void 'addPart falls back to getFilename() when getSubmittedFileName() is absent (Tomcat 7)'() { + given: + def collector = new ParameterCollector.ParameterCollectorImpl(true) + + when: + collector.addPart(new Tomcat7Part('tomcat7.txt', 'tomcat7 content')) + + then: + collector.getContents() == ['tomcat7 content'] + collector.getFilenames() == ['tomcat7.txt'] + } + + void 'addPart uses Content-Type charset declared by part'() { + given: + def collector = new ParameterCollector.ParameterCollectorImpl(true) + def text = 'héllo wörld' + def bytes = text.getBytes('UTF-8') + + when: + collector.addPart(new TestPartWithContentType('utf8.txt', bytes, 'text/plain; charset=UTF-8')) + + then: + collector.getContents()[0] == text + } + + void 'ParameterCollectorNoop getContents returns empty list'() { + expect: + ParameterCollector.ParameterCollectorNoop.INSTANCE.getContents().isEmpty() + } + + void 'concurrent addPart with two different Part classes never drops parts due to stale cache'() { + // Regression test for the TOCTOU race in the static method cache: + // with 4 flat volatile fields, thread A could pass the "cache == classA" check, + // then thread B could overwrite the fields with classB's methods, and thread A + // would invoke classB.getFilename on a classA instance → IllegalArgumentException + // → part silently dropped. The immutable CachedMethods holder fixes this because + // each thread reads the volatile reference once into a local variable and uses + // only that consistent snapshot. + given: + def iterations = 2000 + def latch = new CountDownLatch(1) + def thread1Errors = new CopyOnWriteArrayList() + def thread2Errors = new CopyOnWriteArrayList() + + // Each thread uses its own collector and its own Part class to maximise + // cache-flip interleaving (the static cache alternates between the two classes). + def thread1 = Thread.start { + latch.await() + (1..iterations).each { i -> + def collector = new ParameterCollector.ParameterCollectorImpl(false) + collector.addPart(new TestPart("tp-${i}.txt", 'x')) + if (collector.getFilenames() != ["tp-${i}.txt"]) { + thread1Errors.add("iter ${i}: got ${collector.getFilenames()}") + } + } + } + + def thread2 = Thread.start { + latch.await() + (1..iterations).each { i -> + def collector = new ParameterCollector.ParameterCollectorImpl(false) + collector.addPart(new Tomcat7Part("t7-${i}.txt", 'x')) + if (collector.getFilenames() != ["t7-${i}.txt"]) { + thread2Errors.add("iter ${i}: got ${collector.getFilenames()}") + } + } + } + + when: + latch.countDown() + thread1.join(30_000) + thread2.join(30_000) + + then: + thread1Errors.isEmpty() + thread2Errors.isEmpty() + } + + // --- helper classes --- + + static class TestPart { + private final String filename + private final String content + + TestPart(String filename, String content) { + this.filename = filename + this.content = content + } + + String getSubmittedFileName() { + filename + } + + InputStream getInputStream() { + new ByteArrayInputStream((content ?: '').getBytes('ISO-8859-1')) + } + + String getContentType() { + null + } + } + + static class FailingPart { + private final String filename + + FailingPart(String filename) { + this.filename = filename + } + + String getSubmittedFileName() { + filename + } + + InputStream getInputStream() { + throw new IOException('simulated error') + } + } + + static class RawBytesPart { + private final String filename + private final byte[] bytes + + RawBytesPart(String filename, byte[] bytes) { + this.filename = filename + this.bytes = bytes + } + + String getSubmittedFileName() { + filename + } + + InputStream getInputStream() { + new ByteArrayInputStream(bytes) + } + + String getContentType() { + 'text/plain; charset=ISO-8859-1' + } + } + + /** Simulates a Tomcat 7 ApplicationPart that only exposes getFilename(), not getSubmittedFileName(). */ + static class Tomcat7Part { + private final String filename + private final String content + + Tomcat7Part(String filename, String content) { + this.filename = filename + this.content = content + } + + String getFilename() { + filename + } + + InputStream getInputStream() { + new ByteArrayInputStream((content ?: '').getBytes('ISO-8859-1')) + } + + String getContentType() { + null + } + } + + static class TestPartWithContentType { + private final String filename + private final byte[] bytes + private final String contentType + + TestPartWithContentType(String filename, byte[] bytes, String contentType) { + this.filename = filename + this.bytes = bytes + this.contentType = contentType + } + + String getSubmittedFileName() { + filename + } + + InputStream getInputStream() { + new ByteArrayInputStream(bytes) + } + + String getContentType() { + contentType + } + } +} diff --git a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java index fd6c384de21..e8ef80bd4a3 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java @@ -144,6 +144,8 @@ public final class ConfigDefaults { static final int DEFAULT_APPSEC_MAX_STACK_TRACE_DEPTH = 32; static final int DEFAULT_APPSEC_MAX_COLLECTED_HEADERS = 50; static final int DEFAULT_APPSEC_BODY_PARSING_SIZE_LIMIT = 10_000_000; + static final int DEFAULT_APPSEC_MAX_FILE_CONTENT_BYTES = 4096; + static final int DEFAULT_APPSEC_MAX_FILE_CONTENT_COUNT = 25; static final String DEFAULT_IAST_ENABLED = "false"; static final boolean DEFAULT_IAST_DEBUG_ENABLED = false; public static final int DEFAULT_IAST_MAX_CONCURRENT_REQUESTS = 4; diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/AppSecConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/AppSecConfig.java index ea1f6cb9e73..370813aa190 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/AppSecConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/AppSecConfig.java @@ -50,6 +50,8 @@ public final class AppSecConfig { public static final String APPSEC_MAX_STACK_TRACE_DEPTH = "appsec.max.stack-trace.depth"; public static final String APPSEC_MAX_STACKTRACE_DEPTH_DEPRECATED = "appsec.max.stacktrace.depth"; // old non-standard as a fallback alias + public static final String APPSEC_MAX_FILE_CONTENT_BYTES = "appsec.max.file-content.bytes"; + public static final String APPSEC_MAX_FILE_CONTENT_COUNT = "appsec.max.file-content.count"; private AppSecConfig() {} } diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index 34de791eee7..7e342e2cd1a 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -12,6 +12,8 @@ import static datadog.trace.api.ConfigDefaults.DEFAULT_API_SECURITY_MAX_DOWNSTREAM_REQUEST_BODY_ANALYSIS; import static datadog.trace.api.ConfigDefaults.DEFAULT_API_SECURITY_SAMPLE_DELAY; import static datadog.trace.api.ConfigDefaults.DEFAULT_APPSEC_BODY_PARSING_SIZE_LIMIT; +import static datadog.trace.api.ConfigDefaults.DEFAULT_APPSEC_MAX_FILE_CONTENT_BYTES; +import static datadog.trace.api.ConfigDefaults.DEFAULT_APPSEC_MAX_FILE_CONTENT_COUNT; import static datadog.trace.api.ConfigDefaults.DEFAULT_APPSEC_MAX_STACK_TRACES; import static datadog.trace.api.ConfigDefaults.DEFAULT_APPSEC_MAX_STACK_TRACE_DEPTH; import static datadog.trace.api.ConfigDefaults.DEFAULT_APPSEC_REPORTING_INBAND; @@ -215,6 +217,8 @@ import static datadog.trace.api.config.AppSecConfig.APPSEC_HTTP_BLOCKED_TEMPLATE_HTML; import static datadog.trace.api.config.AppSecConfig.APPSEC_HTTP_BLOCKED_TEMPLATE_JSON; import static datadog.trace.api.config.AppSecConfig.APPSEC_IP_ADDR_HEADER; +import static datadog.trace.api.config.AppSecConfig.APPSEC_MAX_FILE_CONTENT_BYTES; +import static datadog.trace.api.config.AppSecConfig.APPSEC_MAX_FILE_CONTENT_COUNT; import static datadog.trace.api.config.AppSecConfig.APPSEC_MAX_STACKTRACES_DEPRECATED; import static datadog.trace.api.config.AppSecConfig.APPSEC_MAX_STACKTRACE_DEPTH_DEPRECATED; import static datadog.trace.api.config.AppSecConfig.APPSEC_MAX_STACK_TRACES; @@ -1030,6 +1034,8 @@ public static String getHostName() { private final int appSecMaxStackTraces; private final int appSecMaxStackTraceDepth; private final int appSecBodyParsingSizeLimit; + private final int appSecMaxFileContentBytes; + private final int appSecMaxFileContentCount; private final boolean apiSecurityEnabled; private final float apiSecuritySampleDelay; private final int apiSecurityEndpointCollectionMessageLimit; @@ -2333,6 +2339,12 @@ PROFILING_DATADOG_PROFILER_ENABLED, isDatadogProfilerSafeInCurrentEnvironment()) appSecBodyParsingSizeLimit = configProvider.getInteger( APPSEC_BODY_PARSING_SIZE_LIMIT, DEFAULT_APPSEC_BODY_PARSING_SIZE_LIMIT); + appSecMaxFileContentBytes = + configProvider.getInteger( + APPSEC_MAX_FILE_CONTENT_BYTES, DEFAULT_APPSEC_MAX_FILE_CONTENT_BYTES); + appSecMaxFileContentCount = + configProvider.getInteger( + APPSEC_MAX_FILE_CONTENT_COUNT, DEFAULT_APPSEC_MAX_FILE_CONTENT_COUNT); apiSecurityEnabled = configProvider.getBoolean( API_SECURITY_ENABLED, DEFAULT_API_SECURITY_ENABLED, API_SECURITY_ENABLED_EXPERIMENTAL); @@ -5618,6 +5630,14 @@ public int getAppSecBodyParsingSizeLimit() { return appSecBodyParsingSizeLimit; } + public int getAppSecMaxFileContentBytes() { + return appSecMaxFileContentBytes; + } + + public int getAppSecMaxFileContentCount() { + return appSecMaxFileContentCount; + } + public boolean isCloudPayloadTaggingEnabledFor(String serviceName) { return cloudPayloadTaggingServices.contains(serviceName); } diff --git a/internal-api/src/main/java/datadog/trace/api/http/MultipartContentDecoder.java b/internal-api/src/main/java/datadog/trace/api/http/MultipartContentDecoder.java index 3aefb2e2e7c..acfd45762fa 100644 --- a/internal-api/src/main/java/datadog/trace/api/http/MultipartContentDecoder.java +++ b/internal-api/src/main/java/datadog/trace/api/http/MultipartContentDecoder.java @@ -1,5 +1,7 @@ package datadog.trace.api.http; +import java.io.IOException; +import java.io.InputStream; import java.nio.ByteBuffer; import java.nio.charset.CharacterCodingException; import java.nio.charset.Charset; @@ -8,6 +10,17 @@ /** Decodes multipart file content bytes to String using the per-part Content-Type charset. */ public final class MultipartContentDecoder { + public static String readInputStream(InputStream is, int maxBytes, String contentType) + throws IOException { + byte[] buf = new byte[maxBytes]; + int total = 0; + int n; + while (total < maxBytes && (n = is.read(buf, total, maxBytes - total)) != -1) { + total += n; + } + return decodeBytes(buf, total, contentType); + } + public static String decodeBytes(byte[] buf, int length, String contentType) { Charset charset = extractCharset(contentType); if (charset == null) charset = Charset.defaultCharset(); diff --git a/internal-api/src/test/java/datadog/trace/api/http/MultipartContentDecoderTest.java b/internal-api/src/test/java/datadog/trace/api/http/MultipartContentDecoderTest.java index de028edf24d..013ac39b3c7 100644 --- a/internal-api/src/test/java/datadog/trace/api/http/MultipartContentDecoderTest.java +++ b/internal-api/src/test/java/datadog/trace/api/http/MultipartContentDecoderTest.java @@ -3,6 +3,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; import java.nio.charset.StandardCharsets; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -148,4 +151,25 @@ void decodeBytesUsesQuotedDeclaredCharset() { MultipartContentDecoder.decodeBytes( bytes, bytes.length, "text/plain; charset=\"ISO-8859-1\"")); } + + @Test + void readInputStreamTruncatesAtMaxBytes() throws IOException { + byte[] data = "hello world".getBytes(StandardCharsets.UTF_8); + assertEquals( + "hello", MultipartContentDecoder.readInputStream(new ByteArrayInputStream(data), 5, null)); + } + + @Test + void readInputStreamHandlesMultipleReadCallsToFillBuffer() throws IOException { + byte[] data = "hello world".getBytes(StandardCharsets.UTF_8); + // InputStream that returns 2 bytes per read() call to exercise the accumulation loop. + InputStream slow = + new ByteArrayInputStream(data) { + @Override + public synchronized int read(byte[] b, int off, int len) { + return super.read(b, off, Math.min(len, 2)); + } + }; + assertEquals("hello world", MultipartContentDecoder.readInputStream(slow, data.length, null)); + } } diff --git a/metadata/supported-configurations.json b/metadata/supported-configurations.json index a1569256c66..e1ad02e47bc 100644 --- a/metadata/supported-configurations.json +++ b/metadata/supported-configurations.json @@ -289,6 +289,22 @@ "aliases": [] } ], + "DD_APPSEC_MAX_FILE_CONTENT_BYTES": [ + { + "version": "A", + "type": "int", + "default": "4096", + "aliases": [] + } + ], + "DD_APPSEC_MAX_FILE_CONTENT_COUNT": [ + { + "version": "A", + "type": "int", + "default": "25", + "aliases": [] + } + ], "DD_APPSEC_MAX_STACKTRACES": [ { "version": "A",