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>

Author: yawkat

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

@@ -29,6 +29,7 @@
 final class UrlEncodedDecoder extends AbstractDecoder implements VintageAccess.UrlEncodedDecoder {
     private static final ByteProcessor FIND_KEY_END = value -> value != '=' && value != '&';
     private static final ByteProcessor FIND_VALUE_END = value -> value != '&' && value != '\r' && value != '\n';
+    private static final ByteProcessor FIND_ESCAPE = value -> value != '%' && value != '+';
 
     private State state = State.KEY;
 
@@ -70,11 +71,20 @@ public Event next() {
                                 key = decodeAttribute(keyByteBuf.toString(charset), charset);
                             } else {
                                 // whatwg spec first does percent decoding, then utf-8 decoding
-                                decodeComponent(keyByteBuf, true);
-                                key = keyByteBuf.toString(charset);
+                                ByteBuf undecodedKey = keyByteBuf;
+                                // decodeComponent takes ownership
+                                keyByteBuf = null;
+                                ByteBuf decodedKey = decodeComponent(undecodedKey, true);
+                                try {
+                                    key = decodedKey.toString(charset);
+                                } finally {
+                                    decodedKey.release();
+                                }
                             }
                         } finally {
-                            keyByteBuf.release();
+                            if (keyByteBuf != null) {
+                                keyByteBuf.release();
+                            }
                         }
                         if (!hasValue && !noValueAtEof) {
                             // go to just before the '&', it will read as an empty value
@@ -247,14 +257,8 @@ public ByteBuf decodedContent() {
             throw new IllegalStateException("Not in CONTENT event");
         }
         ByteBuf b = undecodedContent;
-        try {
-            undecodedContent = null;
-            decodeComponent(b, false);
-            return b;
-        } catch (Exception e) {
-            b.release();
-            throw e;
-        }
+        undecodedContent = null;
+        return decodeComponent(b, false);
     }
 
     @Override
@@ -264,37 +268,55 @@ public ByteBuf undecodedContent() {
         return b;
     }
 
+    /**
+     * Percent-decode the given buffer. The input buffer is never modified, because it may be shared with the caller
+     * (e.g. a slice of a buffer passed to {@link #add(ByteBuf)}) or be read-only. If the input contains no escape
+     * sequences, it is returned as-is. Otherwise, the decoded data is written to a newly allocated buffer.
+     *
+     * @param buffer The buffer to decode. Ownership is transferred to this method, also on failure
+     * @param key Whether this is a key, for the error message
+     * @return The decoded buffer, owned by the caller
+     */
     @Override
