feat(pdk): add kong.service.request.clear_query_arg

changelog/unreleased/kong/feat-pdk-clear-query-arg.yml

@@ -0,0 +1,3 @@
+message: Added `kong.service.request.clear_query_arg(name)` to PDK.
+type: feature
+scope: PDK

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

@@ -0,0 +1,3 @@
+message: "**key-auth**: Fixed to retain order of query arguments when hiding the credentials."
+type: bugfix
+scope: Plugin

kong/pdk/service/request.lua

@@ -23,6 +23,7 @@ local validate_header = checks.validate_header
 local validate_headers = checks.validate_headers
 local check_phase = phase_checker.check
 local escape = require("kong.tools.uri").escape
+local search_remove = require("resty.ada.search").remove
 
 
 local PHASES = phase_checker.phases
@@ -272,11 +273,37 @@ local function new(self)
   end
 
 
+  ---
+  -- Removes all occurrences of the specified query string argument
+  -- from the request to the Service. The order of query string
+  -- arguments is retained.
+  --
+  -- @function kong.service.request.clear_query_arg
+  -- @phases `rewrite`, `access`
+  -- @tparam string name
+  -- @return Nothing; throws an error on invalid inputs.
+  -- @usage
+  -- kong.service.request.clear_query_arg("foo")
+  request.clear_query_arg = function(name)
+    check_phase(access_and_rewrite)
+
+    if type(name) ~= "string" then
+      error("query argument name must be a string", 2)
+    end
+
+    local args = ngx_var.args
+    if args and args ~= "" then
+      ngx_var.args = search_remove(args, name)
+    end
+  end
+
+
   local set_authority
   if ngx.config.subsystem ~= "stream" then
     set_authority = require("resty.kong.grpc").set_authority
   end
 
+
   ---
   -- Sets a header in the request to the Service with the given value. Any existing header
   -- with the same name will be overridden.

kong/plugins/key-auth/handler.lua

