Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

redact produces unquoted strings #599

Closed
wants to merge 3 commits into from
Closed

redact produces unquoted strings #599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d01970e
Bypass redact call on strings
n4zukker Feb 9, 2019
72838f4
Bypass redact call on strings
n4zukker Feb 9, 2019
d4bedea
remove variable
n4zukker Feb 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/redaction.js
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@

const fastRedact = require('fast-redact')
const { redactFmtSym, wildcardFirstSym } = require('./symbols')
const { asString } = require('./tools.js')
const { rx, validator } = fastRedact

const validate = validator({
Expand Down Expand Up @@ -77,7 +78,10 @@ function redaction (opts, serialize) {
return [...Object.keys(shape), ...Object.getOwnPropertySymbols(shape)].reduce((o, k) => {
// top level key:
if (shape[k] === null) o[k] = topCensor
else o[k] = fastRedact({ paths: shape[k], censor, serialize, strict })
else {
const fr = fastRedact({ paths: shape[k], censor, serialize, strict })
o[k] = value => (typeof value === 'string' ? asString(value) : fr(value))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be better fixed in fastRedact itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
return o
}, result)
}
Expand Down
1 change: 1 addition & 0 deletions lib/tools.js
View file
Open in desktop
Expand Up @@ -378,6 +378,7 @@ module.exports = {
buildSafeSonicBoom,
getPrettyStream,
asChindings,
asString,
asJson,
genLog,
createArgsNormalizer,
Expand Down
35 changes: 34 additions & 1 deletion test/redact.test.js
View file
Open in desktop
Expand Up @@ -556,7 +556,7 @@ test('redacts strings at the top level', async ({ is }) => {
is(o.s, '[Redacted]')
})

test('does not redact primitives if not objects', async ({ is }) => {
test('does not redact numbers if not objects', async ({ is }) => {
const stream = sink()
const instance = pino({ redact: ['a.b'] }, stream)
const obj = {
Expand All @@ -567,6 +567,39 @@ test('does not redact primitives if not objects', async ({ is }) => {
is(o.a, 42)
})

test('does not redact strings if not objects', async ({ is }) => {
const stream = sink()
const instance = pino({ redact: ['a.b'] }, stream)
const obj = {
a: 's'
}
instance.info(obj)
const o = await once(stream, 'data')
is(o.a, 's')
})

test('does not redact booleans if not objects', async ({ is }) => {
const stream = sink()
const instance = pino({ redact: ['a.b'] }, stream)
const obj = {
a: true
}
instance.info(obj)
const o = await once(stream, 'data')
is(o.a, true)
})

test('does not redact nulls if not objects', async ({ is }) => {
const stream = sink()
const instance = pino({ redact: ['a.b'] }, stream)
const obj = {
a: null
}
instance.info(obj)
const o = await once(stream, 'data')
is(o.a, null)
})

test('redacts null at the top level', async ({ is }) => {
const stream = sink()
const instance = pino({ redact: ['n'] }, stream)
Expand Down