From 14701ab37e46b334b04840ff90499cb46de4d52f Mon Sep 17 00:00:00 2001
From: samugi <samuele@konghq.com>
Date: Wed, 22 Mar 2023 17:19:47 +0100
Subject: [PATCH 1/2] fix(request-transformer): thread safety

Make the template environment thread safe by scoping it to the request
rather than the module

This was problematic because reading the body (during body
transformation) results in yielding.
---
 kong/plugins/request-transformer/access.lua   | 182 +++++++++---------
 .../36-request-transformer/02-access_spec.lua | 110 +++++++++++
 2 files changed, 198 insertions(+), 94 deletions(-)

diff --git a/kong/plugins/request-transformer/access.lua b/kong/plugins/request-transformer/access.lua
index 982b210d59f..f0772995025 100644
--- a/kong/plugins/request-transformer/access.lua
+++ b/kong/plugins/request-transformer/access.lua
@@ -26,7 +26,6 @@ local pl_copy_table = pl_tablex.deepcopy
 
 local _M = {}
 local template_cache = setmetatable( {}, { __mode = "k" })
-local template_environment
 
 local DEBUG = ngx.DEBUG
 local CONTENT_LENGTH = "content-length"
@@ -70,54 +69,7 @@ local function get_content_type(content_type)
   end
 end
 
--- meta table for the sandbox, exposing lazily loaded values
-local __meta_environment = {
-  __index = function(self, key)
-    local lazy_loaders = {
-      headers = function(self)
-        return get_headers() or EMPTY
-      end,
-      query_params = function(self)
-        return get_uri_args() or EMPTY
-      end,
-      uri_captures = function(self)
-        return (ngx.ctx.router_matches or EMPTY).uri_captures or EMPTY
-      end,
-      shared = function(self)
-        return ((kong or EMPTY).ctx or EMPTY).shared or EMPTY
-      end,
-    }
-    local loader = lazy_loaders[key]
-    if not loader then
-      -- we don't have a loader, so just return nothing
-      return
-    end
-    -- set the result on the table to not load again
-    local value = loader()
-    rawset(self, key, value)
-    return value
-  end,
-  __newindex = function(self)
-    error("This environment is read-only.")
-  end,
-}
-
-template_environment = setmetatable({
-  -- here we can optionally add functions to expose to the sandbox, eg:
-  -- tostring = tostring,  -- for example
-  -- because headers may contain array elements such as duplicated headers
-  -- type is a useful function in these cases. See issue #25.
-  type = type,
-}, __meta_environment)
-
-local function clear_environment(conf)
-  rawset(template_environment, "headers", nil)
-  rawset(template_environment, "query_params", nil)
-  rawset(template_environment, "uri_captures", nil)
-  rawset(template_environment, "shared", nil)
-end
-
-local function param_value(source_template, config_array)
+local function param_value(source_template, config_array, template_env)
   if not source_template or source_template == "" then
     return nil
   end
@@ -139,10 +91,10 @@ local function param_value(source_template, config_array)
     compiled_templates[source_template] = compiled_template
   end
 
-  return compiled_template:render(template_environment)
+  return compiled_template:render(template_env)
 end
 
-local function iter(config_array)
+local function iter(config_array, template_env)
   return function(config_array, i, previous_name, previous_value)
     i = i + 1
     local current_pair = config_array[i]
@@ -156,7 +108,7 @@ local function iter(config_array)
       return i, current_name
     end
 
-    local res, err = param_value(current_value, config_array)
+    local res, err = param_value(current_value, config_array, template_env)
     if err then
       return error("[request-transformer] failed to render the template " ..
                    current_value .. ", error:" .. err)
@@ -182,14 +134,14 @@ local function append_value(current_value, value)
   end
 end
 
-local function transform_headers(conf)
+local function transform_headers(conf, template_env)
   local headers = get_headers()
   local headers_to_remove = {}
 
   headers.host = nil
 
   -- Remove header(s)
-  for _, name, value in iter(conf.remove.headers) do
+  for _, name, value in iter(conf.remove.headers, template_env) do
     name = name:lower()
     if headers[name] then
       headers[name] = nil
