From 9b4e3db9cbb2866086f004cbf0e15d90819248cd Mon Sep 17 00:00:00 2001 From: David Brownman Date: Tue, 28 May 2019 22:38:50 -0400 Subject: [PATCH 1/4] Be more defensive when creating a buffer --- src/tools/create-logger.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/tools/create-logger.js b/src/tools/create-logger.js index 595980f..804feb4 100644 --- a/src/tools/create-logger.js +++ b/src/tools/create-logger.js @@ -111,7 +111,15 @@ const makeSensitiveBank = (event, data) => { const censored = hashing.snipify(val); bank[val] = censored; bank[encodeURIComponent(val)] = censored; - bank[Buffer.from(val).toString('base64')] = censored; + try { + bank[Buffer.from(val).toString('base64')] = censored; + } catch (e) { + if (e.name !== 'TypeError') { + throw e; + } + // ignore; Buffer is semi-selective about what types it takes + // not sure why we get non-strings here, but we do occasionally. + } } return bank; }, From b7b3df830c0de60e6b036e628fc0dcfe9e90e4af Mon Sep 17 00:00:00 2001 From: David Brownman Date: Fri, 31 May 2019 18:10:14 -0400 Subject: [PATCH 2/4] wip testing --- package.json | 4 ++-- src/tools/cleaner.js | 23 ++++++++++++++-------- src/tools/create-logger.js | 21 ++++++++++---------- src/tools/hashing.js | 11 ++++++----- test/create-request-client.js | 37 +++++++++++++++++++++++++++++++++++ test/logger.js | 2 +- 6 files changed, 71 insertions(+), 27 deletions(-) diff --git a/package.json b/package.json index b3a58d2..cdc7b60 100644 --- a/package.json +++ b/package.json @@ -18,9 +18,9 @@ "preversion": "git pull && npm test", "version": "node bin/bump-dependencies.js && npm install && git add package.json package-lock.json", "postversion": "git push && git push --tags", - "test": "mocha -t 10000 --recursive test", + "test": "mocha -t 10000 --inspect-brk --recursive test", "test:w": "mocha -t 10000 --recursive test --watch", - "posttest": "npm run lint", + "/posttest": "npm run lint", "plain-test": "mocha -t 5000 --recursive test", "integration-test": "mocha -t 10000 integration-test", "local-integration-test": "mocha -t 10000 integration-test --local", diff --git a/src/tools/cleaner.js b/src/tools/cleaner.js index 70126bf..93c538a 100644 --- a/src/tools/cleaner.js +++ b/src/tools/cleaner.js @@ -43,27 +43,34 @@ const recurseCleanFuncs = (obj, path) => { // Recurse a nested object replace all instances of keys->vals in the bank. const recurseReplaceBank = (obj, bank = {}) => { + console.log('build blank'); const replacer = out => { - if (typeof out === 'number') { - out = String(out); + if (out === 314159265) { + console.log('replacing'); } - if (typeof out !== 'string') { + if (!['string', 'number'].includes(typeof out)) { return out; } + if (out === 314159265) { + debugger; // eslint-disable-line no-debugger + console.log('stayed'); + } + let str = String(out); + Object.keys(bank).forEach(key => { // Escape characters (ex. {{foo}} => \\{\\{foo\\}\\} ) const escapedKey = key.replace(/[-[\]/{}()\\*+?.^$|]/g, '\\$&'); const matchesKey = new RegExp(escapedKey, 'g'); - if (!matchesKey.test(out)) { + if (!matchesKey.test(str)) { return; } const matchesCurlies = /({{.*?}})/; - const valueParts = out.split(matchesCurlies).filter(Boolean); + const valueParts = str.split(matchesCurlies).filter(Boolean); const replacementValue = bank[key]; - const isPartOfString = !matchesCurlies.test(out) || valueParts.length > 1; + const isPartOfString = !matchesCurlies.test(str) || valueParts.length > 1; const shouldThrowTypeError = isPartOfString && (Array.isArray(replacementValue) || _.isPlainObject(replacementValue)); @@ -76,12 +83,12 @@ const recurseReplaceBank = (obj, bank = {}) => { ); } - out = isPartOfString + str = isPartOfString ? valueParts.join('').replace(matchesKey, replacementValue) : replacementValue; }); - return out; + return str; }; return recurseReplace(obj, replacer); }; diff --git a/src/tools/create-logger.js b/src/tools/create-logger.js index d0b1000..9f5034a 100644 --- a/src/tools/create-logger.js +++ b/src/tools/create-logger.js @@ -98,7 +98,7 @@ const makeSensitiveBank = (event, data) => { return false; }; - dataTools.recurseExtract(data, matcher).map(value => { + dataTools.recurseExtract(data, matcher).forEach(value => { sensitiveValues.push(value); }); @@ -108,19 +108,18 @@ const makeSensitiveBank = (event, data) => { // keeps short values from spamming censor strings in logs, < 6 chars is not a proper secret // see https://github.com/zapier/zapier-platform-core/issues/4#issuecomment-277855071 if (val && String(val).length > 5) { - val = String(val); const censored = hashing.snipify(val); bank[val] = censored; bank[encodeURIComponent(val)] = censored; - try { - bank[Buffer.from(val).toString('base64')] = censored; - } catch (e) { - if (e.name !== 'TypeError') { - throw e; - } - // ignore; Buffer is semi-selective about what types it takes - // not sure why we get non-strings here, but we do occasionally. - } + // try { + bank[Buffer.from(String(val)).toString('base64')] = censored; + // } catch (e) { + // if (e.name !== 'TypeError') { + // throw e; + // } + // // ignore; Buffer is semi-selective about what types it takes + // // not sure why we get non-strings here, but we do occasionally. + // } } return bank; }, diff --git a/src/tools/hashing.js b/src/tools/hashing.js index debd9ee..d73546e 100644 --- a/src/tools/hashing.js +++ b/src/tools/hashing.js @@ -13,13 +13,14 @@ const hashify = (algo, s, encoding, input_encoding) => { // Clean up sensitive values in a hashed manner so they don't get logged. const snipify = s => { - if (typeof s !== 'string') { + if (!['string', 'number'].includes(typeof s)) { return null; } - const length = s.length; - s += process.env.SECRET_SALT || 'doesntmatterreally'; - const result = hashify('sha256', s); - return `:censored:${length}:${result.substr(0, 10)}:`; + const str = String(s); + const length = str.length; + const salted = str + (process.env.SECRET_SALT || 'doesntmatterreally'); + const hashed = hashify('sha256', salted); + return `:censored:${length}:${hashed.substr(0, 10)}:`; }; const md5 = s => diff --git a/test/create-request-client.js b/test/create-request-client.js index 2b2edc5..d22ffad 100644 --- a/test/create-request-client.js +++ b/test/create-request-client.js @@ -583,6 +583,43 @@ describe('request client', () => { }); }); + it.only('should keep valid data types that are hard-coded', () => { + // This may seem like an usual case to be in, and for most apps it is. + // However, converted apps that rely on legacy-scripting-runner can have + // request bodies that are pure data, no {{}}, so we need to be sure to preserve those to + const event = { + bundle: { + inputData: { + number: 123, + bool: true, + float: 123.456, + arr: [1, 2, 3], + nested: { very: 'cool' } + } + } + }; + const bodyInput = createInput({}, event, testLogger); + const request = createAppRequestClient(bodyInput); + return request({ + url: 'https://httpbin.org/post', + method: 'POST', + body: { + number: 123, + bool: true, + float: 123.456, + arr: [1, 2, 3] + } + }).then(response => { + const { json } = response.json; + + should(json.empty).eql(undefined); + json.number.should.eql(123); + json.bool.should.eql(true); + json.float.should.eql(123.456); + json.arr.should.eql([1, 2, 3]); + }); + }); + it('should remove keys from body for empty values if configured to', () => { const event = { bundle: { diff --git a/test/logger.js b/test/logger.js index 663dc72..8b8e510 100644 --- a/test/logger.js +++ b/test/logger.js @@ -245,7 +245,7 @@ describe('logger', () => { }); }); - it('should replace sensitive data that is not a string', () => { + it.only('should replace sensitive data that is not a string', () => { const bundle = { authData: { numerical_token: 314159265 From 2cb329e10e67922c48693fa8935c0cd4989c4c63 Mon Sep 17 00:00:00 2001 From: David Brownman Date: Mon, 3 Jun 2019 18:05:41 -0400 Subject: [PATCH 3/4] fix failing test --- package.json | 3 ++- src/tools/cleaner.js | 31 +++++++++++++++++-------------- test/create-request-client.js | 2 +- test/logger.js | 2 +- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/package.json b/package.json index cdc7b60..421a105 100644 --- a/package.json +++ b/package.json @@ -18,7 +18,8 @@ "preversion": "git pull && npm test", "version": "node bin/bump-dependencies.js && npm install && git add package.json package-lock.json", "postversion": "git push && git push --tags", - "test": "mocha -t 10000 --inspect-brk --recursive test", + "test": "mocha -t 10000 --recursive test", + "debug": "mocha -t 10000 --inspect-brk --recursive test", "test:w": "mocha -t 10000 --recursive test --watch", "/posttest": "npm run lint", "plain-test": "mocha -t 5000 --recursive test", diff --git a/src/tools/cleaner.js b/src/tools/cleaner.js index 93c538a..e3f288a 100644 --- a/src/tools/cleaner.js +++ b/src/tools/cleaner.js @@ -43,34 +43,33 @@ const recurseCleanFuncs = (obj, path) => { // Recurse a nested object replace all instances of keys->vals in the bank. const recurseReplaceBank = (obj, bank = {}) => { - console.log('build blank'); const replacer = out => { - if (out === 314159265) { - console.log('replacing'); - } if (!['string', 'number'].includes(typeof out)) { return out; } - if (out === 314159265) { - debugger; // eslint-disable-line no-debugger - console.log('stayed'); - } - let str = String(out); + // whatever leaves this function replaces values in the calling object + // so, we don't want to return a different data type unless it's a censored string + const originalValue = out; + const originalValueStr = String(out); + let maybeChangedString = originalValueStr; Object.keys(bank).forEach(key => { // Escape characters (ex. {{foo}} => \\{\\{foo\\}\\} ) const escapedKey = key.replace(/[-[\]/{}()\\*+?.^$|]/g, '\\$&'); const matchesKey = new RegExp(escapedKey, 'g'); - if (!matchesKey.test(str)) { + if (!matchesKey.test(maybeChangedString)) { return; } const matchesCurlies = /({{.*?}})/; - const valueParts = str.split(matchesCurlies).filter(Boolean); + const valueParts = maybeChangedString + .split(matchesCurlies) + .filter(Boolean); const replacementValue = bank[key]; - const isPartOfString = !matchesCurlies.test(str) || valueParts.length > 1; + const isPartOfString = + !matchesCurlies.test(maybeChangedString) || valueParts.length > 1; const shouldThrowTypeError = isPartOfString && (Array.isArray(replacementValue) || _.isPlainObject(replacementValue)); @@ -83,12 +82,16 @@ const recurseReplaceBank = (obj, bank = {}) => { ); } - str = isPartOfString + maybeChangedString = isPartOfString ? valueParts.join('').replace(matchesKey, replacementValue) : replacementValue; }); - return str; + if (originalValueStr === maybeChangedString) { + // we didn't censor or replace the value, so return the original + return originalValue; + } + return maybeChangedString; }; return recurseReplace(obj, replacer); }; diff --git a/test/create-request-client.js b/test/create-request-client.js index d22ffad..72dd8b9 100644 --- a/test/create-request-client.js +++ b/test/create-request-client.js @@ -583,7 +583,7 @@ describe('request client', () => { }); }); - it.only('should keep valid data types that are hard-coded', () => { + it('should keep valid data types that are hard-coded', () => { // This may seem like an usual case to be in, and for most apps it is. // However, converted apps that rely on legacy-scripting-runner can have // request bodies that are pure data, no {{}}, so we need to be sure to preserve those to diff --git a/test/logger.js b/test/logger.js index 8b8e510..663dc72 100644 --- a/test/logger.js +++ b/test/logger.js @@ -245,7 +245,7 @@ describe('logger', () => { }); }); - it.only('should replace sensitive data that is not a string', () => { + it('should replace sensitive data that is not a string', () => { const bundle = { authData: { numerical_token: 314159265 From 91c730a0b27ce79f32645a88eb6a0aad276c6639 Mon Sep 17 00:00:00 2001 From: David Brownman Date: Mon, 3 Jun 2019 21:30:57 -0400 Subject: [PATCH 4/4] add failing test for nested object --- package.json | 2 +- src/tools/create-logger.js | 17 ++++++++--------- test/logger.js | 39 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/package.json b/package.json index 421a105..97f616e 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "test": "mocha -t 10000 --recursive test", "debug": "mocha -t 10000 --inspect-brk --recursive test", "test:w": "mocha -t 10000 --recursive test --watch", - "/posttest": "npm run lint", + "posttest": "npm run lint", "plain-test": "mocha -t 5000 --recursive test", "integration-test": "mocha -t 10000 integration-test", "local-integration-test": "mocha -t 10000 integration-test --local", diff --git a/src/tools/create-logger.js b/src/tools/create-logger.js index 9f5034a..2bcbfe0 100644 --- a/src/tools/create-logger.js +++ b/src/tools/create-logger.js @@ -111,15 +111,14 @@ const makeSensitiveBank = (event, data) => { const censored = hashing.snipify(val); bank[val] = censored; bank[encodeURIComponent(val)] = censored; - // try { - bank[Buffer.from(String(val)).toString('base64')] = censored; - // } catch (e) { - // if (e.name !== 'TypeError') { - // throw e; - // } - // // ignore; Buffer is semi-selective about what types it takes - // // not sure why we get non-strings here, but we do occasionally. - // } + try { + bank[Buffer.from(String(val)).toString('base64')] = censored; + } catch (e) { + if (e.name !== 'TypeError') { + throw e; + } + // ignore; Buffer is semi-selective about what types it takes + } } return bank; }, diff --git a/test/logger.js b/test/logger.js index 663dc72..6c29ab0 100644 --- a/test/logger.js +++ b/test/logger.js @@ -280,6 +280,45 @@ describe('logger', () => { }); }); + // this test fails because the function that creates the sensitive bank doesn't + // recurse to find all sensitive values + it.skip('should replace sensitive data that nested', () => { + const bundle = { + authData: { + nested: { secret: 8675309 } + } + }; + const logger = createlogger({ bundle }, options); + + const data = { + response_json: { + nested: { secret: 8675309 } + }, + response_content: `{ + nested: { secret: 8675309 } + }` + }; + + return logger('test', data).then(response => { + response.status.should.eql(200); + response.content.json.should.eql({ + token: options.token, + message: 'test', + data: { + response_json: { + nested: { + secret: ':censored:9:9cb84e8ccc:' + } + }, + response_content: `{ + nested: { secret: :censored:9:9cb84e8ccc: } + }`, + log_type: 'console' + } + }); + }); + }); + it('should not replace safe log keys', () => { const bundle = { authData: {