Skip to content

Commit 66637f0

Browse files
committed
test(appsec): add Netty integration tests for multipart filenames and file content
Also fix HttpPostRequestDecoderInstrumentation to fire on PREEPILOGUE+isLastChunk so that multipart requests delivered as a FullHttpRequest (single shot) trigger the requestBodyProcessed/requestFilesFilenames/requestFilesContent callbacks. The PREEPILOGUE→EPILOGUE transition requires a second parseBody() call which never comes when the full request arrives in one shot; PREEPILOGUE+isLastChunk is equivalent to EPILOGUE in that scenario.
1 parent 3feaaa4 commit 66637f0

3 files changed

Lines changed: 93 additions & 2 deletions

File tree

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
136136
ss.registerCallback(events.requestBodyDone(), callbacks.requestBodyEndCb)
137137
ss.registerCallback(events.requestBodyProcessed(), callbacks.requestBodyObjectCb)
138138
ss.registerCallback(events.requestFilesFilenames(), callbacks.requestFilesFilenamesCb)
139+
ss.registerCallback(events.requestFilesContent(), callbacks.requestFilesContentCb)
139140
ss.registerCallback(events.responseBody(), callbacks.responseBodyObjectCb)
140141
ss.registerCallback(events.responseStarted(), callbacks.responseStartedCb)
141142
ss.registerCallback(events.responseHeader(), callbacks.responseHeaderCb)
@@ -372,6 +373,10 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
372373
false
373374
}
374375

376+
boolean testBodyFilesContent() {
377+
false
378+
}
379+
375380
boolean testBodyJson() {
376381
false
377382
}
@@ -1646,6 +1651,29 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
16461651
response.close()
16471652
}
16481653

1654+
def 'test instrumentation gateway file upload content'() {
1655+
setup:
1656+
assumeTrue(testBodyFilesContent())
1657+
RequestBody fileBody = RequestBody.create(MediaType.parse('application/octet-stream'), 'file content')
1658+
def body = new MultipartBody.Builder()
1659+
.setType(MultipartBody.FORM)
1660+
.addFormDataPart('file', 'test.bin', fileBody)
1661+
.build()
1662+
def httpRequest = request(BODY_MULTIPART, 'POST', body).build()
1663+
def response = client.newCall(httpRequest).execute()
1664+
1665+
when:
1666+
TEST_WRITER.waitForTraces(1)
1667+
1668+
then:
1669+
TEST_WRITER.get(0).any {
1670+
it.getTag('request.body.files_content') == '[file content]'
1671+
}
1672+
1673+
cleanup:
1674+
response.close()
1675+
}
1676+
16491677
def 'test instrumentation gateway json request body'() {
16501678
setup:
16511679
assumeTrue(testBodyJson())
@@ -2581,6 +2609,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
25812609
boolean responseBodyTag
25822610
Object responseBody
25832611
List<String> uploadedFilenames
2612+
List<String> uploadedFilesContent
25842613
}
25852614

25862615
static final String stringOrEmpty(String string) {
@@ -2757,6 +2786,15 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
27572786
Flow.ResultFlow.empty()
27582787
} as BiFunction<RequestContext, List<String>, Flow<Void>>)
27592788

