AlinsRan commented on PR #13680:
URL: https://github.com/apache/apisix/pull/13680#issuecomment-4955077530
Agreed — your framing is the right one, and it supersedes this PR.
I scoped this as "a lua-resty-session bug" and went after the one library.
It is really a class of bug: lua-cjson options are per-instance,
`core/json.lua` only configures the `cjson.safe` singleton, and every
dependency holding a private instance escapes it. Chasing them one at a time
(fork, pin, re-audit each new dep) is the wrong shape of fix.
I verified your claims against the current deps tree:
```
A) singleton has array_mt set, does .new() inherit it? false
B) does require("cjson") get it too? false (separate
config)
C) unpatched lua-resty-session + the cjson.safe.new wrap:
{"sub":"a UID","roles":[]}
```
(C) is the one that settles it — with the wrap in place, the *stock*
lua-resty-session already round-trips correctly, so the fork buys nothing. And
the private-instance list matches what you found: `resty/session/utils.lua`,
`resty/healthcheck.lua`, `resty/worker/events.lua`, `resty/aws/raw-api/*.lua`.
Ordering is fine too: `init.lua:28` runs `patch()` before any dependency is
loaded, and these instances are created lazily on first use, so the wrapped
factory catches all of them.
Please do send the `patch.lua` PR. I will close this one.
On the trade-off you flagged — worth naming the concrete hazard for the
full-suite run: code that decodes `[]` and then assigns string keys to that
same table. Today it re-encodes as an object; with `array_mt` attached it would
encode as `[]` and silently drop the keys. That is the shape of regression the
suite needs to catch. Anything that only reads (`pairs`, `#`, `next`) is
unaffected.
Two things you may want to pick up:
- `t/plugin/openid-connect-userinfo-array.t` in this PR is a plugin-level
e2e: it forges an authenticated session with the plugin secret, drives a
request through routing → `openid-connect` → upstream, and asserts the empty
`roles` claim arrives in `X-Userinfo` as `[]`. It asserts on the resulting
behavior, not on how it is fixed, so it should carry over to the `patch.lua` PR
unchanged. It fails with `got: {}` today. Feel free to lift it.
- bungle/lua-resty-session#207 is still open and still worth landing on its
own merits, as you said — independent of how APISIX defends itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
