Skip to content

Commit 4cf7670

Browse files
authored
Guard parsed-body instrumentation from raw Spring HttpMessageConverters (#9613)
What Does This Do Skip publishing the requestBodyProcessed AppSec event when Spring’s HttpMessageConverters return raw payloads so only structured converters feed the parsed-body channel. Add focused instrumentation tests proving that raw converters no longer publish parsed bodies while form conversion still does. Extend the AppSec smoke test and controller with a custom rule and string-body endpoint to confirm the StringHttpMessageConverter path now completes without triggering the parsed-body WAF rule. Motivation Avoid false positives in WAF rules related with structured body processed like in escalation https://datadoghq.atlassian.net/browse/SCRS-1682
1 parent 0059efd commit 4cf7670

File tree

4 files changed

+163
-0
lines changed

4 files changed

+163
-0
lines changed

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HttpMessageConverterInstrumentation.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ public void methodAdvice(MethodTransformer transformer) {
9696

9797
@RequiresRequestContext(RequestContextSlot.APPSEC)
9898
public static class HttpMessageConverterReadAdvice {
99+
99100
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
100101
public static void after(
101102
@Advice.Return final Object obj,
@@ -105,6 +106,17 @@ public static void after(
105106
return;
106107
}
107108

109+
// CharSequence or byte[] cannot be treated as parsed body content, as they may lead to false
110+
// positives in the WAF rules.
111+
// TODO: These types (CharSequence, byte[]) are candidates to being deserialized before being
112+
// sent to the WAF once we implement that feature.
113+
// Possible types received by this method include: String, byte[], various DTOs/POJOs,
114+
// Collections (List, Map), Jackson JsonNode objects, XML objects, etc.
115+
// We may need to add more types to this block list in the future.
116+
if (obj instanceof CharSequence || obj instanceof byte[]) {
117+
return;
118+
}
119+
108120
CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC);
109121
BiFunction<RequestContext, Object, Flow<Void>> callback =
110122
cbp.getCallback(EVENTS.requestBodyProcessed());
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package datadog.trace.instrumentation.springweb
2+
3+
import datadog.trace.agent.test.InstrumentationSpecification
4+
import datadog.trace.api.gateway.Flow
5+
import datadog.trace.api.gateway.RequestContext
6+
import datadog.trace.api.gateway.RequestContextSlot
7+
import datadog.trace.bootstrap.instrumentation.api.AgentTracer
8+
import datadog.trace.bootstrap.instrumentation.api.TagContext
9+
import org.springframework.http.MediaType
10+
import org.springframework.http.converter.ByteArrayHttpMessageConverter
11+
import org.springframework.http.converter.FormHttpMessageConverter
12+
import org.springframework.http.converter.StringHttpMessageConverter
13+
import org.springframework.mock.http.MockHttpInputMessage
14+
import org.springframework.util.MultiValueMap
15+
16+
import java.nio.charset.StandardCharsets
17+
import java.util.function.BiFunction
18+
19+
import static datadog.trace.api.gateway.Events.EVENTS
20+
21+
class HttpMessageConverterInstrumentationTest extends InstrumentationSpecification {
22+
23+
def scope
24+
def ss = AgentTracer.get().getSubscriptionService(RequestContextSlot.APPSEC)
25+
List<Object> publishedBodies = []
26+
27+
def setup() {
28+
publishedBodies.clear()
29+
TagContext ctx = new TagContext().withRequestContextDataAppSec(new Object())
30+
def span = AgentTracer.startSpan('test-span', ctx)
31+
scope = AgentTracer.activateSpan(span)
32+
33+
ss.registerCallback(EVENTS.requestBodyProcessed(), { RequestContext reqCtx, Object body ->
34+
publishedBodies << body
35+
Flow.ResultFlow.empty()
36+
} as BiFunction<RequestContext, Object, Flow<Void>>)
37+
}
38+
39+
def cleanup() {
40+
ss.reset()
41+
scope?.close()
42+
}
43+
44+
void 'string http message converter does not publish parsed body event'() {
45+
given:
46+
def converter = new StringHttpMessageConverter()
47+
def raw = '{"value":"example"}'
48+
def message = new MockHttpInputMessage(raw.getBytes(StandardCharsets.UTF_8))
49+
message.headers.contentType = MediaType.APPLICATION_JSON
50+
51+
when:
52+
def result = converter.read(String, message)
53+
54+
then:
55+
result == raw
56+
publishedBodies.isEmpty()
57+
}
58+
59+
void 'byte array http message converter does not publish parsed body event'() {
60+
given:
61+
def converter = new ByteArrayHttpMessageConverter()
62+
def raw = '{"value":"bytes"}'.getBytes(StandardCharsets.UTF_8)
63+
def message = new MockHttpInputMessage(raw)
64+
message.headers.contentType = MediaType.APPLICATION_JSON
65+
66+
when:
67+
def result = converter.read(byte[].class, message)
68+
69+
then:
70+
Arrays.equals(result, raw)
71+
publishedBodies.isEmpty()
72+
}
73+
74+
void 'form converter continues to publish parsed body event'() {
75+
given:
76+
def converter = new FormHttpMessageConverter()
77+
def raw = 'value=object&another=value2'
78+
def message = new MockHttpInputMessage(raw.getBytes(StandardCharsets.UTF_8))
79+
message.headers.contentType = MediaType.APPLICATION_FORM_URLENCODED
80+
81+
when:
82+
def result = converter.read(MultiValueMap, message)
83+
84+
then:
85+
result instanceof MultiValueMap
86+
result.getFirst('value') == 'object'
87+
result.getFirst('another') == 'value2'
88+
publishedBodies.size() == 1
89+
def published = publishedBodies[0] as MultiValueMap
90+
published.getFirst('value') == 'object'
91+
published.getFirst('another') == 'value2'
92+
}
93+
}

dd-smoke-tests/appsec/springboot/src/main/java/datadog/smoketest/appsec/springboot/controller/WebController.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,11 @@ public String requestBody(@RequestBody BodyMappedClass obj) {
5555
return obj.v;
5656
}
5757

58+
@PostMapping("/api_security/request-body-string")
59+
public String requestBodyString(@RequestBody String body) {
60+
return body;
61+
}
62+
5863
@GetMapping("/sqli/query")
5964
public String sqliQuery(@RequestParam("id") String id) throws Exception {
6065
Connection conn = DriverManager.getConnection("jdbc:h2:mem:testdb", "sa", "");

dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/AppSecHttpMessageConverterSmokeTest.groovy

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,37 @@ class AppSecHttpMessageConverterSmokeTest extends AbstractAppSecServerSmokeTest
1717

1818
@Override
1919
ProcessBuilder createProcessBuilder() {
20+
String customRulesPath = "${buildDirectory}/tmp/appsec_http_message_converter_rules.json"
21+
mergeRules(
22+
customRulesPath,
23+
[
24+
[
25+
id : '__test_string_http_message_converter',
26+
name : 'test rule for string http message converter',
27+
tags : [
28+
type : 'test',
29+
category : 'test',
30+
confidence: '1',
31+
],
32+
conditions : [
33+
[
34+
parameters: [
35+
inputs: [[address: 'server.request.body']],
36+
regex : 'dd-test-http-message-converter',
37+
],
38+
operator : 'match_regex',
39+
]
40+
],
41+
transformers: [],
42+
on_match : ['block']
43+
]
44+
])
45+
2046
String springBootShadowJar = System.getProperty("datadog.smoketest.appsec.springboot.shadowJar.path")
2147

2248
List<String> command = new ArrayList<>()
2349
command.add(javaPath())
50+
// command.add("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005")
2451
command.addAll(defaultJavaProperties)
2552
command.addAll(defaultAppSecProperties)
2653
command.addAll((String[]) [
@@ -63,6 +90,32 @@ class AppSecHttpMessageConverterSmokeTest extends AbstractAppSecServerSmokeTest
6390
assert schema == [["main": [[[["key": [8], "value": [16]]]], ["len": 2]], "nullable": [1]]]
6491
}
6592

93+
void 'string http message converter raw body does not trigger parsed body rule'() {
94+
given:
95+
def url = "http://localhost:${httpPort}/api_security/request-body-string"
96+
def rawBody = '{"value":"dd-test-http-message-converter"}'
97+
def request = new Request.Builder()
98+
.url(url)
99+
.post(RequestBody.create(MediaType.get('application/json'), rawBody))
100+
.build()
101+
102+
when:
103+
final response = client.newCall(request).execute()
104+
105+
then:
106+
response.code() == 200
107+
response.body().string() == rawBody
108+
109+
when:
110+
waitForTraceCount(1)
111+
112+
then:
113+
def spanWithTrigger = rootSpans.find { span ->
114+
(span.triggers ?: []).any { it['rule']['id'] == '__test_string_http_message_converter' }
115+
}
116+
assert spanWithTrigger == null
117+
}
118+
66119
private static byte[] unzip(final String text) {
67120
final inflaterStream = new GZIPInputStream(new ByteArrayInputStream(text.decodeBase64()))
68121
return inflaterStream.getBytes()

0 commit comments

Comments
 (0)