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.

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

feat(serializers): avoid implicit sanitization #2081

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -515,13 +515,27 @@ Default: `'msg'`

The string key for the 'message' in the JSON object.

<a id=opt-messagekey></a>
<a id=opt-errorkey></a>
#### `errorKey` (String)

Default: `'err'`

The string key for the 'error' in the JSON object.

<a id=opt-requestkey></a>
#### `requestKey` (String)

Default: `'req'`

The string key for the 'Request' in the JSON object.

<a id=opt-responsekey></a>
#### `responseKey` (String)

Default: `'res'`

The string key for the 'Response' in the JSON object.

<a id=opt-nestedkey></a>
#### `nestedKey` (String)

Expand Down
9 changes: 9 additions & 0 deletions lib/proto.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
/* eslint no-prototype-builtins: 0 */

const { EventEmitter } = require('node:events')
const { IncomingMessage, ServerResponse } = require('node:http')
const {
lsCacheSym,
levelValSym,
Expand All @@ -20,6 +21,8 @@ const {
serializersSym,
formattersSym,
errorKeySym,
requestKeySym,
responseKeySym,
messageKeySym,
useOnlyCustomLevelsSym,
needsMetadataGsym,
Expand Down Expand Up @@ -182,6 +185,8 @@ function write (_obj, msg, num) {
const t = this[timeSym]()
const mixin = this[mixinSym]
const errorKey = this[errorKeySym]
const requestKey = this[requestKeySym]
const responseKey = this[responseKeySym]
const messageKey = this[messageKeySym]
const mixinMergeStrategy = this[mixinMergeStrategySym] || defaultMixinMergeStrategy
let obj
Expand All @@ -193,6 +198,10 @@ function write (_obj, msg, num) {
if (msg === undefined) {
msg = _obj.message
}
} else if (_obj instanceof IncomingMessage) {
obj = { [requestKey]: _obj }
} else if (_obj instanceof ServerResponse) {
obj = { [responseKey]: _obj }
Copy link
Author

Choose a reason for hiding this comment

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

If preferred, we can assess type from properties, as previously done in lib/tools.js:

  } else if (_obj.method && _obj.headers && _obj.socket) {
    obj = { [requestKey]: _obj }
  } else if (typeof _obj.setHeader === 'function') {
    obj = { [responseKey]: _obj }

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 property presence will be better here. These could be extended objects, e.g. a FastifyRequest.

Copy link
Author

Choose a reason for hiding this comment

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

done

} else {
obj = _obj
if (msg === undefined && _obj[messageKey] === undefined && _obj[errorKey]) {
Expand Down
4 changes: 4 additions & 0 deletions lib/symbols.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ const nestedKeySym = Symbol('pino.nestedKey')
const nestedKeyStrSym = Symbol('pino.nestedKeyStr')
const mixinMergeStrategySym = Symbol('pino.mixinMergeStrategy')
const msgPrefixSym = Symbol('pino.msgPrefix')
const responseKeySym = Symbol('pino.responseKey')
const requestKeySym = Symbol('pino.requestKey')

const wildcardFirstSym = Symbol('pino.wildcardFirst')

Expand Down Expand Up @@ -62,6 +64,8 @@ module.exports = {
formatOptsSym,
messageKeySym,
errorKeySym,
responseKeySym,
requestKeySym,
nestedKeySym,
wildcardFirstSym,
needsMetadataGsym,
Expand Down
16 changes: 8 additions & 8 deletions lib/tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
/* eslint no-prototype-builtins: 0 */

const format = require('quick-format-unescaped')
const { mapHttpRequest, mapHttpResponse } = require('pino-std-serializers')
const SonicBoom = require('sonic-boom')
const onExit = require('on-exit-leak-free')
const {
Expand All @@ -21,6 +20,8 @@ const {
formattersSym,
messageKeySym,
errorKeySym,
requestKeySym,
responseKeySym,
nestedKeyStrSym,
msgPrefixSym
} = require('./symbols')
Expand All @@ -40,13 +41,6 @@ function genLog (level, hook) {
function LOG (o, ...n) {
if (typeof o === 'object') {
let msg = o
if (o !== null) {
if (o.method && o.headers && o.socket) {
o = mapHttpRequest(o)
} else if (typeof o.setHeader === 'function') {
o = mapHttpResponse(o)
}
}
let formatParams
if (msg === null && n.length === 0) {
formatParams = [null]
Expand Down Expand Up @@ -113,6 +107,8 @@ function asJson (obj, msg, num, time) {
const formatters = this[formattersSym]
const messageKey = this[messageKeySym]
const errorKey = this[errorKeySym]
const responseKey = this[responseKeySym]
const requestKey = this[requestKeySym]
let data = this[lsCacheSym][num] + time

// we need the child bindings added to the output first so instance logged
Expand All @@ -132,6 +128,10 @@ function asJson (obj, msg, num, time) {
value = serializers[key](value)
} else if (key === errorKey && serializers.err) {
value = serializers.err(value)
} else if (key === requestKey && serializers.req) {
value = serializers.req(value)
} else if (key === responseKey && serializers.res) {
value = serializers.res(value)
Comment on lines +131 to +134
Copy link
Author

Choose a reason for hiding this comment

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

Behaviour of all stdSerializers follow the same pattern.

}

const stringifier = stringifiers[key] || wildcardStringifier
Expand Down
10 changes: 10 additions & 0 deletions pino.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,14 @@ declare namespace pino {
* The string key for the 'error' in the JSON object. Default: "err".
*/
errorKey?: string;
/**
* The string key for the 'Request' in the JSON object. Default: "req".
*/
requestKey?: string;
/**
* The string key for the 'Response' in the JSON object. Default: "res".
*/
responseKey?: string;
Comment on lines +411 to +418
Copy link
Author

Choose a reason for hiding this comment

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

The key for request and response are now explicitly configurable. Previously, these had fixed values, set on pino-std-serializers (mapHttpRequest and mapHttpResponse).

Copy link
Author

Choose a reason for hiding this comment

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

maybe this change makes the exported functions mapHttpRequest and mapHttpResponse deprecated, in pino-std-serializers?

/**
* The string key to place any logged object under.
*/
Expand Down Expand Up @@ -741,6 +749,8 @@ declare namespace pino {
readonly formatOptsSym: unique symbol;
readonly messageKeySym: unique symbol;
readonly errorKeySym: unique symbol;
readonly responseKeySym: unique symbol;
readonly requestKeySym: unique symbol;
readonly nestedKeySym: unique symbol;
readonly wildcardFirstSym: unique symbol;
readonly needsMetadataGsym: unique symbol;
Expand Down
14 changes: 13 additions & 1 deletion pino.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ const {
formatOptsSym,
messageKeySym,
errorKeySym,
requestKeySym,
responseKeySym,
nestedKeySym,
mixinSym,
levelCompSym,
Expand All @@ -49,17 +51,23 @@ const { epochTime, nullTime } = time
const { pid } = process
const hostname = os.hostname()
const defaultErrorSerializer = stdSerializers.err
const defaultRequestSerializer = stdSerializers.req
const defaultResponseSerializer = stdSerializers.res
const defaultOptions = {
level: 'info',
levelComparison: SORTING_ORDER.ASC,
levels: DEFAULT_LEVELS,
messageKey: 'msg',
errorKey: 'err',
requestKey: 'req',
responseKey: 'res',
nestedKey: null,
enabled: true,
base: { pid, hostname },
serializers: Object.assign(Object.create(null), {
err: defaultErrorSerializer
err: defaultErrorSerializer,
req: defaultRequestSerializer,
res: defaultResponseSerializer
}),
formatters: Object.assign(Object.create(null), {
bindings (bindings) {
Expand Down Expand Up @@ -95,6 +103,8 @@ function pino (...args) {
timestamp,
messageKey,
errorKey,
requestKey,
responseKey,
nestedKey,
base,
name,
Expand Down Expand Up @@ -182,6 +192,8 @@ function pino (...args) {
[formatOptsSym]: formatOpts,
[messageKeySym]: messageKey,
[errorKeySym]: errorKey,
[requestKeySym]: requestKey,
[responseKeySym]: responseKey,
[nestedKeySym]: nestedKey,
// protect against injection
[nestedKeyStrSym]: nestedKey ? `,${JSON.stringify(nestedKey)}:{` : '',
Expand Down
75 changes: 75 additions & 0 deletions test/http.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,43 @@ test('http request support via serializer', async ({ ok, same, error, teardown }
server.close()
})

test('http request support via serializer (avoids stdSerializers)', async ({ error, equal, match }) => {
let originalReq
const instance = pino({
requestKey: 'myRequest',
serializers: {
req: (req) => {
equal(req, originalReq)
return req.arbitraryProperty
}
}
}, sink((chunk, _enc) => {
match(chunk, {
pid,
hostname,
level: 30,
msg: 'my request',
myRequest: originalReq.arbitraryProperty
})
}))

const server = http.createServer(function (req, res) {
originalReq = req
req.arbitraryProperty = Math.random()

instance.info(req, 'my request')
res.end('hello')
})
server.unref()
server.listen()
const err = await once(server, 'listening')
error(err)

const res = await once(http.get('http://localhost:' + server.address().port), 'response')
res.resume()
server.close()
})

// skipped because request connection is deprecated since v13, and request socket is always available
skip('http request support via serializer without request connection', async ({ ok, same, error, teardown }) => {
let originalReq
Expand Down Expand Up @@ -200,6 +237,44 @@ test('http response support via a serializer', async ({ ok, same, error, teardow
server.close()
})

test('http response support via serializer (avoids stdSerializers)', async ({ match, equal, error }) => {
let originalRes
const instance = pino({
responseKey: 'myResponse',
serializers: {
res: (res) => {
equal(res, originalRes)
return res.arbitraryProperty
}
}
}, sink((chunk, _enc) => {
match(chunk, {
pid,
hostname,
level: 30,
msg: 'my response',
myResponse: originalRes.arbitraryProperty
})
}))

const server = http.createServer(function (_req, res) {
originalRes = res
res.arbitraryProperty = Math.random()

instance.info(res, 'my response')
res.end('hello')
})

server.unref()
server.listen()
const err = await once(server, 'listening')
error(err)

const res = await once(http.get('http://localhost:' + server.address().port), 'response')
res.resume()
server.close()
})

test('http request support via serializer in a child', async ({ ok, same, error, teardown }) => {
let originalReq
const instance = pino({
Expand Down
8 changes: 6 additions & 2 deletions test/serializers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ test('child does not overwrite parent serializers', async ({ equal }) => {
test('Symbol.for(\'pino.serializers\')', async ({ equal, same, not }) => {
const stream = sink()
const expected = Object.assign({
err: stdSerializers.err
err: stdSerializers.err,
req: stdSerializers.req,
res: stdSerializers.res
}, parentSerializers)
const parent = pino({ serializers: parentSerializers }, stream)
const child = parent.child({ a: 'property' })
Expand Down Expand Up @@ -158,7 +160,9 @@ test('children inherit parent Symbol serializers', async ({ equal, same, not })
[Symbol.for('b')]: b
}
const expected = Object.assign({
err: stdSerializers.err
err: stdSerializers.err,
req: stdSerializers.req,
res: stdSerializers.res
}, symbolSerializers)
const parent = pino({ serializers: symbolSerializers }, stream)

Expand Down