Repository: netty/netty
Issue: SpdyHttpEncoder fails to convert HttpResponse to SpdyFrame
Labels: defect

Netty Version: Latest master (f1eddd6117f5e3109257f911d51b80250e17e11d)

When `SpdyHttpEncoder` attempts to create an `SpdyHeadersFrame` from a `HttpResponse` an `IllegalArgumentException` is thrown if the original `HttpResponse` contains a header that includes uppercase characters.  The `IllegalArgumentException` is thrown due to the additional validation check introduced by #4047.

Previous versions of the SPDY codec would handle this by converting the HTTP header name to lowercase before adding the header to the `SpdyHeadersFrame` (see [here](https://github.com/netty/netty/pull/176/files#diff-4fb47d56c527df13fe16a32a66d60d02R305)).

Not the greatest solution, but this could be 'fixed' by lowercasing the header name [here](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java#L175) and [here](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java#L275)

If you guys are comfortable with this solution (or have a better one), I would be more than happy to create a PR.

/cc @Scottmitch @jpinner - for obvious reasons :smiley: 


Issue comments:
@blucas - Sorry for breaking you :(. I guess we have at least 2 options:
1. Add a constructor to `DefaultSpdyHeaders` which takes a `boolean validateHeaders` argument which can make header name validation optional.  We would then need to carry this through the SPDY codec to pass along the `boolean` where ever the `DefaultSpdyHeaders` are created.
2. Your suggestion, plus a `boolean convertHeaderNamesToLowerCase` (or something similar) to control whether conversion should be done or not.

Feel free to submit a PR for one/both of the above :)

@Scottmitch - no problem, it happens to all of us :).

I have made the necessary changes for 1. above (although I've followed the way it was done for HTTP such that if `validateHeaders` is set to `true` (default), header names **and** values will be validated)

Do we really need to add a `boolean convertHeaderNamesToLowerCase` to `SpdyHttpEncoder`? We don't do this for HTTP to HTTP/2 conversion (see [here](https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java#L318)).  I think we should just do the same. What do you think?

> Do we really need to add a boolean convertHeaderNamesToLowerCase to SpdyHttpEncoder? 

It is not strictly necessary. Just a potential fast path or optimization if your environment is controlled.  Feel free to leave it out...we can always add it later.

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

Just a heads up: SpdyHttpHeaders.Names are still uppercase, is that correct?

@CodingFabian - No that is a bug. Fixed with https://github.com/netty/netty/pull/4226


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java
index 8a62c1ac29d..e59afb8a95d 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java
@@ -27,6 +27,7 @@
 import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.HttpResponse;
 import io.netty.handler.codec.http.LastHttpContent;
+import io.netty.util.AsciiString;
 
 import java.util.List;
 import java.util.Map;
@@ -172,7 +173,7 @@ protected void encode(ChannelHandlerContext ctx, HttpObject msg, List<Object> ou
                     SpdyHeadersFrame spdyHeadersFrame = new DefaultSpdyHeadersFrame(currentStreamId);
                     spdyHeadersFrame.setLast(true);
                     for (Map.Entry<CharSequence, CharSequence> entry: trailers) {
-                        spdyHeadersFrame.headers().add(entry.getKey(), entry.getValue());
+                        spdyHeadersFrame.headers().add(AsciiString.of(entry.getKey()).toLowerCase(), entry.getValue());
                     }
 
                     // Write DATA frame and append HEADERS frame
@@ -233,7 +234,7 @@ private SpdySynStreamFrame createSynStreamFrame(HttpRequest httpRequest) throws
 
         // Transfer the remaining HTTP headers
         for (Map.Entry<CharSequence, CharSequence> entry: httpHeaders) {
-            frameHeaders.add(entry.getKey(), entry.getValue());
+            frameHeaders.add(AsciiString.of(entry.getKey()).toLowerCase(), entry.getValue());
         }
         currentStreamId = spdySynStreamFrame.streamId();
         if (associatedToStreamId == 0) {
@@ -272,7 +273,7 @@ private SpdyHeadersFrame createHeadersFrame(HttpResponse httpResponse) throws Ex
 
         // Transfer the remaining HTTP headers
         for (Map.Entry<CharSequence, CharSequence> entry: httpHeaders) {
-            spdyHeadersFrame.headers().add(entry.getKey(), entry.getValue());
+            spdyHeadersFrame.headers().add(AsciiString.of(entry.getKey()).toLowerCase(), entry.getValue());
         }
 
         currentStreamId = streamId;


Test patch: