Skip to content

Obfuscate password name value pairs in log strings #2755

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

Merged

Conversation

flovilmart
Copy link
Contributor

@flovilmart flovilmart commented Sep 20, 2016

fixes: #2680

@facebook-github-bot
Copy link

@flovilmart updated the pull request - view changes

@acinader acinader force-pushed the password-obsfucate-cloud-triggers branch from 35b1549 to d106588 Compare September 20, 2016 23:58
@ghost
Copy link

ghost commented Sep 20, 2016

@flovilmart updated the pull request - view changes

@acinader acinader changed the title Unit test to catch password in logs. Obfuscate password name value pairs in log strings Sep 21, 2016
@acinader
Copy link
Contributor

acinader commented Sep 21, 2016

@flovilmart oh, now i see what you mean in #2680 (comment)

I think that what you're suggesting is trying something like:

export class LoggerController extends AdaptableController {

  log(level, args) {
    args = [].concat(level, [...args]);
+   args.forEach(e => this.cleanLogString(e))
    this.adapter.log.apply(this.adapter, args);
  }

and remove cleaning from the client's responsibility. I like it and will take a look, but can't get to tonight...will need to take a look at what comes into ...args :)

@flovilmart
Copy link
Contributor Author

No problem, there's no rush :)

@skinp
Copy link
Contributor

skinp commented Sep 21, 2016

LGTM

@flovilmart
Copy link
Contributor Author

Alright let's merge that so it's available on the latest branch, we'll refactor later

@flovilmart flovilmart merged commit ad70745 into parse-community:master Sep 21, 2016
acinader pushed a commit to acinader/parse-server that referenced this pull request Sep 21, 2016
Move password masking functionality into LoggerController.

The is a more aggresive approach to masking password string in the logs.

Cleaning the url is still in the PromiseRouter because picking it out of the log string
would be fragile.

This will cause more log messages to be scanned for password strings, and may cause a password
string to be obsfucated that is not neccesarily part of parse internals -- but i think that is
still a good thing....

see: parse-community#2755 & parse-community#2680
flovilmart pushed a commit that referenced this pull request Sep 22, 2016
Move password masking functionality into LoggerController.

The is a more aggresive approach to masking password string in the logs.

Cleaning the url is still in the PromiseRouter because picking it out of the log string
would be fragile.

This will cause more log messages to be scanned for password strings, and may cause a password
string to be obsfucated that is not neccesarily part of parse internals -- but i think that is
still a good thing....

see: #2755 & #2680
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

Successfully merging this pull request may close these issues.

Passwords are being logged in plain text in beforeSave
4 participants