davidmarkclements / fast-redact

very fast object redaction
MIT License
284 stars 30 forks source link

Optional throw Error? #3

Closed robmcguinness closed 6 years ago

robmcguinness commented 6 years ago

I'm using a redacted path like ['data.username']. fast-redact produces the following method signature for redaction:

(function(o
/*``*/) {

    if (typeof o !== 'object' || o == null) {
      throw Error('fast-redact: primitives cannot be redacted')
    }
    const { censor, secret } = this

      if (o.username != null) {
        const val = o.username
        if (val === censor) {
          secret["username"].precensored = true
        } else {
          secret["username"].val = val
          o.username = censor

      switch (true) {

      }

        }
      }
...

})

When using a library like axios, data can be either an object or a string when content type is application/json vs application/x-www-form-urlencoded. What are some thoughts about having a setting to just ignore redaction if an o is primitive vs throwing an error?

davidmarkclements commented 6 years ago

I think just check before passing it?

@mcollina - thoughts?

robmcguinness commented 6 years ago

FYI I'm using pino v5 with redact settings.

mcollina commented 6 years ago

I don’t really understand what is the problem.

davidmarkclements commented 6 years ago

@robmcguinness can you show a bit of code that's causing this problem (with pino preferably)

robmcguinness commented 6 years ago

absolutely. i'll setup an example.

robmcguinness commented 6 years ago

Please check out the example when you can: https://github.com/robmcguinness/pino/commit/85b410fb74e521549811549dcb37c0f207205d3e. Example inlined for reference.

test('redact – optional throw for primitives', async ({ is }) => {
  const stream = sink()
  const instance = pino({ redact: ['data.username'] }, stream)
  instance.info({
    data: {
      username: 'a'
    }
  })
  let { data } = await once(stream, 'data')
  is(data.username, '[Redacted]')

  // Don't throw just ignore?
  instance.info({
    data: 'b'
  })

  // Value should just pass through
  data = await once(stream, 'data')
  is(data, 'b')
})
mcollina commented 6 years ago

I think ignoring is the correct behavior. One of the key principles in pino is that logging a line should never throw (like console.log).

In your case, we are looking for a username property on the data object. I would recommend using a library like Joy or Ajv to validate and normalize your inputs so that it always have the same properties.

davidmarkclements commented 6 years ago

Yes it should be ignoring - the throw is a mistake. PR's very welcome

robmcguinness commented 6 years ago

Thanks for feedback. I'll submit a PR soon.