From 016cc3e08a4e5f6d85944263e5404d0a913111d6 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 11 Jun 2026 17:46:18 +0800 Subject: [PATCH] fix(acl): correctly evaluate multi-match JSONPath labels The ACL plugin read the external_user label field with single-value jp.value(), which returns only the FIRST JSONPath match. For external users that belong to multiple orgs/teams, the allow/deny matching then ran against an incomplete label set: a denied label packed into a later match could be missed (ACL bypass), and a matched value that is a table could be handed to the string parser and crash re_split. Use jp.query() to collect ALL matches. With 0 or 1 match the original jp.value() semantics are preserved. With 2 or more matches, the configured parser is applied to each matched value individually and the parsed sub-values are merged before allow/deny matching, with a type guard so a table sub-value is not passed to the string parser. --- apisix/plugins/acl.lua | 41 ++++++++++++++++++++-- t/plugin/acl.t | 80 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 110 insertions(+), 11 deletions(-) diff --git a/apisix/plugins/acl.lua b/apisix/plugins/acl.lua index ba0c751e95df..3eb94fe3c1bc 100644 --- a/apisix/plugins/acl.lua +++ b/apisix/plugins/acl.lua @@ -225,10 +225,45 @@ function _M.access(conf, ctx) if conf.external_user_label_field_key then label_key = conf.external_user_label_field_key end - local label_value = jp.value(ctx.external_user, conf.external_user_label_field) + -- jp.query() returns nil (not {}) when ctx.external_user is not a table, + -- so guard before taking its length. + local label_values = jp.query(ctx.external_user, conf.external_user_label_field) or {} + local label_value + if #label_values <= 1 then + -- 0 or 1 match: preserve the original jp.value() semantics and let the + -- configured parser run on the single value (a single matched value may + -- itself be a table, which the json/segmented_text parser is expected to + -- reject). label_values[1] is nil when there are no matches. + label_value = label_values[1] + parser = conf.external_user_label_field_parser + sep = conf.external_user_label_field_separator + else + -- Multi-match: apply the configured parser to EACH matched value and + -- merge the results, so allow/deny matching still sees the fully parsed + -- sub-values (e.g. a match like "infra|qa" with segmented_text). Passing + -- the raw list straight to the string parsers would otherwise crash + -- (re_split over a table) or silently skip parsing and miss denied + -- labels. The merged list is already parsed, so parser/sep stay unset. + local cfg_parser = conf.external_user_label_field_parser + local cfg_sep = conf.external_user_label_field_separator + local merged = {} + for _, v in ipairs(label_values) do + local parsed + if cfg_parser and type(v) == "string" then + parsed = extra_values_with_parser(v, cfg_parser, cfg_sep) + else + -- A matched value that is itself a table is already a list of + -- labels; route it through the type-aware path instead of the + -- string parser (segmented_text's re_split cannot handle a table). + parsed = extra_values_without_parser(v) + end + for _, pv in ipairs(parsed) do + merged[#merged + 1] = pv + end + end + label_value = merged + end labels = { [label_key] = label_value } - parser = conf.external_user_label_field_parser - sep = conf.external_user_label_field_separator else return 401, { message = "Missing authentication."} end diff --git a/t/plugin/acl.t b/t/plugin/acl.t index 09de05d2504f..86983713f36e 100644 --- a/t/plugin/acl.t +++ b/t/plugin/acl.t @@ -749,13 +749,15 @@ passed -=== TEST 32: test the ACL extract multiple values from external_user info and the first value can not be expected. -# User may expect the value extracted is "cloud|infra", but it is not. -# Because the values extracted are multiple, we can not expect the value "cloud|infra" is the first. -# This is a normal case, no error_log here. +=== TEST 32: ACL parses every multi-match value, so a packed allowed label is honored +# $.orgs..team matches "cloud|infra" (a string) and {"devops","qa"} (a table). +# Each match is parsed individually: "cloud|infra" -> ["cloud","infra"], the table +# -> ["devops","qa"]. The merged set contains "cloud"/"infra", which match +# allow_labels.team, so the request is allowed. --- request GET /hello ---- error_code: 403 +--- response_body +hello world @@ -1486,7 +1488,69 @@ failed to check the configuration of plugin acl err: invalid external_user_label -=== TEST 54: delete route +=== TEST 54: multi-match JSONPath + parser; denied label hidden inside a parsed value +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "upstream": { + "type": "roundrobin", + "nodes": { + "127.0.0.1:1980": 1 + } + }, + "plugins": { + "serverless-pre-function": { + "functions": [ + "return function(conf, ctx) ctx.external_user = { teams = { { name = \"cloud\" }, { name = \"infra,qa\" } } }; end" + ], + "phase": "access" + }, + "acl": { + "deny_labels": { + "name": ["infra"] + }, + "external_user_label_field": "$..name", + "external_user_label_field_key": "name", + "external_user_label_field_parser": "segmented_text", + "external_user_label_field_separator": ",", + "rejected_code": 403 + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 55: each match is parsed individually before allow/deny matching +# $..name matches ["cloud", "infra,qa"]. The configured segmented_text parser +# must be applied to EACH match, so "infra,qa" expands to ["infra", "qa"] and the +# denied label "infra" is caught -> 403. Skipping the parser on multi-match (or +# only inspecting the first match) would let this request through. +--- request +GET /hello +--- error_code: 403 +--- response_body +{"message":"The consumer is forbidden."} + + + +=== TEST 56: delete route --- config location /t { content_by_lua_block { @@ -1504,7 +1568,7 @@ passed -=== TEST 55: delete jack +=== TEST 57: delete jack --- config location /t { content_by_lua_block { @@ -1522,7 +1586,7 @@ passed -=== TEST 56: delete rose +=== TEST 58: delete rose --- config location /t { content_by_lua_block {