handleFinalRequests logs headers which might contain sensitive information

[mriedem]

I'm opening this mostly to start a discussion and kick around some ideas. I've noticed that when the express server is shutting down and getting requests the handleFinalRequests function is logging the headers which could include sensitive information like an API token.

We are providing our own bunyan logger instance method (either info or error method).

A couple of ideas on how we could deal with this:

• The gracefulExitHandler function could take a new option which is a callback function to scrub header values for logging which by default would just return the headers object back untouched (or a copy of it really so it's not modified by reference, that's probably better to not cause issues with downstream middleware).
• We could probably work around this if the headers are passed as an object to the logger method like how bunyan's log method API works but that is probably not generic enough, i.e. we shouldn't restrict the solution to assuming everyone's logger method is from bunyan. But with that idea the passed in logger function could check if that object contains a header property and scrub is as necessary before passing it on to the underlying logger (bunyan in our case).
• As a fully out-of-tree workaround, our logger function passed in could check the log message for known header keys that we need to scrub and find/replace the values as appropriate. Then there would be no changes to express-graceful-exit. If this is how everyone else is already dealing with this issue, I'm OK with that as the answer.

[ivolucien]

Thank you @mriedem, my apologies for missing this last month.

Great thoughts and I appreciate your going beyond the "here's a PR" with a one sentence description as is so common in OSS.

I'm happy for any further input, and I would currently choose to support a new callback function like in your first suggestion above, but implemented as wrapper around the log call, preLogHook() or similar, that by default passes all the args through to the logger unmodified.

There's a slight chance that returning a copy of any log parameters could break someone's code, so if folks really want it I'd add it to a v1.0 feature set as a possible breaking change. (paranoid perhaps, but I'm of the really no-breaky-breaky camp)

I intend to keep this repo maintained, so please ping me as often as you like if I don't respond within a couple days. My special needs kiddo combined with no child care has rendered me more flaky than I'm happy with.

Cheers - Ivo

[mriedem]

Sure, thanks for the reply and this is pretty low priority for us since we've gone with the fully out of tree workaround which is the 3rd option above:

As a fully out-of-tree workaround, our logger function passed in could check the log message for known header keys that we need to scrub and find/replace the values as appropriate. Then there would be no changes to express-graceful-exit. If this is how everyone else is already dealing with this issue, I'm OK with that as the answer.

That works pretty well for us with something like this:

function gracefulExitLogger (logMethod) {
  // This is based on https://github.com/emostar/express-graceful-exit/blob/v0.5.0/lib/graceful-exit.js#L147
  // which just passes a single log message string but can include request headers.
  return msg => {
    // Mask the x-access-token header from the message if it exists.
    if (msg) {
      msg = msg.replace(/x-access-token: '.*'/, "x-access-token: 'xxx'")
    }
    logMethod(msg)
  }
}

function gracefulShutdown (app, server, logLevel, callback) {
  gracefulExit.gracefulExitHandler(app, server, {
    log: true,
    logger: gracefulExitLogger(log[logLevel].bind(log)),
    suicideTimeout: 15000,
    force: true,
    exitProcess: false,
    callback,
    errorDuringExit: true
  })
}

[ivolucien]

@mriedem - Cool, I'll see if I can get this done fast during work time (shutterstock) or must squeeze it into dribs of free time. :-)

And I hear you that it's low priority. 🙇