Skip to content

Commit ab03211

Browse files
yawkatclaude
yawkat
and
claude
committed
Do not percent-decode url-encoded data in place
UrlEncodedDecoder.decodeComponent rewrote its input buffer with setByte. That input could be caller-visible memory: the key is a retained slice of the buffer passed to add(), an unterminated value is handed out as the added buffer itself, and the vintage HttpPostStandardRequestDecoder retains the offered HttpContent buffer and stores it in the attribute before decoding it. Percent or '+' decoding therefore changed the caller's bytes (and writerIndex), and failed with ReadOnlyBufferException for read-only input. The legacy implementation left offered buffers unchanged. decodeComponent now uses copy-on-write: it scans for '%' or '+' and returns the input unchanged if there is none; otherwise it writes the decoded bytes into a newly allocated buffer. It takes ownership of the input and returns a buffer owned by the caller, so the VintageAccess.UrlEncodedDecoder bridge method now returns the decoded ByteBuf instead of void. The key, decodedContent() and vintage call sites are updated for the new ownership contract. Regression tests check that offered buffers (keys and values, with and without endInput, single and multiple chunks) are unchanged after decoding and that read-only input decodes, for both the core decoder and the vintage HttpPostStandardRequestDecoder. Fixes #38 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 1871b71 commit ab03211

5 files changed

Lines changed: 202 additions & 42 deletions

File tree

‎multipart-core/src/main/java/io/netty/contrib/multipart/UrlEncodedDecoder.java‎

Lines changed: 56 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ final class UrlEncodedDecoder extends AbstractDecoder implements VintageAccess.U
3131
private static final ByteProcessor FIND_KEY_END_OR_EOL =
3232
value -> value != '=' && value != '&' && value != '\r' && value != '\n';
3333
private static final ByteProcessor FIND_VALUE_END = value -> value != '&' && value != '\r' && value != '\n';
34+
private static final ByteProcessor FIND_ESCAPE = value -> value != '%' && value != '+';
3435