2789+
final BiFunction<RequestContext, List<String>, Flow<Void>> requestFilesContentCb =
2790+
({
2791+
RequestContext rqCtxt, List<String> contents ->
2792+
rqCtxt.traceSegment.setTagTop('request.body.files_content', contents as String)
2793+
Context context = rqCtxt.getData(RequestContextSlot.APPSEC)
2794+
context.uploadedFilesContent = contents
2795+
Flow.ResultFlow.empty()
2796+
} as BiFunction<RequestContext, List<String>, Flow<Void>>)
2797+
27602798
final BiFunction<RequestContext, Object, Flow<Void>> responseBodyObjectCb =
27612799
({
27622800
RequestContext rqCtxt, Object obj ->

dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/HttpPostRequestDecoderInstrumentation.java

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,15 @@ public Reference[] additionalMuzzleReferences() {
6161
Reference.EXPECTS_NON_STATIC,
6262
"currentStatus",
6363
"Lio/netty/handler/codec/http/multipart/HttpPostRequestDecoder$MultiPartStatus;")
64+
.withField(new String[0], Reference.EXPECTS_NON_STATIC, "isLastChunk", "Z")
6465
.build(),
6566
new Reference.Builder("io.netty.handler.codec.http.multipart.HttpPostStandardRequestDecoder")
6667
.withField(
6768
new String[0],
6869
Reference.EXPECTS_NON_STATIC,
6970
"currentStatus",
7071
"Lio/netty/handler/codec/http/multipart/HttpPostRequestDecoder$MultiPartStatus;")
72+
.withField(new String[0], Reference.EXPECTS_NON_STATIC, "isLastChunk", "Z")
7173
.build()
7274
};
7375
}
@@ -88,10 +90,17 @@ static class ParseBodyAdvice {
8890
static void after(
8991
@Advice.This InterfaceHttpPostRequestDecoder thiz,
9092
@Advice.FieldValue("currentStatus") Enum currentStatus,
93+
@Advice.FieldValue("isLastChunk") boolean isLastChunk,
9194
@ActiveRequestContext RequestContext requestContext,
9295
@Advice.Thrown(readOnly = false) Throwable thr) {
93-
if (!currentStatus.name().equals("EPILOGUE")) {
94-
return;
96+
String statusName = currentStatus.name();
97+
if (!statusName.equals("EPILOGUE")) {
98+
// For multipart decoders, the PREEPILOGUE→EPILOGUE transition requires a second
99+
// parseBody() call that never comes when the full request arrives in one shot.
100+
// Fire on PREEPILOGUE + isLastChunk to handle that case.
101+
if (!statusName.equals("PREEPILOGUE") || !isLastChunk) {
102+
return;
103+
}
95104
}
96105

97106
CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC);
@@ -160,6 +169,8 @@ static void after(
160169
BlockResponseFunction brf = requestContext.getBlockResponseFunction();
161170
if (brf != null) {
162171
brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba);
172+
// effectivelyBlocked() is intentionally absent: tryCommitBlockingResponse finishes
173+
// the span synchronously in this Netty path; calling it on a finished span throws.
163174
thr = new BlockingException("Blocked request (multipart/urlencoded post data)");
164175
}
165176
}

dd-java-agent/instrumentation/netty/netty-4.1/src/test/groovy/Netty41ServerTest.groovy

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import io.netty.handler.codec.http.HttpResponseStatus
2727
import io.netty.handler.codec.http.HttpServerCodec
2828
import io.netty.handler.codec.http.multipart.Attribute
2929
import io.netty.handler.codec.http.multipart.HttpPostRequestDecoder
30+
import io.netty.handler.codec.http.multipart.InterfaceHttpData
3031
import io.netty.handler.codec.http.websocketx.BinaryWebSocketFrame
3132
import io.netty.handler.codec.http.websocketx.CloseWebSocketFrame
3233
import io.netty.handler.codec.http.websocketx.ContinuationWebSocketFrame
@@ -36,6 +37,7 @@ import io.netty.handler.logging.LogLevel
3637
import io.netty.handler.logging.LoggingHandler
3738
import io.netty.util.CharsetUtil
3839

40+
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_MULTIPART
3941
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.BODY_URLENCODED
4042
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR
4143
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
@@ -133,6 +135,31 @@ abstract class Netty41ServerTest extends HttpServerTest<EventLoopGroup> {
133135
response = new DefaultFullHttpResponse(HTTP_1_1, HttpResponseStatus.valueOf(endpoint.status), content)
134136
}
135137
break
138+
case BODY_MULTIPART:
139+
if (msg instanceof FullHttpRequest) {
140+
HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(
141+
new HttpRequest() {
142+
@Delegate
143+
HttpRequest delegate = request
144+
})
145+
146+
Map m
147+
try {
148+
decoder.offer(msg)
149+
150+
m = decoder.bodyHttpDatas
151+
.findAll { it.httpDataType == InterfaceHttpData.HttpDataType.Attribute }
152+
.collectEntries { d -> [d.name, [((Attribute) d).value]] }
153+
} finally {
154+
try {
155+
decoder.destroy()
156+
} catch (Exception ignored) {}
157+
}
158+
159+
content = Unpooled.copiedBuffer(m as String, CharsetUtil.UTF_8)
160+
response = new DefaultFullHttpResponse(HTTP_1_1, HttpResponseStatus.valueOf(endpoint.status), content)
161+
}
162+
break
136163
case REDIRECT:
137164
response = new DefaultFullHttpResponse(HTTP_1_1, HttpResponseStatus.valueOf(endpoint.status))
138165
response.headers().set(HttpHeaderNames.LOCATION, endpoint.body)
@@ -270,6 +297,21 @@ abstract class Netty41ServerTest extends HttpServerTest<EventLoopGroup> {
270297
true
271298
}
272299

300+
@Override
301+
boolean testBodyMultipart() {
302+
true
303+
}
304+
305+
@Override
306+
boolean testBodyFilenames() {
307+
true
308+
}
309+
310+
@Override
311+
boolean testBodyFilesContent() {
312+
true
313+
}
314+
273315
@Override
274316
boolean testBlocking() {
275317
true

0 commit comments

Comments
 (0)