Empty request body parsing error for automatic persisted queries

[oojacoboo]

@oprypkhantc I was giving the new persisted queries a go to test the implementation, since I wasn't able to do so prior. In doing so, I'm running into the following issue.

InvalidArgumentException: Syntax error in body: ""

/srv/www/vendor/thecodingmachine/graphqlite/src/Http/WebonyxGraphqlMiddleware.php:70

This is part of the process method for the middleware, which looks for the request body contents. For reference, the process method is as follows:

    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        if (! $this->isGraphqlRequest($request)) {
            return $handler->handle($request);
        }

        // Let's json deserialize if this is not already done.
        if (empty($request->getParsedBody())) {
            $content = $request->getBody()->getContents();
            $data = json_decode($content, true);

            if ($data === false || json_last_error() !== JSON_ERROR_NONE) {
                throw new InvalidArgumentException(json_last_error_msg() . ' in body: "' . $content . '"'); // @codeCoverageIgnore
            }

            $request = $request->withParsedBody($data);
        }

        $context = $this->config->getContext();
        if ($context instanceof ResetableContextInterface) {
            $context->reset();
        }
        $result = $this->standardServer->executePsrRequest($request);
        //return $this->standardServer->processPsrRequest($request, $this->responseFactory->createResponse(), $this->streamFactory->createStream());

        return $this->getJsonResponse($this->processResult($result), $this->decideHttpCode($result));
    }

Was this overlooked? We need an integration test for persisted queries for sure. Am I missing something here?

[oprypkhantc]

I don't think it's related. This happens way before persisted queries. What is the body and content-type you're sending?

I'd say we need an integration test for the HTTP implementation as a whole. For that, we could do what Apollo Server did and use JS graphql-http server audits: https://github.com/graphql/graphql-http/blob/main/src/audits/server.ts

It'd be nice to just use those as those should be a reference anyway, and those would be true integration tests.

[oprypkhantc]

What's nice about graphql-http's serverAudits is that they fully test the compliance with graphql-over-http, so it might find other problems as well.

[oprypkhantc]

Seems like all GET requests through WebonyxGraphqlMiddleware are broken. It always attempts to parse the body, which isn't present for GET requests. Again, not related to persisted queries, but graphql-http would have covered that.

An improvement for WebonyxGraphqlMiddlewareTest would be sufficient too.

[oprypkhantc]

I've played around with graphq-http audits and sure enough one of the 13 failed tests was this:

● Integration tests › MAY accept application/x-www-form-urlencoded formatted GET requests

    Response status code is not 200: An error occurred: Syntax error in body: ""

[oojacoboo]

@oprypkhantc Yea, the body is empty for a GET request, naturally. And, the WebonyxGraphqlMiddleware processes the request very early. Do you know if webonyx/graphql-php needs/expects a request body? I'm assuming not if persisted queries are supported. The middleware is basically assuming webonyx needs a json decoded request body as the parsedBody on the request object. It might be that all of that logic is unnecessary and can be removed?

[oprypkhantc]

Probably unnecessary. I'm pretty sure webonyx handles parsing the RequestInterface all on their own and they definitely don't require request bodies for GET requests.

It'd be nice to offload the whole HTTP handling thing to webonyx/graphql-php, since most of the parsing is already being done on their side. They don't, however, convert ExecutionResults into responses. They have an open issue for that, so maybe it's worth it to just contribute to webonyx/graphql-php instead of writing tests/fixing things here.

If we could offload this to webonyx then we'd not have to worry about fixing this, or testing it, or writing integration tests for the HTTP layer. For now, just adding a few tests to WebonyxGraphqlMiddlewareTest should be plenty though, to at least cover the most basic use cases such as this.

[oojacoboo]

So, I was able to run our integration tests for graphql with the request body parsing commented out and all tests are passing. That parsing and body validation check seems entirely unnecessary.

I am, however, getting the following error now:

GraphQL Request must include at least one of those two parameters: "query" or "queryId"

When making a GET request with a query key on queryParams property. That's a webonyx error.

I agree with primarily offloading the request handling to webonyx. I do think it's a good idea that we maintain some minimal integration test coverage for requests though. I don't think we need anything too extensive, but having some coverage there is wise.

[oprypkhantc]

When making a GET request with a query key on queryParams property. That's a webonyx error.

The query parameter must be a top-level query parameter, i.e. /graphql?query=query { __typename }

So, I was able to run our integration tests for graphql with the request body parsing commented out and all tests are passing. That parsing and body validation check seems entirely unnecessary.