-    public void decodeComponent(ByteBuf buffer, boolean key) {
-        int wi = buffer.readerIndex();
-        for (int ri = wi; ri < buffer.writerIndex(); wi++, ri++) {
-            byte b = buffer.getByte(ri);
-            if (b == '%') {
-                if (ri < buffer.writerIndex() - 2) {
-                    int hi = StringUtil.decodeHexNibble((char) buffer.getByte(ri + 1));
-                    int lo = StringUtil.decodeHexNibble((char) buffer.getByte(ri + 2));
-                    if (hi != -1 && lo != -1) {
-                        buffer.setByte(wi, (byte) ((hi << 4) + lo));
-                        ri += 2;
-                        continue;
-                    } else if (quirks.contains(DecoderQuirk.REFUSE_NON_HEX_PERCENT_DECODE)) {
+    public ByteBuf decodeComponent(ByteBuf buffer, boolean key) {
+        int firstEscape = buffer.forEachByte(FIND_ESCAPE);
+        if (firstEscape == -1) {
+            return buffer;
+        }
+        ByteBuf decoded = null;
+        try {
+            decoded = buffer.alloc().buffer(buffer.readableBytes());
+            decoded.writeBytes(buffer, buffer.readerIndex(), firstEscape - buffer.readerIndex());
+            for (int ri = firstEscape; ri < buffer.writerIndex(); ri++) {
+                byte b = buffer.getByte(ri);
+                if (b == '%') {
+                    if (ri < buffer.writerIndex() - 2) {
+                        int hi = StringUtil.decodeHexNibble((char) buffer.getByte(ri + 1));
+                        int lo = StringUtil.decodeHexNibble((char) buffer.getByte(ri + 2));
+                        if (hi != -1 && lo != -1) {
+                            decoded.writeByte((hi << 4) + lo);
+                            ri += 2;
+                            continue;
+                        } else if (quirks.contains(DecoderQuirk.REFUSE_NON_HEX_PERCENT_DECODE)) {
+                            // whatwg URL spec allows this
+                            failPercentDecode(key);
+                        }
+                    } else if (quirks.contains(DecoderQuirk.REFUSE_SHORT_PERCENT_DECODE)) {
                         // whatwg URL spec allows this
                         failPercentDecode(key);
                     }
-                } else if (quirks.contains(DecoderQuirk.REFUSE_SHORT_PERCENT_DECODE)) {
-                    // whatwg URL spec allows this
-                    failPercentDecode(key);
                 }
+                decoded.writeByte(b == '+' ? ' ' : b);
             }
-            if (b == '+') {
-                buffer.setByte(wi, (byte) ' ');
-                continue;
-            }
-            if (ri != wi) {
-                buffer.setByte(wi, b);
+            ByteBuf result = decoded;
+            decoded = null;
+            return result;
+        } finally {
+            buffer.release();
+            if (decoded != null) {
+                decoded.release();
             }
         }
-        buffer.writerIndex(wi);
     }
 
     private void failPercentDecode(boolean key) {

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

@@ -63,7 +63,12 @@ public interface MultipartDecoder extends PostBodyDecoder {
     public interface UrlEncodedDecoder extends PostBodyDecoder {
         ByteBuf undecodedContent();
 
-        void decodeComponent(ByteBuf buffer, boolean key);
+        /**
+         * Percent-decode the given buffer without modifying it. Takes ownership of the input buffer (also on
+         * failure) and returns a buffer owned by the caller, which may be the same instance if nothing needed
+         * decoding.
+         */
+        ByteBuf decodeComponent(ByteBuf buffer, boolean key);
 
         int getCompactionThreshold();
 

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

@@ -15,10 +15,13 @@
  */
 package io.netty.contrib.multipart;
 
+import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import io.netty.handler.codec.http.HttpHeaderNames;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
@@ -119,6 +122,69 @@ public void spacesValue() {
         }
     }
 
+    @ParameterizedTest
+    @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"})
+    public void percentDecodingDoesNotModifyCallerBuffer(String input) {
+        ByteBuf first = Unpooled.copiedBuffer(input, StandardCharsets.UTF_8);
+        try (PostBodyDecoder decoder = PostBodyDecoder.builder().forUrlEncodedData()) {
+            decoder.add(first.retain());
+            decoder.endInput();
+            drainAndDecode(decoder);
+            Assertions.assertEquals(input.length(), first.writerIndex());
+            Assertions.assertEquals(input, first.toString(0, input.length(), StandardCharsets.UTF_8));
+        } finally {
+            first.release();
+        }
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"foo=b%41r", "f%41o=bar", "foo=b+r", "f+o=bar", "f%41o=b+r&x%41=y%41"})
+    public void percentDecodingDoesNotModifyCallerBufferWithoutEof(String input) {
+        // without endInput, an unterminated value is handed out as the whole remaining input buffer
+        ByteBuf first = Unpooled.copiedBuffer(input, StandardCharsets.UTF_8);
+        try (PostBodyDecoder decoder = PostBodyDecoder.builder().forUrlEncodedData()) {
+            decoder.add(first.retain());
+            drainAndDecode(decoder);
+            Assertions.assertEquals(input.length(), first.writerIndex());
+            Assertions.assertEquals(input, first.toString(0, input.length(), StandardCharsets.UTF_8));
+        } finally {
+            first.release();
+        }
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"foo=b%41r", "f%41o=bar", "foo=b+r", "f+o=bar", "f%41o=b+r&x%41=y%41"})
+    public void percentDecodingReadOnlyBuffer(String input) {
+        try (PostBodyDecoder decoder = PostBodyDecoder.builder().forUrlEncodedData()) {
+            decoder.add(Unpooled.copiedBuffer(input, StandardCharsets.UTF_8).asReadOnly());
+            decoder.endInput();
+            drainAndDecode(decoder);
+        }
+    }
+
+    @Test
+    public void percentDecodingReadOnlyBufferValues() {
+        try (PostBodyDecoder decoder = PostBodyDecoder.builder().forUrlEncodedData()) {
+            decoder.add(Unpooled.copiedBuffer("f%41o=b%41r&x+y=1+2", StandardCharsets.UTF_8).asReadOnly());
+            decoder.endInput();
+
+            expectField(decoder, "fAo", "bAr");
+            expectField(decoder, "x y", "1 2");
+            Assertions.assertNull(decoder.next());
+        }
+    }
+
+    private static void drainAndDecode(PostBodyDecoder decoder) {
+        PostBodyDecoder.Event event;
+        while ((event = decoder.next()) != null) {
+            if (event == PostBodyDecoder.Event.HEADER) {
+                Assertions.assertNotNull(decoder.parsedHeaderValue());
+            } else if (event == PostBodyDecoder.Event.CONTENT) {
+                decoder.decodedContent().release();
+            }
+        }
+    }
+
     @Test
     public void bufferCompaction() throws IOException {
         byte[] fullData = new byte[10 * 1024 * 1024];

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

@@ -428,13 +428,9 @@ private void parseBodyAttributesStandard() {
                 } else if (event == PostBodyDecoder.Event.FIELD_COMPLETE) {
                     currentAttribute.addContent(Unpooled.EMPTY_BUFFER, true);
                     // in netty 4, decoding happens late
-                    ByteBuf bb = currentAttribute.getByteBuf().retain();
-                    try {
-                        decoder.decodeComponent(bb, false);
-                    } catch (Exception e) {
-                        bb.release();
-                        throw e;
-                    }
+                    // decodeComponent does not modify the attribute content, which may be shared with the
+                    // caller's HttpContent buffer. It takes ownership of this retained reference.
+                    ByteBuf bb = decoder.decodeComponent(currentAttribute.getByteBuf().retain(), false);
                     currentAttribute.setContent(bb);
 
                     addHttpData(currentAttribute);

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

@@ -233,6 +233,74 @@ void testDecodeZeroAttributesWithAmpersandPrefix() {
         decoder.destroy();
     }
 
+    @Test
+    void testPercentDecodingDoesNotModifyOfferedBuffers() throws Exception {
+        String firstBody = "k%41=v%41&x";
+        String secondBody = "=y%42+z";
+        HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/upload");
+        HttpPostStandardRequestDecoder decoder = HttpPostRequestDecoder.builder()
+                .dataFactory(httpDiskDataFactory())
+                .buildStandard(request);
+        ByteBuf first = Unpooled.copiedBuffer(firstBody, CharsetUtil.UTF_8);
+        ByteBuf second = Unpooled.copiedBuffer(secondBody, CharsetUtil.UTF_8);
+        try {
+            decoder.offer(new DefaultHttpContent(first));
+            decoder.offer(new DefaultLastHttpContent(second));
+
+            assertEquals(2, decoder.getBodyHttpDatas().size());
+            assertMemoryAttribute(decoder.getBodyHttpData("kA"), "vA");
+            assertMemoryAttribute(decoder.getBodyHttpData("x"), "yB z");
+
+            // absolute indices: the caller buffer content must not be changed by percent decoding
+            assertEquals(firstBody, first.toString(0, firstBody.length(), CharsetUtil.UTF_8));
+            assertEquals(secondBody, second.toString(0, secondBody.length(), CharsetUtil.UTF_8));
+        } finally {
+            decoder.destroy();
+            first.release();
+            second.release();
+        }
+    }
+
+    @Test
+    void testPercentDecodingSingleChunkDoesNotModifyOfferedBuffer() {
+        String body = "k%41+x=v%41+";
+        HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/upload");
+        HttpPostStandardRequestDecoder decoder = HttpPostRequestDecoder.builder()
+                .dataFactory(httpDiskDataFactory())
+                .buildStandard(request);
+        ByteBuf buf = Unpooled.copiedBuffer(body, CharsetUtil.UTF_8);
+        try {
+            decoder.offer(new DefaultLastHttpContent(buf));
+
+            assertEquals(1, decoder.getBodyHttpDatas().size());
+            assertMemoryAttribute(decoder.getBodyHttpData("kA x"), "vA ");
+            assertEquals(body.length(), buf.writerIndex());
+            assertEquals(body, buf.toString(0, body.length(), CharsetUtil.UTF_8));
+        } finally {
+            decoder.destroy();
+            buf.release();
+        }
+    }
+
+    @Test
+    void testPercentDecodingReadOnlyBuffer() {
+        HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/upload");
+        HttpPostStandardRequestDecoder decoder = HttpPostRequestDecoder.builder()
+                .dataFactory(httpDiskDataFactory())
+                .buildStandard(request);
+        ByteBuf buf = Unpooled.copiedBuffer("k%41=v%41&a+b=c+d", CharsetUtil.UTF_8).asReadOnly();
+        try {
+            decoder.offer(new DefaultLastHttpContent(buf));
+
+            assertEquals(2, decoder.getBodyHttpDatas().size());
+            assertMemoryAttribute(decoder.getBodyHttpData("kA"), "vA");
+            assertMemoryAttribute(decoder.getBodyHttpData("a b"), "c d");
+        } finally {
+            decoder.destroy();
+            buf.release();
+        }
+    }
+
     private static DefaultHttpDataFactory httpDiskDataFactory() {
         return new DefaultHttpDataFactory(false);
     }