Repository: netty/netty
Issue: HttpObjectAggregator.replace headers may enforce validation when they should not

`HttpObjectAggregator.replace` creates a new `DefaultFullHttpRequest` and does not specify a value for the `validateHeaders` during construction. This would mean that header validation is not consistent from the user's perspective and may enforce validation when the user does not want validation ... which may throw unexpected/undesired errors.

This issue also exists in other implementations of `replace` in codec-http.


Issue comments:
@Scottmitch I will take care.

Fixed by https://github.com/netty/netty/pull/7503

Pull request review comments:
Note this change was needed as we not correctly copied the headers before in our replace(...) methods.
Also we missed this before :(
Also we missed this before :(
`set` will also do `clear` which will do some unnecessary work (array filling, pointer updating, assignments). We should be able to just use `addImpl` instead of `set`.
nit: consider simplifying this to a tertiary statement:

```java
return (headers instanceof DefaultHttpHeaders) ? ((DefaultHttpHeaders) headers).copy()
                : new DefaultHttpHeaders().set(headers);
```
this class doesn't implement `clone` so should we get rid of the `implements Cloneable`?
`i nthe` -> `in the`
`We expect to not have` -> `We do not expect to have`
@Scottmitch doh.. good catch, thats a left-over as I first wanted to just use `clone()` for it.
can we just make `copy()` a method of the `HttpHeaders` class because it is an abstract class? Then we don't have to use the static method outside and can just call `.copy()` on the object.

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java
index ba55d9657e9..117e6dbf648 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java
@@ -142,7 +142,10 @@ public FullHttpRequest retainedDuplicate() {
 
     @Override
     public FullHttpRequest replace(ByteBuf content) {
-        return new DefaultFullHttpRequest(protocolVersion(), method(), uri(), content, headers(), trailingHeaders());
+        FullHttpRequest request = new DefaultFullHttpRequest(protocolVersion(), method(), uri(), content,
+                headers().copy(), trailingHeaders().copy());
+        request.setDecoderResult(decoderResult());
+        return request;
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java
index 134865cabb5..012b996a240 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java
@@ -149,7 +149,10 @@ public FullHttpResponse retainedDuplicate() {
 
     @Override
     public FullHttpResponse replace(ByteBuf content) {
-        return new DefaultFullHttpResponse(protocolVersion(), status(), content, headers(), trailingHeaders());
+        FullHttpResponse response = new DefaultFullHttpResponse(protocolVersion(), status(), content,
+                headers().copy(), trailingHeaders().copy());
+        response.setDecoderResult(decoderResult());
+        return response;
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java
index e8fcf3e7adb..88af27f738e 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java
@@ -348,6 +348,11 @@ public int hashCode() {
         return headers.hashCode(CASE_SENSITIVE_HASHER);
     }
 
+    @Override
+    public HttpHeaders copy() {
+        return new DefaultHttpHeaders(headers.copy());
+    }
+
     private static void validateHeaderNameElement(byte value) {
         switch (value) {
         case 0x00:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java
index 7df00eac2b7..35cfc5c5f18 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java
@@ -1693,4 +1693,11 @@ public boolean contains(CharSequence name, CharSequence value, boolean ignoreCas
     public String toString() {
         return HeadersUtils.toString(getClass(), iteratorCharSequence(), size());
     }
+
+    /**
+     * Returns a deap copy of the passed in {@link HttpHeaders}.
+     */
+    public HttpHeaders copy() {
+        return new DefaultHttpHeaders().set(this);
+    }
 }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
index ace7e4642b4..257581fe232 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
@@ -424,9 +424,8 @@ public FullHttpRequest retainedDuplicate() {
 
         @Override
         public FullHttpRequest replace(ByteBuf content) {
-            DefaultFullHttpRequest dup = new DefaultFullHttpRequest(protocolVersion(), method(), uri(), content);
-            dup.headers().set(headers());
-            dup.trailingHeaders().set(trailingHeaders());
+            DefaultFullHttpRequest dup = new DefaultFullHttpRequest(protocolVersion(), method(), uri(), content,
+                    headers().copy(), trailingHeaders().copy());
             dup.setDecoderResult(decoderResult());
             return dup;
         }
@@ -523,9 +522,8 @@ public FullHttpResponse retainedDuplicate() {
 
         @Override
         public FullHttpResponse replace(ByteBuf content) {
-            DefaultFullHttpResponse dup = new DefaultFullHttpResponse(getProtocolVersion(), getStatus(), content);
-            dup.headers().set(headers());
-            dup.trailingHeaders().set(trailingHeaders());
+            DefaultFullHttpResponse dup = new DefaultFullHttpResponse(getProtocolVersion(), getStatus(), content,
+                    headers().copy(), trailingHeaders().copy());
             dup.setDecoderResult(decoderResult());
             return dup;
         }
diff --git a/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java b/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java
index 5664706b987..01b64e85623 100644
--- a/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java
+++ b/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java
@@ -952,6 +952,16 @@ private T thisT() {
         return (T) this;
     }
 
+    /**
+     * Returns a deep copy of this instance.
+     */
+    public DefaultHeaders<K, V, T> copy() {
+        DefaultHeaders<K, V, T> copy = new DefaultHeaders<K, V, T>(
+                hashingStrategy, valueConverter, nameValidator, entries.length);
+        copy.addImpl(this);
+        return copy;
+    }
+
     private final class HeaderIterator implements Iterator<Map.Entry<K, V>> {
         private HeaderEntry<K, V> current = head;
 


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java
index b29c0966d7d..8fb4ebfc678 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java
@@ -62,6 +62,7 @@
 import static io.netty.handler.codec.http2.Http2TestUtil.runInChannel;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -607,6 +608,9 @@ public void run() throws Http2Exception {
             verify(serverListener, times(2)).messageReceived(requestCaptor.capture());
             capturedRequests = requestCaptor.getAllValues();
             assertEquals(2, capturedRequests.size());
+            // We do not expect to have this header in the captured request so remove it now.
+            assertNotNull(request.headers().remove("x-http2-stream-weight"));
+
             assertEquals(request, capturedRequests.get(0));
             assertEquals(request2, capturedRequests.get(1));