From 8505715dc2f251e54e5dc3dbd574945c1d35617c Mon Sep 17 00:00:00 2001 From: Arthur Cinader Date: Wed, 21 Sep 2016 15:15:58 -0700 Subject: [PATCH] Move password masking out of logging clients where possible 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 --- spec/WinstonLoggerAdapter.spec.js | 16 --------- src/Controllers/LoggerController.js | 51 ++++++++++++++++++++++++----- src/PromiseRouter.js | 24 ++------------ src/Routers/FunctionsRouter.js | 4 +-- src/triggers.js | 8 ++--- 5 files changed, 50 insertions(+), 53 deletions(-) diff --git a/spec/WinstonLoggerAdapter.spec.js b/spec/WinstonLoggerAdapter.spec.js index b739c0270f..9393776610 100644 --- a/spec/WinstonLoggerAdapter.spec.js +++ b/spec/WinstonLoggerAdapter.spec.js @@ -97,20 +97,4 @@ describe('verbose logs', () => { done(); }) }); - - it("should not mask information in non _User class", (done) => { - let obj = new Parse.Object('users'); - obj.set('password', 'pw'); - obj.save().then(() => { - let winstonLoggerAdapter = new WinstonLoggerAdapter(); - return winstonLoggerAdapter.query({ - from: new Date(Date.now() - 500), - size: 100, - level: 'verbose' - }); - }).then((results) => { - expect(results[1].body.password).toEqual("pw"); - done(); - }); - }); }); diff --git a/src/Controllers/LoggerController.js b/src/Controllers/LoggerController.js index 8904daa12e..bc4cdce131 100644 --- a/src/Controllers/LoggerController.js +++ b/src/Controllers/LoggerController.js @@ -2,6 +2,7 @@ import { Parse } from 'parse/node'; import PromiseRouter from '../PromiseRouter'; import AdaptableController from './AdaptableController'; import { LoggerAdapter } from '../Adapters/Logger/LoggerAdapter'; +import url from 'url'; const MILLISECONDS_IN_A_DAY = 24 * 60 * 60 * 1000; const LOG_STRING_TRUNCATE_LENGTH = 1000; @@ -19,8 +20,48 @@ export const LogOrder = { export class LoggerController extends AdaptableController { + maskSensitiveUrl(urlString) { + const password = url.parse(urlString, true).query.password; + + if (password) { + urlString = urlString.replace('password=' + password, 'password=********'); + } + return urlString; + } + + maskSensitive(argArray) { + return argArray.map(e => { + if (!e) { + return e; + } + + if (typeof e === 'string') { + return e.replace(/(password".?:.?")[^"]*"/g, '$1********"'); + } + // else it is an object... + + // check the url + if (e.url) { + e.url = this.maskSensitiveUrl(e.url); + } + + if (e.body) { + for (let key of Object.keys(e.body)) { + if (key === 'password') { + e.body[key] = '********'; + break; + } + } + } + + return e; + }); + } + log(level, args) { - args = [].concat(level, [...args]); + // make the passed in arguments object an array with the spread operator + args = this.maskSensitive([...args]); + args = [].concat(level, args); this.adapter.log.apply(this.adapter, args); } @@ -61,14 +102,6 @@ export class LoggerController extends AdaptableController { return null; } - cleanAndTruncateLogMessage(string) { - return this.truncateLogMessage(this.cleanLogMessage(string)); - } - - cleanLogMessage(string) { - return string.replace(/password":"[^"]*"/g, 'password":"********"'); - } - truncateLogMessage(string) { if (string && string.length > LOG_STRING_TRUNCATE_LENGTH) { const truncated = string.substring(0, LOG_STRING_TRUNCATE_LENGTH) + truncationMarker; diff --git a/src/PromiseRouter.js b/src/PromiseRouter.js index b69fd9c2f7..bd5cfc5d0e 100644 --- a/src/PromiseRouter.js +++ b/src/PromiseRouter.js @@ -144,7 +144,7 @@ function makeExpressHandler(appId, promiseHandler) { return function(req, res, next) { try { let url = maskSensitiveUrl(req); - let body = maskSensitiveBody(req); + let body = Object.assign({}, req.body); let stringifiedBody = JSON.stringify(body, null, 2); log.verbose(`REQUEST for [${req.method}] ${url}: ${stringifiedBody}`, { method: req.method, @@ -198,33 +198,13 @@ function makeExpressHandler(appId, promiseHandler) { } } -function maskSensitiveBody(req) { - let maskBody = Object.assign({}, req.body); - let shouldMaskBody = (req.method === 'POST' && req.originalUrl.endsWith('/users') - && !req.originalUrl.includes('classes')) || - (req.method === 'PUT' && /users\/\w+$/.test(req.originalUrl) - && !req.originalUrl.includes('classes')) || - (req.originalUrl.includes('classes/_User')); - if (shouldMaskBody) { - for (let key of Object.keys(maskBody)) { - if (key == 'password') { - maskBody[key] = '********'; - break; - } - } - } - return maskBody; -} function maskSensitiveUrl(req) { let maskUrl = req.originalUrl.toString(); let shouldMaskUrl = req.method === 'GET' && req.originalUrl.includes('/login') && !req.originalUrl.includes('classes'); if (shouldMaskUrl) { - let password = url.parse(req.originalUrl, true).query.password; - if (password) { - maskUrl = maskUrl.replace('password=' + password, 'password=********') - } + maskUrl = log.maskSensitiveUrl(maskUrl); } return maskUrl; } diff --git a/src/Routers/FunctionsRouter.js b/src/Routers/FunctionsRouter.js index 306d888260..3c208f6bdb 100644 --- a/src/Routers/FunctionsRouter.js +++ b/src/Routers/FunctionsRouter.js @@ -125,10 +125,10 @@ export class FunctionsRouter extends PromiseRouter { return new Promise(function (resolve, reject) { const userString = (req.auth && req.auth.user) ? req.auth.user.id : undefined; - const cleanInput = logger.cleanAndTruncateLogMessage(JSON.stringify(params)); + const cleanInput = logger.truncateLogMessage(JSON.stringify(params)); var response = FunctionsRouter.createResponseObject((result) => { try { - const cleanResult = logger.cleanAndTruncateLogMessage(JSON.stringify(result.response.result)); + const cleanResult = logger.truncateLogMessage(JSON.stringify(result.response.result)); logger.info(`Ran cloud function ${functionName} for user ${userString} ` + `with:\n Input: ${cleanInput }\n Result: ${cleanResult }`, { functionName, diff --git a/src/triggers.js b/src/triggers.js index 1a1119afdf..0bbce38228 100644 --- a/src/triggers.js +++ b/src/triggers.js @@ -212,7 +212,7 @@ function userIdForLog(auth) { } function logTriggerAfterHook(triggerType, className, input, auth) { - const cleanInput = logger.cleanAndTruncateLogMessage(JSON.stringify(input)); + const cleanInput = logger.truncateLogMessage(JSON.stringify(input)); logger.info(`${triggerType} triggered for ${className} for user ${userIdForLog(auth)}:\n Input: ${cleanInput}`, { className, triggerType, @@ -221,8 +221,8 @@ function logTriggerAfterHook(triggerType, className, input, auth) { } function logTriggerSuccessBeforeHook(triggerType, className, input, result, auth) { - const cleanInput = logger.cleanAndTruncateLogMessage(JSON.stringify(input)); - const cleanResult = logger.cleanAndTruncateLogMessage(JSON.stringify(result)); + const cleanInput = logger.truncateLogMessage(JSON.stringify(input)); + const cleanResult = logger.truncateLogMessage(JSON.stringify(result)); logger.info(`${triggerType} triggered for ${className} for user ${userIdForLog(auth)}:\n Input: ${cleanInput}\n Result: ${cleanResult}`, { className, triggerType, @@ -231,7 +231,7 @@ function logTriggerSuccessBeforeHook(triggerType, className, input, result, auth } function logTriggerErrorBeforeHook(triggerType, className, input, auth, error) { - const cleanInput = logger.cleanAndTruncateLogMessage(JSON.stringify(input)); + const cleanInput = logger.truncateLogMessage(JSON.stringify(input)); logger.error(`${triggerType} failed for ${className} for user ${userIdForLog(auth)}:\n Input: ${cleanInput}\n Error: ${JSON.stringify(error)}`, { className, triggerType,