Skip to content

Commit 619e7b9

Browse files
jandro996jandro996
committed
refactor(appsec): extract Netty file content reading to NettyFileUploadContentReader helper
Move the in-memory/disk-backed FileUpload content reading logic out of ParseBodyAdvice into a testable static helper class. Also fixes the partial-read risk on disk-backed uploads (single fis.read() replaced by a loop) and broadens the catch to Exception to handle IllegalReferenceCountException from released ByteBufs.
1 parent 66637f0 commit 619e7b9

3 files changed

Lines changed: 187 additions & 23 deletions

File tree

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

Lines changed: 8 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,13 @@
1818
import datadog.trace.api.gateway.RequestContext;
1919
import datadog.trace.api.gateway.RequestContextSlot;
2020
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
21-
import io.netty.buffer.ByteBuf;
2221
import io.netty.handler.codec.http.EmptyHttpHeaders;
2322
import io.netty.handler.codec.http.multipart.Attribute;
2423
import io.netty.handler.codec.http.multipart.FileUpload;
2524
import io.netty.handler.codec.http.multipart.InterfaceHttpData;
2625
import io.netty.handler.codec.http.multipart.InterfaceHttpPostRequestDecoder;
27-
import java.io.FileInputStream;
2826
import java.io.IOException;
2927
import java.lang.reflect.UndeclaredThrowableException;
30-
import java.nio.charset.StandardCharsets;
3128
import java.util.ArrayList;
3229
import java.util.LinkedHashMap;
3330
import java.util.List;
@@ -74,6 +71,13 @@ public Reference[] additionalMuzzleReferences() {
7471
};
7572
}
7673

