fix(proxy/pdk) re-implement X-Forwarded-Prefix / kong.request.get_for…

…warded_prefix

### Summary

The PR #5620 implemented `X-Forwarded-Prefix` wrong as reported in these:
- #6190
- Kong/kubernetes-ingress-controller#784
- #6132

And also by some other sources.

This commit re-implements `X-Forwarded-Prefix`:

1. Is there trusted `X-Forwarded-Prefix` header in request? If yes, then set
   that as the `X-Forwarded-Prefix` (note: at this point we don't support
   multi-value `X-Forwarded-Prefix` headers, e.g. appending. We don't support
   multi-value in `X-Forwarded-Host/-Port/-Proto` either.
2. Has Kong router stripped the prefix of the request URI before proxying, if
   yes, then set `X-Forwarded-Prefix` to match the stripped prefix.
3. If 1 or 2 didn't set the header, set it to `/`.

### Issues Resolved

Fix #6190
Fix Kong/kubernetes-ingress-controller#784
Fix #6132

kong/pdk/request.lua

@@ -16,6 +16,7 @@ local find = string.find
 local lower = string.lower
 local type = type
 local error = error
+local pcall = pcall
 local tonumber = tonumber
 local check_phase = phase_checker.check
 local check_not_phase = phase_checker.check_not
@@ -285,6 +286,54 @@ local function new(self)
   end
 
 
+  ---
+  -- Returns the prefix path component of the request's URL that Kong stripped
+  -- before proxying to upstream. It also checks if `X-Forwarded-Prefix` comes
+  -- from a trusted source, and uses it as is when given. The value is returned
+  -- as a Lua string.
+  --
+  -- In general, this function should be called after Kong has ran its router,
+  -- as the Kong router may strip the prefix of the request path. That stripped
+  -- path will become the return value of this function, unless there was already
+  -- a trusted `X-Forwarded-Prefix` header in the request.
+  --
+  -- Whether this function considers `X-Forwarded-Prefix` or not depends on
+  -- several Kong configuration parameters:
+  --
+  -- * [trusted\_ips](https://getkong.org/docs/latest/configuration/#trusted_ips)
+  -- * [real\_ip\_header](https://getkong.org/docs/latest/configuration/#real_ip_header)
+  -- * [real\_ip\_recursive](https://getkong.org/docs/latest/configuration/#real_ip_recursive)
+  --
+  -- **Note**: we do not currently do any normalization on the request path prefix
+  --           except return `"/"` on empty prefix.
+  --
+  -- @function kong.request.get_forwarded_prefix
+  -- @phases access, header_filter, body_filter, log, admin_api
+  -- @treturn string the forwarded path prefix
+  -- @usage
+  -- kong.request.get_forwarded_prefix() -- /prefix
+  function _REQUEST.get_forwarded_prefix()
+    check_phase(PHASES.request)
+
+    if self.ip.is_trusted(self.client.get_ip()) then
+      local prefix = _REQUEST.get_header(X_FORWARDED_PREFIX)
+      if prefix then
+        return prefix
+      end
+    end
+
+    local ok, prefix = pcall(function()
+      return ngx.var.upstream_x_forwarded_prefix
+    end)
+
+    if not ok or not prefix then
+      return "/"
+    end
+
+    return prefix == "" and "/" or prefix
+  end
+
+
   ---
   -- Returns the HTTP version used by the client in the request as a Lua
   -- number, returning values such as `1`, `1.1`, `2.0`, or `nil` for

kong/router.lua

@@ -846,9 +846,14 @@ do
               ctx.matches.uri = uri_t.value
 
               if m.uri_postfix then
+                ctx.matches.uri_prefix = sub(ctx.req_uri, 1, -(#m.uri_postfix + 1))
+
                 -- remove the uri_postfix group
-                m[#m]          = nil
+                m[#m] = nil
                 m.uri_postfix = nil
+
+              else
+                ctx.matches.uri_prefix = "/"
               end
 
               if uri_t.has_captures then
@@ -860,6 +865,7 @@ do
           end
 
           -- plain or prefix match from the index
+          ctx.matches.uri_prefix = sub(ctx.req_uri, 1, #uri_t.value)
           ctx.matches.uri_postfix = sub(ctx.req_uri, #uri_t.value + 1)
           ctx.matches.uri = uri_t.value
 
@@ -882,9 +888,14 @@ do
             ctx.matches.uri = uri_t.value
 
             if m.uri_postfix then
+              ctx.matches.uri_prefix = sub(ctx.req_uri, 1, -(#m.uri_postfix + 1))
+
               -- remove the uri_postfix group
-              m[#m]          = nil
+              m[#m] = nil
               m.uri_postfix = nil
+
+            else
+              ctx.matches.uri_prefix = "/"
             end
 
             if uri_t.has_captures then
@@ -898,6 +909,7 @@ do
           -- plain or prefix match (not from the index)
           local from, to = find(ctx.req_uri, uri_t.value, nil, true)
           if from == 1 then
+            ctx.matches.uri_prefix = sub(ctx.req_uri, 1, to)
             ctx.matches.uri_postfix = sub(ctx.req_uri, to + 1)
             ctx.matches.uri = uri_t.value
 
@@ -1560,9 +1572,13 @@ function _M.new(routes)
               matched_route.route = routes_by_id[matched_route.route.id].route
             end
 
+            local request_prefix
+
             -- Path construction
 
             if matched_route.type == "http" then
+              request_prefix = matched_route.strip_uri and matches.uri_prefix or "/"
+
               -- if we do not have a path-match, then the postfix is simply the
               -- incoming path, without the initial slash
               local request_postfix = matches.uri_postfix or sub(req_uri, 2, -1)
@@ -1651,6 +1667,7 @@ function _M.new(routes)
               upstream_scheme = upstream_url_t.scheme,
               upstream_uri    = upstream_uri,
               upstream_host   = upstream_host,
+              prefix          = request_prefix,
               matches         = {
                 uri_captures  = matches.uri_captures,
                 uri           = matches.uri,

kong/runloop/handler.lua

@@ -1112,6 +1112,7 @@ return {
       local forwarded_host
       local forwarded_port
       local forwarded_path
+      local forwarded_prefix
 
       -- X-Forwarded-* Headers Parsing
       --
@@ -1128,6 +1129,7 @@ return {
         forwarded_host   = var.http_x_forwarded_host   or host
         forwarded_port   = var.http_x_forwarded_port   or port
         forwarded_path   = var.http_x_forwarded_path
+        forwarded_prefix = var.http_x_forwarded_prefix
 
       else
         forwarded_proto  = scheme
@@ -1143,6 +1145,14 @@ return {
         end
       end
 
+      if not forwarded_prefix then
+        forwarded_prefix = match_t.prefix
+
+        if not forwarded_prefix or forwarded_prefix == "" then
+          forwarded_prefix = "/"
+        end
+      end
+
       local protocols = route.protocols
       if (protocols and protocols.https and not protocols.http and
           forwarded_proto ~= "https")
@@ -1231,6 +1241,7 @@ return {
       var.upstream_x_forwarded_host   = forwarded_host
       var.upstream_x_forwarded_port   = forwarded_port
       var.upstream_x_forwarded_path   = forwarded_path
+      var.upstream_x_forwarded_prefix = forwarded_prefix
 
       -- At this point, the router and `balancer_setup_stage1` have been
       -- executed; detect requests that need to be redirected from `proxy_pass`

spec/01-unit/08-router_spec.lua

@@ -2498,7 +2498,7 @@ describe("Router", function()
       assert.equal("/endel%C3%B8st", match_t.upstream_uri)
     end)
 
-    describe("stripped paths", function()
+    describe("stripped paths #strip", function()
       local router
       local use_case_routes = {
         {
@@ -2516,6 +2516,7 @@ describe("Router", function()
             id         = uuid(),
             methods    = { "POST" },
             paths      = { "/my-route", "/this-route" },
+            strip_path = false,
           },
         },
       }
@@ -2530,6 +2531,7 @@ describe("Router", function()
         router._set_ngx(_ngx)
         local match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/my-route", match_t.prefix)
         assert.equal("/hello/world", match_t.upstream_uri)
       end)
 
@@ -2538,6 +2540,7 @@ describe("Router", function()
         router._set_ngx(_ngx)
         local match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/my-route", match_t.prefix)
         assert.equal("/", match_t.upstream_uri)
       end)
 
@@ -2547,6 +2550,7 @@ describe("Router", function()
         router._set_ngx(_ngx)
         local match_t = router.exec()
         assert.same(use_case_routes[2].route, match_t.route)
+        assert.equal("/", match_t.prefix)
         assert.equal("/my-route/hello/world", match_t.upstream_uri)
       end)
 
@@ -2568,6 +2572,7 @@ describe("Router", function()
         router._set_ngx(_ngx)
         local match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/", match_t.prefix)
         assert.equal("/my-route/hello/world", match_t.upstream_uri)
       end)
 
@@ -2576,12 +2581,14 @@ describe("Router", function()
         router._set_ngx(_ngx)
         local match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/my-route", match_t.prefix)
         assert.equal("/", match_t.upstream_uri)
 
         _ngx = mock_ngx("GET", "/my-route", { host = "domain.org" })
         router._set_ngx(_ngx)
         match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/my-route", match_t.prefix)
         assert.equal("/", match_t.upstream_uri)
       end)
 
@@ -2590,24 +2597,28 @@ describe("Router", function()
         router._set_ngx(_ngx)
         local match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/my-route", match_t.prefix)
         assert.equal("/", match_t.upstream_uri)
 
         _ngx = mock_ngx("GET", "/this-route", { host = "domain.org" })
         router._set_ngx(_ngx)
         match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/this-route", match_t.prefix)
         assert.equal("/", match_t.upstream_uri)
 
         _ngx = mock_ngx("GET", "/my-route", { host = "domain.org" })
         router._set_ngx(_ngx)
         match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/my-route", match_t.prefix)
         assert.equal("/", match_t.upstream_uri)
 
         _ngx = mock_ngx("GET", "/this-route", { host = "domain.org" })
         router._set_ngx(_ngx)
         match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/this-route", match_t.prefix)
         assert.equal("/", match_t.upstream_uri)
       end)
 
@@ -2627,6 +2638,7 @@ describe("Router", function()
         router._set_ngx(_ngx)
         local match_t = router.exec()
         assert.same(use_case_routes[1].route, match_t.route)
+        assert.equal("/endel%C3%B8st", match_t.prefix)
         assert.equal("/", match_t.upstream_uri)
       end)
 
@@ -2646,6 +2658,7 @@ describe("Router", function()
                               { host = "domain.org" })
         router._set_ngx(_ngx)
         local match_t = router.exec()
+        assert.equal("/users/123/profile", match_t.prefix)
         assert.equal("/hello/world", match_t.upstream_uri)
       end)
 
@@ -2665,6 +2678,7 @@ describe("Router", function()
                               { host = "domain.org" })
         router._set_ngx(_ngx)
         local match_t = router.exec()
+        assert.equal("/users/123/profile", match_t.prefix)
         assert.equal("/hello/world", match_t.upstream_uri)
       end)
     end)

spec/02-integration/05-proxy/03-upstream_headers_spec.lua

@@ -303,6 +303,25 @@ for _, strategy in helpers.each_strategy() do
         end)
       end)
 
+      describe("X-Forwarded-Prefix", function()
+        it("should be added if not present in request", function()
+          local headers = request_headers {
+            ["Host"] = "headers-inspect.com",
+          }
+
+          assert.equal("/", headers["x-forwarded-prefix"])
+        end)
+
+        it("should be replaced if present in request", function()
+          local headers = request_headers {
+            ["Host"]               = "headers-inspect.com",
+            ["X-Forwarded-Prefix"] = "/replaced",
+          }
+
+          assert.equal("/", headers["x-forwarded-prefix"])
+        end)
+      end)
+
       describe("with the downstream host preserved", function()
         it("should be added if not present in request while preserving the downstream host", function()
           local headers = request_headers {
@@ -316,6 +335,7 @@ for _, strategy in helpers.each_strategy() do
           assert.equal("preserved.com", headers["x-forwarded-host"])
           assert.equal(helpers.get_proxy_port(false), tonumber(headers["x-forwarded-port"]))
           assert.equal("/", headers["x-forwarded-path"])
+          assert.equal("/", headers["x-forwarded-prefix"])
         end)
 
         it("should be added if present in request while preserving the downstream host", function()
@@ -335,6 +355,7 @@ for _, strategy in helpers.each_strategy() do
           assert.equal("preserved.com", headers["x-forwarded-host"])
           assert.equal(helpers.get_proxy_port(false), tonumber(headers["x-forwarded-port"]))
           assert.equal("/", headers["x-forwarded-path"])
+          assert.equal("/", headers["x-forwarded-prefix"])
         end)
       end)
 
@@ -353,6 +374,7 @@ for _, strategy in helpers.each_strategy() do
           assert.equal("headers-inspect.com", headers["x-forwarded-host"])
           assert.equal(helpers.get_proxy_port(false), tonumber(headers["x-forwarded-port"]))
           assert.equal("/", headers["x-forwarded-path"])
+          assert.equal("/", headers["x-forwarded-prefix"])
         end)
 
         it("if present in request while discarding the downstream host", function()
@@ -374,6 +396,7 @@ for _, strategy in helpers.each_strategy() do
           assert.equal("headers-inspect.com", headers["x-forwarded-host"])
           assert.equal(helpers.get_proxy_port(false), tonumber(headers["x-forwarded-port"]))
           assert.equal("/", headers["x-forwarded-path"])
+          assert.equal("/", headers["x-forwarded-prefix"])
         end)
       end)
 
@@ -505,6 +528,25 @@ for _, strategy in helpers.each_strategy() do
         end)
       end)
 
+      describe("X-Forwarded-Prefix", function()
+        it("should be added if not present in request", function()
+          local headers = request_headers {
+            ["Host"] = "headers-inspect.com",
+          }
+
+          assert.equal("/", headers["x-forwarded-prefix"])
+        end)
+
+        it("should be forwarded if present in request", function()
+          local headers = request_headers {
+            ["Host"] = "headers-inspect.com",
+            ["X-Forwarded-Prefix"] = "/original-path",
+          }
+
+          assert.equal("/original-path", headers["x-forwarded-prefix"])
+        end)
+      end)
+
     end)
 
     describe("(using the non-trusted configuration values)", function()
@@ -632,6 +674,25 @@ for _, strategy in helpers.each_strategy() do
           assert.equal("/", headers["x-forwarded-path"])
         end)
       end)
+
+      describe("X-Forwarded-Prefix", function()
+        it("should be added if not present in request", function()
+          local headers = request_headers {
+            ["Host"] = "headers-inspect.com",
+          }
+
+          assert.equal("/", headers["x-forwarded-prefix"])
+        end)
+
+        it("should be replaced if present in request", function()
+          local headers = request_headers {
+            ["Host"]             = "headers-inspect.com",
+            ["X-Forwarded-Prefix"] = "/untrusted",
+          }
+
+          assert.equal("/", headers["x-forwarded-prefix"])
+        end)
+      end)
     end)
 
     describe("(using the recursive trusted configuration values)", function()