3536
private State state = State.KEY;
3637
/**
@@ -93,11 +94,20 @@ public Event next() {
9394
key = decodeAttribute(keyByteBuf.toString(charset), charset);
9495
} else {
9596
// whatwg spec first does percent decoding, then utf-8 decoding
96-
decodeComponent(keyByteBuf, true);
97-
key = keyByteBuf.toString(charset);
97+
ByteBuf undecodedKey = keyByteBuf;
98+
// decodeComponent takes ownership
99+
keyByteBuf = null;
100+
ByteBuf decodedKey = decodeComponent(undecodedKey, true);
101+
try {
102+
key = decodedKey.toString(charset);
103+
} finally {
104+
decodedKey.release();
105+
}
98106
}
99107
} finally {
100-
keyByteBuf.release();
108+
if (keyByteBuf != null) {
109+
keyByteBuf.release();
110+
}
101111
}
102112
keyWithoutValue = !hasValue;
103113
if (!hasValue && !noValueAtEof) {
@@ -296,14 +306,8 @@ public ByteBuf decodedContent() {
296306
throw new IllegalStateException("Not in CONTENT event");
297307
}
298308
ByteBuf b = undecodedContent;
299-
try {
300-
undecodedContent = null;
301-
decodeComponent(b, false);
302-
return b;
303-
} catch (Exception e) {
304-
b.release();
305-
throw e;
306-
}
309+
undecodedContent = null;
310+
return decodeComponent(b, false);
307311
}
308312

309313
@Override
@@ -313,37 +317,55 @@ public ByteBuf undecodedContent() {
313317
return b;
314318
}
315319

320+
/**
321+
* Percent-decode the given buffer. The input buffer is never modified, because it may be shared with the caller
322+
* (e.g. a slice of a buffer passed to {@link #add(ByteBuf)}) or be read-only. If the input contains no escape
323+
* sequences, it is returned as-is. Otherwise, the decoded data is written to a newly allocated buffer.
324+
*
325+
* @param buffer The buffer to decode. Ownership is transferred to this method, also on failure
326+
* @param key Whether this is a key, for the error message
327+
* @return The decoded buffer, owned by the caller
328+
*/
316329
@Override
317-
public void decodeComponent(ByteBuf buffer, boolean key) {
318-
int wi = buffer.readerIndex();
319-
for (int ri = wi; ri < buffer.writerIndex(); wi++, ri++) {
320-
byte b = buffer.getByte(ri);
321-
if (b == '%') {
322-
if (ri < buffer.writerIndex() - 2) {
323-
int hi = StringUtil.decodeHexNibble((char) buffer.getByte(ri + 1));
324-
int lo = StringUtil.decodeHexNibble((char) buffer.getByte(ri + 2));
325-
if (hi != -1 && lo != -1) {
326-
buffer.setByte(wi, (byte) ((hi << 4) + lo));
327-
ri += 2;
328-
continue;
329-
} else if (quirks.contains(DecoderQuirk.REFUSE_NON_HEX_PERCENT_DECODE)) {
330+
public ByteBuf decodeComponent(ByteBuf buffer, boolean key) {
331+
int firstEscape = buffer.forEachByte(FIND_ESCAPE);
332+
if (firstEscape == -1) {
333+
return buffer;
334+
}
335+
ByteBuf decoded = null;
336+
try {
337+
decoded = buffer.alloc().buffer(buffer.readableBytes());
338+
decoded.writeBytes(buffer, buffer.readerIndex(), firstEscape - buffer.readerIndex());
339+
for (int ri = firstEscape; ri < buffer.writerIndex(); ri++) {
340+
byte b = buffer.getByte(ri);
341+
if (b == '%') {
342+
if (ri < buffer.writerIndex() - 2) {
343+
int hi = StringUtil.decodeHexNibble((char) buffer.getByte(ri + 1));
344+
int lo = StringUtil.decodeHexNibble((char) buffer.getByte(ri + 2));
345+
if (hi != -1 && lo != -1) {
346+
decoded.writeByte((hi << 4) + lo);
347+
ri += 2;
348+
continue;
349+
} else if (quirks.contains(DecoderQuirk.REFUSE_NON_HEX_PERCENT_DECODE)) {
350+
// whatwg URL spec allows this
351+
failPercentDecode(key);
352+
}
353+
} else if (quirks.contains(DecoderQuirk.REFUSE_SHORT_PERCENT_DECODE)) {
330354
// whatwg URL spec allows this
331355
failPercentDecode(key);
332356
}
333-
} else if (quirks.contains(DecoderQuirk.REFUSE_SHORT_PERCENT_DECODE)) {
334-
// whatwg URL spec allows this
335-
failPercentDecode(key);
336357
}
358+
decoded.writeByte(b == '+' ? ' ' : b);
337359
}
338-
if (b == '+') {
339-
buffer.setByte(wi, (byte) ' ');
340-
continue;
341-
}
342-
if (ri != wi) {
343-
buffer.setByte(wi, b);
360+
ByteBuf result = decoded;
361+
decoded = null;
362+
return result;
363+
} finally {
364+
buffer.release();
365+
if (decoded != null) {
366+
decoded.release();
344367
}
345368
}
346-
buffer.writerIndex(wi);
347369
}
348370

349371
private void failPercentDecode(boolean key) {

‎multipart-core/src/main/java/io/netty/contrib/multipart/VintageAccess.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,12 @@ public interface MultipartDecoder extends PostBodyDecoder {
6767
public interface UrlEncodedDecoder extends PostBodyDecoder {
6868
ByteBuf undecodedContent();
6969

70-
void decodeComponent(ByteBuf buffer, boolean key);
70+
/**
71+
* Percent-decode the given buffer without modifying it. Takes ownership of the input buffer (also on
72+
* failure) and returns a buffer owned by the caller, which may be the same instance if nothing needed
73+
* decoding.
74+
*/
75+
ByteBuf decodeComponent(ByteBuf buffer, boolean key);
7176

7277
int getCompactionThreshold();
7378

‎multipart-core/src/test/java/io/netty/contrib/multipart/UrlEncodedDecoderTest.java‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.junit.jupiter.api.Test;
2424
import org.junit.jupiter.params.ParameterizedTest;
2525
import org.junit.jupiter.params.provider.MethodSource;
26+
import org.junit.jupiter.params.provider.ValueSource;
2627

2728
import java.io.IOException;
2829
import java.nio.charset.StandardCharsets;
@@ -179,6 +180,21 @@ public void callerBufferNotModified() {
179180
}
180181
}
181182

183+
@ParameterizedTest
184+
@ValueSource(strings = {"foo=b%41r", "f%41o=bar", "f%41o=b%41r", "foo=b+r", "f+o=bar", "f%41o=b+r&x%41=y%41"})
185+
public void percentDecodingDoesNotModifyCallerBuffer(String input) {
186+
ByteBuf first = Unpooled.copiedBuffer(input, StandardCharsets.UTF_8);
187+
try (PostBodyDecoder decoder = PostBodyDecoder.builder().forUrlEncodedData()) {
188+
decoder.add(first.retain());
189+
decoder.endInput();
190+
drainAndDecode(decoder);
191+
Assertions.assertEquals(input.length(), first.writerIndex());
192+
Assertions.assertEquals(input, first.toString(0, input.length(), StandardCharsets.UTF_8));
193+
} finally {
194+
first.release();
195+
}
196+
}
197+
182198
@Test
183199
public void callerCompositeBufferNotModified() {
184200
CompositeByteBuf first = Unpooled.compositeBuffer();
@@ -195,6 +211,54 @@ public void callerCompositeBufferNotModified() {
195211
}
196212
}
197213

214+
@ParameterizedTest
215+
@ValueSource(strings = {"foo=b%41r", "f%41o=bar", "foo=b+r", "f+o=bar", "f%41o=b+r&x%41=y%41"})
216+
public void percentDecodingDoesNotModifyCallerBufferWithoutEof(String input) {
217+
// without endInput, an unterminated value is handed out as the whole remaining input buffer
218+
ByteBuf first = Unpooled.copiedBuffer(input, StandardCharsets.UTF_8);
219+
try (PostBodyDecoder decoder = PostBodyDecoder.builder().forUrlEncodedData()) {
220+
decoder.add(first.retain());
221+
drainAndDecode(decoder);
222+
Assertions.assertEquals(input.length(), first.writerIndex());
223+
Assertions.assertEquals(input, first.toString(0, input.length(), StandardCharsets.UTF_8));
224+
} finally {
225+
first.release();
226+
}
227+
}
228+
229+
@ParameterizedTest
230+
@ValueSource(strings = {"foo=b%41r", "f%41o=bar", "foo=b+r", "f+o=bar", "f%41o=b+r&x%41=y%41"})
231+
public void percentDecodingReadOnlyBuffer(String input) {
232+
try (PostBodyDecoder decoder = PostBodyDecoder.builder().forUrlEncodedData()) {
233+
decoder.add(Unpooled.copiedBuffer(input, StandardCharsets.UTF_8).asReadOnly());
234+
decoder.endInput();
235+
drainAndDecode(decoder);
236+
}
237+
}
238+
239+
@Test
240+
public void percentDecodingReadOnlyBufferValues() {
241+
try (PostBodyDecoder decoder = PostBodyDecoder.builder().forUrlEncodedData()) {
242+
decoder.add(Unpooled.copiedBuffer("f%41o=b%41r&x+y=1+2", StandardCharsets.UTF_8).asReadOnly());
243+
decoder.endInput();
244+
245+
expectField(decoder, "fAo", "bAr");
246+
expectField(decoder, "x y", "1 2");
247+
Assertions.assertNull(decoder.next());
248+
}
249+
}
250+
251+
private static void drainAndDecode(PostBodyDecoder decoder) {
252+
PostBodyDecoder.Event event;
253+
while ((event = decoder.next()) != null) {
254+
if (event == PostBodyDecoder.Event.HEADER) {
255+
Assertions.assertNotNull(decoder.parsedHeaderValue());
256+
} else if (event == PostBodyDecoder.Event.CONTENT) {
257+
decoder.decodedContent().release();
258+
}
259+
}
260+
}
261+
198262
@Test
199263
public void bufferCompaction() throws IOException {
200264
byte[] fullData = new byte[10 * 1024 * 1024];

‎multipart-vintage/src/main/java/io/netty/contrib/multipart/vintage/HttpPostStandardRequestDecoder.java‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -437,13 +437,9 @@ private void parseBodyAttributesStandard() {
437437
}
438438
currentAttribute.addContent(Unpooled.EMPTY_BUFFER, true);
439439
// in netty 4, decoding happens late
440-
ByteBuf bb = currentAttribute.getByteBuf().retain();
441-
try {
442-
decoder.decodeComponent(bb, false);
443-
} catch (Exception e) {
444-
bb.release();
445-
throw e;
446-
}
440+
// decodeComponent does not modify the attribute content, which may be shared with the
441+
// caller's HttpContent buffer. It takes ownership of this retained reference.
442+
ByteBuf bb = decoder.decodeComponent(currentAttribute.getByteBuf().retain(), false);
447443
currentAttribute.setContent(bb);
448444

449445
addHttpData(currentAttribute);

‎multipart-vintage/src/test/java/io/netty/contrib/multipart/vintage/HttpPostStandardRequestDecoderTest.java‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,79 @@ void testKeyWithoutValueCompletedState(String requestBody, String keysWithoutVal
345345
}
346346
}
347347

348+
@Test
349+
void testPercentDecodingDoesNotModifyOfferedBuffers() {
350+
String firstBody = "k%41=v%41&x";
351+
String secondBody = "=y%42+z";
352+
HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/upload");
353+
HttpPostStandardRequestDecoder decoder = HttpPostRequestDecoder.builder()
354+
.dataFactory(httpDiskDataFactory())
355+
.buildStandard(request);
356+
ByteBuf first = Unpooled.copiedBuffer(firstBody, CharsetUtil.UTF_8);
357+
ByteBuf second = Unpooled.copiedBuffer(secondBody, CharsetUtil.UTF_8);
358+
try {
359+
decoder.offer(new DefaultHttpContent(first));
360+
decoder.offer(new DefaultLastHttpContent(second));
361+
362+
assertEquals(2, decoder.getBodyHttpDatas().size());
363+
assertMemoryAttribute(decoder.getBodyHttpData("kA"), "vA");
364+
assertMemoryAttribute(decoder.getBodyHttpData("x"), "yB z");
365+
366+
// neither the indices nor the content of the caller buffers may be changed by percent decoding
367+
assertEquals(0, first.readerIndex());
368+
assertEquals(firstBody.length(), first.writerIndex());
369+
assertEquals(firstBody, first.toString(CharsetUtil.UTF_8));
370+
assertEquals(0, second.readerIndex());
371+
assertEquals(secondBody.length(), second.writerIndex());
372+
assertEquals(secondBody, second.toString(CharsetUtil.UTF_8));
373+
} finally {
374+
decoder.destroy();
375+
first.release();
376+
second.release();
377+
}
378+
}
379+
380+
@Test
381+
void testPercentDecodingSingleChunkDoesNotModifyOfferedBuffer() {
382+
String body = "k%41+x=v%41+";
383+
HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/upload");
384+
HttpPostStandardRequestDecoder decoder = HttpPostRequestDecoder.builder()
385+
.dataFactory(httpDiskDataFactory())
386+
.buildStandard(request);
387+
ByteBuf buf = Unpooled.copiedBuffer(body, CharsetUtil.UTF_8);
388+
try {
389+
decoder.offer(new DefaultLastHttpContent(buf));
390+
391+
assertEquals(1, decoder.getBodyHttpDatas().size());
392+
assertMemoryAttribute(decoder.getBodyHttpData("kA x"), "vA ");
393+
assertEquals(0, buf.readerIndex());
394+
assertEquals(body.length(), buf.writerIndex());
395+
assertEquals(body, buf.toString(CharsetUtil.UTF_8));
396+
} finally {
397+
decoder.destroy();
398+
buf.release();
399+
}
400+
}
401+
402+
@Test
403+
void testPercentDecodingReadOnlyBuffer() {
404+
HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/upload");
405+
HttpPostStandardRequestDecoder decoder = HttpPostRequestDecoder.builder()
406+
.dataFactory(httpDiskDataFactory())
407+
.buildStandard(request);
408+
ByteBuf buf = Unpooled.copiedBuffer("k%41=v%41&a+b=c+d", CharsetUtil.UTF_8).asReadOnly();
409+
try {
410+
decoder.offer(new DefaultLastHttpContent(buf));
411+
412+
assertEquals(2, decoder.getBodyHttpDatas().size());
413+
assertMemoryAttribute(decoder.getBodyHttpData("kA"), "vA");
414+
assertMemoryAttribute(decoder.getBodyHttpData("a b"), "c d");
415+
} finally {
416+
decoder.destroy();
417+
buf.release();
418+
}
419+
}
420+
348421
private static DefaultHttpDataFactory httpDiskDataFactory() {
349422
return new DefaultHttpDataFactory(false);
350423
}

0 commit comments

Comments
 (0)