From ffcb6f8868466c4066533dc1108018b554596f37 Mon Sep 17 00:00:00 2001 From: David Chanin Date: Thu, 10 Oct 2019 10:27:16 +0100 Subject: [PATCH 1/2] fix: fixing bug where responses don't get rewritten with pretty-printing enabled --- src/index.ts | 42 ++++++++++++++++++++++++++++++++++++-- src/types.d.ts | 4 ++++ test/index.test.ts | 51 ++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 93 insertions(+), 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index f5b3351..cb13b8d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -10,7 +10,8 @@ interface RewriterMiddlewareOpts { const rewriteResJson = (res: Response) => { const originalJsonFunc = res.json.bind(res); res.json = function(body: any) { - if (!this.req || !this.req._rewriteHandler) return originalJsonFunc(body); + if (!this.req || !this.req._rewriteHandler || this._isRewritten) return originalJsonFunc(body); + this._isRewritten = true; const rewriteHandler = this.req._rewriteHandler; if (typeof body === 'object' && !(body instanceof Buffer) && body.data) { const newResponseData = rewriteHandler.rewriteResponse(body.data); @@ -21,6 +22,43 @@ const rewriteResJson = (res: Response) => { }; }; +const rewriteResRaw = (res: Response) => { + const originalEndFunc = res.end.bind(res); + res.end = function(body: any) { + if (!this.req || !this.req._rewriteHandler || this._isRewritten || this.headersSent) { + return originalEndFunc(body); + } + this._isRewritten = true; + const existingHeaders = this.getHeaders(); + const isJsonContent = existingHeaders['content-type'] === 'application/json; charset=utf-8'; + const rewriteHandler = this.req._rewriteHandler; + if (isJsonContent && body instanceof Buffer) { + try { + const bodyJson = JSON.parse(body.toString('utf8')); + if (bodyJson.data) { + const newResponseData = rewriteHandler.rewriteResponse(bodyJson.data); + const newResBodyJson = { ...bodyJson, data: newResponseData }; + // assume this was pretty-printed if we're here and not in the res.json handler + const newResBodyStr = JSON.stringify(newResBodyJson, null, 2); + const newResChunk = Buffer.from(newResBodyStr, 'utf8'); + this.setHeader('Content-Length', String(newResChunk.length)); + return originalEndFunc(newResChunk); + } + } catch (err) { + // if we can't decode the response as json, just forward it along + return originalEndFunc(body); + } + } + return originalEndFunc(body); + }; +}; + +const rewriteRes = (res: Response) => { + rewriteResJson(res); + // if res.json isn't available, or pretty-printing is enabled, express-graphql uses raw res.end() + rewriteResRaw(res); +}; + const graphqlRewriterMiddleware = ({ rewriters, ignoreParsingErrors = true @@ -46,7 +84,7 @@ const graphqlRewriterMiddleware = ({ req.body = newBody; } req._rewriteHandler = rewriteHandler; - rewriteResJson(res); + rewriteRes(res); } catch (err) { if (!ignoreParsingErrors) return next(err); } diff --git a/src/types.d.ts b/src/types.d.ts index ec01db1..874e92a 100644 --- a/src/types.d.ts +++ b/src/types.d.ts @@ -3,4 +3,8 @@ declare namespace Express { interface Request { _rewriteHandler?: import('graphql-query-rewriter').RewriteHandler; } + + interface Response { + _isRewritten?: boolean; + } } diff --git a/test/index.test.ts b/test/index.test.ts index 19ce5c8..66a8969 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -166,7 +166,7 @@ describe('middleware test', () => { }); }); - const setupMutationApp = () => { + const setupMutationApp = (extraExpressGraphqlOpts: any = {}) => { const app = express(); app.use( @@ -190,7 +190,8 @@ describe('middleware test', () => { '/graphql', graphqlHTTP({ schema, - rootValue + rootValue, + ...extraExpressGraphqlOpts }) ); return app; @@ -401,6 +402,52 @@ describe('middleware test', () => { expect(invalidQueryRes.body).toEqual('jimmy'); }); + it('is able to rewriter responses with pretty printing enabled on express-graphql', async () => { + const app = setupMutationApp({ pretty: true }); + // in the past, we didn't use input or output types correctly + // so we need to rewrite the query to this old query will work still + const deprecatedQuery = ` + mutation makePokemonWithWrongType($name: String!) { + makePokemon(name: $name) { + id + name + } + } + `; + + const deprecatedRes = await request(app) + .post('/graphql') + .send({ query: deprecatedQuery, variables: { name: 'Squirtle' } }); + expect(deprecatedRes.body.errors).toBe(undefined); + expect(deprecatedRes.body.data.makePokemon).toEqual({ + id: '17', + name: 'Squirtle' + }); + + // the new version of the query should still work with no problem though + const newQuery = ` + mutation makePokemon($input: MakePokemonInput!) { + makePokemon(input: $input) { + pokemon { + id + name + } + } + } + `; + + const newRes = await request(app) + .post('/graphql') + .send({ query: newQuery, variables: { input: { name: 'Squirtle' } } }); + expect(newRes.body.errors).toBe(undefined); + expect(newRes.body.data.makePokemon).toEqual({ + pokemon: { + id: '17', + name: 'Squirtle' + } + }); + }); + it('throws on invalid graphql if ignoreParsingErrors === false', async () => { const app = express(); From 61056cc0258ca7b732f79c27f55ce840a05be67b Mon Sep 17 00:00:00 2001 From: David Chanin Date: Thu, 10 Oct 2019 11:03:27 +0100 Subject: [PATCH 2/2] adding another parsing test and extra guard --- src/index.ts | 2 +- test/index.test.ts | 56 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index cb13b8d..6d9ffa0 100644 --- a/src/index.ts +++ b/src/index.ts @@ -35,7 +35,7 @@ const rewriteResRaw = (res: Response) => { if (isJsonContent && body instanceof Buffer) { try { const bodyJson = JSON.parse(body.toString('utf8')); - if (bodyJson.data) { + if (bodyJson && bodyJson.data) { const newResponseData = rewriteHandler.rewriteResponse(bodyJson.data); const newResBodyJson = { ...bodyJson, data: newResponseData }; // assume this was pretty-printed if we're here and not in the res.json handler diff --git a/test/index.test.ts b/test/index.test.ts index 66a8969..b724e3a 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -363,7 +363,7 @@ describe('middleware test', () => { expect(invalidQueryRes.body.data).toEqual({ random: true }); }); - it('ignores invalid json responses', async () => { + it('ignores invalid json responses sent via response.json()', async () => { const app = express(); app.use( @@ -402,6 +402,60 @@ describe('middleware test', () => { expect(invalidQueryRes.body).toEqual('jimmy'); }); + it('ignores invalid json responses sent via response.end()', async () => { + const app = express(); + + app.use( + '/graphql', + graphqlRewriterMiddleware({ + rewriters: [ + new FieldArgsToInputTypeRewriter({ + fieldName: 'makePokemon', + argNames: ['name'] + }), + new NestFieldOutputsRewriter({ + fieldName: 'makePokemon', + newOutputName: 'pokemon', + outputsToNest: ['id', 'name'] + }) + ] + }) + ); + + app.use('/graphql', (req, res) => { + const messedUpRes = Buffer.from('jisdhfiods{{{{', 'utf8'); + res.setHeader('Content-Type', 'application/json; charset=utf-8'); + res.setHeader('Content-Length', String(messedUpRes.length)); + res.end(messedUpRes); + }); + + const deprecatedQuery = ` + mutation { + makePokemon(name: "Squirtle") { + id + name + } + } + `; + + const invalidQueryRes = await request(app) + .post('/graphql') + .send({ query: deprecatedQuery }) + // disable supertest json parsing + .buffer(true) + .parse((res, cb) => { + let data = Buffer.from(''); + res.on('data', chunk => { + data = Buffer.concat([data, chunk]); + }); + res.on('end', () => { + cb(null, data.toString()); + }); + }); + + expect(invalidQueryRes.body).toEqual('jisdhfiods{{{{'); + }); + it('is able to rewriter responses with pretty printing enabled on express-graphql', async () => { const app = setupMutationApp({ pretty: true }); // in the past, we didn't use input or output types correctly