fix(helpers): make formatHttpRequest and formatHttpResponse more defensive (#69)

This also changes those and formatError to return a bool for whether the
given obj was processed. The loggers will be able to use this to control
passing through an unprocessed special field.

Refs: #58

Author: trentm

helpers/CHANGELOG.md

@@ -1,5 +1,15 @@
 # @elastic/ecs-helpers Changelog
 
+## Unreleased
+
+- Fix `formatHttpRequest` and `formatHttpResponse` to be more defensive. If
+  the given request or response object, respectively, is not one they know
+  how to process (for example if a user logs a `req` or `res` field that
+  is not a Node.js http request or response object), then processing is skipped.
+  The functions now return true if the given object could be processed,
+  false otherwise. `formatError` was similarly changed to return true/false for
+  whether the given `err` could be processed.
+
 ## v1.0.0
 
 - Change `formatHttpRequest` and `formatHttpResponse` to no longer exclude

helpers/README.md

@@ -46,10 +46,11 @@ of objects with circular references. This generally means that ecs-logging-nodej
 libraries will throw a "Converting circular structure to JSON" exception if an
 attempt is made to log an object with circular references.
 
-### `formatError(obj, err)`
+### `formatError(obj, err) -> bool`
 
 A function that adds [ECS Error fields](https://www.elastic.co/guide/en/ecs/current/ecs-error.html)
-for a given `Error` object.
+for a given `Error` object. It returns true iff the given `err` was an Error
+object it could process.
 
 ```js
 const { formatError } = require('@elastic/ecs-helpers')
@@ -79,14 +80,19 @@ metadata field passed to a logging statement. E.g.
 `log.warn({err: myErr}, '...')` for pino, `log.warn('...', {err: myErr})`
 for winston.
 
-### `formatHttpRequest(obj, req)`
+### `formatHttpRequest(obj, req) -> bool`
 
 Function that enhances an ECS object with http request data.
 The given request object, `req`, must be one of the following:
 - Node.js's core [`http.IncomingMessage`](https://nodejs.org/api/all.html#http_class_http_incomingmessage),
 - [Express's request object](https://expressjs.com/en/5x/api.html#req) that extends IncomingMessage, or
 - a [hapi request object](https://hapi.dev/api/#request).
 
+The function returns true iff the given `req` was a request object it could
+process. Note that currently this notably does not process a
+[`http.ClientRequest`](https://nodejs.org/api/all.html#http_class_http_clientrequest)
+as returned from `http.request()`.
+
 ```js
 const http = require('http')
 const { formatHttpRequest } = require('@elastic/ecs-helpers')
@@ -138,6 +144,13 @@ The given request object, `req`, must be one of the following:
 - [Express's response object](https://expressjs.com/en/5x/api.html#res) that extends ServerResponse, or
 - a [hapi **request** object](https://hapi.dev/api/#request)
 
+The function returns true iff the given `res` was a response object it could
+process. Note that currently this notably does not process a
+[`http.IncomingMessage`](https://nodejs.org/api/all.html#http_class_http_incomingmessage)
+that is the argument to the
+["response" event](https://nodejs.org/api/all.html#http_event_response) of a
+[client `http.request()`](https://nodejs.org/api/all.html#http_http_request_options_callback)
+
 ```js
 const http = require('http')
 const { formatHttpRequest } = require('@elastic/ecs-helpers')

helpers/lib/error-formatters.js

@@ -21,10 +21,11 @@ const { toString } = Object.prototype
 
 // Format an Error instance into ECS-compatible fields on the `ecs` object.
 // https://www.elastic.co/guide/en/ecs/current/ecs-error.html
+// Return true iff the given `err` was an Error object that could be processed.
 function formatError (ecsFields, err) {
   if (!(err instanceof Error)) {
     ecsFields.err = err
-    return
+    return false
   }
 
   ecsFields.error = {
@@ -34,6 +35,8 @@ function formatError (ecsFields, err) {
     message: err.message,
     stack_trace: err.stack
   }
+
+  return true
 }
 
 module.exports = { formatError }

helpers/lib/http-formatters.js

@@ -17,13 +17,23 @@
 
 'use strict'
 
+// Write ECS fields for the given HTTP request (expected to be
+// `http.IncomingMessage`-y) into the `ecs` object. This returns true iff
+// the given `req` was a request object it could process.
 function formatHttpRequest (ecs, req) {
+  if (req === undefined || req === null || typeof req !== 'object') {
+    return false
+  }
   if (req.raw && req.raw.req && req.raw.req.httpVersion) {
     // This looks like a hapi request object (https://hapi.dev/api/#request),
     // use the raw Node.js http.IncomingMessage that it references.
     // TODO: Use hapi's already parsed `req.url` for speed.
     req = req.raw.req
   }
+  // Use duck-typing to check this is a `http.IncomingMessage`-y object.
+  if (!('httpVersion' in req && 'headers' in req && 'method' in req)) {
+    return false
+  }
 
   const {
     id,
@@ -103,14 +113,26 @@ function formatHttpRequest (ecs, req) {
       ecs.user_agent.original = headers['user-agent']
     }
   }
+
+  return true
 }
 
+// Write ECS fields for the given HTTP response (expected to be
+// `http.ServiceResponse`-y) into the `ecs` object. This returns true iff
+// the given `res` was a response object it could process.
 function formatHttpResponse (ecs, res) {
+  if (res === undefined || res === null || typeof res !== 'object') {
+    return false
+  }
   if (res.raw && res.raw.res && typeof (res.raw.res.getHeaders) === 'function') {
     // This looks like a hapi request object (https://hapi.dev/api/#request),
     // use the raw Node.js http.ServerResponse that it references.
     res = res.raw.res
   }
+  // Use duck-typing to check this is a `http.ServerResponse`-y object.
+  if (!('statusCode' in res && typeof res.getHeaders === 'function')) {
+    return false
+  }
 
   const { statusCode } = res
   ecs.http = ecs.http || {}
@@ -129,6 +151,8 @@ function formatHttpResponse (ecs, res) {
       ecs.http.response.body.bytes = cLen
     }
   }
+
+  return true
 }
 
 module.exports = { formatHttpRequest, formatHttpResponse }

helpers/test/basic.test.js

@@ -18,6 +18,7 @@
 'use strict'
 
 const http = require('http')
+const { inspect } = require('util')
 
 const addFormats = require('ajv-formats').default
 const Ajv = require('ajv').default
@@ -144,8 +145,10 @@ test('formatHttpRequest and formatHttpResponse should return a valid ecs object'
     // add anchor
     req.url += '#anchor'
 
-    formatHttpRequest(ecs, req)
-    formatHttpResponse(ecs, res)
+    let rv = formatHttpRequest(ecs, req)
+    t.ok(rv, 'formatHttpRequest processed req')
+    rv = formatHttpResponse(ecs, res)
+    t.ok(rv, 'formatHttpResponse processed res')
 
     const line = JSON.parse(stringify(ecs))
     t.ok(validate(line))
@@ -192,6 +195,36 @@ test('formatHttpRequest and formatHttpResponse should return a valid ecs object'
   }
 })
 
+test('format* should not process non-req/res/err values', t => {
+  const inputs = [
+    null,
+    'hi',
+    42,
+    {},
+    []
+  ]
+  let obj
+  let rv
+  inputs.forEach(input => {
+    obj = {}
+    rv = formatError(obj, input)
+    t.strictEqual(rv, false, `formatError did not process input: ${inspect(input)}`)
+    // Cannot test that obj is unmodified because `formatError` sets obj.err.
+    // See https://github.com/elastic/ecs-logging-nodejs/issues/66 to change that.
+
+    obj = {}
+    rv = formatHttpRequest(obj, input)
+    t.strictEqual(rv, false, `formatHttpRequest did not process input: ${inspect(input)}`)
+    t.equal(Object.keys(obj).length, 0, `obj was not modified: ${inspect(obj)}`)
+
+    obj = {}
+    rv = formatHttpResponse(obj, input)
+    t.strictEqual(rv, false, `formatHttpResponse did not process input: ${inspect(input)}`)
+    t.equal(Object.keys(obj).length, 0, `obj was not modified: ${inspect(obj)}`)
+  })
+  t.end()
+})
+
 test('Should export a valid version', t => {
   t.ok(semver.valid(version))
   t.end()

helpers/test/express.test.js

@@ -38,8 +38,10 @@ test('express res/req serialization', t => {
     res.write('hi')
     res.end()
 
-    formatHttpRequest(rec, req)
-    formatHttpResponse(rec, res)
+    let rv = formatHttpRequest(rec, req)
+    t.ok(rv, 'formatHttpRequest processed req')
+    rv = formatHttpResponse(rec, res)
+    t.ok(rv, 'formatHttpResponse processed res')
 
     t.deepEqual(rec.user_agent, { original: 'cool-agent' })
     t.deepEqual(rec.url, {

helpers/test/hapi.test.js

@@ -48,8 +48,10 @@ test('hapi res/req serialization', testOpts, t => {
 
   server.events.on('response', (request) => {
     const rec = {}
-    formatHttpRequest(rec, request)
-    formatHttpResponse(rec, request)
+    let rv = formatHttpRequest(rec, request)
+    t.ok(rv, 'formatHttpRequest processed request')
+    rv = formatHttpResponse(rec, request)
+    t.ok(rv, 'formatHttpResponse processed request')
 
     t.deepEqual(rec.user_agent, { original: 'cool-agent' })
     t.deepEqual(rec.url, {