From 808daa1ea5b830590b618ca86c324378051d4f1b Mon Sep 17 00:00:00 2001 From: thefosk Date: Tue, 10 Nov 2015 16:03:12 -0800 Subject: [PATCH] Fixing trailing slash bug --- kong/core/resolver.lua | 23 +++++++++++----- .../admin_api/kong_routes_spec.lua | 1 - spec/integration/proxy/api_resolver_spec.lua | 26 ++++++++++++++++++- spec/unit/core/resolver_spec.lua | 4 +++ 4 files changed, 46 insertions(+), 8 deletions(-) diff --git a/kong/core/resolver.lua b/kong/core/resolver.lua index c070acd99ca7..46d020505cd8 100644 --- a/kong/core/resolver.lua +++ b/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,12 +226,14 @@ 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 @@ -229,7 +241,6 @@ function _M.execute() else ngx.var.upstream_host = get_host_from_url(upstream_url) end - return api, upstream_url end diff --git a/spec/integration/admin_api/kong_routes_spec.lua b/spec/integration/admin_api/kong_routes_spec.lua index f6d1d4f920c6..7d4a1af0f152 100644 --- a/spec/integration/admin_api/kong_routes_spec.lua +++ b/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 diff --git a/spec/integration/proxy/api_resolver_spec.lua b/spec/integration/proxy/api_resolver_spec.lua index c10db4b65056..90557dc9401b 100644 --- a/spec/integration/proxy/api_resolver_spec.lua +++ b/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() diff --git a/spec/unit/core/resolver_spec.lua b/spec/unit/core/resolver_spec.lua index 9aa477188f8d..f49b3cd85a05 100644 --- a/spec/unit/core/resolver_spec.lua +++ b/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)