From 4acc832bf232d98ec320acc93b2d1a52ca1be47b Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Wed, 26 Aug 2026 15:01:51 +0200 Subject: [PATCH 01/10] content type dispatch --- .../trace/lambda/ContentTypeBodyParser.java | 112 +++++++++++ .../trace/lambda/LambdaEventParser.java | 32 +-- .../lambda/ContentTypeBodyParserTest.java | 190 ++++++++++++++++++ .../trace/lambda/LambdaAppSecHandlerTest.java | 51 +++++ 4 files changed, 362 insertions(+), 23 deletions(-) create mode 100644 dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java create mode 100644 dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java new file mode 100644 index 00000000000..ceb4d852ece --- /dev/null +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java @@ -0,0 +1,112 @@ +package datadog.trace.lambda; + +import datadog.trace.api.appsec.MediaType; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.StringTokenizer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Turns a Lambda request body into the shape the AppSec WAF expects. The declared {@code + * Content-Type} decides how the body is structured; a best-effort JSON parse handles both the + * JSON-ish types and the case where no type is declared at all. + * + *

A body is never dropped: any type we cannot structure — and any parse failure — degrades to + * the raw {@link String}, which the WAF can still match string rules against. + */ +final class ContentTypeBodyParser { + + private static final Logger log = LoggerFactory.getLogger(ContentTypeBodyParser.class); + + // These bound the work done in this parser only. The WAF truncates its inputs independently, so + // exceeding them is wasteful rather than incorrect. + static final int MAX_DEPTH = 20; + static final int MAX_ELEMENTS = 256; + + private ContentTypeBodyParser() {} + + /** Parses a decoded request body according to its {@code Content-Type}. */ + static Object parseBody(final String body, final String contentType) { + return dispatch(body, contentType, 0); + } + + static Object dispatch(final String body, final String contentType, final int depth) { + if (body == null) { + return null; + } + if (depth >= MAX_DEPTH) { + log.debug("Body nesting depth {} reached, keeping raw string", depth); + return body; + } + if (contentType == null || contentType.trim().isEmpty() || isJsonLike(contentType)) { + final Object parsed = LambdaEventParser.parseBodyAsJson(body); + return parsed != null ? parsed : body; + } + final MediaType mediaType = MediaType.parse(contentType); + if ("application".equals(mediaType.getType()) + && "x-www-form-urlencoded".equals(mediaType.getSubtype())) { + final Object parsed = parseUrlEncoded(body); + return parsed != null ? parsed : body; + } + // text/*, multipart/* and everything else stay raw strings. In particular a text/plain body of + // "12345" must reach the WAF as a String, not as the Double a JSON parse would produce. + return body; + } + + static boolean isJsonLike(final String contentType) { + if (contentType == null) { + return false; + } + final String lower = contentType.toLowerCase(Locale.ROOT); + return lower.contains("json") || lower.contains("javascript"); + } + + /** + * Parses an {@code application/x-www-form-urlencoded} body into a multimap, matching the shape + * produced for query parameters. + * + * @return the parsed parameters, or {@code null} if nothing usable was found + */ + private static Map> parseUrlEncoded(final String body) { + if (body.isEmpty()) { + return null; + } + final Map> parameters = new LinkedHashMap<>(); + int pairs = 0; + final StringTokenizer tokenizer = new StringTokenizer(body, "&"); + while (tokenizer.hasMoreTokens() && pairs < MAX_ELEMENTS) { + final String pair = tokenizer.nextToken(); + pairs++; + final int equals = pair.indexOf('='); + final String name = decode(equals == -1 ? pair : pair.substring(0, equals)); + if (!name.isEmpty()) { + parameters + .computeIfAbsent(name, k -> new ArrayList<>(1)) + .add(equals == -1 ? "" : decode(pair.substring(equals + 1))); + } + } + if (parameters.isEmpty()) { + return null; + } + log.debug("Body parsed as {} urlencoded parameters", parameters.size()); + return parameters; + } + + /** Percent-decodes a single token, keeping it undecoded rather than dropping it on failure. */ + private static String decode(final String value) { + if (value.isEmpty()) { + return value; + } + try { + return URLDecoder.decode(value, "UTF-8"); + } catch (final UnsupportedEncodingException | IllegalArgumentException e) { + return value; + } + } +} diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java index 1848e78b620..4916a157699 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java @@ -162,13 +162,8 @@ static LambdaResponseData parseResponse(String json) { if (bodyString != null) { String contentType = headers.get("content-type"); - // If JSON content-type or unknown, attempt JSON parsing - // Normalise casing: media type tokens are case-insensitive per RFC 7231 - String contentTypeLower = - contentType == null ? null : contentType.toLowerCase(Locale.ROOT); - if (contentTypeLower == null - || contentTypeLower.contains("json") - || contentTypeLower.contains("javascript")) { + // If JSON content-type or unknown, attempt JSON parsing. + if (contentType == null || ContentTypeBodyParser.isJsonLike(contentType)) { Object parsed = parseBodyAsJson(bodyString); body = parsed != null ? parsed : bodyString; } else { @@ -247,7 +242,7 @@ private static LambdaRequestData extractApiGatewayV1Data(Map eve if (queryParameters.isEmpty()) { queryParameters = extractQueryParameters(event.get("queryStringParameters")); } - Object body = extractBody(event); + Object body = extractBody(event, headers); Map requestContext = (Map) event.get("requestContext"); String method = (String) requestContext.get("httpMethod"); @@ -283,7 +278,7 @@ private static LambdaRequestData extractApiGatewayV2HttpData( Map pathParameters = extractPathParameters(event.get("pathParameters")); Map> queryParameters = extractQueryParameters(event.get("queryStringParameters")); - Object body = extractBody(event); + Object body = extractBody(event, headers); Map requestContext = (Map) event.get("requestContext"); Map http = (Map) requestContext.get("http"); @@ -337,7 +332,7 @@ private static LambdaRequestData extractApiGatewayV2WebSocketData(Map pathParameters = extractPathParameters(event.get("pathParameters")); Map> queryParameters = extractQueryParameters(event.get("queryStringParameters")); - Object body = extractBody(event); + Object body = extractBody(event, headers); Map requestContext = (Map) event.get("requestContext"); @@ -418,7 +413,7 @@ private static LambdaRequestData extractAlbData( queryParameters = extractQueryParameters(event.get("queryStringParameters")); } - Object body = extractBody(event); + Object body = extractBody(event, headers); String method = (String) event.get("httpMethod"); String path = (String) event.get("path"); @@ -674,7 +669,7 @@ private static Map extractHeadersWithCookies(Map } /** Helper method to extract and parse body from event */ - private static Object extractBody(Map event) { + private static Object extractBody(Map event, Map headers) { Object bodyObj = event.get("body"); if (bodyObj == null) { return null; @@ -693,20 +688,11 @@ private static Object extractBody(Map event) { } } - // Try to parse as JSON - Object parsedBody = parseBodyAsJson(bodyString); - if (parsedBody != null) { - log.debug("Body parsed as JSON successfully"); - return parsedBody; - } - - // If not JSON, return the raw string - log.debug("Body is not JSON, returning raw string"); - return bodyString; + return ContentTypeBodyParser.parseBody(bodyString, headers.get("content-type")); } /** Helper method to parse body as JSON */ - private static Object parseBodyAsJson(String body) { + static Object parseBodyAsJson(String body) { if (body == null || body.isEmpty() || "null".equals(body)) { return null; } diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java new file mode 100644 index 00000000000..3395f72c7da --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java @@ -0,0 +1,190 @@ +package datadog.trace.lambda; + +import static datadog.trace.lambda.ContentTypeBodyParser.MAX_DEPTH; +import static datadog.trace.lambda.ContentTypeBodyParser.MAX_ELEMENTS; +import static datadog.trace.lambda.ContentTypeBodyParser.parseBody; +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; + +class ContentTypeBodyParserTest { + + @ParameterizedTest(name = "[{index}] {1} -> {2}") + @CsvSource( + delimiter = '|', + value = { + // content type absent or blank: best-effort JSON, as before content-type dispatch existed + "{\"a\":1} | | MAP", + "{\"a\":1} | ' ' | MAP", + "not json | | STRING", + // JSON, including suffixed subtypes and parameters + "{\"a\":1} | application/json | MAP", + "{\"a\":1} | application/json; charset=utf-8 | MAP", + "{\"a\":1} | APPLICATION/JSON | MAP", + "{\"a\":1} | application/vnd.api+json | MAP", + "{\"a\":1 | application/json | STRING", + // JSON-ish vendor types: no +json suffix, but the payload is JSON + "{\"a\":1} | application/x-amz-json-1.1 | MAP", + "{\"a\":1} | application/javascript | MAP", + // urlencoded + "a=1 | application/x-www-form-urlencoded | MAP", + "a=1 | APPLICATION/X-WWW-FORM-URLENCODED | MAP", + // multipart is not structured yet + "{\"a\":1} | multipart/form-data; boundary=xy | STRING", + // text/*: never structured, even when it holds JSON. A JSON parse of "12345" would yield a + // Double, which no string rule can match + "{\"a\":1} | text/plain | STRING", + "12345 | text/plain | STRING", + "{\"a\":1} | text/html | STRING", + // anything else + "{\"a\":1} | application/xml | STRING", + "{\"a\":1} | application/octet-stream | STRING", + "{\"a\":1} | garbage | STRING", + // known gap: a JSON-carrying type whose name says neither "json" nor "javascript" keeps + // the raw string, which the WAF can still match string rules against + "{\"a\":1} | application/csp-report | STRING", + }) + void dispatchesOnContentType(String body, String contentType, String expectedKind) { + Object parsed = parseBody(body, contentType); + + if ("MAP".equals(expectedKind)) { + assertInstanceOf(Map.class, parsed); + } else { + assertEquals(body, parsed); + } + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + void parsesJsonWhenContentTypeIsAbsent(String contentType) { + Object parsed = parseBody("{\"user\":\"admin\"}", contentType); + + assertInstanceOf(Map.class, parsed); + assertEquals("admin", ((Map) parsed).get("user")); + } + + @Test + void returnsNullForNullBody() { + assertNull(parseBody(null, "application/json")); + } + + @Test + void keepsEmptyBodyAsEmptyString() { + assertEquals("", parseBody("", "application/json")); + assertEquals("", parseBody("", "application/x-www-form-urlencoded")); + } + + @Test + void keepsRawStringOnceMaxDepthIsReached() { + String body = "{\"a\":1}"; + + assertEquals(body, ContentTypeBodyParser.dispatch(body, "application/json", MAX_DEPTH)); + assertInstanceOf( + Map.class, ContentTypeBodyParser.dispatch(body, "application/json", MAX_DEPTH - 1)); + } + + @Test + void parsesUrlEncodedIntoAMultimap() { + Map> parsed = urlEncoded("user=admin&role=root"); + + assertEquals(singletonList("admin"), parsed.get("user")); + assertEquals(singletonList("root"), parsed.get("role")); + assertEquals(2, parsed.size()); + } + + @Test + void groupsRepeatedUrlEncodedKeysIntoOneList() { + assertEquals(asList("a", "b", "c"), urlEncoded("x=a&x=b&x=c").get("x")); + } + + @Test + void decodesUrlEncodedPercentEscapesAndPluses() { + Map> parsed = urlEncoded("na+me=hello+world&q=%7B%22a%22%3A1%7D"); + + assertEquals(singletonList("hello world"), parsed.get("na me")); + assertEquals(singletonList("{\"a\":1}"), parsed.get("q")); + } + + @Test + void keepsUndecodableUrlEncodedTokensAsIs() { + Map> parsed = urlEncoded("a=%&%=b"); + + assertEquals(singletonList("%"), parsed.get("a")); + assertEquals(singletonList("b"), parsed.get("%")); + } + + @Test + void treatsValuelessUrlEncodedPairsAsEmptyValues() { + Map> parsed = urlEncoded("flag&other=&last"); + + assertEquals(singletonList(""), parsed.get("flag")); + assertEquals(singletonList(""), parsed.get("other")); + assertEquals(singletonList(""), parsed.get("last")); + } + + @Test + void skipsEmptyUrlEncodedPairsAndNames() { + Map> parsed = urlEncoded("&&=orphan&&a=1&&"); + + assertEquals(singletonList("1"), parsed.get("a")); + assertEquals(1, parsed.size()); + } + + @Test + void splitsUrlEncodedValuesAtTheFirstEqualsOnly() { + assertEquals(singletonList("b=c=d"), urlEncoded("a=b=c=d").get("a")); + } + + @Test + void doesNotSeparateUrlEncodedPairsOnSemicolons() { + assertEquals(singletonList("1;b=2"), urlEncoded("a=1;b=2").get("a")); + } + + @Test + void capsUrlEncodedPairsAtMaxElements() { + StringBuilder body = new StringBuilder(); + for (int i = 0; i < MAX_ELEMENTS + 1; i++) { + body.append(i == 0 ? "" : "&").append('k').append(i).append("=v"); + } + + assertEquals(MAX_ELEMENTS, urlEncoded(body.toString()).size()); + } + + @Test + void stopsScanningNamelessUrlEncodedPairsAtTheCap() { + StringBuilder body = new StringBuilder(); + for (int i = 0; i < MAX_ELEMENTS; i++) { + body.append("=v&"); + } + body.append("a=1"); + String raw = body.toString(); + + // Nameless pairs still do decoding work, so they exhaust the cap and the trailing parameter is + // never reached; nothing survives, so the raw body is kept + assertEquals(raw, parseBody(raw, "application/x-www-form-urlencoded")); + } + + @Test + void keepsUnparseableUrlEncodedBodyAsRawString() { + // Nothing but separators: no parameter survives, so the raw body is kept + assertEquals("&&&", parseBody("&&&", "application/x-www-form-urlencoded")); + } + + @SuppressWarnings("unchecked") + private static Map> urlEncoded(String body) { + Object parsed = parseBody(body, "application/x-www-form-urlencoded"); + assertInstanceOf(Map.class, parsed); + return (Map>) parsed; + } +} diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java index 33e68f528e2..24a20261838 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java @@ -700,6 +700,57 @@ void handlesEmptyBodyCorrectly() { assertEquals("", capturedBody[0]); } + @Test + @SuppressWarnings("unchecked") + void parsesUrlEncodedBodyIntoAMultimap() { + String eventJson = + "{" + + "\"body\": \"user=admin&role=root\"," + + "\"headers\": {\"Content-Type\": \"application/x-www-form-urlencoded\"}," + + "\"requestContext\": {\"httpMethod\": \"POST\"}" + + "}"; + ByteArrayInputStream event = createInputStream(eventJson); + + Object[] capturedBody = {null}; + + setupMockCallbacks(new Callbacks().onBody(body -> capturedBody[0] = body)); + + AgentSpanContext result = LambdaAppSecHandler.processRequestStart(event); + + assertNotNull(result); + assertInstanceOf(Map.class, capturedBody[0]); + Map> parameters = (Map>) capturedBody[0]; + assertEquals(Arrays.asList("admin"), parameters.get("user")); + assertEquals(Arrays.asList("root"), parameters.get("role")); + } + + @Test + @SuppressWarnings("unchecked") + void appliesContentTypeDispatchToBase64DecodedBodies() { + String base64Body = + Base64.getEncoder().encodeToString("user=admin".getBytes(StandardCharsets.UTF_8)); + String eventJson = + "{" + + "\"body\": \"" + + base64Body + + "\"," + + "\"isBase64Encoded\": true," + + "\"headers\": {\"Content-Type\": \"application/x-www-form-urlencoded\"}," + + "\"requestContext\": {\"httpMethod\": \"POST\"}" + + "}"; + ByteArrayInputStream event = createInputStream(eventJson); + + Object[] capturedBody = {null}; + + setupMockCallbacks(new Callbacks().onBody(body -> capturedBody[0] = body)); + + AgentSpanContext result = LambdaAppSecHandler.processRequestStart(event); + + assertNotNull(result); + assertInstanceOf(Map.class, capturedBody[0]); + assertEquals(Arrays.asList("admin"), ((Map>) capturedBody[0]).get("user")); + } + @Test void handlesPathWithQueryStringCorrectly() { String eventJson = From 291b70c2875d2e382d3144d5e9f46bd14479a86a Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Fri, 28 Aug 2026 14:50:06 +0200 Subject: [PATCH 02/10] Report multipart file part filenames to the WAF Co-Authored-By: Claude Opus 5 --- .../trace/lambda/ContentTypeBodyParser.java | 200 +++++++++- .../trace/lambda/LambdaAppSecHandler.java | 15 + .../trace/lambda/LambdaEventParser.java | 40 +- .../trace/lambda/MultipartSplitter.java | 283 ++++++++++++++ .../lambda/ContentTypeBodyParserTest.java | 354 +++++++++++++++++- .../trace/lambda/LambdaAppSecHandlerTest.java | 124 ++++++ .../trace/lambda/MultipartSplitterTest.java | 233 ++++++++++++ 7 files changed, 1214 insertions(+), 35 deletions(-) create mode 100644 dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java create mode 100644 dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java index ceb4d852ece..b2e08f4e724 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java @@ -1,13 +1,17 @@ package datadog.trace.lambda; import datadog.trace.api.appsec.MediaType; +import datadog.trace.lambda.MultipartSplitter.Part; import java.io.UnsupportedEncodingException; import java.net.URLDecoder; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Set; import java.util.StringTokenizer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -24,19 +28,79 @@ final class ContentTypeBodyParser { private static final Logger log = LoggerFactory.getLogger(ContentTypeBodyParser.class); - // These bound the work done in this parser only. The WAF truncates its inputs independently, so - // exceeding them is wasteful rather than incorrect. + // These bound the work done in this parser only. Exceeding any of them degrades the body to a raw + // string rather than dropping content, and the WAF truncates its inputs independently anyway. static final int MAX_DEPTH = 20; static final int MAX_ELEMENTS = 256; + static final int MAX_PARTS = 256; + static final int MAX_MULTIPART_SIZE = 1_000_000; private ContentTypeBodyParser() {} + /** + * State shared across a whole parse: the element and part allowances, and the filenames collected + * along the way. A multipart part may itself hold an urlencoded or multipart body, so a per-call + * cap would multiply across nesting levels and a per-call list would lose nested file parts. + */ + static final class ParseContext { + private int parts = MAX_PARTS; + private int elements = MAX_ELEMENTS; + + /** Allocated only once a file part is seen, which most bodies never do. */ + private List filenames; + + int remainingParts() { + return parts; + } + + void consumeParts(final int count) { + parts -= count; + } + + /** + * @return {@code false} once the element allowance is spent + */ + boolean takeElement() { + if (elements == 0) { + return false; + } + elements--; + return true; + } + + void addFilename(final String filename) { + if (filenames == null) { + filenames = new ArrayList<>(2); + } + filenames.add(filename); + } + + /** + * @return the filenames of the multipart file parts found, in body order, empty when there were + * none + */ + List filenames() { + return filenames == null ? Collections.emptyList() : filenames; + } + } + /** Parses a decoded request body according to its {@code Content-Type}. */ static Object parseBody(final String body, final String contentType) { - return dispatch(body, contentType, 0); + return parseBody(body, contentType, new ParseContext()); + } + + /** + * Parses a decoded request body according to its {@code Content-Type}. + * + * @param context also collects the filenames of any multipart file parts found, which the caller + * reports separately from the body + */ + static Object parseBody(final String body, final String contentType, final ParseContext context) { + return dispatch(body, contentType, 0, context); } - static Object dispatch(final String body, final String contentType, final int depth) { + static Object dispatch( + final String body, final String contentType, final int depth, final ParseContext context) { if (body == null) { return null; } @@ -51,11 +115,15 @@ static Object dispatch(final String body, final String contentType, final int de final MediaType mediaType = MediaType.parse(contentType); if ("application".equals(mediaType.getType()) && "x-www-form-urlencoded".equals(mediaType.getSubtype())) { - final Object parsed = parseUrlEncoded(body); + final Object parsed = parseUrlEncoded(body, context); + return parsed != null ? parsed : body; + } + if ("multipart".equals(mediaType.getType())) { + final Object parsed = parseMultipart(body, contentType, depth, context, MAX_MULTIPART_SIZE); return parsed != null ? parsed : body; } - // text/*, multipart/* and everything else stay raw strings. In particular a text/plain body of - // "12345" must reach the WAF as a String, not as the Double a JSON parse would produce. + // text/* and everything else stay raw strings. In particular a text/plain body of "12345" must + // reach the WAF as a String, not as the Double a JSON parse would produce. return body; } @@ -63,26 +131,38 @@ static boolean isJsonLike(final String contentType) { if (contentType == null) { return false; } - final String lower = contentType.toLowerCase(Locale.ROOT); - return lower.contains("json") || lower.contains("javascript"); + // Match on the type and subtype only. Parameters such as a multipart boundary are chosen by + // the client, so "multipart/form-data; boundary=--json" must not reach the JSON parser and + // thereby skip multipart parsing entirely. + final int semicolon = contentType.indexOf(';'); + final String essence = + (semicolon == -1 ? contentType : contentType.substring(0, semicolon)) + .toLowerCase(Locale.ROOT); + return essence.contains("json") || essence.contains("javascript"); } /** * Parses an {@code application/x-www-form-urlencoded} body into a multimap, matching the shape * produced for query parameters. * - * @return the parsed parameters, or {@code null} if nothing usable was found + * @return the parsed parameters, or {@code null} if nothing usable was found or the body exhausts + * the element allowance */ - private static Map> parseUrlEncoded(final String body) { + private static Map> parseUrlEncoded( + final String body, final ParseContext context) { if (body.isEmpty()) { return null; } final Map> parameters = new LinkedHashMap<>(); - int pairs = 0; final StringTokenizer tokenizer = new StringTokenizer(body, "&"); - while (tokenizer.hasMoreTokens() && pairs < MAX_ELEMENTS) { + while (tokenizer.hasMoreTokens()) { + if (!context.takeElement()) { + // Bail out rather than hand the WAF a truncated map: a parameter dropped here would be + // invisible to every rule, whereas the raw body can still be string-matched. + log.debug("Element allowance exhausted, keeping urlencoded body as a raw string"); + return null; + } final String pair = tokenizer.nextToken(); - pairs++; final int equals = pair.indexOf('='); final String name = decode(equals == -1 ? pair : pair.substring(0, equals)); if (!name.isEmpty()) { @@ -98,6 +178,98 @@ private static Map> parseUrlEncoded(final String body) { return parameters; } + /** + * Parses a {@code multipart/*} body into its form fields. + * + * @param sizeLimit the largest body to attempt, or {@code 0} to disable multipart parsing + * @return the fields found, or {@code null} if the body is too large, has no usable boundary, + * holds more parts than the allowance, or yields no field + */ + static Object parseMultipart( + final String body, + final String contentType, + final int depth, + final ParseContext context, + final int sizeLimit) { + if (sizeLimit <= 0 || body.length() > sizeLimit) { + log.debug("Multipart body of {} chars not parsed, keeping raw string", body.length()); + return null; + } + final String boundary = MultipartSplitter.extractBoundary(contentType); + if (boundary == null) { + log.debug("Multipart body without a usable boundary, keeping raw string"); + return null; + } + // One over the allowance, so that a body holding more parts than may be read is distinguishable + // from one holding exactly the allowance + final int allowance = context.remainingParts(); + final List parts = MultipartSplitter.split(body, boundary, allowance + 1); + if (parts.size() > allowance) { + // Bail out rather than hand the WAF a truncated map, as the urlencoded path does: + // the raw body can still be string-matched. + log.debug("Part allowance exhausted, keeping multipart body as a raw string"); + return null; + } + context.consumeParts(parts.size()); + + final Map fields = new LinkedHashMap<>(); + final Set promoted = new HashSet<>(); + for (final Part part : parts) { + final String disposition = part.header("content-disposition"); + if (disposition == null) { + continue; + } + final String filename = MultipartSplitter.parameter(disposition, "filename"); + if (filename != null) { + if (!filename.isEmpty()) { + context.addFilename(filename); + } + continue; + } + final String name = MultipartSplitter.parameter(disposition, "name"); + if (name == null || name.isEmpty()) { + continue; + } + final Object value = + dispatch( + body.substring(part.contentStart, part.contentEnd), + part.header("content-type"), + depth + 1, + context); + addField(fields, promoted, name, value); + } + return fields.isEmpty() ? null : fields; + } + + /** + * Accumulates a field as a scalar on first sight and promotes it to a list on repeat. + * Deliberately a different shape from urlencoded's always-a-list, matching the peer tracers. + * + * @param promoted the names already promoted to a list, mutated as fields are promoted + */ + @SuppressWarnings("unchecked") + private static void addField( + final Map fields, + final Set promoted, + final String name, + final Object value) { + // A part value is never null, so an absent key is exactly a null lookup + final Object existing = fields.get(name); + if (existing == null) { + fields.put(name, value); + } else if (promoted.contains(name)) { + ((List) existing).add(value); + } else { + // Promotion is tracked rather than inferred from the stored value's type: a part whose body + // parsed as a JSON array is itself a List, and appending to it would flatten the two apart. + final List values = new ArrayList<>(2); + values.add(existing); + values.add(value); + fields.put(name, values); + promoted.add(name); + } + } + /** Percent-decodes a single token, keeping it undecoded rather than dropping it on failure. */ private static String decode(final String value) { if (value.isEmpty()) { diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaAppSecHandler.java b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaAppSecHandler.java index 4aac101e98a..bf1c05d692c 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaAppSecHandler.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaAppSecHandler.java @@ -39,6 +39,7 @@ import java.io.ByteArrayOutputStream; import java.nio.charset.StandardCharsets; import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.function.BiFunction; @@ -477,6 +478,20 @@ private static AgentSpanContext processAppSecRequestData( log.debug("requestBodyProcessed callback is null"); } } + + // Call requestFilesFilenames. Only the names are reported: the file content shares the + // body's UTF-8 decode, so for anything that is not text it is already lossy. + if (!eventData.filenames.isEmpty()) { + BiFunction, Flow> filenamesCallback = + tracer + .getCallbackProvider(RequestContextSlot.APPSEC) + .getCallback(EVENTS.requestFilesFilenames()); + if (filenamesCallback != null) { + filenamesCallback.apply(requestContext, eventData.filenames); + } else { + log.debug("requestFilesFilenames callback is null"); + } + } } return tagContext; } diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java index 4916a157699..c918ea97e17 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java @@ -3,6 +3,7 @@ import com.squareup.moshi.JsonAdapter; import com.squareup.moshi.Moshi; import datadog.trace.api.Config; +import datadog.trace.lambda.ContentTypeBodyParser.ParseContext; import java.io.ByteArrayInputStream; import java.io.IOException; import java.net.URLEncoder; @@ -242,7 +243,8 @@ private static LambdaRequestData extractApiGatewayV1Data(Map eve if (queryParameters.isEmpty()) { queryParameters = extractQueryParameters(event.get("queryStringParameters")); } - Object body = extractBody(event, headers); + ParseContext parseContext = new ParseContext(); + Object body = extractBody(event, headers, parseContext); Map requestContext = (Map) event.get("requestContext"); String method = (String) requestContext.get("httpMethod"); @@ -268,7 +270,8 @@ private static LambdaRequestData extractApiGatewayV1Data(Map eve extractHost(requestContext, headers), // REST APIs expose the parameterized route as the top-level "resource" stringOrNull(event.get("resource")), - null); + null, + parseContext.filenames()); } /** Extracts data from API Gateway v2 (HTTP API) or Lambda URL event */ @@ -278,7 +281,8 @@ private static LambdaRequestData extractApiGatewayV2HttpData( Map pathParameters = extractPathParameters(event.get("pathParameters")); Map> queryParameters = extractQueryParameters(event.get("queryStringParameters")); - Object body = extractBody(event, headers); + ParseContext parseContext = new ParseContext(); + Object body = extractBody(event, headers, parseContext); Map requestContext = (Map) event.get("requestContext"); Map http = (Map) requestContext.get("http"); @@ -306,7 +310,8 @@ private static LambdaRequestData extractApiGatewayV2HttpData( body, extractHost(requestContext, headers), extractRouteKey(requestContext), - extractRawUri(event)); + extractRawUri(event), + parseContext.filenames()); } /** @@ -332,7 +337,8 @@ private static LambdaRequestData extractApiGatewayV2WebSocketData(Map pathParameters = extractPathParameters(event.get("pathParameters")); Map> queryParameters = extractQueryParameters(event.get("queryStringParameters")); - Object body = extractBody(event, headers); + ParseContext parseContext = new ParseContext(); + Object body = extractBody(event, headers, parseContext); Map requestContext = (Map) event.get("requestContext"); @@ -360,7 +366,8 @@ private static LambdaRequestData extractApiGatewayV2WebSocketData(Map extractHeadersWithCookies(Map } /** Helper method to extract and parse body from event */ - private static Object extractBody(Map event, Map headers) { + private static Object extractBody( + Map event, Map headers, ParseContext parseContext) { Object bodyObj = event.get("body"); if (bodyObj == null) { return null; @@ -688,7 +698,7 @@ private static Object extractBody(Map event, Map } } - return ContentTypeBodyParser.parseBody(bodyString, headers.get("content-type")); + return ContentTypeBodyParser.parseBody(bodyString, headers.get("content-type"), parseContext); } /** Helper method to parse body as JSON */ @@ -758,6 +768,9 @@ static class LambdaRequestData { */ final String rawUri; + /** Filenames of the multipart file parts carried by the body, empty when there are none. */ + final List filenames; + static final LambdaRequestData EMPTY = new LambdaRequestData( Collections.emptyMap(), @@ -792,7 +805,8 @@ static class LambdaRequestData { body, null, null, - null); + null, + Collections.emptyList()); } LambdaRequestData( @@ -807,7 +821,8 @@ static class LambdaRequestData { Object body, String host, String route, - String rawUri) { + String rawUri, + List filenames) { this.headers = headers; this.method = method; this.path = path; @@ -820,6 +835,7 @@ static class LambdaRequestData { this.host = host; this.route = route; this.rawUri = rawUri; + this.filenames = filenames; } } diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java new file mode 100644 index 00000000000..60ebd156f3d --- /dev/null +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java @@ -0,0 +1,283 @@ +package datadog.trace.lambda; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +/** + * Splits a {@code multipart/*} body into its parts and reads {@code Content-Type} / {@code + * Content-Disposition} parameters. + */ +final class MultipartSplitter { + + private static final int MAX_BOUNDARY_LENGTH = 70; + + private MultipartSplitter() {} + + /** + * One part of a multipart body. Content is an index range into the body passed to {@link #split}. + */ + static final class Part { + private final Map headers; + final int contentStart; + final int contentEnd; + + private Part(final Map headers, final int contentStart, final int contentEnd) { + this.headers = headers; + this.contentStart = contentStart; + this.contentEnd = contentEnd; + } + + /** + * @param lowercaseName the header name, lowercased by the caller + */ + String header(final String lowercaseName) { + return headers.get(lowercaseName); + } + } + + /** + * Splits a multipart body into at most {@code partBudget} parts. + * + * @return the parts found, in order; empty if the body holds none + */ + static List split(final String body, final String boundary, final int partBudget) { + final List parts = new ArrayList<>(); + if (body == null || boundary == null || boundary.isEmpty() || partBudget <= 0) { + return parts; + } + final String delimiter = "--" + boundary; + final int length = body.length(); + int position = body.startsWith(delimiter) ? 0 : nextDelimiter(body, delimiter, 0); + + while (position >= 0 && parts.size() < partBudget) { + final int afterDelimiter = position + delimiter.length(); + if (body.startsWith("--", afterDelimiter)) { + // Close delimiter: anything past it is the epilogue + break; + } + final int headerStart = lineStart(body, afterDelimiter); + if (headerStart < 0) { + // Not a part boundary after all — the delimiter was part of some part's content + position = nextDelimiter(body, delimiter, afterDelimiter); + continue; + } + final Map headers = new HashMap<>(4); + int cursor = headerStart; + boolean headersComplete = false; + int malformedPartEnd = -1; + while (cursor < length) { + if (body.startsWith(delimiter, cursor)) { + // This part's headers are not followed by a blank line. Stop here: reading on would + // consume the next part's delimiter and merge its headers into this part, collapsing + // every following part into this one. + malformedPartEnd = cursor; + break; + } + final int newline = body.indexOf('\n', cursor); + final int lineEnd = newline < 0 ? length : newline; + final int trimmed = + lineEnd > cursor && body.charAt(lineEnd - 1) == '\r' ? lineEnd - 1 : lineEnd; + if (trimmed == cursor) { + headersComplete = true; + cursor = newline < 0 ? length : newline + 1; + break; + } + addHeader(headers, body, cursor, trimmed); + if (newline < 0) { + cursor = length; + break; + } + cursor = newline + 1; + } + if (malformedPartEnd >= 0) { + // Resume at the delimiter the headers ran into. It is past the current position, so the + // outer scan still makes progress. + position = malformedPartEnd; + continue; + } + if (!headersComplete) { + // Body truncated inside the headers: there is no content to report + break; + } + final int next = nextDelimiter(body, delimiter, cursor); + parts.add(new Part(headers, cursor, contentEnd(body, cursor, next))); + position = next; + } + return parts; + } + + /** + * Reads the {@code boundary} parameter of a {@code Content-Type} header, case preserved. + * + * @return the boundary, or {@code null} if absent or unusable + */ + static String extractBoundary(final String contentType) { + final String boundary = parameter(contentType, "boundary"); + if (boundary == null || boundary.isEmpty() || boundary.length() > MAX_BOUNDARY_LENGTH) { + return null; + } + return boundary; + } + + /** + * Reads a header parameter, case preserved. The parameter name is matched case-insensitively and + * only at a parameter boundary, so {@code filename} does not satisfy a lookup for {@code name}. + * Quoted values may contain separators and {@code \"} escapes, and are skipped whole: a field + * named {@code "; filename=x"} does not read as a {@code filename} parameter. RFC 7230 optional + * whitespace is tolerated on both sides of the {@code =}. + * + * @return the parameter value, {@code ""} when it is present but empty, or {@code null} when the + * parameter is absent + */ + static String parameter(final String headerValue, final String paramName) { + if (headerValue == null || paramName == null) { + return null; + } + final int length = headerValue.length(); + final int nameLength = paramName.length(); + boolean atBoundary = true; + for (int i = 0; i < length; i++) { + final char c = headerValue.charAt(i); + if (c == '"') { + i = endOfQuoted(headerValue, i); + atBoundary = false; + continue; + } + if (atBoundary && headerValue.regionMatches(true, i, paramName, 0, nameLength)) { + final int equals = skipOptionalWhitespace(headerValue, i + nameLength); + if (equals < length && headerValue.charAt(equals) == '=') { + return value(headerValue, skipOptionalWhitespace(headerValue, equals + 1)); + } + } + atBoundary = isParameterSeparator(c); + } + return null; + } + + /** + * Walks a quoted value, honouring {@code \"} escapes. + * + * @param openQuote the index of the opening quote + * @return the index of the closing quote, or the body length when the quote is unterminated + */ + private static int endOfQuoted(final String headerValue, final int openQuote) { + final int length = headerValue.length(); + for (int i = openQuote + 1; i < length; i++) { + final char c = headerValue.charAt(i); + if (c == '\\' && i + 1 < length) { + i++; + } else if (c == '"') { + return i; + } + } + return length; + } + + private static String value(final String headerValue, final int from) { + final int length = headerValue.length(); + if (from < length && headerValue.charAt(from) == '"') { + // An unterminated quote ends at the body length, so what was read is kept rather than the + // parameter being dropped. + final int close = endOfQuoted(headerValue, from); + final StringBuilder unquoted = new StringBuilder(close - from); + for (int i = from + 1; i < close; i++) { + final char c = headerValue.charAt(i); + if (c == '\\' && i + 1 < length) { + unquoted.append(headerValue.charAt(++i)); + } else { + unquoted.append(c); + } + } + return unquoted.toString(); + } + int end = length; + for (int i = from; i < length; i++) { + if (isParameterSeparator(headerValue.charAt(i))) { + end = i; + break; + } + } + return headerValue.substring(from, end); + } + + private static int skipOptionalWhitespace(final String headerValue, final int from) { + int i = from; + final int length = headerValue.length(); + while (i < length && (headerValue.charAt(i) == ' ' || headerValue.charAt(i) == '\t')) { + i++; + } + return i; + } + + private static boolean isParameterSeparator(final char c) { + return c == ';' || c == ',' || c == ' ' || c == '\t'; + } + + /** + * Finds the next delimiter at or after {@code from}. + * + * @return the index the delimiter starts at, or {@code -1} if there is none + */ + private static int nextDelimiter(final String body, final String delimiter, final int from) { + if (body.startsWith(delimiter, from)) { + return from; + } + for (int newline = body.indexOf('\n', from); + newline >= 0; + newline = body.indexOf('\n', newline + 1)) { + if (body.startsWith(delimiter, newline + 1)) { + return newline + 1; + } + } + return -1; + } + + /** + * Skips the linear whitespace and line break that follow a delimiter. + * + * @return the index the header lines start at, or {@code -1} if no line break follows + */ + private static int lineStart(final String body, final int from) { + int i = from; + final int length = body.length(); + while (i < length && (body.charAt(i) == ' ' || body.charAt(i) == '\t')) { + i++; + } + if (i < length && body.charAt(i) == '\r') { + i++; + } + return i < length && body.charAt(i) == '\n' ? i + 1 : -1; + } + + /** + * Ends a part's content before the line break that introduces the next delimiter, or at the end + * of the body when the closing delimiter was truncated away. + */ + private static int contentEnd(final String body, final int contentStart, final int next) { + if (next < 0) { + return body.length(); + } + int end = next > contentStart ? next - 1 : contentStart; + if (end > contentStart && body.charAt(end - 1) == '\r') { + end--; + } + return end; + } + + private static void addHeader( + final Map headers, final String body, final int from, final int to) { + for (int i = from; i < to; i++) { + if (body.charAt(i) == ':') { + final String name = body.substring(from, i).trim().toLowerCase(Locale.ROOT); + if (!name.isEmpty()) { + headers.put(name, body.substring(i + 1, to).trim()); + } + return; + } + } + // No colon: not a header line, and no obs-fold support, so drop it + } +} diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java index 3395f72c7da..b59a529e07e 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java @@ -2,13 +2,19 @@ import static datadog.trace.lambda.ContentTypeBodyParser.MAX_DEPTH; import static datadog.trace.lambda.ContentTypeBodyParser.MAX_ELEMENTS; +import static datadog.trace.lambda.ContentTypeBodyParser.MAX_PARTS; +import static datadog.trace.lambda.ContentTypeBodyParser.dispatch; import static datadog.trace.lambda.ContentTypeBodyParser.parseBody; +import static datadog.trace.lambda.ContentTypeBodyParser.parseMultipart; import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNull; +import datadog.trace.lambda.ContentTypeBodyParser.ParseContext; import java.util.List; import java.util.Map; import org.junit.jupiter.api.Test; @@ -39,8 +45,9 @@ class ContentTypeBodyParserTest { // urlencoded "a=1 | application/x-www-form-urlencoded | MAP", "a=1 | APPLICATION/X-WWW-FORM-URLENCODED | MAP", - // multipart is not structured yet - "{\"a\":1} | multipart/form-data; boundary=xy | STRING", + // a multipart body whose boundary happens to contain "json" must still reach the multipart + // parser rather than being handed to the JSON parser + "not multipart | multipart/x; boundary=--json | STRING", // text/*: never structured, even when it holds JSON. A JSON parse of "12345" would yield a // Double, which no string rule can match "{\"a\":1} | text/plain | STRING", @@ -89,9 +96,9 @@ void keepsEmptyBodyAsEmptyString() { void keepsRawStringOnceMaxDepthIsReached() { String body = "{\"a\":1}"; - assertEquals(body, ContentTypeBodyParser.dispatch(body, "application/json", MAX_DEPTH)); + assertEquals(body, dispatch(body, "application/json", MAX_DEPTH, new ParseContext())); assertInstanceOf( - Map.class, ContentTypeBodyParser.dispatch(body, "application/json", MAX_DEPTH - 1)); + Map.class, dispatch(body, "application/json", MAX_DEPTH - 1, new ParseContext())); } @Test @@ -152,9 +159,9 @@ void doesNotSeparateUrlEncodedPairsOnSemicolons() { } @Test - void capsUrlEncodedPairsAtMaxElements() { + void parsesUrlEncodedPairsUpToMaxElements() { StringBuilder body = new StringBuilder(); - for (int i = 0; i < MAX_ELEMENTS + 1; i++) { + for (int i = 0; i < MAX_ELEMENTS; i++) { body.append(i == 0 ? "" : "&").append('k').append(i).append("=v"); } @@ -162,7 +169,21 @@ void capsUrlEncodedPairsAtMaxElements() { } @Test - void stopsScanningNamelessUrlEncodedPairsAtTheCap() { + void keepsUrlEncodedBodyOverMaxElementsAsRawString() { + StringBuilder body = new StringBuilder(); + for (int i = 0; i < MAX_ELEMENTS; i++) { + body.append('k').append(i).append("=v&"); + } + body.append("attack=payload"); + String raw = body.toString(); + + // Truncating would hide the trailing parameter from every WAF rule, so the whole body is kept + // as a string instead — still matchable, just unstructured + assertEquals(raw, parseBody(raw, "application/x-www-form-urlencoded")); + } + + @Test + void countsNamelessUrlEncodedPairsTowardsTheCap() { StringBuilder body = new StringBuilder(); for (int i = 0; i < MAX_ELEMENTS; i++) { body.append("=v&"); @@ -170,8 +191,8 @@ void stopsScanningNamelessUrlEncodedPairsAtTheCap() { body.append("a=1"); String raw = body.toString(); - // Nameless pairs still do decoding work, so they exhaust the cap and the trailing parameter is - // never reached; nothing survives, so the raw body is kept + // Nameless pairs still do decoding work, so they count towards the cap even though none of them + // ends up in the map assertEquals(raw, parseBody(raw, "application/x-www-form-urlencoded")); } @@ -181,6 +202,321 @@ void keepsUnparseableUrlEncodedBodyAsRawString() { assertEquals("&&&", parseBody("&&&", "application/x-www-form-urlencoded")); } + @Test + void parsesMultipartFieldsIntoAMap() { + Map fields = multipart(field("user", "admin"), field("role", "root")); + + assertEquals("admin", fields.get("user")); + assertEquals("root", fields.get("role")); + assertEquals(2, fields.size()); + } + + @Test + void skipsMultipartFileParts() { + Map fields = multipart(field("user", "admin"), file("upload", "f.txt", "data")); + + assertEquals("admin", fields.get("user")); + assertEquals(1, fields.size()); + } + + @Test + void skipsMultipartPartsWithAnEmptyFilename() { + // An untouched file input: browsers send filename="", which is a file part all the same + Map fields = multipart(field("user", "admin"), file("upload", "", "")); + + assertEquals(1, fields.size()); + } + + @Test + void keepsAFileOnlyMultipartBodyAsARawString() { + // A raw string still matches string rules; an empty map would tell the WAF the body was empty + String body = outer(file("upload", "f.txt", "data")); + + assertEquals(body, parseBody(body, MULTIPART)); + } + + @Test + void skipsMultipartPartsWithoutAName() { + assertEquals( + outer(part("form-data", "novalue"), part("form-data; name=", "empty")), + parseBody( + outer(part("form-data", "novalue"), part("form-data; name=", "empty")), MULTIPART)); + } + + @Test + void skipsMultipartPartsWithoutAContentDisposition() { + Map fields = multipart(field("user", "admin"), part(null, "orphan")); + + assertEquals(1, fields.size()); + } + + @Test + void dispatchesOnEachMultipartPartsOwnContentType() { + Map fields = + multipart( + part("form-data; name=payload", "application/json", "{\"a\":1}"), + part("form-data; name=plain", "text/plain", "12345"), + part("form-data; name=form", "application/x-www-form-urlencoded", "k=v")); + + assertInstanceOf(Map.class, fields.get("payload")); + assertEquals("12345", fields.get("plain")); + assertEquals(singletonList("v"), ((Map) fields.get("form")).get("k")); + } + + @Test + void promotesRepeatedMultipartFieldNamesToAList() { + Map fields = multipart(field("x", "a"), field("x", "b"), field("x", "c")); + + assertEquals(asList("a", "b", "c"), fields.get("x")); + } + + @Test + void nestsARepeatedJsonArrayPartValueRatherThanFlatteningIt() { + // The first value is itself a List, so a repeat cannot be told from a fresh key by the stored + // value's type: appending to it would hand the WAF a structure that was never sent + Map fields = + multipart(part("form-data; name=x", "application/json", "[1,2]"), field("x", "b")); + + assertEquals(asList(asList(1.0, 2.0), "b"), fields.get("x")); + } + + @Test + void doesNotTakeAFieldNameThatForgesAFilenameForAFilePart() { + Map fields = + multipart(outer(part("form-data; name=\"; filename=x\"", "payload"))); + + assertEquals("payload", fields.get("; filename=x")); + } + + @Test + @SuppressWarnings("unchecked") + void parsesNestedMultipartBodiesAndSkipsTheirFileParts() { + String inner = body("inner", field("nested", "value"), file("upload", "f.txt", "data")); + String nesting = outer(part("form-data; name=group", "multipart/mixed; boundary=inner", inner)); + + Map fields = (Map) parseBody(nesting, MULTIPART); + + assertEquals(singletonMap("nested", "value"), fields.get("group")); + } + + @Test + void sharesTheElementAllowanceAcrossNestingLevels() { + // Two thirds of the allowance each: the first part fits, the second cannot, which only holds if + // both draw from one allowance rather than from a per-part counter + String urlEncoded = urlEncodedPairs(MAX_ELEMENTS * 2 / 3); + Map fields = + multipart( + outer( + part("form-data; name=first", URL_ENCODED, urlEncoded), + part("form-data; name=second", URL_ENCODED, urlEncoded))); + + assertInstanceOf(Map.class, fields.get("first")); + assertEquals(urlEncoded, fields.get("second")); + } + + @Test + void spendsTheElementAllowanceOnlyOnPairsItReads() { + // Just under the allowance twice over would exceed it if the whole body were charged upfront + String urlEncoded = urlEncodedPairs(MAX_ELEMENTS / 2); + Map fields = + multipart( + outer( + part("form-data; name=first", URL_ENCODED, urlEncoded), + part("form-data; name=second", URL_ENCODED, urlEncoded))); + + assertInstanceOf(Map.class, fields.get("first")); + assertInstanceOf(Map.class, fields.get("second")); + } + + private static final String URL_ENCODED = "application/x-www-form-urlencoded"; + + private static String urlEncodedPairs(int count) { + StringBuilder body = new StringBuilder(); + for (int i = 0; i < count; i++) { + body.append(i == 0 ? "" : "&").append('k').append(i).append("=v"); + } + return body.toString(); + } + + @Test + void parsesMultipartPartsUpToThePartAllowance() { + assertEquals(MAX_PARTS, multipart(outer(fieldParts(MAX_PARTS))).size()); + } + + @Test + void keepsMultipartBodyOverThePartAllowanceAsRawString() { + // One part over the allowance drops that part from every WAF address, so the whole body + // degrades to a raw string instead, as an over-allowance urlencoded body does + String body = outer(fieldParts(MAX_PARTS + 1)); + + assertEquals(body, parseBody(body, MULTIPART)); + } + + @Test + void sharesThePartAllowanceAcrossNestingLevels() { + // The inner body alone is within the allowance, but the outer part it sits in has already + // spent one, so it can no longer be read and degrades to a raw string on its own + String inner = body("inner", fieldParts(MAX_PARTS)); + Map fields = + multipart(outer(part("form-data; name=group", "multipart/mixed; boundary=inner", inner))); + + assertEquals(inner, fields.get("group")); + } + + private static String[] fieldParts(int count) { + String[] parts = new String[count]; + for (int i = 0; i < count; i++) { + parts[i] = field("k" + i, "v"); + } + return parts; + } + + @Test + void keepsRawStringOnceMaxDepthIsReachedInAMultipartPart() { + String body = outer(field("user", "admin")); + + assertEquals(body, dispatch(body, MULTIPART, MAX_DEPTH, new ParseContext())); + } + + @Test + void keepsMultipartBodyAsRawStringWithoutAUsableBoundary() { + String body = outer(field("user", "admin")); + + assertEquals(body, parseBody(body, "multipart/form-data")); + assertEquals(body, parseBody(body, "multipart/form-data; boundary=")); + } + + @Test + void keepsUnsplittableMultipartBodyAsRawString() { + assertEquals("no parts here", parseBody("no parts here", MULTIPART)); + } + + @Test + void keepsMultipartBodyAboveTheSizeLimitAsRawString() { + String body = outer(field("user", "admin")); + + assertInstanceOf( + Map.class, parseMultipart(body, MULTIPART, 0, new ParseContext(), body.length())); + assertNull(parseMultipart(body, MULTIPART, 0, new ParseContext(), body.length() - 1)); + } + + @Test + void disablesMultipartParsingWhenTheSizeLimitIsZero() { + String body = outer(field("user", "admin")); + + assertNull(parseMultipart(body, MULTIPART, 0, new ParseContext(), 0)); + } + + @Test + void reportsTheFilenamesOfFileParts() { + String body = + outer( + field("user", "admin"), + file("avatar", "cat.png", "bytes"), + file("doc", "report.pdf", "bytes")); + + assertEquals(asList("cat.png", "report.pdf"), filenamesOf(body)); + // The file parts are not fields, so only the field survives into the body map + assertEquals(singletonMap("user", "admin"), multipart(body)); + } + + @Test + void marksAFilePartWithoutReportingAnEmptyFilename() { + // Browsers send filename="" for an untouched file input: the part is still a file, but the + // empty name gives a rule nothing to match + String body = outer(file("avatar", "", "bytes"), field("user", "admin")); + + assertEquals(emptyList(), filenamesOf(body)); + assertEquals(singletonMap("user", "admin"), multipart(body)); + } + + @Test + void reportsFilenamesFromNestedMultipartParts() { + String inner = body("inner", file("attachment", "nested.txt", "bytes")); + String nesting = outer(part("form-data; name=group", "multipart/mixed; boundary=inner", inner)); + + assertEquals(singletonList("nested.txt"), filenamesOf(nesting)); + } + + @Test + void reportsFilenamesEvenWhenTheBodyDegradesToARawString() { + // Nothing but file parts, so there is no field to report and the body stays a raw string. + // The filenames are then the only structured thing left to hand the WAF. + String body = outer(file("avatar", "cat.png", "bytes")); + + assertEquals(body, parseBody(body, MULTIPART)); + assertEquals(singletonList("cat.png"), filenamesOf(body)); + } + + @Test + void reportsNoFilenameWhenTheMultipartBodyIsNotParsed() { + String body = outer(file("avatar", "cat.png", "bytes")); + + ParseContext sizeCapped = new ParseContext(); + assertNull(parseMultipart(body, MULTIPART, 0, sizeCapped, 0)); + assertEquals(emptyList(), sizeCapped.filenames()); + + ParseContext noBoundary = new ParseContext(); + assertEquals(body, parseBody(body, "multipart/form-data", noBoundary)); + assertEquals(emptyList(), noBoundary.filenames()); + } + + private static List filenamesOf(String body) { + ParseContext context = new ParseContext(); + parseBody(body, MULTIPART, context); + return context.filenames(); + } + + private static final String MULTIPART = "multipart/form-data; boundary=outer"; + + @SuppressWarnings("unchecked") + private static Map multipart(String... parts) { + return multipart(outer(parts)); + } + + @SuppressWarnings("unchecked") + private static Map multipart(String body) { + Object parsed = parseBody(body, MULTIPART); + assertInstanceOf(Map.class, parsed); + return (Map) parsed; + } + + /** Joins the parts with CRLF and appends the close delimiter, using the default boundary. */ + private static String outer(String... parts) { + return body("outer", parts); + } + + private static String body(String boundary, String... parts) { + StringBuilder body = new StringBuilder(); + for (String part : parts) { + body.append("--").append(boundary).append("\r\n").append(part).append("\r\n"); + } + return body.append("--").append(boundary).append("--\r\n").toString(); + } + + private static String field(String name, String value) { + return part("form-data; name=\"" + name + "\"", value); + } + + private static String file(String name, String filename, String value) { + return part("form-data; name=\"" + name + "\"; filename=\"" + filename + "\"", value); + } + + private static String part(String disposition, String value) { + return part(disposition, null, value); + } + + private static String part(String disposition, String contentType, String value) { + StringBuilder part = new StringBuilder(); + if (disposition != null) { + part.append("Content-Disposition: ").append(disposition).append("\r\n"); + } + if (contentType != null) { + part.append("Content-Type: ").append(contentType).append("\r\n"); + } + return part.append("\r\n").append(value).toString(); + } + @SuppressWarnings("unchecked") private static Map> urlEncoded(String body) { Object parsed = parseBody(body, "application/x-www-form-urlencoded"); diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java index 24a20261838..a278fb16bad 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java @@ -724,6 +724,109 @@ void parsesUrlEncodedBodyIntoAMultimap() { assertEquals(Arrays.asList("root"), parameters.get("role")); } + @Test + @SuppressWarnings("unchecked") + void parsesMultipartBodyIntoItsFields() { + String eventJson = + "{" + + "\"body\": \"--xy\\r\\nContent-Disposition: form-data; name=\\\"user\\\"\\r\\n\\r\\nadmin" + + "\\r\\n--xy\\r\\nContent-Disposition: form-data; name=\\\"role\\\"\\r\\n\\r\\nroot" + + "\\r\\n--xy--\"," + + "\"headers\": {\"Content-Type\": \"multipart/form-data; boundary=xy\"}," + + "\"requestContext\": {\"httpMethod\": \"POST\"}" + + "}"; + ByteArrayInputStream event = createInputStream(eventJson); + + Object[] capturedBody = {null}; + + setupMockCallbacks(new Callbacks().onBody(body -> capturedBody[0] = body)); + + AgentSpanContext result = LambdaAppSecHandler.processRequestStart(event); + + assertNotNull(result); + assertInstanceOf(Map.class, capturedBody[0]); + Map fields = (Map) capturedBody[0]; + assertEquals("admin", fields.get("user")); + assertEquals("root", fields.get("role")); + } + + @Test + @SuppressWarnings("unchecked") + void reportsMultipartFilenamesToTheWaf() { + String eventJson = + "{" + + "\"body\": \"--xy\\r\\nContent-Disposition: form-data; name=\\\"user\\\"\\r\\n\\r\\nadmin" + + "\\r\\n--xy\\r\\nContent-Disposition: form-data; name=\\\"avatar\\\";" + + " filename=\\\"cat.png\\\"\\r\\n\\r\\nbytes" + + "\\r\\n--xy--\"," + + "\"headers\": {\"Content-Type\": \"multipart/form-data; boundary=xy\"}," + + "\"requestContext\": {\"httpMethod\": \"POST\"}" + + "}"; + ByteArrayInputStream event = createInputStream(eventJson); + + Object[] capturedBody = {null}; + Object[] capturedFilenames = {null}; + + setupMockCallbacks( + new Callbacks() + .onBody(body -> capturedBody[0] = body) + .onFilenames(filenames -> capturedFilenames[0] = filenames)); + + AgentSpanContext result = LambdaAppSecHandler.processRequestStart(event); + + assertNotNull(result); + assertEquals(Arrays.asList("cat.png"), capturedFilenames[0]); + // The file part is reported by name only: it is not a field, and its content is left out + Map fields = (Map) capturedBody[0]; + assertEquals("admin", fields.get("user")); + assertNull(fields.get("avatar")); + } + + @Test + void doesNotReportFilenamesForAMultipartBodyWithoutFileParts() { + String eventJson = + "{" + + "\"body\": \"--xy\\r\\nContent-Disposition: form-data; name=\\\"user\\\"\\r\\n\\r\\nadmin" + + "\\r\\n--xy--\"," + + "\"headers\": {\"Content-Type\": \"multipart/form-data; boundary=xy\"}," + + "\"requestContext\": {\"httpMethod\": \"POST\"}" + + "}"; + ByteArrayInputStream event = createInputStream(eventJson); + + Object[] capturedFilenames = {null}; + + setupMockCallbacks(new Callbacks().onFilenames(filenames -> capturedFilenames[0] = filenames)); + + AgentSpanContext result = LambdaAppSecHandler.processRequestStart(event); + + assertNotNull(result); + assertNull(capturedFilenames[0]); + } + + @Test + void keepsMultipartBodyWithoutBoundaryAsRawString() { + String body = + "--xy\\r\\nContent-Disposition: form-data; name=user\\r\\n\\r\\nadmin\\r\\n--xy--"; + String eventJson = + "{" + + "\"body\": \"" + + body + + "\"," + + "\"headers\": {\"Content-Type\": \"multipart/form-data\"}," + + "\"requestContext\": {\"httpMethod\": \"POST\"}" + + "}"; + ByteArrayInputStream event = createInputStream(eventJson); + + Object[] capturedBody = {null}; + + setupMockCallbacks(new Callbacks().onBody(b -> capturedBody[0] = b)); + + AgentSpanContext result = LambdaAppSecHandler.processRequestStart(event); + + assertNotNull(result); + assertEquals(body.replace("\\r\\n", "\r\n"), capturedBody[0]); + } + @Test @SuppressWarnings("unchecked") void appliesContentTypeDispatchToBase64DecodedBodies() { @@ -2433,6 +2536,7 @@ private static class Callbacks { BiConsumer onSocketAddress; Consumer> onPathParams; Consumer onBody; + Consumer> onFilenames; Callbacks onMethodUri(BiConsumer cb) { this.onMethodUri = cb; @@ -2458,6 +2562,11 @@ Callbacks onBody(Consumer cb) { this.onBody = cb; return this; } + + Callbacks onFilenames(Consumer> cb) { + this.onFilenames = cb; + return this; + } } @SuppressWarnings("unchecked") @@ -2534,6 +2643,19 @@ private void setupMockCallbacks(Callbacks callbacks) { .apply(any(), any()); } + BiFunction, Flow> filenamesCallback = null; + if (callbacks.onFilenames != null) { + filenamesCallback = mock(BiFunction.class); + Consumer> capture = callbacks.onFilenames; + doAnswer( + inv -> { + capture.accept(inv.getArgument(1)); + return Flow.ResultFlow.empty(); + }) + .when(filenamesCallback) + .apply(any(), any()); + } + CallbackProvider mockCallbackProvider = mock(CallbackProvider.class); when(mockCallbackProvider.getCallback(EVENTS.requestStarted())) .thenReturn(requestStartedCallback); @@ -2547,6 +2669,8 @@ private void setupMockCallbacks(Callbacks callbacks) { when(mockCallbackProvider.getCallback(EVENTS.requestPathParams())) .thenReturn(pathParamsCallback); when(mockCallbackProvider.getCallback(EVENTS.requestBodyProcessed())).thenReturn(bodyCallback); + when(mockCallbackProvider.getCallback(EVENTS.requestFilesFilenames())) + .thenReturn(filenamesCallback); AgentTracer.TracerAPI mockTracer = mock(AgentTracer.TracerAPI.class); when(mockTracer.getCallbackProvider(RequestContextSlot.APPSEC)) diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java new file mode 100644 index 00000000000..9cd24a74342 --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java @@ -0,0 +1,233 @@ +package datadog.trace.lambda; + +import static datadog.trace.lambda.MultipartSplitter.extractBoundary; +import static datadog.trace.lambda.MultipartSplitter.parameter; +import static datadog.trace.lambda.MultipartSplitter.split; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.trace.lambda.MultipartSplitter.Part; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +class MultipartSplitterTest { + + private static final int NO_PART_BUDGET_LIMIT = 256; + + @ParameterizedTest(name = "[{index}] {0} -> {1}") + @CsvSource( + delimiter = '|', + nullValues = "NULL", + value = { + // case is preserved: the boundary is matched byte-for-byte against the body + "multipart/form-data; boundary=AbC123 | AbC123", + // the parameter name is not + "multipart/form-data; BOUNDARY=xy | xy", + // quoted values may hold separators + "multipart/form-data; boundary=\"a;b c\" | a;b c", + // position among the other parameters does not matter + "multipart/form-data; boundary=xy; charset=x | xy", + "multipart/form-data; charset=x; boundary=xy | xy", + // a parameter that merely ends in "boundary" is not one + "multipart/form-data; xboundary=xy | NULL", + "multipart/form-data | NULL", + "multipart/form-data; boundary= | NULL", + "NULL | NULL", + }) + void extractsTheBoundary(String contentType, String expected) { + assertEquals(expected, extractBoundary(contentType)); + } + + @Test + void rejectsABoundaryOverSeventyCharacters() { + String maximum = repeat('a', 70); + + assertEquals(maximum, extractBoundary("multipart/form-data; boundary=" + maximum)); + assertNull(extractBoundary("multipart/form-data; boundary=" + maximum + "a")); + } + + @ParameterizedTest(name = "[{index}] {1} of {0} -> {2}") + @CsvSource( + delimiter = '|', + nullValues = "NULL", + value = { + "form-data; name=user | name | user", + "form-data; name=\"user\" | name | user", + "form-data; name=\"a;b\" | name | a;b", + "form-data; NAME=user | name | user", + "form-data; name=user; charset=utf-8| name | user", + // "filename" must not answer a lookup for "name": the match has to start at a parameter + "form-data; filename=\"f\"; name=u | name | u", + // present but empty, which is what browsers send for an untouched file input + "form-data; filename=\"\" | filename | ''", + "form-data; filename= | filename | ''", + "form-data; name=user | filename | NULL", + "NULL | name | NULL", + // a quoted value cannot forge a parameter boundary: the quoted span is skipped whole, so + // this field is not mistaken for a file part and dropped + "form-data; name=\"; filename=x\" | filename | NULL", + "form-data; name=\"; filename=x\" | name | '; filename=x'", + // and the same aliasing the other way round does not rename the field + "form-data; filename=\"; name=y\" | name | NULL", + // RFC 7230 optional whitespace is tolerated on both sides of the "=" + "form-data; name =user | name | user", + "form-data; name= user | name | user", + "form-data; name = \"a b\" | name | a b", + // a bare parameter name is not a parameter + "form-data; name | name | NULL", + }) + void readsParameters(String headerValue, String paramName, String expected) { + assertEquals(expected, parameter(headerValue, paramName)); + } + + @Test + void unescapesQuotedParameterValues() { + assertEquals("a\"b", parameter("form-data; name=\"a\\\"b\"", "name")); + } + + @Test + void toleratesTabsAroundTheParameterEquals() { + assertEquals("user", parameter("form-data;\tname\t=\tuser", "name")); + } + + @Test + void keepsWhatWasReadOfAnUnterminatedQuotedValue() { + assertEquals("abc", parameter("form-data; name=\"abc", "name")); + assertEquals("a\"", parameter("form-data; name=\"a\\\"", "name")); + } + + @ParameterizedTest(name = "[{index}] {0} -> {2} part(s)") + @CsvSource( + delimiter = '|', + value = { + "happy path | --x@A: b@@v@--x-- | 1", + "two parts | --x@@a@--x@@b@--x-- | 2", + "preamble is discarded | junk@--x@@v@--x-- | 1", + "epilogue is discarded | --x@@v@--x--@junk | 1", + // a truncated body still yields its last part + "truncated last delimiter | --x@A: b@@v | 1", + "truncated in the headers | --x@A: b | 0", + "no line break at all | --x | 0", + "close delimiter only | --x-- | 0", + "empty part content | --x@A: b@@ | 1", + "part without headers | --x@@v@--x-- | 1", + "unrelated delimiter | --y@@v@--y-- | 0", + }) + void splitsBodies(String name, String template, int expectedParts) { + assertEquals( + expectedParts, split(template.replace("@", "\r\n"), "x", NO_PART_BUDGET_LIMIT).size()); + } + + @Test + void toleratesBareLineFeeds() { + String body = "--x\nContent-Disposition: form-data; name=a\n\nvalue\n--x--\n"; + + List parts = split(body, "x", NO_PART_BUDGET_LIMIT); + + assertEquals(1, parts.size()); + assertEquals("value", content(body, parts.get(0))); + } + + @Test + void stopsAtThePartBudget() { + String body = ("--x\r\n\r\na\r\n--x\r\n\r\nb\r\n--x\r\n\r\nc\r\n--x--"); + + assertEquals(2, split(body, "x", 2).size()); + assertEquals(0, split(body, "x", 0).size()); + } + + @Test + void delimitsContentExactly() { + // Dashes, line breaks, a replacement character and a multi-byte character all inside the + // content: + // the reported range must not be thrown off by a near-miss on the line-feed anchor + String content = "--not-a-boundary\r\n-x\nlast�é"; + String body = + "--x\r\nContent-Disposition: form-data; name=a\r\n\r\n" + content + "\r\n--x--\r\n"; + + List parts = split(body, "x", NO_PART_BUDGET_LIMIT); + + assertEquals(1, parts.size()); + assertEquals(content, content(body, parts.get(0))); + assertEquals("form-data; name=a", parts.get(0).header("content-disposition")); + } + + @Test + void confinesAPartWhoseHeadersRunIntoTheNextDelimiter() { + // The first part's headers are not followed by a blank line. Reading past the delimiter would + // merge the following part's headers into this one, so a well-formed field part would be + // reported as the malformed part's own — and skipped entirely if it carried a filename. + String body = + "--x\r\nX-First: 1\r\n" + + "--x\r\nContent-Disposition: form-data; name=\"b\"\r\n\r\nsecond\r\n--x--"; + + List parts = split(body, "x", NO_PART_BUDGET_LIMIT); + + assertEquals(1, parts.size()); + assertNull(parts.get(0).header("x-first")); + assertEquals("form-data; name=\"b\"", parts.get(0).header("content-disposition")); + assertEquals("second", content(body, parts.get(0))); + } + + @Test + void lowercasesHeaderNamesAndTrimsValues() { + String body = "--x\r\nCONTENT-Disposition: form-data; name=a \r\nX:\r\n\r\nv\r\n--x--"; + + Part part = split(body, "x", NO_PART_BUDGET_LIMIT).get(0); + + assertEquals("form-data; name=a", part.header("content-disposition")); + assertEquals("", part.header("x")); + assertNull(part.header("absent")); + } + + @Test + @Timeout(value = 10, unit = SECONDS) + void returnsPromptlyOnAnAdversarialBoundary() { + // Dashes are legal boundary characters, so a scan seeded on '-' would be quadratic here. + // Anchored + // on the mandatory line feed, of which this body has none, the scan is linear. + String boundary = repeat('-', 69) + "X"; + String body = repeat('-', 1_000_000); + + assertTrue(split(body, boundary, NO_PART_BUDGET_LIMIT).isEmpty()); + } + + @Test + @Timeout(value = 30, unit = SECONDS) + void neverThrowsOnAMutatedBody() { + String body = + "preamble\r\n--x\r\nContent-Disposition: form-data; name=\"a\"\r\n" + + "Content-Type: application/json\r\n\r\n{\"k\":1}\r\n" + + "--x\r\nContent-Disposition: form-data; name=b; filename=\"f\"\r\n\r\nfile\r\n" + + "--x--\r\nepilogue"; + // Fixed positions rather than a random seed, so a failure is reproducible + char[] substitutes = {'-', '\r', '\n', ':', ';', '"', '\\', '=', '\0', '�'}; + + for (int i = 0; i <= body.length(); i++) { + split(body.substring(0, i), "x", NO_PART_BUDGET_LIMIT); + for (char substitute : substitutes) { + if (i < body.length()) { + split( + body.substring(0, i) + substitute + body.substring(i + 1), "x", NO_PART_BUDGET_LIMIT); + } + } + } + } + + private static String content(String body, Part part) { + return body.substring(part.contentStart, part.contentEnd); + } + + private static String repeat(char c, int count) { + StringBuilder builder = new StringBuilder(count); + for (int i = 0; i < count; i++) { + builder.append(c); + } + return builder.toString(); + } +} From 3ea96fbbdb44a819ee27aa9d75a55e3de5abeb0f Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Fri, 28 Aug 2026 18:21:14 +0200 Subject: [PATCH 03/10] Keep multipart parts that declare no content type as raw strings Co-Authored-By: Claude Opus 5 --- .../trace/lambda/ContentTypeBodyParser.java | 15 +++++++------- .../trace/lambda/LambdaEventParser.java | 5 +++-- .../lambda/ContentTypeBodyParserTest.java | 20 +++++++++++++++++++ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java index b2e08f4e724..6b82fa6eca1 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java @@ -18,8 +18,8 @@ /** * Turns a Lambda request body into the shape the AppSec WAF expects. The declared {@code - * Content-Type} decides how the body is structured; a best-effort JSON parse handles both the - * JSON-ish types and the case where no type is declared at all. + * Content-Type} decides how the body is structured; a best-effort JSON parse handles the JSON-ish + * types and a top-level body that declares no type at all. * *

A body is never dropped: any type we cannot structure — and any parse failure — degrades to * the raw {@link String}, which the WAF can still match string rules against. @@ -230,12 +230,13 @@ static Object parseMultipart( if (name == null || name.isEmpty()) { continue; } + final String partContentType = part.header("content-type"); + final String content = body.substring(part.contentStart, part.contentEnd); + // A part that declares no type is kept as a raw string final Object value = - dispatch( - body.substring(part.contentStart, part.contentEnd), - part.header("content-type"), - depth + 1, - context); + partContentType == null || partContentType.trim().isEmpty() + ? content + : dispatch(content, partContentType, depth + 1, context); addField(fields, promoted, name, value); } return fields.isEmpty() ? null : fields; diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java index c918ea97e17..b619212c534 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java @@ -95,8 +95,9 @@ static LambdaRequestData parseEvent(String json) { case ALB_MULTI_VALUE: return extractAlbData(event, triggerType); default: - // Unsupported trigger: AppSec skips the invocation entirely, so there is nothing to - // extract. The trigger type is carried by the caller, not by this result. + // Unsupported trigger: returning EMPTY makes the caller skip the invocation, so there is + // nothing to extract. The caller already recorded UNKNOWN as the trigger type, which is + // what reports the unsupported event at request end. return LambdaRequestData.EMPTY; } } catch (Exception e) { diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java index b59a529e07e..c46743aa376 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java @@ -263,6 +263,26 @@ void dispatchesOnEachMultipartPartsOwnContentType() { assertEquals(singletonList("v"), ((Map) fields.get("form")).get("k")); } + @Test + void keepsMultipartPartsThatDeclareNoContentTypeAsRawStrings() { + // A part with no Content-Type is text/plain per RFC 7578, section 4.4, not a body of unknown + // type: a JSON parse would hand the WAF a Double, a Boolean and a Map, so the same form + // submitted urlencoded and as multipart would no longer match the same string rules + Map fields = + multipart(field("amount", "12345"), field("flag", "true"), field("json", "{\"a\":1}")); + + assertEquals("12345", fields.get("amount")); + assertEquals("true", fields.get("flag")); + assertEquals("{\"a\":1}", fields.get("json")); + } + + @Test + void keepsMultipartPartsWithAnEmptyContentTypeAsRawStrings() { + Map fields = multipart(outer(part("form-data; name=\"amount\"", "", "12345"))); + + assertEquals("12345", fields.get("amount")); + } + @Test void promotesRepeatedMultipartFieldNamesToAList() { Map fields = multipart(field("x", "a"), field("x", "b"), field("x", "c")); From ebad5227cda4e44f98fa575fa00ce4cc7c017ecc Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Tue, 1 Sep 2026 17:24:43 +0200 Subject: [PATCH 04/10] Harden Lambda AppSec body parsing allowances and multipart header reads Co-Authored-By: Claude Opus 5 --- .../trace/lambda/ContentTypeBodyParser.java | 145 ++++++++----- .../trace/lambda/LambdaAppSecHandler.java | 2 +- .../trace/lambda/LambdaEventParser.java | 45 +--- .../trace/lambda/MultipartSplitter.java | 90 +++++--- .../lambda/ContentTypeBodyParserTest.java | 193 ++++++++---------- .../trace/lambda/LambdaAppSecHandlerTest.java | 28 +++ .../trace/lambda/MultipartSplitterTest.java | 53 +++-- 7 files changed, 324 insertions(+), 232 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java index 6b82fa6eca1..db75ff9afe0 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java @@ -9,7 +9,6 @@ import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; -import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.StringTokenizer; @@ -29,22 +28,38 @@ final class ContentTypeBodyParser { private static final Logger log = LoggerFactory.getLogger(ContentTypeBodyParser.class); // These bound the work done in this parser only. Exceeding any of them degrades the body to a raw - // string rather than dropping content, and the WAF truncates its inputs independently anyway. - static final int MAX_DEPTH = 20; - static final int MAX_ELEMENTS = 256; + // string rather than dropping content. + // + // The depth and part limits mirror the WAF's own (WAFModule.MAX_DEPTH / MAX_ELEMENTS): structure + // beyond them is discarded by ObjectIntrospection before the WAF ever sees it, so producing it + // would be wasted work. The byte allowance answers a different question — how much reading and + // copying one event may cost us — and AWS already caps a synchronous payload at 6 MiB. + static final int MAX_BYTES = 1024 * 1024; static final int MAX_PARTS = 256; - static final int MAX_MULTIPART_SIZE = 1_000_000; + static final int MAX_DEPTH = 20; private ContentTypeBodyParser() {} /** - * State shared across a whole parse: the element and part allowances, and the filenames collected + * State shared across a whole parse: the byte and part allowances, and the filenames collected * along the way. A multipart part may itself hold an urlencoded or multipart body, so a per-call - * cap would multiply across nesting levels and a per-call list would lose nested file parts. + * allowance would be re-satisfied at every nesting level and a per-call list would lose nested + * file parts. */ static final class ParseContext { + private int bytes; private int parts = MAX_PARTS; - private int elements = MAX_ELEMENTS; + + ParseContext() { + this(MAX_BYTES); + } + + /** + * @param byteAllowance the total number of characters this parse may read, nesting included + */ + ParseContext(final int byteAllowance) { + this.bytes = byteAllowance; + } /** Allocated only once a file part is seen, which most bodies never do. */ private List filenames; @@ -58,13 +73,26 @@ void consumeParts(final int count) { } /** - * @return {@code false} once the element allowance is spent + * @return {@code false} when the parse can no longer afford to read {@code count} characters + */ + boolean takeBytes(final int count) { + if (bytes < count) { + return false; + } + bytes -= count; + return true; + } + + /** + * @return {@code false} once the part allowance is spent. A multipart part and an urlencoded + * parameter both draw from it: they are the same question, how many values one body may + * yield. */ - boolean takeElement() { - if (elements == 0) { + boolean takePart() { + if (parts == 0) { return false; } - elements--; + parts--; return true; } @@ -84,11 +112,6 @@ List filenames() { } } - /** Parses a decoded request body according to its {@code Content-Type}. */ - static Object parseBody(final String body, final String contentType) { - return parseBody(body, contentType, new ParseContext()); - } - /** * Parses a decoded request body according to its {@code Content-Type}. * @@ -108,18 +131,24 @@ static Object dispatch( log.debug("Body nesting depth {} reached, keeping raw string", depth); return body; } - if (contentType == null || contentType.trim().isEmpty() || isJsonLike(contentType)) { - final Object parsed = LambdaEventParser.parseBodyAsJson(body); - return parsed != null ? parsed : body; + if (!context.takeBytes(body.length())) { + log.debug( + "Byte allowance cannot cover a body of {} chars, keeping raw string", body.length()); + return body; } final MediaType mediaType = MediaType.parse(contentType); + // A null type is one MediaType could not read: the header was absent, blank, or nothing but + // parameters. Such a body gets the same best-effort JSON parse as a JSON-ish one. + if (mediaType.getType() == null || isJsonLike(mediaType)) { + return jsonOrRaw(body); + } if ("application".equals(mediaType.getType()) && "x-www-form-urlencoded".equals(mediaType.getSubtype())) { final Object parsed = parseUrlEncoded(body, context); return parsed != null ? parsed : body; } if ("multipart".equals(mediaType.getType())) { - final Object parsed = parseMultipart(body, contentType, depth, context, MAX_MULTIPART_SIZE); + final Object parsed = parseMultipart(body, contentType, depth, context); return parsed != null ? parsed : body; } // text/* and everything else stay raw strings. In particular a text/plain body of "12345" must @@ -127,18 +156,34 @@ static Object dispatch( return body; } - static boolean isJsonLike(final String contentType) { - if (contentType == null) { - return false; - } - // Match on the type and subtype only. Parameters such as a multipart boundary are chosen by - // the client, so "multipart/form-data; boundary=--json" must not reach the JSON parser and - // thereby skip multipart parsing entirely. - final int semicolon = contentType.indexOf(';'); - final String essence = - (semicolon == -1 ? contentType : contentType.substring(0, semicolon)) - .toLowerCase(Locale.ROOT); - return essence.contains("json") || essence.contains("javascript"); + /** + * Applies the "no declared type, or a JSON-ish one" rule to a body the caller does not structure + * any further. This is the whole of how a response body is handled; the request path shares the + * rule through {@link #dispatch} and additionally structures urlencoded and multipart bodies. + */ + static Object jsonOrRaw(final String body, final String contentType) { + final MediaType mediaType = MediaType.parse(contentType); + return mediaType.getType() == null || isJsonLike(mediaType) ? jsonOrRaw(body) : body; + } + + /** A best-effort JSON parse, degrading to the raw string rather than dropping the body. */ + private static Object jsonOrRaw(final String body) { + final Object parsed = LambdaEventParser.parseBodyAsJson(body); + return parsed != null ? parsed : body; + } + + /** + * Matches on the type and subtype only. Parameters such as a multipart boundary are chosen by the + * client, so {@code multipart/form-data; boundary=--json} must not reach the JSON parser and + * thereby skip multipart parsing entirely; {@link MediaType#parse} strips them, and lowercases + * what is left. + */ + private static boolean isJsonLike(final MediaType mediaType) { + return jsonLike(mediaType.getType()) || jsonLike(mediaType.getSubtype()); + } + + private static boolean jsonLike(final String essence) { + return essence != null && (essence.contains("json") || essence.contains("javascript")); } /** @@ -146,7 +191,7 @@ static boolean isJsonLike(final String contentType) { * produced for query parameters. * * @return the parsed parameters, or {@code null} if nothing usable was found or the body exhausts - * the element allowance + * the part allowance */ private static Map> parseUrlEncoded( final String body, final ParseContext context) { @@ -156,10 +201,10 @@ private static Map> parseUrlEncoded( final Map> parameters = new LinkedHashMap<>(); final StringTokenizer tokenizer = new StringTokenizer(body, "&"); while (tokenizer.hasMoreTokens()) { - if (!context.takeElement()) { + if (!context.takePart()) { // Bail out rather than hand the WAF a truncated map: a parameter dropped here would be // invisible to every rule, whereas the raw body can still be string-matched. - log.debug("Element allowance exhausted, keeping urlencoded body as a raw string"); + log.debug("Part allowance exhausted, keeping urlencoded body as a raw string"); return null; } final String pair = tokenizer.nextToken(); @@ -181,20 +226,11 @@ private static Map> parseUrlEncoded( /** * Parses a {@code multipart/*} body into its form fields. * - * @param sizeLimit the largest body to attempt, or {@code 0} to disable multipart parsing - * @return the fields found, or {@code null} if the body is too large, has no usable boundary, - * holds more parts than the allowance, or yields no field + * @return the fields found, or {@code null} if the body has no usable boundary, it holds more + * parts than the allowance, or it yields no field */ - static Object parseMultipart( - final String body, - final String contentType, - final int depth, - final ParseContext context, - final int sizeLimit) { - if (sizeLimit <= 0 || body.length() > sizeLimit) { - log.debug("Multipart body of {} chars not parsed, keeping raw string", body.length()); - return null; - } + private static Object parseMultipart( + final String body, final String contentType, final int depth, final ParseContext context) { final String boundary = MultipartSplitter.extractBoundary(contentType); if (boundary == null) { log.debug("Multipart body without a usable boundary, keeping raw string"); @@ -215,7 +251,7 @@ static Object parseMultipart( final Map fields = new LinkedHashMap<>(); final Set promoted = new HashSet<>(); for (final Part part : parts) { - final String disposition = part.header("content-disposition"); + final String disposition = part.contentDisposition; if (disposition == null) { continue; } @@ -230,11 +266,14 @@ static Object parseMultipart( if (name == null || name.isEmpty()) { continue; } - final String partContentType = part.header("content-type"); + // Already trimmed by MultipartSplitter, so a whitespace-only header arrives as "" + final String partContentType = part.contentType; final String content = body.substring(part.contentStart, part.contentEnd); - // A part that declares no type is kept as a raw string + // A part that declares no type is kept as a raw string, the opposite of what a whole body + // with no type gets: RFC 7578, section 4.4 defaults a part to text/plain rather than leaving + // its type unknown. final Object value = - partContentType == null || partContentType.trim().isEmpty() + partContentType == null || partContentType.isEmpty() ? content : dispatch(content, partContentType, depth + 1, context); addField(fields, promoted, name, value); diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaAppSecHandler.java b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaAppSecHandler.java index bf1c05d692c..3aae3404281 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaAppSecHandler.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaAppSecHandler.java @@ -59,7 +59,7 @@ public class LambdaAppSecHandler { private static final RatelimitedLogger rlLog = new RatelimitedLogger(log, 5, TimeUnit.MINUTES); /** - * Marks an invocation AppSec did not process because the trigger is not HTTP, or if the even is + * Marks an invocation AppSec did not process because the trigger is not HTTP, or if the event is * unreadable (not a {@code ByteArrayInputStream}, empty, oversized, or unparseable). */ private static final String UNSUPPORTED_EVENT_TYPE_METRIC = "_dd.appsec.unsupported_event_type"; diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java index b619212c534..03258888dd2 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java @@ -162,15 +162,10 @@ static LambdaResponseData parseResponse(String json) { } if (bodyString != null) { - String contentType = headers.get("content-type"); - - // If JSON content-type or unknown, attempt JSON parsing. - if (contentType == null || ContentTypeBodyParser.isJsonLike(contentType)) { - Object parsed = parseBodyAsJson(bodyString); - body = parsed != null ? parsed : bodyString; - } else { - body = bodyString; - } + // A response body is only ever structured as JSON, never as urlencoded or multipart, but + // the rule deciding whether to try is shared with the request path so the two cannot + // drift + body = ContentTypeBodyParser.jsonOrRaw(bodyString, headers.get("content-type")); } } @@ -782,33 +777,11 @@ static class LambdaRequestData { LambdaTriggerType.UNKNOWN, Collections.emptyMap(), Collections.emptyMap(), - null); - - LambdaRequestData( - Map headers, - String method, - String path, - String sourceIp, - Integer sourcePort, - LambdaTriggerType triggerType, - Map pathParameters, - Map> queryParameters, - Object body) { - this( - headers, - method, - path, - sourceIp, - sourcePort, - triggerType, - pathParameters, - queryParameters, - body, - null, - null, - null, - Collections.emptyList()); - } + null, + null, + null, + null, + Collections.emptyList()); LambdaRequestData( Map headers, diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java index 60ebd156f3d..a28affa7f78 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java @@ -1,10 +1,7 @@ package datadog.trace.lambda; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; -import java.util.Locale; -import java.util.Map; /** * Splits a {@code multipart/*} body into its parts and reads {@code Content-Type} / {@code @@ -20,22 +17,23 @@ private MultipartSplitter() {} * One part of a multipart body. Content is an index range into the body passed to {@link #split}. */ static final class Part { - private final Map headers; + /** {@code null} when the part declares no such header. Duplicates keep the last seen value. */ + final String contentDisposition; + + final String contentType; final int contentStart; final int contentEnd; - private Part(final Map headers, final int contentStart, final int contentEnd) { - this.headers = headers; + private Part( + final String contentDisposition, + final String contentType, + final int contentStart, + final int contentEnd) { + this.contentDisposition = contentDisposition; + this.contentType = contentType; this.contentStart = contentStart; this.contentEnd = contentEnd; } - - /** - * @param lowercaseName the header name, lowercased by the caller - */ - String header(final String lowercaseName) { - return headers.get(lowercaseName); - } } /** @@ -64,7 +62,10 @@ static List split(final String body, final String boundary, final int part position = nextDelimiter(body, delimiter, afterDelimiter); continue; } - final Map headers = new HashMap<>(4); + // Only these two headers are ever read, so the others are matched and dropped rather than + // collected: a part declaring thousands of them costs nothing but the scan. + String contentDisposition = null; + String contentType = null; int cursor = headerStart; boolean headersComplete = false; int malformedPartEnd = -1; @@ -85,7 +86,15 @@ static List split(final String body, final String boundary, final int part cursor = newline < 0 ? length : newline + 1; break; } - addHeader(headers, body, cursor, trimmed); + final String disposition = headerValue(body, cursor, trimmed, "Content-Disposition"); + if (disposition != null) { + contentDisposition = disposition; + } else { + final String type = headerValue(body, cursor, trimmed, "Content-Type"); + if (type != null) { + contentType = type; + } + } if (newline < 0) { cursor = length; break; @@ -103,7 +112,7 @@ static List split(final String body, final String boundary, final int part break; } final int next = nextDelimiter(body, delimiter, cursor); - parts.add(new Part(headers, cursor, contentEnd(body, cursor, next))); + parts.add(new Part(contentDisposition, contentType, cursor, contentEnd(body, cursor, next))); position = next; } return parts; @@ -206,7 +215,7 @@ private static String value(final String headerValue, final int from) { private static int skipOptionalWhitespace(final String headerValue, final int from) { int i = from; final int length = headerValue.length(); - while (i < length && (headerValue.charAt(i) == ' ' || headerValue.charAt(i) == '\t')) { + while (i < length && isOptionalWhitespace(headerValue.charAt(i))) { i++; } return i; @@ -243,7 +252,7 @@ private static int nextDelimiter(final String body, final String delimiter, fina private static int lineStart(final String body, final int from) { int i = from; final int length = body.length(); - while (i < length && (body.charAt(i) == ' ' || body.charAt(i) == '\t')) { + while (i < length && isOptionalWhitespace(body.charAt(i))) { i++; } if (i < length && body.charAt(i) == '\r') { @@ -267,17 +276,46 @@ private static int contentEnd(final String body, final int contentStart, final i return end; } - private static void addHeader( - final Map headers, final String body, final int from, final int to) { + /** + * Reads one header line, without allocating unless the name is the one asked for. + * + * @param from the first index of the line, {@code to} the index past its last character, the + * trailing {@code \r} already excluded + * @param name the header name to match, case-insensitively + * @return the trimmed header value, or {@code null} when the line declares another header or is + * not a header line at all — there is no colon, and obs-fold is unsupported + */ + private static String headerValue( + final String body, final int from, final int to, final String name) { + int colon = -1; for (int i = from; i < to; i++) { if (body.charAt(i) == ':') { - final String name = body.substring(from, i).trim().toLowerCase(Locale.ROOT); - if (!name.isEmpty()) { - headers.put(name, body.substring(i + 1, to).trim()); - } - return; + colon = i; + break; } } - // No colon: not a header line, and no obs-fold support, so drop it + if (colon < 0) { + return null; + } + int start = from; + int end = colon; + while (start < end && isOptionalWhitespace(body.charAt(start))) { + start++; + } + while (end > start && isOptionalWhitespace(body.charAt(end - 1))) { + end--; + } + // The canonical spelling is tried first: its comparison is intrinsified where the + // case-insensitive one walks char by char, and it is what every real client sends. + if (end - start != name.length() + || !(body.regionMatches(start, name, 0, name.length()) + || body.regionMatches(true, start, name, 0, name.length()))) { + return null; + } + return body.substring(colon + 1, to).trim(); + } + + private static boolean isOptionalWhitespace(final char c) { + return c == ' ' || c == '\t'; } } diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java index c46743aa376..768d1bbb980 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java @@ -1,11 +1,8 @@ package datadog.trace.lambda; import static datadog.trace.lambda.ContentTypeBodyParser.MAX_DEPTH; -import static datadog.trace.lambda.ContentTypeBodyParser.MAX_ELEMENTS; import static datadog.trace.lambda.ContentTypeBodyParser.MAX_PARTS; import static datadog.trace.lambda.ContentTypeBodyParser.dispatch; -import static datadog.trace.lambda.ContentTypeBodyParser.parseBody; -import static datadog.trace.lambda.ContentTypeBodyParser.parseMultipart; import static java.util.Arrays.asList; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; @@ -20,11 +17,12 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; -import org.junit.jupiter.params.provider.NullAndEmptySource; -import org.junit.jupiter.params.provider.ValueSource; class ContentTypeBodyParserTest { + private static final String MULTIPART = "multipart/form-data; boundary=outer"; + private static final String URL_ENCODED = "application/x-www-form-urlencoded"; + @ParameterizedTest(name = "[{index}] {1} -> {2}") @CsvSource( delimiter = '|', @@ -32,7 +30,10 @@ class ContentTypeBodyParserTest { // content type absent or blank: best-effort JSON, as before content-type dispatch existed "{\"a\":1} | | MAP", "{\"a\":1} | ' ' | MAP", + "{\"a\":1} | '' | MAP", "not json | | STRING", + // a header holding nothing but parameters declares no type either + "{\"a\":1} | '; charset=utf-8' | MAP", // JSON, including suffixed subtypes and parameters "{\"a\":1} | application/json | MAP", "{\"a\":1} | application/json; charset=utf-8 | MAP", @@ -48,6 +49,8 @@ class ContentTypeBodyParserTest { // a multipart body whose boundary happens to contain "json" must still reach the multipart // parser rather than being handed to the JSON parser "not multipart | multipart/x; boundary=--json | STRING", + // a json-ish subtype wins over the multipart branch, whatever the type says + "{\"a\":1} | multipart/json | MAP", // text/*: never structured, even when it holds JSON. A JSON parse of "12345" would yield a // Double, which no string rule can match "{\"a\":1} | text/plain | STRING", @@ -71,16 +74,6 @@ void dispatchesOnContentType(String body, String contentType, String expectedKin } } - @ParameterizedTest - @NullAndEmptySource - @ValueSource(strings = {" "}) - void parsesJsonWhenContentTypeIsAbsent(String contentType) { - Object parsed = parseBody("{\"user\":\"admin\"}", contentType); - - assertInstanceOf(Map.class, parsed); - assertEquals("admin", ((Map) parsed).get("user")); - } - @Test void returnsNullForNullBody() { assertNull(parseBody(null, "application/json")); @@ -103,7 +96,7 @@ void keepsRawStringOnceMaxDepthIsReached() { @Test void parsesUrlEncodedIntoAMultimap() { - Map> parsed = urlEncoded("user=admin&role=root"); + Map parsed = urlEncoded("user=admin&role=root"); assertEquals(singletonList("admin"), parsed.get("user")); assertEquals(singletonList("root"), parsed.get("role")); @@ -117,7 +110,7 @@ void groupsRepeatedUrlEncodedKeysIntoOneList() { @Test void decodesUrlEncodedPercentEscapesAndPluses() { - Map> parsed = urlEncoded("na+me=hello+world&q=%7B%22a%22%3A1%7D"); + Map parsed = urlEncoded("na+me=hello+world&q=%7B%22a%22%3A1%7D"); assertEquals(singletonList("hello world"), parsed.get("na me")); assertEquals(singletonList("{\"a\":1}"), parsed.get("q")); @@ -125,7 +118,7 @@ void decodesUrlEncodedPercentEscapesAndPluses() { @Test void keepsUndecodableUrlEncodedTokensAsIs() { - Map> parsed = urlEncoded("a=%&%=b"); + Map parsed = urlEncoded("a=%&%=b"); assertEquals(singletonList("%"), parsed.get("a")); assertEquals(singletonList("b"), parsed.get("%")); @@ -133,7 +126,7 @@ void keepsUndecodableUrlEncodedTokensAsIs() { @Test void treatsValuelessUrlEncodedPairsAsEmptyValues() { - Map> parsed = urlEncoded("flag&other=&last"); + Map parsed = urlEncoded("flag&other=&last"); assertEquals(singletonList(""), parsed.get("flag")); assertEquals(singletonList(""), parsed.get("other")); @@ -142,7 +135,7 @@ void treatsValuelessUrlEncodedPairsAsEmptyValues() { @Test void skipsEmptyUrlEncodedPairsAndNames() { - Map> parsed = urlEncoded("&&=orphan&&a=1&&"); + Map parsed = urlEncoded("&&=orphan&&a=1&&"); assertEquals(singletonList("1"), parsed.get("a")); assertEquals(1, parsed.size()); @@ -159,19 +152,19 @@ void doesNotSeparateUrlEncodedPairsOnSemicolons() { } @Test - void parsesUrlEncodedPairsUpToMaxElements() { + void parsesUrlEncodedPairsUpToThePartAllowance() { StringBuilder body = new StringBuilder(); - for (int i = 0; i < MAX_ELEMENTS; i++) { + for (int i = 0; i < MAX_PARTS; i++) { body.append(i == 0 ? "" : "&").append('k').append(i).append("=v"); } - assertEquals(MAX_ELEMENTS, urlEncoded(body.toString()).size()); + assertEquals(MAX_PARTS, urlEncoded(body.toString()).size()); } @Test - void keepsUrlEncodedBodyOverMaxElementsAsRawString() { + void keepsUrlEncodedBodyOverThePartAllowanceAsRawString() { StringBuilder body = new StringBuilder(); - for (int i = 0; i < MAX_ELEMENTS; i++) { + for (int i = 0; i < MAX_PARTS; i++) { body.append('k').append(i).append("=v&"); } body.append("attack=payload"); @@ -185,7 +178,7 @@ void keepsUrlEncodedBodyOverMaxElementsAsRawString() { @Test void countsNamelessUrlEncodedPairsTowardsTheCap() { StringBuilder body = new StringBuilder(); - for (int i = 0; i < MAX_ELEMENTS; i++) { + for (int i = 0; i < MAX_PARTS; i++) { body.append("=v&"); } body.append("a=1"); @@ -212,37 +205,12 @@ void parsesMultipartFieldsIntoAMap() { } @Test - void skipsMultipartFileParts() { - Map fields = multipart(field("user", "admin"), file("upload", "f.txt", "data")); - - assertEquals("admin", fields.get("user")); - assertEquals(1, fields.size()); - } - - @Test - void skipsMultipartPartsWithAnEmptyFilename() { - // An untouched file input: browsers send filename="", which is a file part all the same - Map fields = multipart(field("user", "admin"), file("upload", "", "")); - - assertEquals(1, fields.size()); - } - - @Test - void keepsAFileOnlyMultipartBodyAsARawString() { - // A raw string still matches string rules; an empty map would tell the WAF the body was empty - String body = outer(file("upload", "f.txt", "data")); + void skipsMultipartPartsWithoutAName() { + String body = outer(part("form-data", "novalue"), part("form-data; name=", "empty")); assertEquals(body, parseBody(body, MULTIPART)); } - @Test - void skipsMultipartPartsWithoutAName() { - assertEquals( - outer(part("form-data", "novalue"), part("form-data; name=", "empty")), - parseBody( - outer(part("form-data", "novalue"), part("form-data; name=", "empty")), MULTIPART)); - } - @Test void skipsMultipartPartsWithoutAContentDisposition() { Map fields = multipart(field("user", "admin"), part(null, "orphan")); @@ -278,7 +246,9 @@ void keepsMultipartPartsThatDeclareNoContentTypeAsRawStrings() { @Test void keepsMultipartPartsWithAnEmptyContentTypeAsRawStrings() { - Map fields = multipart(outer(part("form-data; name=\"amount\"", "", "12345"))); + // Only the empty case is worth covering: MultipartSplitter trims header values, so a + // whitespace-only Content-Type reaches the parser as "" and not as its original spelling + Map fields = multipart(part("form-data; name=\"amount\"", "", "12345")); assertEquals("12345", fields.get("amount")); } @@ -302,54 +272,49 @@ void nestsARepeatedJsonArrayPartValueRatherThanFlatteningIt() { @Test void doesNotTakeAFieldNameThatForgesAFilenameForAFilePart() { - Map fields = - multipart(outer(part("form-data; name=\"; filename=x\"", "payload"))); + Map fields = multipart(part("form-data; name=\"; filename=x\"", "payload")); assertEquals("payload", fields.get("; filename=x")); } @Test - @SuppressWarnings("unchecked") void parsesNestedMultipartBodiesAndSkipsTheirFileParts() { String inner = body("inner", field("nested", "value"), file("upload", "f.txt", "data")); String nesting = outer(part("form-data; name=group", "multipart/mixed; boundary=inner", inner)); - Map fields = (Map) parseBody(nesting, MULTIPART); + Map fields = asMap(parseBody(nesting, MULTIPART)); assertEquals(singletonMap("nested", "value"), fields.get("group")); } @Test - void sharesTheElementAllowanceAcrossNestingLevels() { + void sharesThePartAllowanceBetweenUrlEncodedParametersAndMultipartParts() { // Two thirds of the allowance each: the first part fits, the second cannot, which only holds if // both draw from one allowance rather than from a per-part counter - String urlEncoded = urlEncodedPairs(MAX_ELEMENTS * 2 / 3); + String urlEncoded = urlEncodedPairs(MAX_PARTS * 2 / 3); Map fields = multipart( - outer( - part("form-data; name=first", URL_ENCODED, urlEncoded), - part("form-data; name=second", URL_ENCODED, urlEncoded))); + part("form-data; name=first", URL_ENCODED, urlEncoded), + part("form-data; name=second", URL_ENCODED, urlEncoded)); assertInstanceOf(Map.class, fields.get("first")); assertEquals(urlEncoded, fields.get("second")); } @Test - void spendsTheElementAllowanceOnlyOnPairsItReads() { - // Just under the allowance twice over would exceed it if the whole body were charged upfront - String urlEncoded = urlEncodedPairs(MAX_ELEMENTS / 2); + void spendsThePartAllowanceOnlyOnPairsItReads() { + // Half the allowance each, less the two multipart parts carrying them: together they fit + // exactly, which they could not if either body were charged upfront + String urlEncoded = urlEncodedPairs((MAX_PARTS - 2) / 2); Map fields = multipart( - outer( - part("form-data; name=first", URL_ENCODED, urlEncoded), - part("form-data; name=second", URL_ENCODED, urlEncoded))); + part("form-data; name=first", URL_ENCODED, urlEncoded), + part("form-data; name=second", URL_ENCODED, urlEncoded)); assertInstanceOf(Map.class, fields.get("first")); assertInstanceOf(Map.class, fields.get("second")); } - private static final String URL_ENCODED = "application/x-www-form-urlencoded"; - private static String urlEncodedPairs(int count) { StringBuilder body = new StringBuilder(); for (int i = 0; i < count; i++) { @@ -360,7 +325,7 @@ private static String urlEncodedPairs(int count) { @Test void parsesMultipartPartsUpToThePartAllowance() { - assertEquals(MAX_PARTS, multipart(outer(fieldParts(MAX_PARTS))).size()); + assertEquals(MAX_PARTS, multipart(fieldParts(MAX_PARTS)).size()); } @Test @@ -378,7 +343,7 @@ void sharesThePartAllowanceAcrossNestingLevels() { // spent one, so it can no longer be read and degrades to a raw string on its own String inner = body("inner", fieldParts(MAX_PARTS)); Map fields = - multipart(outer(part("form-data; name=group", "multipart/mixed; boundary=inner", inner))); + multipart(part("form-data; name=group", "multipart/mixed; boundary=inner", inner)); assertEquals(inner, fields.get("group")); } @@ -391,13 +356,6 @@ private static String[] fieldParts(int count) { return parts; } - @Test - void keepsRawStringOnceMaxDepthIsReachedInAMultipartPart() { - String body = outer(field("user", "admin")); - - assertEquals(body, dispatch(body, MULTIPART, MAX_DEPTH, new ParseContext())); - } - @Test void keepsMultipartBodyAsRawStringWithoutAUsableBoundary() { String body = outer(field("user", "admin")); @@ -412,19 +370,38 @@ void keepsUnsplittableMultipartBodyAsRawString() { } @Test - void keepsMultipartBodyAboveTheSizeLimitAsRawString() { + void keepsABodyTheByteAllowanceCannotCoverAsRawString() { String body = outer(field("user", "admin")); + assertInstanceOf(Map.class, parseBody(body, MULTIPART, new ParseContext(body.length()))); + assertEquals(body, parseBody(body, MULTIPART, new ParseContext(body.length() - 1))); + } + + @Test + void chargesTheByteAllowanceWhateverTheContentType() { + // The allowance bounds reading, not multipart specifically, so a JSON body is charged too + String body = "{\"a\":1}"; + assertInstanceOf( - Map.class, parseMultipart(body, MULTIPART, 0, new ParseContext(), body.length())); - assertNull(parseMultipart(body, MULTIPART, 0, new ParseContext(), body.length() - 1)); + Map.class, parseBody(body, "application/json", new ParseContext(body.length()))); + assertEquals(body, parseBody(body, "application/json", new ParseContext(body.length() - 1))); } @Test - void disablesMultipartParsingWhenTheSizeLimitIsZero() { - String body = outer(field("user", "admin")); + void sharesTheByteAllowanceAcrossNestingLevels() { + // Without a shared allowance a nested body is re-measured against a fresh allowance at every + // level, so one crafted body costs MAX_DEPTH times its own size to scan and copy + String inner = body("inner", field("deep", "v")); + String nested = outer(part("form-data; name=\"n\"", "multipart/mixed; boundary=inner", inner)); + + // enough for the outer body alone, one character short of also covering the nested one + ParseContext exhausted = new ParseContext(nested.length() + inner.length() - 1); + Map fields = asMap(parseBody(nested, MULTIPART, exhausted)); + assertEquals(inner, fields.get("n")); - assertNull(parseMultipart(body, MULTIPART, 0, new ParseContext(), 0)); + ParseContext sufficient = new ParseContext(nested.length() + inner.length()); + Map parsed = asMap(parseBody(nested, MULTIPART, sufficient)); + assertEquals("v", ((Map) parsed.get("n")).get("deep")); } @Test @@ -437,7 +414,7 @@ void reportsTheFilenamesOfFileParts() { assertEquals(asList("cat.png", "report.pdf"), filenamesOf(body)); // The file parts are not fields, so only the field survives into the body map - assertEquals(singletonMap("user", "admin"), multipart(body)); + assertEquals(singletonMap("user", "admin"), multipartBody(body)); } @Test @@ -447,7 +424,7 @@ void marksAFilePartWithoutReportingAnEmptyFilename() { String body = outer(file("avatar", "", "bytes"), field("user", "admin")); assertEquals(emptyList(), filenamesOf(body)); - assertEquals(singletonMap("user", "admin"), multipart(body)); + assertEquals(singletonMap("user", "admin"), multipartBody(body)); } @Test @@ -460,8 +437,9 @@ void reportsFilenamesFromNestedMultipartParts() { @Test void reportsFilenamesEvenWhenTheBodyDegradesToARawString() { - // Nothing but file parts, so there is no field to report and the body stays a raw string. - // The filenames are then the only structured thing left to hand the WAF. + // Nothing but file parts, so there is no field to report. An empty map would tell the WAF the + // body was empty, so the raw string is kept, and the filenames are then the only structured + // thing left to hand it. String body = outer(file("avatar", "cat.png", "bytes")); assertEquals(body, parseBody(body, MULTIPART)); @@ -472,8 +450,8 @@ void reportsFilenamesEvenWhenTheBodyDegradesToARawString() { void reportsNoFilenameWhenTheMultipartBodyIsNotParsed() { String body = outer(file("avatar", "cat.png", "bytes")); - ParseContext sizeCapped = new ParseContext(); - assertNull(parseMultipart(body, MULTIPART, 0, sizeCapped, 0)); + ParseContext sizeCapped = new ParseContext(body.length() - 1); + assertEquals(body, parseBody(body, MULTIPART, sizeCapped)); assertEquals(emptyList(), sizeCapped.filenames()); ParseContext noBoundary = new ParseContext(); @@ -487,16 +465,30 @@ private static List filenamesOf(String body) { return context.filenames(); } - private static final String MULTIPART = "multipart/form-data; boundary=outer"; + private static Object parseBody(String body, String contentType) { + return parseBody(body, contentType, new ParseContext()); + } - @SuppressWarnings("unchecked") + private static Object parseBody(String body, String contentType, ParseContext context) { + return ContentTypeBodyParser.parseBody(body, contentType, context); + } + + /** Wraps the parts in a body with the default boundary and parses it. */ private static Map multipart(String... parts) { - return multipart(outer(parts)); + return multipartBody(outer(parts)); + } + + /** Distinct from {@link #multipart}, whose varargs would otherwise swallow a whole body. */ + private static Map multipartBody(String body) { + return asMap(parseBody(body, MULTIPART)); + } + + private static Map urlEncoded(String body) { + return asMap(parseBody(body, URL_ENCODED)); } @SuppressWarnings("unchecked") - private static Map multipart(String body) { - Object parsed = parseBody(body, MULTIPART); + private static Map asMap(Object parsed) { assertInstanceOf(Map.class, parsed); return (Map) parsed; } @@ -536,11 +528,4 @@ private static String part(String disposition, String contentType, String value) } return part.append("\r\n").append(value).toString(); } - - @SuppressWarnings("unchecked") - private static Map> urlEncoded(String body) { - Object parsed = parseBody(body, "application/x-www-form-urlencoded"); - assertInstanceOf(Map.class, parsed); - return (Map>) parsed; - } } diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java index a278fb16bad..508c8ddd346 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java @@ -3,6 +3,7 @@ import static datadog.trace.api.gateway.Events.EVENTS; import static datadog.trace.lambda.LambdaEventParser.detectTriggerType; import static datadog.trace.lambda.LambdaEventParser.parseResponse; +import static java.util.Collections.singletonMap; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -47,6 +48,7 @@ import datadog.trace.bootstrap.instrumentation.api.Tags; import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter; import datadog.trace.core.DDCoreJavaSpecification; +import datadog.trace.lambda.LambdaEventParser.LambdaResponseData; import datadog.trace.lambda.LambdaEventParser.LambdaTriggerType; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -69,6 +71,8 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; class LambdaAppSecHandlerTest extends DDCoreJavaSpecification { @@ -2187,6 +2191,30 @@ void extractResponseDataReturnsNullForEmptyString() { assertNull(parseResponse("")); } + @ParameterizedTest(name = "[{index}] content-type {0}") + @ValueSource(strings = {"", " ", "application/json"}) + void parsesAResponseBodyAsJsonWhenTheContentTypeIsBlankOrJson(String contentType) { + // A blank content type says nothing about the body, so it gets the same best-effort JSON parse + // as an absent one, matching the request path. + LambdaResponseData response = + parseResponse( + "{\"statusCode\": 200, \"headers\": {\"content-type\": \"" + + contentType + + "\"}, \"body\": \"{\\\"a\\\":1}\"}"); + assertNotNull(response); + assertEquals(singletonMap("a", 1.0), response.body); + } + + @Test + void keepsAResponseBodyRawWhenTheContentTypeIsNotJson() { + LambdaResponseData response = + parseResponse( + "{\"statusCode\": 200, \"headers\": {\"content-type\": \"text/plain\"}," + + " \"body\": \"{\\\"a\\\":1}\"}"); + assertNotNull(response); + assertEquals("{\"a\":1}", response.body); + } + // ============================================================================ // HTTP span tags // ============================================================================ diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java index 9cd24a74342..488ee9a3e17 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java @@ -144,8 +144,7 @@ void stopsAtThePartBudget() { @Test void delimitsContentExactly() { // Dashes, line breaks, a replacement character and a multi-byte character all inside the - // content: - // the reported range must not be thrown off by a near-miss on the line-feed anchor + // content: the reported range must not be thrown off by a near-miss on the line-feed anchor String content = "--not-a-boundary\r\n-x\nlast�é"; String body = "--x\r\nContent-Disposition: form-data; name=a\r\n\r\n" + content + "\r\n--x--\r\n"; @@ -154,7 +153,7 @@ void delimitsContentExactly() { assertEquals(1, parts.size()); assertEquals(content, content(body, parts.get(0))); - assertEquals("form-data; name=a", parts.get(0).header("content-disposition")); + assertEquals("form-data; name=a", parts.get(0).contentDisposition); } @Test @@ -168,29 +167,59 @@ void confinesAPartWhoseHeadersRunIntoTheNextDelimiter() { List parts = split(body, "x", NO_PART_BUDGET_LIMIT); + // The surviving part is the second one: had the two merged, its content would have swallowed + // the delimiter between them. assertEquals(1, parts.size()); - assertNull(parts.get(0).header("x-first")); - assertEquals("form-data; name=\"b\"", parts.get(0).header("content-disposition")); + assertEquals("form-data; name=\"b\"", parts.get(0).contentDisposition); assertEquals("second", content(body, parts.get(0))); } @Test - void lowercasesHeaderNamesAndTrimsValues() { - String body = "--x\r\nCONTENT-Disposition: form-data; name=a \r\nX:\r\n\r\nv\r\n--x--"; + void matchesHeaderNamesCaseInsensitivelyAndTrimsValues() { + String body = + "--x\r\nCONTENT-Disposition: form-data; name=a \r\n" + + "content-type \t:\ttext/plain \r\n\r\nv\r\n--x--"; + + Part part = split(body, "x", NO_PART_BUDGET_LIMIT).get(0); + + assertEquals("form-data; name=a", part.contentDisposition); + assertEquals("text/plain", part.contentType); + } + + @Test + void reportsNoValueForAHeaderThatIsPresentButEmpty() { + String body = "--x\r\nContent-Type:\r\n\r\nv\r\n--x--"; Part part = split(body, "x", NO_PART_BUDGET_LIMIT).get(0); - assertEquals("form-data; name=a", part.header("content-disposition")); - assertEquals("", part.header("x")); - assertNull(part.header("absent")); + assertEquals("", part.contentType); + assertNull(part.contentDisposition); + } + + @Test + @Timeout(value = 10, unit = SECONDS) + void dropsHeadersItDoesNotReadWhateverTheirNumber() { + // Only Content-Disposition and Content-Type are kept, so a part may declare arbitrarily many + // others without any of them being retained. + StringBuilder headers = new StringBuilder(); + for (int i = 0; i < 100_000; i++) { + headers.append("X-Filler-").append(i).append(": ").append(i).append("\r\n"); + } + String body = "--x\r\n" + headers + "Content-Disposition: form-data; name=a\r\n\r\nv\r\n--x--"; + + List parts = split(body, "x", NO_PART_BUDGET_LIMIT); + + assertEquals(1, parts.size()); + assertEquals("form-data; name=a", parts.get(0).contentDisposition); + assertNull(parts.get(0).contentType); + assertEquals("v", content(body, parts.get(0))); } @Test @Timeout(value = 10, unit = SECONDS) void returnsPromptlyOnAnAdversarialBoundary() { // Dashes are legal boundary characters, so a scan seeded on '-' would be quadratic here. - // Anchored - // on the mandatory line feed, of which this body has none, the scan is linear. + // Anchored on the mandatory line feed, of which this body has none, the scan is linear. String boundary = repeat('-', 69) + "X"; String body = repeat('-', 1_000_000); From 8521f8b339a80d919337f5f91d1c9e02460e46b3 Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Wed, 2 Sep 2026 13:27:10 +0200 Subject: [PATCH 05/10] Require a multipart delimiter to end its line Co-Authored-By: Claude Opus 5 --- .../trace/lambda/MultipartSplitter.java | 30 ++++++++++++------- .../trace/lambda/MultipartSplitterTest.java | 7 +++-- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java index a28affa7f78..f1cbd90ceec 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java @@ -70,7 +70,7 @@ static List split(final String body, final String boundary, final int part boolean headersComplete = false; int malformedPartEnd = -1; while (cursor < length) { - if (body.startsWith(delimiter, cursor)) { + if (body.startsWith(delimiter, cursor) && endsLine(body, cursor + delimiter.length())) { // This part's headers are not followed by a blank line. Stop here: reading on would // consume the next part's delimiter and merge its headers into this part, collapsing // every following part into this one. @@ -132,11 +132,10 @@ static String extractBoundary(final String contentType) { } /** - * Reads a header parameter, case preserved. The parameter name is matched case-insensitively and - * only at a parameter boundary, so {@code filename} does not satisfy a lookup for {@code name}. - * Quoted values may contain separators and {@code \"} escapes, and are skipped whole: a field - * named {@code "; filename=x"} does not read as a {@code filename} parameter. RFC 7230 optional - * whitespace is tolerated on both sides of the {@code =}. + * Reads a header parameter, case preserved. The name is matched case-insensitively and only at a + * parameter boundary, so {@code filename} does not satisfy a lookup for {@code name}. Quoted + * values may hold separators and {@code \"} escapes, and are skipped whole: a field named {@code + * "; filename=x"} does not read as a {@code filename} parameter. * * @return the parameter value, {@code ""} when it is present but empty, or {@code null} when the * parameter is absent @@ -231,19 +230,31 @@ private static boolean isParameterSeparator(final char c) { * @return the index the delimiter starts at, or {@code -1} if there is none */ private static int nextDelimiter(final String body, final String delimiter, final int from) { - if (body.startsWith(delimiter, from)) { + if (body.startsWith(delimiter, from) && endsLine(body, from + delimiter.length())) { return from; } for (int newline = body.indexOf('\n', from); newline >= 0; newline = body.indexOf('\n', newline + 1)) { - if (body.startsWith(delimiter, newline + 1)) { + if (body.startsWith(delimiter, newline + 1) + && endsLine(body, newline + 1 + delimiter.length())) { return newline + 1; } } return -1; } + /** + * A delimiter only delimits if its line ends there, RFC 2046 transport padding aside. A content + * line that merely starts with it — {@code --x-not-a-boundary} for a boundary of {@code x} — is + * data, and must not end the part early and hide the rest from the WAF. + * + * @param after the index just past the matched delimiter + */ + private static boolean endsLine(final String body, final int after) { + return after == body.length() || body.startsWith("--", after) || lineStart(body, after) >= 0; + } + /** * Skips the linear whitespace and line break that follow a delimiter. * @@ -305,8 +316,7 @@ private static String headerValue( while (end > start && isOptionalWhitespace(body.charAt(end - 1))) { end--; } - // The canonical spelling is tried first: its comparison is intrinsified where the - // case-insensitive one walks char by char, and it is what every real client sends. + // The canonical spelling first: that comparison is intrinsified, and every real client sends it if (end - start != name.length() || !(body.regionMatches(start, name, 0, name.length()) || body.regionMatches(true, start, name, 0, name.length()))) { diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java index 488ee9a3e17..9a91f6512a2 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java @@ -117,6 +117,8 @@ void keepsWhatWasReadOfAnUnterminatedQuotedValue() { "empty part content | --x@A: b@@ | 1", "part without headers | --x@@v@--x-- | 1", "unrelated delimiter | --y@@v@--y-- | 0", + // RFC 2046 transport padding between the delimiter and its line break + "padded delimiter | --x @@v@--x-- | 1", }) void splitsBodies(String name, String template, int expectedParts) { assertEquals( @@ -144,8 +146,9 @@ void stopsAtThePartBudget() { @Test void delimitsContentExactly() { // Dashes, line breaks, a replacement character and a multi-byte character all inside the - // content: the reported range must not be thrown off by a near-miss on the line-feed anchor - String content = "--not-a-boundary\r\n-x\nlast�é"; + // content, plus a line that starts with the delimiter without being one: the reported range + // must not be thrown off by a near miss on the line-feed anchor or on the delimiter itself + String content = "--not-a-boundary\r\n--x-not-a-boundary\r\n-x\nlast�é"; String body = "--x\r\nContent-Disposition: form-data; name=a\r\n\r\n" + content + "\r\n--x--\r\n"; From 0ed399c12cd5ffa59e4590aef41a9aba987322d8 Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Wed, 2 Sep 2026 13:27:12 +0200 Subject: [PATCH 06/10] Trim redundant Lambda body parsing tests and comments Co-Authored-By: Claude Opus 5 --- .../trace/lambda/ContentTypeBodyParser.java | 29 +++----- .../lambda/ContentTypeBodyParserTest.java | 36 +++------ .../trace/lambda/LambdaAppSecHandlerTest.java | 74 ------------------- 3 files changed, 20 insertions(+), 119 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java index db75ff9afe0..d3c052a1ab5 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java @@ -27,13 +27,10 @@ final class ContentTypeBodyParser { private static final Logger log = LoggerFactory.getLogger(ContentTypeBodyParser.class); - // These bound the work done in this parser only. Exceeding any of them degrades the body to a raw - // string rather than dropping content. - // - // The depth and part limits mirror the WAF's own (WAFModule.MAX_DEPTH / MAX_ELEMENTS): structure - // beyond them is discarded by ObjectIntrospection before the WAF ever sees it, so producing it - // would be wasted work. The byte allowance answers a different question — how much reading and - // copying one event may cost us — and AWS already caps a synchronous payload at 6 MiB. + // These bound the work done in this parser only: exceeding any of them degrades the body to a raw + // string rather than dropping content. The depth and part limits mirror the WAF's own + // (WAFModule.MAX_DEPTH / MAX_ELEMENTS), beyond which ObjectIntrospection discards the structure + // anyway. The byte allowance instead bounds what reading one event may cost us. static final int MAX_BYTES = 1024 * 1024; static final int MAX_PARTS = 256; static final int MAX_DEPTH = 20; @@ -42,9 +39,8 @@ private ContentTypeBodyParser() {} /** * State shared across a whole parse: the byte and part allowances, and the filenames collected - * along the way. A multipart part may itself hold an urlencoded or multipart body, so a per-call - * allowance would be re-satisfied at every nesting level and a per-call list would lose nested - * file parts. + * along the way. A multipart part may itself hold a multipart body, so a per-call allowance would + * be re-satisfied at every nesting level. */ static final class ParseContext { private int bytes; @@ -85,8 +81,7 @@ boolean takeBytes(final int count) { /** * @return {@code false} once the part allowance is spent. A multipart part and an urlencoded - * parameter both draw from it: they are the same question, how many values one body may - * yield. + * parameter both draw from it. */ boolean takePart() { if (parts == 0) { @@ -173,10 +168,9 @@ private static Object jsonOrRaw(final String body) { } /** - * Matches on the type and subtype only. Parameters such as a multipart boundary are chosen by the - * client, so {@code multipart/form-data; boundary=--json} must not reach the JSON parser and - * thereby skip multipart parsing entirely; {@link MediaType#parse} strips them, and lowercases - * what is left. + * Matches on the type and subtype only, which {@link MediaType#parse} has already stripped of + * parameters: a client-chosen {@code multipart/form-data; boundary=--json} must not reach the + * JSON parser and thereby skip multipart parsing entirely. */ private static boolean isJsonLike(final MediaType mediaType) { return jsonLike(mediaType.getType()) || jsonLike(mediaType.getSubtype()); @@ -241,8 +235,7 @@ private static Object parseMultipart( final int allowance = context.remainingParts(); final List parts = MultipartSplitter.split(body, boundary, allowance + 1); if (parts.size() > allowance) { - // Bail out rather than hand the WAF a truncated map, as the urlencoded path does: - // the raw body can still be string-matched. + // Bail out rather than truncate, as the urlencoded path does log.debug("Part allowance exhausted, keeping multipart body as a raw string"); return null; } diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java index 768d1bbb980..26dd61ffcb8 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java @@ -234,8 +234,7 @@ void dispatchesOnEachMultipartPartsOwnContentType() { @Test void keepsMultipartPartsThatDeclareNoContentTypeAsRawStrings() { // A part with no Content-Type is text/plain per RFC 7578, section 4.4, not a body of unknown - // type: a JSON parse would hand the WAF a Double, a Boolean and a Map, so the same form - // submitted urlencoded and as multipart would no longer match the same string rules + // type: a JSON parse would hand the WAF a Double and a Boolean no string rule can match Map fields = multipart(field("amount", "12345"), field("flag", "true"), field("json", "{\"a\":1}")); @@ -262,8 +261,7 @@ void promotesRepeatedMultipartFieldNamesToAList() { @Test void nestsARepeatedJsonArrayPartValueRatherThanFlatteningIt() { - // The first value is itself a List, so a repeat cannot be told from a fresh key by the stored - // value's type: appending to it would hand the WAF a structure that was never sent + // The first value is itself a List, so appending to it would flatten two values into one Map fields = multipart(part("form-data; name=x", "application/json", "[1,2]"), field("x", "b")); @@ -289,8 +287,7 @@ void parsesNestedMultipartBodiesAndSkipsTheirFileParts() { @Test void sharesThePartAllowanceBetweenUrlEncodedParametersAndMultipartParts() { - // Two thirds of the allowance each: the first part fits, the second cannot, which only holds if - // both draw from one allowance rather than from a per-part counter + // Two thirds of the allowance each: the second only fails if both draw from one allowance String urlEncoded = urlEncodedPairs(MAX_PARTS * 2 / 3); Map fields = multipart( @@ -303,8 +300,7 @@ void sharesThePartAllowanceBetweenUrlEncodedParametersAndMultipartParts() { @Test void spendsThePartAllowanceOnlyOnPairsItReads() { - // Half the allowance each, less the two multipart parts carrying them: together they fit - // exactly, which they could not if either body were charged upfront + // Half the allowance each, less their two parts: they only fit if neither is charged upfront String urlEncoded = urlEncodedPairs((MAX_PARTS - 2) / 2); Map fields = multipart( @@ -330,8 +326,7 @@ void parsesMultipartPartsUpToThePartAllowance() { @Test void keepsMultipartBodyOverThePartAllowanceAsRawString() { - // One part over the allowance drops that part from every WAF address, so the whole body - // degrades to a raw string instead, as an over-allowance urlencoded body does + // One part over the allowance degrades the whole body, as an urlencoded body over it does String body = outer(fieldParts(MAX_PARTS + 1)); assertEquals(body, parseBody(body, MULTIPART)); @@ -339,8 +334,7 @@ void keepsMultipartBodyOverThePartAllowanceAsRawString() { @Test void sharesThePartAllowanceAcrossNestingLevels() { - // The inner body alone is within the allowance, but the outer part it sits in has already - // spent one, so it can no longer be read and degrades to a raw string on its own + // The inner body fits the allowance on its own, but the outer part it sits in has spent one String inner = body("inner", fieldParts(MAX_PARTS)); Map fields = multipart(part("form-data; name=group", "multipart/mixed; boundary=inner", inner)); @@ -377,20 +371,9 @@ void keepsABodyTheByteAllowanceCannotCoverAsRawString() { assertEquals(body, parseBody(body, MULTIPART, new ParseContext(body.length() - 1))); } - @Test - void chargesTheByteAllowanceWhateverTheContentType() { - // The allowance bounds reading, not multipart specifically, so a JSON body is charged too - String body = "{\"a\":1}"; - - assertInstanceOf( - Map.class, parseBody(body, "application/json", new ParseContext(body.length()))); - assertEquals(body, parseBody(body, "application/json", new ParseContext(body.length() - 1))); - } - @Test void sharesTheByteAllowanceAcrossNestingLevels() { - // Without a shared allowance a nested body is re-measured against a fresh allowance at every - // level, so one crafted body costs MAX_DEPTH times its own size to scan and copy + // Unshared, a nested body is re-measured at every level: MAX_DEPTH times its size to copy String inner = body("inner", field("deep", "v")); String nested = outer(part("form-data; name=\"n\"", "multipart/mixed; boundary=inner", inner)); @@ -437,9 +420,8 @@ void reportsFilenamesFromNestedMultipartParts() { @Test void reportsFilenamesEvenWhenTheBodyDegradesToARawString() { - // Nothing but file parts, so there is no field to report. An empty map would tell the WAF the - // body was empty, so the raw string is kept, and the filenames are then the only structured - // thing left to hand it. + // Nothing but file parts, so there is no field to report: an empty map would tell the WAF the + // body was empty, so the raw string is kept and the filenames reported alongside it String body = outer(file("avatar", "cat.png", "bytes")); assertEquals(body, parseBody(body, MULTIPART)); diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java index 508c8ddd346..96a0b287cce 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java @@ -704,56 +704,6 @@ void handlesEmptyBodyCorrectly() { assertEquals("", capturedBody[0]); } - @Test - @SuppressWarnings("unchecked") - void parsesUrlEncodedBodyIntoAMultimap() { - String eventJson = - "{" - + "\"body\": \"user=admin&role=root\"," - + "\"headers\": {\"Content-Type\": \"application/x-www-form-urlencoded\"}," - + "\"requestContext\": {\"httpMethod\": \"POST\"}" - + "}"; - ByteArrayInputStream event = createInputStream(eventJson); - - Object[] capturedBody = {null}; - - setupMockCallbacks(new Callbacks().onBody(body -> capturedBody[0] = body)); - - AgentSpanContext result = LambdaAppSecHandler.processRequestStart(event); - - assertNotNull(result); - assertInstanceOf(Map.class, capturedBody[0]); - Map> parameters = (Map>) capturedBody[0]; - assertEquals(Arrays.asList("admin"), parameters.get("user")); - assertEquals(Arrays.asList("root"), parameters.get("role")); - } - - @Test - @SuppressWarnings("unchecked") - void parsesMultipartBodyIntoItsFields() { - String eventJson = - "{" - + "\"body\": \"--xy\\r\\nContent-Disposition: form-data; name=\\\"user\\\"\\r\\n\\r\\nadmin" - + "\\r\\n--xy\\r\\nContent-Disposition: form-data; name=\\\"role\\\"\\r\\n\\r\\nroot" - + "\\r\\n--xy--\"," - + "\"headers\": {\"Content-Type\": \"multipart/form-data; boundary=xy\"}," - + "\"requestContext\": {\"httpMethod\": \"POST\"}" - + "}"; - ByteArrayInputStream event = createInputStream(eventJson); - - Object[] capturedBody = {null}; - - setupMockCallbacks(new Callbacks().onBody(body -> capturedBody[0] = body)); - - AgentSpanContext result = LambdaAppSecHandler.processRequestStart(event); - - assertNotNull(result); - assertInstanceOf(Map.class, capturedBody[0]); - Map fields = (Map) capturedBody[0]; - assertEquals("admin", fields.get("user")); - assertEquals("root", fields.get("role")); - } - @Test @SuppressWarnings("unchecked") void reportsMultipartFilenamesToTheWaf() { @@ -807,30 +757,6 @@ void doesNotReportFilenamesForAMultipartBodyWithoutFileParts() { assertNull(capturedFilenames[0]); } - @Test - void keepsMultipartBodyWithoutBoundaryAsRawString() { - String body = - "--xy\\r\\nContent-Disposition: form-data; name=user\\r\\n\\r\\nadmin\\r\\n--xy--"; - String eventJson = - "{" - + "\"body\": \"" - + body - + "\"," - + "\"headers\": {\"Content-Type\": \"multipart/form-data\"}," - + "\"requestContext\": {\"httpMethod\": \"POST\"}" - + "}"; - ByteArrayInputStream event = createInputStream(eventJson); - - Object[] capturedBody = {null}; - - setupMockCallbacks(new Callbacks().onBody(b -> capturedBody[0] = b)); - - AgentSpanContext result = LambdaAppSecHandler.processRequestStart(event); - - assertNotNull(result); - assertEquals(body.replace("\\r\\n", "\r\n"), capturedBody[0]); - } - @Test @SuppressWarnings("unchecked") void appliesContentTypeDispatchToBase64DecodedBodies() { From 2a005bef44b201344f655ab74b2fb129446aa802 Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Wed, 2 Sep 2026 14:58:44 +0200 Subject: [PATCH 07/10] Collapse the JSON content-type gate into one method and drop unchecked casts Co-Authored-By: Claude Opus 5 --- .../trace/lambda/ContentTypeBodyParser.java | 81 ++++++------------- .../trace/lambda/LambdaEventParser.java | 13 +-- .../lambda/ContentTypeBodyParserTest.java | 51 ++++++------ 3 files changed, 58 insertions(+), 87 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java index d3c052a1ab5..29996b059ce 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/ContentTypeBodyParser.java @@ -6,11 +6,10 @@ import java.net.URLDecoder; import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; +import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.StringTokenizer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -28,9 +27,7 @@ final class ContentTypeBodyParser { private static final Logger log = LoggerFactory.getLogger(ContentTypeBodyParser.class); // These bound the work done in this parser only: exceeding any of them degrades the body to a raw - // string rather than dropping content. The depth and part limits mirror the WAF's own - // (WAFModule.MAX_DEPTH / MAX_ELEMENTS), beyond which ObjectIntrospection discards the structure - // anyway. The byte allowance instead bounds what reading one event may cost us. + // string rather than dropping content. static final int MAX_BYTES = 1024 * 1024; static final int MAX_PARTS = 256; static final int MAX_DEPTH = 20; @@ -132,10 +129,9 @@ static Object dispatch( return body; } final MediaType mediaType = MediaType.parse(contentType); - // A null type is one MediaType could not read: the header was absent, blank, or nothing but - // parameters. Such a body gets the same best-effort JSON parse as a JSON-ish one. - if (mediaType.getType() == null || isJsonLike(mediaType)) { - return jsonOrRaw(body); + if (isJsonOrUntyped(mediaType)) { + final Object parsed = LambdaEventParser.parseBodyAsJson(body); + return parsed != null ? parsed : body; } if ("application".equals(mediaType.getType()) && "x-www-form-urlencoded".equals(mediaType.getSubtype())) { @@ -151,33 +147,10 @@ static Object dispatch( return body; } - /** - * Applies the "no declared type, or a JSON-ish one" rule to a body the caller does not structure - * any further. This is the whole of how a response body is handled; the request path shares the - * rule through {@link #dispatch} and additionally structures urlencoded and multipart bodies. - */ - static Object jsonOrRaw(final String body, final String contentType) { - final MediaType mediaType = MediaType.parse(contentType); - return mediaType.getType() == null || isJsonLike(mediaType) ? jsonOrRaw(body) : body; - } - - /** A best-effort JSON parse, degrading to the raw string rather than dropping the body. */ - private static Object jsonOrRaw(final String body) { - final Object parsed = LambdaEventParser.parseBodyAsJson(body); - return parsed != null ? parsed : body; - } - - /** - * Matches on the type and subtype only, which {@link MediaType#parse} has already stripped of - * parameters: a client-chosen {@code multipart/form-data; boundary=--json} must not reach the - * JSON parser and thereby skip multipart parsing entirely. - */ - private static boolean isJsonLike(final MediaType mediaType) { - return jsonLike(mediaType.getType()) || jsonLike(mediaType.getSubtype()); - } - - private static boolean jsonLike(final String essence) { - return essence != null && (essence.contains("json") || essence.contains("javascript")); + static boolean isJsonOrUntyped(final MediaType mediaType) { + final String subtype = mediaType.getSubtype(); + return mediaType.getType() == null + || (subtype != null && (subtype.contains("json") || subtype.contains("javascript"))); } /** @@ -196,8 +169,6 @@ private static Map> parseUrlEncoded( final StringTokenizer tokenizer = new StringTokenizer(body, "&"); while (tokenizer.hasMoreTokens()) { if (!context.takePart()) { - // Bail out rather than hand the WAF a truncated map: a parameter dropped here would be - // invisible to every rule, whereas the raw body can still be string-matched. log.debug("Part allowance exhausted, keeping urlencoded body as a raw string"); return null; } @@ -235,14 +206,13 @@ private static Object parseMultipart( final int allowance = context.remainingParts(); final List parts = MultipartSplitter.split(body, boundary, allowance + 1); if (parts.size() > allowance) { - // Bail out rather than truncate, as the urlencoded path does log.debug("Part allowance exhausted, keeping multipart body as a raw string"); return null; } context.consumeParts(parts.size()); final Map fields = new LinkedHashMap<>(); - final Set promoted = new HashSet<>(); + final Map> promoted = new HashMap<>(); for (final Part part : parts) { final String disposition = part.contentDisposition; if (disposition == null) { @@ -259,12 +229,9 @@ private static Object parseMultipart( if (name == null || name.isEmpty()) { continue; } - // Already trimmed by MultipartSplitter, so a whitespace-only header arrives as "" final String partContentType = part.contentType; final String content = body.substring(part.contentStart, part.contentEnd); - // A part that declares no type is kept as a raw string, the opposite of what a whole body - // with no type gets: RFC 7578, section 4.4 defaults a part to text/plain rather than leaving - // its type unknown. + // A part that declares no type is kept as a raw string final Object value = partContentType == null || partContentType.isEmpty() ? content @@ -278,28 +245,30 @@ private static Object parseMultipart( * Accumulates a field as a scalar on first sight and promotes it to a list on repeat. * Deliberately a different shape from urlencoded's always-a-list, matching the peer tracers. * - * @param promoted the names already promoted to a list, mutated as fields are promoted + * @param promoted the list each promoted field was given, by name, mutated as fields are + * promoted. Tracked rather than inferred from the stored value's type: a part whose body + * parsed as a JSON array is itself a List, and appending to it would flatten the two apart. */ - @SuppressWarnings("unchecked") private static void addField( final Map fields, - final Set promoted, + final Map> promoted, final String name, final Object value) { + final List values = promoted.get(name); + if (values != null) { + values.add(value); + return; + } // A part value is never null, so an absent key is exactly a null lookup final Object existing = fields.get(name); if (existing == null) { fields.put(name, value); - } else if (promoted.contains(name)) { - ((List) existing).add(value); } else { - // Promotion is tracked rather than inferred from the stored value's type: a part whose body - // parsed as a JSON array is itself a List, and appending to it would flatten the two apart. - final List values = new ArrayList<>(2); - values.add(existing); - values.add(value); - fields.put(name, values); - promoted.add(name); + final List promotion = new ArrayList<>(2); + promotion.add(existing); + promotion.add(value); + fields.put(name, promotion); + promoted.put(name, promotion); } } diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java index 03258888dd2..5b8ad1fc35e 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/LambdaEventParser.java @@ -3,6 +3,7 @@ import com.squareup.moshi.JsonAdapter; import com.squareup.moshi.Moshi; import datadog.trace.api.Config; +import datadog.trace.api.appsec.MediaType; import datadog.trace.lambda.ContentTypeBodyParser.ParseContext; import java.io.ByteArrayInputStream; import java.io.IOException; @@ -96,8 +97,7 @@ static LambdaRequestData parseEvent(String json) { return extractAlbData(event, triggerType); default: // Unsupported trigger: returning EMPTY makes the caller skip the invocation, so there is - // nothing to extract. The caller already recorded UNKNOWN as the trigger type, which is - // what reports the unsupported event at request end. + // nothing to extract sinc ethe event is not supported. return LambdaRequestData.EMPTY; } } catch (Exception e) { @@ -162,10 +162,11 @@ static LambdaResponseData parseResponse(String json) { } if (bodyString != null) { - // A response body is only ever structured as JSON, never as urlencoded or multipart, but - // the rule deciding whether to try is shared with the request path so the two cannot - // drift - body = ContentTypeBodyParser.jsonOrRaw(bodyString, headers.get("content-type")); + // A response body is only ever structured as JSON, never as urlencoded or multipart + MediaType mediaType = MediaType.parse(headers.get("content-type")); + Object parsed = + ContentTypeBodyParser.isJsonOrUntyped(mediaType) ? parseBodyAsJson(bodyString) : null; + body = parsed != null ? parsed : bodyString; } } diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java index 26dd61ffcb8..ef71a15ae14 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java @@ -60,6 +60,8 @@ class ContentTypeBodyParserTest { "{\"a\":1} | application/xml | STRING", "{\"a\":1} | application/octet-stream | STRING", "{\"a\":1} | garbage | STRING", + // a slashless header declares a type but no subtype, so it is not JSON-ish + "{\"a\":1} | json | STRING", // known gap: a JSON-carrying type whose name says neither "json" nor "javascript" keeps // the raw string, which the WAF can still match string rules against "{\"a\":1} | application/csp-report | STRING", @@ -96,7 +98,7 @@ void keepsRawStringOnceMaxDepthIsReached() { @Test void parsesUrlEncodedIntoAMultimap() { - Map parsed = urlEncoded("user=admin&role=root"); + Map parsed = urlEncoded("user=admin&role=root"); assertEquals(singletonList("admin"), parsed.get("user")); assertEquals(singletonList("root"), parsed.get("role")); @@ -110,7 +112,7 @@ void groupsRepeatedUrlEncodedKeysIntoOneList() { @Test void decodesUrlEncodedPercentEscapesAndPluses() { - Map parsed = urlEncoded("na+me=hello+world&q=%7B%22a%22%3A1%7D"); + Map parsed = urlEncoded("na+me=hello+world&q=%7B%22a%22%3A1%7D"); assertEquals(singletonList("hello world"), parsed.get("na me")); assertEquals(singletonList("{\"a\":1}"), parsed.get("q")); @@ -118,7 +120,7 @@ void decodesUrlEncodedPercentEscapesAndPluses() { @Test void keepsUndecodableUrlEncodedTokensAsIs() { - Map parsed = urlEncoded("a=%&%=b"); + Map parsed = urlEncoded("a=%&%=b"); assertEquals(singletonList("%"), parsed.get("a")); assertEquals(singletonList("b"), parsed.get("%")); @@ -126,7 +128,7 @@ void keepsUndecodableUrlEncodedTokensAsIs() { @Test void treatsValuelessUrlEncodedPairsAsEmptyValues() { - Map parsed = urlEncoded("flag&other=&last"); + Map parsed = urlEncoded("flag&other=&last"); assertEquals(singletonList(""), parsed.get("flag")); assertEquals(singletonList(""), parsed.get("other")); @@ -135,7 +137,7 @@ void treatsValuelessUrlEncodedPairsAsEmptyValues() { @Test void skipsEmptyUrlEncodedPairsAndNames() { - Map parsed = urlEncoded("&&=orphan&&a=1&&"); + Map parsed = urlEncoded("&&=orphan&&a=1&&"); assertEquals(singletonList("1"), parsed.get("a")); assertEquals(1, parsed.size()); @@ -197,7 +199,7 @@ void keepsUnparseableUrlEncodedBodyAsRawString() { @Test void parsesMultipartFieldsIntoAMap() { - Map fields = multipart(field("user", "admin"), field("role", "root")); + Map fields = multipart(field("user", "admin"), field("role", "root")); assertEquals("admin", fields.get("user")); assertEquals("root", fields.get("role")); @@ -213,14 +215,14 @@ void skipsMultipartPartsWithoutAName() { @Test void skipsMultipartPartsWithoutAContentDisposition() { - Map fields = multipart(field("user", "admin"), part(null, "orphan")); + Map fields = multipart(field("user", "admin"), part(null, "orphan")); assertEquals(1, fields.size()); } @Test void dispatchesOnEachMultipartPartsOwnContentType() { - Map fields = + Map fields = multipart( part("form-data; name=payload", "application/json", "{\"a\":1}"), part("form-data; name=plain", "text/plain", "12345"), @@ -235,7 +237,7 @@ void dispatchesOnEachMultipartPartsOwnContentType() { void keepsMultipartPartsThatDeclareNoContentTypeAsRawStrings() { // A part with no Content-Type is text/plain per RFC 7578, section 4.4, not a body of unknown // type: a JSON parse would hand the WAF a Double and a Boolean no string rule can match - Map fields = + Map fields = multipart(field("amount", "12345"), field("flag", "true"), field("json", "{\"a\":1}")); assertEquals("12345", fields.get("amount")); @@ -247,14 +249,14 @@ void keepsMultipartPartsThatDeclareNoContentTypeAsRawStrings() { void keepsMultipartPartsWithAnEmptyContentTypeAsRawStrings() { // Only the empty case is worth covering: MultipartSplitter trims header values, so a // whitespace-only Content-Type reaches the parser as "" and not as its original spelling - Map fields = multipart(part("form-data; name=\"amount\"", "", "12345")); + Map fields = multipart(part("form-data; name=\"amount\"", "", "12345")); assertEquals("12345", fields.get("amount")); } @Test void promotesRepeatedMultipartFieldNamesToAList() { - Map fields = multipart(field("x", "a"), field("x", "b"), field("x", "c")); + Map fields = multipart(field("x", "a"), field("x", "b"), field("x", "c")); assertEquals(asList("a", "b", "c"), fields.get("x")); } @@ -262,7 +264,7 @@ void promotesRepeatedMultipartFieldNamesToAList() { @Test void nestsARepeatedJsonArrayPartValueRatherThanFlatteningIt() { // The first value is itself a List, so appending to it would flatten two values into one - Map fields = + Map fields = multipart(part("form-data; name=x", "application/json", "[1,2]"), field("x", "b")); assertEquals(asList(asList(1.0, 2.0), "b"), fields.get("x")); @@ -270,7 +272,7 @@ void nestsARepeatedJsonArrayPartValueRatherThanFlatteningIt() { @Test void doesNotTakeAFieldNameThatForgesAFilenameForAFilePart() { - Map fields = multipart(part("form-data; name=\"; filename=x\"", "payload")); + Map fields = multipart(part("form-data; name=\"; filename=x\"", "payload")); assertEquals("payload", fields.get("; filename=x")); } @@ -280,7 +282,7 @@ void parsesNestedMultipartBodiesAndSkipsTheirFileParts() { String inner = body("inner", field("nested", "value"), file("upload", "f.txt", "data")); String nesting = outer(part("form-data; name=group", "multipart/mixed; boundary=inner", inner)); - Map fields = asMap(parseBody(nesting, MULTIPART)); + Map fields = asMap(parseBody(nesting, MULTIPART)); assertEquals(singletonMap("nested", "value"), fields.get("group")); } @@ -289,7 +291,7 @@ void parsesNestedMultipartBodiesAndSkipsTheirFileParts() { void sharesThePartAllowanceBetweenUrlEncodedParametersAndMultipartParts() { // Two thirds of the allowance each: the second only fails if both draw from one allowance String urlEncoded = urlEncodedPairs(MAX_PARTS * 2 / 3); - Map fields = + Map fields = multipart( part("form-data; name=first", URL_ENCODED, urlEncoded), part("form-data; name=second", URL_ENCODED, urlEncoded)); @@ -302,7 +304,7 @@ void sharesThePartAllowanceBetweenUrlEncodedParametersAndMultipartParts() { void spendsThePartAllowanceOnlyOnPairsItReads() { // Half the allowance each, less their two parts: they only fit if neither is charged upfront String urlEncoded = urlEncodedPairs((MAX_PARTS - 2) / 2); - Map fields = + Map fields = multipart( part("form-data; name=first", URL_ENCODED, urlEncoded), part("form-data; name=second", URL_ENCODED, urlEncoded)); @@ -336,7 +338,7 @@ void keepsMultipartBodyOverThePartAllowanceAsRawString() { void sharesThePartAllowanceAcrossNestingLevels() { // The inner body fits the allowance on its own, but the outer part it sits in has spent one String inner = body("inner", fieldParts(MAX_PARTS)); - Map fields = + Map fields = multipart(part("form-data; name=group", "multipart/mixed; boundary=inner", inner)); assertEquals(inner, fields.get("group")); @@ -379,11 +381,11 @@ void sharesTheByteAllowanceAcrossNestingLevels() { // enough for the outer body alone, one character short of also covering the nested one ParseContext exhausted = new ParseContext(nested.length() + inner.length() - 1); - Map fields = asMap(parseBody(nested, MULTIPART, exhausted)); + Map fields = asMap(parseBody(nested, MULTIPART, exhausted)); assertEquals(inner, fields.get("n")); ParseContext sufficient = new ParseContext(nested.length() + inner.length()); - Map parsed = asMap(parseBody(nested, MULTIPART, sufficient)); + Map parsed = asMap(parseBody(nested, MULTIPART, sufficient)); assertEquals("v", ((Map) parsed.get("n")).get("deep")); } @@ -456,23 +458,22 @@ private static Object parseBody(String body, String contentType, ParseContext co } /** Wraps the parts in a body with the default boundary and parses it. */ - private static Map multipart(String... parts) { + private static Map multipart(String... parts) { return multipartBody(outer(parts)); } /** Distinct from {@link #multipart}, whose varargs would otherwise swallow a whole body. */ - private static Map multipartBody(String body) { + private static Map multipartBody(String body) { return asMap(parseBody(body, MULTIPART)); } - private static Map urlEncoded(String body) { + private static Map urlEncoded(String body) { return asMap(parseBody(body, URL_ENCODED)); } - @SuppressWarnings("unchecked") - private static Map asMap(Object parsed) { + private static Map asMap(Object parsed) { assertInstanceOf(Map.class, parsed); - return (Map) parsed; + return (Map) parsed; } /** Joins the parts with CRLF and appends the close delimiter, using the default boundary. */ From d9c5296587f55207947cdaeb3de7e9fa309a33aa Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Wed, 2 Sep 2026 14:58:46 +0200 Subject: [PATCH 08/10] Keep a malformed multipart body as a raw string instead of reporting the surviving parts Co-Authored-By: Claude Opus 5 --- .../trace/lambda/MultipartSplitter.java | 20 +++++++------------ .../trace/lambda/MultipartSplitterTest.java | 15 ++++---------- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java index f1cbd90ceec..7fda2126db4 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java @@ -1,6 +1,7 @@ package datadog.trace.lambda; import java.util.ArrayList; +import java.util.Collections; import java.util.List; /** @@ -39,7 +40,8 @@ private Part( /** * Splits a multipart body into at most {@code partBudget} parts. * - * @return the parts found, in order; empty if the body holds none + * @return the parts found, in order; empty if the body holds none or is not parseable as + * multipart */ static List split(final String body, final String boundary, final int partBudget) { final List parts = new ArrayList<>(); @@ -68,14 +70,12 @@ static List split(final String body, final String boundary, final int part String contentType = null; int cursor = headerStart; boolean headersComplete = false; - int malformedPartEnd = -1; while (cursor < length) { if (body.startsWith(delimiter, cursor) && endsLine(body, cursor + delimiter.length())) { - // This part's headers are not followed by a blank line. Stop here: reading on would - // consume the next part's delimiter and merge its headers into this part, collapsing - // every following part into this one. - malformedPartEnd = cursor; - break; + // This part's headers are not followed by a blank line, so no conforming parser reads + // this body as multipart. Report nothing rather than the parts that happen to survive: + // the caller then keeps the raw string, and the WAF sees every byte of it. + return Collections.emptyList(); } final int newline = body.indexOf('\n', cursor); final int lineEnd = newline < 0 ? length : newline; @@ -101,12 +101,6 @@ static List split(final String body, final String boundary, final int part } cursor = newline + 1; } - if (malformedPartEnd >= 0) { - // Resume at the delimiter the headers ran into. It is past the current position, so the - // outer scan still makes progress. - position = malformedPartEnd; - continue; - } if (!headersComplete) { // Body truncated inside the headers: there is no content to report break; diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java index 9a91f6512a2..9341343a039 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java @@ -160,21 +160,14 @@ void delimitsContentExactly() { } @Test - void confinesAPartWhoseHeadersRunIntoTheNextDelimiter() { - // The first part's headers are not followed by a blank line. Reading past the delimiter would - // merge the following part's headers into this one, so a well-formed field part would be - // reported as the malformed part's own — and skipped entirely if it carried a filename. + void reportsNothingForAPartWhoseHeadersRunIntoTheNextDelimiter() { + // The first part's headers are not followed by a blank line, which no conforming parser + // accepts. Reporting the second part alone would show the WAF less than the app receives. String body = "--x\r\nX-First: 1\r\n" + "--x\r\nContent-Disposition: form-data; name=\"b\"\r\n\r\nsecond\r\n--x--"; - List parts = split(body, "x", NO_PART_BUDGET_LIMIT); - - // The surviving part is the second one: had the two merged, its content would have swallowed - // the delimiter between them. - assertEquals(1, parts.size()); - assertEquals("form-data; name=\"b\"", parts.get(0).contentDisposition); - assertEquals("second", content(body, parts.get(0))); + assertTrue(split(body, "x", NO_PART_BUDGET_LIMIT).isEmpty()); } @Test From 58ba1ec8bced70015713811bacb03a7bd6e4ee54 Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Wed, 2 Sep 2026 15:23:06 +0200 Subject: [PATCH 09/10] Drop redundant Lambda body parsing test cases and merge overlapping ones Co-Authored-By: Claude Opus 5 --- .../lambda/ContentTypeBodyParserTest.java | 77 +++---------------- .../trace/lambda/LambdaAppSecHandlerTest.java | 2 +- .../trace/lambda/MultipartSplitterTest.java | 40 ++-------- 3 files changed, 17 insertions(+), 102 deletions(-) diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java index ef71a15ae14..c55af1eaa11 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/ContentTypeBodyParserTest.java @@ -29,8 +29,6 @@ class ContentTypeBodyParserTest { value = { // content type absent or blank: best-effort JSON, as before content-type dispatch existed "{\"a\":1} | | MAP", - "{\"a\":1} | ' ' | MAP", - "{\"a\":1} | '' | MAP", "not json | | STRING", // a header holding nothing but parameters declares no type either "{\"a\":1} | '; charset=utf-8' | MAP", @@ -45,7 +43,6 @@ class ContentTypeBodyParserTest { "{\"a\":1} | application/javascript | MAP", // urlencoded "a=1 | application/x-www-form-urlencoded | MAP", - "a=1 | APPLICATION/X-WWW-FORM-URLENCODED | MAP", // a multipart body whose boundary happens to contain "json" must still reach the multipart // parser rather than being handed to the JSON parser "not multipart | multipart/x; boundary=--json | STRING", @@ -55,16 +52,11 @@ class ContentTypeBodyParserTest { // Double, which no string rule can match "{\"a\":1} | text/plain | STRING", "12345 | text/plain | STRING", - "{\"a\":1} | text/html | STRING", // anything else "{\"a\":1} | application/xml | STRING", - "{\"a\":1} | application/octet-stream | STRING", "{\"a\":1} | garbage | STRING", // a slashless header declares a type but no subtype, so it is not JSON-ish "{\"a\":1} | json | STRING", - // known gap: a JSON-carrying type whose name says neither "json" nor "javascript" keeps - // the raw string, which the WAF can still match string rules against - "{\"a\":1} | application/csp-report | STRING", }) void dispatchesOnContentType(String body, String contentType, String expectedKind) { Object parsed = parseBody(body, contentType); @@ -153,16 +145,6 @@ void doesNotSeparateUrlEncodedPairsOnSemicolons() { assertEquals(singletonList("1;b=2"), urlEncoded("a=1;b=2").get("a")); } - @Test - void parsesUrlEncodedPairsUpToThePartAllowance() { - StringBuilder body = new StringBuilder(); - for (int i = 0; i < MAX_PARTS; i++) { - body.append(i == 0 ? "" : "&").append('k').append(i).append("=v"); - } - - assertEquals(MAX_PARTS, urlEncoded(body.toString()).size()); - } - @Test void keepsUrlEncodedBodyOverThePartAllowanceAsRawString() { StringBuilder body = new StringBuilder(); @@ -177,20 +159,6 @@ void keepsUrlEncodedBodyOverThePartAllowanceAsRawString() { assertEquals(raw, parseBody(raw, "application/x-www-form-urlencoded")); } - @Test - void countsNamelessUrlEncodedPairsTowardsTheCap() { - StringBuilder body = new StringBuilder(); - for (int i = 0; i < MAX_PARTS; i++) { - body.append("=v&"); - } - body.append("a=1"); - String raw = body.toString(); - - // Nameless pairs still do decoding work, so they count towards the cap even though none of them - // ends up in the map - assertEquals(raw, parseBody(raw, "application/x-www-form-urlencoded")); - } - @Test void keepsUnparseableUrlEncodedBodyAsRawString() { // Nothing but separators: no parameter survives, so the raw body is kept @@ -234,24 +202,19 @@ void dispatchesOnEachMultipartPartsOwnContentType() { } @Test - void keepsMultipartPartsThatDeclareNoContentTypeAsRawStrings() { + void keepsMultipartPartsWithoutAUsableContentTypeAsRawStrings() { // A part with no Content-Type is text/plain per RFC 7578, section 4.4, not a body of unknown - // type: a JSON parse would hand the WAF a Double and a Boolean no string rule can match + // type: a JSON parse would hand the WAF a Double no string rule can match. An empty header + // reads the same way, and is the only other spelling to reach here — MultipartSplitter trims. Map fields = - multipart(field("amount", "12345"), field("flag", "true"), field("json", "{\"a\":1}")); + multipart( + field("amount", "12345"), + field("json", "{\"a\":1}"), + part("form-data; name=\"empty\"", "", "12345")); assertEquals("12345", fields.get("amount")); - assertEquals("true", fields.get("flag")); assertEquals("{\"a\":1}", fields.get("json")); - } - - @Test - void keepsMultipartPartsWithAnEmptyContentTypeAsRawStrings() { - // Only the empty case is worth covering: MultipartSplitter trims header values, so a - // whitespace-only Content-Type reaches the parser as "" and not as its original spelling - Map fields = multipart(part("form-data; name=\"amount\"", "", "12345")); - - assertEquals("12345", fields.get("amount")); + assertEquals("12345", fields.get("empty")); } @Test @@ -278,13 +241,14 @@ void doesNotTakeAFieldNameThatForgesAFilenameForAFilePart() { } @Test - void parsesNestedMultipartBodiesAndSkipsTheirFileParts() { + void parsesNestedMultipartBodiesAndReportsTheirFilePartsByNameOnly() { String inner = body("inner", field("nested", "value"), file("upload", "f.txt", "data")); String nesting = outer(part("form-data; name=group", "multipart/mixed; boundary=inner", inner)); Map fields = asMap(parseBody(nesting, MULTIPART)); assertEquals(singletonMap("nested", "value"), fields.get("group")); + assertEquals(singletonList("f.txt"), filenamesOf(nesting)); } @Test @@ -300,19 +264,6 @@ void sharesThePartAllowanceBetweenUrlEncodedParametersAndMultipartParts() { assertEquals(urlEncoded, fields.get("second")); } - @Test - void spendsThePartAllowanceOnlyOnPairsItReads() { - // Half the allowance each, less their two parts: they only fit if neither is charged upfront - String urlEncoded = urlEncodedPairs((MAX_PARTS - 2) / 2); - Map fields = - multipart( - part("form-data; name=first", URL_ENCODED, urlEncoded), - part("form-data; name=second", URL_ENCODED, urlEncoded)); - - assertInstanceOf(Map.class, fields.get("first")); - assertInstanceOf(Map.class, fields.get("second")); - } - private static String urlEncodedPairs(int count) { StringBuilder body = new StringBuilder(); for (int i = 0; i < count; i++) { @@ -412,14 +363,6 @@ void marksAFilePartWithoutReportingAnEmptyFilename() { assertEquals(singletonMap("user", "admin"), multipartBody(body)); } - @Test - void reportsFilenamesFromNestedMultipartParts() { - String inner = body("inner", file("attachment", "nested.txt", "bytes")); - String nesting = outer(part("form-data; name=group", "multipart/mixed; boundary=inner", inner)); - - assertEquals(singletonList("nested.txt"), filenamesOf(nesting)); - } - @Test void reportsFilenamesEvenWhenTheBodyDegradesToARawString() { // Nothing but file parts, so there is no field to report: an empty map would tell the WAF the diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java index 96a0b287cce..a44598eae9b 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/LambdaAppSecHandlerTest.java @@ -2118,7 +2118,7 @@ void extractResponseDataReturnsNullForEmptyString() { } @ParameterizedTest(name = "[{index}] content-type {0}") - @ValueSource(strings = {"", " ", "application/json"}) + @ValueSource(strings = {"", "application/json"}) void parsesAResponseBodyAsJsonWhenTheContentTypeIsBlankOrJson(String contentType) { // A blank content type says nothing about the body, so it gets the same best-effort JSON parse // as an absent one, matching the request path. diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java index 9341343a039..0edd0a05f54 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java @@ -32,7 +32,6 @@ class MultipartSplitterTest { "multipart/form-data; boundary=\"a;b c\" | a;b c", // position among the other parameters does not matter "multipart/form-data; boundary=xy; charset=x | xy", - "multipart/form-data; charset=x; boundary=xy | xy", // a parameter that merely ends in "boundary" is not one "multipart/form-data; xboundary=xy | NULL", "multipart/form-data | NULL", @@ -76,8 +75,6 @@ void rejectsABoundaryOverSeventyCharacters() { "form-data; filename=\"; name=y\" | name | NULL", // RFC 7230 optional whitespace is tolerated on both sides of the "=" "form-data; name =user | name | user", - "form-data; name= user | name | user", - "form-data; name = \"a b\" | name | a b", // a bare parameter name is not a parameter "form-data; name | name | NULL", }) @@ -174,41 +171,16 @@ void reportsNothingForAPartWhoseHeadersRunIntoTheNextDelimiter() { void matchesHeaderNamesCaseInsensitivelyAndTrimsValues() { String body = "--x\r\nCONTENT-Disposition: form-data; name=a \r\n" - + "content-type \t:\ttext/plain \r\n\r\nv\r\n--x--"; - - Part part = split(body, "x", NO_PART_BUDGET_LIMIT).get(0); - - assertEquals("form-data; name=a", part.contentDisposition); - assertEquals("text/plain", part.contentType); - } - - @Test - void reportsNoValueForAHeaderThatIsPresentButEmpty() { - String body = "--x\r\nContent-Type:\r\n\r\nv\r\n--x--"; - - Part part = split(body, "x", NO_PART_BUDGET_LIMIT).get(0); - - assertEquals("", part.contentType); - assertNull(part.contentDisposition); - } - - @Test - @Timeout(value = 10, unit = SECONDS) - void dropsHeadersItDoesNotReadWhateverTheirNumber() { - // Only Content-Disposition and Content-Type are kept, so a part may declare arbitrarily many - // others without any of them being retained. - StringBuilder headers = new StringBuilder(); - for (int i = 0; i < 100_000; i++) { - headers.append("X-Filler-").append(i).append(": ").append(i).append("\r\n"); - } - String body = "--x\r\n" + headers + "Content-Disposition: form-data; name=a\r\n\r\nv\r\n--x--"; + + "content-type \t:\ttext/plain \r\n\r\nv\r\n" + + "--x\r\nContent-Type:\r\n\r\nv\r\n--x--"; List parts = split(body, "x", NO_PART_BUDGET_LIMIT); - assertEquals(1, parts.size()); assertEquals("form-data; name=a", parts.get(0).contentDisposition); - assertNull(parts.get(0).contentType); - assertEquals("v", content(body, parts.get(0))); + assertEquals("text/plain", parts.get(0).contentType); + // A header present but empty is reported as "", distinct from the null of an absent one + assertEquals("", parts.get(1).contentType); + assertNull(parts.get(1).contentDisposition); } @Test From 693dfe0be29e1f65fff6f786bf08155920ebab0e Mon Sep 17 00:00:00 2001 From: Clara Poncet Date: Wed, 2 Sep 2026 16:54:24 +0200 Subject: [PATCH 10/10] Require a close delimiter to end its line and void a body truncated inside part headers Co-Authored-By: Claude Opus 5 --- .../datadog/trace/lambda/MultipartSplitter.java | 16 ++++++++++------ .../trace/lambda/MultipartSplitterTest.java | 10 +++++++--- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java index 7fda2126db4..a64159f94f5 100644 --- a/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java +++ b/dd-trace-core/src/main/java/datadog/trace/lambda/MultipartSplitter.java @@ -102,8 +102,10 @@ static List split(final String body, final String boundary, final int part cursor = newline + 1; } if (!headersComplete) { - // Body truncated inside the headers: there is no content to report - break; + // The body ended inside this part's headers. They are content in their own right — a + // filename lives there — and no parser accepts a body cut short like this, so report + // nothing and let the caller keep the raw string. + return Collections.emptyList(); } final int next = nextDelimiter(body, delimiter, cursor); parts.add(new Part(contentDisposition, contentType, cursor, contentEnd(body, cursor, next))); @@ -239,14 +241,16 @@ && endsLine(body, newline + 1 + delimiter.length())) { } /** - * A delimiter only delimits if its line ends there, RFC 2046 transport padding aside. A content - * line that merely starts with it — {@code --x-not-a-boundary} for a boundary of {@code x} — is - * data, and must not end the part early and hide the rest from the WAF. + * A delimiter only delimits if its line ends there, RFC 2046 transport padding aside — for the + * close delimiter, past its trailing {@code --}. A content line that merely starts with one, be + * it {@code --x-not-a-boundary} or {@code --x--not-a-close}, is data, and must not end the part + * early and hide the rest from the WAF. * * @param after the index just past the matched delimiter */ private static boolean endsLine(final String body, final int after) { - return after == body.length() || body.startsWith("--", after) || lineStart(body, after) >= 0; + final int end = body.startsWith("--", after) ? after + 2 : after; + return end == body.length() || lineStart(body, end) >= 0; } /** diff --git a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java index 0edd0a05f54..8ab01aea92c 100644 --- a/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java +++ b/dd-trace-core/src/test/java/datadog/trace/lambda/MultipartSplitterTest.java @@ -109,6 +109,9 @@ void keepsWhatWasReadOfAnUnterminatedQuotedValue() { // a truncated body still yields its last part "truncated last delimiter | --x@A: b@@v | 1", "truncated in the headers | --x@A: b | 0", + // A part truncated inside its headers voids the parts before it too: its own headers are + // content the WAF would otherwise never see + "truncated after a part | --x@@v@--x@A: b | 0", "no line break at all | --x | 0", "close delimiter only | --x-- | 0", "empty part content | --x@A: b@@ | 1", @@ -143,9 +146,10 @@ void stopsAtThePartBudget() { @Test void delimitsContentExactly() { // Dashes, line breaks, a replacement character and a multi-byte character all inside the - // content, plus a line that starts with the delimiter without being one: the reported range - // must not be thrown off by a near miss on the line-feed anchor or on the delimiter itself - String content = "--not-a-boundary\r\n--x-not-a-boundary\r\n-x\nlast�é"; + // content, plus lines that start with the part and close delimiters without being either: the + // reported range must not be thrown off by a near miss on the line-feed anchor, on the + // delimiter itself, or on its trailing dashes + String content = "--not-a-boundary\r\n--x-not-a-boundary\r\n--x--not-a-close\r\n-x\nlast�é"; String body = "--x\r\nContent-Disposition: form-data; name=a\r\n\r\n" + content + "\r\n--x--\r\n";