Fixing trailing slash bug

kong/core/resolver.lua

@@ -39,7 +39,7 @@ local function get_upstream_url(api)
   local len = string_len(result)
   if string_sub(result, len, len) == "/" then
     -- Remove one slash to avoid having a double slash
-    -- Because ngx.var.uri always starts with a slash
+    -- Because ngx.var.request_uri always starts with a slash
     result = string_sub(result, 0, len - 1)
   end
 
@@ -159,9 +159,14 @@ end
 
 -- Replace `/request_path` with `request_path`, and then prefix with a `/`
 -- or replace `/request_path/foo` with `/foo`, and then do not prefix with `/`.
-function _M.strip_request_path(uri, strip_request_path_pattern)
+function _M.strip_request_path(uri, strip_request_path_pattern, upstream_url_has_path)
   local uri = string_gsub(uri, strip_request_path_pattern, "", 1)
-  if string_sub(uri, 0, 1) ~= "/" then
+
+  -- Sometimes uri can be an empty string, and adding a slash "/"..uri will lead to a trailing slash
+  -- We don't want to add a trailing slash in one specific scenario, when the upstream_url already has
+  -- a path (so it's not root, like http://hello.com/, but http://hello.com/path) in order to avoid
+  -- having an unnecessary trailing slash not wanted by the user. Hence the "upstream_url_has_path" check.
+  if string_sub(uri, 0, 1) ~= "/" and not upstream_url_has_path then
     uri = "/"..uri
   end
   return uri
@@ -203,6 +208,11 @@ local function find_api(uri)
   return nil, api, all_hosts, strip_request_path_pattern
 end
 
+local function url_has_path(url)
+  local _, count_slashes = string.gsub(url, "/", "")
+  return count_slashes > 2
+end
+
 function _M.execute()
   local uri = stringy.split(ngx.var.request_uri, "?")[1]
   local err, api, hosts, strip_request_path_pattern = find_api(uri)
@@ -216,20 +226,21 @@ function _M.execute()
     }
   end
 
+  local upstream_url = get_upstream_url(api)
+
   -- If API was retrieved by request_path and the request_path needs to be stripped
   if strip_request_path_pattern and api.strip_request_path then
-    uri = _M.strip_request_path(uri, strip_request_path_pattern)
+    uri = _M.strip_request_path(uri, strip_request_path_pattern, url_has_path(upstream_url))
   end
 
-  local upstream_url = get_upstream_url(api)..uri
+  upstream_url = upstream_url..uri
 
   -- Set the
   if api.preserve_host then
     ngx.var.upstream_host = ngx.req.get_headers()["host"]
   else
     ngx.var.upstream_host = get_host_from_url(upstream_url)
   end
-
   return api, upstream_url
 end
 

spec/integration/admin_api/kong_routes_spec.lua

@@ -1,7 +1,6 @@
 local json = require "cjson"
 local http_client = require "kong.tools.http_client"
 local spec_helper = require "spec.spec_helpers"
-local IO = require "kong.tools.io"
 local utils = require "kong.tools.utils"
 local env = spec_helper.get_env() -- test environment
 local dao_factory = env.dao_factory

spec/integration/proxy/api_resolver_spec.lua

@@ -37,7 +37,11 @@ describe("Resolver", function()
         {name = "tests-wildcard-subdomain-2", upstream_url = "http://mockbin.com/status/201", request_host = "wildcard.*"},
         {name = "tests-preserve-host", request_host = "httpbin-nopreserve.com", upstream_url = "http://httpbin.org"},
         {name = "tests-preserve-host-2", request_host = "httpbin-preserve.com", upstream_url = "http://httpbin.org", preserve_host = true},
-        {name = "tests-uri", request_host = "mockbin-uri.com", upstream_url = "http://mockbin.org"}
+        {name = "tests-uri", request_host = "mockbin-uri.com", upstream_url = "http://mockbin.org"},
+        {name = "tests-trailing-slash-path", request_path = "/test-trailing-slash", strip_request_path = true, upstream_url = "http://www.mockbin.org/request"},
+        {name = "tests-trailing-slash-path2", request_path = "/test-trailing-slash2", strip_request_path = false, upstream_url = "http://www.mockbin.org/request"},
+        {name = "tests-trailing-slash-path3", request_path = "/test-trailing-slash3", strip_request_path = true, upstream_url = "http://www.mockbin.org"},
+        {name = "tests-trailing-slash-path4", request_path = "/test-trailing-slash4", strip_request_path = true, upstream_url = "http://www.mockbin.org/"}
       },
       plugin = {
         {name = "key-auth", config = {key_names = {"apikey"} }, __api = 2}
@@ -194,6 +198,26 @@ describe("Resolver", function()
         local upstream_url = body.log.entries[1].request.url
         assert.equal("http://mockbin.com/har/of/request", upstream_url)
       end)
+      it("should not add a trailing slash when strip_path is enabled", function()
+        local response, status = http_client.get(spec_helper.PROXY_URL.."/test-trailing-slash", { hello = "world"})
+        assert.equal(200, status)
+        assert.equal("http://www.mockbin.org/request?hello=world", cjson.decode(response).url)
+      end)
+      it("should not add a trailing slash when strip_path is disabled", function()
+        local response, status = http_client.get(spec_helper.PROXY_URL.."/test-trailing-slash2", { hello = "world"})
+        assert.equal(200, status)
+        assert.equal("http://www.mockbin.org/request/test-trailing-slash2?hello=world", cjson.decode(response).url)
+      end)
+      it("should not add a trailing slash when strip_path is enabled and upstream_url has no path", function()
+        local response, status = http_client.get(spec_helper.PROXY_URL.."/test-trailing-slash3/request", { hello = "world"})
+        assert.equal(200, status)
+        assert.equal("http://www.mockbin.org/request?hello=world", cjson.decode(response).url)
+      end)
+      it("should not add a trailing slash when strip_path is enabled and upstream_url has single path", function()
+        local response, status = http_client.get(spec_helper.PROXY_URL.."/test-trailing-slash4/request", { hello = "world"})
+        assert.equal(200, status)
+        assert.equal("http://www.mockbin.org/request?hello=world", cjson.decode(response).url)
+      end)
     end)
 
     it("should return the correct Server and Via headers when the request was proxied", function()

spec/unit/core/resolver_spec.lua

@@ -116,5 +116,9 @@ describe("Resolver Access", function()
     it("should only strip the first pattern", function()
       assert.equal("/mockbin/status/200/mockbin", resolver_access.strip_request_path("/mockbin/mockbin/status/200/mockbin", apis_dics.request_path_arr[1].strip_request_path_pattern))
     end)
+    it("should not add final slash", function()
+      assert.equal("hello", resolver_access.strip_request_path("hello", apis_dics.request_path_arr[3].strip_request_path_pattern, true))
+      assert.equal("/hello", resolver_access.strip_request_path("hello", apis_dics.request_path_arr[3].strip_request_path_pattern, false))
+    end)
   end)
 end)