From f1f1c61333f908c327e31f77c6c478a1902f0b2b Mon Sep 17 00:00:00 2001
From: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Date: Fri, 24 Jan 2020 01:38:45 +0200
Subject: [PATCH] fix(proxy) hop-by-hop headers to not clear upgrade header on
 upgrade (#5495)

### Summary

`HTTP` `Connection` header usually carries a list of `headers` meant
for receiver (in this case proxy) that are not meant to be proxied
further (so called hop-by-hop headers). Our gateway correctly cleans
these headers, but it was a bit too aggressive.

It turns out as reported on #5465 that websockets upgrade mechanism
is broken with Firefox, but works on Chrome. The difference being that
Chrome sends following headers:

```http
Connection: Upgrade
Upgrade: websocket
```

while Firefox sends the following:

```
Connection: keep-alive, Upgrade
Upgrade: websocket
```

As the `Upgrade` was listed on `Connection` header by Firefox, the Kong
incorrecly removed the `Upgrade` header from the upstream headers, and
thus it made the websockets upgrade to fail.

This PR fixed it so that we do not remove `Upgrade` header on such case.

This PR also adds `keep-alive` to upstream request `Connection` header
when proxying the upgrade request (just like Firefox does).

### Issues Resolved

Fix #5465
---
 kong/runloop/handler.lua                         | 13 +++++++++----
 .../05-proxy/03-upstream_headers_spec.lua        | 16 ++++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/kong/runloop/handler.lua b/kong/runloop/handler.lua
index 513ded40061..a3a0282419c 100644
--- a/kong/runloop/handler.lua
+++ b/kong/runloop/handler.lua
@@ -1117,7 +1117,7 @@ return {
 
       -- Keep-Alive and WebSocket Protocol Upgrade Headers
       if var.http_upgrade and lower(var.http_upgrade) == "websocket" then
-        var.upstream_connection = "upgrade"
+        var.upstream_connection = "keep-alive, Upgrade"
         var.upstream_upgrade    = "websocket"
 
       else
@@ -1198,9 +1198,14 @@ return {
       -- clear hop-by-hop request headers:
       for _, header_name in csv(var.http_connection) do
         -- some of these are already handled by the proxy module,
-        -- proxy-authorization being an exception that is handled
-        -- below with special semantics.
-        if header_name ~= "proxy-authorization" then
+        -- proxy-authorization and upgrade being an exception that
+        -- is handled below with special semantics.
+        if header_name == "upgrade" then
+          if var.upstream_connection == "keep-alive" then
+            clear_header(header_name)
+          end
+
+        elseif header_name ~= "proxy-authorization" then
           clear_header(header_name)
         end
       end
diff --git a/spec/02-integration/05-proxy/03-upstream_headers_spec.lua b/spec/02-integration/05-proxy/03-upstream_headers_spec.lua
index 94cf2549e74..449b79d2cac 100644
--- a/spec/02-integration/05-proxy/03-upstream_headers_spec.lua
+++ b/spec/02-integration/05-proxy/03-upstream_headers_spec.lua
@@ -162,6 +162,22 @@ for _, strategy in helpers.each_strategy() do
 
         assert.equal("Expires", headers["Trailer"])
       end)
+
+      it("keeps upgrade when upgrading", function()
+        local res = assert(proxy_client:send {
+          method  = "GET",
+          headers = {
+            ["Host"] = "headers-inspect.com",
+            ["Connection"] = "keep-alive, Upgrade",
+            ["Upgrade"] = "websocket"
+          },
+          path = "/get",
+        })
+
+        local json = cjson.decode(assert.res_status(200, res))
+        assert.equal("keep-alive, Upgrade", json.headers.connection)
+        assert.equal("websocket", json.headers.upgrade)
+      end)
     end)
 
     describe("(using the default configuration values)", function()