Be more defensive when creating a buffer

package.json

@@ -19,6 +19,7 @@
     "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",
+    "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",

src/tools/cleaner.js

@@ -44,26 +44,32 @@ const recurseCleanFuncs = (obj, path) => {
 // Recurse a nested object replace all instances of keys->vals in the bank.
 const recurseReplaceBank = (obj, bank = {}) => {
   const replacer = out => {
-    if (typeof out === 'number') {
-      out = String(out);
-    }
-    if (typeof out !== 'string') {
+    if (!['string', 'number'].includes(typeof out)) {
       return 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(out)) {
+      if (!matchesKey.test(maybeChangedString)) {
         return;
       }
 
       const matchesCurlies = /({{.*?}})/;
-      const valueParts = out.split(matchesCurlies).filter(Boolean);
+      const valueParts = maybeChangedString
+        .split(matchesCurlies)
+        .filter(Boolean);
       const replacementValue = bank[key];
-      const isPartOfString = !matchesCurlies.test(out) || valueParts.length > 1;
+      const isPartOfString =
+        !matchesCurlies.test(maybeChangedString) || valueParts.length > 1;
       const shouldThrowTypeError =
         isPartOfString &&
         (Array.isArray(replacementValue) || _.isPlainObject(replacementValue));
@@ -76,12 +82,16 @@ const recurseReplaceBank = (obj, bank = {}) => {
         );
       }
 
-      out = isPartOfString
+      maybeChangedString = isPartOfString
         ? valueParts.join('').replace(matchesKey, replacementValue)
         : replacementValue;
     });
 
-    return out;
+    if (originalValueStr === maybeChangedString) {
+      // we didn't censor or replace the value, so return the original
+      return originalValue;
+    }
+    return maybeChangedString;
   };
   return recurseReplace(obj, replacer);
 };

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,11 +108,17 @@ 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;
-        bank[Buffer.from(val).toString('base64')] = 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
+        }
       }
       return bank;
     },

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 =>

test/create-request-client.js

@@ -583,6 +583,43 @@ describe('request client', () => {
       });
     });
 
+    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
+      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: {

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: {