feat(serializers): avoid implicit sanitization #2081

gerardolima · 2024-11-09T19:04:24Z

When serializers are defined for HTTP Request or HTTP Response, do not run the correspondent stdSerializers before calling the provided serializer functions.

ISSUE #1991

gerardolima · 2024-11-09T19:13:51Z

lib/proto.js

+  } else if (_obj instanceof IncomingMessage) {
+    obj = { [requestKey]: _obj }
+  } else if (_obj instanceof ServerResponse) {
+    obj = { [responseKey]: _obj }


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 }

I think property presence will be better here. These could be extended objects, e.g. a FastifyRequest.

gerardolima · 2024-11-09T19:18:51Z

lib/tools.js

+      } else if (key === requestKey && serializers.req) {
+        value = serializers.req(value)
+      } else if (key === responseKey && serializers.res) {
+        value = serializers.res(value)


Behaviour of all stdSerializers follow the same pattern.

gerardolima · 2024-11-09T19:23:26Z

pino.d.ts

+        /**
+         * 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;


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

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

mcollina · 2024-11-10T09:30:38Z

test/http.test.js

@@ -160,34 +154,31 @@ test('http response support', async ({ ok, same, error, teardown }) => {
  server.close()
 })

-test('http response support via a serializer', async ({ ok, same, error, teardown }) => {
+test('http response support via a serializer', async ({ match, error }) => {


can you please keep the previous test and only add a new one?

Original tests restored. Also fixed linter issues.

gerardolima · 2024-11-12T17:14:13Z

ok, I was running isolated tests and only now I noticed there were some assertions on the default serialisers; I am fixing them right now and soon the tests will be fixed; sorry for the silly mistake

gerardolima · 2024-11-22T13:38:10Z

hey, @mcollina, are these the changes you expected? is there something that needs improvement or still missing?

mcollina

lgtm

jsumners

The issue mentioned deprecating the current implementation and switching to the new one in the next major. I don't see this being implemented in this PR. Have I missed it?

jsumners · 2024-11-27T12:00:36Z

lib/proto.js

+  } else if (_obj instanceof IncomingMessage) {
+    obj = { [requestKey]: _obj }
+  } else if (_obj instanceof ServerResponse) {
+    obj = { [responseKey]: _obj }


I think property presence will be better here. These could be extended objects, e.g. a FastifyRequest.

gerardolima · 2024-11-27T20:44:17Z

The issue mentioned deprecating the current implementation and switching to the new one in the next major. I don't see this being implemented in this PR. Have I missed it?

This changeset only modifies the following behviour: neither logged Request nor Response objects are not transformed by their corresponding stdSerializers unconditionally, at the start of LOG (lib/tools.js).

This change in behaviour is asserted in test/http.test.js, lines 86 and 240, with tests that create an arbitrary property that should be removed from them, if their standard serializers ran.

jsumners · 2024-11-28T12:28:18Z

This changeset only modifies the following behviour: neither logged Request nor Response objects are not transformed by their corresponding stdSerializers unconditionally, at the start of LOG (lib/tools.js).

This change in behaviour is asserted in test/http.test.js, lines 86 and 240, with tests that create an arbitrary property that should be removed from them, if their standard serializers ran.

This is a breaking change, correct?

gerardolima · 2024-11-28T16:35:24Z

This is a breaking change, correct?

Yes, this is correct. When custom serializers are provided and they depend on the changes made by the standard serializers on the target objects (ie: to sanitize them?), they will be impacted.

Though running standard serializers when custom serializers are given is not exaclty obvious behaviour -- and I think nor documented --, I understand there might be code in the wild that dependes on it (Hyrum's Law) and publishing this as breaking change should be the safest strategy.

jsumners · 2024-11-28T20:10:54Z

Yes, this is correct. When custom serializers are provided and they depend on the changes made by the standard serializers on the target objects (ie: to sanitize them?), they will be impacted.

Then we are missing the deprecation and opt-in for the current version.

gerardolima · 2024-11-29T09:18:39Z

Then we are missing the deprecation and opt-in for the current version.

I'm sorry, but I'm not familiar with the versioning process in Pino -- I've already checked CONTRIBUTING.md. Would it suffice updating major version in package.json? Could you, please help me?

jsumners · 2024-11-29T13:33:43Z

Either @mcollina or I will handle incrementing the version when we publish. What the original discussion called for, and what is missing here, is the deprecation of the current behavior so that people are alerted to the change in default behavior in the next major release. See the following examples:

pino/lib/tools.js

Lines 416 to 423 in ba31fc1

    
           if ('changeLevelName' in opts) { 
        
             process.emitWarning( 
        
               'The changeLevelName option is deprecated and will be removed in v7. Use levelKey instead.', 
        
               { code: 'changeLevelName_deprecation' } 
        
             ) 
        
             opts.levelKey = opts.changeLevelName 
        
             delete opts.changeLevelName 
        
           }

pino/lib/tools.js

Lines 430 to 434 in ba31fc1

    
           if (prettyPrint) { 
        
             warning.emit('PINODEP008') 
        
             const prettyOpts = Object.assign({ messageKey }, prettyPrint) 
        
             stream = getPrettyStream(prettyOpts, prettifier, stream, instance) 
        
           }

We could introduce a PR that only adds the deprecation notice, issue a release with that, and then merge this one (with it removing the notice) and issue a new major. But we still haven't finished updating all of the org's maintained modules for v9. So I'm not sure we have much appetite for a new major so soon after v9 right now.

gerardolima · 2024-12-02T15:20:15Z

We could introduce a PR that only adds the deprecation notice, issue a release with that, and then merge this one (with it removing the notice) and issue a new major. But we still haven't finished updating all of the org's maintained modules for v9. So I'm not sure we have much appetite for a new major so soon after v9 right now.

hey, @jsumners, I'm sorry, but I still don't get it. No property has been deprecated nor renamed -- and I believe the explanation of the change in behaviour would be somewhat lengthy for a warning message. Also, I don't see any example of process.emitWarning nor warning. in the the actual codebase ... which should I use? which number should I use for PINODEPxxx?

jsumners · 2024-12-03T12:08:17Z

stdSerializers cannot be avoided for Request and Response #1991 (comment) suggested that the original functionality be deprecated. That means the current behavior does not change, but will generate a warning when the functionality is used.
This PR changes the published functionality, thus introducing a breaking change and necessitating a new major version to be published after it is merged.
We avoid needing to publish a new major version by deprecating the current functionality and providing an opt-in path to the functionality provided in this PR.
We utilize https://www.npmjs.com/package/process-warning to issue deprecation warnings. I provided links to historical code where we have issued deprecation warnings so that you can learn how to implement such a deprecation in this feature. A clearer example may be seen in 6c53815. The next available deprecation code is currently commented out in the deprecations source file (PINODEP010).

When serializers are defined for HTTP Request or HTTP Response, do not run the correspondent `stdSerializers` before calling the provided serializer functions. ISSUE pinojs#1991

gerardolima · 2024-12-09T17:19:09Z

hey, @jsumners and @mcollina, I created the opts property future to allow consumers to chose these breaking changes as opt-in, and avoid bumping the major version right now. This increased the size of the changeset, but I think it was a sensible way for consumers of Pino to chose the between the new and old behaviour when creating the top-level Pino object. Maybe this feature should be subject of a PR of its own, to be merged before these changes in the serialization of requests/responses?

gerardolima · 2024-12-09T17:22:06Z

test/http.test.js

@@ -83,6 +83,81 @@ test('http request support via serializer', async ({ ok, same, error, teardown }
  server.close()
 })

+test('http request support via serializer (avoids stdSerializers)', async ({ test }) => {


I noticed this library didn't have any nested test before. I think this makes it easier to identify and clean-up the tests that asserts the old behaviour when the new version will be released. If avoiding nested tests was intentional, these can be easily refactored.

gerardolima · 2024-12-09T17:24:37Z

lib/deprecations.js

+ * on a opt-in basis, anticipating behavior of the next major-version. All future flag must be frozen as false. These
+ * future flags are specific to Pino major-version 9.
+ */
+const future = Object.freeze({


I noticed Object.freeze is not used for default configuration. This can be easily refactored, if avoiding this feature is intentional.

gerardolima commented Nov 9, 2024

View reviewed changes

gerardolima force-pushed the override-http-std-serializers branch from f705b9c to a3b1681 Compare November 9, 2024 19:54

mcollina reviewed Nov 10, 2024

View reviewed changes

gerardolima requested a review from mcollina November 12, 2024 17:19

mcollina approved these changes Nov 22, 2024

View reviewed changes

mcollina requested a review from jsumners November 22, 2024 14:25

jsumners requested changes Nov 27, 2024

View reviewed changes

gerardolima force-pushed the override-http-std-serializers branch 2 times, most recently from 86e666f to 6cde4f5 Compare December 9, 2024 17:04

gerardolima requested review from jsumners and mcollina December 9, 2024 17:05

gerardolima added 8 commits December 9, 2024 17:06

feat(serializers): avoid implicit sanitization

1460394

When serializers are defined for HTTP Request or HTTP Response, do not run the correspondent `stdSerializers` before calling the provided serializer functions. ISSUE pinojs#1991

test(http): use alternative names to req and res

d84609d

test(http): restore original tests

b8b2b74

fix: linter issues

39b0697

tests: fix assertions on default serializers

7d47964

feat(serializers): use properties to infer types Request and Response

b393732

feat(future): add flags to anticipate breaking changes

ebd84c3

feat(serializers): wrap breaking changes in future

da9a16f

gerardolima force-pushed the override-http-std-serializers branch from 6cde4f5 to da9a16f Compare December 9, 2024 17:08

gerardolima commented Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(serializers): avoid implicit sanitization #2081

feat(serializers): avoid implicit sanitization #2081

gerardolima commented Nov 9, 2024

gerardolima Nov 9, 2024

jsumners Nov 27, 2024

gerardolima Nov 27, 2024

gerardolima Nov 9, 2024

gerardolima Nov 9, 2024

gerardolima Nov 11, 2024

mcollina Nov 10, 2024

gerardolima Nov 10, 2024

gerardolima commented Nov 12, 2024

gerardolima commented Nov 22, 2024

mcollina left a comment

jsumners left a comment

jsumners Nov 27, 2024

gerardolima commented Nov 27, 2024

jsumners commented Nov 28, 2024

gerardolima commented Nov 28, 2024

jsumners commented Nov 28, 2024

gerardolima commented Nov 29, 2024

jsumners commented Nov 29, 2024

gerardolima commented Dec 2, 2024

jsumners commented Dec 3, 2024

gerardolima commented Dec 9, 2024

gerardolima Dec 9, 2024

gerardolima Dec 9, 2024

feat(serializers): avoid implicit sanitization #2081

Are you sure you want to change the base?

feat(serializers): avoid implicit sanitization #2081

Conversation

gerardolima commented Nov 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerardolima commented Nov 12, 2024

gerardolima commented Nov 22, 2024

mcollina left a comment

Choose a reason for hiding this comment

jsumners left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerardolima commented Nov 27, 2024

jsumners commented Nov 28, 2024

gerardolima commented Nov 28, 2024

jsumners commented Nov 28, 2024

gerardolima commented Nov 29, 2024

jsumners commented Nov 29, 2024

gerardolima commented Dec 2, 2024

jsumners commented Dec 3, 2024

gerardolima commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment