fix(NODE-3356): update redaction logic for command monitoring events (#…

…2847)
mongodb · Jun 16, 2021 · 8c8b4c3 · 8c8b4c3
1 parent 2c5d440
commit 8c8b4c3
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 22 deletions.
diff --git a/lib/core/connection/apm.js b/lib/core/connection/apm.js
@@ -17,14 +17,20 @@ const SENSITIVE_COMMANDS = new Set([
   'copydb'
 ]);
 
+const HELLO_COMMANDS = new Set(['hello', 'ismaster', 'isMaster']);
+
 // helper methods
 const extractCommandName = commandDoc => Object.keys(commandDoc)[0];
 const namespace = command => command.ns;
 const databaseName = command => command.ns.split('.')[0];
 const collectionName = command => command.ns.split('.')[1];
 const generateConnectionId = pool =>
   pool.options ? `${pool.options.host}:${pool.options.port}` : pool.address;
-const maybeRedact = (commandName, result) => (SENSITIVE_COMMANDS.has(commandName) ? {} : result);
+const maybeRedact = (commandName, cmd, result) =>
+  SENSITIVE_COMMANDS.has(commandName) ||
+  (HELLO_COMMANDS.has(commandName) && cmd.speculativeAuthenticate)
+    ? {}
+    : result;
 const isLegacyPool = pool => pool.s && pool.queue;
 
 const LEGACY_FIND_QUERY_MAP = {
@@ -181,17 +187,11 @@ class CommandStartedEvent {
     const commandName = extractCommandName(cmd);
     const connectionDetails = extractConnectionDetails(pool);
 
-    // NOTE: remove in major revision, this is not spec behavior
-    if (SENSITIVE_COMMANDS.has(commandName)) {
-      this.commandObj = {};
-      this.commandObj[commandName] = true;
-    }
-
     Object.assign(this, connectionDetails, {
       requestId: command.requestId,
       databaseName: databaseName(command),
       commandName,
-      command: cmd
+      command: maybeRedact(commandName, cmd, cmd)
     });
   }
 }
@@ -215,7 +215,7 @@ class CommandSucceededEvent {
       requestId: command.requestId,
       commandName,
       duration: calculateDurationInMs(started),
-      reply: maybeRedact(commandName, extractReply(command, reply))
+      reply: maybeRedact(commandName, cmd, extractReply(command, reply))
     });
   }
 }
@@ -239,7 +239,7 @@ class CommandFailedEvent {
       requestId: command.requestId,
       commandName,
       duration: calculateDurationInMs(started),
-      failure: maybeRedact(commandName, error)
+      failure: maybeRedact(commandName, cmd, error)
     });
   }
 }

diff --git a/test/functional/apm.test.js b/test/functional/apm.test.js
@@ -622,7 +622,7 @@ describe('APM', function() {
           expect(started).to.have.length(1);
           expect(succeeded).to.have.length(1);
           expect(failed).to.have.length(0);
-          expect(started[0].commandObj).to.eql({ getnonce: true });
+          expect(started[0].command).to.eql({});
           expect(succeeded[0].reply).to.eql({});
           return client.close();
         });
@@ -1129,22 +1129,12 @@ describe('APM', function() {
   describe('command monitoring unified spec tests', () => {
     for (const loadedSpec of loadSpecTests('command-monitoring/unified')) {
       expect(loadedSpec).to.include.all.keys(['description', 'tests']);
-      // TODO: NODE-3356 unskip redaction tests
-      const testsToSkip =
-        loadedSpec.description === 'redacted-commands'
-          ? loadedSpec.tests
-              .map(test => test.description)
-              .filter(
-                description =>
-                  description !== 'hello without speculative authenticate is not redacted'
-              )
-          : [];
       context(String(loadedSpec.description), function() {
         for (const test of loadedSpec.tests) {
           it(String(test.description), {
             metadata: { sessions: { skipLeakTests: true } },
             test() {
-              return runUnifiedTest(this, loadedSpec, test, testsToSkip);
+              return runUnifiedTest(this, loadedSpec, test);
             }
           });
         }