Skip to content

Commit

Permalink
Avoid TypeError when querystring is present, but query missing
Browse files Browse the repository at this point in the history
The express-graphql reference implementation [provides a check]
(https://github.com/graphql/express-graphql/blob/2e27a7335875f23ae5cccc97a9d6926970fb08c3/src/index.js#L201-L208)
to protect against cases where a GET request is made that does not
have a `query` parameter where the GraphQL query would be present.

Without this guard, graphql-js's `parse` will return `undefined`
for the `DocumentNode` since it has no document to read. Subsequently
passing this to `isQueryOperation` results in a `TypeError`, because
we are providing `undefined` where `getOperationAst` [expects to get
a DocumentNode](https://github.com/graphql/graphql-js/blob/5fe39262a308df944a87cc85b225228e7556aaa4/src/utilities/getOperationAST.js#L19).

A new test file is added for `runHttpQuery`, as one previously did
not seem to exist.
  • Loading branch information
steverice committed Apr 13, 2018
1 parent 6306f82 commit 7ad44d1
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ All of the packages in the `apollo-server` repo are released with the same versi
### vNEXT

* `apollo-server-adonis`: The `Content-type` of an operation response will now be correctly set to `application/json`.
* `apollo-server-core`: Fix `TypeError` on GET requests with missing `query` parameter

### v1.3.4

Expand Down
53 changes: 53 additions & 0 deletions packages/apollo-server-core/src/runHttpQuery.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/* tslint:disable:no-unused-expression */
import { expect } from 'chai';
import { stub } from 'sinon';
import 'mocha';

import {
GraphQLSchema,
GraphQLObjectType,
GraphQLString,
GraphQLInt,
} from 'graphql';

import { runHttpQuery, HttpQueryError } from './runHttpQuery';

const queryType = new GraphQLObjectType({
name: 'QueryType',
fields: {
testString: {
type: GraphQLString,
resolve() {
return 'it works';
},
},
},
});

const schema = new GraphQLSchema({
query: queryType,
});

describe('runHttpQuery', () => {
describe('handling a GET query', () => {
const mockQueryRequest = {
method: 'GET',
query: {
query: '{ testString }',
},
options: {
schema,
},
};

it('raises a 400 error if the query is missing', () => {
const noQueryRequest = Object.assign({}, mockQueryRequest, {
query: 'foo',
});
return runHttpQuery([], noQueryRequest).catch(err => {
expect(err.statusCode).to.equal(400);
expect(err.message).to.equal('Must provide query string.');
});
});
});
});
2 changes: 2 additions & 0 deletions packages/apollo-server-core/src/runHttpQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ export async function runHttpQuery(
if (typeof query === 'string') {
// preparse the query incase of GET so we can assert the operation.
query = parse(query);
} else if (!query) {
throw new HttpQueryError(400, 'Must provide query string.');
}

if (!isQueryOperation(query, requestParams.operationName)) {
Expand Down
1 change: 1 addition & 0 deletions test/tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const NODE_MAJOR_REVISION = parseInt(NODE_VERSION[1]);
process.env.NODE_ENV = 'test';

require('../packages/apollo-server-core/dist/runQuery.test.js');
require('../packages/apollo-server-core/dist/runHttpQuery.test.js');
require('../packages/apollo-server-module-operation-store/dist/operationStore.test');
NODE_MAJOR_VERSION >= 7 &&
require('../packages/apollo-server-adonis/dist/adonisApollo.test');
Expand Down

0 comments on commit 7ad44d1

Please sign in to comment.