Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
burleight opened this issue Jul 28, 2023 · 0 comments
Open

Comments

@burleight
Copy link

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

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

No branches or pull requests

2 participants