fix: server crash by SyntaxError from express JSON parser (#214)

Co-authored-by: Marcel Schwarz <[email protected]>

Author: mariayord

CHANGELOG.md

@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Sanitization of logged queries, when running in production, if literal values contain parentheses
 - Read GraphiQL HTML file only once on startup, and only if GraphiQL is enabled, to avoid unnecessary file system access
+- Server crash by `SyntaxError` from express JSON parser
 
 ### Removed
 

lib/logger.js

@@ -8,8 +8,17 @@ const LOG = cds.log('graphql')
 const { parse, visit, Kind, print } = require('graphql')
 
 const _isEmptyObject = o => Object.keys(o).length === 0
+class InvalidJSON extends Error {}
+InvalidJSON.prototype.name = 'Invalid JSON body'
+InvalidJSON.prototype.status = 400
 
-router.use(express.json({ ...cds.env.server.body_parser })) //> required by logger below
+router.use(function jsonBodyParser(req, res, next) {
+  express.json({ ...cds.env.server.body_parser })(req, res, function http_body_parser_next(err) {
+    // Need to wrap, as CAP server deliberately crashes on SyntaxErrors
+    if (err) return next(new InvalidJSON(err.message))
+    next()
+  })
+})
 
 router.use(function queryLogger(req, _, next) {
   let query = req.body?.query || (req.query.query && decodeURIComponent(req.query.query))

test/tests/concurrency.test.js

@@ -8,7 +8,6 @@ describe('graphql - resolver concurrency', () => {
   axios.defaults.validateStatus = false
 
   describe('execution order of query and mutation resolvers', () => {
-
     let _log = []
 
     beforeEach(() => {

test/tests/error-handling-prod.test.js

@@ -21,7 +21,6 @@ describe('graphql - error handling in production', () => {
     _error = []
   })
 
-
   describe('Errors thrown by CDS', () => {
     test('Single @mandatory validation error', async () => {
       const query = gql`
@@ -39,7 +38,7 @@ describe('graphql - error handling in production', () => {
         {
           message: expect.any(String),
           extensions: {
-            code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/) ,
+            code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/),
             target: 'notEmptyI'
           }
         }
@@ -48,7 +47,10 @@ describe('graphql - error handling in production', () => {
       expect(response.data).toMatchObject({ errors })
       expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production
       const log = _warn[0][1] || JSON.parse(_warn[0][0])
-      expect(log).toMatchObject({ code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/) , target: 'notEmptyI' })
+      expect(log).toMatchObject({
+        code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/),
+        target: 'notEmptyI'
+      })
     })
 
     test('Multiple @mandatory validation errors', async () => {
@@ -70,12 +72,12 @@ describe('graphql - error handling in production', () => {
             code: 'MULTIPLE_ERRORS',
             details: [
               {
-                code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/) ,
+                code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/),
                 message: expect.any(String),
                 target: 'notEmptyI'
               },
               {
-                code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/) ,
+                code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/),
                 message: expect.any(String),
                 target: 'notEmptyS'
               }
@@ -135,7 +137,7 @@ describe('graphql - error handling in production', () => {
             code: 'MULTIPLE_ERRORS',
             details: [
               {
-                code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/) ,
+                code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/),
                 message: 'Wert ist erforderlich',
                 target: 'inRange'
               },
@@ -158,7 +160,7 @@ describe('graphql - error handling in production', () => {
       expect(log).toMatchObject({
         code: 'MULTIPLE_ERRORS',
         details: [
-          { code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/) , target: 'inRange' },
+          { code: expect.stringMatching(/ASSERT_MANDATORY|ASSERT_NOT_NULL/), target: 'inRange' },
           {
             code: 'ASSERT_ENUM',
             target: 'oneOfEnumValues'

test/tests/http.test.js

@@ -0,0 +1,33 @@
+const cds = require('@sap/cds')
+const path = require('path')
+const util = require('util')
+const { axios } = cds
+  .test('serve', 'srv/empty-service.cds')
+  .in(path.join(__dirname, '../resources/empty-csn-definitions'))
+const _format = e => util.formatWithOptions({ colors: false, depth: null }, ...(Array.isArray(e) ? e : [e]))
+
+let _error = []
+
+describe('GraphQL express json parser error scenario', () => {
+  beforeEach(() => {
+    console.warn = (...s) => _error.push(s) // eslint-disable-line no-console
+  })
+
+  afterEach(() => {
+    _error = []
+  })
+  test('should trigger InvalidJSON for malformed JSON', async () => {
+    expect.hasAssertions()
+    try {
+      response = await axios.request({
+        method: 'POST',
+        url: `/graphql`,
+        data: '{ some_value'
+      })
+    } catch (err) {
+      expect(err.status).toBe(400)
+      expect(err.response.data.error.message).toMatch(/not valid JSON/)
+      expect(_format(_error[0])).toContain('InvalidJSON')
+    }
+  })
+})

test/tests/schema.test.js

@@ -3,7 +3,7 @@ const path = require('path')
 // Load @cap-js/graphql plugin to ensure .to.gql and .to.graphql compile targets are registered
 require('../../cds-plugin')
 
-const  { models } = require('../resources')
+const { models } = require('../resources')
 const { SCHEMAS_DIR } = require('../util')
 const { printSchema, validateSchema } = require('graphql')