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

Bug: About maskStrings parameter #40

Closed
pegasuspect opened this issue Aug 10, 2020 · 3 comments
Closed

Bug: About maskStrings parameter #40

pegasuspect opened this issue Aug 10, 2020 · 3 comments
Labels
bug Something isn't working

Comments

@pegasuspect
Copy link

Describe the bug
When maskStrings parameter is used tslog is masking the keys as well and also adding an extra colon. Also the maskStrings works case sensitive unlike maskValuesOfKeys. See the screenshot below.

To Reproduce
Steps to reproduce the behavior:

  1. mkdir tslogBugMaskStrings
  2. cd tslogBugMaskStrings
  3. npm init -y
  4. npm i tslog
  5. Create a js file called index.js, touch index.js perhaps
  6. run the following code:
const { Logger } = require('tslog');
let log = new Logger({
  maskStrings: ['password'],
  maskValuesOfKeys: ['password', 'password_hash']
});

log.info({ 
  password: 'password',
  password_hash: 'hush-hush-hush',
  paSSword_column: "pass Pass password."
});

Expected behavior
There should be one colon after the key and if it is not intended, keys should not be masked.

Screenshots

Additional context
I added this as a separate issue to keep things cleaner with issue #37

@pegasuspect pegasuspect added the bug Something isn't working label Aug 10, 2020
@terehov
Copy link
Contributor

terehov commented Aug 10, 2020

Hi @pegasuspect,

thanks again and I appreciate a new issue.
Double colon is obviously a mistake I added yesterday, however, masking of "all" strings is by design.
My thought was: A password should not be a "password" but rather a random string (e.g. kff538dd.hgdKJGhfn) and if you tell tslog to mask it, it should be masked everywhere no matter where it appears.

I would appreciate to hear you thoughts on this.

@pegasuspect
Copy link
Author

I think its OK to maskStrings either way, the only thing is it must be made obvious in the documentation what it does. Like you explained you might add: It will also mask keys if it encounters this random string.

The naming of the parameter could also help in understanding it. maskStrings is a little general name. It might be maskAllOccurances, or if you wanna keep it short maskAny. Because when you say strings to me it means literal json string type, as if its not gonna work with number. To somebody else it might mean different as well.

terehov added a commit that referenced this issue Aug 10, 2020
Fix #40, Rename maskStrings into maskAny
@terehov
Copy link
Contributor

terehov commented Aug 10, 2020

Thank you @pegasuspect for your valuable input. I also believe that this name is misleading and have therefore decided to rename it and also clarify in the documentation.

I have just released a new next version. You can give it a spin and tomorrow I'll release as a new minor

npm i tslog@next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants