diff --git a/apisix/core/json.lua b/apisix/core/json.lua index 9418917b355e..a7ce18ae38e5 100644 --- a/apisix/core/json.lua +++ b/apisix/core/json.lua @@ -38,8 +38,6 @@ local rapidjson_null local rapidjson_encode_opts = { sort_keys = true } -cjson.encode_escape_forward_slash(false) -cjson.decode_array_with_array_mt(true) local _M = { version = 0.1, array_mt = cjson.array_mt, diff --git a/apisix/patch.lua b/apisix/patch.lua index f8d504b74a49..31383b770128 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -364,7 +364,40 @@ local function luasocket_tcp() end +-- lua-cjson options are per-instance, and cjson.new() always starts from the +-- compile-time defaults instead of inheriting the singleton's configuration. +-- core/json.lua only enables decode_array_with_array_mt on the `cjson.safe` +-- singleton, so every dependency holding a private instance (lua-resty-session, +-- lua-resty-healthcheck, lua-resty-worker-events, lua-resty-aws, ...) decodes +-- arrays without the array metatable, and an empty array is then re-encoded as +-- an object. Enable the option on both singletons and let every instance created +-- afterwards inherit it. This only changes the default: a library explicitly +-- calling decode_array_with_array_mt(false) on its own instance still wins. +-- +-- encode_escape_forward_slash is set here as well. It looks redundant today, +-- because in the bundled lua-cjson that setter mutates a shared escape table and +-- therefore already applies process-wide, including to instances created before +-- it was called. Upstream has since made it per-instance, so once a runtime +-- upgrade picks that up, every private instance would silently go back to +-- escaping `/`. Setting it here pins the current behaviour instead of relying on +-- that implementation detail. +local function patch_cjson(cjson) + cjson.decode_array_with_array_mt(true) + cjson.encode_escape_forward_slash(false) + + local original_new = cjson.new + cjson.new = function (...) + local instance = original_new(...) + patch_cjson(instance) + return instance + end +end + + function _M.patch() + patch_cjson(require("cjson")) + patch_cjson(require("cjson.safe")) + -- make linter happy -- luacheck: ignore ngx_socket.tcp = function () diff --git a/t/core/json.t b/t/core/json.t index 1409f54f1e7b..68fc3675cfec 100644 --- a/t/core/json.t +++ b/t/core/json.t @@ -193,3 +193,51 @@ b.d: 1 e: text a or default: default top-level null: nil + + + +=== TEST 9: cjson instances created by dependencies keep empty arrays as arrays +--- config + location /t { + content_by_lua_block { + -- a dependency that keeps a private instance, e.g. lua-resty-session + local safe_instance = require("cjson.safe").new() + ngx.say("cjson.safe instance: ", + safe_instance.encode(safe_instance.decode('{"arr":[]}'))) + + local instance = require("cjson").new() + ngx.say("cjson instance: ", instance.encode(instance.decode('{"arr":[]}'))) + + -- the plain singleton is a separate config from cjson.safe + local cjson = require("cjson") + ngx.say("cjson singleton: ", cjson.encode(cjson.decode('{"arr":[]}'))) + } + } +--- response_body +cjson.safe instance: {"arr":[]} +cjson instance: {"arr":[]} +cjson singleton: {"arr":[]} + + + +=== TEST 10: cjson instances created by dependencies do not escape forward slashes +--- config + location /t { + content_by_lua_block { + -- today the bundled lua-cjson keeps a shared escape table, so this + -- already holds; the patch pins it for when the option becomes + -- per-instance upstream + local safe_instance = require("cjson.safe").new() + ngx.say("cjson.safe instance: ", safe_instance.encode({uri = "/a/b"})) + + local instance = require("cjson").new() + ngx.say("cjson instance: ", instance.encode({uri = "/a/b"})) + + local cjson = require("cjson") + ngx.say("cjson singleton: ", cjson.encode({uri = "/a/b"})) + } + } +--- response_body +cjson.safe instance: {"uri":"/a/b"} +cjson instance: {"uri":"/a/b"} +cjson singleton: {"uri":"/a/b"} diff --git a/t/plugin/openid-connect-userinfo-array.t b/t/plugin/openid-connect-userinfo-array.t new file mode 100644 index 000000000000..ce91e1d80e1a --- /dev/null +++ b/t/plugin/openid-connect-userinfo-array.t @@ -0,0 +1,140 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +use t::APISIX 'no_plan'; + +repeat_each(1); +no_long_string(); +no_root_location(); +no_shuffle(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if ((!defined $block->error_log) && (!defined $block->no_error_log)) { + $block->set_value("no_error_log", "[error]"); + } + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } +}); + +run_tests(); + +__DATA__ + +=== TEST 1: set up a route guarded by openid-connect with set_userinfo_header +--- 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, + [[{ + "plugins": { + "openid-connect": { + "client_id": "kbyuFDidLLm280LIwVFiazOqjO3ty8KH", + "client_secret": "60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa", + "discovery": "http://127.0.0.1:1980/.well-known/openid-configuration", + "redirect_uri": "http://127.0.0.1:1984/callback", + "ssl_verify": false, + "use_pkce": false, + "set_userinfo_header": true, + "renew_access_token_on_expiry": false, + "session": { + "secret": "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK" + } + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/uri" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 2: an empty userinfo claim reaches the upstream as an array +--- config + location /t { + content_by_lua_block { + local http = require("resty.http") + local session = require("resty.session") + local core = require("apisix.core") + + -- Forge the session openid-connect writes after a successful login, + -- so the plugin takes the "already authenticated" path and restores + -- the userinfo from the session, exactly as it does on every request + -- after the callback. No IdP is contacted: with a non-expired access + -- token and renew_access_token_on_expiry disabled, openidc never + -- reaches for the discovery document. + local s = session.new({ secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK" }) + s:set("authenticated", true) + s:set("last_authenticated", ngx.time()) + s:set("id_token", { sub = "a UID" }) + s:set("access_token", "fake-access-token") + s:set("access_token_expiration", ngx.time() + 3600) + s:set("user", core.json.decode( + '{"sub":"a UID","name":"Testuser One","roles":[]}')) + + local ok, err = s:save() + if not ok then + ngx.say("failed to save session: ", err) + return + end + + local cookie = ngx.header["Set-Cookie"] + if type(cookie) == "table" then + cookie = cookie[1] + end + ngx.header["Set-Cookie"] = nil + + local httpc = http.new() + local res, err = httpc:request_uri("http://127.0.0.1:1984/uri", { + headers = { Cookie = cookie }, + }) + if not res then + ngx.say("request failed: ", err) + return + end + + -- the upstream echoes back the request headers it received + local encoded = res.body:match("x%-userinfo: ([%w+/=]+)") + if not encoded then + ngx.say("no X-Userinfo reached the upstream") + return + end + + local userinfo = ngx.decode_base64(encoded) + ngx.say(core.json.encode(core.json.decode(userinfo).roles)) + } + } +--- response_body +[]