Repository: mercurius-js/mercurius
Pull request #653 (2021-11-17 17:30:30)

Polishing/error formatter context
This is an implementation of the idea discussed in issue #616 .
According to types in `index.d.ts` custom error formatter should be always called with request context: https://github.com/mercurius-js/mercurius/blob/master/index.d.ts#L473

In reality, it sometimes was called with context, but sometimes without it.
This PR goal is to standardize this behavior and provide request context every time the error formatter is called.


Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index a14d71f..982977e 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -21,6 +21,6 @@ jobs:
     needs: test
     runs-on: ubuntu-latest
     steps:
-      - uses: fastify/github-action-merge-dependabot@v2.5.0
+      - uses: fastify/github-action-merge-dependabot@v2.6.0
         with:
           github-token: ${{secrets.GITHUB_TOKEN}}
diff --git a/README.md b/README.md
index 305a1bd..6a01c35 100644
--- a/README.md
+++ b/README.md
@@ -1,3 +1,6 @@
+
+![Mercurius Logo](https://github.com/mercurius-js/graphics/blob/main/mercurius-horizontal.svg)
+
 # mercurius
 
 ![CI workflow](https://github.com/mercurius-js/mercurius/workflows/CI%20workflow/badge.svg)
diff --git a/docs/api/options.md b/docs/api/options.md
index 0c959ee..90a8e96 100644
--- a/docs/api/options.md
+++ b/docs/api/options.md
@@ -49,7 +49,7 @@
 - `prefix`: String. Change the route prefix of the graphql endpoint if enabled.
 - `defineMutation`: Boolean. Add the empty Mutation definition if schema is not defined (Default: `false`).
 - `errorHandler`: `Function`  or `boolean`. Change the default error handler (Default: `true`). _Note: If a custom error handler is defined, it should return the standardized response format according to [GraphQL spec](https://graphql.org/learn/serving-over-http/#response)._
-- `errorFormatter`: `Function`. Change the default error formatter. Allows the status code of the response to be set, and a GraphQL response for the error to be defined. This can be used to format errors for batched queries, which return a successful response overall but individual errors, or to obfuscate or format internal errors. The first argument is the error object, while the second one _might_ be the context if it is available.
+- `errorFormatter`: `Function`. Change the default error formatter. Allows the status code of the response to be set, and a GraphQL response for the error to be defined. This can be used to format errors for batched queries, which return a successful response overall but individual errors, or to obfuscate or format internal errors. The first argument is the error object, while the second one is the context object.
 - `queryDepth`: `Integer`. The maximum depth allowed for a single query. _Note: GraphiQL IDE sends an introspection query when it starts up. This query has a depth of 7 so when the `queryDepth` value is smaller than 7 this query will fail with a `Bad Request` error_
 - `validationRules`: `Function` or `Function[]`. Optional additional validation rules that the queries must satisfy in addition to those defined by the GraphQL specification. When using `Function`, arguments include additional data from graphql request and the return value must be validation rules `Function[]`.
 - `subscription`: Boolean | Object. Enable subscriptions. It uses [mqemitter](https://github.com/mcollina/mqemitter) when it is true and exposes the pubsub interface to `app.graphql.pubsub`. To use a custom emitter set the value to an object containing the emitter.
@@ -66,6 +66,7 @@
     - `service.name`: A unique name for the service. Required.
     - `service.url`: The URL of the service endpoint. It can also be an `Array` of URLs and in which case all the requests will be load balanced throughout the URLs. Required.
     - `service.mandatory`: `Boolean` Marks service as mandatory. If any of the mandatory services are unavailable, gateway will exit with an error. (Default: `false`)
+    - `service.useSecureParse`: `Boolean` Marks if the service response needs to be parsed securely using [secure-json-parse](https://github.com/fastify/secure-json-parse). (Default: `false`)
     - `service.rewriteHeaders`: `Function` A function that gets the original headers as a parameter and returns an object containing values that should be added to the headers
     - `service.initHeaders`: `Function` or `Object` An object or a function that returns the headers sent to the service for the initial \_service SDL query.
     - `service.connections`: The number of clients to create. (Default: `10`)
@@ -553,6 +554,6 @@ To control the status code for the response, the third optional parameter can be
 
 Allows the status code of the response to be set, and a GraphQL response for the error to be defined. 
 
-By default uses the defaultErrorFormatter, but it can be overridden in the [mercurius options](/docs/api/options.md#plugin-options) changing the errorFormatter parameter.
+By default uses the `defaultErrorFormatter`, but it can be overridden in the [mercurius options](/docs/api/options.md#plugin-options) changing the errorFormatter parameter.
 
 **Important**: *using the default formatter, when the error has a data property the response status code will be always 200*
diff --git a/docs/federation.md b/docs/federation.md
index 51f1ef4..c485d96 100644
--- a/docs/federation.md
+++ b/docs/federation.md
@@ -388,3 +388,34 @@ server.listen(3002)
 ```
 
 _Note: The default behavior of `errorHandler` is call `errorFormatter` to send the result. When is provided an `errorHandler` make sure to **call `errorFormatter` manually if needed**._
+
+#### Securely parse service responses in Gateway mode
+
+Gateway service responses can be securely parsed using the `useSecureParse` flag. By default, the target service is considered trusted and thus this flag is set to `false`. If there is a need to securely parse the JSON response from a service, this flag can be set to `true` and it will use the [secure-json-parse](https://github.com/fastify/secure-json-parse) library.
+
+```js
+const Fastify = require('fastify')
+const mercurius = require('mercurius')
+
+const server = Fastify()
+
+server.register(mercurius, {
+  graphiql: true,
+  gateway: {
+    services: [
+      {
+        name: 'user',
+        url: 'http://localhost:3000/graphql',
+        useSecureParse: true
+      },
+      {
+        name: 'company',
+        url: 'http://localhost:3001/graphql'
+      }
+    ]
+  },
+  pollingInterval: 2000
+})
+
+server.listen(3002)
+```
diff --git a/lib/errors.js b/lib/errors.js
index 09abe47..e82c633 100644
--- a/lib/errors.js
+++ b/lib/errors.js
@@ -63,18 +63,13 @@ function toGraphQLError (err) {
 
 function defaultErrorFormatter (err, ctx) {
   let errors = [{ message: err.message }]
-  let log
-
-  if (ctx) {
-    // There is always app if there is a context
-    log = ctx.reply ? ctx.reply.log : ctx.app.log
-  }
+  // There is always app if there is a context
+  const log = ctx.reply ? ctx.reply.log : ctx.app.log
 
   if (err.errors) {
     errors = err.errors.map((error, idx) => {
-      if (log) {
-        log.error({ err: error }, error.message)
-      }
+      log.error({ err: error }, error.message)
+
       // parses, converts & combines errors if they are the result of a federated request
       if (error.message === FEDERATED_ERROR.toString()) {
         return error.extensions.errors.map(err => formatError(toGraphQLError(err)))
diff --git a/lib/gateway/request.js b/lib/gateway/request.js
index 9421bef..b340a06 100644
--- a/lib/gateway/request.js
+++ b/lib/gateway/request.js
@@ -58,7 +58,7 @@ function buildRequest (opts) {
   }
 }
 
-function sendRequest (request, url) {
+function sendRequest (request, url, useSecureParse) {
   return function (opts) {
     return new Promise((resolve, reject) => {
       request({
@@ -89,7 +89,7 @@ function sendRequest (request, url) {
           }
 
           try {
-            const json = sJSON.parse(data.toString())
+            const json = (useSecureParse ? sJSON : JSON).parse(data.toString())
 
             if (json.errors && json.errors.length) {
               // return a `FederatedError` instance to keep `graphql` happy
@@ -102,7 +102,7 @@ function sendRequest (request, url) {
               json
             })
           } catch (e) {
-            reject(e)
+            reject(new FederatedError(e))
           }
         })
       })
diff --git a/lib/gateway/service-map.js b/lib/gateway/service-map.js
index ab53f75..88b13cd 100644
--- a/lib/gateway/service-map.js
+++ b/lib/gateway/service-map.js
@@ -113,13 +113,13 @@ async function buildServiceMap (services, errorHandler) {
   const serviceMap = {}
 
   for (const service of services) {
-    const { name, mandatory = false, initHeaders, ...opts } = service
+    const { name, mandatory = false, initHeaders, useSecureParse = false, ...opts } = service
     const { request, close } = buildRequest(opts)
     const url = new URL(Array.isArray(opts.url) ? opts.url[0] : opts.url)
 
     const serviceConfig = {
       mandatory: mandatory,
-      sendRequest: sendRequest(request, url),
+      sendRequest: sendRequest(request, url, useSecureParse),
       close,
       async refresh () {
         // if this is using a supplied schema refresh is done manually with setSchema
diff --git a/lib/routes.js b/lib/routes.js
index abcc476..7921f86 100644
--- a/lib/routes.js
+++ b/lib/routes.js
@@ -3,6 +3,7 @@
 const { join } = require('path')
 const Static = require('fastify-static')
 const subscription = require('./subscription')
+const { kRequestContext } = require('./symbols')
 const sJSON = require('secure-json-parse')
 const {
   defaultErrorFormatter,
@@ -134,14 +135,19 @@ module.exports = async function (app, opts) {
   if (typeof opts.errorHandler === 'function') {
     app.setErrorHandler(opts.errorHandler)
   } else if (opts.errorHandler === true || opts.errorHandler === undefined) {
-    app.setErrorHandler((error, _, reply) => {
-      const { statusCode, response } = errorFormatter(error)
+    app.setErrorHandler((error, request, reply) => {
+      const { statusCode, response } = errorFormatter(
+        error,
+        request[kRequestContext]
+      )
       reply.code(statusCode).send(response)
     })
   }
   const contextFn = opts.context
   const { subscriptionContextFn } = opts
 
+  app.decorateRequest(kRequestContext)
+
   const {
     path: graphqlPath = '/graphql',
     subscriber,
@@ -171,14 +177,16 @@ module.exports = async function (app, opts) {
       return new MER_ERR_GQL_PERSISTED_QUERY_NOT_FOUND('Unknown query')
     }
 
-    // Generate the context for this request
-    let context = {}
-    if (contextFn) {
-      context = await contextFn(request, reply)
-    }
-
     // Handle the query, throwing an error if required
-    return reply.graphql(query, Object.assign(context, { pubsub: subscriber, __currentQuery: query }), variables, operationName)
+    return reply.graphql(
+      query,
+      Object.assign(
+        request[kRequestContext],
+        { pubsub: subscriber, __currentQuery: query }
+      ),
+      variables,
+      operationName
+    )
   }
 
   function executeRegularQuery (body, request, reply) {
@@ -243,6 +251,14 @@ module.exports = async function (app, opts) {
     schema: getSchema,
     attachValidation: true,
     handler: async function (request, reply) {
+      // Generate the context for this request
+      if (contextFn) {
+        request[kRequestContext] = await contextFn(request, reply)
+        Object.assign(request[kRequestContext], { reply, app })
+      } else {
+        request[kRequestContext] = { reply, app }
+      }
+
       validationHandler(request.validationError)
 
       const { variables, extensions } = request.query
@@ -279,6 +295,14 @@ module.exports = async function (app, opts) {
     schema: postSchema(allowBatchedQueries),
     attachValidation: true
   }, async function (request, reply) {
+    // Generate the context for this request
+    if (contextFn) {
+      request[kRequestContext] = await contextFn(request, reply)
+      Object.assign(request[kRequestContext], { reply, app })
+    } else {
+      request[kRequestContext] = { reply, app }
+    }
+
     validationHandler(request.validationError)
 
     if (allowBatchedQueries && Array.isArray(request.body)) {
@@ -286,7 +310,7 @@ module.exports = async function (app, opts) {
       return Promise.all(request.body.map(r =>
         execute(r, request, reply)
           .catch(e => {
-            const { response } = errorFormatter(e)
+            const { response } = errorFormatter(e, request[kRequestContext])
             return response
           })
       ))
diff --git a/lib/symbols.js b/lib/symbols.js
index f22a70c..fff6873 100644
--- a/lib/symbols.js
+++ b/lib/symbols.js
@@ -4,7 +4,8 @@ const keys = {
   kLoaders: Symbol('mercurius.loaders'),
   kFactory: Symbol('mercurius.loadersFactory'),
   kSubscriptionFactory: Symbol('mercurius.subscriptionLoadersFactory'),
-  kHooks: Symbol('mercurius.hooks')
+  kHooks: Symbol('mercurius.hooks'),
+  kRequestContext: Symbol('mercurius.requestContext')
 }
 
 module.exports = keys
diff --git a/package.json b/package.json
index 2aa2ee7..2a1acc3 100644
--- a/package.json
+++ b/package.json
@@ -50,7 +50,7 @@
     "split2": "^4.0.0",
     "standard": "^16.0.3",
     "tap": "^15.0.9",
-    "tsd": "^0.18.0",
+    "tsd": "^0.19.0",
     "typescript": "^4.3.5",
     "wait-on": "^6.0.0"
   },


Test patch:
diff --git a/test/batched.js b/test/batched.js
index 435bc08..2a1d47b 100644
--- a/test/batched.js
+++ b/test/batched.js
@@ -111,6 +111,55 @@ test('POST single bad batched query', async (t) => {
   t.same(JSON.parse(res.body), [{ data: null, errors: [{ message: 'Syntax Error: Expected "$", found <EOF>.', locations: [{ line: 2, column: 37 }] }] }])
 })
 
+test('POST single bad batched query with cutom error formatter and custom async context', async (t) => {
+  t.plan(2)
+
+  const app = Fastify()
+
+  const schema = `
+      type Query {
+        add(x: Int, y: Int): Int
+      }
+    `
+
+  const resolvers = {
+    add: async ({ x, y }) => x + y
+  }
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    allowBatchedQueries: true,
+    context: async () => {
+      return { topic: 'NOTIFICATIONS_ADDED' }
+    },
+    errorFormatter: (_execution, context) => {
+      t.include(context, { topic: 'NOTIFICATIONS_ADDED' })
+      return {
+        response: {
+          data: null,
+          errors: [{ message: 'Internal Server Error' }]
+        }
+      }
+    }
+  })
+
+  const res = await app.inject({
+    method: 'POST',
+    url: '/graphql',
+    body: [
+      {
+        operationName: 'AddQuery',
+        variables: { x: 1, y: 2 },
+        query: `
+            query AddQuery ($x: Int!`
+      }
+    ]
+  })
+
+  t.same(JSON.parse(res.body), [{ data: null, errors: [{ message: 'Internal Server Error' }] }])
+})
+
 test('POST batched query', async (t) => {
   const app = Fastify()
 
diff --git a/test/gateway/request.js b/test/gateway/request.js
index f0909a0..7c85991 100644
--- a/test/gateway/request.js
+++ b/test/gateway/request.js
@@ -84,3 +84,82 @@ test('sendRequest method rejects when response contains errors', async (t) => {
 
   t.end()
 })
+
+test('sendRequest method should accept useSecureParse flag and parse the response securely', async (t) => {
+  const app = fastify()
+  app.post('/', async (request, reply) => {
+    return '{" __proto__": { "foo": "bar" } }'
+  })
+
+  await app.listen(0)
+
+  const url = new URL(`http://localhost:${app.server.address().port}`)
+  const { request, close } = buildRequest({ url })
+  t.teardown(() => {
+    close()
+    return app.close()
+  })
+  const result = await
+  sendRequest(
+    request,
+    url,
+    true
+  )({
+    method: 'POST',
+    body: JSON.stringify({
+      query: `
+    query ServiceInfo {
+      _service {
+        sdl
+      }
+    }
+    `
+    })
+  })
+
+  // checking for prototype leakage: https://github.com/fastify/secure-json-parse#introduction
+  // secure parsing should not allow it
+  t.ok(result.json)
+  t.notOk(result.json.foo)
+  const testObject = Object.assign({}, result.json)
+  t.notOk(testObject.foo)
+
+  t.end()
+})
+
+test('sendRequest method should run without useSecureParse flag', async (t) => {
+  const app = fastify()
+  app.post('/', async (request, reply) => {
+    return '{ "foo": "bar" }'
+  })
+
+  await app.listen(0)
+
+  const url = new URL(`http://localhost:${app.server.address().port}`)
+  const { request, close } = buildRequest({ url })
+  t.teardown(() => {
+    close()
+    return app.close()
+  })
+  const result = await
+  sendRequest(
+    request,
+    url,
+    false
+  )({
+    method: 'POST',
+    body: JSON.stringify({
+      query: `
+      query ServiceInfo {
+        _service {
+          sdl
+        }
+      }
+      `
+    })
+  })
+
+  t.same(result.json, { foo: 'bar' })
+
+  t.end()
+})
diff --git a/test/routes.js b/test/routes.js
index 8cbb0a1..7c6c01a 100644
--- a/test/routes.js
+++ b/test/routes.js
@@ -582,6 +582,49 @@ test('POST return 400 on error', async (t) => {
   t.matchSnapshot(JSON.stringify(JSON.parse(res.body), null, 2))
 })
 
+test('POST return 400 error handler provide custom context to custom async error formatter', async (t) => {
+  t.plan(2)
+
+  const app = Fastify()
+  const schema = `
+    type Query {
+      add(x: Int, y: Int): Int
+    }
+  `
+
+  const resolvers = {
+    add: async ({ x, y }) => x + y
+  }
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    errorHandler: true,
+    context: async () => {
+      return { topic: 'POINT_ADDED' }
+    },
+    errorFormatter: (_execution, context) => {
+      t.has(context, { topic: 'POINT_ADDED' })
+      return {
+        statusCode: 400,
+        response: {
+          data: { add: null },
+          errors: [{ message: 'Internal Server Error' }]
+        }
+      }
+    }
+  })
+
+  // Invalid query
+  const res = await app.inject({
+    method: 'POST',
+    url: '/graphql',
+    body: { query: '{ add(x: 2, y: 2)' }
+  })
+
+  t.equal(res.statusCode, 400)
+})
+
 test('mutation with POST', async (t) => {
   const app = Fastify()
   const schema = `
@@ -1655,7 +1698,11 @@ test('error thrown from onDisconnect is logged', t => {
   const error = new Error('error')
 
   const app = Fastify()
+
+  // override `app.log` to avoid polluting other tests
+  app.log = Object.create(app.log)
   app.log.error = (e) => { t.same(error, e) }
+
   t.teardown(() => app.close())
 
   const schema = `
@@ -1702,6 +1749,9 @@ test('promise rejection from onDisconnect is logged', t => {
   const error = new Error('error')
 
   const app = Fastify()
+
+  // override `app.log` to avoid polluting other tests
+  app.log = Object.create(app.log)
   app.log.error = (e) => { t.same(error, e) }
   t.teardown(() => app.close())