lordcheng10 opened a new pull request, #14068:
URL: https://github.com/apache/apisix/pull/14068
### Description
Fix a crash in `graphql-limit-count` and `graphql-proxy-cache`: a POST body
that is valid JSON but not a JSON object (for example `123` or `true`) caused
an uncaught Lua error and HTTP 500 instead of the 400 used for other invalid
GraphQL payloads.
Both plugins decode the request body and immediately index it:
```lua
local res, err = core.json.decode(body)
if not res then
return false, "invalid graphql request, " .. err
end
if not res[GRAPHQL_REQ_QUERY] then
```
`cjson` decodes any JSON document, not only objects. A scalar body decodes
successfully to a Lua number, boolean or string, so `res[GRAPHQL_REQ_QUERY]`
indexes a scalar and raises `attempt to index local 'res' (a number value)`
inside `check_graphql_request.POST`, which runs during the access phase. The
body is fully client controlled, so any request to a route with either plugin
enabled can trigger it.
Fix: reject a decoded body whose type is not `table`, returning the same 400
shape as the existing "missing query field" path:
```
invalid graphql request, json body is not an object
```
`null` is covered as well: `graphql-limit-count` decodes it to
`cjson.null` (a non-table lightuserdata) and `graphql-proxy-cache` uses
`null_as_nil`, where it is already caught by the existing `if not res`
branch.
#### Which issue(s) this PR fixes:
Fixes #14066
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not,
please discuss on the [APISIX mailing
list](https://github.com/apache/apisix/tree/master#community) first)
### Additional notes
No documentation update is needed: the configuration schema, the plugin
behaviour for valid GraphQL requests and the error messages for the existing
invalid-body cases are unchanged. Only bodies that previously crashed now
get a
deterministic 400.
Regression tests:
- `t/plugin/graphql-limit-count.t` TEST 31 sends `123` as the JSON body and
expects 400 with `invalid graphql request, json body is not an object`;
- `t/plugin/graphql-proxy-cache/graphql.t` TEST 22 does the same with `true`.
Reviewer note on verification: this environment has no OpenResty, so the two
test files could not be executed locally. The failure path is deterministic:
`cjson.decode("123")` returns the number `123` with no error, and the
following
`res[GRAPHQL_REQ_QUERY]` indexes it. With the added type check the request
returns the 400 branch instead. CI will run both test files.
--
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]