74+
@Override
75+
public String[] helperClassNames() {
76+
return new String[] {
77+
packageName + ".NettyFileUploadContentReader",
78+
};
79+
}
80+
7781
@Override
7882
public void methodAdvice(MethodTransformer transformer) {
7983
transformer.applyAdvice(
@@ -83,7 +87,6 @@ public void methodAdvice(MethodTransformer transformer) {
8387

8488
@RequiresRequestContext(RequestContextSlot.APPSEC)
8589
static class ParseBodyAdvice {
86-
private static final int MAX_CONTENT_BYTES = 4096;
8790
private static final int MAX_FILES_TO_INSPECT = 25;
8891

8992
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
@@ -139,25 +142,7 @@ static void after(
139142
filenames.add(filename);
140143
}
141144
if (contentCb != null && filesContent.size() < MAX_FILES_TO_INSPECT) {
142-
String contentStr = "";
143-
try {
144-
if (fileUpload.isInMemory()) {
145-
ByteBuf buf = fileUpload.getByteBuf();
146-
int length = Math.min(MAX_CONTENT_BYTES, buf.readableBytes());
147-
byte[] bytes = new byte[length];
148-
buf.getBytes(buf.readerIndex(), bytes);
149-
contentStr = new String(bytes, StandardCharsets.ISO_8859_1);
150-
} else {
151-
byte[] bytes = new byte[MAX_CONTENT_BYTES];
152-
int read;
153-
try (FileInputStream fis = new FileInputStream(fileUpload.getFile())) {
154-
read = fis.read(bytes);
155-
}
156-
contentStr = new String(bytes, 0, read < 0 ? 0 : read, StandardCharsets.ISO_8859_1);
157-
}
158-
} catch (IOException ignored) {
159-
}
160-
filesContent.add(contentStr);
145+
filesContent.add(NettyFileUploadContentReader.readContent(fileUpload));
161146
}
162147
}
163148
}

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package datadog.trace.instrumentation.netty41;
2+
3+
import io.netty.buffer.ByteBuf;
4+
import io.netty.handler.codec.http.multipart.FileUpload;
5+
import java.io.FileInputStream;
6+
import java.nio.charset.StandardCharsets;
7+
8+
/** Reads uploaded file content from a Netty {@link FileUpload} for WAF inspection. */
9+
public final class NettyFileUploadContentReader {
10+
public static final int MAX_CONTENT_BYTES = 4096;
11+
12+
public static String readContent(FileUpload fileUpload) {
13+
try {
14+
if (fileUpload.isInMemory()) {
15+
ByteBuf buf = fileUpload.getByteBuf();
16+
int length = Math.min(MAX_CONTENT_BYTES, buf.readableBytes());
17+
byte[] bytes = new byte[length];
18+
buf.getBytes(buf.readerIndex(), bytes);
19+
return new String(bytes, StandardCharsets.ISO_8859_1);
20+
} else {
21+
byte[] bytes = new byte[MAX_CONTENT_BYTES];
22+
int total = 0;
23+
int n;
24+
try (FileInputStream fis = new FileInputStream(fileUpload.getFile())) {
25+
while (total < MAX_CONTENT_BYTES
26+
&& (n = fis.read(bytes, total, MAX_CONTENT_BYTES - total)) != -1) {
27+
total += n;
28+
}
29+
}
30+
return new String(bytes, 0, total, StandardCharsets.ISO_8859_1);
31+
}
32+
} catch (Exception ignored) {
33+
return "";
34+
}
35+
}
36+
37+
private NettyFileUploadContentReader() {}
38+
}

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

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
import datadog.trace.instrumentation.netty41.NettyFileUploadContentReader
2+
import io.netty.buffer.Unpooled
3+
import io.netty.handler.codec.http.multipart.FileUpload
4+
import spock.lang.Specification
5+
import spock.lang.TempDir
6+
7+
import java.nio.charset.StandardCharsets
8+
import java.nio.file.Path
9+
10+
class NettyFileUploadContentReaderTest extends Specification {
11+
12+
@TempDir
13+
Path tempDir
14+
15+
// --- in-memory path ---
16+
17+
void 'readContent returns content from in-memory FileUpload'() {
18+
given:
19+
def upload = inMemoryUpload('hello world')
20+
21+
expect:
22+
NettyFileUploadContentReader.readContent(upload) == 'hello world'
23+
}
24+
25+
void 'readContent truncates in-memory content at MAX_CONTENT_BYTES'() {
26+
given:
27+
def upload = inMemoryUpload('X' * inputSize)
28+
29+
when:
30+
def result = NettyFileUploadContentReader.readContent(upload)
31+
32+
then:
33+
result.length() == expectedSize
34+
35+
where:
36+
inputSize | expectedSize
37+
13 | 13
38+
NettyFileUploadContentReader.MAX_CONTENT_BYTES - 1 | NettyFileUploadContentReader.MAX_CONTENT_BYTES - 1
39+
NettyFileUploadContentReader.MAX_CONTENT_BYTES | NettyFileUploadContentReader.MAX_CONTENT_BYTES
40+
NettyFileUploadContentReader.MAX_CONTENT_BYTES + 500 | NettyFileUploadContentReader.MAX_CONTENT_BYTES
41+
}
42+
43+
void 'readContent returns empty string for empty in-memory content'() {
44+
given:
45+
def upload = inMemoryUpload('')
46+
47+
expect:
48+
NettyFileUploadContentReader.readContent(upload) == ''
49+
}
50+
51+
void 'readContent does not advance readerIndex of the underlying ByteBuf'() {
52+
given:
53+
def content = 'sensitive data'
54+
def upload = inMemoryUpload(content)
55+
def buf = upload.getByteBuf()
56+
def indexBefore = buf.readerIndex()
57+
58+
when:
59+
NettyFileUploadContentReader.readContent(upload)
60+
61+
then:
62+
buf.readerIndex() == indexBefore
63+
}
64+
65+
// --- disk-backed path ---
66+
67+
void 'readContent returns content from disk-backed FileUpload'() {
68+
given:
69+
def upload = diskBackedUpload('hello from disk')
70+
71+
expect:
72+
NettyFileUploadContentReader.readContent(upload) == 'hello from disk'
73+
}
74+
75+
void 'readContent truncates disk-backed content at MAX_CONTENT_BYTES'() {
76+
given:
77+
def upload = diskBackedUpload('Y' * inputSize)
78+
79+
when:
80+
def result = NettyFileUploadContentReader.readContent(upload)
81+
82+
then:
83+
result.length() == expectedSize
84+
85+
where:
86+
inputSize | expectedSize
87+
13 | 13
88+
NettyFileUploadContentReader.MAX_CONTENT_BYTES - 1 | NettyFileUploadContentReader.MAX_CONTENT_BYTES - 1
89+
NettyFileUploadContentReader.MAX_CONTENT_BYTES | NettyFileUploadContentReader.MAX_CONTENT_BYTES
90+
NettyFileUploadContentReader.MAX_CONTENT_BYTES + 500 | NettyFileUploadContentReader.MAX_CONTENT_BYTES
91+
}
92+
93+
void 'readContent returns empty string for empty disk-backed file'() {
94+
given:
95+
def upload = diskBackedUpload('')
96+
97+
expect:
98+
NettyFileUploadContentReader.readContent(upload) == ''
99+
}
100+
101+
// --- error handling ---
102+
103+
void 'readContent returns empty string when getByteBuf throws'() {
104+
given:
105+
FileUpload upload = Stub(FileUpload)
106+
upload.isInMemory() >> true
107+
upload.getByteBuf() >> { throw new RuntimeException('simulated error') }
108+
109+
expect:
110+
NettyFileUploadContentReader.readContent(upload) == ''
111+
}
112+
113+
void 'readContent returns empty string when getFile throws'() {
114+
given:
115+
FileUpload upload = Stub(FileUpload)
116+
upload.isInMemory() >> false
117+
upload.getFile() >> { throw new IOException('simulated error') }
118+
119+
expect:
120+
NettyFileUploadContentReader.readContent(upload) == ''
121+
}
122+
123+
// --- helpers ---
124+
125+
private FileUpload inMemoryUpload(String content) {
126+
def buf = Unpooled.copiedBuffer(content, StandardCharsets.ISO_8859_1)
127+
FileUpload upload = Stub(FileUpload)
128+
upload.isInMemory() >> true
129+
upload.getByteBuf() >> buf
130+
return upload
131+
}
132+
133+
private FileUpload diskBackedUpload(String content) {
134+
def file = tempDir.resolve('upload.bin').toFile()
135+
file.bytes = content.getBytes(StandardCharsets.ISO_8859_1)
136+
FileUpload upload = Stub(FileUpload)
137+
upload.isInMemory() >> false
138+
upload.getFile() >> file
139+
return upload
140+
}
141+
}

0 commit comments

Comments
 (0)