diff --git a/changelog/unreleased/kong/feat-pdk-clear-query-arg.yml b/changelog/unreleased/kong/feat-pdk-clear-query-arg.yml new file mode 100644 index 0000000000..c7de562047 --- /dev/null +++ b/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 diff --git a/changelog/unreleased/kong/fix-key-auth-retain-query-order.yml b/changelog/unreleased/kong/fix-key-auth-retain-query-order.yml new file mode 100644 index 0000000000..813856f827 --- /dev/null +++ b/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 diff --git a/kong/pdk/service/request.lua b/kong/pdk/service/request.lua index 28fab489e6..24fd9b15d9 100644 --- a/kong/pdk/service/request.lua +++ b/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. diff --git a/kong/plugins/key-auth/handler.lua b/kong/plugins/key-auth/handler.lua index 73902e2e3b..89786aa752 100644 --- a/kong/plugins/key-auth/handler.lua +++ b/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 diff --git a/spec/03-plugins/09-key-auth/02-access_spec.lua b/spec/03-plugins/09-key-auth/02-access_spec.lua index 6c63dbde3e..a1a7b3a925 100644 --- a/spec/03-plugins/09-key-auth/02-access_spec.lua +++ b/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() diff --git a/t/01-pdk/06-service-request/00-phase_checks.t b/t/01-pdk/06-service-request/00-phase_checks.t index d459338a9b..6d87c75b46 100644 --- a/t/01-pdk/06-service-request/00-phase_checks.t +++ b/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" }, diff --git a/t/01-pdk/06-service-request/14-clear_query_arg.t b/t/01-pdk/06-service-request/14-clear_query_arg.t new file mode 100644 index 0000000000..88b2bd610d --- /dev/null +++ b/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]