fix(key-auth): keep query params order if hide_credentials is true

changelog/unreleased/kong/fix-key-auth-query-params-order.yml

@@ -0,0 +1,3 @@
+message: "keep query params order in key-auth plugin if hide_credentials is true"
+type: bugfix
+scope: Plugin

kong/plugins/key-auth/handler.lua

@@ -8,6 +8,8 @@ local error = error
 local ipairs = ipairs
 local tostring = tostring
 local fmt = string.format
+local ngx_re_gsub = ngx.re.gsub
+local ngx_unescape_uri = ngx.unescape_uri
 
 
 local HEADERS_CONSUMER_ID           = constants.HEADERS.CONSUMER_ID
@@ -105,6 +107,21 @@ local function unauthorized(message, www_auth_content)
   return { status = 401, message = message, headers = { ["WWW-Authenticate"] = www_auth_content } }
 end
 
+local function remove_query_key(raw_query, key)
+  local pattern = key .. "=[^&]*&?"
+  -- make sure we are dealing with a url-decoded query before applying the substitution.
+  local unescaped_raw_query = ngx_unescape_uri(raw_query)
+  local new_query, _, err = ngx_re_gsub(unescaped_raw_query, pattern, "", "oj")
+  if err then
+    return nil, err
+  end
+  new_query, _, err = ngx_re_gsub(new_query, "&$", "", "oj")
+  if err then
+    return nil, err
+  end
+  return new_query
+end
+
 local function do_authentication(conf)
   if type(conf.key_names) ~= "table" then
     kong.log.err("no conf.key_names set, aborting plugin execution")
@@ -143,9 +160,18 @@ local function do_authentication(conf)
       key = v
 
       if conf.hide_credentials then
-        query[name] = nil
-        kong.service.request.set_query(query)
-        kong.service.request.clear_header(name)
+        if conf.key_in_query then
+          local raw_query = kong.request.get_raw_query()
+          local new_query, err = remove_query_key(raw_query, name)
+          if err then
+            kong.log.info("Cannot remove key from query: ", err)
+          else
+            kong.service.request.set_raw_query(new_query)
+          end
+        end
+        if conf.key_in_header then
+          kong.service.request.clear_header(name)
+        end
 
         if conf.key_in_body then
           if not body then
@@ -163,7 +189,6 @@ local function do_authentication(conf)
       end
 
       break
-
     elseif type(v) == "table" then
       -- duplicate API key
       return nil, unauthorized(ERR_DUPLICATE_API_KEY, www_auth_content)
@@ -267,5 +292,4 @@ function KeyAuthHandler:access(conf)
   end
 end
 
-
 return KeyAuthHandler

spec/03-plugins/09-key-auth/02-access_spec.lua

@@ -712,6 +712,48 @@ for _, strategy in helpers.each_strategy() do
         assert.matches("No API key found in request", json.message)
         assert.equal('Key', res.headers["WWW-Authenticate"])
       end)
+
+      it("false does not remove apikey and preserves order of query parameters", function()
+        local res = assert(proxy_client:send {
+          method = "GET",
+          path = "/request?c=value1&b=value2&apikey=kong&a=value3",
+          headers = {
+            ["Host"] = "key-auth1.test"
+          }
+        })
+        local body = assert.res_status(200, res)
+        local json = cjson.decode(body)
+
+        assert.equal("/request?c=value1&b=value2&apikey=kong&a=value3", json.vars.request_uri)
+      end)
+
+      it("true removes apikey and preserves order of query parameters", function()
+        local res = assert(proxy_client:send{
+          method = "GET",
+          path = "/request?c=value1&b=value2&apikey=kong&a=value3",
+          headers = {
+            ["Host"] = "key-auth2.test"
+          }
+        })
+        local body = assert.res_status(200, res)
+        local json = cjson.decode(body)
+
+        assert.equal("/request?c=value1&b=value2&a=value3", json.vars.request_uri)
+      end)
+
+      it("true removes apikey in encoded query and preserves order of query parameters", function()
+        local res = assert(proxy_client:send {
+          method = "GET",
+          path = "/request?c=valu%651&b=value2&api%6B%65%79=kong&a=valu%653",
+          headers = {
+            ["Host"] = "key-auth2.test"
+          }
+        })
+        local body = assert.res_status(200, res)
+        local json = cjson.decode(body)
+
+        assert.equal("/request?c=value1&b=value2&a=value3", json.vars.request_uri)
+      end)
     end)
 
     describe("config.anonymous", function()