@@ -143,8 +143,7 @@ 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_query_arg(name)
         kong.service.request.clear_header(name)
 
         if conf.key_in_body then

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("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("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("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()

t/01-pdk/06-service-request/00-phase_checks.t

@@ -102,6 +102,18 @@ qq{
                 body_filter   = "forced false",
                 log           = "forced false",
                 admin_api     = "forced false",
+            }, {
+                method        = "clear_query_arg",
+                args          = { "foo" },
+                init_worker   = false,
+                certificate   = "pending",
+                rewrite       = true,
+                access        = true,
+                response      = "forced false",
+                header_filter = "forced false",
+                body_filter   = "forced false",
+                log           = "forced false",
+                admin_api     = "forced false",
             }, {
                 method        = "set_header",
                 args          = { "X-Foo", "bar" },

t/01-pdk/06-service-request/14-clear_query_arg.t

@@ -0,0 +1,234 @@
+use strict;
+use warnings FATAL => 'all';
+use Test::Nginx::Socket::Lua;
+do "./t/Util.pm";
+
+$ENV{TEST_NGINX_NXSOCK} ||= html_dir();
+
+plan tests => repeat_each() * (blocks() * 3);
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: service.request.clear_query_arg() errors if arguments are not given
+--- http_config eval: $t::Util::HttpConfig
+--- config
+    location = /t {
+        content_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            local pok, err = pcall(pdk.service.request.clear_query_arg)
+            ngx.say(err)
+        }
+    }
+--- request
+GET /t
+--- response_body
+query argument name must be a string
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: service.request.clear_query_arg() errors if query argument name is not a string
+--- http_config eval: $t::Util::HttpConfig
+--- config
+    location = /t {
+        content_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            local pok, err = pcall(pdk.service.request.clear_query_arg, 127001)
+            ngx.say(err)
+        }
+    }
+--- request
+GET /t
+--- response_body
+query argument name must be a string
+--- no_error_log
+[error]
+
+
+
+=== TEST 3: service.request.clear_query_arg() clears a given query argument
+--- http_config eval
+qq{
+    $t::Util::HttpConfig
+
+    server {
+        listen unix:$ENV{TEST_NGINX_NXSOCK}/nginx.sock;
+
+        location /t {
+            content_by_lua_block {
+                ngx.say("foo: {" .. tostring(ngx.req.get_uri_args()["foo"]) .. "}")
+            }
+        }
+    }
+}
+--- config
+    location = /t {
+
+        access_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            pdk.service.request.clear_query_arg("foo")
+        }
+
+        proxy_pass http://unix:/$TEST_NGINX_NXSOCK/nginx.sock;
+    }
+--- request
+GET /t?foo=bar
+--- response_body
+foo: {nil}
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: service.request.clear_query_arg() clears multiple given query arguments
+--- http_config eval
+qq{
+    $t::Util::HttpConfig
+
+    server {
+        listen unix:$ENV{TEST_NGINX_NXSOCK}/nginx.sock;
+
+        location /t {
+            content_by_lua_block {
+                ngx.say("foo: {" .. tostring(ngx.req.get_uri_args()["foo"]) .. "}")
+            }
+        }
+    }
+}
+--- config
+    location = /t {
+
+        access_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            pdk.service.request.clear_query_arg("foo")
+        }
+
+        proxy_pass http://unix:/$TEST_NGINX_NXSOCK/nginx.sock;
+    }
+--- request
+GET /t?foo=bar&foo=baz
+--- response_body
+foo: {nil}
+--- no_error_log
+[error]
+
+
+
+=== TEST 5: service.request.clear_query_arg() clears query arguments set via set_query
+--- http_config eval
+qq{
+    $t::Util::HttpConfig
+
+    server {
+        listen unix:$ENV{TEST_NGINX_NXSOCK}/nginx.sock;
+
+        location /t {
+            content_by_lua_block {
+                ngx.say("foo: {" .. tostring(ngx.req.get_uri_args()["foo"]) .. "}")
+            }
+        }
+    }
+}
+--- config
+    location = /t {
+
+        access_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            pdk.service.request.set_query({ foo = "bar" })
+            pdk.service.request.clear_query_arg("foo")
+        }
+
+        proxy_pass http://unix:/$TEST_NGINX_NXSOCK/nginx.sock;
+    }
+--- request
+GET /t
+--- response_body
+foo: {nil}
+--- no_error_log
+[error]
+
+
+
+=== TEST 6: service.request.clear_query_arg() clears query arguments set via set_raw_query
+--- http_config eval
+qq{
+    $t::Util::HttpConfig
+
+    server {
+        listen unix:$ENV{TEST_NGINX_NXSOCK}/nginx.sock;
+
+        location /t {
+            content_by_lua_block {
+                ngx.say("foo: {" .. tostring(ngx.req.get_uri_args()["foo"]) .. "}")
+            }
+        }
+    }
+}
+--- config
+    location = /t {
+
+        access_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            pdk.service.request.set_raw_query("foo=bar")
+            pdk.service.request.clear_query_arg("foo")
+        }
+
+        proxy_pass http://unix:/$TEST_NGINX_NXSOCK/nginx.sock;
+    }
+--- request
+GET /t
+--- response_body
+foo: {nil}
+--- no_error_log
+[error]
+
+
+
+=== TEST 7: service.request.clear_query_arg() retains the order of query arguments
+--- http_config eval
+qq{
+    $t::Util::HttpConfig
+
+    server {
+        listen unix:$ENV{TEST_NGINX_NXSOCK}/nginx.sock;
+
+        location /t {
+            content_by_lua_block {
+                ngx.say("query: " .. tostring(ngx.var.args))
+            }
+        }
+    }
+}
+--- config
+    location = /t {
+
+        access_by_lua_block {
+            local PDK = require "kong.pdk"
+            local pdk = PDK.new()
+
+            pdk.service.request.clear_query_arg("a")
+        }
+
+        proxy_pass http://unix:/$TEST_NGINX_NXSOCK/nginx.sock;
+    }
+--- request
+GET /t?a=0&d=1&a=2&c=3&a=4&b=5&a=6&d=7&a=8
+--- response_body
+query: d=1&c=3&b=5&d=7
+--- no_error_log
+[error]