Skip to content

fix(graphql): reject non-object JSON request bodies - #14068

Open
lordcheng10 wants to merge 3 commits into
apache:masterfrom
lordcheng10:fix/graphql-json-scalar-body
Open

lordcheng10 wants to merge 3 commits into
apache:masterfrom
lordcheng10:fix/graphql-json-scalar-body

Conversation

@lordcheng10

Copy link
Copy Markdown
Contributor

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:

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

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list 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.

graphql-limit-count and graphql-proxy-cache decode the POST body and index it without checking its type. cjson decodes a scalar body such as 123 or true successfully, so the next res[query] indexes a number or boolean and raises an uncaught Lua error, answering 500 instead of the 400 used for other invalid GraphQL payloads.

Reject any decoded body that is not a table and add regression tests for both plugins.
@lordcheng10

Copy link
Copy Markdown
Contributor Author

PTAL, thanks! @AlinsRan @Baoyuantop @nic-6443

@AlinsRan this touches the GraphQL request parsing you recently worked on; the change is a type guard around the decoded JSON body in both graphql-limit-count and graphql-proxy-cache.

@lordcheng10

Copy link
Copy Markdown
Contributor Author

CI note: all 9 failing jobs stop in the dependency-install step before any test runs, with:

Error: Unable to locate package libpcre3
Error: Package 'libpcre3-dev' has no installation candidate

This is caused by ubuntu-latest now resolving to Ubuntu 26.04, where libpcre3-dev no longer exists. The same jobs fail on current master (run 39878888512), so it is not specific to this PR; the GraphQL tests never execute until the workflow packages are updated.

@lordcheng10

Copy link
Copy Markdown
Contributor Author

The only failing CI shard stopped during ci-env-up because host port 43000 was already in use, before the GraphQL tests ran. This is a runner/container networking conflict unrelated to this patch. I attempted to rerun the failed job, but GitHub rejected it because external contributors do not have Actions rerun permission on apache/apisix; a maintainer rerun is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: graphql plugins return 500 on non-object JSON bodies instead of 400

1 participant