@@ -198,7 +150,7 @@ local function transform_headers(conf)
   end
 
   -- Rename headers(s)
-  for _, old_name, new_name in iter(conf.rename.headers) do
+  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
@@ -209,7 +161,7 @@ local function transform_headers(conf)
   end
 
   -- Replace header(s)
-  for _, name, value in iter(conf.replace.headers) do
+  for _, name, value in iter(conf.replace.headers, template_env) do
     name = name:lower()
     if headers[name] or name == HOST then
       headers[name] = value
@@ -217,14 +169,14 @@ local function transform_headers(conf)
   end
 
   -- Add header(s)
-  for _, name, value in iter(conf.add.headers) do
+  for _, name, value in iter(conf.add.headers, template_env) do
     if not headers[name] and name:lower() ~= HOST then
       headers[name] = value
     end
   end
 
   -- Append header(s)
-  for _, name, value in iter(conf.append.headers) do
+  for _, name, value in iter(conf.append.headers, template_env) do
     local name_lc = name:lower()
 
     if name_lc ~= HOST and name ~= name_lc and headers[name] ~= nil then
@@ -247,7 +199,7 @@ local function transform_headers(conf)
   set_headers(headers)
 end
 
-local function transform_querystrings(conf)
+local function transform_querystrings(conf, template_env)
 
   if not (#conf.remove.querystring > 0 or #conf.rename.querystring > 0 or
           #conf.replace.querystring > 0 or #conf.add.querystring > 0 or
@@ -255,41 +207,41 @@ local function transform_querystrings(conf)
     return
   end
 
-  local querystring = pl_copy_table(template_environment.query_params)
+  local querystring = pl_copy_table(template_env.query_params)
 
   -- Remove querystring(s)
-  for _, name, value in iter(conf.remove.querystring) do
+  for _, name, value in iter(conf.remove.querystring, template_env) do
     querystring[name] = nil
   end
 
   -- Rename querystring(s)
-  for _, old_name, new_name in iter(conf.rename.querystring) do
+  for _, old_name, new_name in iter(conf.rename.querystring, template_env) do
     local value = querystring[old_name]
     querystring[new_name] = value
     querystring[old_name] = nil
   end
 
-  for _, name, value in iter(conf.replace.querystring) do
+  for _, name, value in iter(conf.replace.querystring, template_env) do
     if querystring[name] then
       querystring[name] = value
     end
   end
 
   -- Add querystring(s)
-  for _, name, value in iter(conf.add.querystring) do
+  for _, name, value in iter(conf.add.querystring, template_env) do
     if not querystring[name] then
       querystring[name] = value
     end
   end
 
   -- Append querystring(s)
-  for _, name, value in iter(conf.append.querystring) do
+  for _, name, value in iter(conf.append.querystring, template_env) do
     querystring[name] = append_value(querystring[name], value)
   end
   set_uri_args(querystring)
 end
 
-local function transform_json_body(conf, body, content_length)
+local function transform_json_body(conf, body, content_length, template_env)
   local removed, renamed, replaced, added, appended = false, false, false, false, false
   local content_length = (body and #body) or 0
   local parameters = parse_json(body)
@@ -301,14 +253,14 @@ local function transform_json_body(conf, body, content_length)
   end
 
   if content_length > 0 and #conf.remove.body > 0 then
-    for _, name, value in iter(conf.remove.body) do
+    for _, name, value in iter(conf.remove.body, template_env) do
       parameters[name] = nil
       removed = true
     end
   end
 
   if content_length > 0 and #conf.rename.body > 0 then
-    for _, old_name, new_name in iter(conf.rename.body) do
+    for _, old_name, new_name in iter(conf.rename.body, template_env) do
       local value = parameters[old_name]
       parameters[new_name] = value
       parameters[old_name] = nil
@@ -317,7 +269,7 @@ local function transform_json_body(conf, body, content_length)
   end
 
   if content_length > 0 and #conf.replace.body > 0 then
-    for _, name, value in iter(conf.replace.body) do
+    for _, name, value in iter(conf.replace.body, template_env) do
       if parameters[name] then
         parameters[name] = value
         replaced = true
@@ -326,7 +278,7 @@ local function transform_json_body(conf, body, content_length)
   end
 
   if #conf.add.body > 0 then
-    for _, name, value in iter(conf.add.body) do
+    for _, name, value in iter(conf.add.body, template_env) do
       if not parameters[name] then
         parameters[name] = value
         added = true
@@ -335,7 +287,7 @@ local function transform_json_body(conf, body, content_length)
   end
 
   if #conf.append.body > 0 then
-    for _, name, value in iter(conf.append.body) do
+    for _, name, value in iter(conf.append.body, template_env) do
       local old_value = parameters[name]
       parameters[name] = append_value(old_value, value)
       appended = true
@@ -347,19 +299,19 @@ local function transform_json_body(conf, body, content_length)
   end
 end
 
-local function transform_url_encoded_body(conf, body, content_length)
+local function transform_url_encoded_body(conf, body, content_length, template_env)
   local renamed, removed, replaced, added, appended = false, false, false, false, false
   local parameters = decode_args(body)
 
   if content_length > 0 and #conf.remove.body > 0 then
-    for _, name, value in iter(conf.remove.body) do
+    for _, name, value in iter(conf.remove.body, template_env) do
       parameters[name] = nil
       removed = true
     end
   end
 
   if content_length > 0 and #conf.rename.body > 0 then
-    for _, old_name, new_name in iter(conf.rename.body) do
+    for _, old_name, new_name in iter(conf.rename.body, template_env) do
       local value = parameters[old_name]
       parameters[new_name] = value
       parameters[old_name] = nil
@@ -368,7 +320,7 @@ local function transform_url_encoded_body(conf, body, content_length)
   end
 
   if content_length > 0 and #conf.replace.body > 0 then
-    for _, name, value in iter(conf.replace.body) do
+    for _, name, value in iter(conf.replace.body, template_env) do
       if parameters[name] then
         parameters[name] = value
         replaced = true
@@ -377,7 +329,7 @@ local function transform_url_encoded_body(conf, body, content_length)
   end
 
   if #conf.add.body > 0 then
-    for _, name, value in iter(conf.add.body) do
+    for _, name, value in iter(conf.add.body, template_env) do
       if parameters[name] == nil then
         parameters[name] = value
         added = true
@@ -386,7 +338,7 @@ local function transform_url_encoded_body(conf, body, content_length)
   end
 
   if #conf.append.body > 0 then
-    for _, name, value in iter(conf.append.body) do
+    for _, name, value in iter(conf.append.body, template_env) do
       local old_value = parameters[name]
       parameters[name] = append_value(old_value, value)
       appended = true
@@ -398,12 +350,12 @@ local function transform_url_encoded_body(conf, body, content_length)
   end
 end
 
-local function transform_multipart_body(conf, body, content_length, content_type_value)
+local function transform_multipart_body(conf, body, content_length, content_type_value, template_env)
   local removed, renamed, replaced, added, appended = false, false, false, false, false
   local parameters = multipart(body and body or "", content_type_value)
 
   if content_length > 0 and #conf.rename.body > 0 then
-    for _, old_name, new_name in iter(conf.rename.body) do
+    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
         parameters:set_simple(new_name, value)
@@ -414,14 +366,14 @@ local function transform_multipart_body(conf, body, content_length, content_type
   end
 
   if content_length > 0 and #conf.remove.body > 0 then
-    for _, name, value in iter(conf.remove.body) do
+    for _, name, value in iter(conf.remove.body, template_env) do
       parameters:delete(name)
       removed = true
     end
   end
 
   if content_length > 0 and #conf.replace.body > 0 then
-    for _, name, value in iter(conf.replace.body) do
+    for _, name, value in iter(conf.replace.body, template_env) do
       if parameters:get(name) then
         parameters:delete(name)
         parameters:set_simple(name, value)
@@ -431,7 +383,7 @@ local function transform_multipart_body(conf, body, content_length, content_type
   end
 
   if #conf.add.body > 0 then
-    for _, name, value in iter(conf.add.body) do
+    for _, name, value in iter(conf.add.body, template_env) do
       if not parameters:get(name) then
         parameters:set_simple(name, value)
         added = true
@@ -444,7 +396,7 @@ local function transform_multipart_body(conf, body, content_length, content_type
   end
 end
 
-local function transform_body(conf)
+local function transform_body(conf, template_env)
   local content_type_value = get_header(CONTENT_TYPE)
   local content_type = get_content_type(content_type_value)
   if content_type == nil or #conf.rename.body < 1 and
@@ -454,16 +406,19 @@ local function transform_body(conf)
   end
 
   -- Call req_read_body to read the request body first
-  local body = get_raw_body()
+  local body, err = get_raw_body()
+  if err then
+    kong.log.warn(err)
+  end
   local is_body_transformed = false
   local content_length = (body and #body) or 0
 
   if content_type == ENCODED then
-    is_body_transformed, body = transform_url_encoded_body(conf, body, content_length)
+    is_body_transformed, body = transform_url_encoded_body(conf, body, content_length, template_env)
   elseif content_type == MULTI then
-    is_body_transformed, body = transform_multipart_body(conf, body, content_length, content_type_value)
+    is_body_transformed, body = transform_multipart_body(conf, body, content_length, content_type_value, template_env)
   elseif content_type == JSON then
-    is_body_transformed, body = transform_json_body(conf, body, content_length)
+    is_body_transformed, body = transform_json_body(conf, body, content_length, template_env)
   end
 
   if is_body_transformed then
@@ -505,10 +460,10 @@ local function transform_method(conf)
   end
 end
 
-local function transform_uri(conf)
+local function transform_uri(conf, template_env)
   if conf.replace.uri then
 
-    local res, err = param_value(conf.replace.uri, conf.replace)
+    local res, err = param_value(conf.replace.uri, conf.replace, template_env)
     if err then
       error("[request-transformer] failed to render the template " ..
         tostring(conf.replace.uri) .. ", error:" .. err)
@@ -524,12 +479,51 @@ local function transform_uri(conf)
 end
 
 function _M.execute(conf)
-  clear_environment()
-  transform_uri(conf)
+  -- meta table for the sandbox, exposing lazily loaded values
+  local __meta_environment = {
+    __index = function(self, key)
+      local lazy_loaders = {
+        headers = function(self)
+          return get_headers() or EMPTY
+        end,
+        query_params = function(self)
+          return get_uri_args() or EMPTY
+        end,
+        uri_captures = function(self)
+          return (ngx.ctx.router_matches or EMPTY).uri_captures or EMPTY
+        end,
+        shared = function(self)
+          return ((kong or EMPTY).ctx or EMPTY).shared or EMPTY
+        end,
+      }
+      local loader = lazy_loaders[key]
+      if not loader then
+        -- we don't have a loader, so just return nothing
+        return
+      end
+      -- set the result on the table to not load again
+      local value = loader()
+      rawset(self, key, value)
+      return value
+    end,
+    __newindex = function(self)
+      error("This environment is read-only.")
+    end,
+  }
+
+  local template_env = setmetatable({
+    -- here we can optionally add functions to expose to the sandbox, eg:
+    -- tostring = tostring,  -- for example
+    -- because headers may contain array elements such as duplicated headers
+    -- type is a useful function in these cases. See issue #25.
+    type = type,
+  }, __meta_environment)
+
+  transform_uri(conf, template_env)
   transform_method(conf)
-  transform_headers(conf)
-  transform_body(conf)
-  transform_querystrings(conf)
+  transform_headers(conf, template_env)
+  transform_body(conf, template_env)
+  transform_querystrings(conf, template_env)
 end
 
 return _M
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 ccf61457626..760964b9699 100644
--- a/spec/03-plugins/36-request-transformer/02-access_spec.lua
+++ b/spec/03-plugins/36-request-transformer/02-access_spec.lua
@@ -2227,4 +2227,114 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function()
     end)
   end)
 end)
+
+describe("Plugin: request-transformer (thread safety) [#" .. strategy .. "]", function()
+  local db_strategy = strategy ~= "off" and strategy or nil
+
+  lazy_setup(function()
+    local bp = helpers.get_db_utils(db_strategy, {
+      "routes",
+      "services",
+      "plugins",
+    }, { "request-transformer", "pre-function" })
+
+    local route = bp.routes:insert({
+      hosts = { "test_thread_safety.test" }
+    })
+
+    bp.plugins:insert {
+      route = { id = route.id },
+      name = "pre-function",
+      config = {
+        access = {
+          [[
+            local delay = kong.request.get_header("slow_body_delay")
+            local orig_read_body = ngx.req.read_body
+            ngx.ctx.orig_read_body = orig_read_body
+            ngx.req.read_body = function()
+              ngx.sleep(tonumber(delay))
+              return orig_read_body()
+            end
+          ]]
+        },
+        header_filter = {
+          [[
+            ngx.req.read_body = ngx.ctx.orig_read_body or ngx.req.read_body
+          ]]
+        },
+      }
+    }
+
+    bp.plugins:insert {
+      route = { id = route.id },
+      name = "request-transformer",
+      config = {
+        add = {
+          querystring = { "added_q:yes_q" },
+          headers = { "added_h:yes_h" },
+          body = { "added_b:yes_b" }
+        }
+      }
+    }
+
+    assert(helpers.start_kong({
+      database = db_strategy,
+      plugins = "bundled, request-transformer",
+      nginx_conf = "spec/fixtures/custom_nginx.template",
+      nginx_worker_processes = 1
+    }))
+  end)
+
+  lazy_teardown(function()
+    helpers.stop_kong()
+  end)
+
+  it("sends requests with the expected values for headers, body, query", function()
+    local race_conditions = ""
+
+    local get_handler = function(header_val, body_param_val, query_param_val, delay)
+      return function()
+        local tmp_client = helpers.proxy_client()
+        local r = assert(tmp_client:send({
+          method = "POST",
+          path = "/request",
+          headers = {
+            ["Content-Type"] = "application/json",
+            host = "test_thread_safety.test",
+            slow_body_delay = delay,
+            h = header_val
+          },
+          body = {
+            k = body_param_val
+          },
+          query = {
+            q = query_param_val
+          }
+        }))
+
+        assert.response(r).has.status(200)
+        local header = assert.request(r).has.header("h")
+        local body_param = assert.request(r).has.jsonbody().params.k
+        local query_param = assert.request(r).has.queryparam("q")
+        if header_val ~= header then
+          race_conditions = race_conditions .. fmt("expected: %s, received: %s", header_val, header)
+        end
+        if body_param_val ~= body_param then
+          race_conditions = race_conditions .. fmt("expected: %s, received: %s", body_param_val, body_param)
+        end
+        if query_param ~= query_param_val then
+          race_conditions = race_conditions .. fmt("expected: %s, received: %s", query_param, query_param_val)
+        end
+        tmp_client:close()
+      end
+    end
+
+    local thread_1 = ngx.thread.spawn(get_handler("vh1", "b1", "vq1", 2))
+    local thread_2 = ngx.thread.spawn(get_handler("vh2", "b2", "vq2", 0))
+    ngx.thread.wait(thread_1)
+    ngx.thread.wait(thread_2)
+
+    assert.equals("", race_conditions)
+  end)
+end)
 end

From aea629a5791197657e8813b8635205a95da5f0f3 Mon Sep 17 00:00:00 2001
From: samugi <samuele@konghq.com>
Date: Wed, 22 Mar 2023 17:32:05 +0100
Subject: [PATCH 2/2] chore(*): changelog

---
 CHANGELOG.md | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0db935bf428..76059b96066 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -139,6 +139,11 @@
 - Fix an issue where empty value of URI argument `custom_id` crashes `/consumer`.
   [#10475](https://github.com/Kong/kong/pull/10475)
 
+#### Plugins
+- **Request-Transformer**: fix an issue where requests would intermittently
+  be proxied with incorrect query parameters.
+  [10539](https://github.com/Kong/kong/pull/10539)
+
 ### Changed
 
 #### Core