Repository: netty/netty
Issue: HttpClientUpgradeHandler.setUpgradeHeaders() append a wrong 'upgrade'.
Labels: defect

See [here](https://github.com/netty/netty/blob/92dee9e500bc141162e6c3a6ef91d8d22e50c449/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java#L231).

It has to be `HttpHeaderValues.UPGRADE` instead. Most servers should be OK with `HttpHeaderNames.UPGRADE` because they do case-insensitive comparisons, but some (e.g. Jetty) requires it to be `Upgrade` (no lowercase u).


Issue comments:
Seems like we should use `HttpHeaderValues` here. The specification calls out the lowercase variant [1]. Should we change `HttpHeaderValues.UPGRADE` to lowercase, and should/are we using case insensitive compare when evaluating this header value?

[1] https://tools.ietf.org/html/rfc7230#section-6.7

> When Upgrade is sent, the sender MUST also send a Connection header
>    field (Section 6.1) that contains an "upgrade" connection option

We are using case-insensitive comparison, which is fine in my opinion. (more tolerating)

Jetty uses case-sensitive comparison and expects `Upgrade`, which seems incorrect. (They just do `.contains()` on a `List<String>` IIRC.)

So, we could:
- Use `HttpHeaderValues.UPGRADE` instead,
- Change it from `Upgrade` to `upgrade`, and
- Ping the Jetty folks about this so they fix this or they have a chance to disagree.

Here's the code that uses `.contains("Upgrade")`: 

https://github.com/eclipse/jetty.project/blob/65d33f8b49bd51140d9fe9b024c5dd42ad7571d8/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java#L373

`git blame` tells @gregw could help us here.

@trustin Ooops that looks wrong.  Fixing now..... standby.....

@trustin Thanks for confirming we use case-insensitive compare. I agree we should change `HttpHeaderValues.UPGRADE` to lowercase, and use it here.

@gregw - Thanks for the fix!

@trustin @Scottmitch   Fix is committed to jetty 9.3 and master branches: https://bugs.eclipse.org/bugs/show_bug.cgi?id=483039

@gregw thanks!

@Scottmitch should I take care ?

@normanmaurer - Sure. Thanks!

Ok working on a fix


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java
index b7b3fda8e93..f8c452de24a 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java
@@ -266,7 +266,7 @@ private void setUpgradeRequestHeaders(ChannelHandlerContext ctx, HttpRequest req
             builder.append(part);
             builder.append(',');
         }
-        builder.append(HttpHeaderNames.UPGRADE);
+        builder.append(HttpHeaderValues.UPGRADE);
         request.headers().set(HttpHeaderNames.CONNECTION, builder.toString());
     }
 }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaderValues.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaderValues.java
index 9c75473acc5..b06fad57169 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaderValues.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaderValues.java
@@ -193,9 +193,9 @@ public final class HttpHeaderValues {
      */
     public static final AsciiString TRAILERS = new AsciiString("trailers");
     /**
-     * {@code "Upgrade"}
+     * {@code "upgrade"}
      */
-    public static final AsciiString UPGRADE = new AsciiString("Upgrade");
+    public static final AsciiString UPGRADE = new AsciiString("upgrade");
     /**
      * {@code "websocket"}
      */


Test patch: