From 399042e1731a7d733e506cef08e91c0d3caefa17 Mon Sep 17 00:00:00 2001 From: xumin Date: Tue, 30 Jul 2024 16:42:59 +0800 Subject: [PATCH] fix(plugins): request transformer rename handling 1. Changing cases; 2. overriding existing argument Fix KAG-4915 --- kong/plugins/request-transformer/access.lua | 53 +++++++++-------- .../36-request-transformer/02-access_spec.lua | 59 ++++++++++++++++++- 2 files changed, 86 insertions(+), 26 deletions(-) diff --git a/kong/plugins/request-transformer/access.lua b/kong/plugins/request-transformer/access.lua index 8b8a301e6a50..c3a93da6f3a5 100644 --- a/kong/plugins/request-transformer/access.lua +++ b/kong/plugins/request-transformer/access.lua @@ -148,6 +148,19 @@ local function append_value(current_value, value) end end +local function rename(tbl, old_name, new_name) + if old_name == new_name then + return + end + + local value = tbl[old_name] + if value then + tbl[old_name] = nil + tbl[new_name] = value + return old_name + end +end + local function transform_headers(conf, template_env) local headers = get_headers() local headers_to_remove = {} @@ -165,12 +178,15 @@ local function transform_headers(conf, template_env) -- Rename headers(s) for _, old_name, new_name in iter(conf.rename.headers, template_env) do - old_name = old_name:lower() - local value = headers[old_name] - if value then - headers[new_name:lower()] = value - headers[old_name] = nil - headers_to_remove[old_name] = true + local lower_old_name, lower_new_name = old_name:lower(), new_name:lower() + -- headers by default are case-insensitive + -- but if we have a case change, we need to handle it as a special case + if lower_old_name ~= lower_new_name then + old_name, new_name = lower_old_name, lower_new_name + end + local toremove = rename(headers, old_name, new_name) + if toremove then + headers_to_remove[toremove] = true end end @@ -230,11 +246,7 @@ local function transform_querystrings(conf, template_env) -- Rename querystring(s) for _, old_name, new_name in iter(conf.rename.querystring, template_env) do - local value = querystring[old_name] - if value then - querystring[old_name] = nil - querystring[new_name] = value - end + rename(querystring, old_name, new_name) end for _, name, value in iter(conf.replace.querystring, template_env) do @@ -277,12 +289,7 @@ local function transform_json_body(conf, body, content_length, template_env) if content_length > 0 and #conf.rename.body > 0 then for _, old_name, new_name in iter(conf.rename.body, template_env) do - local value = parameters[old_name] - if value then - parameters[old_name] = nil - parameters[new_name] = value - renamed = true - end + renamed = rename(parameters, old_name, new_name) and true end end @@ -330,12 +337,7 @@ local function transform_url_encoded_body(conf, body, content_length, template_e if content_length > 0 and #conf.rename.body > 0 then for _, old_name, new_name in iter(conf.rename.body, template_env) do - local value = parameters[old_name] - if value then - parameters[old_name] = nil - parameters[new_name] = value - renamed = true - end + renamed = rename(parameters, old_name, new_name) and true or renamed end end @@ -376,8 +378,9 @@ local function transform_multipart_body(conf, body, content_length, content_type if content_length > 0 and #conf.rename.body > 0 then for _, old_name, new_name in iter(conf.rename.body, template_env) do - if parameters:get(old_name) then - local value = parameters:get(old_name).value + local para = parameters:get(old_name) + if para and old_name ~= new_name then + local value = para.value parameters:set_simple(new_name, value) parameters:delete(old_name) renamed = true diff --git a/spec/03-plugins/36-request-transformer/02-access_spec.lua b/spec/03-plugins/36-request-transformer/02-access_spec.lua index aeffa72c7f45..105623b77841 100644 --- a/spec/03-plugins/36-request-transformer/02-access_spec.lua +++ b/spec/03-plugins/36-request-transformer/02-access_spec.lua @@ -2,6 +2,9 @@ local admin_api = require "spec.fixtures.admin_api" local helpers = require "spec.helpers" local cjson = require "cjson" local pl_file = require "pl.file" +local http_mock = require "spec.helpers.http_mock" + +local MOCK_PORT = helpers.get_available_port() local fmt = string.format @@ -16,7 +19,7 @@ end for _, strategy in helpers.each_strategy() do describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function() - local client + local client, mock_server lazy_setup(function() local bp = helpers.get_db_utils(strategy, { @@ -487,6 +490,30 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function() } } + do -- rename case tests + -- as assert.request(r) does not support case-sensitive header checks + -- we need to use a mock server + mock_server = http_mock.new(MOCK_PORT) + mock_server:start() + local mock_service = bp.services:insert { + url = "http://localhost:" .. MOCK_PORT + } + local route = bp.routes:insert { + hosts = { "rename.mock" }, + service = mock_service, + } + bp.plugins:insert { + route = { id = route.id }, + name = "request-transformer", + config = { + rename = { + headers = { "rename:Rename" }, + querystring = { "inexist:exist" }, + } + } + } + end + assert(helpers.start_kong({ database = strategy, plugins = "bundled, request-transformer", @@ -495,6 +522,7 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function() end) lazy_teardown(function() + mock_server:stop() helpers.stop_kong() end) @@ -909,6 +937,35 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function() local json = assert.request(r).has.jsonbody() assert.equals("{\"emptyarray\":[]}", json.data) end) + it("rename correctly when only changing capitalization", function() + local r = assert(client:send { + method = "GET", + path = "/request", + headers = { + host = "rename.mock", + ["rename"] = "true", + } + }) + assert.response(r).has.status(200) + local ret = mock_server:get_request() + + assert.equals("true", ret.headers["Rename"]) + assert.is_nil(ret.headers["rename"]) + end) + -- but should we override with a value? + it("does not override existing value with nil", function() + local r = assert(client:send { + method = "GET", + path = "/request?exist=true", + headers = { + host = "rename.mock", + } + }) + assert.response(r).has.status(200) + local ret = mock_server:get_request() + print(require"inspect"(ret)) + assert.equals("/request?exist=true", ret.uri) + end) end) describe("replace", function()