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

Field scrubbing *mutates* logged objects! #509

Closed
serhalp opened this issue Nov 20, 2017 · 1 comment
Closed

Field scrubbing *mutates* logged objects! #509

serhalp opened this issue Nov 20, 2017 · 1 comment
Assignees

Comments

@serhalp
Copy link

serhalp commented Nov 20, 2017

The feature that scrubs fields detected to be "sensitive" – which suffers from false positives, is poorly documented, and seemingly cannot be disabled(?) – actually mutates these properties on the logged objects. This is a pretty egregious breach of expected conventional node module behavior. If this is intentional (I hope not...), it should be very clearly indicated in the documentation.

The combination of this mutation bug and the false positive bug made for a pretty nasty (though admittedly fascinating) bug, that I could easily see happening to others. We're using the cookie-session middleware, which populates the contents of a user cookie on req.session. We attach a part of this user context to our logger, which ships errors to Rollbar with that context. The cookie-session middleware's behavior is to set a set-cookie header on the response when req.session has been changed. These bugs led to, when an error is logged, a boolean property named tempWorker being mutated to "********", which sent an updated corrupted cookie down to the browser, which was persisted then sent back on subsequent requests, which led to authentication errors. Oof.

Looks like the mutation is in the traverse function and it does seem like the behavior was somewhat intentional...

I could open a PR if creating a deep clone of the object and returning a scrubbed copy in scrub() sounds good. Thanks!

@rokob
Copy link
Contributor

rokob commented Nov 20, 2017

Yikes, there are several issues here which you have brought up that should be resolved. Sorry about this. I am working on fixing these right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants