[Security] get queries did not run through validation

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Changelog
 
 ### VNEXT
+* **Security Fix** GET queries did not run through validation ([@DxCx](https://github.com/DxCx)) on [#424](https://github.com/apollographql/graphql-server/pull/424)
 
 ### v0.8.0
 * Persist `window.location.hash` on URL updates [#386](https://github.com/apollographql/graphql-server/issues/386)

packages/graphql-server-core/src/runQuery.test.ts

@@ -151,20 +151,6 @@ describe('runQuery', () => {
       });
   });
 
-  it('does not run validation if the query is a document', () => {
-      // this would not pass validation, because $base ought to be Int!, not String
-      // what effecively happens is string concatentation, but it's returned as Int
-      const query = parse(`query TestVar($base: String){ testArgumentValue(base: $base) }`);
-      const expected = { testArgumentValue: 15 };
-      return runQuery({
-          schema,
-          query: query,
-          variables: { base: 1 },
-      }).then((res) => {
-          return expect(res.data).to.deep.equal(expected);
-      });
-  });
-
   it('correctly passes in the rootValue', () => {
       const query = `{ testRootValue }`;
       const expected = { testRootValue: 'it also works' };

packages/graphql-server-core/src/runQuery.ts

@@ -94,33 +94,31 @@ function doRunQuery(options: QueryOptions): Promise<ExecutionResult> {
     logFunction({action: LogAction.request, step: LogStep.status, key: 'operationName', data: options.operationName});
 
     // if query is already an AST, don't parse or validate
+    // XXX: This refers the operations-store flow.
     if (typeof options.query === 'string') {
         try {
-            // TODO: time this with log function
             logFunction({action: LogAction.parse, step: LogStep.start});
             documentAST = parse(options.query as string);
             logFunction({action: LogAction.parse, step: LogStep.end});
         } catch (syntaxError) {
             logFunction({action: LogAction.parse, step: LogStep.end});
             return Promise.resolve({ errors: format([syntaxError]) });
         }
-
-        // TODO: time this with log function
-
-        let rules = specifiedRules;
-        if (options.validationRules) {
-          rules = rules.concat(options.validationRules);
-        }
-        logFunction({action: LogAction.validation, step: LogStep.start});
-        const validationErrors = validate(options.schema, documentAST, rules);
-        logFunction({action: LogAction.validation, step: LogStep.end});
-        if (validationErrors.length) {
-            return Promise.resolve({ errors: format(validationErrors) });
-        }
     } else {
         documentAST = options.query as DocumentNode;
     }
 
+    let rules = specifiedRules;
+    if (options.validationRules) {
+      rules = rules.concat(options.validationRules);
+    }
+    logFunction({action: LogAction.validation, step: LogStep.start});
+    const validationErrors = validate(options.schema, documentAST, rules);
+    logFunction({action: LogAction.validation, step: LogStep.end});
+    if (validationErrors.length) {
+      return Promise.resolve({ errors: format(validationErrors) });
+    }
+
     try {
         logFunction({action: LogAction.execute, step: LogStep.start});
         return execute(

packages/graphql-server-express/src/apolloServerHttp.test.ts

@@ -336,14 +336,33 @@ describe(`GraphQL-HTTP (apolloServer) tests for ${version} express`, () => {
       const response = await request(app)
         .post(urlString())
         .send({
-          query: '{test(who: 123)}',
+          query: '{notExists}',
         });
 
       expect(response.status).to.equal(400);
       expect(JSON.parse(response.text)).to.deep.equal({
         errors: [ {
-          message: 'Argument \"who\" has invalid value 123.\nExpected type \"String\", found 123.',
-          locations: [ { line: 1, column: 12 } ],
+          message: 'Cannot query field \"notExists\" on type \"QueryRoot\".',
+          locations: [ { line: 1, column: 2 } ],
+        } ]
+      });
+    });
+
+    it('handles type validation (GET)', async () => {
+      const app = express();
+
+      app.use(urlString(), graphqlExpress({
+        schema: TestSchema
+      }));
+
+      const response = await request(app)
+        .get(urlString({ query: '{notExists}' }))
+
+      expect(response.status).to.equal(400);
+      expect(JSON.parse(response.text)).to.deep.equal({
+        errors: [ {
+          message: 'Cannot query field \"notExists\" on type \"QueryRoot\".',
+          locations: [ { line: 1, column: 2 } ],
         } ]
       });
     });

packages/graphql-server-restify/src/restifyApollo.test.ts

@@ -4,7 +4,6 @@ import { graphiqlRestify, graphqlRestify } from './restifyApollo';
 import testSuite, { schema, CreateAppOptions } from 'graphql-server-integration-testsuite';
 import { expect } from 'chai';
 import { GraphQLOptions } from 'graphql-server-core';
-import 'mocha';
 
 function createApp(options: CreateAppOptions = {}) {
   const server = restify.createServer({

test/tests.js

@@ -9,6 +9,9 @@ require('../packages/graphql-server-express/dist/connectApollo.test');
 require('../packages/graphql-server-hapi/dist/hapiApollo.test');
 (NODE_MAJOR_VERSION >= 6) && require('../packages/graphql-server-micro/dist/microApollo.test');
 (NODE_MAJOR_VERSION >= 7) && require('../packages/graphql-server-koa/dist/koaApollo.test');
-require('../packages/graphql-server-restify/dist/restifyApollo.test');
 require('../packages/graphql-server-lambda/dist/lambdaApollo.test');
 require('../packages/graphql-server-express/dist/apolloServerHttp.test');
+
+// XXX: Running restify last as it breaks http.
+// for more info: https://github.com/restify/node-restify/issues/700
+require('../packages/graphql-server-restify/dist/restifyApollo.test');