Skip to content

Commit c3d2ee3

Browse files
authored
Add server.request.body.filenames AppSec address for Undertow and Play (#11174)
Add server.request.body.filenames support for Undertow and Play - Undertow: extract filenames from FormData attachments in MultiPartUploadHandlerInstrumentation - Play 2.5/2.6: extract filenames from MultipartFormData.files() in BodyParserHelpers Both implementations fire the requestFilesFilenames() IG event and support blocking on malicious filenames. Fix server.request.body.filenames for in-memory uploads in Undertow 2.2 In undertow 2.2+, FormValueImpl.isFile() returns false for in-memory file uploads (file size below fileSizeThreshold) because it checks fileItem.isInMemory(). Use getFileName() to identify file uploads regardless of storage, which works across all undertow versions. Also check the filenames callback before building the list to avoid allocations on requests where the feature is inactive. Decouple requestBodyProcessed and requestFilesFilenames callbacks in Undertow Both callbacks are now fetched upfront; the method only returns early when both are null. Previously an early return on requestBodyProcessed==null silently skipped filename detection, breaking deployments with filename-only WAF rules. Fix Scala 2.13 muzzle incompatibility in Play multipart filenames support Use reflection to invoke MultipartFormData.files() so the bytecode does not embed a hard reference to the Scala 2.11/2.12 return type (Lscala/collection/Seq;). In Scala 2.13 (Play 2.7+) the method returns scala.collection.immutable.Seq, causing muzzle to disable the entire PlayBodyParsersInstrumentation and breaking all body-parsing features. Also enable testBodyFilenames() in Play 2.5/2.6/2.7 test suites. Skip filenames WAF callback when body callback already caused a block Avoids a redundant WAF evaluation when the request was already blocked by the requestBodyProcessed callback. The filenamesCb is now only invoked when t == null (no block committed yet), and the inner t == null guard is removed since it is now guaranteed by the outer condition. Add unit tests for FormDataMap and BodyParserHelpers.jsValueToJavaObject Fix tryCommitBlockingResponse return-value check; simplify appsec guard; add play-2.5 unit tests Align play-appsec-2.6 handleMultipartFilenames with play-appsec-2.5 safe pattern Cache MultipartFormData.files() Method to avoid per-request reflection lookup Fix FormDataMapTest anonymous FormValue missing undertow 2.2.x methods Add getCharset(), getFileItem(), isFileItem(), and isBigField() stubs to the anonymous FormData.FormValue in addInMemoryFileValue(). These abstract methods were added in undertow 2.2.x and caused compilation failure when the latestDepForkedTest resolved the latest 2.2.x release. Use Proxy in FormDataMapTest to handle undertow 2.0/2.2 interface differences FormData.FormValue gained getCharset(), getFileItem(), isFileItem(), and isBigField() in undertow 2.2.x. A static anonymous class can't implement all versions simultaneously. Using a Proxy resolves the interface at runtime, so the test compiles against undertow 2.0 and runs correctly against the latest 2.2.x dependency. Move testBodyFilenames from AbstractPlayServerTest to play-appsec-2.6 tests AbstractPlayServerTest is shared by both play-2.6 (no AppSec) and play-appsec-2.6 tests. Setting testBodyFilenames=true there caused the plain play-2.6 tests to check the request.body.filenames tag, which is never set without the AppSec instrumentation. Move the override into PlayServerTest and PlayAsyncServerTest in play-appsec-2.6, which are the modules where the instrumentation is active. Fix dual-fire rule and BlockingException propagation clarity in Play and Undertow multipart - Play 2.5/2.6 handleMultipartFormData: apply pendingBlock pattern so both body and filenames callbacks always fire even when one of them blocks first - Play 2.5/2.6: split catch(Exception) into explicit catch(BlockingException)/catch(Exception) to make blocking propagation unambiguous (was relying on non-obvious re-throw in handleException) - Play 2.5/2.6: extract collectFilenames() as package-private to enable unit testing - Play 2.5/2.6 BodyParserHelpersTest: add collectFilenames unit tests with real FilePart instances - Undertow MultiPartUploadHandlerInstrumentation: remove && t == null guard on filenames callback so filenames fires even when body already blocked Make filenames blocking guard consistent with body blocking guard in Undertow advice Both now use `if (success && t == null)` to avoid overwriting a pre-existing throwable, matching the pattern already used in the body-callback block. Fix collectFilenames to accept java.util.Iterator to enable unit testing Adapt the scala.collection.Iterator from MULTIPART_FILES_METHOD with an anonymous java.util.Iterator wrapper at the call site, so collectFilenames can be called directly from Java tests without a Scala iterator. Replace anonymous iterator with named ScalaIteratorAdapter; add to helperClassNames The anonymous java.util.Iterator class compiled as BodyParserHelpers$1 was missing from helperClassNames, causing muzzle validation failures. Replace with a named static inner class ScalaIteratorAdapter and declare it explicitly in all helperClassNames arrays that already reference BodyParserHelpers. Guard filenames tryCommitBlockingResponse if body already blocked in Undertow UndertowBlockResponseFunction.tryCommitBlockingResponse is not idempotent: it overwrites exchange attachments and re-dispatches on the IO thread on every call. Move the t == null guard before the call so the filename blocking path is skipped when the body blocking path has already committed a response. Trigger devflow re-evaluation Merge branch 'master' into alejandro.gonzalez/APPSEC-61873-4-undertow-play Co-authored-by: alejandro.gonzalez <alejandro.gonzalez@datadoghq.com>
1 parent 43fc0e8 commit c3d2ee3

24 files changed

Lines changed: 779 additions & 35 deletions

File tree

dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/BodyParserHelpers.java

Lines changed: 111 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import datadog.trace.api.gateway.RequestContextSlot;
2020
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
2121
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
22+
import java.lang.reflect.Method;
2223
import java.util.ArrayList;
2324
import java.util.HashMap;
2425
import java.util.List;
@@ -47,6 +48,21 @@ public class BodyParserHelpers {
4748
private static final Logger log = LoggerFactory.getLogger(BodyParserHelpers.class);
4849
public static final int MAX_RECURSION = 15;
4950

51+
// Cached via reflection to avoid embedding a hard binary reference to
52+
// files():Lscala/collection/Seq; — the return type changed to
53+
// Lscala/collection/immutable/Seq; in Scala 2.13 (Play 2.7+), which would
54+
// cause muzzle to disable the instrumentation for Play 2.7.
55+
private static final Method MULTIPART_FILES_METHOD;
56+
57+
static {
58+
Method m = null;
59+
try {
60+
m = MultipartFormData.class.getMethod("files");
61+
} catch (Exception ignored) {
62+
}
63+
MULTIPART_FILES_METHOD = m;
64+
}
65+
5066
private static JFunction1<
5167
scala.collection.immutable.Map<String, Seq<String>>,
5268
scala.collection.immutable.Map<String, Seq<String>>>
@@ -105,20 +121,91 @@ private static String handleText(String s) {
105121

106122
private static MultipartFormData<?> handleMultipartFormData(MultipartFormData<?> data) {
107123
scala.collection.immutable.Map<String, Seq<String>> mpfd = data.asFormUrlEncoded();
108-
109-
if (mpfd == null || mpfd.isEmpty()) {
110-
return data;
124+
BlockingException pendingBlock = null;
125+
126+
if (mpfd != null && !mpfd.isEmpty()) {
127+
try {
128+
Object conv = tryConvertingScalaContainers(mpfd, MAX_CONVERSION_DEPTH);
129+
handleArbitraryPostData(conv, "multipartFormData");
130+
} catch (BlockingException be) {
131+
pendingBlock = be;
132+
} catch (Exception e) {
133+
log.debug("Error handling result of multipartFormData BodyParser", e);
134+
}
111135
}
112136

113137
try {
114-
Object conv = tryConvertingScalaContainers(mpfd, MAX_CONVERSION_DEPTH);
115-
handleArbitraryPostData(conv, "multipartFormData");
138+
if (MULTIPART_FILES_METHOD != null) {
139+
Object files = MULTIPART_FILES_METHOD.invoke(data);
140+
if (files instanceof scala.collection.Iterable) {
141+
handleMultipartFilenames(
142+
new ScalaIteratorAdapter(((scala.collection.Iterable<?>) files).iterator()));
143+
}
144+
}
145+
} catch (BlockingException be) {
146+
if (pendingBlock == null) pendingBlock = be;
116147
} catch (Exception e) {
117-
handleException(e, "Error handling result of multipartFormData BodyParser");
148+
log.debug("Error handling multipartFormData filenames", e);
118149
}
150+
151+
if (pendingBlock != null) throw pendingBlock;
119152
return data;
120153
}
121154

155+
private static void handleMultipartFilenames(java.util.Iterator<?> iterator) {
156+
AgentSpan span = activeSpan();
157+
if (span == null) {
158+
return;
159+
}
160+
RequestContext reqCtx = span.getRequestContext();
161+
if (reqCtx == null || reqCtx.getData(RequestContextSlot.APPSEC) == null) {
162+
return;
163+
}
164+
165+
List<String> filenames = collectFilenames(iterator);
166+
if (filenames.isEmpty()) {
167+
return;
168+
}
169+
170+
CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC);
171+
BiFunction<RequestContext, List<String>, Flow<Void>> callback =
172+
cbp.getCallback(EVENTS.requestFilesFilenames());
173+
if (callback == null) {
174+
return;
175+
}
176+
executeFilenamesCallback(reqCtx, callback, filenames);
177+
}
178+
179+
static List<String> collectFilenames(java.util.Iterator<?> iterator) {
180+
List<String> filenames = new ArrayList<>();
181+
while (iterator.hasNext()) {
182+
MultipartFormData.FilePart<?> part = (MultipartFormData.FilePart<?>) iterator.next();
183+
String filename = part.filename();
184+
if (filename != null && !filename.isEmpty()) {
185+
filenames.add(filename);
186+
}
187+
}
188+
return filenames;
189+
}
190+
191+
private static void executeFilenamesCallback(
192+
RequestContext reqCtx,
193+
BiFunction<RequestContext, List<String>, Flow<Void>> callback,
194+
List<String> filenames) {
195+
Flow<Void> flow = callback.apply(reqCtx, filenames);
196+
Flow.Action action = flow.getAction();
197+
if (action instanceof Flow.Action.RequestBlockingAction) {
198+
Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action;
199+
BlockResponseFunction brf = reqCtx.getBlockResponseFunction();
200+
if (brf != null) {
201+
boolean success = brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba);
202+
if (success) {
203+
throw new BlockingException("Blocked request (multipart file upload)");
204+
}
205+
}
206+
}
207+
}
208+
122209
public static Function1<JsValue, JsValue> getHandleJsonF() {
123210
return HANDLE_JSON;
124211
}
@@ -302,4 +389,22 @@ private static Object jsNodeToJavaObject(JsonNode value, int maxRecursion) {
302389
return value.asText("");
303390
}
304391
}
392+
393+
static final class ScalaIteratorAdapter implements java.util.Iterator<Object> {
394+
private final Iterator<?> delegate;
395+
396+
ScalaIteratorAdapter(Iterator<?> delegate) {
397+
this.delegate = delegate;
398+
}
399+
400+
@Override
401+
public boolean hasNext() {
402+
return delegate.hasNext();
403+
}
404+
405+
@Override
406+
public Object next() {
407+
return delegate.next();
408+
}
409+
}
305410
}

dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/FormUrlEncodedInstrumentation.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public String instrumentedType() {
3131
@Override
3232
public String[] helperClassNames() {
3333
return new String[] {
34-
packageName + ".BodyParserHelpers",
34+
packageName + ".BodyParserHelpers", packageName + ".BodyParserHelpers$ScalaIteratorAdapter",
3535
};
3636
}
3737

dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/PlayBodyParsersInstrumentation.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public String[] knownMatchingTypes() {
3838
@Override
3939
public String[] helperClassNames() {
4040
return new String[] {
41-
packageName + ".BodyParserHelpers",
41+
packageName + ".BodyParserHelpers", packageName + ".BodyParserHelpers$ScalaIteratorAdapter",
4242
};
4343
}
4444

dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/ResultsStatusInstrumentation.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public String instrumentedType() {
2828
@Override
2929
public String[] helperClassNames() {
3030
return new String[] {
31-
packageName + ".BodyParserHelpers",
31+
packageName + ".BodyParserHelpers", packageName + ".BodyParserHelpers$ScalaIteratorAdapter",
3232
};
3333
}
3434

dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/TolerantJsonInstrumentation.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public String instrumentedType() {
3535
@Override
3636
public String[] helperClassNames() {
3737
return new String[] {
38-
packageName + ".BodyParserHelpers",
38+
packageName + ".BodyParserHelpers", packageName + ".BodyParserHelpers$ScalaIteratorAdapter",
3939
};
4040
}
4141

dd-java-agent/instrumentation/play/play-appsec-2.5/src/main/java/datadog/trace/instrumentation/play25/appsec/TolerantTextInstrumentation.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ public String instrumentedType() {
3030
@Override
3131
public String[] helperClassNames() {
3232
return new String[] {
33-
packageName + ".BodyParserHelpers",
33+
packageName + ".BodyParserHelpers", packageName + ".BodyParserHelpers$ScalaIteratorAdapter",
3434
};
3535
}
3636

dd-java-agent/instrumentation/play/play-appsec-2.5/src/test/groovy/datadog/trace/instrumentation/play25/server/PlayServerTest.groovy

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ class PlayServerTest extends HttpServerTest<Server> {
8383
true
8484
}
8585

86+
@Override
87+
boolean testBodyFilenames() {
88+
true
89+
}
90+
8691
@Override
8792
boolean testBlocking() {
8893
true
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
package datadog.trace.instrumentation.play25.appsec;
2+
3+
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertNull;
5+
import static org.junit.jupiter.api.Assertions.assertTrue;
6+
7+
import java.lang.reflect.Method;
8+
import java.math.BigDecimal;
9+
import java.util.Arrays;
10+
import java.util.Collections;
11+
import java.util.List;
12+
import java.util.Map;
13+
import org.junit.jupiter.api.Test;
14+
import play.api.libs.json.JsValue;
15+
import play.api.mvc.MultipartFormData;
16+
17+
class BodyParserHelpersTest {
18+
19+
private static JsValue parse(String json) {
20+
return play.api.libs.json.Json$.MODULE$.parse(json);
21+
}
22+
23+
@Test
24+
void jsValueToJavaObject_nullInputReturnsNull() {
25+
assertNull(BodyParserHelpers.jsValueToJavaObject(null));
26+
}
27+
28+
@Test
29+
void jsValueToJavaObject_jsNullReturnsNull() {
30+
assertNull(BodyParserHelpers.jsValueToJavaObject(parse("null")));
31+
}
32+
33+
@Test
34+
void jsValueToJavaObject_string() {
35+
Object result = BodyParserHelpers.jsValueToJavaObject(parse("\"hello\""));
36+
assertEquals("hello", result);
37+
}
38+
39+
@Test
40+
void jsValueToJavaObject_number() {
41+
Object result = BodyParserHelpers.jsValueToJavaObject(parse("42"));
42+
assertTrue(result instanceof BigDecimal);
43+
assertEquals(0, ((BigDecimal) result).compareTo(new BigDecimal("42")));
44+
}
45+
46+
@Test
47+
void jsValueToJavaObject_booleanTrue() {
48+
Object result = BodyParserHelpers.jsValueToJavaObject(parse("true"));
49+
assertEquals(Boolean.TRUE, result);
50+
}
51+
52+
@Test
53+
void jsValueToJavaObject_booleanFalse() {
54+
Object result = BodyParserHelpers.jsValueToJavaObject(parse("false"));
55+
assertEquals(Boolean.FALSE, result);
56+
}
57+
58+
@Test
59+
@SuppressWarnings("unchecked")
60+
void jsValueToJavaObject_object() {
61+
Object result = BodyParserHelpers.jsValueToJavaObject(parse("{\"key\":\"value\",\"num\":1}"));
62+
assertTrue(result instanceof Map);
63+
Map<String, Object> map = (Map<String, Object>) result;
64+
assertEquals("value", map.get("key"));
65+
assertTrue(map.get("num") instanceof BigDecimal);
66+
}
67+
68+
@Test
69+
@SuppressWarnings("unchecked")
70+
void jsValueToJavaObject_array() {
71+
Object result = BodyParserHelpers.jsValueToJavaObject(parse("[\"a\",\"b\",\"c\"]"));
72+
assertTrue(result instanceof List);
73+
List<Object> list = (List<Object>) result;
74+
assertEquals(3, list.size());
75+
assertEquals("a", list.get(0));
76+
assertEquals("b", list.get(1));
77+
assertEquals("c", list.get(2));
78+
}
79+
80+
@Test
81+
@SuppressWarnings("unchecked")
82+
void jsValueToJavaObject_nestedObject() {
83+
Object result =
84+
BodyParserHelpers.jsValueToJavaObject(parse("{\"outer\":{\"inner\":\"deep\"}}"));
85+
assertTrue(result instanceof Map);
86+
Map<String, Object> outer = (Map<String, Object>) result;
87+
assertTrue(outer.get("outer") instanceof Map);
88+
Map<String, Object> inner = (Map<String, Object>) outer.get("outer");
89+
assertEquals("deep", inner.get("inner"));
90+
}
91+
92+
@Test
93+
void jsValueToJavaObject_zeroRecursionReturnsNull() {
94+
Object result = BodyParserHelpers.jsValueToJavaObject(parse("{\"key\":\"value\"}"), 0);
95+
assertNull(result);
96+
}
97+
98+
@Test
99+
@SuppressWarnings("unchecked")
100+
void jsValueToJavaObject_recursionLimitTruncatesNesting() {
101+
// depth=1: outer object is converted, but children exceed the limit and become null
102+
Object result = BodyParserHelpers.jsValueToJavaObject(parse("{\"a\":{\"b\":\"val\"}}"), 1);
103+
assertTrue(result instanceof Map);
104+
Map<String, Object> map = (Map<String, Object>) result;
105+
assertNull(map.get("a"));
106+
}
107+
108+
// --- collectFilenames tests ---
109+
110+
@Test
111+
void collectFilenames_emptyIterator() {
112+
List<String> result = BodyParserHelpers.collectFilenames(Collections.emptyIterator());
113+
assertTrue(result.isEmpty());
114+
}
115+
116+
@Test
117+
void collectFilenames_nullFilenameExcluded() throws Exception {
118+
List<String> result =
119+
BodyParserHelpers.collectFilenames(
120+
Collections.<Object>singletonList(filePart("f", null)).iterator());
121+
assertTrue(result.isEmpty());
122+
}
123+
124+
@Test
125+
void collectFilenames_emptyFilenameExcluded() throws Exception {
126+
List<String> result =
127+
BodyParserHelpers.collectFilenames(
128+
Collections.<Object>singletonList(filePart("f", "")).iterator());
129+
assertTrue(result.isEmpty());
130+
}
131+
132+
@Test
133+
void collectFilenames_validFilenameIncluded() throws Exception {
134+
List<String> result =
135+
BodyParserHelpers.collectFilenames(
136+
Collections.<Object>singletonList(filePart("f", "evil.php")).iterator());
137+
assertEquals(Collections.singletonList("evil.php"), result);
138+
}
139+
140+
@Test
141+
void collectFilenames_mixedPartsFiltered() throws Exception {
142+
List<Object> parts =
143+
Arrays.<Object>asList(
144+
filePart("f1", "a.pdf"),
145+
filePart("f2", null),
146+
filePart("f3", ""),
147+
filePart("f4", "b.jpg"));
148+
List<String> result = BodyParserHelpers.collectFilenames(parts.iterator());
149+
assertEquals(Arrays.asList("a.pdf", "b.jpg"), result);
150+
}
151+
152+
@SuppressWarnings("unchecked")
153+
private static MultipartFormData.FilePart<Object> filePart(String key, String filename)
154+
throws Exception {
155+
// FilePart is a Scala case class nested in object MultipartFormData.
156+
// Use the companion object's apply() to avoid JVM inner-class constructor complexity.
157+
Class<?> companionClass = Class.forName("play.api.mvc.MultipartFormData$FilePart$");
158+
Object companion = companionClass.getField("MODULE$").get(null);
159+
for (Method m : companionClass.getMethods()) {
160+
if ("apply".equals(m.getName()) && m.getParameterCount() == 4) {
161+
return (MultipartFormData.FilePart<Object>)
162+
m.invoke(companion, key, filename, scala.None$.MODULE$, new Object());
163+
}
164+
}
165+
throw new IllegalStateException("FilePart.apply(4 params) not found");
166+
}
167+
}

0 commit comments

Comments
 (0)