redact produces unquoted strings

[n4zukker]

pino has a bug where it can produce invalid JSON. Here's an example:

$ cat ./test.js
'use strict'

var pino = require('./pino')({ redact: { paths: [ 'foo.bar' ] } })
pino.info({ foo: 'abc' }, 'msg')

$ node test.js | pino-pretty
{"level":30,"time":1549754571633,"msg":"msg","pid":10427,"hostname":"xxx","foo":abc,"v":1}
$

The bug happens when pino is called with a string property that matches the root of one of the redact paths. In the example above, we have a redact path of "foo.bar", but pino is called with an object that has a string for "foo". pino outputs that string value without quotes. "foo":abc should be "foo":"abc".

This bug is troublesome if the redact path starts with a wildcard, such as "*.password". It causes pino to output, without quotes, every string in the logged object.

With this bug, pino is susceptible to JSON injection. Change the call above to:

pino.info({ foo: '{"a":false}' }, 'msg')

and the output is:

$ node test.js | pino-pretty
[1549755468666] INFO (10869 on xxx): msg
    foo: {
      "a": false
    }

Fake messages can be injected with:

pino.info({ foo: '0}\n{"a":false' }, 'msg')

$ node test.js | pino-pretty
{"level":30,"time":1549755810950,"msg":"msg","pid":11206,"hostname":"xxx","foo":0}
[undefined] USERLVL:
    a: false

Why does this happen?
It happens because the non-strict fast-redact simply returns primitive objects. (https://github.com/davidmarkclements/fast-redact#strict-boolean---true) Strings are returned, without quotes and without escapes. So when the redactor gets called with a string, it simply echos the string value back.

What's the fix?
Add a test before the redactor is called. If it would be called with a string, make a call to tools.asString instead.

Note: There was a test case that pino worked properly with primitives and paths. But the test was checking numbers and not strings, so this bug wasn't caught. I've added individual test cases for string, boolean and null.

[mcollina]

I think this might be better fixed in fastRedact itself.

[mcollina]

Or maybe I misunderstood and it is a fast-redact thing.

[n4zukker]

https://github.com/davidmarkclements/fast-redact is a little odd because it returns JSON text when an object is passed in, primitives otherwise (with strict set to false). Perhaps it would be more consistent to always return JSON text. When primitives were passed in, JSON.stringify() would be called for the return.

It would be a simple matter of changing the last lines of lib/redactor.js in fast-redact

function strictImpl (strict) {
  return strict === true ? `throw Error('fast-redact: primitives cannot be redacted')` : `return JSON.stringify(o)`    <--- used to be "return o"
}

but that breaks the existing unit test which checks that when a number is passed in, a number is returned. The return could be written as:
return typeof o === 'string' ? JSON.stringify(o) : o
(basically the same as what is in this PR). All tests would pass, but then the definition of strict=false would be a little awkward. And the code doesn't use the cool optimized asString() function which pino has.

I think if you fixed fast-redact now, you'd be changing its documented behavior and so might break existing code in other packages which make use of it.

I suggest that a little caveat is added to the fast-readact markdown file warning of the strict=false treatment of string primitives. And we work around this in the pino code.

[mcollina]

I think if you fixed fast-redact now, you'd be changing its documented behavior and so might break existing code in other packages which make use of it.

I think we should release a new major version of fast-redact with the correct behavior, and then update pino. In this way other packages will not break unless they update to the new (fixed) version.

Would you mind sending a PR to fast-redact in this sense?

[n4zukker]

Sounds good to me. I've created davidmarkclements/fast-redact#15

So we can close this PR without merging. And update package.json with the new fast-redact version when it is released.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.