silverstripe / silverstripe-graphql

Serves Silverstripe data as GraphQL representations
BSD 3-Clause "New" or "Revised" License
52 stars 61 forks source link

Should/does configuring persisted queries enforce query whitelisting? #545

Open burleight opened 1 year ago

burleight commented 1 year ago
namespace SilverStripe\GraphQL;

// etc

class Controller extends BaseController {

// etc

    /**
     * Parse query and variables from the given request
     *
     * @throws LogicException
     */
    protected function getRequestQueryVariables(HTTPRequest $request): array
    {
        $contentType = $request->getHeader('content-type');
        $isJson = preg_match('#^application/json\b#', $contentType ?? '');
        if ($isJson) {
            $rawBody = $request->getBody();
            $data = json_decode($rawBody ?: '', true);
            $query = isset($data['query']) ? $data['query'] : null;
            $variables = isset($data['variables']) ? (array)$data['variables'] : null;
        } else {
            /** @var RequestProcessor $persistedProcessor  */
            $persistedProcessor = Injector::inst()->get(RequestProcessor::class);
            list($query, $variables) = $persistedProcessor->getRequestQueryVariables($request);
        }

        return [$query, $variables];
    }

Does this allows arbitrary queries even if a RequestProcessor is enabled?

If so, this may be OK for performance reasons, but could be a surprise to anyone that configures it for security reasons after they read this in the documentation:

it allows you to whitelist only specific query IDs, and block all other ad-hoc, potentially malicious queries, which adds an extra layer of security to your API, particularly if it's public.

Perhaps the documentation could spell out that additional steps are needed to enable query whitelisting, or the code could be rewritten so that it is easier to configure whitelisting.

The first part of getRequestQueryVariables could be refactored into a new implementation of RequestProcessor, for example:

class RequestBodyRequestProcessor implements RequestProcessor {
  public function getRequestQueryVariables(HTTPRequest $request): array 
  {
    $contentType = $request->getHeader('content-type');
        $isJson = preg_match('#^application/json\b#', $contentType ?? '');
        if ($isJson) {
            $rawBody = $request->getBody();
            $data = json_decode($rawBody ?: '', true);
            $query = isset($data['query']) ? $data['query'] : null;
            $variables = isset($data['variables']) ? (array)$data['variables'] : null;
        }
        return [$query, $variables];
  }
}

Make this the default implementation of RequestProcessor, then simplify getRequestQueryVariables:

protected function getRequestQueryVariables(HTTPRequest $request): array
    {
      /** @var RequestProcessor $persistedProcessor  */
      $persistedProcessor = Injector::inst()->get(RequestProcessor::class);
      return $persistedProcessor->getRequestQueryVariables($request);
    }

Then, to enable query whitelisting, developers will have to inject RequestIDProcessor in place of RequestBodyRequestProcessor