It seems like at least 15 of graphql-http tests fail when I comment out that body parsing part, so it seems like it is indeed needed :( Here's one of those:

audit('2C94', 'MUST accept POST requests', async () => {
      const res = await fetchFn(await getUrl(opts.url), {
        method: 'POST',
        headers: { 'content-type': 'application/json' },
        body: JSON.stringify({ query: '{ __typename }' }),
      });
      ressert(res).status.toBe(200);
    }),

Response status code is not 200. Response: [400] {"errors":[{"message":"GraphQL Request must include at least one of those two parameters: \"query\" or \"queryId\""}]}

Changing the check to if (empty($request->getParsedBody()) && $request->getMethod() !== 'GET') { helps.

[oojacoboo]

It seems like at least 15 of graphql-http tests fail when I comment out that body parsing part, so it seems like it is indeed needed :(

Yea, so our boot stack is probably setting the parsedBody already, as we have to parse it for some authorization concerns. I don't know why that should be necessary though for webonyx.

The query parameter must be a top-level query parameter, i.e. /graphql?query=query { __typename }

Not sure what you mean by "top-level" here. You mean appended to the actual request URL, as opposed to being a request param on the request object? And why?

[oprypkhantc]

They don't do any body parsing for requests that implement ServerRequestInterface, and all of the requests that go through WebonyxGraphqlMiddleware are ServerRequestInterface, so webonyx literally never parses request bodies.

The implementation I used (laminas/laminas-httphandlerrunner + laminas/laminas-stratigility) doesn't parse the body either, so I believe that's why the fix is there.

Not sure what you mean by "top-level" here. You mean appended to the actual request URL, as opposed to being a request param on the request object? And why?

You mentioned it being under the queryParams property, so I assumed your query params look like this: queryParams[query]=query { __typename } instead of query=query { __typename }. The latter works for me.

[oojacoboo]

You mentioned it being under the queryParams property, so I assumed your query params look like this: queryParams[query]=query { __typename } instead of query=query { __typename }. The latter works for me.

Here is an excerpt of the dumped request object:

0 => Laminas\Diactoros\ServerRequest^ {#8591
        -attributes: []
        -uploadedFiles: []
        -serverParams: []
        -cookieParams: []
        -queryParams: array:4 [
            "query" => """
                query testAutomaticPersistedQueries {\n
                            company {\n
                                id\n
                                legalEntity {\n
                                    companyName\n
                                }\n
                            }\n
                        }
                """
            "variables" => []
            "operationName" => "testAutomaticPersistedQueries"
            "hash" => "cf5def5f484a49ea52906f038c0176e057a5b305b8a7d87638acfa4581c2c362"
        ]
        -parsedBody: null
        -method: "GET"

Ignore the superfulous params. They're just there b/c of our integration test implementation - should be harmless.

[oprypkhantc]

Got it. I'm not sure why you're not getting a 200. Here's the test server I use: https://gist.github.com/oprypkhantc/fe173460cce4ccc0e258749b67989479

If you start that with php -S 0.0.0.0:8085 and do GET http://localhost:8085/graphql?query=query{__typename} you'll get a 200 with this response:

{
  "data": {
    "__typename": "Query"
  }
}

[oojacoboo]

See #427 as possibly related issue.

[oojacoboo]

So, I don't know if this is a Laminas ServerRequestInterface implementation bug, or there is something else I'm missing here. Basically, Laminas' ServerRequest object, which implements the PSR interface and what we use and is mostly used by this lib, has the method withQueryParams, which is just an immutable method, as you'd expect. That's what gives you the output for the queryParams in the dump above.

However, when digging into webonyx, the query params are built like so...

\parse_str(\html_entity_decode($request->getUri()->getQuery()), $queryParams);

As you can see, it's getting them from the Uri, which is initialized at construction and includes the query params properly when appended to the uri string. Either I'm misunderstanding the purpose of queryParams on the ServerRequestInterface, there is a flaw in the PSR design, such that query params are spread out across multiple objects, making them unreliable, or there is a Laminas implementation issue where Laminas should be updating the Uri as well.

It's worth pointing out that Laminas also has Uri::withQuery($query) which takes the query string. ServerRequestInterface::queryParams seem like more like a parsedBody type thing for query param. But I don't see anywhere they're "parsed".

The issue is resolved by using Uri::withQuery. But I still find all this behavior and design awkward.

The additional parsing of the request body logic does appear to be needed based on the StandardServer. It's looking for the pasedBody. The body, does only need to be parsed for POST requests though. I'll push a commit for that.