From aef1c7a80bfa4cd30623357ce36a41984f72d014 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Nowak?= Date: Mon, 30 Sep 2024 10:02:39 +0200 Subject: [PATCH 1/3] fix(admin): reject AdminAPI call with empty tags When querying entities that have tags assigned a user can send a request with `tags` parameter. Previously sending `tags=''` (empty string or explicit nil) resulted in 500 error. This commit changes that so that it returns 400 error as empty explicit tags are not allowed KAG-5496 Fix #13591 --- .../kong/fix-admin-api-for-empty-tags.yml | 3 +++ kong/api/endpoints.lua | 13 +++++++++++-- kong/api/routes/consumers.lua | 6 +++++- kong/api/routes/upstreams.lua | 13 +++++++++++-- .../04-admin_api/03-consumers_routes_spec.lua | 10 ++++++++++ .../04-admin_api/07-upstreams_routes_spec.lua | 10 ++++++++++ spec/02-integration/04-admin_api/14-tags_spec.lua | 10 ++++++++++ 7 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 changelog/unreleased/kong/fix-admin-api-for-empty-tags.yml diff --git a/changelog/unreleased/kong/fix-admin-api-for-empty-tags.yml b/changelog/unreleased/kong/fix-admin-api-for-empty-tags.yml new file mode 100644 index 000000000000..4f6f87ea1016 --- /dev/null +++ b/changelog/unreleased/kong/fix-admin-api-for-empty-tags.yml @@ -0,0 +1,3 @@ +message: Fix for querying admin API entities with emtpy tags +type: bugfix +scope: Admin API diff --git a/kong/api/endpoints.lua b/kong/api/endpoints.lua index 3129fe6ddbe7..0085d8b63ec7 100644 --- a/kong/api/endpoints.lua +++ b/kong/api/endpoints.lua @@ -137,7 +137,7 @@ local function handle_error(err_t) end -local function extract_options(args, schema, context) +local function extract_options(db, args, schema, context) local options = { nulls = true, pagination = { @@ -156,6 +156,11 @@ local function extract_options(args, schema, context) end if schema.fields.tags and args.tags ~= nil and context == "page" then + if args.tags == null then + local error_message = "tags cannot be null (or empty string)" + return nil, error_message, db[schema.name].errors:invalid_options({tags = error_message}) + end + local tags = args.tags if type(tags) == "table" then tags = tags[1] @@ -207,7 +212,11 @@ local function query_entity(context, self, db, schema, method) args = self.args.uri end - local opts = extract_options(args, schema, context) + local opts, err, err_t = extract_options(db, args, schema, context) + if err then + return nil, err, err_t + end + local schema_name = schema.name local dao = db[schema_name] diff --git a/kong/api/routes/consumers.lua b/kong/api/routes/consumers.lua index 470281f60989..fc1c595e22f5 100644 --- a/kong/api/routes/consumers.lua +++ b/kong/api/routes/consumers.lua @@ -20,7 +20,11 @@ return { -- Search by custom_id: /consumers?custom_id=xxx if custom_id then - local opts = endpoints.extract_options(args, db.consumers.schema, "select") + local opts, _, err_t = endpoints.extract_options(db, args, db.consumers.schema, "select") + if err_t then + return endpoints.handle_error(err_t) + end + local consumer, _, err_t = db.consumers:select_by_custom_id(custom_id, opts) if err_t then return endpoints.handle_error(err_t) diff --git a/kong/api/routes/upstreams.lua b/kong/api/routes/upstreams.lua index 614bef1721dc..df97aa230fe4 100644 --- a/kong/api/routes/upstreams.lua +++ b/kong/api/routes/upstreams.lua @@ -25,7 +25,12 @@ local function set_target_health(self, db, is_healthy) target, _, err_t = endpoints.select_entity(self, db, db.targets.schema) else - local opts = endpoints.extract_options(self.args.uri, db.targets.schema, "select") + local opts + opts, _, err_t = endpoints.extract_options(db, self.args.uri, db.targets.schema, "select") + if err_t then + return endpoints.handle_error(err_t) + end + local upstream_pk = db.upstreams.schema:extract_pk_values(upstream) local filter = { target = unescape_uri(self.params.targets) } target, _, err_t = db.targets:select_by_upstream_filter(upstream_pk, filter, opts) @@ -94,7 +99,11 @@ local function target_endpoint(self, db, callback) target, _, err_t = endpoints.select_entity(self, db, db.targets.schema) else - local opts = endpoints.extract_options(self.args.uri, db.targets.schema, "select") + local opts + opts, _, err_t = endpoints.extract_options(db, self.args.uri, db.targets.schema, "select") + if err_t then + return endpoints.handle_error(err_t) + end local upstream_pk = db.upstreams.schema:extract_pk_values(upstream) local filter = { target = unescape_uri(self.params.targets) } target, _, err_t = db.targets:select_by_upstream_filter(upstream_pk, filter, opts) diff --git a/spec/02-integration/04-admin_api/03-consumers_routes_spec.lua b/spec/02-integration/04-admin_api/03-consumers_routes_spec.lua index 06fbb8968899..cd40040fb653 100644 --- a/spec/02-integration/04-admin_api/03-consumers_routes_spec.lua +++ b/spec/02-integration/04-admin_api/03-consumers_routes_spec.lua @@ -282,6 +282,16 @@ describe("Admin API (#" .. strategy .. "): ", function() local body = assert.response(res).has.status(200) assert.match('"data":%[%]', body) end) + it("returns bad request for empty tags", function() + local res = assert(client:send { + method = "GET", + path = "/consumers", + query = { tags = ngx.null} + }) + res = assert.res_status(400, res) + local json = cjson.decode(res) + assert.same("invalid option (tags: tags cannot be null (or empty string))", json.message) + end) end) it("returns 405 on invalid method", function() local methods = {"DELETE", "PATCH"} diff --git a/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua b/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua index 69f7bb52ea74..9ea586fcdbab 100644 --- a/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua +++ b/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua @@ -637,6 +637,16 @@ describe("Admin API: #" .. strategy, function() }) assert.res_status(200, res) end) + it("returns bad request for empty tags", function() + local res = assert(client:send { + method = "GET", + path = "/upstreams", + query = { tags = ngx.null} + }) + res = assert.res_status(400, res) + local json = cjson.decode(res) + assert.same("invalid option (tags: tags cannot be null (or empty string))", json.message) + end) describe("empty results", function() lazy_setup(function() diff --git a/spec/02-integration/04-admin_api/14-tags_spec.lua b/spec/02-integration/04-admin_api/14-tags_spec.lua index 37586104683d..5fce2adfda9d 100644 --- a/spec/02-integration/04-admin_api/14-tags_spec.lua +++ b/spec/02-integration/04-admin_api/14-tags_spec.lua @@ -73,6 +73,16 @@ describe("Admin API - tags", function() end end) + it("filter by emtpy tag", function() + local res = assert(client:send { + method = "GET", + path = "/consumers?tags=" + }) + local body = assert.res_status(400, res) + local json = cjson.decode(body) + assert.same("invalid option (tags: tags cannot be null (or empty string))", json.message) + end) + it("filter by multiple tags with AND", function() local res = assert(client:send { method = "GET", From d3cac86d0b1010d7748f5382743e99f178f13f68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Nowak?= Date: Tue, 29 Oct 2024 16:47:45 +0100 Subject: [PATCH 2/3] fixup! fix(admin): reject AdminAPI call with empty tags PR Review fixes - remove typo emtpy -> empty --- changelog/unreleased/kong/fix-admin-api-for-empty-tags.yml | 2 +- spec/02-integration/04-admin_api/14-tags_spec.lua | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog/unreleased/kong/fix-admin-api-for-empty-tags.yml b/changelog/unreleased/kong/fix-admin-api-for-empty-tags.yml index 4f6f87ea1016..9d96d0738467 100644 --- a/changelog/unreleased/kong/fix-admin-api-for-empty-tags.yml +++ b/changelog/unreleased/kong/fix-admin-api-for-empty-tags.yml @@ -1,3 +1,3 @@ -message: Fix for querying admin API entities with emtpy tags +message: Fix for querying admin API entities with empty tags type: bugfix scope: Admin API diff --git a/spec/02-integration/04-admin_api/14-tags_spec.lua b/spec/02-integration/04-admin_api/14-tags_spec.lua index 5fce2adfda9d..eab1db7536d0 100644 --- a/spec/02-integration/04-admin_api/14-tags_spec.lua +++ b/spec/02-integration/04-admin_api/14-tags_spec.lua @@ -73,7 +73,7 @@ describe("Admin API - tags", function() end end) - it("filter by emtpy tag", function() + it("filter by empty tag", function() local res = assert(client:send { method = "GET", path = "/consumers?tags=" From 3b5af359cda51f1f09d72a674cfa38dc3349b554 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Nowak?= Date: Wed, 30 Oct 2024 17:23:13 +0100 Subject: [PATCH 3/3] fixup! fix(admin): reject AdminAPI call with empty tags PR Review fixes - improve error message: remove info about empty string --- kong/api/endpoints.lua | 4 ++-- .../04-admin_api/03-consumers_routes_spec.lua | 2 +- .../04-admin_api/07-upstreams_routes_spec.lua | 2 +- spec/02-integration/04-admin_api/14-tags_spec.lua | 12 +++++++++++- 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/kong/api/endpoints.lua b/kong/api/endpoints.lua index 0085d8b63ec7..1a0b41bbd3ca 100644 --- a/kong/api/endpoints.lua +++ b/kong/api/endpoints.lua @@ -156,8 +156,8 @@ local function extract_options(db, args, schema, context) end if schema.fields.tags and args.tags ~= nil and context == "page" then - if args.tags == null then - local error_message = "tags cannot be null (or empty string)" + if args.tags == null or #args.tags == 0 then + local error_message = "cannot be null" return nil, error_message, db[schema.name].errors:invalid_options({tags = error_message}) end diff --git a/spec/02-integration/04-admin_api/03-consumers_routes_spec.lua b/spec/02-integration/04-admin_api/03-consumers_routes_spec.lua index cd40040fb653..fc7c87b48e05 100644 --- a/spec/02-integration/04-admin_api/03-consumers_routes_spec.lua +++ b/spec/02-integration/04-admin_api/03-consumers_routes_spec.lua @@ -290,7 +290,7 @@ describe("Admin API (#" .. strategy .. "): ", function() }) res = assert.res_status(400, res) local json = cjson.decode(res) - assert.same("invalid option (tags: tags cannot be null (or empty string))", json.message) + assert.same("invalid option (tags: cannot be null)", json.message) end) end) it("returns 405 on invalid method", function() diff --git a/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua b/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua index 9ea586fcdbab..8012e5d4d849 100644 --- a/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua +++ b/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua @@ -645,7 +645,7 @@ describe("Admin API: #" .. strategy, function() }) res = assert.res_status(400, res) local json = cjson.decode(res) - assert.same("invalid option (tags: tags cannot be null (or empty string))", json.message) + assert.same("invalid option (tags: cannot be null)", json.message) end) describe("empty results", function() diff --git a/spec/02-integration/04-admin_api/14-tags_spec.lua b/spec/02-integration/04-admin_api/14-tags_spec.lua index eab1db7536d0..99bb91d9adca 100644 --- a/spec/02-integration/04-admin_api/14-tags_spec.lua +++ b/spec/02-integration/04-admin_api/14-tags_spec.lua @@ -80,7 +80,17 @@ describe("Admin API - tags", function() }) local body = assert.res_status(400, res) local json = cjson.decode(body) - assert.same("invalid option (tags: tags cannot be null (or empty string))", json.message) + assert.same("invalid option (tags: cannot be null)", json.message) + end) + + it("filter by empty string tag", function() + local res = assert(client:send { + method = "GET", + path = "/consumers?tags=''" + }) + local body = assert.res_status(200, res) + local json = cjson.decode(body) + assert.equals(0, #json.data) end) it("filter by multiple tags with AND", function()