censor novel secrets in querystring (#526)

zapier · Apr 8, 2022 · 5e1b05c · 5e1b05c
1 parent f4880b9
commit 5e1b05c
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 14 deletions.
diff --git a/packages/core/src/tools/create-logger.js b/packages/core/src/tools/create-logger.js
@@ -1,6 +1,7 @@
 'use strict';
 
 const { Transform } = require('stream');
+const { parse: querystringParse } = require('querystring');
 
 const _ = require('lodash');
 
@@ -18,7 +19,10 @@ const {
   recurseExtract,
 } = require('@zapier/secret-scrubber');
 // not really a public function, but it came from here originally
-const { isUrlWithSecrets } = require('@zapier/secret-scrubber/lib/convenience');
+const {
+  isUrlWithSecrets,
+  isSensitiveKey,
+} = require('@zapier/secret-scrubber/lib/convenience');
 
 // The payload size per request to stream logs. This should be slighly lower
 // than the limit (16 MB) on the server side.
@@ -124,7 +128,13 @@ const buildSensitiveValues = (event, data) => {
   const authData = bundle.authData || {};
   // for the most part, we should censor all the values from authData
   // the exception is safe urls, which should be filtered out - we want those to be logged
+  // but, we _should_ censor-no-matter-what sensitive keys, even if their value is a safe url
+  // this covers the case where someone's password is a valid url ¯\_(ツ)_/¯
   const sensitiveAuthData = recurseExtract(authData, (key, value) => {
+    if (isSensitiveKey(key)) {
+      return true;
+    }
+
     if (isUrl(value) && !isUrlWithSecrets(value)) {
       return false;
     }
@@ -145,7 +155,9 @@ const buildSensitiveValues = (event, data) => {
       result.push(...attemptFindSecretsInStr(data[prop]));
     }
   }
-
+  if (data.request_params) {
+    result.push(...findSensitiveValues(querystringParse(data.request_params)));
+  }
   // unique- no point in duplicates
   return [...new Set(result)];
 };

diff --git a/packages/core/test/logger.js b/packages/core/test/logger.js
@@ -14,22 +14,29 @@ const {
 } = require('../src/http-middlewares/after/log-response');
 
 // little helper to prepare a req/res pair like the http logger does
-const prepareTestRequest = (reqBody = {}, resBody = {}) =>
+const prepareTestRequest = ({
+  reqBody = {},
+  resBody = {},
+  reqQueryParams = '',
+} = {}) =>
   prepareRequestLog(
     {
-      url: 'http://example.com',
+      url: `http://example.com${
+        reqQueryParams ? '?' + querystring.stringify(reqQueryParams) : ''
+      }`,
       method: 'POST',
       headers: {
         accept: 'application/json',
       },
+      // we usually stringify this in prepare-request.coerceBody, so mirror that behavior here
       body: JSON.stringify(reqBody),
     },
     {
       status: 200,
       headers: {
         'content-type': 'application/json',
       },
-      // we stringify this in prepare-request.coerceBody
+
       content: resBody,
     }
   );
@@ -250,17 +257,18 @@ describe('logger', () => {
     };
     const logger = createlogger({ bundle }, options);
 
-    const { message, data } = prepareTestRequest(
-      {
+    const { message, data } = prepareTestRequest({
+      reqBody: {
         // value appears only here; logger needs to parse this out of a string to censor it properly
         access_token: 'super_secret',
         refresh_token: bundle.authData.refresh_token,
       },
-      {
+      resBody: {
         // same here
         access_token: 'some new token',
-      }
-    );
+      },
+      reqQueryParams: { api_key: 'secret-key' },
+    });
 
     logger(message, data);
     const response = await logger.end();
@@ -273,6 +281,7 @@ describe('logger', () => {
           log_type: 'http',
           request_type: 'devplatform-outbound',
           request_url: 'http://example.com',
+          request_params: 'api_key=:censored:10:ad15d65bcc:',
           request_method: 'POST',
           request_headers: 'accept: application/json',
           request_data:
@@ -296,7 +305,9 @@ describe('logger', () => {
     const logger = createlogger({ bundle }, options);
 
     const { message, data } = prepareTestRequest({
-      refresh_token: bundle.authData.refresh_token,
+      reqBody: {
+        refresh_token: bundle.authData.refresh_token,
+      },
     });
 
     logger(message, data);

diff --git a/yarn.lock b/yarn.lock
@@ -1590,9 +1590,9 @@
   integrity sha512-sL/cEvJWAnClXw0wHk85/2L0G6Sj8UB0Ctc1TEMbKSsmpRosqhwj9gWgFRZSrBr2f9tiXISwNhCPmlfqUqyb9Q==
 
 "@zapier/secret-scrubber@^1.0.3":
-  version "1.0.3"
-  resolved "https://registry.yarnpkg.com/@zapier/secret-scrubber/-/secret-scrubber-1.0.3.tgz#7b272ec82afdfe501d69d1c6dc046c13597137b1"
-  integrity sha512-RmQ3c8gh0LMsWWPDyDytn0r4xQJPhfijtd3KP07g7cUuNGLjzYDiuvZU6H21XsGSdcKu90ntcuKq1lifVH2UDg==
+  version "1.0.6"
+  resolved "https://registry.yarnpkg.com/@zapier/secret-scrubber/-/secret-scrubber-1.0.6.tgz#d10fa24c3bea1cbf307997337af4d6bc353230fa"
+  integrity sha512-oaqIMiQ9U0UUd+24UXsx1a+xKmmP5q6SzqwOWpZ51lCS9McvsdEmwCAC6oLf7Q2PApER6At1kbKp1RQ9M5oXTQ==
   dependencies:
     lodash.isplainobject "^4.0.6"