Merge pull request #122 from BackendStack21/fix/error-status-code-validation

fix: validate error-derived status codes to prevent process crash (DoS)

Author: jkyberneees

index.js

@@ -8,7 +8,7 @@
 
 const requestRouter = require('./libs/request-router')
 const applySecurityHeaders = require('./libs/security-headers')
-const { deepFreezeObject } = require('./libs/utils')
+const { deepFreezeObject, toHttpStatusCode } = require('./libs/utils')
 const exts = {
   request: {},
   response: require('./libs/response-extensions')
@@ -18,9 +18,7 @@ module.exports = (options = {}) => {
   options.errorHandler =
     options.errorHandler ||
     ((err, req, res) => {
-      const statusCode = typeof (err.status || err.code || err.statusCode) === 'number'
-        ? (err.status || err.code || err.statusCode)
-        : 500
+      const statusCode = toHttpStatusCode(err.status || err.code || err.statusCode)
       res.send({ code: statusCode, message: 'Internal Server Error' }, statusCode)
     })
 

libs/response-extensions.js

@@ -1,6 +1,6 @@
 'use strict'
 
-const { forEachObject } = require('./utils')
+const { forEachObject, toHttpStatusCode } = require('./utils')
 
 const CONTENT_TYPE_HEADER = 'content-type'
 const TYPE_JSON = 'application/json; charset=utf-8'
@@ -32,14 +32,13 @@ const beforeEnd = (res, contentType, statusCode, data) => {
   if (contentType) {
     res.setHeader(CONTENT_TYPE_HEADER, contentType)
   }
-  res.statusCode = statusCode
+  res.statusCode = toHttpStatusCode(statusCode, res.statusCode)
 }
 
 const isProduction = () => process.env.NODE_ENV === 'production'
 
 const parseErr = error => {
-  const errorCode = error.status || error.code || error.statusCode
-  const statusCode = typeof errorCode === 'number' ? errorCode : 500
+  const statusCode = toHttpStatusCode(error.status || error.code || error.statusCode)
 
   if (isProduction()) {
     return {

libs/utils.js

@@ -1,5 +1,18 @@
 'use strict'
 
+/**
+ * Coerces a value into a valid HTTP status code.
+ * Returns the fallback when the value is not an integer in the 100-999 range,
+ * preventing `RangeError: Invalid status code` crashes on res.statusCode.
+ *
+ * @param {*} code
+ * @param {number} [fallback=500]
+ * @returns {number}
+ */
+module.exports.toHttpStatusCode = (code, fallback = 500) => {
+  return Number.isInteger(code) && code >= 100 && code <= 999 ? code : fallback
+}
+
 module.exports.forEachObject = (obj, cb) => {
   const keys = Object.keys(obj)
   const length = keys.length

specs/security.test.js

@@ -310,4 +310,108 @@ describe('Security Fixes', () => {
       await service.close()
     })
   })
+
+  describe('SEC-005: Out-of-range error status codes fall back to 500', () => {
+    let server
+    const service = require('../index')({
+      errorHandler (err, req, res) {
+        res.send(err)
+      }
+    })
+
+    service.get('/hello', (req, res) => {
+      res.send('world')
+    })
+
+    service.get('/error-code-99', (req, res) => {
+      const err = new Error('Low code')
+      err.code = 99
+      throw err
+    })
+
+    service.get('/error-status-huge', (req, res) => {
+      const err = new Error('Huge status')
+      err.status = 12345
+      throw err
+    })
+
+    service.get('/error-float-code', (req, res) => {
+      const err = new Error('Float code')
+      err.code = 200.5
+      throw err
+    })
+
+    it('should start service', async () => {
+      server = await service.start(~~process.env.PORT)
+    })
+
+    it('should return 500 instead of crashing when err.code is out of range', async () => {
+      await request(server)
+        .get('/error-code-99')
+        .expect(500)
+        .then((response) => {
+          expect(response.body.code).to.equal(500)
+        })
+    })
+
+    it('should return 500 instead of crashing when err.status exceeds 999', async () => {
+      await request(server)
+        .get('/error-status-huge')
+        .expect(500)
+        .then((response) => {
+          expect(response.body.code).to.equal(500)
+        })
+    })
+
+    it('should return 500 instead of crashing when err.code is not an integer', async () => {
+      await request(server)
+        .get('/error-float-code')
+        .expect(500)
+        .then((response) => {
+          expect(response.body.code).to.equal(500)
+        })
+    })
+
+    it('server should still be alive and responsive after out-of-range errors', async () => {
+      await request(server)
+        .get('/hello')
+        .expect(200)
+        .then((response) => {
+          expect(response.text).to.equal('world')
+        })
+    })
+
+    it('should successfully terminate the service', async () => {
+      await service.close()
+    })
+  })
+
+  describe('SEC-005b: Default error handler also survives out-of-range codes', () => {
+    let server
+    const service = require('../index')()
+
+    service.get('/error-bad-code', (req, res) => {
+      const err = new Error('boom')
+      err.code = 99
+      throw err
+    })
+
+    it('should start service', async () => {
+      server = await service.start(~~process.env.PORT)
+    })
+
+    it('should return 500 with generic message and keep the process alive', async () => {
+      await request(server)
+        .get('/error-bad-code')
+        .expect(500)
+        .then((response) => {
+          expect(response.body.code).to.equal(500)
+          expect(response.body.message).to.equal('Internal Server Error')
+        })
+    })
+
+    it('should successfully terminate the service', async () => {
+      await service.close()
+    })
+  })
 })